[Federation][kubefed]: Use StorageClassName for etcd pvc #46323

marun · 2017-05-24T04:21:10Z

This PR updates kubefed to use the StorageClassName field added in 1.6 for etcd's pvc to allow the user to specify which storage class they want to use. If no value is provided to kubefed init, the field will not be set, and initialization of the pvc may fail on a cluster without a default storage class configured.

The alpha annotation that was previously used (volume.alpha.kubernetes.io/storage-class) was deprecated as of 1.4 according to the following blog post:

http://blog.kubernetes.io/2016/10/dynamic-provisioning-and-storage-in-kubernetes.html

Release note:

'kubefed init' has been updated to support specification of the storage class (via --etcd-pv-storage-class) for the Persistent Volume Claim (PVC) used for etcd storage.  If --etcd-pv-storage-class is not specified, the default storage class configured for the cluster will be used.

cc: @kubernetes/sig-federation-pr-reviews

marun · 2017-05-24T14:36:59Z

@k8s-bot pull-kubernetes-kubemark-e2e-gce test this
@k8s-bot pull-kubernetes-e2e-kops-aws test this
@k8s-bot pull-kubernetes-federation-e2e-gce test this

saad-ali

Couple of questions

saad-ali · 2017-05-24T21:02:36Z

federation/pkg/kubefed/init/init.go

@@ -159,6 +160,7 @@ func (o *initFederationOptions) Bind(flags *pflag.FlagSet, defaultServerImage, d
 	flags.StringVar(&o.dnsProviderConfig, "dns-provider-config", "", "Config file path on local file system for configuring DNS provider.")
 	flags.StringVar(&o.etcdImage, "etcd-image", defaultEtcdImage, "Image to use for etcd server.")
 	flags.StringVar(&o.etcdPVCapacity, "etcd-pv-capacity", "10Gi", "Size of persistent volume claim to be used for etcd.")
+	flags.StringVar(&o.etcdPVStorageClass, "etcd-pv-storage-class", "", "The storage class of the persistent volume claim used for etcd.   Must be provided if a default storage class is not enabled for the host cluster.")


This is a change in behavior from the alpha annotation, and I want to make sure you're aware and making the decision consciously: If a user runs without specifying etcd-pv-storage-class and their cluster doesn't have a default storage class installed this will result in the PVC not provisioning a new volume and instead waiting to bind to an existing PV, whereas the alpha annotation always provisioned a new volume.

Yes, I'm aware of that change, and I'm looking for feedback from someone on the federation side as to whether that is acceptable. For openshift it's an essential change since support for the alpha annotation has already been removed.

I think it is fine. We need to get rid of the alpha annotation at some point or we will be forced to do it anyway.

saad-ali · 2017-05-24T21:06:31Z

federation/pkg/kubefed/init/init.go

 	capacity, err := resource.ParseQuantity(etcdPVCapacity)
 	if err != nil {
 		return nil, err
 	}

+	annotations := map[string]string{federation.FederationNameAnnotation: federationName}
+	if len(etcdPVStorageClass) > 0 {
+		annotations["volume.beta.kubernetes.io/storage-class"] = etcdPVStorageClass


Is there anyway to change this behavior based on k8s cluster version?

k8s.version >= 1.6

Use GA storageClass field

k8s.version >= 1.4 && k8s.version < 1.6

Use beta annotation

k8s.version >= 1.2 && k8s.version < 1.4

Use alpha annotation

k8s.version < 1.2

Unsupported

@liggitt made the comment that since federation doesn't currently support version skew, it should really be using whichever mechanism is current (i.e. stable field for >= 1.6). I'll update the PR accordingly.

This is very different than the version skew conversation we are having right now. The current conversation is about the skew between the cluster versions and the federation control plane version. This conversation is about hosting the federation control plane. The answers to these two could be very different.

It seems odd to me that we are now going to say that users can't host federation control plane in clusters older than 1.6 using kubefed while they could with kubefed 1.5 and 1.6. I don't feel too strongly about it, but in the absence of a well defined version skew statement, I would rather be liberal and use the beta annotation to accommodate 1.5 clusters. We don't have to worry about clusters older than 1.5 since kubefed did not even exist before then.

perotinus

LGTM once @saad-ali's comments are addressed

marun · 2017-05-25T17:27:16Z

I've revised the PR to use the stable field instead of an annotation. I'd appreciate feedback as to whether it is acceptable to require users to specify a storage class if a default is not provided for the host cluster. Given that this will be the behavior going forward, this change would appear necessary in the future if not now.

saad-ali · 2017-05-26T00:35:22Z

It sounds like you understand the implications of this change, so the change LGTM as long as federation folks are ok with it.

shashidharatd · 2017-05-26T07:46:08Z

federation/pkg/kubefed/init/init.go

@@ -662,6 +669,7 @@ func createPVC(clientset client.Interface, namespace, svcName, federationName, e
 					api.ResourceStorage: capacity,
 				},
 			},
+			StorageClassName: storageClassName,


Better to not set this field, if none is specified by user, so that DefaultStorageClass is used.
Here is note from the documentation in https://kubernetes.io/docs/concepts/storage/persistent-volumes/#storageclasses

Do not use volume.beta.kubernetes.io/storage-class: with any value including the empty string since it will prevent DefaultStorageClass admission controller from running if enabled.

In the future, we expect most clusters to have DefaultStorageClass enabled, and to have some form of storage available. However, there may not be any storage class names which work on all clusters, so continue to not set one by default. At some point, the alpha annotation will cease to have meaning, but the unset storageClass field on the PVC will have the desired effect.

I'm not sure what your concern is. Isn't assigning a field a value of nil effectively the same as not setting the field?

LGTM. I overlooked above if condition.

shashidharatd · 2017-05-30T04:17:48Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this
applying lgtm as all discussions been resolved.
/lgtm

madhusudancs · 2017-05-30T22:10:49Z

/approve

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

k8s-github-robot · 2017-05-30T22:17:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: madhusudancs, marun, shashidharatd

Needs approval from an approver in each of these OWNERS Files:

~~federation/OWNERS~~ [madhusudancs]
~~hack/verify-flags/OWNERS~~ [madhusudancs,shashidharatd]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

shashidharatd · 2017-06-01T06:30:39Z

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

k8s-github-robot · 2017-06-01T10:42:06Z

Automatic merge from submit-queue (batch tested with PRs 46686, 45049, 46323, 45708, 46487)

marun added the sig/federation label May 24, 2017

marun assigned madhusudancs May 24, 2017

marun requested review from madhusudancs, perotinus and csbell May 24, 2017 04:21

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/federation labels May 24, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 24, 2017

marun force-pushed the fed-kubefed-beta-storage-class-annotation branch from 417aaf9 to 50c091d Compare May 24, 2017 05:16

saad-ali self-assigned this May 24, 2017

saad-ali reviewed May 24, 2017

View reviewed changes

perotinus reviewed May 24, 2017

View reviewed changes

kubefed: Use StoraceClassName class field for etcd pvc

2c886e9

marun force-pushed the fed-kubefed-beta-storage-class-annotation branch from 50c091d to 2c886e9 Compare May 25, 2017 17:21

marun changed the title ~~[Federation][kubefed]: Use beta storage class annotation for etcd pvc~~ [Federation][kubefed]: Use StorageClassName for etcd pvcd May 25, 2017

marun changed the title ~~[Federation][kubefed]: Use StorageClassName for etcd pvcd~~ [Federation][kubefed]: Use StorageClassName for etcd pvc May 25, 2017

shashidharatd reviewed May 26, 2017

View reviewed changes

k8s-ci-robot assigned shashidharatd May 30, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 30, 2017

k8s-github-robot merged commit 3e1d686 into kubernetes:master Jun 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Federation][kubefed]: Use StorageClassName for etcd pvc #46323

[Federation][kubefed]: Use StorageClassName for etcd pvc #46323

marun commented May 24, 2017 •

edited

Loading

marun commented May 24, 2017

saad-ali left a comment

saad-ali May 24, 2017

marun May 25, 2017

madhusudancs May 26, 2017

saad-ali May 24, 2017

marun May 25, 2017

madhusudancs May 26, 2017

perotinus left a comment

marun commented May 25, 2017

saad-ali commented May 26, 2017

shashidharatd May 26, 2017

marun May 29, 2017

shashidharatd May 30, 2017

shashidharatd commented May 30, 2017

madhusudancs commented May 30, 2017

k8s-github-robot commented May 30, 2017

shashidharatd commented Jun 1, 2017

k8s-github-robot commented Jun 1, 2017

[Federation][kubefed]: Use StorageClassName for etcd pvc #46323

[Federation][kubefed]: Use StorageClassName for etcd pvc #46323

Conversation

marun commented May 24, 2017 • edited Loading

marun commented May 24, 2017

saad-ali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perotinus left a comment

Choose a reason for hiding this comment

marun commented May 25, 2017

saad-ali commented May 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashidharatd commented May 30, 2017

madhusudancs commented May 30, 2017

k8s-github-robot commented May 30, 2017

shashidharatd commented Jun 1, 2017

k8s-github-robot commented Jun 1, 2017

marun commented May 24, 2017 •

edited

Loading