-
Notifications
You must be signed in to change notification settings - Fork 554
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
doc: add design doc for RBD QoS #4089
Conversation
@travisn PTAL |
docs/design/proposals/rbd-qos.md
Outdated
@@ -0,0 +1,269 @@ | |||
# Design Doc for RBD QOS using cgroup v2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QoS is probably the more common abbreviation?
docs/design/proposals/rbd-qos.md
Outdated
cgroup.stat cpuset.cpus.effective io.pressure memory.pressure sys-kernel-config.mount | ||
``` | ||
|
||
`kubepods.slice`` is the starting point and it contains multiple hierarchy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one ` too many
docs/design/proposals/rbd-qos.md
Outdated
Now go back to the application pod and check if we have the right limit set | ||
|
||
```bash | ||
[$]oc exec: oc exec -it csi-rbd-demo-pod -- sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use kubectl
examples?
docs/design/proposals/rbd-qos.md
Outdated
|
||
| Parameter | Description | | ||
| --- | --- | | ||
| MaxReadIOOperationsPerSecond | Max read IO operations per second | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just use IOPS, that is common enough, IOOperationsPerSecond sounds very weird.
docs/design/proposals/rbd-qos.md
Outdated
|
||
## Different approaches | ||
|
||
The above solution can be implemented in CephCSI using 3 different approaches. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the name of the project is Ceph-CSI 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed it now :D
docs/design/proposals/rbd-qos.md
Outdated
|
||
Both options 2 and 3 are contingent upon changes to the CSI spec and Kubernetes | ||
support. Additionally, VolumeAttributeClass is currently being developed within | ||
the Kubernetes realm and will initially be in the Alpha stage. Consequently, it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add inline link to the KEP please
docs/design/proposals/rbd-qos.md
Outdated
|
||
#### Implementation of VolumeAttributeClass QOS | ||
|
||
1. Modify CSIDriver object to pass pod details to the NodePublish Volume RPC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe: ... NodePublishVolume
CSI procedure.
docs/design/proposals/rbd-qos.md
Outdated
#### Implementation of VolumeAttributeClass QOS | ||
|
||
1. Modify CSIDriver object to pass pod details to the NodePublish Volume RPC | ||
1. Add support in cephCSI to expose ModifyVolume RPC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cephCSI -> Ceph-CSI
docs/design/proposals/rbd-qos.md
Outdated
|
||
Considering the advantages and drawbacks, we can use StorageClass and | ||
VolumeAttributeClass to support QoS, with VolumeAttributeClass taking | ||
precedence over StorageClass. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider adding something like VolumeAttributeClass
to CSI-Addons? That would allow changing the parameters similar to ModifyVolume
already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nixpanic not as of today as kubernetes is working on it, if not we can look for adding it in future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've now checked the KEP and it seems to have noted Alpha status for Kubernetes 1.28. I am not sure implementing additional custom parameters in the StorageClass is the right approach if the feature lands in Kubernetes soon. Having the parameters in the StorageClass means maintaining support for at least 2 configuration directives, which adds work on the testing and maintenance.
Ideally we only need to implement and maintain the VolumeAttributeClass
CSI procedures that Kubernetes is implementing. A single standard mechanism benefits us as well as Ceph-CSI users. Putting efforts in getting the Kubernetes work done is important, and if the Ceph-CSI contributors can assists with that, all the better!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nixpanic this is deferred for Kubernetes 1.29 kubernetes/enhancements#3751 (comment) and am not sure it will happen (hope it will happen), and as it goes with Alpha, Beta Feature cycle it takes more time for users to use this in production and not everyone will agree to upgrade to latest kubernetes for this one. I think it better to have both solutions and IMO it is not a maintenance and testing burden for this one as its minimal one, later once VolumeAttributeClass is stable we can deprecate the support for storageclass Qos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nixpanic this is deferred for Kubernetes 1.29 kubernetes/enhancements#3751 (comment) and am not sure it will happen (hope it will happen), and as it goes with Alpha, Beta Feature cycle it takes more time for users to use this in production and not everyone will agree to upgrade to latest kubernetes for this one. I think it better to have both solutions and IMO it is not a maintenance and testing burden for this one as its minimal one, later once VolumeAttributeClass is stable we can deprecate the support for storageclass Qos.
+1
docs/design/proposals/rbd-qos.md
Outdated
|
||
### References | ||
|
||
* <https://kubernetes.io/docs/concepts/architecture/cgroups/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please link with a subject/description of what is at the URL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nixpanic any specific format you are looking at, provide a suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something like
* [Kubernetes cgroup v2 Architecture](https://kubernetes.io/docs/concepts/architecture/cgroups/)
544ebb1
to
6c3462a
Compare
docs/design/proposals/rbd-qos.md
Outdated
|
||
### References | ||
|
||
* <https://kubernetes.io/docs/concepts/architecture/cgroups/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something like
* [Kubernetes cgroup v2 Architecture](https://kubernetes.io/docs/concepts/architecture/cgroups/)
docs/design/proposals/rbd-qos.md
Outdated
|
||
Both options 2 and 3 are contingent upon changes to the CSI spec and Kubernetes | ||
support. Additionally, | ||
[VolumeAttributeClass](https://github.com/kubernetes/enhancements/pull/3780) is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a link to the actual doc is probably easier for people to read https://github.com/kubernetes/enhancements/blob/master/keps/sig-storage/3751-volume-attributes-class/README.md
docs/design/proposals/rbd-qos.md
Outdated
|
||
Considering the advantages and drawbacks, we can use StorageClass and | ||
VolumeAttributeClass to support QoS, with VolumeAttributeClass taking | ||
precedence over StorageClass. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've now checked the KEP and it seems to have noted Alpha status for Kubernetes 1.28. I am not sure implementing additional custom parameters in the StorageClass is the right approach if the feature lands in Kubernetes soon. Having the parameters in the StorageClass means maintaining support for at least 2 configuration directives, which adds work on the testing and maintenance.
Ideally we only need to implement and maintain the VolumeAttributeClass
CSI procedures that Kubernetes is implementing. A single standard mechanism benefits us as well as Ceph-CSI users. Putting efforts in getting the Kubernetes work done is important, and if the Ceph-CSI contributors can assists with that, all the better!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks !
Left a few comments to highlight options and small nits.
The design doc looks very complete and captures everything perfectly to me.
docs/design/proposals/rbd-qos.md
Outdated
|
||
The above solution can be implemented using 3 different approaches. | ||
|
||
### QoS using new parameters in RBD StorageClass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
### QoS using new parameters in RBD StorageClass | |
### 1. QoS using new parameters in RBD StorageClass |
docs/design/proposals/rbd-qos.md
Outdated
1. Need to take a backup and restore to New QoS StorageClass | ||
1. Delete and Recreate the PV object | ||
|
||
### Add support for VolumeAttributeClass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
### Add support for VolumeAttributeClass | |
### 2. QoS using parameters in VolumeAttributeClass |
docs/design/proposals/rbd-qos.md
Outdated
up to get the new QoS value even though its changed in the PVC object, this is | ||
sometime impossible as it will have downtime. | ||
|
||
### VolumeAttributeClass with NodePublish Secret |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
### VolumeAttributeClass with NodePublish Secret | |
### 3. QoS using parameters in VolumeAttributeClass with NodePublish Secret |
docs/design/proposals/rbd-qos.md
Outdated
in the Alpha stage. Consequently, it will be disabled by default, but it does | ||
offer the following advantages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the Alpha stage. Consequently, it will be disabled by default, but it does | |
offer the following advantages. | |
in the Alpha stage. Consequently, it will be disabled by default. | |
#### Advantages of QoS using VolumeAttributeClass |
Let's highlight by adding another heading here.
7da0a8c
to
7bdff7d
Compare
Pull request has been modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few nits, otherwise LGTM!
|
||
### Hybrid Approach | ||
|
||
Considering the advantages and drawbacks, we can use StorageClass and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for users to override the QoS in the storage class with the VolumeAttributeClass settings? This sounds good assuming the admin controls which VolumeAttributeClass is available for the user to specify in their PVC. Otherwise, the user could override admin settings in the storage class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both storageclass and VolumeAttributeClass are admin created ones and the user will not have access to create one, Yes users can update the VolumeAttributeClass in the PVC later creation but he/she should get the name of the VolumeAttributeClass from the admin.
@Mergifyio queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at 7c0ce53 |
Add design doc for QoS for rbd devices mapped with both krbd and rbd-nbd closes: ceph#521 Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
/test ci/centos/upgrade-tests-cephfs |
/test ci/centos/k8s-e2e-external-storage/1.26 |
/test ci/centos/upgrade-tests-rbd |
/test ci/centos/k8s-e2e-external-storage/1.28 |
/test ci/centos/mini-e2e-helm/k8s-1.26 |
/test ci/centos/mini-e2e-helm/k8s-1.28 |
/test ci/centos/mini-e2e/k8s-1.26 |
/test ci/centos/k8s-e2e-external-storage/1.27 |
/test ci/centos/mini-e2e/k8s-1.28 |
/test ci/centos/mini-e2e-helm/k8s-1.27 |
/test ci/centos/mini-e2e/k8s-1.27 |
Add design doc for QoS for rbd devices mapped with both krbd and rbd-nbd
closes: #521
Thanks to @idryomov for helping me to understand and experiment on this one 🎉