Allow additional node selector terms to be set

This enables additional node selector terms to be added aside from the provisioner node, enabling use-cases such as shared disks, as affinity can not be changed after provisioning the PV. Limitation in the current implementation is that the provisioner can /not/ be ANDed with additional terms. This change only allows for additional terms that will be /ORed/ with the provisioner name term.
kubernetes-sigs · May 13, 2024 · c5d5074 · c5d5074
1 parent 879f9e1
commit c5d5074
Show file tree

Hide file tree

Showing 5 changed files with 84 additions and 72 deletions.
diff --git a/helm/provisioner/templates/configmap.yaml b/helm/provisioner/templates/configmap.yaml
@@ -26,13 +26,13 @@ data:
 {{- end }}
 {{- if .Values.useJobForCleaning }}
   useJobForCleaning: "yes"
-{{- end}}
+{{- end }}
 {{- if .Values.useNodeNameOnly }}
   useNodeNameOnly: "true"
 {{- end }}
 {{- if .Values.minResyncPeriod }}
   minResyncPeriod: {{ .Values.minResyncPeriod | quote }}
-{{- end}}
+{{- end }}
   storageClassMap: |
     {{- range $classConfig := .Values.classes }}
     {{ $classConfig.name }}:
@@ -42,7 +42,7 @@ data:
       blockCleanerCommand:
       {{- range $val := $classConfig.blockCleanerCommand }}
         - {{ $val | quote }}
-      {{- end}}
+      {{- end }}
       {{- end }}
       {{- if $classConfig.volumeMode }}
       volumeMode: {{ $classConfig.volumeMode }}
@@ -53,4 +53,8 @@ data:
       {{- if $classConfig.namePattern }}
       namePattern: {{ $classConfig.namePattern | quote }}
       {{- end }}
+      {{- if $classConfig.selector }}
+      selector:
+      {{- toYaml $classConfig.selector | nindent 8 }}
+      {{- end }}
     {{- end }}
diff --git a/pkg/common/common.go b/pkg/common/common.go
@@ -136,6 +136,9 @@ type MountConfig struct {
 	// NamePattern name pattern check
 	// only discover file name matching pattern("*" by default)
 	NamePattern string `json:"namePattern" yaml:"namePattern"`
+	// Additional selector terms to set for node affinity in addition to the provisioner node name.
+	// Useful for shared disks as affinity can not be changed after provisioning the PV.
+	Selector []v1.NodeSelectorTerm `json:"selector" yaml:"selector"`
 }
 
 // RuntimeConfig stores all the objects that the provisioner needs to run

diff --git a/pkg/common/common_test.go b/pkg/common/common_test.go
@@ -236,6 +236,46 @@ func TestLoadProvisionerConfigs(t *testing.T) {
 			},
 			nil,
 		},
+		{
+			map[string]string{"storageClassMap": `local-storage:
+   hostDir: /mnt/disks
+   mountDir: /mnt/disks
+   selector:
+     - matchExpressions:
+         - key: "kubernetes.io/hostname"
+           operator: "In"
+           values:
+             - otherNode1
+`,
+			},
+			ProvisionerConfiguration{
+				StorageClassConfig: map[string]MountConfig{
+					"local-storage": {
+						HostDir:             "/mnt/disks",
+						MountDir:            "/mnt/disks",
+						BlockCleanerCommand: []string{"/scripts/quick_reset.sh"},
+						VolumeMode:          "Filesystem",
+						NamePattern:         "*",
+						Selector:            []v1.NodeSelectorTerm{
+							{
+								MatchExpressions: []v1.NodeSelectorRequirement{
+									{
+										Key: "kubernetes.io/hostname",
+										Operator: v1.NodeSelectorOpIn,
+										Values: []string{"otherNode1"},
+									},
+								},
+							},
+						},
+					},
+				},
+				UseAlphaAPI: true,
+				MinResyncPeriod: metav1.Duration{
+					Duration: time.Hour + time.Minute*30,
+				},
+			},
+			nil,
+		},
 	}
 	for _, v := range testcases {
 		for name, value := range v.data {

diff --git a/pkg/discovery/discovery.go b/pkg/discovery/discovery.go
@@ -23,6 +23,7 @@ import (
 	"hash/fnv"
 	"net/http"
 	"path/filepath"
+	"slices"
 	"strings"
 	"sync"
 	"time"
@@ -47,8 +48,7 @@ type Discoverer struct {
 	// ProcTable is a reference to running processes so that we can prevent PV from being created while
 	// it is being cleaned
 	CleanupTracker  *deleter.CleanupStatusTracker
-	nodeAffinityAnn string
-	nodeAffinity    *v1.VolumeNodeAffinity
+	nodeSelector    *v1.NodeSelector
 	classLister     storagev1listers.StorageClassLister
 	ownerReference  *metav1.OwnerReference
 
@@ -106,38 +106,17 @@ func NewDiscoverer(config *common.RuntimeConfig, cleanupTracker *deleter.Cleanup
 		return nil, fmt.Errorf("Failed to generate owner reference: %v", err)
 	}
 
-	if config.UseAlphaAPI {
-		nodeAffinity, err := generateNodeAffinity(config.Node)
-		if err != nil {
-			return nil, fmt.Errorf("Failed to generate node affinity: %v", err)
-		}
-		tmpAnnotations := map[string]string{}
-		err = StorageNodeAffinityToAlphaAnnotation(tmpAnnotations, nodeAffinity)
-		if err != nil {
-			return nil, fmt.Errorf("Failed to convert node affinity to alpha annotation: %v", err)
-		}
-		return &Discoverer{
-			RuntimeConfig:   config,
-			Labels:          labelMap,
-			CleanupTracker:  cleanupTracker,
-			classLister:     sharedInformer.Lister(),
-			nodeAffinityAnn: tmpAnnotations[common.AlphaStorageNodeAffinityAnnotation],
-			ownerReference:  ownerRef,
-			Readyz:          &readyzCheck{},
-		}, nil
-	}
-
-	volumeNodeAffinity, err := generateVolumeNodeAffinity(config.Node)
+	nodeSelector, err := generateNodeSelector(config.Node)
 	if err != nil {
-		return nil, fmt.Errorf("Failed to generate volume node affinity: %v", err)
+		return nil, fmt.Errorf("Failed to generate node selector: %v", err)
 	}
 
 	return &Discoverer{
 		RuntimeConfig:  config,
 		Labels:         labelMap,
 		CleanupTracker: cleanupTracker,
 		classLister:    sharedInformer.Lister(),
-		nodeAffinity:   volumeNodeAffinity,
+		nodeSelector:   nodeSelector,
 		ownerReference: ownerRef,
 		Readyz:         &readyzCheck{},
 	}, nil
@@ -160,7 +139,7 @@ func generateOwnerReference(node *v1.Node) (*metav1.OwnerReference, error) {
 	}, nil
 }
 
-func generateNodeAffinity(node *v1.Node) (*v1.NodeAffinity, error) {
+func generateNodeSelector(node *v1.Node) (*v1.NodeSelector, error) {
 	if node.Labels == nil {
 		return nil, fmt.Errorf("Node does not have labels")
 	}
@@ -169,42 +148,14 @@ func generateNodeAffinity(node *v1.Node) (*v1.NodeAffinity, error) {
 		return nil, fmt.Errorf("Node does not have expected label %s", common.NodeLabelKey)
 	}
 
-	return &v1.NodeAffinity{
-		RequiredDuringSchedulingIgnoredDuringExecution: &v1.NodeSelector{
-			NodeSelectorTerms: []v1.NodeSelectorTerm{
-				{
-					MatchExpressions: []v1.NodeSelectorRequirement{
-						{
-							Key:      common.NodeLabelKey,
-							Operator: v1.NodeSelectorOpIn,
-							Values:   []string{nodeValue},
-						},
-					},
-				},
-			},
-		},
-	}, nil
-}
-
-func generateVolumeNodeAffinity(node *v1.Node) (*v1.VolumeNodeAffinity, error) {
-	if node.Labels == nil {
-		return nil, fmt.Errorf("Node does not have labels")
-	}
-	nodeValue, found := node.Labels[common.NodeLabelKey]
-	if !found {
-		return nil, fmt.Errorf("Node does not have expected label %s", common.NodeLabelKey)
-	}
-
-	return &v1.VolumeNodeAffinity{
-		Required: &v1.NodeSelector{
-			NodeSelectorTerms: []v1.NodeSelectorTerm{
-				{
-					MatchExpressions: []v1.NodeSelectorRequirement{
-						{
-							Key:      common.NodeLabelKey,
-							Operator: v1.NodeSelectorOpIn,
-							Values:   []string{nodeValue},
-						},
+	return  &v1.NodeSelector{
+		NodeSelectorTerms: []v1.NodeSelectorTerm{
+			{
+				MatchExpressions: []v1.NodeSelectorRequirement{
+					{
+						Key:      common.NodeLabelKey,
+						Operator: v1.NodeSelectorOpIn,
+						Values:   []string{nodeValue},
 					},
 				},
 			},
@@ -437,11 +388,25 @@ func (d *Discoverer) createPV(file, class string, reclaimPolicy v1.PersistentVol
 		OwnerReference:  d.ownerReference,
 	}
 
+	volumeNodeSelector := &v1.NodeSelector{
+		NodeSelectorTerms: slices.Concat(d.nodeSelector.NodeSelectorTerms, config.Selector),
+	}
+
 	if d.UseAlphaAPI {
+		nodeAffinity := &v1.NodeAffinity{
+			RequiredDuringSchedulingIgnoredDuringExecution: volumeNodeSelector,
+		}
+		tmpAnnotations := map[string]string{}
+		err := StorageNodeAffinityToAlphaAnnotation(tmpAnnotations, nodeAffinity)
+		if err != nil {
+			return fmt.Errorf("error converting volume affinity to alpha annotation: %v", err)
+		}
 		localPVConfig.UseAlphaAPI = true
-		localPVConfig.AffinityAnn = d.nodeAffinityAnn
+		localPVConfig.AffinityAnn = tmpAnnotations[common.AlphaStorageNodeAffinityAnnotation]
 	} else {
-		localPVConfig.NodeAffinity = d.nodeAffinity
+		localPVConfig.NodeAffinity = &v1.VolumeNodeAffinity{
+			Required: volumeNodeSelector,
+		}
 	}
 
 	if config.FsType != "" {

diff --git a/pkg/discovery/discovery_test.go b/pkg/discovery/discovery_test.go
@@ -753,16 +753,16 @@ func TestUseAlphaAPI(t *testing.T) {
 	if d.UseAlphaAPI {
 		t.Fatal("UseAlphaAPI should be false")
 	}
-	if len(d.nodeAffinityAnn) != 0 || d.nodeAffinity == nil {
-		t.Fatal("the value nodeAffinityAnn shouldn't be set while nodeAffinity should")
+	if d.nodeSelector == nil {
+		t.Fatal("the value nodeSelector should be set")
 	}
 
 	d = testSetup(t, test, true, false)
 	if !d.UseAlphaAPI {
 		t.Fatal("UseAlphaAPI should be true")
 	}
-	if d.nodeAffinity != nil || len(d.nodeAffinityAnn) == 0 {
-		t.Fatal("the value nodeAffinityAnn should be set while nodeAffinity should not")
+	if d.nodeSelector == nil {
+		t.Fatal("the value nodeSelector should be set")
 	}
 }