vrg: add vrg owner annotation to pv and pvc #627

raghavendra-talur · 2022-11-21T16:52:07Z

Set the vrg owner name annotation to the PV and PVC.

This will be used to detect and prevent multiple owners of the PVC. The
annotation on the PVC is used to detect conflicts. The annotation on the
PV is only for debugging.

ShyamsundarR

I think overall we need to do the following:

Detect a VRG conflicts with another in the same namespace with the same label selector (across MetroDR, volsync and VolRep)
Let only one VRG reconcile if there is a collision
The other VRG should have a broader error reported in the summary status (DataReady)

I suggest we use locks, mapping them to a map with key based on --. This will ensure only one VRG is reconciled at a time, and serialize the conflict checks in the code (PVC ownership).

There would be an additional case where a new PVC is created, and in this case we potentially want the VRG that protected the data to succeed and not the VRG that went second/later. This needs to be resolved post locking to ensure PVCs that need protection are owned by the current VRG before further reconciliation.

ShyamsundarR · 2022-11-22T18:05:52Z

controllers/volumereplicationgroup_controller.go

-	pvVRAnnotationRetentionKey    = "volumereplicationgroups.ramendr.openshift.io/vr-retained"
-	pvVRAnnotationRetentionValue  = "retained"
-	PVRestoreAnnotation           = "volumereplicationgroups.ramendr.openshift.io/ramen-restore"
+	pvcVRAnnotationOwnerKey       = "volumereplicationgroups.ramendr.openshift.io/vrg-name"


(nit) Can fold protected and owner as a single annotation

ShyamsundarR · 2022-11-22T18:08:01Z

controllers/volumereplicationgroup_controller.go

+	pvVRAnnotationRetentionKey   = "volumereplicationgroups.ramendr.openshift.io/vr-retained"
+	pvVRAnnotationRetentionValue = "retained"
+	PVRestoreAnnotation          = "volumereplicationgroups.ramendr.openshift.io/ramen-restore"
+	pvVRAnnotationOwnerKey       = "volumereplicationgroups.ramendr.openshift.io/vrg-name"


PV owner is needed for MetroDR? As ownership of PVC is enough to decide not to protect the PVC, the PV should not matter at that point is the assumption.

This can be removed. I don't think it is required.

ShyamsundarR · 2022-11-22T18:12:03Z

controllers/vrg_volrep.go

@@ -210,6 +210,17 @@ func (v *VRGInstance) preparePVCForVRProtection(pvc *corev1.PersistentVolumeClai
 		return !requeue, !skip
 	}

+	// if PVC is protected by another VRG, skip this PVC
+	// there might be PVCs that don't have a conflicting owner
+	if pvcVRGOwner, found := pvc.Annotations[pvcVRAnnotationOwnerKey]; found {


At this point we have conflicting VRGs and should not allow progression of the reconcile, as PVCs maybe protected by either VRGs as they race in reconciliation.

I suggest we use locks, mapping them to a map with key based on --. This will ensure only one VRG is reconciled at a time, and serialize the conflict checks in the code (PVC ownership).

Could you elaborate a bit more on "mapping them to a map with key based on --".

IIUC, this is a lock within the controller. Wouldn't that have an impact the processing time as there could be many VRGs in the system?

The map of locks is this pattern. This will serialize only those reconciles that conflict on the string that is common, and hence other VRG resources would reconcile in parallel.

Looks like I did not add a potential key example, it would be say <namespace>-<label-selector-hash> so that 2 VRGs in the same namespace having the same label selector will not reconcile in parallel.

ShyamsundarR · 2022-11-22T18:12:38Z

controllers/vrg_volrep.go

@@ -1516,6 +1527,7 @@ func (v *VRGInstance) addProtectedAnnotationForPVC(pvc *corev1.PersistentVolumeC
 	}

 	pvc.ObjectMeta.Annotations[pvcVRAnnotationProtectedKey] = pvcVRAnnotationProtectedValue
+	pvc.ObjectMeta.Annotations[pvcVRAnnotationOwnerKey] = v.instance.Name


This will not be added during deletion, which may cause some conflict? (need to analyze)

BenamarMk · 2022-11-30T13:19:41Z

controllers/vrg_volrep.go

+	// if PVC is protected by another VRG, skip this PVC
+	// there might be PVCs that don't have a conflicting owner
+	if pvcVRGOwner, found := pvc.Annotations[pvcVRAnnotationOwnerKey]; found {
+		if pvcVRGOwner != v.instance.Name {


If a PVC is owned by another instance of VRG, then this is an error. We should at least log it as such.

Set the vrg owner name annotation to the PV and PVC. This will be used to detect and prevent multiple owners of the PVC. The annotation on the PVC is used to detect conflicts. The annotation on the PV is only for debugging. Signed-off-by: Raghavendra Talur <raghavendra.talur@gmail.com>

raghavendra-talur · 2022-12-19T14:59:52Z

Moved it to draft because I am making many forced updates. Will remove from draft status soon.

raghavendra-talur force-pushed the rtalur-fix-bug1 branch from 797db48 to 50c9cdd Compare November 21, 2022 16:53

raghavendra-talur changed the title ~~vrg: add vrg owner annotation to pv~~ vrg: add vrg owner annotation to pv and pvc Nov 21, 2022

raghavendra-talur requested review from BenamarMk and ShyamsundarR and removed request for BenamarMk November 21, 2022 16:54

ShyamsundarR reviewed Nov 22, 2022

View reviewed changes

BenamarMk reviewed Nov 30, 2022

View reviewed changes

raghavendra-talur force-pushed the rtalur-fix-bug1 branch 3 times, most recently from f211990 to 845cabf Compare December 16, 2022 20:22

raghavendra-talur force-pushed the rtalur-fix-bug1 branch from 845cabf to d5b9214 Compare December 19, 2022 14:58

raghavendra-talur marked this pull request as draft December 19, 2022 14:59

hatfieldbrian mentioned this pull request Mar 3, 2023

design: how to delete PVCs/PVs without stopping DR protection #697

Open

vrg: add vrg owner annotation to pv and pvc #627

vrg: add vrg owner annotation to pv and pvc #627

raghavendra-talur commented Nov 21, 2022 •

edited

ShyamsundarR left a comment

ShyamsundarR Nov 22, 2022

ShyamsundarR Nov 22, 2022

raghavendra-talur Nov 30, 2022

ShyamsundarR Nov 22, 2022

raghavendra-talur Nov 30, 2022

ShyamsundarR Nov 30, 2022

ShyamsundarR Nov 22, 2022

BenamarMk Nov 30, 2022

raghavendra-talur Nov 30, 2022

raghavendra-talur Dec 15, 2022

raghavendra-talur commented Dec 19, 2022

vrg: add vrg owner annotation to pv and pvc #627

Are you sure you want to change the base?

vrg: add vrg owner annotation to pv and pvc #627

Conversation

raghavendra-talur commented Nov 21, 2022 • edited

ShyamsundarR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raghavendra-talur commented Dec 19, 2022

raghavendra-talur commented Nov 21, 2022 •

edited