Add admission controller for default storage class. #30900

jsafrane · 2016-08-18T17:07:33Z

The admission controller adds a default class to PVCs that do not require any
specific class. This way, users (=PVC authors) do not need to care about
storage classes, administrator can configure a default one and all these PVCs
that do not care about class will get the default one.

The marker of default class is annotation "volume.beta.kubernetes.io/storage-class", which must be set to "true" to work. All other values (or missing annotation) makes the class non-default.

Based on @thockin's code, added tests and made it not to reject a PVC when no class is marked as default.
.
@kubernetes/sig-storage

This change is

The admission controller adds a default class to PVCs that do not require any specific class. This way, users (=PVC authors) do not need to care about storage classes, administrator can configure a default one and all these PVCs that do not care about class will get the default one.

jsafrane · 2016-08-18T17:09:40Z

Marking as do-not-merge, I actually did not test if it works, will do that tomorrow. Volunteers are welcome.

thockin · 2016-08-18T17:11:48Z

@erictune @derekwaynecarr I have no hands-on experience with admission controllers, so I have no idea if this follows established conventions

thockin · 2016-08-18T17:13:18Z

plugin/pkg/admission/persistentvolumeclaim/default/admission.go

+const classAnnotation = "volume.beta.kubernetes.io/storage-class"
+
+// This indicates that a particular StorageClass nominates itself as the system default.
+const isDefaultAnnotation = "storage-class.beta.kubernetes.io/isDefaultClass"


should be something like storageclass.beta.kubernetes.io/is-default-class - my bad, sorry

thockin · 2016-08-18T17:14:07Z

If we do this we should probably link it into the set of admissions used by default.

thockin · 2016-08-18T17:14:27Z

and will need at least basic e2e

derekwaynecarr · 2016-08-18T20:57:58Z

plugin/pkg/admission/persistentvolumeclaim/default/admission.go

+)
+
+const (
+	PluginName = "SimplePersistentVolumeClaimDefault"


I would prefer the plug-in have a name that associates it with managing default storage class. As a result, can we have "StorageClass" somewhere in the name and the plugin pkg?

IMO it does not really manage StorageClasses, it maniplulates PVCs only, ergo the persistentvolumeclaim package.

Renamed to SimpleDefaultStorageClassForPVC. There may be different defaulting policies later, this one is called Simple.

derekwaynecarr · 2016-08-18T21:06:27Z

Added a number of comments, poke when more review is needed.

erictune · 2016-08-19T08:12:23Z

Suggest not allowing user to change the storage class once PVC is created.

On Thu, Aug 18, 2016 at 2:07 PM, Derek Carr notifications@github.com
wrote:

Added a number of comments, poke when more review is needed.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#30900 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHuudu1hP0OQUbDovW8E_iC4ZZVqwBdyks5qhMmGgaJpZM4JntWR
.

jsafrane · 2016-08-19T11:07:44Z

Suggest not allowing user to change the storage class once PVC is created.

Ack, however, the class is just an annotation now. I will add a validator to check for this when we have a real PVC.Spec.Class attribute.

jsafrane · 2016-08-19T11:38:41Z

Addressed feedback. TODO:

e2e test
make it default

jsafrane · 2016-08-19T11:58:46Z

Tested manually, works as expected.

jsafrane · 2016-08-19T12:12:36Z

@derekwaynecarr, are there any e2e tests for admission controllers? And what if someone installs a cluster without the admission controller that is being tested? I noticed only some ServiceAccount tests.

thockin · 2016-08-19T16:02:48Z

plugin/pkg/admission/persistentvolumeclaim/default/admission.go

+)
+
+const (
+	PluginName = "SimpleDefaultStorageClassForPVC"


I know I am arguing with Derek, but I don't think this is just about PVC. It just happens that PVC is the only thing attached to storage class today. If there were some other object that needed a storage class, and the cluster admin wanted to use this simple mode of defaulting, it would be weird to have different defaults for the other (hypothetical) thing.

I think it's a minor point but this name is over-precise. I won't block on it, but i think it was better before :)

So, assuming that this plugin will put default storage class into several hypothetical objects,

it should be named just DefaultStorageClass (even without Simple)

it should reside in storageclass directory, not in persistentvolumeclaim.

I'll add an extra commit for this, just in case we'd like to revert back.

markturansky · 2016-08-19T16:34:20Z

We need to discuss multi-zone and how we distribute PVs across zones when end users are not aware of storageClasses.

markturansky · 2016-08-19T16:37:11Z

plugin/pkg/admission/persistentvolumeclaim/default/admission.go

+	if len(defaultClasses) == 0 {
+		return nil, nil
+	}
+	if len(defaultClasses) > 1 {


could we have many default classes and perhaps this admission controller round-robins between them?

Talk to Erin :)

On Fri, Aug 19, 2016 at 9:38 AM, Mark Turansky notifications@github.com
wrote:

In plugin/pkg/admission/persistentvolumeclaim/default/admission.go
#30900 (comment)
:

defaultClasses := []*extensions.StorageClass{}

for _, c := range store.List() {

class, ok := c.(*extensions.StorageClass)

if !ok {

return nil, errors.NewInternalError(fmt.Errorf("error converting stored object to StorageClass: %v", c))

}

if class.Annotations[isDefaultAnnotation] == "true" {

defaultClasses = append(defaultClasses, class)

glog.V(4).Infof("getDefaultClass added: %s", class.Name)

}

}

if len(defaultClasses) == 0 {

return nil, nil

}

if len(defaultClasses) > 1 {

could we have many default classes and perhaps this admission controller
round-robins between them?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/kubernetes/pull/30900/files/395e9eac70fc10735c7f5bf246331244343f6b46#r75512939,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVHOhHDJiHjkq-7fzEZ5MxF4uFxyFks5qhdvrgaJpZM4JntWR
.

thockin · 2016-08-19T16:40:18Z

We have conflicting requirements. If a user asks for a PVC and we default
their class we can:

a) assert num_default_classes === 1
b) pick a random default from the set of default classes

I initially argued for (b). Erin Boyd talked me down. I can see arguments
in both directions.

If you need to create a per-zone gold, maybe you should also create a
dont-care-about-zone gold and make THAT one the default, and rely on the
in-code spreading

On Fri, Aug 19, 2016 at 9:35 AM, Mark Turansky notifications@github.com
wrote:

We need to discuss multi-zone and how we distribute PVs across zones when
end users are not aware of storageClasses.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#30900 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVGHUsVGGExYbLrixRrprnIToihTSks5qhdtBgaJpZM4JntWR
.

markturansky · 2016-08-19T16:48:09Z

Having our OpenShift Online multi-zone requirements in mind, I would have argued for (b) with you :)

As it is, a single default class won't work for multi-zone, AFAICT. is there something I am missing?

markturansky · 2016-08-19T16:54:51Z

Use Case:

Google Container Engine / OpenShift Online both have clusters spanning 10 zones. Admins want to rely on Kube to spread pods across 10 zones, but EBS/GCEPDs are created within single zones. The scheduler makes sure a pod ends up in the right zone according to its storage.

I assume we don't want users to have to pick a zone or even know about storage classes.
Current provisioner requires 1 StorageClass per zone because zone is a configurable option.
A single default storageClass will put all pods in 1 zone and leave 9 zones devoid of pods.

How do we resolve this and spread PVs across all zones equally?

@smarterclayton @abhgupta

markturansky · 2016-08-20T01:47:37Z

ok, i missed the bit of code that choses a zone from all the available zones if none is configured in a storageClass :) My concerns were misplaced.

jsafrane · 2016-08-22T12:18:09Z

Renamed to DefaultStorageClass and moved to plugin/pkg/admission/storageclass/default/. No e2e yet (is it even possible?)

k8s-bot · 2016-08-22T13:26:01Z

GCE e2e build/test passed for commit 5f6efef.

pmorie · 2016-08-22T20:44:09Z

plugin/pkg/admission/storageclass/default/admission.go

+}
+
+// This is a stand-in until we have a real field.  This string should be a const somewhere.
+const classAnnotation = "volume.beta.kubernetes.io/storage-class"


isn't this already a const in another package?

thockin · 2016-08-22T21:11:16Z

I am satisfied. There are a couple nits from other folks. I'm going to LGTM this and if we lose the race with the bot, we can followup.

+lgtm

Reviewed 23 of 23 files at r4.
Review status: all files reviewed at latest revision, 9 unresolved discussions.

plugin/pkg/admission/storageclass/default/admission.go, line 109 [r1] (raw file):

Previously, markturansky (Mark Turansky) wrote…

@jsafrane pvc.Spec is currently immutable post Create. Changing the Class won't be allowed with current validation.

is this resolved? I don't see where we specify to do this on create only

Comments from Reviewable

derekwaynecarr · 2016-08-23T15:22:37Z

@jsafrane - can you send a follow-on PR to document this admission controller here:

https://github.com/kubernetes/kubernetes.github.io/blob/master/docs/admin/admission-controllers.md

Thanks!

k8s-github-robot · 2016-08-24T09:49:59Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-08-24T10:32:41Z

GCE e2e build/test passed for commit 5f6efef.

k8s-github-robot · 2016-08-24T10:32:46Z

Automatic merge from submit-queue

jsafrane · 2016-08-24T11:03:25Z

@derekwaynecar: filled kubernetes/website#1064

jsafrane added sig/storage Categorizes an issue or PR as relevant to SIG Storage. team/cluster release-note-none Denotes a PR that doesn't merit a release note. labels Aug 18, 2016

googlebot added the cla: yes label Aug 18, 2016

jsafrane changed the title ~~Add admission controller for default storage class.~~ WIP: Add admission controller for default storage class. Aug 18, 2016

jsafrane added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Aug 18, 2016

k8s-github-robot assigned derekwaynecarr Aug 18, 2016

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 18, 2016

thockin reviewed Aug 18, 2016
View reviewed changes

derekwaynecarr reviewed Aug 18, 2016
View reviewed changes

jsafrane force-pushed the pvc-admission branch from 0fb0c07 to e768d07 Compare August 19, 2016 11:35

jsafrane changed the title ~~WIP: Add admission controller for default storage class.~~ Add admission controller for default storage class. Aug 19, 2016

jsafrane removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Aug 19, 2016

jsafrane force-pushed the pvc-admission branch from e768d07 to 395e9ea Compare August 19, 2016 15:02

thockin reviewed Aug 19, 2016
View reviewed changes

markturansky reviewed Aug 19, 2016
View reviewed changes

wongma7 mentioned this pull request Aug 19, 2016

Add section on storage classes to PV doc kubernetes/website#1064

Merged

[squash] Rename and move to storageclass/

5f6efef

jsafrane force-pushed the pvc-admission branch from 395e9ea to c57b3f5 Compare August 22, 2016 12:17

jsafrane force-pushed the pvc-admission branch from c57b3f5 to 5f6efef Compare August 22, 2016 12:46

pmorie reviewed Aug 22, 2016
View reviewed changes

thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2016

derekwaynecarr assigned thockin Aug 23, 2016

lavalamp modified the milestones: v1.5, v1.4 Aug 23, 2016

k8s-github-robot merged commit ef27186 into kubernetes:master Aug 24, 2016

LittleChimera mentioned this pull request May 21, 2021

[WIP] Set default storage class on PVC sync if default StorageClass is present #100031

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add admission controller for default storage class. #30900

Add admission controller for default storage class. #30900

jsafrane commented Aug 18, 2016 •

edited by k8s-oncall

Loading

jsafrane commented Aug 18, 2016

thockin commented Aug 18, 2016

thockin Aug 18, 2016

jsafrane Aug 19, 2016

thockin commented Aug 18, 2016

thockin commented Aug 18, 2016

derekwaynecarr Aug 18, 2016

jsafrane Aug 19, 2016

derekwaynecarr commented Aug 18, 2016

erictune commented Aug 19, 2016

jsafrane commented Aug 19, 2016

jsafrane commented Aug 19, 2016

jsafrane commented Aug 19, 2016

jsafrane commented Aug 19, 2016

thockin Aug 19, 2016

jsafrane Aug 22, 2016

markturansky commented Aug 19, 2016

markturansky Aug 19, 2016

thockin Aug 19, 2016

thockin commented Aug 19, 2016

markturansky commented Aug 19, 2016

markturansky commented Aug 19, 2016

markturansky commented Aug 20, 2016

jsafrane commented Aug 22, 2016

k8s-bot commented Aug 22, 2016

pmorie Aug 22, 2016

thockin commented Aug 22, 2016

derekwaynecarr commented Aug 23, 2016

k8s-github-robot commented Aug 24, 2016

k8s-bot commented Aug 24, 2016

k8s-github-robot commented Aug 24, 2016

jsafrane commented Aug 24, 2016

Add admission controller for default storage class. #30900

Add admission controller for default storage class. #30900

Conversation

jsafrane commented Aug 18, 2016 • edited by k8s-oncall Loading

jsafrane commented Aug 18, 2016

thockin commented Aug 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Aug 18, 2016

thockin commented Aug 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekwaynecarr commented Aug 18, 2016

erictune commented Aug 19, 2016

jsafrane commented Aug 19, 2016

jsafrane commented Aug 19, 2016

jsafrane commented Aug 19, 2016

jsafrane commented Aug 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markturansky commented Aug 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Aug 19, 2016

markturansky commented Aug 19, 2016

markturansky commented Aug 19, 2016

markturansky commented Aug 20, 2016

jsafrane commented Aug 22, 2016

k8s-bot commented Aug 22, 2016

Choose a reason for hiding this comment

thockin commented Aug 22, 2016

derekwaynecarr commented Aug 23, 2016

k8s-github-robot commented Aug 24, 2016

k8s-bot commented Aug 24, 2016

k8s-github-robot commented Aug 24, 2016

jsafrane commented Aug 24, 2016

jsafrane commented Aug 18, 2016 •

edited by k8s-oncall

Loading