rbd: consider rbd as default mounter if not set. #3080

Madhu-1 · 2022-05-04T09:04:53Z

For the default mounter the mounter option will not be set in the storageclass and as it is not available in the storageclass same will not be set in the volume context, Because of this, the mapOptions are getting discarded. If the mounter is not set assuming it's an rbd mounter.

Note:- If the mounter is not set in the storageclass we can set it in the volume context explicitly, Doing this check-in node server to support backward existing volumes and the check is minimal we are not altering the volume context.

fixes: #3076

Signed-off-by: Madhu Rajanna madhupr007@gmail.com

pkalever · 2022-05-04T09:09:27Z

@Madhu-1
It will be good to have a test case for validating mount options from various mounters. Is this something that can be addressed in this PR?

Madhu-1 · 2022-05-04T09:13:30Z

@Madhu-1 It will be good to have a test case for validating mount options from various mounters. Is this something that can be addressed in this PR?

@pkalever i tried to check the map options like "mapOptions": "lock_on_read,queue_depth=1024" which we have in e2e. this does not show up in the mount |grep rbd output. do you have any other way to extract the mount options of the rbd device?

Madhu-1 · 2022-05-04T09:15:57Z

@idryomov might help here. @idryomov is there a way to check the map options of the already mapped rbd device?

pkalever · 2022-05-04T09:18:25Z

@pkalever i tried to check the map options like "mapOptions": "lock_on_read,queue_depth=1024" which we have in e2e. this does not show up in the mount |grep rbd output. do you have any other way to extract the mount options of the rbd device?

These are map time options and not mount options IMO. For userspace mounters check if the process cmd has the options tied and for krbd some /sys/blah/blah should have them (just grep lock_on_read in the /sys dir).

Thanks!

Madhu-1 · 2022-05-04T09:23:17Z

@pkalever i tried to check the map options like "mapOptions": "lock_on_read,queue_depth=1024" which we have in e2e. this does not show up in the mount |grep rbd output. do you have any other way to extract the mount options of the rbd device?

These are map time options and not mount options IMO.

👍

For userspace mounters check if the process cmd has the options tied and for krbd, some /sys/blah/blah should have them (just grep lock_on_read in the /sys dir).

Yes for nbd we can do that but let's wait for @idryomov reply to see if we can have a common approach to identity it.

Thanks!

idryomov · 2022-05-04T09:43:25Z

internal/rbd/nodeserver.go

@@ -252,7 +256,7 @@ func populateRbdVol(
 		// fallback to rbd-nbd,
 		rv.Mounter = rbdNbdMounter
 	} else {
-		rv.Mounter = req.GetVolumeContext()["mounter"]
+		rv.Mounter = mounter


Shouldn't rv.Mounter be already set here regardless because of

if rbdVol.Mounter, ok = volOptions["mounter"]; !ok { rbdVol.Mounter = rbdDefaultMounter }

in genVolFromVolumeOptions?

Yes, correct, this is doing the same thing. removed it now.

idryomov · 2022-05-04T09:47:06Z

internal/rbd/nodeserver.go

@@ -241,7 +241,11 @@ func populateRbdVol(
 		return nil, status.Error(codes.Internal, err.Error())
 	}

-	if req.GetVolumeContext()["mounter"] == rbdDefaultMounter &&


I might be misreading, but I think you could just switch to querying rv.Mounter instead of req.GetVolumeContext()["mounter"] here and get rid of the else branch entirely.

idryomov · 2022-05-04T09:50:01Z

@idryomov might help here. @idryomov is there a way to check the map options of the already mapped rbd device?

There is -- config_info file /sys/bus/rbd/device/<id> directory (where <id> corresponds to /dev/rbd<id>) but I don't see any need to do that here, neither for krbd nor for rbd-nbd.

Madhu-1 · 2022-05-04T09:55:50Z

@idryomov changes are done PTAL.

pkalever · 2022-05-04T11:30:49Z

@Madhu-1 are you still considering adding e2e for MapOptions as part of this PR?

Madhu-1 · 2022-05-04T11:31:40Z

@Madhu-1 are you still considering adding e2e for MapOptions as part of this PR?

As it needs to be added in multiple places, will send a follow-up PR.

pkalever

If you want to delay the e2e test case addition PR, please consider raising an issue for now.
Rest all looks good to me!

Madhu-1 · 2022-05-09T11:42:19Z

@Mergifyio rebase

For the default mounter the mounter option will not be set in the storageclass and as it is not available in the storageclass same will not be set in the volume context, Because of this the mapOptions are getting discarded. If the mounter is not set assuming it's an rbd mounter. Note:- If the mounter is not set in the storageclass we can set it in the volume context explicitly, Doing this check-in node server to support backward existing volumes and the check is minimal we are not altering the volume context. fixes: ceph#3076 Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>

mergify · 2022-05-09T11:43:05Z

rebase

✅ Branch has been successfully rebased

humblec · 2022-05-09T11:53:29Z

hmm.. ideally if mounter has not been set in the SC, discarding the mapOptions is also valid I think..That could avoid some accidental mapOption setting via SC if its not meant for default mounter.. No strong opinion on we should stick to that model though.

Madhu-1 · 2022-05-09T11:55:36Z

hmm.. ideally if mounter has not been set in the SC, discarding the mapOptions is also valid I think..That could avoid some accidental mapOption setting via SC if its not meant for default mounter..

This is a bug fix that got introduced recently. if the mounter is not set in the SC, default is chosen as the rbd if they are till now we are applying the mapOptions. i don't really understand the concern here.

humblec · 2022-05-09T12:00:28Z

hmm.. ideally if mounter has not been set in the SC, discarding the mapOptions is also valid I think..That could avoid some accidental mapOption setting via SC if its not meant for default mounter..

This is a bug fix that got introduced recently. if the mounter is not set in the SC, default is chosen as the rbd if they are till now we are applying the mapOptions. i don't really understand the concern here.

One thing which we have to be careful here is the scenario of mounter has not been set but tryothermounters set to true. In that case, the mapOptions could be gone to other mounters if krbd is not present and fail ?

Madhu-1 · 2022-05-09T12:03:30Z

hmm.. ideally if mounter has not been set in the SC, discarding the mapOptions is also valid I think..That could avoid some accidental mapOption setting via SC if its not meant for default mounter..

This is a bug fix that got introduced recently. if the mounter is not set in the SC, default is chosen as the rbd if they are till now we are applying the mapOptions. i don't really understand the concern here.

One thing which we have to be careful here is the scenario of mounter has not been set but tryothermounters set to true. In that case, the mapOptions could be gone to other mounters if krbd is not present and fail ?

what is the problem with that? why nbd will use krbd map options? mapOptions contains both krbd and nbd specific mapOptions. based on the mounter the specific mount options will be chosen.

nixpanic

The change looks good to me. The commit message is a little confusing, but I guess the small diff explains itself well enough too.

ceph-csi-bot · 2022-05-09T14:40:05Z

/retest ci/centos/mini-e2e-helm/k8s-1.21

ceph-csi-bot · 2022-05-09T14:40:06Z

@Madhu-1 "ci/centos/mini-e2e-helm/k8s-1.21" test failed. Logs are available at location for debugging

ceph-csi-bot · 2022-05-09T14:40:06Z

@Mergifyio requeue

mergify · 2022-05-09T14:40:27Z

requeue

❌ This pull request head commit has not been previously disembarked from queue.

ceph-csi-bot · 2022-05-09T15:43:57Z

/retest ci/centos/mini-e2e-helm/k8s-1.21

ceph-csi-bot · 2022-05-09T15:43:58Z

@Madhu-1 "ci/centos/mini-e2e-helm/k8s-1.21" test failed. Logs are available at location for debugging

ceph-csi-bot · 2022-05-09T15:43:59Z

@Mergifyio requeue

mergify · 2022-05-09T15:44:19Z

requeue

❌ This pull request head commit has not been previously disembarked from queue.

ceph-csi-bot · 2022-05-09T16:44:58Z

/retest ci/centos/mini-e2e-helm/k8s-1.21

ceph-csi-bot · 2022-05-09T16:44:59Z

@Madhu-1 "ci/centos/mini-e2e-helm/k8s-1.21" test failed. Logs are available at location for debugging

ceph-csi-bot · 2022-05-09T16:44:59Z

@Mergifyio requeue

mergify · 2022-05-09T16:45:30Z

requeue

❌ This pull request head commit has not been previously disembarked from queue.

ceph-csi-bot · 2022-05-09T17:44:52Z

/retest ci/centos/mini-e2e-helm/k8s-1.21

ceph-csi-bot · 2022-05-09T17:44:52Z

@Madhu-1 "ci/centos/mini-e2e-helm/k8s-1.21" test failed. Logs are available at location for debugging

ceph-csi-bot · 2022-05-09T17:44:53Z

@Mergifyio requeue

mergify · 2022-05-09T17:45:12Z

requeue

❌ This pull request head commit has not been previously disembarked from queue.

mergify bot added the component/rbd Issues related to RBD label May 4, 2022

Madhu-1 requested review from pkalever and a team May 4, 2022 09:04

Madhu-1 added the backport-to-release-v3.6 label May 4, 2022

Madhu-1 force-pushed the fix-3076 branch from 2ff8908 to b3d088b Compare May 4, 2022 09:08

idryomov reviewed May 4, 2022

View reviewed changes

Madhu-1 force-pushed the fix-3076 branch from b3d088b to 564eb68 Compare May 4, 2022 09:52

idryomov approved these changes May 4, 2022

View reviewed changes

Rakshith-R approved these changes May 4, 2022

View reviewed changes

pkalever approved these changes May 4, 2022

View reviewed changes

Madhu-1 mentioned this pull request May 4, 2022

Add E2E test cases for rbd map options. #3082

Closed

Rakshith-R requested review from humblec and nixpanic May 5, 2022 10:10

Madhu-1 force-pushed the fix-3076 branch from 564eb68 to badb565 Compare May 9, 2022 11:43

nixpanic approved these changes May 9, 2022

View reviewed changes

Madhu-1 added the ci/retry/e2e Label to retry e2e retesting on approved PR's label May 9, 2022

mergify bot merged commit 7067456 into ceph:devel May 9, 2022

mergify bot mentioned this pull request May 9, 2022

rbd: consider rbd as default mounter if not set. (backport #3080) #3107

Merged

pkalever mentioned this pull request May 10, 2022

rbd: fix bug with missing supported_features #3033

Merged

rbd: consider rbd as default mounter if not set. #3080

rbd: consider rbd as default mounter if not set. #3080

Conversation

Madhu-1 commented May 4, 2022

pkalever commented May 4, 2022

Madhu-1 commented May 4, 2022

Madhu-1 commented May 4, 2022

pkalever commented May 4, 2022

Madhu-1 commented May 4, 2022

idryomov May 4, 2022

Choose a reason for hiding this comment

Madhu-1 May 4, 2022

Choose a reason for hiding this comment

idryomov May 4, 2022

Choose a reason for hiding this comment

idryomov commented May 4, 2022 • edited Loading

Madhu-1 commented May 4, 2022

pkalever commented May 4, 2022

Madhu-1 commented May 4, 2022

pkalever left a comment

Choose a reason for hiding this comment

Madhu-1 commented May 9, 2022

mergify bot commented May 9, 2022

✅ Branch has been successfully rebased

humblec commented May 9, 2022 • edited Loading

Madhu-1 commented May 9, 2022

humblec commented May 9, 2022

Madhu-1 commented May 9, 2022

nixpanic left a comment

Choose a reason for hiding this comment

ceph-csi-bot commented May 9, 2022

ceph-csi-bot commented May 9, 2022

ceph-csi-bot commented May 9, 2022

mergify bot commented May 9, 2022

❌ This pull request head commit has not been previously disembarked from queue.

ceph-csi-bot commented May 9, 2022

ceph-csi-bot commented May 9, 2022

ceph-csi-bot commented May 9, 2022

mergify bot commented May 9, 2022

❌ This pull request head commit has not been previously disembarked from queue.

ceph-csi-bot commented May 9, 2022

ceph-csi-bot commented May 9, 2022

ceph-csi-bot commented May 9, 2022

mergify bot commented May 9, 2022

❌ This pull request head commit has not been previously disembarked from queue.

ceph-csi-bot commented May 9, 2022

ceph-csi-bot commented May 9, 2022

ceph-csi-bot commented May 9, 2022

mergify bot commented May 9, 2022

❌ This pull request head commit has not been previously disembarked from queue.

idryomov commented May 4, 2022 •

edited

Loading

humblec commented May 9, 2022 •

edited

Loading