Merge pull request #1198 from leonardoce/set-volumehandle

Set VolumeHandle for dynamically provisioned VolumeGroupSnapshots

Author: k8s-ci-robot

pkg/common-controller/groupsnapshot_controller_helper.go

@@ -504,6 +504,12 @@ func (ctrl *csiSnapshotCommonController) createSnapshotsForGroupSnapshotContent(
 		return groupSnapshotContent, nil
 	}
 
+	// No volume group snapshot handle is present.
+	// Let's wait for the snapshotter sidecar to fill it.
+	if groupSnapshotContent.Status.VolumeGroupSnapshotHandle == nil {
+		return groupSnapshotContent, nil
+	}
+
 	// The contents of the volume group snapshot class are needed to set the
 	// metadata containing the secrets to recover the snapshots
 	if groupSnapshot.Spec.VolumeGroupSnapshotClassName == nil {
@@ -557,6 +563,9 @@ func (ctrl *csiSnapshotCommonController) createSnapshotsForGroupSnapshotContent(
 		volumeSnapshotContent := &crdv1.VolumeSnapshotContent{
 			ObjectMeta: metav1.ObjectMeta{
 				Name: volumeSnapshotContentName,
+				Labels: map[string]string{
+					utils.VolumeGroupSnapshotHandleLabel: *groupSnapshotContent.Status.VolumeGroupSnapshotHandle,
+				},
 			},
 			Spec: crdv1.VolumeSnapshotContentSpec{
 				VolumeSnapshotRef: v1.ObjectReference{
@@ -567,7 +576,7 @@ func (ctrl *csiSnapshotCommonController) createSnapshotsForGroupSnapshotContent(
 				DeletionPolicy: groupSnapshotContent.Spec.DeletionPolicy,
 				Driver:         groupSnapshotContent.Spec.Driver,
 				Source: crdv1.VolumeSnapshotContentSource{
-					SnapshotHandle: &snapshotHandle,
+					VolumeHandle: &volumeHandle,
 				},
 			},
 			// The status will be set by VolumeSnapshotContent reconciler
@@ -586,24 +595,28 @@ func (ctrl *csiSnapshotCommonController) createSnapshotsForGroupSnapshotContent(
 			metav1.SetMetaDataAnnotation(&volumeSnapshotContent.ObjectMeta, utils.AnnDeletionSecretRefNamespace, groupSnapshotSecret.Namespace)
 		}
 
-		label := make(map[string]string)
-		label[utils.VolumeGroupSnapshotNameLabel] = groupSnapshotContent.Spec.VolumeGroupSnapshotRef.Name
 		volumeSnapshot := &crdv1.VolumeSnapshot{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:       volumeSnapshotName,
-				Namespace:  volumeSnapshotNamespace,
-				Labels:     label,
-				Finalizers: []string{utils.VolumeSnapshotInGroupFinalizer},
-			},
-			Spec: crdv1.VolumeSnapshotSpec{
-				Source: crdv1.VolumeSnapshotSource{
-					PersistentVolumeClaimName: &pv.Spec.ClaimRef.Name,
+				Name:      volumeSnapshotName,
+				Namespace: volumeSnapshotNamespace,
+				Labels: map[string]string{
+					utils.VolumeGroupSnapshotNameLabel: groupSnapshotContent.Spec.VolumeGroupSnapshotRef.Name,
 				},
+				Finalizers: []string{utils.VolumeSnapshotInGroupFinalizer},
 			},
+			// The spec stanza is set immediately
 			// The status will be set by VolumeSnapshot reconciler
 		}
 
-		_, err = ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Create(ctx, volumeSnapshotContent, metav1.CreateOptions{})
+		if pv != nil {
+			volumeSnapshot.Spec.Source.PersistentVolumeClaimName = &pv.Spec.ClaimRef.Name
+		} else {
+			// If no persistent volume was found, set the PVC name to empty
+			var emptyString string
+			volumeSnapshot.Spec.Source.PersistentVolumeClaimName = &emptyString
+		}
+
+		createdVolumeSnapshotContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().Create(ctx, volumeSnapshotContent, metav1.CreateOptions{})
 		if err != nil && !apierrs.IsAlreadyExists(err) {
 			return groupSnapshotContent, fmt.Errorf(
 				"createSnapshotsForGroupSnapshotContent: creating volumesnapshotcontent %w", err)
@@ -648,6 +661,32 @@ func (ctrl *csiSnapshotCommonController) createSnapshotsForGroupSnapshotContent(
 			return groupSnapshotContent, fmt.Errorf(
 				"createSnapshotsForGroupSnapshotContent: binding volumesnapshot to volumesnapshotcontent %w", err)
 		}
+
+		// set the snapshot handle and the group snapshot handle
+		// inside the volume snapshot content to allow
+		// the CSI Snapshotter sidecar to reconcile its status
+		_, err = utils.PatchVolumeSnapshotContent(createdVolumeSnapshotContent, []utils.PatchOp{
+			{
+				Op:    "replace",
+				Path:  "/status",
+				Value: &crdv1.VolumeSnapshotContentStatus{},
+			},
+			{
+				Op:    "replace",
+				Path:  "/status/snapshotHandle",
+				Value: snapshotHandle,
+			},
+			{
+				Op:    "replace",
+				Path:  "/status/volumeGroupSnapshotHandle",
+				Value: groupSnapshotContent.Status.VolumeGroupSnapshotHandle,
+			},
+		}, ctrl.clientset, "status")
+		if err != nil {
+			return groupSnapshotContent, fmt.Errorf(
+				"createSnapshotsForGroupSnapshotContent: setting snapshotHandle in volumesnapshotcontent %w", err)
+		}
+
 	}
 
 	return groupSnapshotContent, nil

pkg/sidecar-controller/snapshot_controller.go

@@ -83,10 +83,15 @@ func (ctrl *csiSnapshotSideCarController) syncContent(content *crdv1.VolumeSnaps
 		return false, nil
 
 	}
-	if content.Spec.Source.VolumeHandle != nil && content.Status == nil {
+
+	// Create snapshot calling the CSI driver only if it is a dynamic
+	// provisioning for an independent snapshot.
+	_, groupSnapshotMember := content.Labels[utils.VolumeGroupSnapshotHandleLabel]
+	if content.Spec.Source.VolumeHandle != nil && content.Status == nil && !groupSnapshotMember {
 		klog.V(5).Infof("syncContent: Call CreateSnapshot for content %s", content.Name)
 		return ctrl.createSnapshot(content)
 	}
+
 	// Skip checkandUpdateContentStatus() if ReadyToUse is
 	// already true. We don't want to keep calling CreateSnapshot
 	// or ListSnapshots CSI methods over and over again for
@@ -253,11 +258,13 @@ func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatusOperation(c
 	var size int64
 	readyToUse := false
 	var driverName string
-	var snapshotID, groupSnapshotID string
+	var groupSnapshotID string
 	var snapshotterListCredentials map[string]string
 
-	if content.Spec.Source.SnapshotHandle != nil {
-		klog.V(5).Infof("checkandUpdateContentStatusOperation: call GetSnapshotStatus for snapshot which is pre-bound to content [%s]", content.Name)
+	volumeGroupSnapshotMember := content.Status != nil && content.Status.VolumeGroupSnapshotHandle != nil
+
+	if content.Spec.Source.SnapshotHandle != nil || (volumeGroupSnapshotMember && content.Status.SnapshotHandle != nil) {
+		klog.V(5).Infof("checkandUpdateContentStatusOperation: call GetSnapshotStatus for snapshot content [%s]", content.Name)
 
 		if content.Spec.VolumeSnapshotClassName != nil {
 			class, err := ctrl.getSnapshotClass(*content.Spec.VolumeSnapshotClassName)
@@ -286,7 +293,13 @@ func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatusOperation(c
 			return content, err
 		}
 		driverName = content.Spec.Driver
-		snapshotID = *content.Spec.Source.SnapshotHandle
+
+		var snapshotID string
+		if content.Spec.Source.SnapshotHandle != nil {
+			snapshotID = *content.Spec.Source.SnapshotHandle
+		} else {
+			snapshotID = *content.Status.SnapshotHandle
+		}
 
 		klog.V(5).Infof("checkandUpdateContentStatusOperation: driver %s, snapshotId %s, creationTime %v, size %d, readyToUse %t, groupSnapshotID %s", driverName, snapshotID, creationTime, size, readyToUse, groupSnapshotID)
 

pkg/utils/util.go

@@ -150,6 +150,13 @@ const (
 	// of a VolumeGroupSnapshot, and indicates the name of the latter.
 	VolumeGroupSnapshotNameLabel = "groupsnapshot.storage.k8s.io/volumeGroupSnapshotName"
 
+	// VolumeGroupSnapshotHandleLabel is applied to VolumeSnapshotContents that are member
+	// of a VolumeGroupSnapshotContent, and indicates the handle of the latter.
+	//
+	// This label is applied to inform the sidecar not to call CSI driver
+	// to create snapshot if the snapshot belongs to a group.
+	VolumeGroupSnapshotHandleLabel = "groupsnapshot.storage.k8s.io/volumeGroupSnapshotHandle"
+
 	// VolumeSnapshotContentInvalidLabel is applied to invalid content as a label key. The value does not matter.
 	// See https://github.com/kubernetes/enhancements/blob/master/keps/sig-storage/177-volume-snapshot/tighten-validation-webhook-crd.md#automatic-labelling-of-invalid-objects
 	VolumeSnapshotContentInvalidLabel = "snapshot.storage.kubernetes.io/invalid-snapshot-content-resource"