diff --git a/pkg/controller/volume/pvcprotection/BUILD b/pkg/controller/volume/pvcprotection/BUILD index 5c713a259d2..a296dd22c0c 100644 --- a/pkg/controller/volume/pvcprotection/BUILD +++ b/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", diff --git a/pkg/controller/volume/pvcprotection/pvc_protection_controller.go b/pkg/controller/volume/pvcprotection/pvc_protection_controller.go index 40bf3e5de5c..8ce491ffcd3 100644 --- a/pkg/controller/volume/pvcprotection/pvc_protection_controller.go +++ b/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) +} diff --git a/pkg/kubelet/volumemanager/populator/BUILD b/pkg/kubelet/volumemanager/populator/BUILD index a3bd36cc057..f8938c1ce3d 100644 --- a/pkg/kubelet/volumemanager/populator/BUILD +++ b/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", diff --git a/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator.go b/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator.go index ac762a040fa..f696becd3c4 100644 --- a/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator.go +++ b/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, diff --git a/pkg/util/slice/slice.go b/pkg/util/slice/slice.go index b408dbae841..b9809cc2972 100644 --- a/pkg/util/slice/slice.go +++ b/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 +} diff --git a/pkg/util/slice/slice_test.go b/pkg/util/slice/slice_test.go index c39f54c1f14..19b46c227e8 100644 --- a/pkg/util/slice/slice_test.go +++ b/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) + } + } +} diff --git a/pkg/volume/util/BUILD b/pkg/volume/util/BUILD index 48a0b0ee6bf..7a4ed937692 100644 --- a/pkg/volume/util/BUILD +++ b/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", diff --git a/pkg/volume/util/finalizer.go b/pkg/volume/util/finalizer.go index 84631545060..1bc03ad8e78 100644 --- a/pkg/volume/util/finalizer.go +++ b/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) -} diff --git a/pkg/volume/util/finalizer_test.go b/pkg/volume/util/finalizer_test.go deleted file mode 100644 index 210ea3b3e63..00000000000 --- a/pkg/volume/util/finalizer_test.go +++ /dev/null @@ -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) - } - } -}