DeleteVolumeGroupSnapshot API - part 2 #952

RaunakShah · 2023-11-07T09:57:47Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

This PR is a follow up to #882

Individual snapshots are also deleted as part of the group volume snapshot delete API.
Also prevent users from deleting individual snapshots when a group snapshot exists. There is no finaliser added as part of this change, just a look up to verify if the group snapshot that this snapshot points to exists or not.
Refactor some code

This PR also reverts de93167

There will be a follow up PR dedicated to adding a finaliser to volume snapshot objects that are part of the group, as well as some logic in the web hook to disallow individual volume snapshot deletion as long as the group snapshot exists

Testing

Create a volume group snapshot

% kubectl create -f volumegroupsnapshot-example.yaml 
volumegroupsnapshot.groupsnapshot.storage.k8s.io/my-group-snapshot created

Manually delete snapshot part of the group and verify that it is not deleted

% kubectl get vs
NAME                                                                                          READYTOUSE   SOURCEPVC   SOURCESNAPSHOTCONTENT                                                                            RESTORESIZE   SNAPSHOTCLASS   SNAPSHOTCONTENT                                                                                  CREATIONTIME   AGE
snapshot-e88ff8f7967905f13666c41d3d894c5a71ed65f3da84d7b388229053b0acd4c2-2023-11-07-9.54.8   true                     snapcontent-e88ff8f7967905f13666c41d3d894c5a71ed65f3da84d7b388229053b0acd4c2-2023-11-07-9.54.8   1Gi                           snapcontent-e88ff8f7967905f13666c41d3d894c5a71ed65f3da84d7b388229053b0acd4c2-2023-11-07-9.54.8   46s            46s
snapshot-fc153da77b23efc1f2d4093b00ec72270b02f4c76a011f1dc3d9621580aad564-2023-11-07-9.54.8   true                     snapcontent-fc153da77b23efc1f2d4093b00ec72270b02f4c76a011f1dc3d9621580aad564-2023-11-07-9.54.8   1Gi                           snapcontent-fc153da77b23efc1f2d4093b00ec72270b02f4c76a011f1dc3d9621580aad564-2023-11-07-9.54.8   46s            46s
% kubectl delete vs snapshot-e88ff8f7967905f13666c41d3d894c5a71ed65f3da84d7b388229053b0acd4c2-2023-11-07-9.54.8
volumesnapshot.snapshot.storage.k8s.io "snapshot-e88ff8f7967905f13666c41d3d894c5a71ed65f3da84d7b388229053b0acd4c2-2023-11-07-9.54.8" deleted
^C

% kubectl describe vs snapshot-e88ff8f7967905f13666c41d3d894c5a71ed65f3da84d7b388229053b0acd4c2-2023-11-07-9.54.8
Name:         snapshot-e88ff8f7967905f13666c41d3d894c5a71ed65f3da84d7b388229053b0acd4c2-2023-11-07-9.54.8
Namespace:    default
Labels:       volumeGroupSnapshotName=my-group-snapshot
Annotations:  <none>
API Version:  snapshot.storage.k8s.io/v1
Kind:         VolumeSnapshot
Metadata:
  Creation Timestamp:             2023-11-07T09:54:08Z
  Deletion Grace Period Seconds:  0
  Deletion Timestamp:             2023-11-07T09:54:59Z
  Finalizers:
    snapshot.storage.kubernetes.io/volumesnapshot-as-source-protection
    snapshot.storage.kubernetes.io/volumesnapshot-bound-protection
  Generation:  2
  Managed Fields:
    API Version:  snapshot.storage.k8s.io/v1
    Fields Type:  FieldsV1
    fieldsV1:
      f:metadata:
        f:labels:
          .:
          f:volumeGroupSnapshotName:
      f:spec:
        .:
        f:source:
          .:
          f:volumeSnapshotContentName:
    Manager:      csi-snapshotter
    Operation:    Update
    Time:         2023-11-07T09:54:08Z
    API Version:  snapshot.storage.k8s.io/v1
    Fields Type:  FieldsV1
    fieldsV1:
      f:status:
        .:
        f:boundVolumeSnapshotContentName:
        f:creationTime:
        f:readyToUse:
        f:restoreSize:
        f:volumeGroupSnapshotName:
    Manager:      snapshot-controller
    Operation:    Update
    Subresource:  status
    Time:         2023-11-07T09:54:10Z
    API Version:  snapshot.storage.k8s.io/v1
    Fields Type:  FieldsV1
    fieldsV1:
      f:metadata:
        f:finalizers:
          .:
          v:"snapshot.storage.kubernetes.io/volumesnapshot-as-source-protection":
          v:"snapshot.storage.kubernetes.io/volumesnapshot-bound-protection":
    Manager:         snapshot-controller
    Operation:       Update
    Time:            2023-11-07T09:54:11Z
  Resource Version:  1418460
  UID:               ab02688f-6c5f-4c57-99f5-738d688aec99
Spec:
  Source:
    Volume Snapshot Content Name:  snapcontent-e88ff8f7967905f13666c41d3d894c5a71ed65f3da84d7b388229053b0acd4c2-2023-11-07-9.54.8
Status:
  Bound Volume Snapshot Content Name:  snapcontent-e88ff8f7967905f13666c41d3d894c5a71ed65f3da84d7b388229053b0acd4c2-2023-11-07-9.54.8
  Creation Time:                       2023-11-07T09:54:08Z
  Ready To Use:                        true
  Restore Size:                        1Gi
  Volume Group Snapshot Name:          my-group-snapshot
Events:
  Type     Reason               Age               From                 Message
  ----     ------               ----              ----                 -------
  Normal   SnapshotCreated      63s               snapshot-controller  Snapshot default/snapshot-e88ff8f7967905f13666c41d3d894c5a71ed65f3da84d7b388229053b0acd4c2-2023-11-07-9.54.8 was successfully created by the CSI driver.
  Normal   SnapshotReady        63s               snapshot-controller  Snapshot default/snapshot-e88ff8f7967905f13666c41d3d894c5a71ed65f3da84d7b388229053b0acd4c2-2023-11-07-9.54.8 is ready to use.
  Warning  SnapshotDeleteError  6s (x4 over 13s)  snapshot-controller  failed to delete snapshot API default/snapshot-e88ff8f7967905f13666c41d3d894c5a71ed65f3da84d7b388229053b0acd4c2-2023-11-07-9.54.8 object as it belongs to group snapshot default/my-group-snapshot

Delete the group snapshot and verify all resources are cleaned up

% kubectl delete -f volumegroupsnapshot-example.yaml 
volumegroupsnapshot.groupsnapshot.storage.k8s.io "my-group-snapshot" deleted
% kubectl get vgs
No resources found in default namespace.
% kubectl get vgsc
No resources found
 % kubectl get vsc
No resources found
% kubectl get vs 
No resources found in default namespace.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Delete individual snapshots as part of volume group snapshots delete API

RaunakShah · 2023-11-08T16:50:42Z

/assign @xing-yang

client/go.mod

cmd/csi-snapshotter/main.go

xing-yang · 2023-11-10T07:46:09Z

cmd/snapshot-controller/main.go

@@ -72,8 +72,7 @@ var (
 	retryIntervalMax              = flag.Duration("retry-interval-max", 5*time.Minute, "Maximum retry interval of failed volume snapshot creation or deletion. Default is 5 minutes.")
 	enableDistributedSnapshotting = flag.Bool("enable-distributed-snapshotting", false, "Enables each node to handle snapshotting for the local volumes created on that node")
 	preventVolumeModeConversion   = flag.Bool("prevent-volume-mode-conversion", false, "Prevents an unauthorised user from modifying the volume mode when creating a PVC from an existing VolumeSnapshot.")
-	// TODO(xing-yang): Enable enableVolumeGroupSnapshots when the feature is fully implemented
-	// enableVolumeGroupSnapshots    = flag.Bool("enable-volume-group-snapshots", false, "Enables the volume group snapshot feature, allowing the user to create snapshots of groups of volumes.")
+	enableVolumeGroupSnapshots    = flag.Bool("enable-volume-group-snapshots", false, "Enables the volume group snapshot feature, allowing the user to create snapshots of groups of volumes.")


create snapshots of groups of volumes -> create a snapshot of a group of volumes

pkg/common-controller/groupsnapshot_controller_helper.go

xing-yang · 2023-11-10T07:57:49Z

pkg/common-controller/snapshot_controller.go

+		if err == nil {
+			msg := fmt.Sprintf("failed to delete snapshot API %s object as it belongs to group snapshot %s", utils.SnapshotKey(snapshot), utils.GroupSnapshotKey(groupSnapshot))
+			ctrl.eventRecorder.Event(snapshot, v1.EventTypeWarning, "SnapshotDeleteError", msg)
+			return fmt.Errorf(msg)


I think we either need to add a webhook to prevent the deletion from happening in the first place or add a finalizer. I'm fine to add that in a followup PR. Can you add a note in your PR description?

failed to delete snapshot API %s object as it belongs to group snapshot %s -> deletion of the individual volume snapshot %s is not allowed as it belongs to group snapshot %s. Delete the group snapshot will trigger the deletion of all the individual volume snapshots that are part of the group.

Also add an error msg.

Amongst the other changes I had a tough time testing out the finaliser code, so I removed it. I can address that in a follow up PR. We definitely need some logic in case the web hook is not deployed. I'll change the PR description.

Sure. Yes, finalizer is better as the webhook may not be deployed.

pkg/common-controller/snapshot_controller.go

pkg/sidecar-controller/snapshot_controller.go

leonardoce · 2023-11-13T15:33:39Z

Hi! I was just thinking that we could include the update of the dependencies in a separate PR.
By doing that, this PR would be easier to read.

xing-yang · 2023-11-13T15:38:28Z

Hi! I was just thinking that we could include the update of the dependencies in a separate PR.
By doing that, this PR would be easier to read.

Thanks @leonardoce for the review comments.

@RaunakShah Can you please split the client side changes into a separate PR? Thanks.

xing-yang · 2023-11-16T14:20:57Z

Can you please squash the commits and rebase to get the latest Trivy fix?

…te API and prevent users from deleting individual volume snapshots if it is part of an existing group snapshot Also revert commit bb29899.

RaunakShah · 2023-11-17T08:55:21Z

Can you please squash the commits and rebase to get the latest Trivy fix?

@xing-yang Done!

xing-yang · 2023-11-17T15:05:57Z

/lgtm
/approve

k8s-ci-robot · 2023-11-17T15:06:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RaunakShah, 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 requested review from andyzhangx and xing-yang November 7, 2023 09:58

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 7, 2023

RaunakShah changed the title ~~[WIP] DeleteVolumeGroupSnapshot API - part 2~~ [WIP][Do not review] DeleteVolumeGroupSnapshot API - part 2 Nov 8, 2023

RaunakShah mentioned this pull request Nov 8, 2023

Update go mod and vendor in client dir to 0.28.0 #955

Merged

RaunakShah force-pushed the deletegroupsnapshot-part-2 branch from 5f809cc to 7797aa7 Compare November 8, 2023 16:39

RaunakShah changed the title ~~[WIP][Do not review] DeleteVolumeGroupSnapshot API - part 2~~ DeleteVolumeGroupSnapshot API - part 2 Nov 8, 2023

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 8, 2023

k8s-ci-robot assigned xing-yang Nov 8, 2023

xing-yang reviewed Nov 10, 2023

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 16, 2023

RaunakShah force-pushed the deletegroupsnapshot-part-2 branch from 12c9f11 to 8a9f7e7 Compare November 16, 2023 05:57

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 16, 2023

RaunakShah force-pushed the deletegroupsnapshot-part-2 branch from 8a9f7e7 to ad67ef3 Compare November 16, 2023 06:02

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 16, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 16, 2023

RaunakShah force-pushed the deletegroupsnapshot-part-2 branch from 71266eb to c5a433b Compare November 17, 2023 08:51

RaunakShah force-pushed the deletegroupsnapshot-part-2 branch from 708674a to 060ee23 Compare November 17, 2023 08:54

Delete individual volume snapshots as part of the group snapshot dele…

060ee23

…te API and prevent users from deleting individual volume snapshots if it is part of an existing group snapshot Also revert commit bb29899.

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

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

k8s-ci-robot merged commit c68695b into kubernetes-csi:master Nov 17, 2023
8 checks passed

nixpanic mentioned this pull request Nov 24, 2023

Snapshot Controller and sidecar: DeleteVolumeGroupSnapshot #833

Closed

mpatlasov mentioned this pull request Feb 5, 2024

STOR-1700: Rebase external-snapshotter to v7.0.0 to get VolumeGroupSnapshot openshift/csi-external-snapshotter#139

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DeleteVolumeGroupSnapshot API - part 2 #952

DeleteVolumeGroupSnapshot API - part 2 #952

RaunakShah commented Nov 7, 2023 •

edited

RaunakShah commented Nov 8, 2023

xing-yang Nov 10, 2023

xing-yang Nov 10, 2023 •

edited

RaunakShah Nov 16, 2023

xing-yang Nov 16, 2023

leonardoce commented Nov 13, 2023

xing-yang commented Nov 13, 2023

xing-yang commented Nov 16, 2023

RaunakShah commented Nov 17, 2023

xing-yang commented Nov 17, 2023

k8s-ci-robot commented Nov 17, 2023

DeleteVolumeGroupSnapshot API - part 2 #952

DeleteVolumeGroupSnapshot API - part 2 #952

Conversation

RaunakShah commented Nov 7, 2023 • edited

RaunakShah commented Nov 8, 2023

xing-yang Nov 10, 2023

Choose a reason for hiding this comment

xing-yang Nov 10, 2023 • edited

Choose a reason for hiding this comment

RaunakShah Nov 16, 2023

Choose a reason for hiding this comment

xing-yang Nov 16, 2023

Choose a reason for hiding this comment

leonardoce commented Nov 13, 2023

xing-yang commented Nov 13, 2023

xing-yang commented Nov 16, 2023

RaunakShah commented Nov 17, 2023

xing-yang commented Nov 17, 2023

k8s-ci-robot commented Nov 17, 2023

RaunakShah commented Nov 7, 2023 •

edited

xing-yang Nov 10, 2023 •

edited