Create New Default StorageClass: kops-ssd-1-17

[joshbranham]

Since kops manages the default storageclass for AWS, I feel we should add some improvements.

First, we can allow volume expansion now which is handy. https://kubernetes.io/docs/concepts/storage/persistent-volumes/#expanding-persistent-volumes-claims

Second, in order to prevent race conditions with Pods, setting the volumeBindingMode will only create a PV for a PVC and mount it to a node once the pod is scheduled.

https://kubernetes.io/docs/concepts/storage/storage-classes/#volume-binding-mode

Third, enable default EBS encryption. This one is debatable but I think for a production-ready kubernetes cluster in AWS this a best practice default.

This PR has the volumeBindingMode set as well, but I made a new PR since I am suggesting a few other changes. #8416

[joshbranham]

/ok-to-test

[johngmyers]

/lgtm

[hakman]

Maybe add purpose instead of the generic kops keyword?

Suggested change

	name: gp2-kops
	name: gp2-wait

[justinsb]

That's another good option that avoids my prefix vs suffix nit-pick :-)

[joshbranham]

I think the purpose bit makes it too specific since I am also suggesting we set some other non-default settings

[justinsb]

Sorry to nit-pick... I'm wondering if we should use a consistent prefix, rather than a suffix. We've started doing this with e.g. clusterroles, and there we've used kops: but I don't know if the colon is available everywhere (?). WDYT about kops-gp2 or kops:gp2 in general?

[joshbranham]

I think that is a good idea to start with something consistent and move forward with it. Unfortunately it won't let us do kops:gp2 but I think kops-gp2 make sense @justinsb

Thoughts @hakman

[hakman]

kops-gp2-1-18? I am sure we will change it again someday. :)

[justinsb]

I do think it's OK to change these going forwards; "kops-gp2" can mean "the gp2 class, but otherwise whatever kops recommends". We should probably have a note that if people want to lock in a specific behaviour, they should probably use their own storage class (though to be honest I'm not sure how users could do that without knowing what features are coming in the future).

This is sort of philosophy with most of the kops fields - if you leave it unspecified, that means "whatever kops recommends" (I sometimes call it omakase). But if you care, you should specify it, in which case kops honors your intention. We can - and do - change the defaults, but if you specify a field we either honor it or it is an error.

[justinsb]

(BTW, if we wanted to, we could also name this kops-ssd, and then offer storage classes that work across clouds, though I personally find it better to just use the default storage class in my apps)

[joshbranham]

(BTW, if we wanted to, we could also name this kops-ssd, and then offer storage classes that work across clouds, though I personally find it better to just use the default storage class in my apps)

I do like the idea of kops-ssd as a starting point. Should I add a version to it as well? I would like to see this go out whenever it can, so should I target 1.16 or 1.17? Therefore the name could be:

kops-ssd-1-16 for instance

[johngmyers]

I would prefer we target 1.17. This is too featureish to go into 1.16 at this time.

[joshbranham]

Done @johngmyers @justinsb

[joshbranham]

although if we are trying to target a better default for new clusters, then maybe release notes with steps to --force update the existing one is the better approach than modifying the name? At least in this case, it won't effect existing volumes. However I could see this pattern of fix the default for new clusters > release notes for manual fix on existing being harder to implement for things like deployments where labels can't change etc and --force just deletes the old causing potentially downtime for that workload. Hmmmmm

[johngmyers]

I was under the impression that we were targeting a better default for new volumes, even on old clusters.

[joshbranham]

I was under the impression that we were targeting a better default for new volumes, even on old clusters.

That seems like the easiest path forward. If you agree then I’ll just go with kops-ssd-1-17. The likelihood we change these settings much is low so I doubt we’ll run into multiple tweaks per k8s/kops minor version

[cpanato]

@joshbranham i will close my PR

[johngmyers]

/lgtm

[cpanato]

/lgtm

[justinsb]

Thanks @joshbranham

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: joshbranham, justinsb

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:

• OWNERS [justinsb]

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

[joshbranham]

/retest

[dignajar]

this PR is so strange, changing default value from the previous version (backward compatibility will fail) and adding a version number to the storage class name?

What is going to happen after we update from an early version to v1.17; We are going to have two defaults and if for some reason we re-deploy some application will take another type of storage class.

[joshbranham]

The two defaults issue was fixed, pr is linked here. The storage class is essentially parameters to tell EBS what to use when making the volume. It has no impact on existing volumes and you can delete storage classes without affecting existing volumes. This change was just meant to make expanding volumes possible, primarily. Do you see an issue still even with the linked PRs? I will agree the defaulting behavior was a mistake, and hard to manage in long lived clusters anyways when things are always apply-only.

In my case, we manage our storage classes ourselves and enforce via OPA which classes devs can use.