core: add flatten-rbd-pvc command #229

satoru-takeuchi · 2024-01-26T04:34:51Z

Description of your changes:

Flatten an RBD image corrensponding to the target PVC
Remove the corresponding temporary RBD image

Which issue is resolved by this Pull Request:
Resolves #222

Checklist:

Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
Reviewed the developer guide on Submitting a Pull Request
CI tests has been updated, if necessary.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.

Madhu-1

@satoru-takeuchi i left some early suggestion (even for draft PR) please let me know if it makes sense :)

cmd/commands/flatten_rbd_pvc.go

satoru-takeuchi · 2024-02-01T07:42:09Z

@Madhu-1 Thank you for your valuable comments! I'll reflect them later.

subhamkrai · 2024-03-20T11:07:50Z

@satoru-takeuchi any update on this?

satoru-takeuchi · 2024-04-02T05:48:43Z

@subhamkrai Oops, sorry. I overlooked your message. I'll update this PR to be ready to review in this week.

satoru-takeuchi · 2024-04-05T08:04:20Z

Tests has not been finished yet. I'll udate this PR the beginning of the next week.

satoru-takeuchi · 2024-04-10T10:19:22Z

I lost working-on branch by mistake. Please wait for a more bit..

satoru-takeuchi · 2024-04-10T12:25:47Z

@Madhu-1 I reflected all your comments. I have two questions about testing.

Q1. unit test: Do unit tests necessary for the new command? There isn't any test code under cmd/commands rather than root_test.go.
Q2. integration test: Is it OK to just add a test to flatten a cloned-from-PVC image for both "default-namespace" and "custom-namespace"? Or should I add more tests (e.g. cloned-from-snapshot, try to flatten in-use PVC, and so on) to improve coverage? The current other code seems to test only happy pathes.

subhamkrai · 2024-04-29T04:27:24Z

@Madhu-1 I reflected all your comments. I have two questions about testing.

Q1. unit test: Do unit tests necessary for the new command? There isn't any test code under cmd/commands rather than root_test.go.

@satoru-takeuchi not all the commands required unit test, generally when we string passing in helper function we are adding unit tests but overall not really.

Q2. integration test: Is it OK to just add a test to flatten a cloned-from-PVC image for both "default-namespace" and "custom-namespace"? Or should I add more tests (e.g. cloned-from-snapshot, try to flatten in-use PVC, and so on) to improve coverage? The current other code seems to test only happy pathes.

@Madhu-1 could you take q2? Thanks

Madhu-1 · 2024-04-30T05:48:39Z

Q2. integration test: Is it OK to just add a test to flatten a cloned-from-PVC image for both "default-namespace" and "custom-namespace"? Or should I add more tests (e.g. cloned-from-snapshot, try to flatten in-use PVC, and so on) to improve coverage? The current other code seems to test only happy pathes.

Sorry for delay as i was on PTO, Lets not block this PR for it but good to have tests to cover all cases, we can open a new issue to add more tests to ensure it works for all the cases

satoru-takeuchi · 2024-05-01T08:12:48Z

@subhamkrai @Madhu-1 Thank you for your comments. I'm trying on adding an integration test. I'll make this PR ready to review after that.

Madhu-1

some nits, @satoru-takeuchi you have covered most of the cases 👏🏻

cmd/commands/flatten_rbd_pvc.go

tests/github-action-helper.sh

Madhu-1 · 2024-05-02T06:38:08Z

tests/github-action-helper.sh

+}
+
+wait_for_rbd_pvc_clone_to_be_bound() {
+  kubectl create -f https://raw.githubusercontent.com/rook/rook/master/deploy/examples/csi/rbd/pvc-clone.yaml


i think you are referring to pvc-pvc clone here not pvc-restore right?

where are we creating the volumesnapshotclass and volumesnapshot, if its planned in different PR can we make the changes related to it in same PR instead of here?

i think you are referring to pvc-pvc clone here not pvc-restore right?

Yes, I only handle pvc-pvc clone for now.

I'll add other tests in another PR as you suggested.

#229 (comment)

Lets not block this PR for it but good to have tests to cover all cases, we can open a new issue to add more tests to ensure it works for all the cases

I'll create an issue to track this test improvement. The other tests will have the following items.

PVC restore: pass without deleting the temporary image corresponding to VolumeSnapshot.

static PVC: fail

in-use PVC without allow-in-use option: fail

in-use PVC with allow-in-use option: pass

non-cloned/restored PVC: fail

not-bounded PVC: fail

satoru-takeuchi · 2024-05-08T10:43:47Z

@Madhu-1 Thank you for your comments. I'm back from PTO. I'll update this PR tomorrow.

Madhu-1

@satoru-takeuchi Thanks for the recent update, i have 2 more comments. everything else LGTM.

Madhu-1 · 2024-05-14T07:06:58Z

cmd/commands/flatten_rbd_pvc.go

+const (
+	storageProvisionerBetaAnnotation    = "volume.beta.kubernetes.io/storage-provisioner"
+	storageProvisionerAnnotation        = "volume.kubernetes.io/storage-provisioner"
+	cephCSIRBDStorageProviderAnnotation = "volume.beta.kubernetes.io/storage-provisioner"


this looks to be same as storageProvisionerBetaAnnotation do we need both?

Oops, sorry. I meant not "volume.beta.kubernetes.io/storage-provisioner" but "ceph-ssd.rbd.csi.ceph.com" for cephCSIRBDStorageProviderAnnotation.

Madhu-1 · 2024-05-14T07:08:33Z

cmd/commands/flatten_rbd_pvc.go

+		if pvc.Annotations[storageProvisionerAnnotation] == cephCSIRBDStorageProviderAnnotation || pvc.Annotations[storageProvisionerBetaAnnotation] == cephCSIRBDStorageProviderAnnotation {
+			logging.Fatal(fmt.Errorf("PVC %s is not a CSI RBD PVC", pvcName))
+		}


This check seems to be wrong, we are checking annotation but not checking if its rbd PVC. can you please check

Yes, it's completely wrong. Sorry for the inconvenience. This code passed integration tests contingency.

What I want to do is verify whether the target PVC is really a RBD PVC by checking the existence of one of the the following annotations.

volume.beta.kubernetes.io/storage-provisioner: "ceph-ssd.rbd.csi.ceph.com"

volume.kubernetes.io/storage-provisioner: "ceph-ssd.rbd.csi.ceph.com"

Static PVC doesn't have these annotations.

instead of annotation can we check the presence of ImageName or something or check if it is CSI PV not in-tree PV?

OK, I'll update this PR tomorrorw.

@Madhu-1 I realized that we've already checked the existence of ImageName as follows.

imageName, ok := pv.Spec.CSI.VolumeAttributes["imageName"] if !ok { logging.Fatal(fmt.Errorf("PV %s doesn't contains `imageName` in VolumeAttributes", pvName)) }

In other words, we got the name of RBD image name from RBD PVC's spec.csi.volumeAttributes.imageName. Since this field only exists in RBD PVC, it can be said that we checked whether the target PVC is really an RBD PVC here.

So re-removing checking attribute is enough and I updated this PR as so. Could you check the updated PR again? Correct me if I'm wrong.

Note:
I edited this message to correct the mention from myself to Madhu-1.

Madhu-1 · 2024-05-15T09:10:11Z

cmd/commands/flatten_rbd_pvc.go

+		}
+		if !(strings.HasSuffix(pvc.Annotations[storageProvisionerAnnotationKey], cephCSIRBDStorageProviderAnnotationValSuffix) ||
+			strings.HasSuffix(pvc.Annotations[storageProvisionerBetaAnnotationKey], cephCSIRBDStorageProviderAnnotationValSuffix)) {
+			logging.Fatal(fmt.Errorf("PVC %s is not a CSI RBD PVC", pvcName))


i think we dont need to worry about the static PV as most of the cases they are backed by RBD image not clones.

Madhu-1

@satoru-takeuchi Thanks for taking this up!. LGTM

subhamkrai · 2024-05-17T09:57:07Z

cmd/commands/flatten_rbd_pvc.go

+
+[1]: https://github.com/ceph/ceph-csi/blob/devel/docs/design/proposals/rbd-snap-clone.md`,
+	Args: cobra.ExactArgs(1),
+	Run: func(cmd *cobra.Command, args []string) {


@satoru-takeuchi overall pr looks good, small suggestion if you could move the implementation function to under pkg/ folder and not put it in the same file as where command is written. Thanks after that PR will be ready to merge.

@subhamkrai Indeed, moved.

Add flatten-rbd-pvc command to accomplish the following things: - Flatten an RBD image corrensponding to the target PVC. - Remove the temporary RBD image too. Closes: rook#222 Signed-off-by: Satoru Takeuchi <satoru.takeuchi@gmail.com>

satoru-takeuchi added the do-not-merge label Jan 26, 2024

satoru-takeuchi force-pushed the rook-add-flatten-rbd-pvc-command branch from 998315d to 07c4b39 Compare January 26, 2024 05:10

Madhu-1 reviewed Jan 26, 2024

View reviewed changes

satoru-takeuchi force-pushed the rook-add-flatten-rbd-pvc-command branch 3 times, most recently from 3039295 to 86ce349 Compare February 27, 2024 11:01

satoru-takeuchi force-pushed the rook-add-flatten-rbd-pvc-command branch from 86ce349 to 29b75f6 Compare April 10, 2024 10:06

satoru-takeuchi mentioned this pull request Apr 10, 2024

support namespace option in flatten-rbd-pvc sub-command #276

Open

satoru-takeuchi force-pushed the rook-add-flatten-rbd-pvc-command branch from 556c9e3 to 67fd2bf Compare May 1, 2024 08:08

satoru-takeuchi force-pushed the rook-add-flatten-rbd-pvc-command branch 8 times, most recently from c17aa32 to 66a2398 Compare May 1, 2024 10:35

satoru-takeuchi removed the do-not-merge label May 1, 2024

satoru-takeuchi force-pushed the rook-add-flatten-rbd-pvc-command branch 2 times, most recently from 0a12e1f to 77541b1 Compare May 1, 2024 10:39

satoru-takeuchi marked this pull request as ready for review May 1, 2024 10:39

satoru-takeuchi force-pushed the rook-add-flatten-rbd-pvc-command branch 4 times, most recently from c9a8056 to c934d63 Compare May 1, 2024 12:09

satoru-takeuchi requested a review from Madhu-1 May 1, 2024 12:28

Madhu-1 reviewed May 2, 2024

View reviewed changes

satoru-takeuchi force-pushed the rook-add-flatten-rbd-pvc-command branch 2 times, most recently from c74c409 to 7207eab Compare May 9, 2024 00:50

satoru-takeuchi requested a review from Madhu-1 May 9, 2024 01:46

Madhu-1 reviewed May 14, 2024

View reviewed changes

satoru-takeuchi force-pushed the rook-add-flatten-rbd-pvc-command branch 2 times, most recently from 3f27f53 to 48302e8 Compare May 15, 2024 05:36

Madhu-1 reviewed May 15, 2024

View reviewed changes

Madhu-1 approved these changes May 15, 2024

View reviewed changes

satoru-takeuchi force-pushed the rook-add-flatten-rbd-pvc-command branch from 48302e8 to b2473fb Compare May 16, 2024 01:03

subhamkrai reviewed May 17, 2024

View reviewed changes

core: add flatten-rbd-pvc command

f003e54

Add flatten-rbd-pvc command to accomplish the following things: - Flatten an RBD image corrensponding to the target PVC. - Remove the temporary RBD image too. Closes: rook#222 Signed-off-by: Satoru Takeuchi <satoru.takeuchi@gmail.com>

satoru-takeuchi force-pushed the rook-add-flatten-rbd-pvc-command branch from b2473fb to f003e54 Compare May 22, 2024 00:40

satoru-takeuchi requested a review from subhamkrai May 22, 2024 00:59

subhamkrai approved these changes May 22, 2024

View reviewed changes

subhamkrai merged commit c7c01c5 into rook:master May 22, 2024
6 checks passed

satoru-takeuchi mentioned this pull request May 22, 2024

Enrich e2e test to flatten-rbd-pvc command #294

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: add flatten-rbd-pvc command #229

core: add flatten-rbd-pvc command #229

satoru-takeuchi commented Jan 26, 2024 •

edited

Madhu-1 left a comment

satoru-takeuchi commented Feb 1, 2024

subhamkrai commented Mar 20, 2024

satoru-takeuchi commented Apr 2, 2024

satoru-takeuchi commented Apr 5, 2024

satoru-takeuchi commented Apr 10, 2024

satoru-takeuchi commented Apr 10, 2024

subhamkrai commented Apr 29, 2024

Madhu-1 commented Apr 30, 2024

satoru-takeuchi commented May 1, 2024

Madhu-1 left a comment

Madhu-1 May 2, 2024

satoru-takeuchi May 9, 2024 •

edited

satoru-takeuchi commented May 8, 2024

Madhu-1 left a comment •

edited

Madhu-1 May 14, 2024

satoru-takeuchi May 15, 2024

Madhu-1 May 14, 2024

satoru-takeuchi May 15, 2024

Madhu-1 May 15, 2024

satoru-takeuchi May 15, 2024

satoru-takeuchi May 16, 2024 •

edited

Madhu-1 May 15, 2024

Madhu-1 left a comment

subhamkrai May 17, 2024

satoru-takeuchi May 22, 2024

subhamkrai May 22, 2024

core: add flatten-rbd-pvc command #229

core: add flatten-rbd-pvc command #229

Conversation

satoru-takeuchi commented Jan 26, 2024 • edited

Madhu-1 left a comment

Choose a reason for hiding this comment

satoru-takeuchi commented Feb 1, 2024

subhamkrai commented Mar 20, 2024

satoru-takeuchi commented Apr 2, 2024

satoru-takeuchi commented Apr 5, 2024

satoru-takeuchi commented Apr 10, 2024

satoru-takeuchi commented Apr 10, 2024

subhamkrai commented Apr 29, 2024

Madhu-1 commented Apr 30, 2024

satoru-takeuchi commented May 1, 2024

Madhu-1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satoru-takeuchi May 9, 2024 • edited

Choose a reason for hiding this comment

satoru-takeuchi commented May 8, 2024

Madhu-1 left a comment • edited

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

satoru-takeuchi May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Madhu-1 left a 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

satoru-takeuchi commented Jan 26, 2024 •

edited

satoru-takeuchi May 9, 2024 •

edited

Madhu-1 left a comment •

edited

satoru-takeuchi May 16, 2024 •

edited