Skip to content

rbd: return InvalidArgument for unrecognized QoS - #6457

Open
Madhu-1 wants to merge 2 commits into
ceph:develfrom
Madhu-1:6443
Open

rbd: return InvalidArgument for unrecognized QoS#6457
Madhu-1 wants to merge 2 commits into
ceph:develfrom
Madhu-1:6443

Conversation

@Madhu-1

@Madhu-1 Madhu-1 commented Aug 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Unrecognized or invalid QoS parameters in mutable volume attributes are a client input error, not an internal
server error. Wrap these errors with ErrInvalidArgument so callers return codes.InvalidArgument instead of codes.Internal.

Allow unsetting the Qos by setting the values as max

@Madhu-1 Madhu-1 linked an issue Aug 5, 2026 that may be closed by this pull request
[RBD] Unrecognized VolumeAttributesClass parameter names silently clear all cgroup QoS metadata #6443
Open
@mergify mergify Bot added the component/rbd Issues related to RBD label Aug 5, 2026
@Madhu-1
Madhu-1 requested review from a team August 5, 2026 05:51
iPraveenParihar
iPraveenParihar requested changes Aug 5, 2026
View reviewed changes

@iPraveenParihar iPraveenParihar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Madhu-1, Have you tested if when the VAC is deleted or removed from a PVC, the QoS settings are cleared?

Comment thread internal/rbd/controllerserver.go
Comment thread internal/rbd/controllerserver.go
@Madhu-1

Madhu-1 commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

@Madhu-1, Have you tested if when the VAC is deleted or removed from a PVC, the QoS settings are cleared?

This can never happened, only VAC can be changed from the PVC and VAC cannot be deleted if its attached to a PVC, what we have is safe check where it never happens in kubernetes. kept it to be on safer side for other CO

@Madhu-1
Madhu-1 requested a review from iPraveenParihar August 5, 2026 09:19
Rakshith-R
Rakshith-R previously approved these changes Aug 5, 2026
View reviewed changes
iPraveenParihar
iPraveenParihar previously approved these changes Aug 5, 2026
View reviewed changes
@Madhu-1

Madhu-1 commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

/queue

@ceph-csi-bot

ceph-csi-bot commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

@Mergifyio rebase

@mergify

mergify Bot commented Aug 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

rebase

🛑 The pull request rule doesn't match anymore

Details

This action has been cancelled.

@ceph-csi-bot ceph-csi-bot added ok-to-test Label to trigger E2E tests and removed queued/rebase labels Aug 5, 2026
@ceph-csi-bot

ceph-csi-bot commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot

ceph-csi-bot commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot

ceph-csi-bot commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

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

@ceph-csi-bot

ceph-csi-bot commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

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

@ceph-csi-bot

ceph-csi-bot commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

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

@ceph-csi-bot

ceph-csi-bot commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

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

@ceph-csi-bot

ceph-csi-bot commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

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

@ceph-csi-bot

ceph-csi-bot commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

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

@ceph-csi-bot

ceph-csi-bot commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

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

@ceph-csi-bot

ceph-csi-bot commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

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

@ceph-csi-bot

ceph-csi-bot commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

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

@ceph-csi-bot ceph-csi-bot added the ci/in-progress/e2e This label acts like a guard and prevents Mergify from adding the `ok-to-test` label again. label Aug 5, 2026
@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Aug 5, 2026
@mergify mergify Bot removed the ci/in-progress/e2e This label acts like a guard and prevents Mergify from adding the `ok-to-test` label again. label Aug 5, 2026
@iPraveenParihar

iPraveenParihar commented Aug 5, 2026

Copy link
Copy Markdown
Contributor 
nvmeof Test NVMe CSI [It] create a PVC with VAC containing allowHostNQNs, apply new VAC then delete the PVC

/go/src/github.com/ceph/ceph-csi/e2e/nvmeof.go:316

  [FAILED] Unexpected error:
      <*fmt.wrapError | 0x550ffd42da0>: 
      failed to get pvc: client rate limiter Wait returned an error: context deadline exceeded
      {
          msg: "failed to get pvc: client rate limiter Wait returned an error: context deadline exceeded",
          err: <*fmt.wrapError | 0x550ffd42d80>{
              msg: "client rate limiter Wait returned an error: context deadline exceeded",
              err: <context.deadlineExceededError>{},
          },
      }
  occurred
  In [It] at: /go/src/github.com/ceph/ceph-csi/e2e/nvmeof.go:395 @ 08/05/26 11:21:49.653

------------------------------

SS

@Madhu-1

Madhu-1 commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

@nixpanic reusing the RBD functions like CreateVolume can cause problem for nvme, we need to separate it out as checks are unique for RBD and nvme will never pass through or we need to unset parameters specific to nvme before calling RBD functions

@nixpanic

nixpanic commented Aug 5, 2026

Copy link
Copy Markdown
Member

I guess there is something in the NVMe-oF Controller that calls ControllerModifyVolume on the RBD Controller 🤔

All failed CI jobs (3 currently, all other e2e still running), failed with this:

nvmeof Test NVMe CSI [It] create a PVC with VAC containing allowHostNQNs, apply new VAC then delete the PVC

/cc @gadididi

@Madhu-1
rbd: return InvalidArgument for unrecognized QoS
9a97d35 
Unrecognized or invalid QoS parameters in
mutable volume attributes
are a client input error, not an internal
server error. Wrap these errors with
ErrInvalidArgument so callers return
codes.InvalidArgument instead of
codes.Internal.

Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
@Madhu-1

Madhu-1 commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

I guess there is something in the NVMe-oF Controller that calls ControllerModifyVolume on the RBD Controller 🤔

All failed CI jobs (3 currently, all other e2e still running), failed with this:

nvmeof Test NVMe CSI [It] create a PVC with VAC containing allowHostNQNs, apply new VAC then delete the PVC

/cc @gadididi

it because we are calling rbd createVolume from vnme CreateVolume. added a 2nd commit to strip nvme specific mutable parameters

@mergify
mergify Bot dismissed stale reviews from iPraveenParihar and Rakshith-R August 5, 2026 12:28

Pull request has been modified.

@Madhu-1

Madhu-1 commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

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

@Madhu-1

Madhu-1 commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

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

@Madhu-1 Madhu-1 mentioned this pull request Aug 5, 2026
Open
gadididi
gadididi reviewed Aug 5, 2026
View reviewed changes
Comment thread internal/nvmeof/controller/controllerserver_test.go Outdated
@Madhu-1

Madhu-1 commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

test running here https://jenkins-ceph-csi.apps.ocp.cloud.ci.centos.org/blue/organizations/jenkins/mini-e2e_k8s-1.35/detail/mini-e2e_k8s-1.35/337/pipeline

@Madhu-1
Madhu-1 requested a review from gadididi August 5, 2026 12:54
nixpanic
nixpanic reviewed Aug 5, 2026
View reviewed changes
Comment thread internal/nvmeof/controller/controllerserver.go Outdated
@Madhu-1

Madhu-1 commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

Added one more commit to allow unsetting the Qos

@Madhu-1
Madhu-1 requested a review from nixpanic August 6, 2026 05:39
black-dragon74
black-dragon74 reviewed Aug 6, 2026
View reviewed changes
Comment thread internal/rbd/cgroup_qos.go Outdated
black-dragon74
black-dragon74 reviewed Aug 6, 2026
View reviewed changes
Comment thread internal/rbd/cgroup_qos.go Outdated
@Madhu-1
rbd: allow "max" and "0" as valid cgroup QoS
45bb98e 
Accept "max" to reset QoS limits to
unlimited and "0" to fully
throttle I/O. This enables users to
revert QoS configurations
applied by a VolumeAttributesClass
by creating a VAC with all
values set to "max".

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
@Madhu-1
Madhu-1 requested a review from black-dragon74 August 6, 2026 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iPraveenParihar iPraveenParihar Awaiting requested review from iPraveenParihar
@Rakshith-R Rakshith-R Awaiting requested review from Rakshith-R
@gadididi gadididi Awaiting requested review from gadididi
@nixpanic nixpanic Awaiting requested review from nixpanic
@black-dragon74 black-dragon74 Awaiting requested review from black-dragon74

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

component/rbd Issues related to RBD

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[RBD] Unrecognized VolumeAttributesClass parameter names silently clear all cgroup QoS metadata

7 participants

@Madhu-1 @ceph-csi-bot @iPraveenParihar @nixpanic @black-dragon74 @Rakshith-R @gadididi