github/kubernetes
4
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Addressing Comments from Code Review

Browse Source 
Addressing comments from code review (https://github.com/kubernetes/kubernetes/pull/55824#pullrequestreview-78597250) in order to simplify the code.
This commit is contained in:
pospispa
2017-11-23 16:00:35 +01:00
parent 2aeace402a
commit e1312f2c00
9 changed files with 101 additions and 287 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
pkg/controller/volume/pvcprotection/BUILD
View File

@@ -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",

19
pkg/controller/volume/pvcprotection/pvc_protection_controller.go
View File

@@ -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)
}

1
pkg/kubelet/volumemanager/populator/BUILD
View File

@@ -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",

3
pkg/kubelet/volumemanager/populator/desired_state_of_world_populator.go
View File

@@ -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,

21
pkg/util/slice/slice.go
View File

@@ -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
}

64
pkg/util/slice/slice_test.go
View File

@@ -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)
}
}
}

2
pkg/volume/util/BUILD
View File

@@ -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",

46
pkg/volume/util/finalizer.go
View File

@@ -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)
}

231
pkg/volume/util/finalizer_test.go
View File

@@ -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)
}
}
}