Skip to content

rbd: cleanup and improve read-only volume handling #5290

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 30, 2025
Merged

rbd: cleanup and improve read-only volume handling #5290

merged 2 commits into from
Apr 30, 2025

Conversation

@nixpanic
Copy link
Member

@nixpanic nixpanic commented Apr 24, 2025 •
edited
Loading

There are two components in this PR that are all related to the read-only handling while staging an RBD-image that is expected to be read-only:

  1. use helper functions from csi-common for VolumeCapability checking
    The internal/csi-common package offers helper functions like
    IsReaderOnly() and IsBlockMultiNode(). These should be used instead
    of checking the VolumeCapability that is passed in a request in
    different places.
    This also suggested that adding the "ro" mount option in
    NodeServer.mountVolumeToStagePath() is not appropriate, as the
    csi-common helper ConstructMountOptions() can take care of that
    already too.

  2. Volumes that were requested with a read-only capability should not be resized.

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

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

@mergify mergify bot added the component/rbd Issues related to RBD label Apr 24, 2025
nixpanic
nixpanic commented Apr 24, 2025
View reviewed changes
nixpanic
nixpanic commented Apr 24, 2025
View reviewed changes
internal/rbd/nodeserver.go
// Allow image to be mounted on multiple nodes if it is ROX
if req.GetVolumeCapability().GetAccessMode().GetMode() == csi.VolumeCapability_AccessMode_MULTI_NODE_READER_ONLY {
log.ExtendedLog(ctx, "setting disableInUseChecks on rbd volume to: %v", req.GetVolumeId)
volOptions.DisableInUseChecks = true
Copy link
Member Author

Choose a reason for hiding this comment

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

For other reviewers: it may look confusing that volOptions.DisableInUseChecks is removed here. It isn't needed, because the volOptions was created by populateRbdVol() which sets it already too.

@nixpanic
Copy link
Member Author

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

@nixpanic
Copy link
Member Author

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

Failed with:

[FAILED] backend images not matching kubernetes resource count,image count 2 kubernetes resource count 1
  backend image Info:
   [csi-vol-45f709e0-4a12-4c9d-a08f-8e842a54d254 csi-vol-58f399e6-b792-41c2-97bd-ce267b6597dd]
   images information and status Pool: replicapool, Image: csi-vol-45f709e0-4a12-4c9d-a08f-8e842a54d254, Info: {"name":"csi-vol-45f709e0-4a12-4c9d-a08f-8e842a54d254","id":"16944b8101e0","size":1073741824,"objects":256,"order":22,"object_size":4194304,"snapshot_count":0,"block_name_prefix":"rbd_data.16944b8101e0","format":2,"features":["layering"],"op_features":[],"flags":[],"create_timestamp":"Fri Apr 25 08:05:55 2025","access_timestamp":"Fri Apr 25 08:05:55 2025","modify_timestamp":"Fri Apr 25 08:05:55 2025"}
  , Status: {"watchers":[]}

  Pool: replicapool, Image: csi-vol-58f399e6-b792-41c2-97bd-ce267b6597dd, Info: , Status:

logs

@nixpanic
Copy link
Member Author

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

@nixpanic nixpanic requested a review from a team April 25, 2025 10:54
@black-dragon74
Copy link
Member

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

@nixpanic nixpanic requested a review from a team April 28, 2025 12:23
Rakshith-R
Rakshith-R approved these changes Apr 30, 2025
View reviewed changes
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

@nixpanic
Copy link
Member Author

@Mergifyio queue

@mergify
Copy link
Contributor

mergify bot commented Apr 30, 2025 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 31da098

nixpanic added 2 commits April 30, 2025 12:04
@nixpanic @mergify
rbd: use helper functions from csi-common for VolumeCapability checking
09e4a96 
The internal/csi-common package offers helper functions like
`IsReaderOnly()` and `IsBlockMultiNode()`. These should be used instead
of checking the VolumeCapability that is passed in a request in
different places.

This also suggested that adding the "ro" mount option in
`NodeServer.mountVolumeToStagePath()` is not appropriate, as the
csi-common helper `ConstructMountOptions()` can take care of that
already too.

Signed-off-by: Niels de Vos <[email protected]>
@nixpanic @mergify
rbd: do not resize read-only volumes while staging
a5a5032 
Volumes that were requested with a read-only capability should not be
resized.

Reported-by: Alex Kalenyuk <[email protected]>
Signed-off-by: Niels de Vos <[email protected]>
@mergify mergify bot force-pushed the rbd/ro-resize branch from 1974feb to a5a5032 Compare April 30, 2025 12:04
@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Apr 30, 2025
@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Apr 30, 2025
@mergify mergify bot merged commit 31da098 into ceph:devel Apr 30, 2025
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@black-dragon74 black-dragon74 black-dragon74 approved these changes

@Rakshith-R Rakshith-R Rakshith-R approved these changes

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.

4 participants

@nixpanic @black-dragon74 @ceph-csi-bot @Rakshith-R