Bugfix: pvs not deleting was: Remove the assumption that a node's name == its hostname

provisioner.go

@@ -422,20 +422,16 @@ func (p *LocalPathProvisioner) provisionFor(opts pvController.ProvisionOptions,
 		// affinity, as path is accessible from any node
 		nodeAffinity = nil
 	} else {
-		valueNode, ok := node.GetLabels()[KeyNode]
-		if !ok {
-			valueNode = nodeName
-		}
 		nodeAffinity = &v1.VolumeNodeAffinity{
 			Required: &v1.NodeSelector{
 				NodeSelectorTerms: []v1.NodeSelectorTerm{
 					{
-						MatchExpressions: []v1.NodeSelectorRequirement{
+						MatchFields: []v1.NodeSelectorRequirement{
 							{
-								Key:      KeyNode,
+								Key:      metav1.ObjectNameField,
 								Operator: v1.NodeSelectorOpIn,
 								Values: []string{
-									valueNode,
+									node.Name,
 								},
 							},
 						},
@@ -475,7 +471,7 @@ func (p *LocalPathProvisioner) deleteFor(pv *v1.PersistentVolume, c *StorageClas
 		err = errors.Wrapf(err, "failed to delete volume %v", pv.Name)
 	}()
 
-	path, node, err := p.getPathAndNodeForPV(pv, c)
+	path, node, nodeLabels, err := p.getPathAndNodeForPV(pv, c)
 	if err != nil {
 		return err
 	}
@@ -498,6 +494,7 @@ func (p *LocalPathProvisioner) deleteFor(pv *v1.PersistentVolume, c *StorageClas
 			Mode:        *pv.Spec.VolumeMode,
 			SizeInBytes: storage.Value(),
 			Node:        node,
+			NodeLabels:  nodeLabels,
 		}, c); err != nil {
 			logrus.Infof("clean up volume %v failed: %v", pv.Name, err)
 			return err
@@ -508,7 +505,7 @@ func (p *LocalPathProvisioner) deleteFor(pv *v1.PersistentVolume, c *StorageClas
 	return nil
 }
 
-func (p *LocalPathProvisioner) getPathAndNodeForPV(pv *v1.PersistentVolume, cfg *StorageClassConfig) (path, node string, err error) {
+func (p *LocalPathProvisioner) getPathAndNodeForPV(pv *v1.PersistentVolume, cfg *StorageClassConfig) (path, node string, nodeLabels map[string]string, err error) {
 	defer func() {
 		err = errors.Wrapf(err, "failed to delete volume %v", pv.Name)
 	}()
@@ -519,49 +516,55 @@ func (p *LocalPathProvisioner) getPathAndNodeForPV(pv *v1.PersistentVolume, cfg
 	} else if volumeSource.Local != nil && volumeSource.HostPath == nil {
 		path = volumeSource.Local.Path
 	} else {
-		return "", "", fmt.Errorf("no path set")
+		return "", "", nil, fmt.Errorf("no path set")
 	}
 
 	sharedFS, err := p.isSharedFilesystem(cfg)
 	if err != nil {
-		return "", "", err
+		return "", "", nil, err
 	}
 
 	if sharedFS {
 		// We don't have affinity and can use any node
-		return path, "", nil
+		return path, "", nil, nil
 	}
 
 	// Dealing with local filesystem
 
 	nodeAffinity := pv.Spec.NodeAffinity
 	if nodeAffinity == nil {
-		return "", "", fmt.Errorf("no NodeAffinity set")
+		return "", "", nil, fmt.Errorf("no NodeAffinity set")
 	}
 	required := nodeAffinity.Required
 	if required == nil {
-		return "", "", fmt.Errorf("no NodeAffinity.Required set")
+		return "", "", nil, fmt.Errorf("no NodeAffinity.Required set")
 	}
 
-	node = ""
+	// If we have an explicit node, use that; otherwise use the selector.
+	for _, selectorTerm := range required.NodeSelectorTerms {
+		for _, expression := range selectorTerm.MatchFields {
+			if expression.Key == metav1.ObjectNameField && expression.Operator == v1.NodeSelectorOpIn {
+				if len(expression.Values) != 1 {
+					return "", "", nil, fmt.Errorf("multiple values for the node affinity")
+				}
+				return path, expression.Values[0], nil, nil
+			}
+		}
+	}
+	// The scheduler must use the PV's node selector to schedule a helper pod.
 	for _, selectorTerm := range required.NodeSelectorTerms {
 		for _, expression := range selectorTerm.MatchExpressions {
 			if expression.Key == KeyNode && expression.Operator == v1.NodeSelectorOpIn {
 				if len(expression.Values) != 1 {
-					return "", "", fmt.Errorf("multiple values for the node affinity")
+					return "", "", nil, fmt.Errorf("multiple values for the node affinity")
 				}
-				node = expression.Values[0]
-				break
+				return path, "", map[string]string{
+					KeyNode: expression.Values[0],
+				}, nil
 			}
 		}
-		if node != "" {
-			break
-		}
 	}
-	if node == "" {
-		return "", "", fmt.Errorf("cannot find affinited node")
-	}
-	return path, node, nil
+	return "", "", nil, fmt.Errorf("cannot find affinited node")
 }
 
 type volumeOptions struct {
@@ -570,6 +573,7 @@ type volumeOptions struct {
 	Mode        v1.PersistentVolumeMode
 	SizeInBytes int64
 	Node        string
+	NodeLabels  map[string]string
 }
 
 func (p *LocalPathProvisioner) createHelperPod(action ActionType, cmd []string, o volumeOptions, cfg *StorageClassConfig) (err error) {
@@ -580,7 +584,7 @@ func (p *LocalPathProvisioner) createHelperPod(action ActionType, cmd []string,
 	if err != nil {
 		return err
 	}
-	if o.Name == "" || o.Path == "" || (!sharedFS && o.Node == "") {
+	if o.Name == "" || o.Path == "" || (!sharedFS && o.Node == "" && o.NodeLabels == nil) {
 		return fmt.Errorf("invalid empty name or path or node")
 	}
 	if !filepath.IsAbs(o.Path) {
@@ -661,9 +665,8 @@ func (p *LocalPathProvisioner) createHelperPod(action ActionType, cmd []string,
 		helperPod.Name = helperPod.Name[:HelperPodNameMaxLength]
 	}
 	helperPod.Namespace = p.namespace
-	if o.Node != "" {
-		helperPod.Spec.NodeName = o.Node
-	}
+	helperPod.Spec.NodeName = o.Node
+	helperPod.Spec.NodeSelector = o.NodeLabels
 	helperPod.Spec.ServiceAccountName = p.serviceAccountName
 	helperPod.Spec.RestartPolicy = v1.RestartPolicyNever
 	helperPod.Spec.Tolerations = append(helperPod.Spec.Tolerations, lpvTolerations...)

test/pod_test.go

@@ -82,38 +82,38 @@ func TestPVCTestSuite(t *testing.T) {
 func (p *PodTestSuite) TestPodWithHostPathVolume() {
 	p.kustomizeDir = "pod"
 
-	runTest(p, []string{p.config.IMAGE}, "ready", hostPathVolumeType)
+	runTest(p, []string{p.config.IMAGE}, waitCondition("ready"), hostPathVolumeType)
 }
 
 func (p *PodTestSuite) TestPodWithLocalVolume() {
 	p.kustomizeDir = "pod-with-local-volume"
 
-	runTest(p, []string{p.config.IMAGE}, "ready", localVolumeType)
+	runTest(p, []string{p.config.IMAGE}, waitCondition("ready"), localVolumeType)
 }
 
 func (p *PodTestSuite) TestPodWithLocalVolumeDefault() {
 	p.kustomizeDir = "pod-with-default-local-volume"
 
-	runTest(p, []string{p.config.IMAGE}, "ready", localVolumeType)
+	runTest(p, []string{p.config.IMAGE}, waitCondition("ready"), localVolumeType)
 }
 
 func (p *PodTestSuite) TestPodWithNodeAffinity() {
 	p.kustomizeDir = "pod-with-node-affinity"
 
-	runTest(p, []string{p.config.IMAGE}, "ready", hostPathVolumeType)
+	runTest(p, []string{p.config.IMAGE}, waitCondition("ready"), hostPathVolumeType)
 }
 
 func (p *PodTestSuite) TestPodWithRWOPVolume() {
 	p.kustomizeDir = "pod-with-rwop-volume"
 
-	runTest(p, []string{p.config.IMAGE}, "ready", localVolumeType)
+	runTest(p, []string{p.config.IMAGE}, waitCondition("ready"), localVolumeType)
 }
 
 func (p *PodTestSuite) TestPodWithSecurityContext() {
 	p.kustomizeDir = "pod-with-security-context"
 	kustomizeDir := testdataFile(p.kustomizeDir)
 
-	runTest(p, []string{p.config.IMAGE}, "podscheduled", hostPathVolumeType)
+	runTest(p, []string{p.config.IMAGE}, waitCondition("podscheduled"), hostPathVolumeType)
 
 	cmd := fmt.Sprintf(`kubectl get pod -l %s=%s -o=jsonpath='{.items[0].status.conditions[?(@.type=="Ready")].reason}'`, LabelKey, LabelValue)
 
@@ -142,22 +142,33 @@ loop:
 func (p *PodTestSuite) TestPodWithSubpath() {
 	p.kustomizeDir = "pod-with-subpath"
 
-	runTest(p, []string{p.config.IMAGE}, "ready", hostPathVolumeType)
+	runTest(p, []string{p.config.IMAGE}, waitCondition("ready"), hostPathVolumeType)
 }
 
 func (p *PodTestSuite) TestPodWithMultipleStorageClasses() {
 	p.kustomizeDir = "multiple-storage-classes"
 
-	runTest(p, []string{p.config.IMAGE}, "ready", hostPathVolumeType)
+	runTest(p, []string{p.config.IMAGE}, waitCondition("ready"), hostPathVolumeType)
 }
 
 func (p *PodTestSuite) TestPodWithCustomPathPatternStorageClasses() {
 	p.kustomizeDir = "custom-path-pattern"
 
-	runTest(p, []string{p.config.IMAGE}, "ready", hostPathVolumeType)
+	runTest(p, []string{p.config.IMAGE}, waitCondition("ready"), hostPathVolumeType)
 }
 
-func runTest(p *PodTestSuite, images []string, waitCondition, volumeType string) {
+func (p *PodTestSuite) TestPodWithLegacyAffinityConstraint() {
+	// The helper pod should be correctly scheduled
+	p.kustomizeDir = "pv-with-legacy-affinity"
+
+	runTest(p, []string{p.config.IMAGE}, "kubectl wait pv pvc-to-clean-up --for delete --timeout=120s", "")
+}
+
+func waitCondition(waitCondition string) string {
+	return fmt.Sprintf("kubectl wait pod -l %s=%s --for condition=%s --timeout=120s", LabelKey, LabelValue, waitCondition)
+}
+
+func runTest(p *PodTestSuite, images []string, waitCmd, volumeType string) {
 	kustomizeDir := testdataFile(p.kustomizeDir)
 
 	var cmds []string
@@ -171,7 +182,7 @@ func runTest(p *PodTestSuite, images []string, waitCondition, volumeType string)
 		cmds,
 		fmt.Sprintf("kustomize edit add label %s:%s -f", LabelKey, LabelValue),
 		"kustomize build | kubectl apply -f -",
-		fmt.Sprintf("kubectl wait pod -l %s=%s --for condition=%s --timeout=120s", LabelKey, LabelValue, waitCondition),
+		waitCmd,
 	)
 
 	for _, cmd := range cmds {
@@ -188,13 +199,15 @@ func runTest(p *PodTestSuite, images []string, waitCondition, volumeType string)
 		}
 	}
 
-	typeCheckCmd := fmt.Sprintf("kubectl get pv $(%s) -o jsonpath='{.spec.%s}'", "kubectl get pv -o jsonpath='{.items[0].metadata.name}'", volumeType)
-	c := createCmd(p.T(), typeCheckCmd, kustomizeDir, p.config.envs(), nil)
-	typeCheckOutput, err := c.CombinedOutput()
-	if err != nil {
-		p.FailNow("", "failed to check volume type: %v", err)
-	}
-	if len(typeCheckOutput) == 0 || !strings.Contains(string(typeCheckOutput), "path") {
-		p.FailNow("volume Type not correct")
+	if volumeType != "" {
+		typeCheckCmd := fmt.Sprintf("kubectl get pv $(%s) -o jsonpath='{.spec.%s}'", "kubectl get pv -o jsonpath='{.items[0].metadata.name}'", volumeType)
+		c := createCmd(p.T(), typeCheckCmd, kustomizeDir, p.config.envs(), nil)
+		typeCheckOutput, err := c.CombinedOutput()
+		if err != nil {
+			p.FailNow("", "failed to check volume type: %v", err)
+		}
+		if len(typeCheckOutput) == 0 || !strings.Contains(string(typeCheckOutput), "path") {
+			p.FailNow("volume Type not correct")
+		}
 	}
 }

test/testdata/kind-cluster.yaml

@@ -3,4 +3,8 @@ kind: Cluster
 nodes:
   - role: control-plane
   - role: worker
+    labels:
+      kubernetes.io/hostname: kind-worker1.hostname
   - role: worker
+    labels:
+      kubernetes.io/hostname: kind-worker2.hostname

test/testdata/pod-with-node-affinity/patch.yaml

@@ -11,4 +11,4 @@ spec:
               - key: kubernetes.io/hostname
                 operator: In
                 values:
-                  - kind-worker
+                  - kind-worker1.hostname

test/testdata/pv-with-legacy-affinity/kustomization.yaml

@@ -0,0 +1,10 @@
+apiVersion: kustomize.config.k8s.io/v1beta1
+kind: Kustomization
+resources:
+- ../../../deploy
+- pv.yaml
+commonLabels:
+  app: local-path-provisioner
+images:
+- name: rancher/local-path-provisioner
+  newTag: dev

test/testdata/pv-with-legacy-affinity/pv.yaml

@@ -0,0 +1,38 @@
+apiVersion: v1
+kind: PersistentVolume
+metadata:
+  annotations:
+    local.path.provisioner/selected-node: kind-worker
+    pv.kubernetes.io/provisioned-by: rancher.io/local-path
+  finalizers:
+    - kubernetes.io/pv-protection
+  labels:
+    test/avoid-cleanup: "true"
+  name: pvc-to-clean-up
+spec:
+  accessModes:
+    - ReadWriteOnce
+  capacity:
+    storage: 100Mi
+  hostPath:
+    path: /opt/local-path-provisioner/default/local-path-pvc
+    type: DirectoryOrCreate
+  nodeAffinity:
+    required:
+      nodeSelectorTerms:
+        - matchExpressions:
+            - key: kubernetes.io/hostname
+              operator: In
+              values:
+                - kind-worker1.hostname
+  claimRef:
+    apiVersion: v1
+    kind: PersistentVolumeClaim
+    name: no-such-pvc
+    namespace: default
+    # The PVC "definitely doesn't exist any more"
+    resourceVersion: "1"
+    uid: 12345678-1234-5678-9abc-123456789abc
+  persistentVolumeReclaimPolicy: Delete
+  storageClassName: local-path-custom-path-pattern
+  volumeMode: Filesystem

test/util.go

@@ -78,7 +78,7 @@ func testdataFile(fields ...string) string {
 func deleteKustomizeDeployment(t *testing.T, kustomizeDir string, envs []string) error {
 	_, err := runCmd(
 		t,
-		"kustomize build | kubectl delete --timeout=180s -f -",
+		"kustomize build | kubectl delete --timeout=180s -f - -l 'test/avoid-cleanup!=true'",
 		testdataFile(kustomizeDir),
 		envs,
 		nil,