pkg/admission/storageclass: pick one storageclass conditionally if >1 present #110559

danishprakash · 2022-06-14T05:20:33Z

What this PR does / why we need it:

If there are two StorageClasses, currently we don't pick either of them as the default as part of admission control. This change picks one StorageClass at random.

Which issue(s) this PR fixes:

Fixes #110514

Signed-off-by: danishprakash grafitykoncept@gmail.com

Does this PR introduce a user-facing change?

Admission control plugin "DefaultStorageClass": If more than one StorageClass is designated as default (via the "storageclass.kubernetes.io/is-default-class" annotation), choose the newest one instead of throwing an error.

k8s-ci-robot · 2022-06-14T05:20:41Z

Hi @danishprakash. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

danishprakash · 2022-06-14T05:22:27Z

plugin/pkg/admission/storage/storageclass/setdefault/admission.go

+		rand.Seed(time.Now().UnixNano())
+		return defaultClasses[rand.Intn(len(defaultClasses))], nil


@thockin I've used rand here for starters, but is there a heuristic we have in mind that we can employ here. I'm open to suggestions, thanks.

Tim should we not have to be consistent in a multimaster deployment?
If we have 2 or more apiservers, I think that all of them should pick the same default

I think that sorting it alphabetically and returning the first one will make it

Why not sort by CreationTimestamp and choose the newest one (on the assumption that this situation USUALLY means someone is adding a new one and removing an old one)?

thockin · 2022-07-07T19:11:56Z

plugin/pkg/admission/storage/storageclass/setdefault/admission_test.go

@@ -170,7 +170,7 @@ func TestAdmission(t *testing.T) {
 			"two defaults, error with PVC with class=nil",
 			[]*storagev1.StorageClass{defaultClass1, defaultClass2, classWithFalseDefault, classWithNoDefault, classWithEmptyDefault},
 			claimWithNoClass,
-			true,
+			false,


How about a testcase for multiple defaults?

thockin · 2022-07-12T20:40:36Z

The IngressClass PR was approved

danishprakash · 2022-07-13T05:16:53Z

I'll update this today, thanks for the heads up.

pkg/controller/volume/persistentvolume/pv_controller_test.go

jsafrane · 2022-08-11T11:48:20Z

/approve
from storage POV
@thockin PTAL

@danishprakash please add a release note in the first comment of this PR, this is user-facing change.
See https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md#does-my-pull-request-need-a-release-note for details.

jsafrane · 2022-08-29T16:22:09Z

/lgtm

jpbetz · 2022-10-14T16:33:23Z

pkg/volume/util/storageclass.go

 	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)


Also put in release note (and maybe the PR description) that the newest is preferred?

Should we document this in the API?

Strictly speaking this is optional and not part of the API :)

plugin/pkg/admission/storage/storageclass/setdefault/admission.go

thockin · 2022-10-18T16:50:16Z

ping, I 'd love to approve this

pkg/volume/util/storageclass.go

Signed-off-by: danishprakash <grafitykoncept@gmail.com>

thockin · 2022-10-19T18:09:18Z

I updated the relnote a bit.

/lgtm
/approve

k8s-ci-robot · 2022-10-19T18:09:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danishprakash, jsafrane, thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/controller/volume/persistentvolume/OWNERS~~ [jsafrane,thockin]
~~pkg/volume/OWNERS~~ [jsafrane,thockin]
~~plugin/pkg/admission/OWNERS~~ [thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 14, 2022

k8s-ci-robot requested review from deads2k and derekwaynecarr June 14, 2022 05:21

danishprakash commented Jun 14, 2022

View reviewed changes

danishprakash changed the title ~~pkg/admission/storageclass: pick random storageclass if >1 present~~ [wip] pkg/admission/storageclass: pick random storageclass if >1 present Jun 14, 2022

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 14, 2022

danishprakash marked this pull request as draft June 14, 2022 05:27

danishprakash changed the title ~~[wip] pkg/admission/storageclass: pick random storageclass if >1 present~~ pkg/admission/storageclass: pick random storageclass if >1 present Jun 14, 2022

danishprakash force-pushed the danish-default-storageclass branch from e62a753 to 46d883f Compare June 19, 2022 14:24

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 19, 2022

danishprakash marked this pull request as ready for review June 19, 2022 14:24

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 19, 2022

AxeZhan mentioned this pull request Jul 4, 2022

Having two Default StorageClasses should be handled better #110514

Closed

thockin reviewed Jul 7, 2022

View reviewed changes

thockin mentioned this pull request Jul 7, 2022

Handle two or more default IngressClasses more cleanly #110974

Merged

thockin self-assigned this Jul 7, 2022

thockin added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Jul 15, 2022

k8s-ci-robot removed the do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Jul 15, 2022

thockin added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 15, 2022

k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Aug 4, 2022

danishprakash commented Aug 4, 2022

View reviewed changes

pkg/controller/volume/persistentvolume/pv_controller_test.go Show resolved Hide resolved

danishprakash changed the title ~~pkg/admission/storageclass: pick random storageclass if >1 present~~ pkg/admission/storageclass: pick one storageclass conditionally if >1 present Aug 11, 2022

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 29, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 2022

jsafrane mentioned this pull request Sep 12, 2022

If multiple default StorageClasses exist, pvc request fails #34549

Closed

thockin assigned jpbetz Oct 14, 2022

jpbetz suggested changes Oct 14, 2022

View reviewed changes

thockin reviewed Oct 14, 2022

View reviewed changes

plugin/pkg/admission/storage/storageclass/setdefault/admission.go Outdated Show resolved Hide resolved

thockin reviewed Oct 18, 2022

View reviewed changes

pkg/volume/util/storageclass.go Show resolved Hide resolved

danishprakash added 2 commits October 19, 2022 09:30

pkg/admission/storageclass: pick random storageclass if >1 present

f12325a

Signed-off-by: danishprakash <grafitykoncept@gmail.com>

pv_controller: update tests for multiple storageclasses

f10f4d3

Signed-off-by: danishprakash <grafitykoncept@gmail.com>

danishprakash force-pushed the danish-default-storageclass branch from 3c961e4 to f10f4d3 Compare October 19, 2022 04:03

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 19, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 19, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 19, 2022

k8s-ci-robot merged commit 962235c into kubernetes:master Oct 19, 2022

k8s-ci-robot added this to the v1.26 milestone Oct 19, 2022

RomanBednar mentioned this pull request Nov 23, 2022

Feature blog for KEP-3333 Retroactive default StorageClass assignment kubernetes/website#37625

Merged

tzneal mentioned this pull request Apr 13, 2023

fix: Discover default storage class when none specified kubernetes-sigs/karpenter#293

Merged

carlory mentioned this pull request May 31, 2023

fix incorrect retroactive_storageclass_errors_total metric #118348

Closed

agilgur5 mentioned this pull request Jul 2, 2023

docs(storage-classes): add section on default StorageClass kubernetes/website#41843

Merged

Nuru mentioned this pull request Jul 30, 2023

Inconsistent documentation of default StorageClass kubernetes/website#42288

Open

Affan-7 mentioned this pull request Jul 31, 2023

Fix storageclass pvc statement kubernetes/website#42177

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/admission/storageclass: pick one storageclass conditionally if >1 present #110559

pkg/admission/storageclass: pick one storageclass conditionally if >1 present #110559

danishprakash commented Jun 14, 2022 •

edited by thockin

k8s-ci-robot commented Jun 14, 2022

danishprakash Jun 14, 2022

aojea Jun 19, 2022 •

edited

aojea Jun 19, 2022

thockin Jul 7, 2022

thockin Jul 7, 2022

thockin commented Jul 12, 2022

danishprakash commented Jul 13, 2022

jsafrane commented Aug 11, 2022

jsafrane commented Aug 29, 2022

jpbetz Oct 14, 2022

thockin Oct 14, 2022

thockin commented Oct 18, 2022

thockin commented Oct 19, 2022

k8s-ci-robot commented Oct 19, 2022

		rand.Seed(time.Now().UnixNano())
		return defaultClasses[rand.Intn(len(defaultClasses))], nil

pkg/admission/storageclass: pick one storageclass conditionally if >1 present #110559

pkg/admission/storageclass: pick one storageclass conditionally if >1 present #110559

Conversation

danishprakash commented Jun 14, 2022 • edited by thockin

What this PR does / why we need it:

Which issue(s) this PR fixes:

k8s-ci-robot commented Jun 14, 2022

danishprakash Jun 14, 2022

Choose a reason for hiding this comment

aojea Jun 19, 2022 • edited

Choose a reason for hiding this comment

aojea Jun 19, 2022

Choose a reason for hiding this comment

thockin Jul 7, 2022

Choose a reason for hiding this comment

thockin Jul 7, 2022

Choose a reason for hiding this comment

thockin commented Jul 12, 2022

danishprakash commented Jul 13, 2022

jsafrane commented Aug 11, 2022

jsafrane commented Aug 29, 2022

jpbetz Oct 14, 2022

Choose a reason for hiding this comment

thockin Oct 14, 2022

Choose a reason for hiding this comment

thockin commented Oct 18, 2022

thockin commented Oct 19, 2022

k8s-ci-robot commented Oct 19, 2022

danishprakash commented Jun 14, 2022 •

edited by thockin

aojea Jun 19, 2022 •

edited