fix: new logic VS/VCS

Author: Neumoin, Konstantin

images/snapshot-controller/client/config/crd/snapshot.storage.k8s.io_volumesnapshotcontents.yaml

@@ -217,14 +217,13 @@ spec:
                 type: object
                 x-kubernetes-map-type: atomic
                 x-kubernetes-validations:
-                - message: both spec.volumeSnapshotRef.name and spec.volumeSnapshotRef.namespace
-                    must be set
-                  rule: has(self.name) && has(self.__namespace__)
+                - message: if spec.volumeSnapshotRef is set, both name and namespace must be specified together, or volumeSnapshotRef must be completely empty (VSC-only model)
+                  rule: '(has(self.name) && has(self.__namespace__)) || (!has(self.name) && !has(self.__namespace__) && !has(self.uid))'
             required:
             - deletionPolicy
             - driver
             - source
-            - volumeSnapshotRef
+            # volumeSnapshotRef is optional to support VSC-only model (VSC without VolumeSnapshot)
             type: object
             x-kubernetes-validations:
             - message: sourceVolumeMode is required once set
@@ -382,7 +381,7 @@ spec:
                 description: name of the VolumeSnapshotClass from which this snapshot was (or will be) created. Note that after provisioning, the VolumeSnapshotClass may be deleted or recreated with different set of values, and as such, should not be referenced post-snapshot creation.
                 type: string
               volumeSnapshotRef:
-                description: volumeSnapshotRef specifies the VolumeSnapshot object to which this VolumeSnapshotContent object is bound. VolumeSnapshot.Spec.VolumeSnapshotContentName field must reference to this VolumeSnapshotContent's name for the bidirectional binding to be valid. For a pre-existing VolumeSnapshotContent object, name and namespace of the VolumeSnapshot object MUST be provided for binding to happen. This field is immutable after creation. Required.
+                description: volumeSnapshotRef specifies the VolumeSnapshot object to which this VolumeSnapshotContent object is bound. VolumeSnapshot.Spec.VolumeSnapshotContentName field must reference to this VolumeSnapshotContent's name for the bidirectional binding to be valid. For a pre-existing VolumeSnapshotContent object, name and namespace of the VolumeSnapshot object MUST be provided for binding to happen. This field is immutable after creation. Optional - can be empty for VSC-only model (VSC created without VolumeSnapshot, e.g., by VCR controller).
                 properties:
                   apiVersion:
                     description: API version of the referent.
@@ -410,7 +409,7 @@ spec:
             - deletionPolicy
             - driver
             - source
-            - volumeSnapshotRef
+            # volumeSnapshotRef is optional to support VSC-only model (VSC without VolumeSnapshot)
             type: object
           status:
             description: status represents the current information of a snapshot.

images/snapshot-controller/pkg/common-controller/snapshot_controller.go

@@ -86,12 +86,15 @@ const (
 const controllerUpdateFailMsg = "snapshot controller failed to update"
 
 // syncContent deals with one key off the queue
+// VSC-only model: This function now works with VolumeSnapshotContent independently,
+// without requiring VolumeSnapshot. VolumeSnapshotRef may be empty (VSC-only) or
+// present (legacy backward compatibility).
 func (ctrl *csiSnapshotCommonController) syncContent(content *crdv1.VolumeSnapshotContent) error {
-	snapshotName := utils.SnapshotRefKey(&content.Spec.VolumeSnapshotRef)
-	klog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]: content is bound to snapshot %s", content.Name, snapshotName)
+	klog.V(4).Infof("synchronizing VolumeSnapshotContent[%s]", content.Name)
 
 	klog.V(5).Infof("syncContent[%s]: check if we should add invalid label on content", content.Name)
 
+	// Validate that exactly one of VolumeHandle and SnapshotHandle is specified
 	if (content.Spec.Source.VolumeHandle == nil && content.Spec.Source.SnapshotHandle == nil) ||
 		(content.Spec.Source.VolumeHandle != nil && content.Spec.Source.SnapshotHandle != nil) {
 		err := fmt.Errorf("Exactly one of VolumeHandle and SnapshotHandle should be specified")
@@ -100,71 +103,56 @@ func (ctrl *csiSnapshotCommonController) syncContent(content *crdv1.VolumeSnapsh
 		return err
 	}
 
-	// The VolumeSnapshotContent is reserved for a VolumeSnapshot;
-	// that VolumeSnapshot has not yet been bound to this VolumeSnapshotContent;
-	// syncSnapshot will handle it.
-	if content.Spec.VolumeSnapshotRef.UID == "" {
-		klog.V(4).Infof("syncContent [%s]: VolumeSnapshotContent is pre-bound to VolumeSnapshot %s", content.Name, snapshotName)
+	// Check if this is VSC-only (VolumeSnapshotRef completely empty) or legacy (VolumeSnapshotRef set)
+	isVSCOnly := content.Spec.VolumeSnapshotRef.UID == "" &&
+		content.Spec.VolumeSnapshotRef.Name == "" &&
+		content.Spec.VolumeSnapshotRef.Namespace == ""
+
+	if isVSCOnly {
+		// VSC-only model: VolumeSnapshotRef is completely empty - this is normal.
+		// VSC is the source of truth, no VolumeSnapshot involved.
+		klog.V(4).Infof("syncContent [%s]: VolumeSnapshotContent is VSC-only (no VolumeSnapshotRef)", content.Name)
+
+		// VSC-only: Add finalizer if needed (same as legacy)
+		if utils.NeedToAddContentFinalizer(content) {
+			klog.V(5).Infof("syncContent [%s]: Add Finalizer for VolumeSnapshotContent (VSC-only)", content.Name)
+			return ctrl.addContentFinalizer(content)
+		}
+
+		// VSC-only: No VolumeSnapshot to manage, no status updates needed.
+		// Sidecar-controller handles CSI operations and updates VSC.Status.
+		// Deletion is handled by DeletionTimestamp on VSC itself.
 		return nil
 	}
 
+	// Legacy mode: VolumeSnapshotRef is set - maintain backward compatibility
+	// Keep original behavior for legacy snapshots to avoid regressions
+	klog.V(4).Infof("syncContent [%s]: VolumeSnapshotContent is legacy (has VolumeSnapshotRef)", content.Name)
+
+	// Legacy: Add finalizer if needed (same as VSC-only)
 	if utils.NeedToAddContentFinalizer(content) {
-		// Content is not being deleted -> it should have the finalizer.
-		klog.V(5).Infof("syncContent [%s]: Add Finalizer for VolumeSnapshotContent", content.Name)
+		klog.V(5).Infof("syncContent [%s]: Add Finalizer for VolumeSnapshotContent (legacy)", content.Name)
 		return ctrl.addContentFinalizer(content)
 	}
 
-	// Check if snapshot exists in cache store
-	// If getSnapshotFromStore returns (nil, nil), it means snapshot not found
-	// and it may have already been deleted, and it will fall into the
-	// snapshot == nil case below
-	var snapshot *crdv1.VolumeSnapshot
+	// Legacy: Check if VolumeSnapshot exists and handle deletion
+	// This preserves original behavior for backward compatibility
+	snapshotName := utils.SnapshotRefKey(&content.Spec.VolumeSnapshotRef)
 	snapshot, err := ctrl.getSnapshotFromStore(snapshotName)
 	if err != nil {
-		return err
+		// Error getting snapshot - log but continue (may be transient)
+		klog.V(4).Infof("syncContent [%s]: error getting snapshot %s (legacy mode): %v", content.Name, snapshotName, err)
+		return nil
 	}
 
-	if snapshot != nil && snapshot.UID != content.Spec.VolumeSnapshotRef.UID {
-		// The snapshot that the content was pointing to was deleted, and another
-		// with the same name created.
-		klog.V(4).Infof("syncContent [%s]: snapshot %s has different UID, the old one must have been deleted", content.Name, snapshotName)
-		// Treat the content as bound to a missing snapshot.
-		snapshot = nil
-	} else {
-		// Check if snapshot.Status is different from content.Status and add snapshot to queue
-		// if there is a difference and it is worth triggering an snapshot status update.
-		if snapshot != nil && ctrl.needsUpdateSnapshotStatus(snapshot, content) {
-			klog.V(4).Infof("synchronizing VolumeSnapshotContent for snapshot [%s]: update snapshot status to true if needed.", snapshotName)
-			// Manually trigger a snapshot status update to happen
-			// right away so that it is in-sync with the content status
-			ctrl.snapshotQueue.Add(snapshotName)
+	if snapshot != nil && snapshot.UID == content.Spec.VolumeSnapshotRef.UID {
+		// Legacy: If snapshot has deletion timestamp, set annotation to trigger deletion
+		if utils.IsSnapshotDeletionCandidate(snapshot) {
+			_, err = ctrl.setAnnVolumeSnapshotBeingDeleted(content)
+			return err
 		}
 	}
 
-	// NOTE(xyang): Do not trigger content deletion if
-	// snapshot is nil. This is to avoid data loss if
-	// the user copied the yaml files and expect it to work
-	// in a different setup. In this case snapshot is nil.
-	// If we trigger content deletion, it will delete
-	// physical snapshot resource on the storage system
-	// and result in data loss!
-	//
-	// Trigger content deletion if snapshot is not nil
-	// and snapshot has deletion timestamp.
-	// If snapshot has deletion timestamp and finalizers, set
-	// AnnVolumeSnapshotBeingDeleted annotation on the content.
-	// This may trigger the deletion of the content in the
-	// sidecar controller depending on the deletion policy
-	// on the content.
-	// Snapshot won't be deleted until content is deleted
-	// due to the finalizer.
-	if snapshot != nil && utils.IsSnapshotDeletionCandidate(snapshot) {
-		// Do not need to use the returned content here, as syncContent will get
-		// the correct version from the cache next time. It is also not used after this.
-		_, err = ctrl.setAnnVolumeSnapshotBeingDeleted(content)
-		return err
-	}
-
 	return nil
 }
 

images/snapshot-controller/pkg/sidecar-controller/csi_handler.go

@@ -76,15 +76,25 @@ func (handler *csiHandler) CreateSnapshot(content *crdv1.VolumeSnapshotContent,
 	ctx, cancel := context.WithTimeout(context.Background(), handler.timeout)
 	defer cancel()
 
-	if content.Spec.VolumeSnapshotRef.UID == "" {
-		return "", "", time.Time{}, 0, false, fmt.Errorf("cannot create snapshot. Snapshot content %s not bound to a snapshot", content.Name)
+	// VSC-only model: VolumeSnapshotRef.UID may be empty. In this case, use VSC.UID for snapshot name.
+	// This allows VSC-only snapshots (created by VCR) to work without VolumeSnapshot.
+	// NOTE: We use content.UID (not content.Name) to match legacy behavior where VolumeSnapshotRef.UID is used.
+	// content.UID is stable for the lifetime of the object, but changes if VSC is recreated.
+	// Alternative: Using content.Name would be more stable across VSC recreations, but would diverge from legacy behavior.
+	var snapshotUID string
+	if content.Spec.VolumeSnapshotRef.UID != "" {
+		// Legacy mode: VolumeSnapshotRef is set - use it
+		snapshotUID = string(content.Spec.VolumeSnapshotRef.UID)
+	} else {
+		// VSC-only mode: VolumeSnapshotRef is empty - use VSC.UID
+		snapshotUID = string(content.UID)
 	}
 
 	if content.Spec.Source.VolumeHandle == nil {
 		return "", "", time.Time{}, 0, false, fmt.Errorf("cannot create snapshot. Volume handle not found in snapshot content %s", content.Name)
 	}
 
-	snapshotName, err := makeSnapshotName(handler.snapshotNamePrefix, string(content.Spec.VolumeSnapshotRef.UID), handler.snapshotNameUUIDLength)
+	snapshotName, err := makeSnapshotName(handler.snapshotNamePrefix, snapshotUID, handler.snapshotNameUUIDLength)
 	if err != nil {
 		return "", "", time.Time{}, 0, false, err
 	}

images/snapshot-controller/pkg/sidecar-controller/framework_test.go

@@ -333,6 +333,8 @@ func (r *snapshotReactor) checkContents(expectedContents []*crdv1.VolumeSnapshot
 		v := v.DeepCopy()
 		v.ResourceVersion = ""
 		v.Spec.VolumeSnapshotRef.ResourceVersion = ""
+		// Clear DeletionTimestamp to avoid time precision issues in comparison
+		v.DeletionTimestamp = nil
 		if v.Status != nil {
 			v.Status.CreationTime = nil
 		}
@@ -347,6 +349,8 @@ func (r *snapshotReactor) checkContents(expectedContents []*crdv1.VolumeSnapshot
 		v := v.DeepCopy()
 		v.ResourceVersion = ""
 		v.Spec.VolumeSnapshotRef.ResourceVersion = ""
+		// Clear DeletionTimestamp to avoid time precision issues in comparison
+		v.DeletionTimestamp = nil
 		if v.Status != nil {
 			v.Status.CreationTime = nil
 			if v.Status.Error != nil {

images/snapshot-controller/pkg/sidecar-controller/snapshot_controller.go

@@ -105,6 +105,15 @@ func (ctrl *csiSnapshotSideCarController) syncContent(content *crdv1.VolumeSnaps
 		}
 		return false, nil
 	}
+
+	// VSC-only model: If VolumeHandle is nil, we can't create snapshot and can't check status.
+	// Skip processing such VSC (it may be pre-provisioned with SnapshotHandle, but that's handled separately).
+	if content.Spec.Source.VolumeHandle == nil && (content.Status == nil || content.Status.SnapshotHandle == nil) {
+		// No VolumeHandle and no SnapshotHandle - nothing to do
+		klog.V(4).Infof("syncContent [%s]: skipping VSC without VolumeHandle and without SnapshotHandle", content.Name)
+		return false, nil
+	}
+
 	return ctrl.checkandUpdateContentStatus(content)
 }
 
@@ -360,8 +369,13 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotWrapper(content *crdv1.V
 		return content, fmt.Errorf("failed to remove CSI Parameters of prefixed keys: %v", err)
 	}
 	if ctrl.extraCreateMetadata {
-		parameters[utils.PrefixedVolumeSnapshotNameKey] = content.Spec.VolumeSnapshotRef.Name
-		parameters[utils.PrefixedVolumeSnapshotNamespaceKey] = content.Spec.VolumeSnapshotRef.Namespace
+		// VSC-only model: VolumeSnapshotRef may be empty. If it's set, use it for backward compatibility.
+		// If empty, we still provide VSC name as metadata.
+		if content.Spec.VolumeSnapshotRef.Name != "" {
+			parameters[utils.PrefixedVolumeSnapshotNameKey] = content.Spec.VolumeSnapshotRef.Name
+			parameters[utils.PrefixedVolumeSnapshotNamespaceKey] = content.Spec.VolumeSnapshotRef.Namespace
+		}
+		// Always provide VSC name - this is the source of truth in VSC-only model
 		parameters[utils.PrefixedVolumeSnapshotContentNameKey] = content.Name
 	}
 
@@ -628,25 +642,35 @@ func (ctrl *csiSnapshotSideCarController) shouldDelete(content *crdv1.VolumeSnap
 	if content.ObjectMeta.DeletionTimestamp == nil {
 		return false
 	}
-	// 1) shouldDelete returns true if a content is not bound
-	// (VolumeSnapshotRef.UID == "") for pre-provisioned snapshot
-	if content.Spec.Source.SnapshotHandle != nil && content.Spec.VolumeSnapshotRef.UID == "" {
-		return true
-	}
 
 	// NOTE(xyang): Handle create snapshot timeout
-	// 2) shouldDelete returns false if AnnVolumeSnapshotBeingCreated
+	// shouldDelete returns false if AnnVolumeSnapshotBeingCreated
 	// annotation is set. This indicates a CreateSnapshot CSI RPC has
 	// not responded with success or failure.
 	// We need to keep waiting for a response from the CSI driver.
 	if metav1.HasAnnotation(content.ObjectMeta, utils.AnnVolumeSnapshotBeingCreated) {
 		return false
 	}
 
-	// 3) shouldDelete returns true if AnnVolumeSnapshotBeingDeleted annotation is set
+	// Legacy: For pre-provisioned snapshots (SnapshotHandle in Source and VolumeSnapshotRef.UID == ""), delete immediately.
+	if content.Spec.Source.SnapshotHandle != nil && content.Spec.VolumeSnapshotRef.UID == "" {
+		return true
+	}
+
+	// VSC-only or legacy: shouldDelete returns true if AnnVolumeSnapshotBeingDeleted annotation is set.
+	// This annotation is set by common-controller when deletion should proceed.
 	if metav1.HasAnnotation(content.ObjectMeta, utils.AnnVolumeSnapshotBeingDeleted) {
 		return true
 	}
+
+	// VSC-only model: If DeletionTimestamp is set and we have SnapshotHandle in status,
+	// we can proceed with deletion. This covers VSC-only snapshots created by VCR.
+	// Common-controller may not set the annotation for VSC-only, so we check status directly.
+	if content.Status != nil && content.Status.SnapshotHandle != nil {
+		// We have a snapshot to delete - proceed with deletion
+		return true
+	}
+
 	return false
 }