Skip to content
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

Merged
merged 1 commit into from
Sep 7, 2023
Merged

doc: add design doc for RBD QoS #4089

merged 1 commit into from
Sep 7, 2023

Conversation

Madhu-1
Copy link
Collaborator

@Madhu-1 Madhu-1 commented Aug 29, 2023

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 🎉

@Madhu-1 Madhu-1 requested review from idryomov and a team August 29, 2023 10:15
@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Aug 29, 2023

@travisn PTAL

@mergify mergify bot added ci/skip/e2e skip running e2e CI jobs ci/skip/multi-arch-build skip building on multiple architectures component/docs Issues and PRs related to documentation labels Aug 29, 2023
@@ -0,0 +1,269 @@
# Design Doc for RBD QOS using cgroup v2
Copy link
Member

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?

cgroup.stat cpuset.cpus.effective io.pressure memory.pressure sys-kernel-config.mount
```

`kubepods.slice`` is the starting point and it contains multiple hierarchy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one ` too many

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use kubectl examples?


| Parameter | Description |
| --- | --- |
| MaxReadIOOperationsPerSecond | Max read IO operations per second |
Copy link
Member

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.


## Different approaches

The above solution can be implemented in CephCSI using 3 different approaches.
Copy link
Member

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 😉

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed it now :D


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
Copy link
Member

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


#### Implementation of VolumeAttributeClass QOS

1. Modify CSIDriver object to pass pod details to the NodePublish Volume RPC
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe: ... NodePublishVolume CSI procedure.

#### 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cephCSI -> Ceph-CSI


Considering the advantages and drawbacks, we can use StorageClass and
VolumeAttributeClass to support QoS, with VolumeAttributeClass taking
precedence over StorageClass.
Copy link
Member

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.

Copy link
Collaborator Author

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

Copy link
Member

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!

Copy link
Collaborator Author

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.

Copy link
Contributor

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


### References

* <https://kubernetes.io/docs/concepts/architecture/cgroups/>
Copy link
Member

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

Copy link
Collaborator Author

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.

Copy link
Member

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/)

@Madhu-1 Madhu-1 requested a review from nixpanic August 29, 2023 12:36
@Madhu-1 Madhu-1 force-pushed the rbd-qos branch 2 times, most recently from 544ebb1 to 6c3462a Compare August 29, 2023 12:45
@Madhu-1 Madhu-1 changed the title doc: add design doc for RBD QOS doc: add design doc for RBD QoS Aug 29, 2023

### References

* <https://kubernetes.io/docs/concepts/architecture/cgroups/>
Copy link
Member

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/)


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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Considering the advantages and drawbacks, we can use StorageClass and
VolumeAttributeClass to support QoS, with VolumeAttributeClass taking
precedence over StorageClass.
Copy link
Member

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!

Copy link
Contributor

@Rakshith-R Rakshith-R left a 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.


The above solution can be implemented using 3 different approaches.

### QoS using new parameters in RBD StorageClass
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
### QoS using new parameters in RBD StorageClass
### 1. QoS using new parameters in RBD StorageClass

1. Need to take a backup and restore to New QoS StorageClass
1. Delete and Recreate the PV object

### Add support for VolumeAttributeClass
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
### Add support for VolumeAttributeClass
### 2. QoS using parameters in VolumeAttributeClass

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
### VolumeAttributeClass with NodePublish Secret
### 3. QoS using parameters in VolumeAttributeClass with NodePublish Secret

docs/design/proposals/rbd-qos.md Show resolved Hide resolved
Comment on lines 242 to 243
in the Alpha stage. Consequently, it will be disabled by default, but it does
offer the following advantages.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

@Madhu-1 Madhu-1 force-pushed the rbd-qos branch 2 times, most recently from 7da0a8c to 7bdff7d Compare September 5, 2023 10:10
@Madhu-1 Madhu-1 requested a review from Rakshith-R September 5, 2023 10:10
Rakshith-R
Rakshith-R previously approved these changes Sep 5, 2023
docs/design/proposals/rbd-qos.md Outdated Show resolved Hide resolved
docs/design/proposals/rbd-qos.md Outdated Show resolved Hide resolved
docs/design/proposals/rbd-qos.md Outdated Show resolved Hide resolved
Copy link
Contributor

@idryomov idryomov left a 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!

docs/design/proposals/rbd-qos.md Outdated Show resolved Hide resolved
docs/design/proposals/rbd-qos.md Outdated Show resolved Hide resolved
docs/design/proposals/rbd-qos.md Outdated Show resolved Hide resolved
docs/design/proposals/rbd-qos.md Outdated Show resolved Hide resolved
docs/design/proposals/rbd-qos.md Show resolved Hide resolved
docs/design/proposals/rbd-qos.md Outdated Show resolved Hide resolved
@Madhu-1 Madhu-1 requested a review from idryomov September 6, 2023 05:31

### Hybrid Approach

Considering the advantages and drawbacks, we can use StorageClass and
Copy link
Member

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.

Copy link
Collaborator Author

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.

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Sep 7, 2023

@Mergifyio queue

@mergify
Copy link
Contributor

mergify bot commented Sep 7, 2023

queue

✅ The pull request has been merged automatically

The 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>
@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Sep 7, 2023
@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.26

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.26

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.26

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.27

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Sep 7, 2023
@mergify mergify bot merged commit 7c0ce53 into ceph:devel Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/skip/e2e skip running e2e CI jobs ci/skip/multi-arch-build skip building on multiple architectures component/docs Issues and PRs related to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

librbd QoS settings for RBD based PVs
6 participants