From 1a3b6665dd53e8edf6a89802601ee33215a51ffd Mon Sep 17 00:00:00 2001 From: Omar Pakker Date: Thu, 28 Mar 2024 19:33:30 +0100 Subject: [PATCH] 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. --- helm/provisioner/templates/configmap.yaml | 10 ++- pkg/common/common.go | 3 + pkg/common/common_test.go | 40 +++++++++ pkg/discovery/discovery.go | 101 +++++++--------------- pkg/discovery/discovery_test.go | 8 +- 5 files changed, 87 insertions(+), 75 deletions(-) diff --git a/helm/provisioner/templates/configmap.yaml b/helm/provisioner/templates/configmap.yaml index 56ecbfb09..20d539130 100644 --- 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 index c55b4d2f3..915d3c766 100644 --- a/pkg/common/common.go +++ b/pkg/common/common.go @@ -138,6 +138,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 index 49756f347..3dee6afcc 100644 --- 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 index ba676f5d8..ba76f13ff 100644 --- 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" @@ -46,11 +47,10 @@ type Discoverer struct { Labels map[string]string // 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 - classLister storagev1listers.StorageClassLister - ownerReference *metav1.OwnerReference + CleanupTracker *deleter.CleanupStatusTracker + nodeSelector *v1.NodeSelector + classLister storagev1listers.StorageClassLister + ownerReference *metav1.OwnerReference Readyz *readyzCheck } @@ -106,30 +106,9 @@ 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{ @@ -137,7 +116,7 @@ func NewDiscoverer(config *common.RuntimeConfig, cleanupTracker *deleter.Cleanup 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 index 55fbdf3a6..30e94950e 100644 --- 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") } }