rbd: support QoS based on capacity for rbd volume #5016

YiteGu · 2024-12-12T09:29:14Z

QoS provides settings for rbd volume read/write iops and read/write bandwidth.
All QoS parameters are placed in the SC, send QoS parameters from SC to Cephcsi through PVC create request.
We need provide QoS parameters in the SC as below:
- BaseReadIops
- BaseWriteIops
- BaseReadBytesPerSecond
- BaseWriteBytesPerSecond
- ReadIopsPerGB
- WriteIopsPerGB
- ReadBpsPerGB
- WriteBpsPerGB
- BaseVolSizeBytes
  There are 4 base qos parameters among them, when users apply for a volume capacity equal to or less than BaseVolSizebytes, use base qos limit. For the portion of capacity exceeding BaseVolSizebytes, QoS will be increased in steps set per GB. If the step size parameter per GB is not provided, only base QoS limit will be used and not associated with capacity size.
If PVC has resize request, adjust the QoS limit according to the QoS parameters after resizing.

Describe what this PR does

Provide some context for the reviewer

Is there anything that requires special attention

Do you have any questions?

Is the change backward compatible?

Are there concerns around backward compatibility?

Provide any external context for the change, if any.

For example:

Kubernetes links that explain why the change is required
CSI spec related changes/catch-up that necessitates this patch
golang related practices that necessitates this change

Related issues

Mention any github issues relevant to this PR. Adding below line
will help to auto close the issue once the PR is merged.

Fixes: #issue_number

Future concerns

List items that are not part of the PR and do not impact it's
functionality, but are work items that can be taken up subsequently.

Checklist:

Commit Message Formatting: Commit titles and messages follow
guidelines in the developer
guide.
Reviewed the developer guide on Submitting a Pull
Request
Pending release
notes
updated with breaking and/or notable changes for the next major release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

Show available bot commands

These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:

/retest ci/centos/<job-name>: retest the <job-name> after unrelated
failure (please report the failure too!)

nixpanic

Thanks for the PR! This is really a nice feature to have. A couple of comments that need to be addressed to improve the maintainability and usability.

move all qos related functions, structs, constants to a new qos.go file
all new functions that receive a *rbdVolume as argument should become a function of the rbdVolume struct (see example for the SetQOS function)
new parameters for the StorageClass need to be added to the documentation
documentation should explain that the QoS settings are currently only used in combination with the NBD mounter
ideally add unit tests for functions that do not interact with a Ceph cluster
ideally add some form or e2e testing, maybe set QoS parameters in the StorageClass, and verify that those are set on an RBD-image?

Thanks again, this is a great start!

internal/rbd/controllerserver.go

internal/rbd/rbd_util.go

internal/rbd/controllerserver.go

internal/rbd/rbd_util.go

YiteGu · 2024-12-18T09:16:13Z

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

internal/rbd/qos_test.go

internal/rbd/controllerserver.go

nixpanic · 2024-12-18T10:12:41Z

Looks quite complete, many thanks for adding the unit and e2e tests!

nixpanic · 2024-12-18T10:13:00Z

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

nixpanic · 2024-12-18T10:14:01Z

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

not sure why that did not work, but the CI is running now (takes ~2 hours to complete): https://jenkins-ceph-csi.apps.ocp.cloud.ci.centos.org/job/mini-e2e_k8s-1.31/283/display/redirect

YiteGu · 2024-12-18T10:26:06Z

Looks quite complete, many thanks for adding the unit and e2e tests!

Write it briefly to verify that e2e testing is available. More scenes need to be added. :-)

nixpanic · 2024-12-19T13:49:24Z

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

YiteGu · 2024-12-20T06:01:26Z

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

YiteGu · 2024-12-20T07:35:52Z

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

github-actions · 2025-01-19T21:01:27Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in two weeks if no further activity occurs. Thank you for your contributions.

Madhu-1 · 2025-01-24T08:27:28Z

docs/rbd/deploy.md

+| `BaseWriteIops`                                                                                     | no                   | the base limit of write operations per second |
+| `BaseReadBytesPerSecond`                                                                            | no                   | the base limit of read bytes per second |
+| `BaseWriteBytesPerSecond`                                                                           | no                   | the base limit of write bytes per second |
+| `ReadIopsPerGB`                                                                                     | no                   | the limit of read operations per GiB |


in the key GB is used and in the comments its mentioned GiB is that correct?

sorry, there is confuse, we should use unit of bytes(IEC).

Madhu-1 · 2025-01-24T08:27:41Z

docs/rbd/deploy.md

+| `WriteIopsPerGB`                                                                                    | no                   | the limit of write operations per GiB |
+| `ReadBpsPerGB`                                                                                      | no                   | the limit of read bytes per GiB |
+| `WriteBpsPerGB`                                                                                     | no                   | the limit of write bytes per GiB |
+| `BaseVolSizeBytes`                                                                                  | no                   | the min size of volume what use to calc qos beased on capacity |


calc to calculate

Madhu-1 · 2025-01-24T08:28:24Z

docs/rbd/deploy.md

@@ -73,6 +73,15 @@ make image-cephcsi
 | `stripeUnit`                                                                                        | no                   | stripe unit in bytes                                                                                                                                                                                                                                                                               |
 | `stripeCount`                                                                                       | no                   | objects to stripe over before looping                                                                                                                                                                                                                                                              |
 | `objectSize`                                                                                        | no                   | object size in bytes                                                                                                                                                                                                                                                                               |
+| `BaseReadIops`                                                                                      | no                   | the base limit of read operations per second |


can all these operations can be supported for krbd?

Yes, these operations can be supported for krbd, but we need to set QoS by cgroup after get QoS limit result by calcQosBasedOnCapacity. if we support burst QoS, krbd can not be support.

Madhu-1 · 2025-01-24T08:29:18Z

e2e/rbd.go

+				qosParameters := map[string]string{
+					"BaseReadIops":            "2000",
+					"BaseWriteIops":           "1000",
+					"BaseReadBytesPerSecond":  "209715200",
+					"BaseWriteBytesPerSecond": "104857600",


please define a var for numeric values as its used in two places to have single source

Madhu-1 · 2025-01-24T08:30:44Z

e2e/rbd.go

+
+				pvc.Namespace = f.UniqueName
+				pvc.Spec.Resources.Requests[v1.ResourceStorage] = resource.MustParse("100Gi")
+				wants = map[string]string{


what will happen for cloned PVC or PVC created from snapshot, are they going to get the same values or different values?

Can you please add a test case of that as well

cloned PVC or PVC created from snapshot get the same values after my testing on the ceph.

Madhu-1 · 2025-01-24T08:39:37Z

internal/rbd/qos.go

+		if errors.Is(err, librbd.ErrNotFound) {
+			// rbdQosPerGBType does not exist, set it empty.
+			perGBLimit = ""


Do we need explict setting it to empty string here , AFAIK it returns empty string in case of an error

Madhu-1 · 2025-01-24T08:40:45Z

internal/rbd/qos.go

+		} else if err != nil {
+			log.ErrorLog(ctx, "failed to get metadata: %s. %v", param.rbdQosPerGBType, err)
+
+			return nil, err


i think we dont need if and else if check here. we can have single check err !=nil && !errors.Is(err, librbd.ErrNotFound)

Madhu-1 · 2025-01-24T08:41:09Z

internal/rbd/qos.go

+	if errors.Is(err, librbd.ErrNotFound) {
+		// rbdBaseQosVolSize does not exist, set it empty.
+		baseVolSize = ""
+	} else if err != nil {
+		log.ErrorLog(ctx, "failed to get metadata: %s. %v", baseQosVolSize, err)
+
+		return nil, err
+	}


same here as well we dont need two checks

Madhu-1 · 2025-01-24T08:41:46Z

internal/rbd/qos.go

+) error {
+	rbdQosParameters, err := getRbdImageQOS(ctx, rv)
+	if err != nil {
+		log.ErrorLog(ctx, "get rbd image qos failed")


log the error and also why we are only logging here not in other places in this function?

this log deleted

Madhu-1 · 2025-01-24T08:43:19Z

internal/rbd/qos.go

+	return rbdQosParameters
+}
+
+func (rv *rbdVolume) SetQOS(


i think we can use rbImage instead of rbdVolume for most of the methods getting added in this PR.

I don't have any particular opinion on this point, I respect your suggestion. :)

Madhu-1 · 2025-01-24T09:01:27Z

examples/rbd/storageclass.yaml

@@ -162,6 +162,38 @@ parameters:
   # stripeCount: <>
   # (optional) The object size in bytes.
   # objectSize: <>
+
+   # rbd volume QoS.


why not use https://kubernetes.io/docs/concepts/storage/volume-attributes-classes/#the-volumeattributesclass-api as these values are not immutable as anything mentioned in the SC is mutable?

Pull request has been modified.

YiteGu · 2025-01-26T10:25:23Z

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

YiteGu · 2025-01-26T12:48:18Z

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

YiteGu · 2025-01-27T05:14:41Z

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

1. QoS provides settings for rbd volume read/write iops and read/write bandwidth. 2. All QoS parameters are placed in the SC, send QoS parameters from SC to Cephcsi through PVC create request. 3. We need provide QoS parameters in the SC as below: - BaseReadIops - BaseWriteIops - BaseReadBytesPerSecond - BaseWriteBytesPerSecond - ReadIopsPerGB - WriteIopsPerGB - ReadBpsPerGB - WriteBpsPerGB - BaseVolSizeBytes There are 4 base qos parameters among them, when users apply for a volume capacity equal to or less than BaseVolSizebytes, use base qos limit. For the portion of capacity exceeding BaseVolSizebytes, QoS will be increased in steps set per GB. If the step size parameter per GB is not provided, only base QoS limit will be used and not associated with capacity size. 4. If PVC has resize request, adjust the QoS limit according to the QoS parameters after resizing. Signed-off-by: Yite Gu <[email protected]>

YiteGu · 2025-01-27T07:56:36Z

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

YiteGu force-pushed the rbd-qos-support branch 2 times, most recently from 8a47010 to c07a41a Compare December 12, 2024 10:20

YiteGu changed the title ~~cephcsi: support QoS based on capacity for rbd volume~~ rbd: support QoS based on capacity for rbd volume Dec 12, 2024

mergify bot added the component/rbd Issues related to RBD label Dec 12, 2024

YiteGu force-pushed the rbd-qos-support branch 3 times, most recently from 63ea87d to f60f7c3 Compare December 12, 2024 12:06

nixpanic reviewed Dec 13, 2024

View reviewed changes

nixpanic added the enhancement New feature or request label Dec 13, 2024

YiteGu force-pushed the rbd-qos-support branch 10 times, most recently from 2b895ad to 31ce867 Compare December 18, 2024 08:29

YiteGu force-pushed the rbd-qos-support branch from 31ce867 to 2b56d76 Compare December 18, 2024 09:27

nixpanic reviewed Dec 18, 2024

View reviewed changes

internal/rbd/qos_test.go Outdated Show resolved Hide resolved

nixpanic reviewed Dec 18, 2024

View reviewed changes

internal/rbd/controllerserver.go Outdated Show resolved Hide resolved

nixpanic reviewed Dec 18, 2024

View reviewed changes

internal/rbd/controllerserver.go Outdated Show resolved Hide resolved

YiteGu force-pushed the rbd-qos-support branch 2 times, most recently from 4d90640 to 0c09729 Compare December 19, 2024 11:05

YiteGu force-pushed the rbd-qos-support branch 2 times, most recently from 47bd9ab to 795ea78 Compare December 19, 2024 11:30

YiteGu force-pushed the rbd-qos-support branch 3 times, most recently from bd4d17e to 4774a82 Compare December 20, 2024 05:05

YiteGu requested a review from nixpanic December 20, 2024 09:39

github-actions bot added the stale label Jan 19, 2025

nixpanic removed the stale label Jan 21, 2025

nixpanic requested a review from Madhu-1 January 21, 2025 10:15

nixpanic previously approved these changes Jan 21, 2025

View reviewed changes

nixpanic requested a review from a team January 21, 2025 13:08

Madhu-1 requested changes Jan 24, 2025

View reviewed changes

Madhu-1 reviewed Jan 24, 2025

View reviewed changes

YiteGu force-pushed the rbd-qos-support branch from 4774a82 to f42036b Compare January 26, 2025 09:48

YiteGu force-pushed the rbd-qos-support branch from f42036b to 348ef23 Compare January 26, 2025 12:33

YiteGu force-pushed the rbd-qos-support branch from 348ef23 to 25a4c0d Compare January 27, 2025 05:04

YiteGu force-pushed the rbd-qos-support branch from 25a4c0d to 0c1ef98 Compare January 27, 2025 07:54

rbd: support QoS based on capacity for rbd volume #5016

Are you sure you want to change the base?

rbd: support QoS based on capacity for rbd volume #5016

Conversation

YiteGu commented Dec 12, 2024 • edited Loading

Describe what this PR does

Is there anything that requires special attention

Related issues

Future concerns

nixpanic left a comment

Choose a reason for hiding this comment

YiteGu commented Dec 18, 2024

nixpanic commented Dec 18, 2024

nixpanic commented Dec 18, 2024

nixpanic commented Dec 18, 2024

YiteGu commented Dec 18, 2024

nixpanic commented Dec 19, 2024

YiteGu commented Dec 20, 2024

YiteGu commented Dec 20, 2024

github-actions bot commented Jan 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YiteGu Jan 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YiteGu commented Jan 26, 2025

YiteGu commented Jan 26, 2025

YiteGu commented Jan 27, 2025

YiteGu commented Jan 27, 2025

YiteGu commented Dec 12, 2024 •

edited

Loading

YiteGu Jan 26, 2025 •

edited

Loading