feat(vrc): filter provisioner when using selectors

internal/constants/const.go

@@ -1,9 +1,11 @@
 package constants
 
 const (
-	LockName              = "spx-volume-replicator-leader-election"
-	VrcValueAnnotation    = "replication.superphenix.net/class"
-	VrcSelectorAnnotation = "replication.superphenix.net/classSelector"
-	VrParentLabel         = "replication.superphenix.net/parent"
-	VrStorageClassGroup   = "replication.superphenix.net/storageClassGroup"
+	LockName                               = "spx-volume-replicator-leader-election"
+	VrcValueAnnotation                     = "replication.superphenix.net/class"
+	VrcSelectorAnnotation                  = "replication.superphenix.net/classSelector"
+	ParentLabel                            = "replication.superphenix.net/parent"
+	StorageClassGroup                      = "replication.superphenix.net/storageClassGroup"
+	StorageProvisionerAnnotation           = "volume.kubernetes.io/storage-provisioner"
+	DeprecatedStorageProvisionerAnnotation = "volume.beta.kubernetes.io/storage-provisioner"
 )

internal/replicator/replicator_test.go

@@ -53,7 +53,7 @@ func TestReconcileVolumeReplication(t *testing.T) {
 			"name":      pvcName,
 			"namespace": nsName,
 			"labels": map[string]interface{}{
-				constants.VrParentLabel: pvcName,
+				constants.ParentLabel: pvcName,
 			},
 		},
 		"spec": map[string]interface{}{

internal/replicator/utils.go

@@ -115,7 +115,7 @@ func getVolumeReplication(key string) (*unstructured.Unstructured, error) {
 
 // isParentLabelPresent returns whether a parent label is present on a VolumeReplication
 func isParentLabelPresent(labels map[string]string) bool {
-	return labels[constants.VrParentLabel] != ""
+	return labels[constants.ParentLabel] != ""
 }
 
 // getLabelsWithParent returns a new map of labels for a VolumeReplication with its parent PVC embedded.
@@ -125,7 +125,7 @@ func getLabelsWithParent(pvcLabels map[string]string, parent string) map[string]
 	for k, v := range pvcLabels {
 		res[k] = v
 	}
-	res[constants.VrParentLabel] = parent
+	res[constants.ParentLabel] = parent
 	return res
 }
 
@@ -155,5 +155,16 @@ func getStorageClassGroup(pvc *corev1.PersistentVolumeClaim) (string, error) {
 	}
 
 	// Retrieve the group of VolumeReplicationClasses associated with this StorageClass
-	return stcLabels[constants.VrStorageClassGroup], nil
+	return stcLabels[constants.StorageClassGroup], nil
+}
+
+// getPvcProvisioner returns the dynamic provisioner used to provision a PVC
+func getPvcProvisioner(pvc *corev1.PersistentVolumeClaim) string {
+	// Try the well-known annotation first
+	if pvc.Annotations[constants.StorageProvisionerAnnotation] != "" {
+		return pvc.Annotations[constants.StorageProvisionerAnnotation]
+	}
+
+	// Fallback to the deprecated annotation
+	return pvc.Annotations[constants.DeprecatedStorageProvisionerAnnotation]
 }

internal/replicator/utils_test.go

@@ -63,7 +63,7 @@ func TestCreateVolumeReplication(t *testing.T) {
 		require.Equal(t, vrcName, vr.GetAnnotations()[constants.VrcValueAnnotation])
 		require.Equal(t, "value", vr.GetAnnotations()["other-annotation"])
 		require.Equal(t, "value", vr.GetLabels()["other-label"])
-		require.Equal(t, pvcName, vr.GetLabels()[constants.VrParentLabel])
+		require.Equal(t, pvcName, vr.GetLabels()[constants.ParentLabel])
 
 		// Check spec
 		spec, ok := vr.Object["spec"].(map[string]interface{})
@@ -145,9 +145,9 @@ func TestIsParentLabelPresent(t *testing.T) {
 		{
 			name: "present",
 			labels: map[string]string{
-				"a":                     "b",
-				"c":                     "d",
-				constants.VrParentLabel: "test",
+				"a":                   "b",
+				"c":                   "d",
+				constants.ParentLabel: "test",
 			},
 			result: true,
 		},
@@ -159,7 +159,7 @@ func TestIsParentLabelPresent(t *testing.T) {
 		{
 			name: "empty value",
 			labels: map[string]string{
-				constants.VrParentLabel: "",
+				constants.ParentLabel: "",
 			},
 			result: false,
 		},
@@ -288,7 +288,7 @@ func TestGetStorageClassGroup(t *testing.T) {
 			ObjectMeta: metav1.ObjectMeta{
 				Name: stcName,
 				Labels: map[string]string{
-					constants.VrStorageClassGroup: groupName,
+					constants.StorageClassGroup: groupName,
 				},
 			},
 		}
@@ -333,7 +333,7 @@ func TestGetLabelsWithParent(t *testing.T) {
 			parent: "test",
 			labels: map[string]string{},
 			result: map[string]string{
-				constants.VrParentLabel: "test",
+				constants.ParentLabel: "test",
 			},
 		},
 		{
@@ -344,29 +344,29 @@ func TestGetLabelsWithParent(t *testing.T) {
 				"c": "d",
 			},
 			result: map[string]string{
-				constants.VrParentLabel: "test",
-				"a":                     "b",
-				"c":                     "d",
+				constants.ParentLabel: "test",
+				"a":                   "b",
+				"c":                   "d",
 			},
 		},
 		{
 			name:   "nil labels",
 			parent: "test",
 			labels: nil,
 			result: map[string]string{
-				constants.VrParentLabel: "test",
+				constants.ParentLabel: "test",
 			},
 		},
 		{
 			name:   "label already present",
 			parent: "new-test",
 			labels: map[string]string{
-				constants.VrParentLabel: "old-test",
-				"a":                     "b",
+				constants.ParentLabel: "old-test",
+				"a":                   "b",
 			},
 			result: map[string]string{
-				constants.VrParentLabel: "new-test",
-				"a":                     "b",
+				constants.ParentLabel: "new-test",
+				"a":                   "b",
 			},
 		},
 	}
@@ -607,3 +607,58 @@ func TestIsVolumeReplicationCorrect(t *testing.T) {
 		})
 	}
 }
+
+func TestGetPvcProvisioner(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name        string
+		annotations map[string]string
+		expected    string
+	}{
+		{
+			name: "standard annotation",
+			annotations: map[string]string{
+				constants.StorageProvisionerAnnotation: "standard-provisioner",
+			},
+			expected: "standard-provisioner",
+		},
+		{
+			name: "deprecated annotation",
+			annotations: map[string]string{
+				constants.DeprecatedStorageProvisionerAnnotation: "deprecated-provisioner",
+			},
+			expected: "deprecated-provisioner",
+		},
+		{
+			name: "both annotations - standard takes precedence",
+			annotations: map[string]string{
+				constants.StorageProvisionerAnnotation:           "standard-provisioner",
+				constants.DeprecatedStorageProvisionerAnnotation: "deprecated-provisioner",
+			},
+			expected: "standard-provisioner",
+		},
+		{
+			name:        "no annotations",
+			annotations: map[string]string{},
+			expected:    "",
+		},
+		{
+			name:        "nil annotations",
+			annotations: nil,
+			expected:    "",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			pvc := &corev1.PersistentVolumeClaim{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: tt.annotations,
+				},
+			}
+			result := getPvcProvisioner(pvc)
+			require.Equal(t, tt.expected, result)
+		})
+	}
+}

internal/replicator/vrc.go

@@ -50,22 +50,22 @@ func getVolumeReplicationClassFromSelector(pvc *corev1.PersistentVolumeClaim) st
 		return ""
 	}
 
-	// Filter all VolumeReplicationClasses in the correct group and with the correct classSelector
-	volumeReplicationClasses, err := filterVrcFromSelector(group, selector)
+	// Filter all VolumeReplicationClasses in the correct group and with the correct classSelector/provisioner
+	volumeReplicationClasses, err := filterVrcFromSelector(group, selector, getPvcProvisioner(pvc))
 	if err != nil {
 		klog.Errorf("failed to filter VRCs for PVC %s/%s: %s", pvc.Namespace, pvc.Name, err.Error())
 		return ""
 	}
 
 	// We expect to find exactly one VolumeReplicationClass
-	if len(volumeReplicationClasses.Items) != 1 {
-		if len(volumeReplicationClasses.Items) > 1 {
-			klog.Errorf("found %d matching VRCs for PVC %s/%s, expected 1", len(volumeReplicationClasses.Items), pvc.Namespace, pvc.Name)
+	if len(volumeReplicationClasses) != 1 {
+		if len(volumeReplicationClasses) > 1 {
+			klog.Errorf("found %d matching VRCs for PVC %s/%s, expected 1", len(volumeReplicationClasses), pvc.Namespace, pvc.Name)
 		}
 		return ""
 	}
 
-	return volumeReplicationClasses.Items[0].GetName()
+	return volumeReplicationClasses[0]
 }
 
 // getVolumeReplicationClassValue returns the VRC to use for a PVC.
@@ -104,13 +104,14 @@ func getAnnotationValue(pvc *corev1.PersistentVolumeClaim, annotation string) st
 }
 
 // filterVrcFromSelector returns a VolumeReplicationClass that is in a specific StorageClass Group
-// and with a specific VolumeReplicationClass selector.
-func filterVrcFromSelector(group, selector string) (*unstructured.UnstructuredList, error) {
+// and with a specific VolumeReplicationClass selector. It also filters for faulty provisioners.
+// It is assumed that a VRC must have a provisioner identical to the provisioner of the PVC.
+func filterVrcFromSelector(group, selector, pvcProvisioner string) ([]string, error) {
 	// Filter only VRCs in the right StorageClass group and with the right selector
 	vrcLister := k8s.DynamicClientSet.Resource(VolumeReplicationClassesResource)
 	labelSelector := &metav1.LabelSelector{
 		MatchLabels: map[string]string{
-			constants.VrStorageClassGroup:   group,
+			constants.StorageClassGroup:     group,
 			constants.VrcSelectorAnnotation: selector,
 		},
 	}
@@ -121,5 +122,15 @@ func filterVrcFromSelector(group, selector string) (*unstructured.UnstructuredLi
 		return nil, err
 	}
 
-	return list, nil
+	// Filter for VRCs that have the same provisioner as our PVC
+	var classes []string
+	for _, item := range list.Items {
+		vrcProvisioner, _, _ := unstructured.NestedString(item.Object, "spec", "provisioner")
+		// Allow the pvcProvisioner to be empty, as some CSI may not place it in any annotation.
+		if vrcProvisioner == pvcProvisioner || pvcProvisioner == "" {
+			classes = append(classes, item.GetName())
+		}
+	}
+
+	return classes, nil
 }