feat: Add rados namespace support to rbd plugin #1035

mehdy · 2020-05-11T11:12:54Z

Describe what this PR does

Make sure to operate within the namespace if any given when dealing
with rbd images and snapshots and volume and snapshot journals.

Add some basic e2e tests using a namespace.

Is there anything that requires special attention

Do you have any questions?
No.

Is the change backward compatible?
Yes.

Are there concerns around backward compatibility?
No.

Provide any external context for the change, if any.

rbd namespace

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: #798

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.

mehdy · 2020-05-13T08:16:17Z

I'm using SetNamespace method a lot in this PR and it was removed this commit :))
Should I use journal.namespace = ns or rewrite a new SetNamespace method again or would you revert this commit?
@Madhu-1

Madhu-1 · 2020-05-13T09:31:22Z

I'm using SetNamespace method a lot in this PR and it was removed this commit :))
Should I use journal.namespace = ns or rewrite a new SetNamespace method again or would you revert this commit?
@Madhu-1

having SetNamespace makes sense now as you need to call it multiple times. @phlogistonjohn any thoughts?

phlogistonjohn · 2020-05-13T12:54:37Z

Mutating a global object frequently strikes me as a "code smell" that should be avoided in general. That's one of the reasons I removed it.

I quickly looked at your PR here and see a very common pattern, and since I'm also attempting to refactor how the journal code works, many call sites I've become familiar with over the last few days. In these cases you're calling SetNamespace right before a call to something like volJournal.CheckReservation.

Without digging deeper into your changes, I'd suggest altering the journal function arguments and adding namespace there rather than repeatedly mutating the state of the volJournal/etc.

FWIW, I'm also trying to break up the functionality such that the operational functions in voljournal.go rely more on local state via a new type rather than the global volJournal/snapJournal values. This means I too will be changing the arguments of the same functions. So this is just a heads up that we're likely to make merge conflicts with each other. I'm not trying to worry or discourage you... just making you aware of it. :-)

mehdy · 2020-05-13T13:25:30Z

@phlogistonjohn I'll change the journal functions to receive namespace as an argument as you suggested. Thanks!

About the refactoring work you mentioned, wouldn't it be better if you do it after this PR? :D

mergify · 2020-08-08T00:12:33Z

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

Make sure to operate within the namespace if any given when dealing with rbd images and snapshots and their journals. Signed-off-by: Mehdy Khoshnoody <[email protected]>

A minimal documentation on how to use a rados namespace with rbd. Signed-off-by: Mehdy Khoshnoody <[email protected]>

These test cases are will be executed against a rados namespace. - Create a PVC and bind it to an app. - Resize block PVC and check device size. - Create a PVC clone and bind it to an app. Signed-off-by: Mehdy Khoshnoody <[email protected]>

mehdy · 2020-08-10T10:40:54Z

@Madhu-1 Resolved the conflicts and ready to merged.

humblec · 2020-08-10T10:59:39Z

examples/csi-config-map-sample.yaml

@@ -12,6 +12,12 @@ kind: ConfigMap
 # each such cluster in use.
 # To add more clusters or edit MON addresses in an existing configmap, use
 # the `kubectl replace` command.
+# The <rados-namespace> is optional and represents a radosNamespace in the pool.
+# If any given all the of the rbd images, snapshots and other metadata


to me, it feels like we need to reword this and below line @mehdy

Sure! How about this?

"To use a specific rados namespace for your rbd images and snapshots, provide the name of the desired rados namespace".

If you have something else in mind, I'd be glad to use the help.

@mehdy it looks good to me ! 👍

humblec · 2020-08-10T11:02:09Z

@mehdy have we opened an issue for this or is it already resolved ?

When running e2e tests there was an error with the raw-block-pod but testing it again without any of changes against a clean environment the issue still exists (maybe it's another issue).

mehdy · 2020-08-10T11:22:32Z

@mehdy have we opened an issue for this or is it already resolved ?

When running e2e tests there was an error with the raw-block-pod but testing it again without any of changes against a clean environment the issue still exists (maybe it's another issue).

This was an issue with my dev environment.

Madhu-1 · 2020-08-10T11:24:52Z

@humblec do you have any final comments on this feature? Can we take this in for v3.1.0?
Any code changes required?

humblec · 2020-08-11T09:31:00Z

@mehdy have we opened an issue for this or is it already resolved ?
When running e2e tests there was an error with the raw-block-pod but testing it again without any of changes against a clean environment the issue still exists (maybe it's another issue).
This was an issue with my dev environment.

@mehdy can you removing it from the PR description then?

mehdy · 2020-08-11T09:36:10Z

@humblec Done 👍

humblec · 2020-08-11T09:40:00Z

@humblec do you have any final comments on this feature? Can we take this in for v3.1.0?
Any code changes required?

@Madhu-1 as discussed, lets take this in for 3.1.0, especially this was deferred from 3.0.0 to avoid last minute hiccups with 3.0.0 release!

humblec · 2020-08-11T09:41:41Z

@humblec Done +1

@mehdy Thanks !! If at ll possible, its also good to have a mention or documentation around this change. But it can be a followup PR or can be discarded too!

Madhu-1 · 2020-08-11T09:43:51Z

@humblec do you have any final comments on this feature? Can we take this in for v3.1.0?
Any code changes required?

@Madhu-1 as discussed, lets take this in for 3.1.0, especially this was deferred from 3.0.0 to avoid last minute hiccups with 3.0.0 release!
As we have minimal upgrade testing

@humblec I would suggest testing the backward compatibility before taking it in.

mehdy · 2020-08-11T09:58:29Z

@humblec Done +1

@mehdy Thanks !! If at ll possible, its also good to have a mention or documentation around this change. But it can be a followup PR or can be discarded too!

@humblec You're welcome! Sure. I'll see what I can do and I'll open a new PR if I had something to add.

humblec · 2020-08-12T10:51:31Z

Merging this manually as discussed in the Slack as the auto merge is blocked for long time.

merging of ceph#1035 broken the cephcsi building. This commits fixes the build issue. Signed-off-by: Madhu Rajanna <[email protected]>

merging of #1035 broken the cephcsi building. This commits fixes the build issue. Signed-off-by: Madhu Rajanna <[email protected]>

mehdy changed the title ~~rbd: Add rados namespace support~~ feat: Add rados namespace support to rbd plugin May 11, 2020

nixpanic added enhancement New feature or request component/rbd Issues related to RBD labels May 11, 2020

mehdy force-pushed the master branch 3 times, most recently from d415b8a to 402b201 Compare May 13, 2020 08:00

mehdy force-pushed the master branch 20 times, most recently from 71ba97a to bb1df07 Compare May 19, 2020 11:53

mehdy force-pushed the master branch 3 times, most recently from cb526de to 8b81a62 Compare August 9, 2020 19:13

mehdy added 3 commits August 10, 2020 11:14

rbd: Add rados namespace support for rbd

74c51b0

Make sure to operate within the namespace if any given when dealing with rbd images and snapshots and their journals. Signed-off-by: Mehdy Khoshnoody <[email protected]>

doc: Add rados namespace to option to csi-configs

e18a342

A minimal documentation on how to use a rados namespace with rbd. Signed-off-by: Mehdy Khoshnoody <[email protected]>

mehdy force-pushed the master branch from 8b81a62 to a1cdc58 Compare August 10, 2020 06:44

humblec reviewed Aug 10, 2020

View reviewed changes

humblec approved these changes Aug 11, 2020

View reviewed changes

humblec mentioned this pull request Aug 11, 2020

Release 3.1.0 of Ceph CSI Driver #1272

Closed

9 tasks

Madhu-1 requested a review from nixpanic August 11, 2020 13:15

Madhu-1 approved these changes Aug 12, 2020

View reviewed changes

humblec merged commit 2044873 into ceph:master Aug 12, 2020

humblec mentioned this pull request Aug 12, 2020

Revert "feat: Add rados namespace support to rbd plugin" #1366

Closed

Madhu-1 added a commit to Madhu-1/ceph-csi that referenced this pull request Aug 12, 2020

rbd: fix build issue for rbd

b500a92

merging of ceph#1035 broken the cephcsi building. This commits fixes the build issue. Signed-off-by: Madhu Rajanna <[email protected]>

Madhu-1 mentioned this pull request Aug 12, 2020

rbd: fix build issue for rbd #1368

Merged

Madhu-1 added a commit that referenced this pull request Aug 12, 2020

rbd: fix build issue for rbd

8a581ec

merging of #1035 broken the cephcsi building. This commits fixes the build issue. Signed-off-by: Madhu Rajanna <[email protected]>

humblec mentioned this pull request Sep 10, 2020

Feat: Add PVC to RBD/CephFS mappings #1086

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add rados namespace support to rbd plugin #1035

feat: Add rados namespace support to rbd plugin #1035

mehdy commented May 11, 2020 •

edited

Loading

mehdy commented May 13, 2020

Madhu-1 commented May 13, 2020

phlogistonjohn commented May 13, 2020

mehdy commented May 13, 2020

mergify bot commented Aug 8, 2020

mehdy commented Aug 10, 2020

humblec Aug 10, 2020

mehdy Aug 10, 2020

humblec Aug 11, 2020

humblec commented Aug 10, 2020

mehdy commented Aug 10, 2020

Madhu-1 commented Aug 10, 2020

humblec commented Aug 11, 2020

mehdy commented Aug 11, 2020

humblec commented Aug 11, 2020

humblec commented Aug 11, 2020

Madhu-1 commented Aug 11, 2020

mehdy commented Aug 11, 2020

humblec commented Aug 12, 2020

feat: Add rados namespace support to rbd plugin #1035

feat: Add rados namespace support to rbd plugin #1035

Conversation

mehdy commented May 11, 2020 • edited Loading

Describe what this PR does

Is there anything that requires special attention

Related issues

Future concerns

mehdy commented May 13, 2020

Madhu-1 commented May 13, 2020

phlogistonjohn commented May 13, 2020

mehdy commented May 13, 2020

mergify bot commented Aug 8, 2020

mehdy commented Aug 10, 2020

humblec Aug 10, 2020

Choose a reason for hiding this comment

mehdy Aug 10, 2020

Choose a reason for hiding this comment

humblec Aug 11, 2020

Choose a reason for hiding this comment

humblec commented Aug 10, 2020

mehdy commented Aug 10, 2020

Madhu-1 commented Aug 10, 2020

humblec commented Aug 11, 2020

mehdy commented Aug 11, 2020

humblec commented Aug 11, 2020

humblec commented Aug 11, 2020

Madhu-1 commented Aug 11, 2020

mehdy commented Aug 11, 2020

humblec commented Aug 12, 2020

mehdy commented May 11, 2020 •

edited

Loading