Merge pull request #56298 from pospispa/566-improvements-suggested-by-thockin-during-review-of-PR55824

Automatic merge from submit-queue (batch tested with PRs 56401, 56506, 56551, 56298, 56581). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Addressing Comments from Code Review

**What this PR does / why we need it**: addressing comments from code review: https://github.com/kubernetes/kubernetes/pull/55824#pullrequestreview-78597250

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: N/A


**Special notes for your reviewer**:
@thockin @jsafrane @msau42 PTAL

**Release note**:

```release-note
NONE
```

pkg/controller/volume/pvcprotection/BUILD

@@ -8,6 +8,7 @@ go_library(
     deps = [
         "//pkg/controller:go_default_library",
         "//pkg/util/metrics:go_default_library",
+        "//pkg/util/slice:go_default_library",
         "//pkg/volume/util:go_default_library",
         "//pkg/volume/util/volumehelper:go_default_library",
         "//vendor/github.com/golang/glog:go_default_library",

pkg/controller/volume/pvcprotection/pvc_protection_controller.go

@@ -33,6 +33,7 @@ import (
 	"k8s.io/client-go/util/workqueue"
 	"k8s.io/kubernetes/pkg/controller"
 	"k8s.io/kubernetes/pkg/util/metrics"
+	"k8s.io/kubernetes/pkg/util/slice"
 	volumeutil "k8s.io/kubernetes/pkg/volume/util"
 	"k8s.io/kubernetes/pkg/volume/util/volumehelper"
 )
@@ -153,7 +154,7 @@ func (c *Controller) processPVC(pvcNamespace, pvcName string) error {
 		return err
 	}
 
-	if volumeutil.IsPVCBeingDeleted(pvc) && volumeutil.IsProtectionFinalizerPresent(pvc) {
+	if isDeletionCandidate(pvc) {
 		// PVC should be deleted. Check if it's used and remove finalizer if
 		// it's not.
 		isUsed, err := c.isBeingUsed(pvc)
@@ -165,7 +166,7 @@ func (c *Controller) processPVC(pvcNamespace, pvcName string) error {
 		}
 	}
 
-	if !volumeutil.IsPVCBeingDeleted(pvc) && !volumeutil.IsProtectionFinalizerPresent(pvc) {
+	if needToAddFinalizer(pvc) {
 		// PVC is not being deleted -> it should have the finalizer. The
 		// finalizer should be added by admission plugin, this is just to add
 		// the finalizer to old PVCs that were created before the admission
@@ -177,7 +178,7 @@ func (c *Controller) processPVC(pvcNamespace, pvcName string) error {
 
 func (c *Controller) addFinalizer(pvc *v1.PersistentVolumeClaim) error {
 	claimClone := pvc.DeepCopy()
-	volumeutil.AddProtectionFinalizer(claimClone)
+	claimClone.ObjectMeta.Finalizers = append(claimClone.ObjectMeta.Finalizers, volumeutil.PVCProtectionFinalizer)
 	_, err := c.client.CoreV1().PersistentVolumeClaims(claimClone.Namespace).Update(claimClone)
 	if err != nil {
 		glog.V(3).Infof("Error adding protection finalizer to PVC %s/%s: %v", pvc.Namespace, pvc.Name)
@@ -189,7 +190,7 @@ func (c *Controller) addFinalizer(pvc *v1.PersistentVolumeClaim) error {
 
 func (c *Controller) removeFinalizer(pvc *v1.PersistentVolumeClaim) error {
 	claimClone := pvc.DeepCopy()
-	volumeutil.RemoveProtectionFinalizer(claimClone)
+	claimClone.ObjectMeta.Finalizers = slice.RemoveString(claimClone.ObjectMeta.Finalizers, volumeutil.PVCProtectionFinalizer, nil)
 	_, err := c.client.CoreV1().PersistentVolumeClaims(claimClone.Namespace).Update(claimClone)
 	if err != nil {
 		glog.V(3).Infof("Error removing protection finalizer from PVC %s/%s: %v", pvc.Namespace, pvc.Name, err)
@@ -247,7 +248,7 @@ func (c *Controller) pvcAddedUpdated(obj interface{}) {
 	}
 	glog.V(4).Infof("Got event on PVC %s", key)
 
-	if (!volumeutil.IsPVCBeingDeleted(pvc) && !volumeutil.IsProtectionFinalizerPresent(pvc)) || (volumeutil.IsPVCBeingDeleted(pvc) && volumeutil.IsProtectionFinalizerPresent(pvc)) {
+	if needToAddFinalizer(pvc) || isDeletionCandidate(pvc) {
 		c.queue.Add(key)
 	}
 }
@@ -282,3 +283,11 @@ func (c *Controller) podAddedDeletedUpdated(obj interface{}, deleted bool) {
 		}
 	}
 }
+
+func isDeletionCandidate(pvc *v1.PersistentVolumeClaim) bool {
+	return pvc.ObjectMeta.DeletionTimestamp != nil && slice.ContainsString(pvc.ObjectMeta.Finalizers, volumeutil.PVCProtectionFinalizer, nil)
+}
+
+func needToAddFinalizer(pvc *v1.PersistentVolumeClaim) bool {
+	return pvc.ObjectMeta.DeletionTimestamp == nil && !slice.ContainsString(pvc.ObjectMeta.Finalizers, volumeutil.PVCProtectionFinalizer, nil)
+}

pkg/kubelet/volumemanager/populator/BUILD

@@ -19,7 +19,6 @@ go_library(
         "//pkg/kubelet/util/format:go_default_library",
         "//pkg/kubelet/volumemanager/cache:go_default_library",
         "//pkg/volume:go_default_library",
-        "//pkg/volume/util:go_default_library",
         "//pkg/volume/util/types:go_default_library",
         "//pkg/volume/util/volumehelper:go_default_library",
         "//vendor/github.com/golang/glog:go_default_library",

pkg/kubelet/volumemanager/populator/desired_state_of_world_populator.go

@@ -41,7 +41,6 @@ import (
 	"k8s.io/kubernetes/pkg/kubelet/util/format"
 	"k8s.io/kubernetes/pkg/kubelet/volumemanager/cache"
 	"k8s.io/kubernetes/pkg/volume"
-	volumeutil "k8s.io/kubernetes/pkg/volume/util"
 	volumetypes "k8s.io/kubernetes/pkg/volume/util/types"
 	"k8s.io/kubernetes/pkg/volume/util/volumehelper"
 )
@@ -444,7 +443,7 @@ func (dswp *desiredStateOfWorldPopulator) getPVCExtractPV(
 		// and users should not be that surprised.
 		// It should happen only in very rare case when scheduler schedules
 		// a pod and user deletes a PVC that's used by it at the same time.
-		if volumeutil.IsPVCBeingDeleted(pvc) {
+		if pvc.ObjectMeta.DeletionTimestamp != nil {
 			return "", "", fmt.Errorf(
 				"can't start pod because PVC %s/%s is being deleted",
 				namespace,

pkg/util/slice/slice.go

@@ -68,3 +68,24 @@ func ContainsString(slice []string, s string, modifier func(s string) string) bo
 	}
 	return false
 }
+
+// RemoveString returns a newly created []string that contains all items from slice that
+// are not equal to s and modifier(s) in case modifier func is provided.
+func RemoveString(slice []string, s string, modifier func(s string) string) []string {
+	newSlice := make([]string, 0)
+	for _, item := range slice {
+		if item == s {
+			continue
+		}
+		if modifier != nil && modifier(item) == s {
+			continue
+		}
+		newSlice = append(newSlice, item)
+	}
+	if len(newSlice) == 0 {
+		// Sanitize for unit tests so we don't need to distinguish empty array
+		// and nil.
+		newSlice = nil
+	}
+	return newSlice
+}

pkg/util/slice/slice_test.go

@@ -106,3 +106,67 @@ func TestContainsString(t *testing.T) {
 		t.Errorf("ContainsString didn't find the string by modifier")
 	}
 }
+
+func TestRemoveString(t *testing.T) {
+	modifier := func(s string) string {
+		if s == "ab" {
+			return "ee"
+		}
+		return s
+	}
+	tests := []struct {
+		testName string
+		input    []string
+		remove   string
+		modifier func(s string) string
+		want     []string
+	}{
+		{
+			testName: "Nil input slice",
+			input:    nil,
+			remove:   "",
+			modifier: nil,
+			want:     nil,
+		},
+		{
+			testName: "Slice doesn't contain the string",
+			input:    []string{"a", "ab", "cdef"},
+			remove:   "NotPresentInSlice",
+			modifier: nil,
+			want:     []string{"a", "ab", "cdef"},
+		},
+		{
+			testName: "All strings removed, result is nil",
+			input:    []string{"a"},
+			remove:   "a",
+			modifier: nil,
+			want:     nil,
+		},
+		{
+			testName: "No modifier func, one string removed",
+			input:    []string{"a", "ab", "cdef"},
+			remove:   "ab",
+			modifier: nil,
+			want:     []string{"a", "cdef"},
+		},
+		{
+			testName: "No modifier func, all(three) strings removed",
+			input:    []string{"ab", "a", "ab", "cdef", "ab"},
+			remove:   "ab",
+			modifier: nil,
+			want:     []string{"a", "cdef"},
+		},
+		{
+			testName: "Removed both the string and the modifier func result",
+			input:    []string{"a", "cd", "ab", "ee"},
+			remove:   "ee",
+			modifier: modifier,
+			want:     []string{"a", "cd"},
+		},
+	}
+	for _, tt := range tests {
+		if got := RemoveString(tt.input, tt.remove, tt.modifier); !reflect.DeepEqual(got, tt.want) {
+			t.Errorf("%v: RemoveString(%v, %q, %T) = %v WANT %v", tt.testName, tt.input, tt.remove, tt.modifier, got, tt.want)
+		}
+	}
+}

pkg/volume/util/BUILD

@@ -62,7 +62,6 @@ go_library(
 go_test(
     name = "go_default_test",
     srcs = [
-        "finalizer_test.go",
         "util_test.go",
     ] + select({
         "@io_bazel_rules_go//go/platform:linux_amd64": [
@@ -76,7 +75,6 @@ go_test(
     deps = [
         "//pkg/apis/core/install:go_default_library",
         "//pkg/apis/core/v1/helper:go_default_library",
-        "//vendor/github.com/davecgh/go-spew/spew:go_default_library",
         "//vendor/k8s.io/api/core/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library",

pkg/volume/util/finalizer.go

@@ -16,53 +16,7 @@ limitations under the License.
 
 package util
 
-import (
-	"k8s.io/api/core/v1"
-)
-
 const (
 	// Name of finalizer on PVCs that have a running pod.
 	PVCProtectionFinalizer = "kubernetes.io/pvc-protection"
 )
-
-// IsPVCBeingDeleted returns:
-// true: in case PVC is being deleted, i.e. ObjectMeta.DeletionTimestamp is set
-// false: in case PVC is not being deleted, i.e. ObjectMeta.DeletionTimestamp is nil
-func IsPVCBeingDeleted(pvc *v1.PersistentVolumeClaim) bool {
-	return pvc.ObjectMeta.DeletionTimestamp != nil
-}
-
-// IsProtectionFinalizerPresent returns true in case PVCProtectionFinalizer is
-// present among the pvc.Finalizers
-func IsProtectionFinalizerPresent(pvc *v1.PersistentVolumeClaim) bool {
-	for _, finalizer := range pvc.Finalizers {
-		if finalizer == PVCProtectionFinalizer {
-			return true
-		}
-	}
-	return false
-}
-
-// RemoveProtectionFinalizer returns pvc without PVCProtectionFinalizer in case
-// it's present in pvc.Finalizers. It expects that pvc is writable (i.e. is not
-// informer's cached copy.)
-func RemoveProtectionFinalizer(pvc *v1.PersistentVolumeClaim) {
-	newFinalizers := make([]string, 0)
-	for _, finalizer := range pvc.Finalizers {
-		if finalizer != PVCProtectionFinalizer {
-			newFinalizers = append(newFinalizers, finalizer)
-		}
-	}
-	if len(newFinalizers) == 0 {
-		// Sanitize for unit tests so we don't need to distinguish empty array
-		// and nil.
-		newFinalizers = nil
-	}
-	pvc.Finalizers = newFinalizers
-}
-
-// AddProtectionFinalizer adds PVCProtectionFinalizer to pvc. It expects that
-// pvc is writable (i.e. is not informer's cached copy.)
-func AddProtectionFinalizer(pvc *v1.PersistentVolumeClaim) {
-	pvc.Finalizers = append(pvc.Finalizers, PVCProtectionFinalizer)
-}

pkg/volume/util/finalizer_test.go

@@ -1,231 +0,0 @@
-/*
-Copyright 2017 The Kubernetes Authors.
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-    http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-
-package util
-
-import (
-	"reflect"
-	"testing"
-	"time"
-
-	"github.com/davecgh/go-spew/spew"
-	"k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-)
-
-var (
-	arbitraryTime = metav1.Date(2017, 11, 1, 14, 28, 47, 0, time.FixedZone("CET", 0))
-)
-
-func TestIsPVCBeingDeleted(t *testing.T) {
-	tests := []struct {
-		pvc  *v1.PersistentVolumeClaim
-		want bool
-	}{
-		{
-			pvc: &v1.PersistentVolumeClaim{
-				ObjectMeta: metav1.ObjectMeta{
-					DeletionTimestamp: nil,
-				},
-			},
-			want: false,
-		},
-		{
-			pvc: &v1.PersistentVolumeClaim{
-				ObjectMeta: metav1.ObjectMeta{
-					DeletionTimestamp: &arbitraryTime,
-				},
-			},
-			want: true,
-		},
-	}
-	for _, tt := range tests {
-		if got := IsPVCBeingDeleted(tt.pvc); got != tt.want {
-			t.Errorf("IsPVCBeingDeleted(%v) = %v WANT %v", tt.pvc, got, tt.want)
-		}
-	}
-}
-
-func TestAddProtectionFinalizer(t *testing.T) {
-	tests := []struct {
-		name string
-		pvc  *v1.PersistentVolumeClaim
-		want *v1.PersistentVolumeClaim
-	}{
-		{
-			"PVC without finalizer",
-			&v1.PersistentVolumeClaim{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "pvc",
-					Namespace: "ns",
-				},
-			},
-			&v1.PersistentVolumeClaim{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:       "pvc",
-					Namespace:  "ns",
-					Finalizers: []string{PVCProtectionFinalizer},
-				},
-			},
-		},
-		{
-			"PVC with some finalizers",
-			&v1.PersistentVolumeClaim{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:       "pvc",
-					Namespace:  "ns",
-					Finalizers: []string{"1", "2", "3", PVCProtectionFinalizer + "suffix", "prefix" + PVCProtectionFinalizer},
-				},
-			},
-			&v1.PersistentVolumeClaim{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:       "pvc",
-					Namespace:  "ns",
-					Finalizers: []string{"1", "2", "3", PVCProtectionFinalizer + "suffix", "prefix" + PVCProtectionFinalizer, PVCProtectionFinalizer},
-				},
-			},
-		},
-	}
-	for _, test := range tests {
-		got := test.pvc.DeepCopy()
-		AddProtectionFinalizer(got)
-		if !reflect.DeepEqual(got, test.want) {
-			t.Errorf("Test %q: expected:\n%s\n\ngot:\n%s", test.name, spew.Sdump(test.want), spew.Sdump(got))
-		}
-	}
-}
-
-func TestRemoveProtectionFinalizer(t *testing.T) {
-	tests := []struct {
-		name string
-		pvc  *v1.PersistentVolumeClaim
-		want *v1.PersistentVolumeClaim
-	}{
-		{
-			"PVC without finalizer",
-			&v1.PersistentVolumeClaim{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "pvc",
-					Namespace: "ns",
-				},
-			},
-			&v1.PersistentVolumeClaim{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "pvc",
-					Namespace: "ns",
-				},
-			},
-		},
-		{
-			"PVC with finalizer",
-			&v1.PersistentVolumeClaim{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:       "pvc",
-					Namespace:  "ns",
-					Finalizers: []string{PVCProtectionFinalizer},
-				},
-			},
-			&v1.PersistentVolumeClaim{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "pvc",
-					Namespace: "ns",
-				},
-			},
-		},
-		{
-			"PVC with many finalizers",
-			&v1.PersistentVolumeClaim{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:       "pvc",
-					Namespace:  "ns",
-					Finalizers: []string{"1", "2", "3", PVCProtectionFinalizer + "suffix", "prefix" + PVCProtectionFinalizer, PVCProtectionFinalizer},
-				},
-			},
-			&v1.PersistentVolumeClaim{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:       "pvc",
-					Namespace:  "ns",
-					Finalizers: []string{"1", "2", "3", PVCProtectionFinalizer + "suffix", "prefix" + PVCProtectionFinalizer},
-				},
-			},
-		},
-	}
-	for _, test := range tests {
-		got := test.pvc.DeepCopy()
-		RemoveProtectionFinalizer(got)
-		if !reflect.DeepEqual(got, test.want) {
-			t.Errorf("Test %q: expected:\n%s\n\ngot:\n%s", test.name, spew.Sdump(test.want), spew.Sdump(got))
-		}
-	}
-}
-
-func TestIsProtectionFinalizerPresent(t *testing.T) {
-	tests := []struct {
-		name string
-		pvc  *v1.PersistentVolumeClaim
-		want bool
-	}{
-		{
-			"PVC without finalizer",
-			&v1.PersistentVolumeClaim{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "pvc",
-					Namespace: "ns",
-				},
-			},
-			false,
-		},
-		{
-			"PVC with many unrelated finalizers",
-			&v1.PersistentVolumeClaim{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:       "pvc",
-					Namespace:  "ns",
-					Finalizers: []string{"1", "2", "3", PVCProtectionFinalizer + "suffix", "prefix" + PVCProtectionFinalizer},
-				},
-			},
-			false,
-		},
-		{
-			"PVC with many finalizers",
-			&v1.PersistentVolumeClaim{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:       "pvc",
-					Namespace:  "ns",
-					Finalizers: []string{"1", "2", "3", PVCProtectionFinalizer + "suffix", "prefix" + PVCProtectionFinalizer, PVCProtectionFinalizer},
-				},
-			},
-			true,
-		},
-		{
-			"PVC with finalizer",
-			&v1.PersistentVolumeClaim{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:       "pvc",
-					Namespace:  "ns",
-					Finalizers: []string{PVCProtectionFinalizer},
-				},
-			},
-			true,
-		},
-	}
-	for _, test := range tests {
-		got := IsProtectionFinalizerPresent(test.pvc)
-		if got != test.want {
-			t.Errorf("Test %q: expected %v, got %v", test.name, test.want, got)
-		}
-	}
-}