Skip to content

Commit b916ae8

Browse files
SkalaNetworksSkalaNetworks
authored
Merge pull request #11 from super-phenix/filter-provisioner
feat(vrc): filter provisioner when using selectors
2 parents c5f8100 + aaa2de0 commit b916ae8

6 files changed

Lines changed: 182 additions & 54 deletions

File tree

‎internal/constants/const.go‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
package constants
22

33
const (
4-
LockName = "spx-volume-replicator-leader-election"
5-
VrcValueAnnotation = "replication.superphenix.net/class"
6-
VrcSelectorAnnotation = "replication.superphenix.net/classSelector"
7-
VrParentLabel = "replication.superphenix.net/parent"
8-
VrStorageClassGroup = "replication.superphenix.net/storageClassGroup"
4+
LockName = "spx-volume-replicator-leader-election"
5+
VrcValueAnnotation = "replication.superphenix.net/class"
6+
VrcSelectorAnnotation = "replication.superphenix.net/classSelector"
7+
ParentLabel = "replication.superphenix.net/parent"
8+
StorageClassGroup = "replication.superphenix.net/storageClassGroup"
9+
StorageProvisionerAnnotation = "volume.kubernetes.io/storage-provisioner"
10+
DeprecatedStorageProvisionerAnnotation = "volume.beta.kubernetes.io/storage-provisioner"
911
)

‎internal/replicator/replicator_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ func TestReconcileVolumeReplication(t *testing.T) {
5353
"name": pvcName,
5454
"namespace": nsName,
5555
"labels": map[string]interface{}{
56-
constants.VrParentLabel: pvcName,
56+
constants.ParentLabel: pvcName,
5757
},
5858
},
5959
"spec": map[string]interface{}{

‎internal/replicator/utils.go‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ func getVolumeReplication(key string) (*unstructured.Unstructured, error) {
115115

116116
// isParentLabelPresent returns whether a parent label is present on a VolumeReplication
117117
func isParentLabelPresent(labels map[string]string) bool {
118-
return labels[constants.VrParentLabel] != ""
118+
return labels[constants.ParentLabel] != ""
119119
}
120120

121121
// 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]
125125
for k, v := range pvcLabels {
126126
res[k] = v
127127
}
128-
res[constants.VrParentLabel] = parent
128+
res[constants.ParentLabel] = parent
129129
return res
130130
}
131131

@@ -155,5 +155,16 @@ func getStorageClassGroup(pvc *corev1.PersistentVolumeClaim) (string, error) {
155155
}
156156

157157
// Retrieve the group of VolumeReplicationClasses associated with this StorageClass
158-
return stcLabels[constants.VrStorageClassGroup], nil
158+
return stcLabels[constants.StorageClassGroup], nil
159+
}
160+
161+
// getPvcProvisioner returns the dynamic provisioner used to provision a PVC
162+
func getPvcProvisioner(pvc *corev1.PersistentVolumeClaim) string {
163+
// Try the well-known annotation first
164+
if pvc.Annotations[constants.StorageProvisionerAnnotation] != "" {
165+
return pvc.Annotations[constants.StorageProvisionerAnnotation]
166+
}
167+
168+
// Fallback to the deprecated annotation
169+
return pvc.Annotations[constants.DeprecatedStorageProvisionerAnnotation]
159170
}

‎internal/replicator/utils_test.go‎

Lines changed: 70 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ func TestCreateVolumeReplication(t *testing.T) {
6363
require.Equal(t, vrcName, vr.GetAnnotations()[constants.VrcValueAnnotation])
6464
require.Equal(t, "value", vr.GetAnnotations()["other-annotation"])
6565
require.Equal(t, "value", vr.GetLabels()["other-label"])
66-
require.Equal(t, pvcName, vr.GetLabels()[constants.VrParentLabel])
66+
require.Equal(t, pvcName, vr.GetLabels()[constants.ParentLabel])
6767

6868
// Check spec
6969
spec, ok := vr.Object["spec"].(map[string]interface{})
@@ -145,9 +145,9 @@ func TestIsParentLabelPresent(t *testing.T) {
145145
{
146146
name: "present",
147147
labels: map[string]string{
148-
"a": "b",
149-
"c": "d",
150-
constants.VrParentLabel: "test",
148+
"a": "b",
149+
"c": "d",
150+
constants.ParentLabel: "test",
151151
},
152152
result: true,
153153
},
@@ -159,7 +159,7 @@ func TestIsParentLabelPresent(t *testing.T) {
159159
{
160160
name: "empty value",
161161
labels: map[string]string{
162-
constants.VrParentLabel: "",
162+
constants.ParentLabel: "",
163163
},
164164
result: false,
165165
},
@@ -288,7 +288,7 @@ func TestGetStorageClassGroup(t *testing.T) {
288288
ObjectMeta: metav1.ObjectMeta{
289289
Name: stcName,
290290
Labels: map[string]string{
291-
constants.VrStorageClassGroup: groupName,
291+
constants.StorageClassGroup: groupName,
292292
},
293293
},
294294
}
@@ -333,7 +333,7 @@ func TestGetLabelsWithParent(t *testing.T) {
333333
parent: "test",
334334
labels: map[string]string{},
335335
result: map[string]string{
336-
constants.VrParentLabel: "test",
336+
constants.ParentLabel: "test",
337337
},
338338
},
339339
{
@@ -344,29 +344,29 @@ func TestGetLabelsWithParent(t *testing.T) {
344344
"c": "d",
345345
},
346346
result: map[string]string{
347-
constants.VrParentLabel: "test",
348-
"a": "b",
349-
"c": "d",
347+
constants.ParentLabel: "test",
348+
"a": "b",
349+
"c": "d",
350350
},
351351
},
352352
{
353353
name: "nil labels",
354354
parent: "test",
355355
labels: nil,
356356
result: map[string]string{
357-
constants.VrParentLabel: "test",
357+
constants.ParentLabel: "test",
358358
},
359359
},
360360
{
361361
name: "label already present",
362362
parent: "new-test",
363363
labels: map[string]string{
364-
constants.VrParentLabel: "old-test",
365-
"a": "b",
364+
constants.ParentLabel: "old-test",
365+
"a": "b",
366366
},
367367
result: map[string]string{
368-
constants.VrParentLabel: "new-test",
369-
"a": "b",
368+
constants.ParentLabel: "new-test",
369+
"a": "b",
370370
},
371371
},
372372
}
@@ -607,3 +607,58 @@ func TestIsVolumeReplicationCorrect(t *testing.T) {
607607
})
608608
}
609609
}
610+
611+
func TestGetPvcProvisioner(t *testing.T) {
612+
t.Parallel()
613+
614+
tests := []struct {
615+
name string
616+
annotations map[string]string
617+
expected string
618+
}{
619+
{
620+
name: "standard annotation",
621+
annotations: map[string]string{
622+
constants.StorageProvisionerAnnotation: "standard-provisioner",
623+
},
624+
expected: "standard-provisioner",
625+
},
626+
{
627+
name: "deprecated annotation",
628+
annotations: map[string]string{
629+
constants.DeprecatedStorageProvisionerAnnotation: "deprecated-provisioner",
630+
},
631+
expected: "deprecated-provisioner",
632+
},
633+
{
634+
name: "both annotations - standard takes precedence",
635+
annotations: map[string]string{
636+
constants.StorageProvisionerAnnotation: "standard-provisioner",
637+
constants.DeprecatedStorageProvisionerAnnotation: "deprecated-provisioner",
638+
},
639+
expected: "standard-provisioner",
640+
},
641+
{
642+
name: "no annotations",
643+
annotations: map[string]string{},
644+
expected: "",
645+
},
646+
{
647+
name: "nil annotations",
648+
annotations: nil,
649+
expected: "",
650+
},
651+
}
652+
653+
for _, tt := range tests {
654+
t.Run(tt.name, func(t *testing.T) {
655+
pvc := &corev1.PersistentVolumeClaim{
656+
ObjectMeta: metav1.ObjectMeta{
657+
Annotations: tt.annotations,
658+
},
659+
}
660+
result := getPvcProvisioner(pvc)
661+
require.Equal(t, tt.expected, result)
662+
})
663+
}
664+
}

‎internal/replicator/vrc.go‎

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -50,22 +50,22 @@ func getVolumeReplicationClassFromSelector(pvc *corev1.PersistentVolumeClaim) st
5050
return ""
5151
}
5252

53-
// Filter all VolumeReplicationClasses in the correct group and with the correct classSelector
54-
volumeReplicationClasses, err := filterVrcFromSelector(group, selector)
53+
// Filter all VolumeReplicationClasses in the correct group and with the correct classSelector/provisioner
54+
volumeReplicationClasses, err := filterVrcFromSelector(group, selector, getPvcProvisioner(pvc))
5555
if err != nil {
5656
klog.Errorf("failed to filter VRCs for PVC %s/%s: %s", pvc.Namespace, pvc.Name, err.Error())
5757
return ""
5858
}
5959

6060
// We expect to find exactly one VolumeReplicationClass
61-
if len(volumeReplicationClasses.Items) != 1 {
62-
if len(volumeReplicationClasses.Items) > 1 {
63-
klog.Errorf("found %d matching VRCs for PVC %s/%s, expected 1", len(volumeReplicationClasses.Items), pvc.Namespace, pvc.Name)
61+
if len(volumeReplicationClasses) != 1 {
62+
if len(volumeReplicationClasses) > 1 {
63+
klog.Errorf("found %d matching VRCs for PVC %s/%s, expected 1", len(volumeReplicationClasses), pvc.Namespace, pvc.Name)
6464
}
6565
return ""
6666
}
6767

68-
return volumeReplicationClasses.Items[0].GetName()
68+
return volumeReplicationClasses[0]
6969
}
7070

7171
// getVolumeReplicationClassValue returns the VRC to use for a PVC.
@@ -104,13 +104,14 @@ func getAnnotationValue(pvc *corev1.PersistentVolumeClaim, annotation string) st
104104
}
105105

106106
// filterVrcFromSelector returns a VolumeReplicationClass that is in a specific StorageClass Group
107-
// and with a specific VolumeReplicationClass selector.
108-
func filterVrcFromSelector(group, selector string) (*unstructured.UnstructuredList, error) {
107+
// and with a specific VolumeReplicationClass selector. It also filters for faulty provisioners.
108+
// It is assumed that a VRC must have a provisioner identical to the provisioner of the PVC.
109+
func filterVrcFromSelector(group, selector, pvcProvisioner string) ([]string, error) {
109110
// Filter only VRCs in the right StorageClass group and with the right selector
110111
vrcLister := k8s.DynamicClientSet.Resource(VolumeReplicationClassesResource)
111112
labelSelector := &metav1.LabelSelector{
112113
MatchLabels: map[string]string{
113-
constants.VrStorageClassGroup: group,
114+
constants.StorageClassGroup: group,
114115
constants.VrcSelectorAnnotation: selector,
115116
},
116117
}
@@ -121,5 +122,15 @@ func filterVrcFromSelector(group, selector string) (*unstructured.UnstructuredLi
121122
return nil, err
122123
}
123124

124-
return list, nil
125+
// Filter for VRCs that have the same provisioner as our PVC
126+
var classes []string
127+
for _, item := range list.Items {
128+
vrcProvisioner, _, _ := unstructured.NestedString(item.Object, "spec", "provisioner")
129+
// Allow the pvcProvisioner to be empty, as some CSI may not place it in any annotation.
130+
if vrcProvisioner == pvcProvisioner || pvcProvisioner == "" {
131+
classes = append(classes, item.GetName())
132+
}
133+
}
134+
135+
return classes, nil
125136
}

0 commit comments

Comments
 (0)