Link PVCs and PVs in VolumeGroupSnapshot and VolumeGroupSnapshotContent #1069

leonardoce · 2024-04-18T11:46:05Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Following #1068 and the discussion in #969, this PR improves the snapshot controller and the CSI snapshotter sidecar to track which PVC and PVs are snapshotted by a VolumeGroupSnapshot.

With this PR applied, a VolumeGroupSnapshot will look like:

apiVersion: groupsnapshot.storage.k8s.io/v1alpha1
kind: VolumeGroupSnapshot
metadata:
  finalizers:
  - groupsnapshot.storage.kubernetes.io/volumegroupsnapshot-bound-protection
  generation: 1
  name: new-groupsnapshot
  namespace: default
  resourceVersion: "64488"
  uid: 02f28293-ff7e-4d1c-8ea8-c64fb2b45a26
spec:
  source:
    selector:
      matchLabels:
        cnpg.io/instanceName: cluster-example-1
  volumeGroupSnapshotClassName: csi-hostpath-groupsnapclass
status:
  boundVolumeGroupSnapshotContentName: groupsnapcontent-02f28293-ff7e-4d1c-8ea8-c64fb2b45a26
  creationTime: "2024-05-14T10:02:50Z"
  pvcVolumeSnapshotRefList:
  - persistentVolumeClaimRef:
      name: cluster-example-1
    volumeSnapshotRef:
      name: snapshot-219cbb76259445b14e5a1f0e11008cb976aacfb23bada2e4346d7d4da81d2d2b-2024-05-14-10.2.52
  - persistentVolumeClaimRef:
      name: cluster-example-1-wal
    volumeSnapshotRef:
      name: snapshot-8b65beb9db8573e67e31c3120401d13f1ccd83bf4974b546970799ce0b0016c3-2024-05-14-10.2.52
  readyToUse: true

The corresponding VolumeGroupSnapshotContent will look like:

apiVersion: groupsnapshot.storage.k8s.io/v1alpha1
kind: VolumeGroupSnapshotContent
metadata:
  creationTimestamp: "2024-05-14T10:02:50Z"
  finalizers:
  - groupsnapshot.storage.kubernetes.io/volumegroupsnapshotcontent-bound-protection
  generation: 1
  name: groupsnapcontent-02f28293-ff7e-4d1c-8ea8-c64fb2b45a26
  resourceVersion: "64482"
  uid: 7671dd61-0af3-4e53-a389-d7528fca5260
spec:
  deletionPolicy: Delete
  driver: hostpath.csi.k8s.io
  source:
    volumeHandles:
    - f4416324-1165-11ef-91b4-22776546f286
    - f440f487-1165-11ef-91b4-22776546f286
  volumeGroupSnapshotClassName: csi-hostpath-groupsnapclass
  volumeGroupSnapshotRef:
    apiVersion: groupsnapshot.storage.k8s.io/v1alpha1
    kind: VolumeGroupSnapshot
    name: new-groupsnapshot
    namespace: default
    resourceVersion: "64440"
    uid: 02f28293-ff7e-4d1c-8ea8-c64fb2b45a26
status:
  creationTime: 1715680970153290824
  pvVolumeSnapshotContentList:
  - persistentVolumeRef:
      name: pvc-e92d8114-160f-4a59-8c29-bb6fb42e345f
    volumeSnapshotContentRef:
      name: snapcontent-219cbb76259445b14e5a1f0e11008cb976aacfb23bada2e4346d7d4da81d2d2b-2024-05-14-10.2.52
  - persistentVolumeRef:
      name: pvc-7b7b185e-2c35-4973-959e-15593d5f74a5
    volumeSnapshotContentRef:
      name: snapcontent-8b65beb9db8573e67e31c3120401d13f1ccd83bf4974b546970799ce0b0016c3-2024-05-14-10.2.52
  readyToUse: true
  volumeGroupSnapshotHandle: 1f3e6247-11d9-11ef-a7bc-b25da50ff257

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Testing

To test this patch, I used the CSI host path driver.
Having two PVCs and creating a VolumeGroupSnapshot matching them is enough to trigger the process.

Does this PR introduce a user-facing change?:

Link the snapshotted PVCs and the corresponding PVs in VolumeGroupSnapshot and VolumeGroupSnapshotContent to make restoring data easier.

k8s-ci-robot · 2024-04-18T11:46:14Z

Hi @leonardoce. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

xing-yang · 2024-04-18T13:14:51Z

/ok-to-test

xing-yang · 2024-04-18T13:14:55Z

/assign

nixpanic · 2024-04-18T15:08:36Z

Once successfully created, maybe remove the groupsnapshot.storage.kubernetes.io/info annotation?

leonardoce · 2024-04-18T15:35:10Z

Once successfully created, maybe remove the groupsnapshot.storage.kubernetes.io/info annotation?

Yes. Nice idea, @nixpanic.

All the information in that annotation is in the status section of both VolumeGroupSnapshot and VolumeGroupSnapshotStatus. But this would require a new API server call.

More importantly, updateGroupSnapshotContentStatus and updateGroupSnapshotStatus won't be idempotent anymore as the annotation will be missing after removal. I need more experience in this codebase to judge this properly.

What's your opinion @xing-yang?

leonardoce · 2024-04-18T15:42:45Z

Thinking aloud, the VolumeGroupSnapshotContent resource would be more beautiful without that annotation...

xing-yang · 2024-04-19T17:55:53Z

/hold

xing-yang · 2024-04-19T18:03:32Z

I'd prefer the annotation gets removed.

leonardoce · 2024-05-14T10:10:36Z

I rebased it on the new master, removed the annotation, and updated the description of this PR with the latest resource definitions.

xing-yang · 2024-05-15T02:00:06Z

Can you update the PR description? It still talks about the groupsnapshot.storage.kubernetes.io/info annotation. Can you also update the "Special notes for your reviewer" section?

xing-yang · 2024-05-15T02:04:21Z

I see that the annotation is still in the code. Can you please squash the commits? It will be easier to see what code is removed this way.

pkg/utils/util.go

pkg/common-controller/groupsnapshot_controller_helper.go

leonardoce · 2024-05-15T09:50:11Z

You're correct, @xing-yang.
The annotation is still used; it is only removed when the VolumeGroupSnapshot is marked ready.
By having that annotation, the patch avoids hitting the API server.

I've another implementation that doesn't use annotation and queries the API server when needed.
Do you prefer that one?

xing-yang · 2024-05-15T15:09:35Z

You're correct, @xing-yang.
The annotation is still used; it is only removed when the VolumeGroupSnapshot is marked ready.
By having that annotation, the patch avoids hitting the API server.

I've another implementation that doesn't use annotation and queries the API server when needed.
Do you prefer that one?

Yes, I'd prefer not to use the annotation. Thanks.

leonardoce · 2024-05-15T15:41:33Z

Done @xing-yang. Thank you.

pkg/utils/pvs.go

pkg/utils/pvs_test.go

pkg/common-controller/groupsnapshot_controller_helper.go

xing-yang · 2024-05-15T15:55:42Z

pkg/common-controller/groupsnapshot_controller_helper.go

+					Name: volumeSnapshotContent.Spec.VolumeSnapshotRef.Name,
+				},
+				PersistentVolumeClaimRef: v1.LocalObjectReference{
+					Name: pvcName,


If pv.Spec.ClaimRef is nil above, this will be an empty string?

Yes, it will. And this is equivalent to an empty LocalObjectReference.
And since the VolumeSnapshotRef itself is marked as omitempty, it will be omitted.

But I realized now that this is not evident from the code. Let me express it more clearly.

Is it better now?

pkg/sidecar-controller/groupsnapshot_helper.go

pkg/common-controller/groupsnapshot_controller_helper.go

This use the update API to set `persistentVolumeClaimRef` in `VolumeGroupSnapshot` and `persistentVolumeName` in `VolumeGroupSnapshotContent` to the corresponding objects. This makes restoring volumes from a VolumeGroupSnapshot easier. Related: kubernetes-csi#969

xing-yang · 2024-05-17T14:13:01Z

/lgtm
/approve

xing-yang · 2024-05-17T14:13:07Z

/hold cancel

k8s-ci-robot · 2024-05-17T14:13:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leonardoce, sxd, xing-yang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [xing-yang]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 18, 2024

k8s-ci-robot requested review from gnufied and mauriciopoppe April 18, 2024 11:46

sxd approved these changes Apr 18, 2024

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 18, 2024

k8s-ci-robot assigned xing-yang Apr 18, 2024

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 19, 2024

leonardoce mentioned this pull request May 10, 2024

REQUEST: New membership for leonardoce kubernetes/org#4940

Closed

11 tasks

leonardoce force-pushed the dev/easier-restore branch from 8cf3ea5 to 71ea1e2 Compare May 14, 2024 07:43

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 14, 2024

xing-yang reviewed May 15, 2024

View reviewed changes

leonardoce force-pushed the dev/easier-restore branch from c69d79b to bb3b87a Compare May 15, 2024 07:26

leonardoce force-pushed the dev/easier-restore branch from bb3b87a to 8dfe567 Compare May 15, 2024 15:35

xing-yang reviewed May 15, 2024

View reviewed changes

leonardoce force-pushed the dev/easier-restore branch from 8dfe567 to d37bf90 Compare May 15, 2024 19:17

leonardoce requested a review from xing-yang May 15, 2024 20:42

xing-yang reviewed May 17, 2024

View reviewed changes

pkg/common-controller/groupsnapshot_controller_helper.go Show resolved Hide resolved

pkg/common-controller/groupsnapshot_controller_helper.go Outdated Show resolved Hide resolved

pkg/common-controller/groupsnapshot_controller_helper.go Show resolved Hide resolved

leonardoce force-pushed the dev/easier-restore branch from d37bf90 to 3f8b79d Compare May 17, 2024 07:10

leonardoce requested a review from xing-yang May 17, 2024 07:11

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 17, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 17, 2024

k8s-ci-robot merged commit 52b72d3 into kubernetes-csi:master May 17, 2024
8 checks passed

dfajmon mentioned this pull request Jun 25, 2024

STOR-1593: Rebase external-snapshotter to v8.0.1 for OCP 4.17 openshift/csi-external-snapshotter#156

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Link PVCs and PVs in VolumeGroupSnapshot and VolumeGroupSnapshotContent #1069

Link PVCs and PVs in VolumeGroupSnapshot and VolumeGroupSnapshotContent #1069

leonardoce commented Apr 18, 2024 •

edited

Loading

k8s-ci-robot commented Apr 18, 2024

xing-yang commented Apr 18, 2024

xing-yang commented Apr 18, 2024

nixpanic commented Apr 18, 2024

leonardoce commented Apr 18, 2024

leonardoce commented Apr 18, 2024

xing-yang commented Apr 19, 2024

xing-yang commented Apr 19, 2024

leonardoce commented May 14, 2024

xing-yang commented May 15, 2024 •

edited

Loading

xing-yang commented May 15, 2024

leonardoce commented May 15, 2024

xing-yang commented May 15, 2024

leonardoce commented May 15, 2024

xing-yang May 15, 2024

leonardoce May 15, 2024 •

edited

Loading

leonardoce May 15, 2024

xing-yang commented May 17, 2024

xing-yang commented May 17, 2024

k8s-ci-robot commented May 17, 2024

Link PVCs and PVs in VolumeGroupSnapshot and VolumeGroupSnapshotContent #1069

Link PVCs and PVs in VolumeGroupSnapshot and VolumeGroupSnapshotContent #1069

Conversation

leonardoce commented Apr 18, 2024 • edited Loading

k8s-ci-robot commented Apr 18, 2024

xing-yang commented Apr 18, 2024

xing-yang commented Apr 18, 2024

nixpanic commented Apr 18, 2024

leonardoce commented Apr 18, 2024

leonardoce commented Apr 18, 2024

xing-yang commented Apr 19, 2024

xing-yang commented Apr 19, 2024

leonardoce commented May 14, 2024

xing-yang commented May 15, 2024 • edited Loading

xing-yang commented May 15, 2024

leonardoce commented May 15, 2024

xing-yang commented May 15, 2024

leonardoce commented May 15, 2024

xing-yang May 15, 2024

Choose a reason for hiding this comment

leonardoce May 15, 2024 • edited Loading

Choose a reason for hiding this comment

leonardoce May 15, 2024

Choose a reason for hiding this comment

xing-yang commented May 17, 2024

xing-yang commented May 17, 2024

k8s-ci-robot commented May 17, 2024

leonardoce commented Apr 18, 2024 •

edited

Loading

xing-yang commented May 15, 2024 •

edited

Loading

leonardoce May 15, 2024 •

edited

Loading