From f12325add3fbac8a2ce31cb12b4649c01d74f248 Mon Sep 17 00:00:00 2001 From: danishprakash Date: Tue, 14 Jun 2022 10:42:03 +0530 Subject: [PATCH 1/2] pkg/admission/storageclass: pick random storageclass if >1 present Signed-off-by: danishprakash --- pkg/volume/util/storageclass.go | 15 ++++-- .../storageclass/setdefault/admission.go | 1 + .../storageclass/setdefault/admission_test.go | 46 ++++++++++++++++--- 3 files changed, 51 insertions(+), 11 deletions(-) diff --git a/pkg/volume/util/storageclass.go b/pkg/volume/util/storageclass.go index 5350bc8e963..fd1e9c4fddf 100644 --- a/pkg/volume/util/storageclass.go +++ b/pkg/volume/util/storageclass.go @@ -17,9 +17,9 @@ limitations under the License. package util import ( - "fmt" + "sort" + storagev1 "k8s.io/api/storage/v1" - "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" storagev1listers "k8s.io/client-go/listers/storage/v1" @@ -54,10 +54,17 @@ func GetDefaultClass(lister storagev1listers.StorageClassLister) (*storagev1.Sto if len(defaultClasses) == 0 { return nil, nil } + + sort.Slice(defaultClasses, func(i, j int) bool { + if defaultClasses[i].CreationTimestamp.UnixNano() == defaultClasses[j].CreationTimestamp.UnixNano() { + return defaultClasses[i].Name < defaultClasses[j].Name + } + return defaultClasses[i].CreationTimestamp.UnixNano() > defaultClasses[j].CreationTimestamp.UnixNano() + }) if len(defaultClasses) > 1 { - klog.V(4).Infof("GetDefaultClass %d defaults found", len(defaultClasses)) - return nil, errors.NewInternalError(fmt.Errorf("%d default StorageClasses were found", len(defaultClasses))) + klog.V(4).Infof("%d default StorageClasses were found, choosing the newest: %s", len(defaultClasses), defaultClasses[0].Name) } + return defaultClasses[0], nil } diff --git a/plugin/pkg/admission/storage/storageclass/setdefault/admission.go b/plugin/pkg/admission/storage/storageclass/setdefault/admission.go index aa4c1c14ba7..4772996a1a7 100644 --- a/plugin/pkg/admission/storage/storageclass/setdefault/admission.go +++ b/plugin/pkg/admission/storage/storageclass/setdefault/admission.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "io" + "k8s.io/kubernetes/pkg/volume/util" "k8s.io/klog/v2" diff --git a/plugin/pkg/admission/storage/storageclass/setdefault/admission_test.go b/plugin/pkg/admission/storage/storageclass/setdefault/admission_test.go index da48d69e7d0..9883fd97e0b 100644 --- a/plugin/pkg/admission/storage/storageclass/setdefault/admission_test.go +++ b/plugin/pkg/admission/storage/storageclass/setdefault/admission_test.go @@ -19,6 +19,7 @@ package setdefault import ( "context" "testing" + "time" "k8s.io/klog/v2" @@ -96,6 +97,30 @@ func TestAdmission(t *testing.T) { }, Provisioner: "nondefault1", } + classWithCreateTime1 := &storagev1.StorageClass{ + TypeMeta: metav1.TypeMeta{ + Kind: "StorageClass", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "default1", + CreationTimestamp: metav1.NewTime(time.Date(2022, time.Month(1), 1, 0, 0, 0, 1, time.UTC)), + Annotations: map[string]string{ + storageutil.IsDefaultStorageClassAnnotation: "true", + }, + }, + } + classWithCreateTime2 := &storagev1.StorageClass{ + TypeMeta: metav1.TypeMeta{ + Kind: "StorageClass", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "default2", + CreationTimestamp: metav1.NewTime(time.Date(2022, time.Month(1), 1, 0, 0, 0, 0, time.UTC)), + Annotations: map[string]string{ + storageutil.IsDefaultStorageClassAnnotation: "true", + }, + }, + } claimWithClass := &api.PersistentVolumeClaim{ TypeMeta: metav1.TypeMeta{ @@ -166,13 +191,6 @@ func TestAdmission(t *testing.T) { false, "foo", }, - { - "two defaults, error with PVC with class=nil", - []*storagev1.StorageClass{defaultClass1, defaultClass2, classWithFalseDefault, classWithNoDefault, classWithEmptyDefault}, - claimWithNoClass, - true, - "", - }, { "two defaults, no modification of PVC with class=''", []*storagev1.StorageClass{defaultClass1, defaultClass2, classWithFalseDefault, classWithNoDefault, classWithEmptyDefault}, @@ -187,6 +205,20 @@ func TestAdmission(t *testing.T) { false, "foo", }, + { + "two defaults with same creation time, choose the one with smaller name", + []*storagev1.StorageClass{defaultClass1, defaultClass2, classWithFalseDefault, classWithNoDefault, classWithEmptyDefault}, + claimWithNoClass, + false, + defaultClass1.Name, + }, + { + "two defaults, choose the one with newer creation time", + []*storagev1.StorageClass{classWithCreateTime1, classWithCreateTime2, classWithFalseDefault, classWithNoDefault, classWithEmptyDefault}, + claimWithNoClass, + false, + classWithCreateTime1.Name, + }, } for _, test := range tests { From f10f4d372c13ddddbccbe01022b3730ee68d1b88 Mon Sep 17 00:00:00 2001 From: danishprakash Date: Thu, 4 Aug 2022 09:37:27 +0530 Subject: [PATCH 2/2] pv_controller: update tests for multiple storageclasses Signed-off-by: danishprakash --- .../persistentvolume/pv_controller_test.go | 34 +++++++++---------- pkg/volume/util/storageclass.go | 2 ++ .../storageclass/setdefault/admission.go | 1 - 3 files changed, 19 insertions(+), 18 deletions(-) diff --git a/pkg/controller/volume/persistentvolume/pv_controller_test.go b/pkg/controller/volume/persistentvolume/pv_controller_test.go index 7d11120d023..a40921a51ba 100644 --- a/pkg/controller/volume/persistentvolume/pv_controller_test.go +++ b/pkg/controller/volume/persistentvolume/pv_controller_test.go @@ -773,23 +773,6 @@ func TestRetroactiveStorageClassAssignment(t *testing.T) { }, }, }, - { - storageClasses: []*storagev1.StorageClass{ - makeDefaultStorageClass(classGold, &modeImmediate), - makeDefaultStorageClass(classSilver, &modeImmediate)}, - tests: []controllerTest{ - { - name: "15-2 - pvc storage class is not assigned retroactively if there are multiple default storage classes", - initialVolumes: novolumes, - expectedVolumes: novolumes, - initialClaims: newClaimArray("claim15-2", "uid15-2", "1Gi", "", v1.ClaimPending, nil), - expectedClaims: newClaimArray("claim15-2", "uid15-2", "1Gi", "", v1.ClaimPending, nil), - expectedEvents: noevents, - errors: noerrors, - test: testSyncClaim, - }, - }, - }, { storageClasses: []*storagev1.StorageClass{ makeDefaultStorageClass(classGold, &modeImmediate), @@ -844,6 +827,23 @@ func TestRetroactiveStorageClassAssignment(t *testing.T) { }, }, }, + { + storageClasses: []*storagev1.StorageClass{ + makeDefaultStorageClass(classGold, &modeImmediate), + makeDefaultStorageClass(classSilver, &modeImmediate)}, + tests: []controllerTest{ + { + name: "15-2 - pvc storage class is assigned retroactively if there are multiple default storage classes", + initialVolumes: novolumes, + expectedVolumes: novolumes, + initialClaims: newClaimArray("claim15-2", "uid15-2", "1Gi", "", v1.ClaimPending, nil), + expectedClaims: newClaimArray("claim15-2", "uid15-2", "1Gi", "", v1.ClaimPending, &classGold), + expectedEvents: noevents, + errors: noerrors, + test: testSyncClaim, + }, + }, + }, { storageClasses: []*storagev1.StorageClass{ makeDefaultStorageClass(classGold, &modeImmediate), diff --git a/pkg/volume/util/storageclass.go b/pkg/volume/util/storageclass.go index fd1e9c4fddf..d2098c25800 100644 --- a/pkg/volume/util/storageclass.go +++ b/pkg/volume/util/storageclass.go @@ -55,6 +55,8 @@ func GetDefaultClass(lister storagev1listers.StorageClassLister) (*storagev1.Sto return nil, nil } + // Primary sort by creation timestamp, newest first + // Secondary sort by class name, ascending order sort.Slice(defaultClasses, func(i, j int) bool { if defaultClasses[i].CreationTimestamp.UnixNano() == defaultClasses[j].CreationTimestamp.UnixNano() { return defaultClasses[i].Name < defaultClasses[j].Name diff --git a/plugin/pkg/admission/storage/storageclass/setdefault/admission.go b/plugin/pkg/admission/storage/storageclass/setdefault/admission.go index 4772996a1a7..aa4c1c14ba7 100644 --- a/plugin/pkg/admission/storage/storageclass/setdefault/admission.go +++ b/plugin/pkg/admission/storage/storageclass/setdefault/admission.go @@ -20,7 +20,6 @@ import ( "context" "fmt" "io" - "k8s.io/kubernetes/pkg/volume/util" "k8s.io/klog/v2"