Use ownership to identify volume group snapshot members

Instead of marking the member snapshot object with a label, this PR uses
an ownership reference.

Member objects are looked up using a new index in the informer cache.

Author: leonardoce

go.mod

@@ -21,6 +21,7 @@ require (
 	k8s.io/component-base v0.31.0
 	k8s.io/component-helpers v0.31.0
 	k8s.io/klog/v2 v2.130.1
+	k8s.io/utils v0.0.0-20240711033017-18e509b52bc8
 )
 
 require (
@@ -72,7 +73,6 @@ require (
 	gopkg.in/yaml.v2 v2.4.0 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
 	k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340 // indirect
-	k8s.io/utils v0.0.0-20240711033017-18e509b52bc8 // indirect
 	sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect
 	sigs.k8s.io/structured-merge-diff/v4 v4.4.1 // indirect
 	sigs.k8s.io/yaml v1.4.0 // indirect

pkg/common-controller/framework_test.go

@@ -1728,15 +1728,15 @@ func newVolumeError(message string) *crdv1.VolumeSnapshotError {
 }
 
 func testSyncSnapshot(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error {
-	return ctrl.syncSnapshot(test.initialSnapshots[0])
+	return ctrl.syncSnapshot(context.TODO(), test.initialSnapshots[0])
 }
 
 func testSyncGroupSnapshot(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error {
 	return ctrl.syncGroupSnapshot(context.TODO(), test.initialGroupSnapshots[0])
 }
 
 func testSyncSnapshotError(ctrl *csiSnapshotCommonController, reactor *snapshotReactor, test controllerTest) error {
-	err := ctrl.syncSnapshot(test.initialSnapshots[0])
+	err := ctrl.syncSnapshot(context.TODO(), test.initialSnapshots[0])
 	if err != nil {
 		return nil
 	}

pkg/common-controller/groupsnapshot_controller_helper.go

@@ -27,6 +27,7 @@ import (
 	apierrs "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes/scheme"
 	ref "k8s.io/client-go/tools/reference"
 	klog "k8s.io/klog/v2"
@@ -301,7 +302,7 @@ func (ctrl *csiSnapshotCommonController) syncGroupSnapshot(ctx context.Context,
 
 	// Proceed with group snapshot deletion and remove finalizers when needed
 	if groupSnapshot.ObjectMeta.DeletionTimestamp != nil {
-		return ctrl.processGroupSnapshotWithDeletionTimestamp(groupSnapshot)
+		return ctrl.processGroupSnapshotWithDeletionTimestamp(ctx, groupSnapshot)
 	}
 
 	klog.V(5).Infof("syncGroupSnapshot[%s]: validate group snapshot to make sure source has been correctly specified", utils.GroupSnapshotKey(groupSnapshot))
@@ -599,8 +600,8 @@ func (ctrl *csiSnapshotCommonController) createSnapshotsForGroupSnapshotContent(
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      volumeSnapshotName,
 				Namespace: volumeSnapshotNamespace,
-				Labels: map[string]string{
-					utils.VolumeGroupSnapshotNameLabel: groupSnapshotContent.Spec.VolumeGroupSnapshotRef.Name,
+				OwnerReferences: []metav1.OwnerReference{
+					utils.BuildVolumeGroupSnapshotOwnerReference(groupSnapshot),
 				},
 				Finalizers: []string{utils.VolumeSnapshotInGroupFinalizer},
 			},
@@ -1376,7 +1377,7 @@ func (ctrl *csiSnapshotCommonController) addGroupSnapshotFinalizer(groupSnapshot
 // with information obtained from step 1. This function name is very long but the
 // name suggests what it does. It determines whether to remove finalizers on group
 // snapshot and whether to delete group snapshot content.
-func (ctrl *csiSnapshotCommonController) processGroupSnapshotWithDeletionTimestamp(groupSnapshot *crdv1alpha1.VolumeGroupSnapshot) error {
+func (ctrl *csiSnapshotCommonController) processGroupSnapshotWithDeletionTimestamp(ctx context.Context, groupSnapshot *crdv1alpha1.VolumeGroupSnapshot) error {
 	klog.V(5).Infof("processGroupSnapshotWithDeletionTimestamp VolumeGroupSnapshot[%s]: %s", utils.GroupSnapshotKey(groupSnapshot), utils.GetGroupSnapshotStatusForLogging(groupSnapshot))
 
 	driverName, err := ctrl.getGroupSnapshotDriverName(groupSnapshot)
@@ -1435,11 +1436,13 @@ func (ctrl *csiSnapshotCommonController) processGroupSnapshotWithDeletionTimesta
 		return nil
 	}
 
-	snapshotMembers, err := ctrl.snapshotLister.List(labels.SelectorFromSet(
-		labels.Set{
-			utils.VolumeGroupSnapshotNameLabel: groupSnapshot.Name,
+	// Look up for members of this volume group snapshot
+	snapshotMembers, err := ctrl.findGroupSnapshotMembers(
+		types.NamespacedName{
+			Name:      groupSnapshot.Name,
+			Namespace: groupSnapshot.Namespace,
 		},
-	))
+	)
 	if err != nil {
 		klog.Errorf(
 			"processGroupSnapshotWithDeletionTimestamp[%s]: Failed to look for snapshot members: %v",
@@ -1489,7 +1492,7 @@ func (ctrl *csiSnapshotCommonController) processGroupSnapshotWithDeletionTimesta
 	// VolumeGroupSnapshotContent won't be deleted immediately due to the VolumeGroupSnapshotContentFinalizer
 	if groupSnapshotContent != nil && deleteGroupSnapshotContent {
 		klog.V(5).Infof("processGroupSnapshotWithDeletionTimestamp[%s]: set DeletionTimeStamp on group snapshot content [%s].", utils.GroupSnapshotKey(groupSnapshot), groupSnapshotContent.Name)
-		err := ctrl.clientset.GroupsnapshotV1alpha1().VolumeGroupSnapshotContents().Delete(context.TODO(), groupSnapshotContent.Name, metav1.DeleteOptions{})
+		err := ctrl.clientset.GroupsnapshotV1alpha1().VolumeGroupSnapshotContents().Delete(ctx, groupSnapshotContent.Name, metav1.DeleteOptions{})
 		if err != nil {
 			ctrl.eventRecorder.Event(groupSnapshot, v1.EventTypeWarning, "GroupSnapshotContentObjectDeleteError", "Failed to delete group snapshot content API object")
 			return fmt.Errorf("failed to delete VolumeGroupSnapshotContent %s from API server: %q", groupSnapshotContent.Name, err)
@@ -1561,6 +1564,32 @@ func (ctrl *csiSnapshotCommonController) setAnnVolumeGroupSnapshotBeingDeleted(g
 	return groupSnapshotContent, nil
 }
 
+// findGroupSnapshotMembers get the list of members of a group snapshot
+// using the local cache and indexer
+func (ctrl *csiSnapshotCommonController) findGroupSnapshotMembers(groupSnapshotName types.NamespacedName) ([]*crdv1.VolumeSnapshot, error) {
+	// Look up for the members of this volume group snapshot
+	snapshotMembers, err := ctrl.snapshotIndexer.ByIndex(
+		utils.VolumeSnapshotParentGroupIndex,
+		utils.VolumeSnapshotParentGroupKeyFuncByComponents(
+			groupSnapshotName,
+		),
+	)
+	if err != nil {
+		return nil, err
+	}
+
+	result := make([]*crdv1.VolumeSnapshot, len(snapshotMembers))
+	for i := range snapshotMembers {
+		var ok bool
+		result[i], ok = snapshotMembers[i].(*crdv1.VolumeSnapshot)
+		if !ok {
+			return nil, fmt.Errorf("unexpected content found in snapshot index: %v", snapshotMembers[i])
+		}
+	}
+
+	return result, nil
+}
+
 // removeGroupSnapshotFinalizer removes a Finalizer for VolumeGroupSnapshot.
 func (ctrl *csiSnapshotCommonController) removeGroupSnapshotFinalizer(groupSnapshot *crdv1alpha1.VolumeGroupSnapshot, removeBoundFinalizer bool) error {
 	if !removeBoundFinalizer {

pkg/common-controller/snapshot_controller.go

@@ -173,7 +173,7 @@ func (ctrl *csiSnapshotCommonController) syncContent(content *crdv1.VolumeSnapsh
 // created, updated or periodically synced. We do not differentiate between
 // these events.
 // For easier readability, it is split into syncUnreadySnapshot and syncReadySnapshot
-func (ctrl *csiSnapshotCommonController) syncSnapshot(snapshot *crdv1.VolumeSnapshot) error {
+func (ctrl *csiSnapshotCommonController) syncSnapshot(ctx context.Context, snapshot *crdv1.VolumeSnapshot) error {
 	klog.V(5).Infof("synchronizing VolumeSnapshot[%s]: %s", utils.SnapshotKey(snapshot), utils.GetSnapshotStatusForLogging(snapshot))
 
 	klog.V(5).Infof("syncSnapshot [%s]: check if we should remove finalizer on snapshot PVC source and remove it if we can", utils.SnapshotKey(snapshot))
@@ -214,7 +214,7 @@ func (ctrl *csiSnapshotCommonController) syncSnapshot(snapshot *crdv1.VolumeSnap
 	if !utils.IsSnapshotReady(snapshot) || !utils.IsBoundVolumeSnapshotContentNameSet(snapshot) {
 		return ctrl.syncUnreadySnapshot(snapshot)
 	}
-	return ctrl.syncReadySnapshot(snapshot)
+	return ctrl.syncReadySnapshot(ctx, snapshot)
 }
 
 // processSnapshotWithDeletionTimestamp processes finalizers and deletes the content when appropriate. It has the following steps:
@@ -395,7 +395,7 @@ func (ctrl *csiSnapshotCommonController) checkandAddSnapshotFinalizers(snapshot
 
 // syncReadySnapshot checks the snapshot which has been bound to snapshot content successfully before.
 // If there is any problem with the binding (e.g., snapshot points to a non-existent snapshot content), update the snapshot status and emit event.
-func (ctrl *csiSnapshotCommonController) syncReadySnapshot(snapshot *crdv1.VolumeSnapshot) error {
+func (ctrl *csiSnapshotCommonController) syncReadySnapshot(ctx context.Context, snapshot *crdv1.VolumeSnapshot) error {
 	if !utils.IsBoundVolumeSnapshotContentNameSet(snapshot) {
 		return fmt.Errorf("snapshot %s is not bound to a content", utils.SnapshotKey(snapshot))
 	}
@@ -415,10 +415,56 @@ func (ctrl *csiSnapshotCommonController) syncReadySnapshot(snapshot *crdv1.Volum
 		return ctrl.updateSnapshotErrorStatusWithEvent(snapshot, true, v1.EventTypeWarning, "SnapshotMisbound", "VolumeSnapshotContent is not bound to the VolumeSnapshot correctly")
 	}
 
+	// If this snapshot is a member of a volume group snapshot, ensure we have
+	// the correct ownership. This happens when the user
+	// statically provisioned volume group snapshot members.
+	if utils.NeedToAddVolumeGroupSnapshotOwnership(snapshot) {
+		if _, err := ctrl.addVolumeGroupSnapshotOwnership(ctx, snapshot); err != nil {
+			return err
+		}
+	}
+
 	// everything is verified, return
 	return nil
 }
 
+// addVolumeGroupSnapshotOwnership adds the ownership information to a statically provisioned VolumeSnapshot
+// that is a member of a volume group snapshot
+func (ctrl *csiSnapshotCommonController) addVolumeGroupSnapshotOwnership(ctx context.Context, snapshot *crdv1.VolumeSnapshot) (*crdv1.VolumeSnapshot, error) {
+	klog.V(4).Infof("addVolumeGroupSnapshotOwnership[%s]: adding ownership information", utils.SnapshotKey(snapshot))
+	if snapshot.Status == nil || snapshot.Status.VolumeGroupSnapshotName == nil {
+		klog.V(4).Infof("addVolumeGroupSnapshotOwnership[%s]: no need to add ownership information, empty volumeGroupSnapshotName", utils.SnapshotKey(snapshot))
+		return nil, nil
+	}
+	parentObjectName := *snapshot.Status.VolumeGroupSnapshotName
+
+	parentGroup, err := ctrl.groupSnapshotLister.VolumeGroupSnapshots(snapshot.Namespace).Get(parentObjectName)
+	if err != nil {
+		klog.V(4).Infof("addVolumeGroupSnapshotOwnership[%s]: error while looking for parent group %v", utils.SnapshotKey(snapshot), err)
+		return nil, err
+	}
+	if parentGroup == nil {
+		klog.V(4).Infof("addVolumeGroupSnapshotOwnership[%s]: parent group not found %v", utils.SnapshotKey(snapshot), err)
+		return nil, fmt.Errorf("missing parent group for snapshot %v", utils.SnapshotKey(snapshot))
+	}
+
+	updatedSnapshot := snapshot.DeepCopy()
+	updatedSnapshot.ObjectMeta.OwnerReferences = append(
+		snapshot.ObjectMeta.OwnerReferences,
+		utils.BuildVolumeGroupSnapshotOwnerReference(parentGroup),
+	)
+
+	newSnapshot, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(snapshot.Namespace).Update(ctx, updatedSnapshot, metav1.UpdateOptions{})
+	if err != nil {
+		klog.V(4).Infof("addVolumeGroupSnapshotOwnership[%s]: error when updating VolumeSnapshot %v", utils.SnapshotKey(snapshot), err)
+		return nil, err
+	}
+
+	klog.V(4).Infof("addVolumeGroupSnapshotOwnership[%s]: updated ownership", utils.SnapshotKey(snapshot))
+
+	return newSnapshot, nil
+}
+
 // syncUnreadySnapshot is the main controller method to decide what to do with a snapshot which is not set to ready.
 func (ctrl *csiSnapshotCommonController) syncUnreadySnapshot(snapshot *crdv1.VolumeSnapshot) error {
 	uniqueSnapshotName := utils.SnapshotKey(snapshot)
@@ -483,7 +529,7 @@ func (ctrl *csiSnapshotCommonController) syncUnreadySnapshot(snapshot *crdv1.Vol
 	}
 
 	// member of a dynamically provisioned volume group snapshot
-	if _, ok := snapshot.Labels[utils.VolumeGroupSnapshotNameLabel]; ok {
+	if utils.IsVolumeGroupSnapshotMember(snapshot) {
 		if snapshot.Status == nil || snapshot.Status.BoundVolumeSnapshotContentName == nil {
 			klog.V(5).Infof(
 				"syncUnreadySnapshot [%s]: detected group snapshot member with no content, retrying",
@@ -1422,7 +1468,7 @@ func (ctrl *csiSnapshotCommonController) SetDefaultSnapshotClass(snapshot *crdv1
 		return nil, snapshot, nil
 	}
 
-	if _, ok := snapshot.Labels[utils.VolumeGroupSnapshotNameLabel]; ok {
+	if utils.IsVolumeGroupSnapshotMember(snapshot) {
 		// don't return error for volume group snapshot members
 		klog.V(5).Infof("Don't need to find SnapshotClass for volume group snapshot member [%s]", snapshot.Name)
 		return nil, snapshot, nil

pkg/common-controller/snapshot_controller_base.go

@@ -87,7 +87,8 @@ type csiSnapshotCommonController struct {
 	preventVolumeModeConversion   bool
 	enableVolumeGroupSnapshots    bool
 
-	pvIndexer cache.Indexer
+	pvIndexer       cache.Indexer
+	snapshotIndexer cache.Indexer
 }
 
 // NewCSISnapshotController returns a new *csiSnapshotCommonController
@@ -158,8 +159,20 @@ func NewCSISnapshotCommonController(
 		},
 		ctrl.resyncPeriod,
 	)
+	volumeSnapshotInformer.Informer().AddIndexers(map[string]cache.IndexFunc{
+		utils.VolumeSnapshotParentGroupIndex: func(obj interface{}) ([]string, error) {
+			if snapshot, ok := obj.(*crdv1.VolumeSnapshot); ok {
+				if key := utils.VolumeSnapshotParentGroupKeyFunc(snapshot); key != "" {
+					return []string{key}, nil
+				}
+			}
+
+			return nil, nil
+		},
+	})
 	ctrl.snapshotLister = volumeSnapshotInformer.Lister()
 	ctrl.snapshotListerSynced = volumeSnapshotInformer.Informer().HasSynced
+	ctrl.snapshotIndexer = volumeSnapshotInformer.Informer().GetIndexer()
 
 	volumeSnapshotContentInformer.Informer().AddEventHandlerWithResyncPeriod(
 		cache.ResourceEventHandlerFuncs{
@@ -323,6 +336,7 @@ func (ctrl *csiSnapshotCommonController) snapshotWorker() {
 
 // syncSnapshotByKey processes a VolumeSnapshot request.
 func (ctrl *csiSnapshotCommonController) syncSnapshotByKey(key string) error {
+	ctx := context.Background()
 	klog.V(5).Infof("syncSnapshotByKey[%s]", key)
 
 	namespace, name, err := cache.SplitMetaNamespaceKey(key)
@@ -344,7 +358,7 @@ func (ctrl *csiSnapshotCommonController) syncSnapshotByKey(key string) error {
 				klog.V(5).Infof("Snapshot %q is being deleted. SnapshotClass has already been removed", key)
 			}
 			klog.V(5).Infof("Updating snapshot %q", key)
-			return ctrl.updateSnapshot(newSnapshot)
+			return ctrl.updateSnapshot(ctx, newSnapshot)
 		}
 		return err
 	}
@@ -476,7 +490,7 @@ func (ctrl *csiSnapshotCommonController) checkAndUpdateSnapshotClass(snapshot *c
 
 // updateSnapshot runs in worker thread and handles "snapshot added",
 // "snapshot updated" and "periodic sync" events.
-func (ctrl *csiSnapshotCommonController) updateSnapshot(snapshot *crdv1.VolumeSnapshot) error {
+func (ctrl *csiSnapshotCommonController) updateSnapshot(ctx context.Context, snapshot *crdv1.VolumeSnapshot) error {
 	// Store the new snapshot version in the cache and do not process it if this is
 	// an old version.
 	klog.V(5).Infof("updateSnapshot %q", utils.SnapshotKey(snapshot))
@@ -488,7 +502,7 @@ func (ctrl *csiSnapshotCommonController) updateSnapshot(snapshot *crdv1.VolumeSn
 		return nil
 	}
 
-	err = ctrl.syncSnapshot(snapshot)
+	err = ctrl.syncSnapshot(ctx, snapshot)
 	if err != nil {
 		if errors.IsConflict(err) {
 			// Version conflict error happens quite often and the controller

pkg/sidecar-controller/groupsnapshot_helper.go

@@ -246,10 +246,18 @@ func (ctrl *csiSnapshotSideCarController) deleteCSIGroupSnapshotOperation(groupS
 		return fmt.Errorf("failed to get input parameters to delete group snapshot for group snapshot content %s: %q", groupSnapshotContent.Name, err)
 	}
 
+	// Collect the snapshot ids considering both dynamic and static provisioning.
+	// For dynamic provisioning, they can be found in groupContent.Status.VolumeSnapshotHandlePairList
+	// For static provisioning, they can be found in groupContent.Spec.Source.GroupSnapshotHandles.VolumeSnapshotHandles
 	var snapshotIDs []string
-	if groupSnapshotContent.Status != nil && len(groupSnapshotContent.Status.VolumeSnapshotHandlePairList) != 0 {
-		for _, contentRef := range groupSnapshotContent.Status.VolumeSnapshotHandlePairList {
-			snapshotIDs = append(snapshotIDs, contentRef.SnapshotHandle)
+	if groupSnapshotContent.Status != nil {
+		if len(groupSnapshotContent.Status.VolumeSnapshotHandlePairList) != 0 {
+			for _, contentRef := range groupSnapshotContent.Status.VolumeSnapshotHandlePairList {
+				snapshotIDs = append(snapshotIDs, contentRef.SnapshotHandle)
+			}
+		} else if groupSnapshotContent.Spec.Source.GroupSnapshotHandles != nil {
+			ids := groupSnapshotContent.Spec.Source.GroupSnapshotHandles.VolumeSnapshotHandles
+			snapshotIDs = slices.Clone(ids)
 		}
 	}
 

pkg/utils/util.go

@@ -146,10 +146,6 @@ const (
 	AnnDeletionGroupSecretRefName      = "groupsnapshot.storage.kubernetes.io/deletion-secret-name"
 	AnnDeletionGroupSecretRefNamespace = "groupsnapshot.storage.kubernetes.io/deletion-secret-namespace"
 
-	// VolumeGroupSnapshotNameLabel is applied to VolumeSnapshots that are member
-	// of a VolumeGroupSnapshot, and indicates the name of the latter.
-	VolumeGroupSnapshotNameLabel = "groupsnapshot.storage.k8s.io/volumeGroupSnapshotName"
-
 	// VolumeGroupSnapshotHandleAnnotation is applied to VolumeSnapshotContents that are member
 	// of a VolumeGroupSnapshotContent, and indicates the handle of the latter.
 	//