From aaa2de037cb749f3d01381b080b905196e38e166 Mon Sep 17 00:00:00 2001 From: SkalaNetworks Date: Mon, 5 Jan 2026 23:31:17 +0100 Subject: [PATCH] feat(vrc): filter provisioner when using selectors Signed-off-by: SkalaNetworks --- internal/constants/const.go | 12 ++-- internal/replicator/replicator_test.go | 2 +- internal/replicator/utils.go | 17 ++++- internal/replicator/utils_test.go | 85 +++++++++++++++++++----- internal/replicator/vrc.go | 31 ++++++--- internal/replicator/vrc_test.go | 89 ++++++++++++++++++++------ 6 files changed, 182 insertions(+), 54 deletions(-) diff --git a/internal/constants/const.go b/internal/constants/const.go index f9b00f8..80d3301 100644 --- a/internal/constants/const.go +++ b/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" ) diff --git a/internal/replicator/replicator_test.go b/internal/replicator/replicator_test.go index a9eabee..4bc97f7 100644 --- a/internal/replicator/replicator_test.go +++ b/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{}{ diff --git a/internal/replicator/utils.go b/internal/replicator/utils.go index 79b5929..09bcff7 100644 --- a/internal/replicator/utils.go +++ b/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] } diff --git a/internal/replicator/utils_test.go b/internal/replicator/utils_test.go index f43b9a1..41b3298 100644 --- a/internal/replicator/utils_test.go +++ b/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,9 +344,9 @@ 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", }, }, { @@ -354,19 +354,19 @@ func TestGetLabelsWithParent(t *testing.T) { 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) + }) + } +} diff --git a/internal/replicator/vrc.go b/internal/replicator/vrc.go index 71756c0..b6a02bf 100644 --- a/internal/replicator/vrc.go +++ b/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 } diff --git a/internal/replicator/vrc_test.go b/internal/replicator/vrc_test.go index be8c9e2..1c4041d 100644 --- a/internal/replicator/vrc_test.go +++ b/internal/replicator/vrc_test.go @@ -52,12 +52,14 @@ func TestGetVolumeReplicationClass(t *testing.T) { stcName := "test-storage-class" groupName := "test-group" + provisionerName := "test-provisioner" + // Create a StorageClass stc := &storagev1.StorageClass{ ObjectMeta: metav1.ObjectMeta{ Name: stcName, Labels: map[string]string{ - constants.VrStorageClassGroup: groupName, + constants.StorageClassGroup: groupName, }, }, } @@ -71,10 +73,13 @@ func TestGetVolumeReplicationClass(t *testing.T) { "metadata": map[string]interface{}{ "name": "vrc-matched", "labels": map[string]interface{}{ - constants.VrStorageClassGroup: groupName, + constants.StorageClassGroup: groupName, constants.VrcSelectorAnnotation: selectorValue, }, }, + "spec": map[string]interface{}{ + "provisioner": provisionerName, + }, }, } _, _ = dynamicClient.Resource(VolumeReplicationClassesResource).Create(context.Background(), vrc, metav1.CreateOptions{}) @@ -123,7 +128,8 @@ func TestGetVolumeReplicationClass(t *testing.T) { Name: "test-pvc", Namespace: nsName, Annotations: map[string]string{ - constants.VrcSelectorAnnotation: selectorValue, + constants.VrcSelectorAnnotation: selectorValue, + constants.StorageProvisionerAnnotation: provisionerName, }, }, Spec: corev1.PersistentVolumeClaimSpec{ @@ -138,6 +144,9 @@ func TestGetVolumeReplicationClass(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "test-pvc", Namespace: nsName, + Annotations: map[string]string{ + constants.StorageProvisionerAnnotation: provisionerName, + }, }, Spec: corev1.PersistentVolumeClaimSpec{ StorageClassName: &stcName, @@ -418,6 +427,7 @@ func TestGetVolumeReplicationClassFromSelector(t *testing.T) { stcName := "test-storage-class" groupName := "test-group" selectorValue := "test-selector" + provisionerName := "test-provisioner" vrc := &unstructured.Unstructured{ Object: map[string]interface{}{ @@ -426,10 +436,13 @@ func TestGetVolumeReplicationClassFromSelector(t *testing.T) { "metadata": map[string]interface{}{ "name": "vrc-matched", "labels": map[string]interface{}{ - constants.VrStorageClassGroup: groupName, + constants.StorageClassGroup: groupName, constants.VrcSelectorAnnotation: selectorValue, }, }, + "spec": map[string]interface{}{ + "provisioner": provisionerName, + }, }, } _, _ = dynamicClient.Resource(VolumeReplicationClassesResource).Create(context.Background(), vrc, metav1.CreateOptions{}) @@ -438,7 +451,7 @@ func TestGetVolumeReplicationClassFromSelector(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: stcName, Labels: map[string]string{ - constants.VrStorageClassGroup: groupName, + constants.StorageClassGroup: groupName, }, }, } @@ -458,7 +471,8 @@ func TestGetVolumeReplicationClassFromSelector(t *testing.T) { pvc := &corev1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - constants.VrcSelectorAnnotation: selectorValue, + constants.VrcSelectorAnnotation: selectorValue, + constants.StorageProvisionerAnnotation: provisionerName, }, }, Spec: corev1.PersistentVolumeClaimSpec{ @@ -469,6 +483,22 @@ func TestGetVolumeReplicationClassFromSelector(t *testing.T) { require.Equal(t, "vrc-matched", result) }) + t.Run("PVC with annotation, matching provisioner not found", func(t *testing.T) { + pvc := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + constants.VrcSelectorAnnotation: selectorValue, + constants.StorageProvisionerAnnotation: "other-provisioner", + }, + }, + Spec: corev1.PersistentVolumeClaimSpec{ + StorageClassName: &stcName, + }, + } + result := getVolumeReplicationClassFromSelector(pvc) + require.Equal(t, "", result) + }) + t.Run("StorageClass has no group", func(t *testing.T) { stcNoGroup := "stc-no-group" stc := &storagev1.StorageClass{ @@ -518,10 +548,13 @@ func TestGetVolumeReplicationClassFromSelector(t *testing.T) { "metadata": map[string]interface{}{ "name": "vrc-matched-2", "labels": map[string]interface{}{ - constants.VrStorageClassGroup: groupName, + constants.StorageClassGroup: groupName, constants.VrcSelectorAnnotation: selectorValue, }, }, + "spec": map[string]interface{}{ + "provisioner": provisionerName, + }, }, } _, _ = dynamicClient.Resource(VolumeReplicationClassesResource).Create(context.Background(), vrc2, metav1.CreateOptions{}) @@ -532,7 +565,8 @@ func TestGetVolumeReplicationClassFromSelector(t *testing.T) { pvc := &corev1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - constants.VrcSelectorAnnotation: selectorValue, + constants.VrcSelectorAnnotation: selectorValue, + constants.StorageProvisionerAnnotation: provisionerName, }, }, Spec: corev1.PersistentVolumeClaimSpec{ @@ -588,10 +622,13 @@ func TestFilterVrcFromSelector(t *testing.T) { "metadata": map[string]interface{}{ "name": "vrc-1", "labels": map[string]interface{}{ - constants.VrStorageClassGroup: "group-1", + constants.StorageClassGroup: "group-1", constants.VrcSelectorAnnotation: "match", }, }, + "spec": map[string]interface{}{ + "provisioner": "provisioner-1", + }, }, } @@ -602,29 +639,41 @@ func TestFilterVrcFromSelector(t *testing.T) { "metadata": map[string]interface{}{ "name": "vrc-2", "labels": map[string]interface{}{ - constants.VrStorageClassGroup: "group-2", + constants.StorageClassGroup: "group-2", constants.VrcSelectorAnnotation: "no-match", }, }, + "spec": map[string]interface{}{ + "provisioner": "provisioner-2", + }, }, } _, _ = dynamicClient.Resource(VolumeReplicationClassesResource).Create(context.Background(), vrc1, metav1.CreateOptions{}) _, _ = dynamicClient.Resource(VolumeReplicationClassesResource).Create(context.Background(), vrc2, metav1.CreateOptions{}) - t.Run("Match found with both labels", func(t *testing.T) { - list, err := filterVrcFromSelector("group-1", "match") + t.Run("Match found with both labels and provisioner", func(t *testing.T) { + list, err := filterVrcFromSelector("group-1", "match", "provisioner-1") + require.NoError(t, err) + require.Equal(t, []string{"vrc-1"}, list) + }) + + t.Run("No match found - wrong provisioner", func(t *testing.T) { + list, err := filterVrcFromSelector("group-1", "match", "wrong-provisioner") + require.NoError(t, err) + require.Empty(t, list) + }) + + t.Run("No match found - wrong selector", func(t *testing.T) { + list, err := filterVrcFromSelector("group-1", "no-match", "provisioner-1") require.NoError(t, err) - require.NotNil(t, list) - require.Len(t, list.Items, 1) - require.Equal(t, "vrc-1", list.Items[0].GetName()) + require.Empty(t, list) }) - t.Run("No match found", func(t *testing.T) { - list, err := filterVrcFromSelector("group-1", "no-match") + t.Run("Match found - empty pvcProvisioner", func(t *testing.T) { + list, err := filterVrcFromSelector("group-1", "match", "") require.NoError(t, err) - require.NotNil(t, list) - require.Len(t, list.Items, 0) + require.Equal(t, []string{"vrc-1"}, list) }) t.Run("API error", func(t *testing.T) { @@ -634,7 +683,7 @@ func TestFilterVrcFromSelector(t *testing.T) { }) defer func() { dynamicClient.ReactionChain = dynamicClient.ReactionChain[1:] }() - list, err := filterVrcFromSelector("group-1", "match") + list, err := filterVrcFromSelector("group-1", "match", "provisioner-1") require.Error(t, err) require.Contains(t, err.Error(), "injected list error") require.Nil(t, list)