Missing group name label from VolumeSnapshots that are part of a statically provisioned group snapshot

[xing-yang]

What happened: VolumeSnapshots from statically provisioned VolumeGroupSnapshot do not have the group name label. Similarly the label is missing fro VolumeSnapshotContents. The label is helpful in identifying what individual volume snapshots are part of a group snapshot.

Here's an example VolumeSnapshot and VolumeSnapshotContent.

- apiVersion: snapshot.storage.k8s.io/v1
  kind: VolumeSnapshot
  metadata:
    annotations:
      kubectl.kubernetes.io/last-applied-configuration: |
        {"apiVersion":"snapshot.storage.k8s.io/v1","kind":"VolumeSnapshot","metadata":{"annotations":{},"name":"static-snapshot1","namespace":"default"},"spec":{"source":{"volumeSnapshotContentName":"static-content1"}}}
    creationTimestamp: "2024-11-23T03:09:41Z"
    finalizers:
    - snapshot.storage.kubernetes.io/volumesnapshot-as-source-protection
    - snapshot.storage.kubernetes.io/volumesnapshot-bound-protection
    generation: 1
    name: static-snapshot1
    namespace: default
    resourceVersion: "104810"
    uid: 4c891ced-39d2-44df-9ea3-3e736f92c8da
  spec:
    source:
      volumeSnapshotContentName: static-content1
  status:
    boundVolumeSnapshotContentName: static-content1
    creationTime: "2024-11-23T02:01:52Z"
    readyToUse: true
    restoreSize: 100Mi
    volumeGroupSnapshotName: static-group-snapshot

- apiVersion: snapshot.storage.k8s.io/v1
  kind: VolumeSnapshotContent
  metadata:
    annotations:
      kubectl.kubernetes.io/last-applied-configuration: |
        {"apiVersion":"snapshot.storage.k8s.io/v1","kind":"VolumeSnapshotContent","metadata":{"annotations":{},"name":"static-content1"},"spec":{"deletionPolicy":"Delete","driver":"hostpath.csi.k8s.io","source":{"snapshotHandle":"e8779147-a93e-11ef-9549-66940726f2fd"},"volumeSnapshotRef":{"name":"static-snapshot1","namespace":"default"}}}
    creationTimestamp: "2024-11-23T03:09:41Z"
    finalizers:
    - snapshot.storage.kubernetes.io/volumesnapshotcontent-bound-protection
    generation: 2
    name: static-content1
    resourceVersion: "104356"
    uid: e0664c69-954f-4aa7-8b03-dd5ca37e5ee3
  spec:
    deletionPolicy: Delete
    driver: hostpath.csi.k8s.io
    source:
      snapshotHandle: e8779147-a93e-11ef-9549-66940726f2fd
    volumeSnapshotRef:
      name: static-snapshot1
      namespace: default
      uid: 4c891ced-39d2-44df-9ea3-3e736f92c8da
  status:
    creationTime: 1732327312499129161
    readyToUse: true
    restoreSize: 104857600
    snapshotHandle: e8779147-a93e-11ef-9549-66940726f2fd
    volumeGroupSnapshotHandle: e8779136-a93e-11ef-9549-66940726f2fd

What you expected to happen:

How to reproduce it:

Anything else we need to know?:

Environment:

• Driver version:
• Kubernetes version (use kubectl version):
• OS (e.g. from /etc/os-release):
• Kernel (e.g. uname -a):
• Install tools:
• Others:

[yati1998]

 kubectl get volumesnapshot snapshot-371159eb708a91a37c971c3acb1b433658a89b0bf02107a015683d525a6429a3 -oyaml
apiVersion: snapshot.storage.k8s.io/v1
kind: VolumeSnapshot
metadata:
  creationTimestamp: "2024-11-21T15:06:49Z"
  finalizers:
  - snapshot.storage.kubernetes.io/volumesnapshot-in-group-protection
  - snapshot.storage.kubernetes.io/volumesnapshot-as-source-protection
  generation: 1
  labels:
    groupsnapshot.storage.k8s.io/volumeGroupSnapshotName: new-groupsnapshot-sc
  name: snapshot-371159eb708a91a37c971c3acb1b433658a89b0bf02107a015683d525a6429a3
  namespace: default
  resourceVersion: "45831"
  uid: 740236fb-2f84-4352-b9e1-075b63d8d20d
spec:
  source:
    persistentVolumeClaimName: csi-pvc-sc
status:
  boundVolumeSnapshotContentName: snapcontent-371159eb708a91a37c971c3acb1b433658a89b0bf02107a015683d525a6429a3
  creationTime: "2024-11-21T15:06:48Z"
  readyToUse: true
  restoreSize: 1Gi
  volumeGroupSnapshotName: new-groupsnapshot-sc

This is the example of dynamically created VS and I dn't see handle here. VolumeSnapshotcontent is supossed to have the handle as it has in above example.

cc @xing-yang

[xing-yang]

@yati1998 I don't know what you meant. The label is visible in your VolumeSnapshot output.

labels:
    groupsnapshot.storage.k8s.io/volumeGroupSnapshotName: new-groupsnapshot-sc

[Madhu-1]

@xing-yang @leonardoce wont we get into the length problem if we use group name as label value here?

[Madhu-1]

I1125 17:35:22.778063       1 groupsnapshot_controller_helper.go:538] createSnapshotsForGroupSnapshotContent[groupsnapcontent-b0b56c91-a399-44bf-9edf-c56a99cd1052]: creating volumesnapshots and volumesnapshotcontent for group snapshot content
I1125 17:35:22.778161       1 groupsnapshot_controller_helper.go:591] createSnapshotsForGroupSnapshotContent: set annotation [snapshot.storage.kubernetes.io/deletion-secret-name] on volume snapshot content [snapcontent-7a11e173d5066084f2dd45dcffff81b977cefa098d80fe387e08aba6ecd3b5a0].
I1125 17:35:22.778183       1 groupsnapshot_controller_helper.go:594] createSnapshotsForGroupSnapshotContent: set annotation [snapshot.storage.kubernetes.io/deletion-secret-namespace] on volume snapshot content [snapcontent-7a11e173d5066084f2dd45dcffff81b977cefa098d80fe387e08aba6ecd3b5a0].
I1125 17:35:22.789260       1 groupsnapshot_controller_helper.go:465] createSnapshotsForGroupSnapshotContent[groupsnapcontent-b0b56c91-a399-44bf-9edf-c56a99cd1052]: failed to create snapshots and snapshotcontents for group snapshot my-group-snapshot-jsdfjhsjfhsfsdakjfdskajfdskl-jsadjfklsajfksjfksajdkljasdf-sdfjksdfjksadjfkljasdkljfds-asdfjksdjfklsdjfkasljfkasdfjaksldfjkasldfjskadlfjadsk: createSnapshotsForGroupSnapshotContent: creating volumesnapshot VolumeSnapshot.snapshot.storage.k8s.io "snapshot-7a11e173d5066084f2dd45dcffff81b977cefa098d80fe387e08aba6ecd3b5a0" is invalid: metadata.labels: Invalid value: "my-group-snapshot-jsdfjhsjfhsfsdakjfdskajfdskl-jsadjfklsajfksjfksajdkljasdf-sdfjksdfjksadjfkljasdkljfds-asdfjksdjfklsdjfkasljfkasdfjaksldfjkasldfjskadlfjadsk": must be no more than 63 characters
E1125 17:35:22.789296       1 groupsnapshot_controller_helper.go:256] could not sync group snapshot "default/my-group-snapshot-jsdfjhsjfhsfsdakjfdskajfdskl-jsadjfklsajfksjfksajdkljasdf-sdfjksdfjksadjfkljasdkljfds-asdfjksdjfklsdjfkasljfkasdfjaksldfjkasldfjskadlfjadsk": createSnapshotsForGroupSnapshotContent: creating volumesnapshot VolumeSnapshot.snapshot.storage.k8s.io "snapshot-7a11e173d5066084f2dd45dcffff81b977cefa098d80fe387e08aba6ecd3b5a0" is invalid: metadata.labels: Invalid value: "my-group-snapshot-jsdfjhsjfhsfsdakjfdskajfdskl-jsadjfklsajfksjfksajdkljasdf-sdfjksdfjksadjfkljasdkljfds-asdfjksdjfklsdjfkasljfkasdfjaksldfjkasldfjskadlfjadsk": must be no more than 63 characters
I1125 17:35:22.789322       1 snapshot_controller_base.go:679] Failed to sync group snapshot "default/my-group-snapshot-jsdfjhsjfhsfsdakjfdskajfdskl-jsadjfklsajfksjfksajdkljasdf-sdfjksdfjksadjfkljasdkljfds-asdfjksdjfklsdjfkasljfkasdfjaksldfjkasldfjskadlfjadsk", will retry again: createSnapshotsForGroupSnapshotContent: creating volumesnapshot VolumeSnapshot.snapshot.storage.k8s.io "snapshot-7a11e173d5066084f2dd45dcffff81b977cefa098d80fe387e08aba6ecd3b5a0" is invalid: metadata.labels: Invalid value: "my-group-snapshot-jsdfjhsjfhsfsdakjfdskajfdskl-jsadjfklsajfksjfksajdkljasdf-sdfjksdfjksadjfkljasdkljfds-asdfjksdjfklsdjfkasljfkasdfjaksldfjkasldfjskadlfjadsk": must be no more than 63 characters

[xing-yang]

@Madhu-1 That's a valid concern. Maybe we should use label first if possible, but if the length is longer than 63, use annotation instead. The benefits of using label is that you can search and find all objects with the label with one kubectl command.

[Madhu-1]

@Madhu-1 That's a valid concern. Maybe we should use label first if possible, but if the length is longer than 63, use annotation instead. The benefits of using label is that you can search and find all objects with the label with one kubectl command.

Agree even the current client only support list based on the labels. but would it make sense to take two approach based on the length or go with single implementation that supports both?

[xing-yang]

Discussed with @msau42 and @gnufied about this. We are going to make VolumeGroupSnapshot an owner of VolumeSnapshot.

[leonardoce]

We are using that label internally to look up all the members of a snapshot group. See here:

https://github.com/kubernetes-csi/external-snapshotter/blob/fe82dd2dc3051251ea5e89636bc715be4a969826/pkg/common-controller/groupsnapshot_controller_helper.go#L1439-L1443

We'll need to replace this with an indexer.

Dumb question about the ownership: what is supposed to happen when deleting a VolumeGroupSnapshot with deletionPolicy=Retain? Do we want the member VolumeSnapshots to be deleted, too? Or will we drop the ownership?

[xing-yang]

If it is Retain, we still want the member VolumeSnapshots to be deleted. Do you see any problems?

[leonardoce]

If it is Retain, we still want the member VolumeSnapshots to be deleted. Do you see any problems?

No, I'm just wrapping my mind around it and trying to understand the behavior we are looking for. It's fine now. Thanks!