tighten sync between smart clone and transfer controllers (#2209)

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>
kubevirt · Apr 1, 2022 · c871cec · c871cec
1 parent b9c0684
commit c871cec
Show file tree

Hide file tree

Showing 4 changed files with 43 additions and 9 deletions.
diff --git a/pkg/controller/smart-clone-controller.go b/pkg/controller/smart-clone-controller.go
@@ -158,12 +158,17 @@ func (r *SmartCloneReconciler) Reconcile(_ context.Context, req reconcile.Reques
 func (r *SmartCloneReconciler) reconcilePvc(log logr.Logger, pvc *corev1.PersistentVolumeClaim) (reconcile.Result, error) {
 	log.WithValues("pvc.Name", pvc.Name).WithValues("pvc.Namespace", pvc.Namespace).Info("Reconciling PVC")
 
+	snapshotName, hasSnapshot := pvc.Annotations[annSmartCloneSnapshot]
+
 	// Don't delete snapshot unless the PVC is bound.
-	if pvc.Status.Phase == corev1.ClaimBound {
-		namespace, name, err := cache.SplitMetaNamespaceKey(pvc.Annotations[annSmartCloneSnapshot])
+	if hasSnapshot && pvc.Status.Phase == corev1.ClaimBound {
+		namespace, name, err := cache.SplitMetaNamespaceKey(snapshotName)
 		if err != nil {
-			log.Info("Handling PVC without snapshot name annotation, exiting")
-			return reconcile.Result{}, nil
+			return reconcile.Result{}, err
+		}
+
+		if err := r.deleteSnapshot(log, namespace, name); err != nil {
+			return reconcile.Result{}, err
 		}
 
 		if v, ok := pvc.Annotations[AnnCloneOf]; !ok || v != "true" {
@@ -176,10 +181,6 @@ func (r *SmartCloneReconciler) reconcilePvc(log logr.Logger, pvc *corev1.Persist
 				return reconcile.Result{}, err
 			}
 		}
-
-		if err := r.deleteSnapshot(log, namespace, name); err != nil {
-			return reconcile.Result{}, err
-		}
 	}
 
 	return reconcile.Result{}, nil

diff --git a/pkg/controller/smart-clone-controller_test.go b/pkg/controller/smart-clone-controller_test.go
@@ -117,6 +117,15 @@ var _ = Describe("All smart clone tests", func() {
 			Expect(err).ToNot(HaveOccurred())
 		})
 
+		It("Should error with malformed annotation", func() {
+			reconciler := createSmartCloneReconciler()
+			pvc := createPVCWithSnapshotSource("test-dv", "invalid")
+			pvc.Annotations["cdi.kubevirt.io/smartCloneSnapshot"] = "foo/bar/baz"
+			_, err := reconciler.reconcilePvc(reconciler.log, pvc)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("unexpected key format"))
+		})
+
 		It("Should add cloneOf annotation and delete snapshot", func() {
 			pvc := createPVCWithSnapshotSource("test-dv", "invalid")
 			snapshot := createSnapshotVolume("invalid", pvc.Namespace, nil)

diff --git a/pkg/controller/transfer/pvc.go b/pkg/controller/transfer/pvc.go
@@ -129,7 +129,8 @@ func (h *pvcTransferHandler) ReconcileRunning(ot *cdiv1.ObjectTransfer) (time.Du
 		return 0, h.reconciler.setCompleteConditionError(ot, err)
 	}
 
-	if sourceExists {
+	// must assert pointing to same PV to avoid races/contention with smartclone controller
+	if sourceExists && source.Spec.VolumeName == pv.Name {
 		if pv.Spec.PersistentVolumeReclaimPolicy != corev1.PersistentVolumeReclaimRetain {
 			pv.Spec.PersistentVolumeReclaimPolicy = corev1.PersistentVolumeReclaimRetain
 			if err := h.reconciler.updateResource(ot, pv); err != nil {

diff --git a/pkg/controller/transfer/pvc_test.go b/pkg/controller/transfer/pvc_test.go
@@ -309,6 +309,29 @@ var _ = Describe("PVC Transfer Tests", func() {
 			checkCompleteFalse(xfer, "Running", "")
 		})
 
+		It("Should create target even when a new source appears", func() {
+			xfer := pvcTransferRunning()
+			xfer.Status.Data["pvReclaim"] = "Delete"
+			pv := sourcePV()
+			pv.Spec.PersistentVolumeReclaimPolicy = corev1.PersistentVolumeReclaimRetain
+			pv.Spec.ClaimRef = nil
+			newSource := createBoundPVC()
+			newSource.Spec.VolumeName = "somethingelse"
+			pvc := &corev1.PersistentVolumeClaim{}
+
+			r := createReconciler(xfer, pv, newSource)
+			_, err := r.Reconcile(context.TODO(), rr(xfer.Name))
+			Expect(err).ToNot(HaveOccurred())
+
+			err = getResource(r.Client, "", xfer.Name, xfer)
+			Expect(err).ToNot(HaveOccurred())
+			err = getResource(r.Client, "target-ns", "target-pvc", pvc)
+			Expect(err).ToNot(HaveOccurred())
+
+			Expect(xfer.Status.Phase).To(Equal(cdiv1.ObjectTransferRunning))
+			checkCompleteFalse(xfer, "Running", "")
+		})
+
 		It("Should wait for target to be bound", func() {
 			xfer := pvcTransferRunning()
 			xfer.Status.Data["pvReclaim"] = "Delete"