replication: fix owner annotation problem

the owner reference should be added only when the VR is created not always, moved adding annotatePVCWithOwner to the correct if check. Signed-off-by: Madhu Rajanna <[email protected]>
csi-addons · Aug 25, 2022 · c762180 · c762180
1 parent 60cfbf5
commit c762180
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 20 deletions.
diff --git a/controllers/replication.storage/pvc.go b/controllers/replication.storage/pvc.go
@@ -60,14 +60,15 @@ func (r VolumeReplicationReconciler) getPVCDataSource(logger logr.Logger, req ty
 }
 
 // annotatePVCWithOwner will add the VolumeReplication details to the PVC annotations.
-func (r *VolumeReplicationReconciler) annotatePVCWithOwner(ctx context.Context, logger logr.Logger, req types.NamespacedName, pvc *corev1.PersistentVolumeClaim) error {
+func (r *VolumeReplicationReconciler) annotatePVCWithOwner(ctx context.Context, logger logr.Logger, reqOwnerName string, pvc *corev1.PersistentVolumeClaim) error {
 	if pvc.ObjectMeta.Annotations == nil {
 		pvc.ObjectMeta.Annotations = map[string]string{}
 	}
 
-	ownerName := pvc.ObjectMeta.Annotations[replicationv1alpha1.VolumeReplicationNameAnnotation]
-	if ownerName == "" {
-		pvc.ObjectMeta.Annotations[replicationv1alpha1.VolumeReplicationNameAnnotation] = req.Name
+	currentOwnerName := pvc.ObjectMeta.Annotations[replicationv1alpha1.VolumeReplicationNameAnnotation]
+	if currentOwnerName == "" {
+		logger.Info("setting owner on PVC annotation", "Name", pvc.Name, "owner", reqOwnerName)
+		pvc.ObjectMeta.Annotations[replicationv1alpha1.VolumeReplicationNameAnnotation] = reqOwnerName
 		err := r.Update(ctx, pvc)
 		if err != nil {
 			logger.Error(err, "Failed to update PVC annotation", "Name", pvc.Name)
@@ -79,14 +80,14 @@ func (r *VolumeReplicationReconciler) annotatePVCWithOwner(ctx context.Context,
 		return nil
 	}
 
-	if ownerName != req.Name {
+	if currentOwnerName != reqOwnerName {
 		logger.Info("cannot change the owner of PVC",
 			"PVC name", pvc.Name,
-			"current owner", ownerName,
-			"requested owner", req.Name)
+			"current owner", currentOwnerName,
+			"requested owner", reqOwnerName)
 
 		return fmt.Errorf("PVC %q not owned by VolumeReplication %q",
-			pvc.Name, req.Name)
+			pvc.Name, reqOwnerName)
 	}
 
 	return nil

diff --git a/controllers/replication.storage/pvc_test.go b/controllers/replication.storage/pvc_test.go
@@ -224,17 +224,24 @@ func TestVolumeReplicationReconciler_annotatePVCWithOwner(t *testing.T) {
 		testPVC := &corev1.PersistentVolumeClaim{}
 		tc.pvc.DeepCopyInto(testPVC)
 
-		namespacedName := types.NamespacedName{
-			Name:      vrName,
-			Namespace: mockNamespace,
-		}
-
+		ctx := context.TODO()
 		reconciler := createFakeVolumeReplicationReconciler(t, testPVC, volumeReplication)
-		err := reconciler.annotatePVCWithOwner(context.TODO(), log.FromContext(context.TODO()), namespacedName, testPVC)
+		err := reconciler.annotatePVCWithOwner(ctx, log.FromContext(context.TODO()), vrName, testPVC)
 		if tc.errorExpected {
 			assert.Error(t, err)
 		} else {
 			assert.NoError(t, err)
+
+			pvcNamespacedName := types.NamespacedName{
+				Name:      testPVC.Name,
+				Namespace: testPVC.Namespace,
+			}
+
+			// check annotation is added
+			err = reconciler.Get(ctx, pvcNamespacedName, testPVC)
+			assert.NoError(t, err)
+
+			assert.Equal(t, testPVC.ObjectMeta.Annotations[replicationv1alpha1.VolumeReplicationNameAnnotation], vrName)
 		}
 
 		err = reconciler.removeOwnerFromPVCAnnotation(context.TODO(), log.FromContext(context.TODO()), testPVC)

diff --git a/controllers/replication.storage/volumereplication_controller.go b/controllers/replication.storage/volumereplication_controller.go
@@ -172,12 +172,6 @@ func (r *VolumeReplicationReconciler) Reconcile(ctx context.Context, req ctrl.Re
 		logger.Info("Replication handle", "ReplicationHandleName", replicationHandle)
 	}
 
-	err = r.annotatePVCWithOwner(ctx, logger, nameSpacedName, pvc)
-	if err != nil {
-		logger.Error(err, "Failed to annotate PVC owner")
-		return ctrl.Result{}, err
-	}
-
 	replicationClient, err := r.getReplicationClient(vrcObj.Spec.Provisioner)
 	if err != nil {
 		logger.Error(err, "Failed to get ReplicationClient")
@@ -206,6 +200,13 @@ func (r *VolumeReplicationReconciler) Reconcile(ctx context.Context, req ctrl.Re
 
 			return reconcile.Result{}, err
 		}
+
+		err = r.annotatePVCWithOwner(ctx, logger, req.Name, pvc)
+		if err != nil {
+			logger.Error(err, "Failed to annotate PVC owner")
+			return ctrl.Result{}, err
+		}
+
 		if err = r.addFinalizerToPVC(logger, pvc); err != nil {
 			logger.Error(err, "Failed to add PersistentVolumeClaim finalizer")