Adding default StorageClass annotation printout for resource_printer and describer and some refactoring #34638

screeley44 · 2016-10-12T19:51:45Z

adding ISDEFAULT for kubectl get storageclass output

[root@screeley-sc1 gce]# kubectl get storageclass
NAME            TYPE                   ISDEFAULT
another-class   kubernetes.io/gce-pd   NO        
generic1-slow   kubernetes.io/gce-pd   YES       
generic2-fast   kubernetes.io/gce-pd   YES

Add ISDEFAULT to kubectl get storageClass output

@kubernetes/sig-storage

This change is

k8s-ci-robot · 2016-10-12T19:51:46Z

Can a kubernetes member verify that this patch is reasonable to test? If so, please reply with "@k8s-bot ok to test" on its own line.

Regular contributors should join the org to skip this step.

erinboyd · 2016-10-14T04:45:47Z

@screeley44 This is very useful! +1

erinboyd · 2016-10-14T04:45:55Z

@saad-ali @eparis please review

pwittrock · 2016-10-14T14:46:42Z

LGTM. @eparis Put you as reviewer since you are closer to the area.

eparis · 2016-10-14T14:49:49Z

@k8s-bot ok to test

eparis · 2016-10-14T14:53:32Z

@screeley44 @erinboyd I'm cc'ing sig-cli.

My gut feeling is that customers are likely to have some number 2-10 StorageClasses and only 1 default. Multiple defaults, while possible, aren't likely to be the norm. Do we really want an entire column for this? Do we have any precedent for other ways to indicate something like this?

Also, what does kubectl describe show?

@kubernetes/sig-cli

I will point out I'm not questioning the need to show this information. I'm questioning HOW to show this information.

adohe-zz · 2016-10-14T15:00:18Z

Do we really want an entire column for this? Do we have any precedent for other ways to indicate something like this?

I would like not use an entire column for this, how about this:

NAME            TYPE                  
another-class(default)   kubernetes.io/gce-pd       
generic1-slow   kubernetes.io/gce-pd       
generic2-fast   kubernetes.io/gce-pd

screeley44 · 2016-10-14T15:58:41Z

@adohe - I like your suggestion much better, I updated the PR

[root@ose1 gce]# kubectl get storageclass
NAME                    TYPE
generic1(default)   kubernetes.io/gce-pd   
slow                       kubernetes.io/gce-pd

screeley44 · 2016-10-14T16:50:08Z

@eparis - I handle the describe stuff in another PR #34495 for storageclass, pv and pvc ... some example output below

# kubectl describe pv pv-gce2
Name:       pv-gce2
Labels:     <none>
StorageClass:   slow
...


# kubectl describe storageclass generic1
Name:       generic1
IsDefaultClass: Yes
Annotations:    storageclass.beta.kubernetes.io/is-default-class=true
Provisioner:    kubernetes.io/gce-pd
...

deads2k · 2016-10-14T20:39:50Z

pkg/kubectl/resource_printer.go

@@ -60,6 +60,9 @@ const (
 	loadBalancerWidth = 16
 )

+// This indicates that a particular StorageClass nominates itself as the system default.
+const betaAnnotation = "storageclass.beta.kubernetes.io/is-default-class"


We used the name default as the default serviceaccount and that has worked reasonably well for us. It ensures nothing collides (can't have two defaults) and avoids special markings like this.

that part has been all sorts of discussed between thockin and Jan. I'm more interested if its worth it to define something like isDefaultAnnotation on the storage class object or something, instead of having the const name twice...

between plugin/pkg/admission/storageclass/default/admission.go this PR and 34495 we've now scattered the test across 3 places in the code. Lets find a way to determine this in a single place.

@eparis - how about this, for now we can use exported const in the types.go from the storage package, that way we have a single place to maintain these and they are easily accessible...

screeley44 · 2016-10-17T15:08:06Z

closing #34495 and combining into this PR

eparis · 2016-10-17T19:09:22Z

pkg/kubectl/describe.go

@@ -162,6 +162,25 @@ func init() {
 	DefaultObjectDescriber = d
 }

+func getStorageClassAnnotation(obj api.ObjectMeta) string {


Seems to me like we should be moving getVolumeClass and getClaimClass from controller/volume/persistentvolume/pv_controller_base.go to pkg/api/helpers.go.

We should also implement an IsDefaultStorageClass() function in a new pkg/apis/storage/helpers.go

These helpers should be used everywhere. You missed the usages in controller/volume/persistentvolume/*

eparis · 2016-10-17T19:10:56Z

pkg/kubectl/describe.go

@@ -162,6 +162,25 @@ func init() {
 	DefaultObjectDescriber = d
 }

+func getStorageClassAnnotation(obj api.ObjectMeta) string {
+	if obj.Annotations[storage.BetaStorageClassAnnotation] != "" {


I think this would be better as below so correctly handly the explicitly set to empty.

if class, ok := obj.Annotations[storage.BetaStorageClassAnnotation]; ok { return class }

eparis · 2016-10-17T19:11:40Z

pkg/kubectl/describe.go

+	return ""
+}
+
+func isDefaultAnnotation(obj api.ObjectMeta) string {


This function might be 'ok-ish' here, but since I want a generic function that one needs to return a bool

eparis · 2016-10-17T19:13:20Z

pkg/kubectl/resource_printer.go

@@ -2078,8 +2078,16 @@ func printNetworkPolicyList(list *extensions.NetworkPolicyList, w io.Writer, opt
 	return nil
 }

+func hasDefaultAnnotation(obj api.ObjectMeta) bool {


This is basically a duplicate of the isDefaultAnnotation() function above. So it should disappear when you move the generic function which returns a bool

eparis · 2016-10-17T19:13:57Z

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

@@ -121,7 +115,7 @@ func (c *claimDefaulterPlugin) Admit(a admission.Attributes) error {
 		return nil
 	}

-	_, found := pvc.Annotations[classAnnotation]
+	_, found := pvc.Annotations[storage.BetaStorageClassAnnotation]


These will all move to your new generic GetClaimStorageClass() function.

k8s-ci-robot · 2016-10-18T17:18:57Z

Jenkins GCE Node e2e failed for commit ae569492631b117bf666750024e57be0d34faca1. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-10-18T17:18:57Z

Jenkins GCI GCE e2e failed for commit ae569492631b117bf666750024e57be0d34faca1. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-10-18T17:18:58Z

Jenkins Kubemark GCE e2e failed for commit ae569492631b117bf666750024e57be0d34faca1. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-10-18T17:19:11Z

Jenkins GCE etcd3 e2e failed for commit ae569492631b117bf666750024e57be0d34faca1. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-10-18T17:19:31Z

Jenkins GCE e2e failed for commit ae569492631b117bf666750024e57be0d34faca1. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-10-18T17:24:41Z

Jenkins verification failed for commit ae569492631b117bf666750024e57be0d34faca1. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-10-19T02:40:06Z

Jenkins unit/integration failed for commit 785d2228ecb8fd0d34c3f9abaccac8374d829173. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

screeley44 · 2016-10-19T02:55:26Z

@k8s-bot test this issue: #35064

screeley44 · 2016-10-19T04:34:29Z

@k8s-bot test this issue: #35064

screeley44 · 2016-10-19T15:34:22Z

@eparis - not sure how to handle the smoke test failure, any tips? I couldn't find any flakes and there isn't a lot of info in the logs to help diagnose

k8s-ci-robot · 2016-10-19T15:36:40Z

Jenkins GKE smoke e2e failed for commit 86f1a94. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

eparis · 2016-10-19T15:39:25Z

@jsafrane this might conflict with some work you are doing. so whoever loses the race is likely to need to rebase.

screeley44 · 2016-10-19T16:24:21Z

@k8s-bot test this issue: #33175

saad-ali · 2016-10-19T17:49:22Z

@k8s-bot gci gke e2e test this, Issue: #22655

k8s-github-robot · 2016-10-19T17:51:17Z

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

k8s-ci-robot · 2016-10-19T18:08:03Z

Jenkins GCI GKE smoke e2e failed for commit 86f1a94. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-github-robot · 2016-10-19T18:36:07Z

Automatic merge from submit-queue

googlebot added the cla: yes label Oct 12, 2016

k8s-github-robot assigned pwittrock Oct 12, 2016

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Oct 12, 2016

pwittrock assigned eparis and unassigned pwittrock Oct 14, 2016

deads2k reviewed Oct 14, 2016

View reviewed changes

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 17, 2016

screeley44 changed the title ~~Adding default StorageClass annotation printout for resource_printer~~ Adding default StorageClass annotation printout for resource_printer and describer Oct 17, 2016

eparis reviewed Oct 17, 2016

View reviewed changes

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 18, 2016

screeley44 changed the title ~~Adding default StorageClass annotation printout for resource_printer and describer~~ Adding default StorageClass annotation printout for resource_printer and describer and some refactoring Oct 18, 2016

k8s-github-robot removed the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Oct 18, 2016

Adding default StorageClass annotation printout for resource_printer

86f1a94

eparis added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Oct 19, 2016

k8s-github-robot merged commit 91b7e1f into kubernetes:master Oct 19, 2016

screeley44 mentioned this pull request Oct 19, 2016

Add storageclass into describer for PV, PVC and StorageClass #34495

Closed

d1egoaz mentioned this pull request Nov 21, 2022

Remove suffix (default) when acting on resources of StorageClasses abrochard/kubel#107

Merged

Adding default StorageClass annotation printout for resource_printer and describer and some refactoring #34638

Adding default StorageClass annotation printout for resource_printer and describer and some refactoring #34638

Conversation

screeley44 commented Oct 12, 2016 • edited by pwittrock

k8s-ci-robot commented Oct 12, 2016

erinboyd commented Oct 14, 2016

erinboyd commented Oct 14, 2016

pwittrock commented Oct 14, 2016

eparis commented Oct 14, 2016

eparis commented Oct 14, 2016 • edited

adohe-zz commented Oct 14, 2016

screeley44 commented Oct 14, 2016

screeley44 commented Oct 14, 2016

Choose a reason for hiding this comment

eparis Oct 14, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

screeley44 commented Oct 17, 2016

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

k8s-ci-robot commented Oct 18, 2016

k8s-ci-robot commented Oct 18, 2016

k8s-ci-robot commented Oct 18, 2016

k8s-ci-robot commented Oct 18, 2016

k8s-ci-robot commented Oct 18, 2016

k8s-ci-robot commented Oct 18, 2016

k8s-ci-robot commented Oct 19, 2016

screeley44 commented Oct 19, 2016

screeley44 commented Oct 19, 2016

screeley44 commented Oct 19, 2016

k8s-ci-robot commented Oct 19, 2016

eparis commented Oct 19, 2016

screeley44 commented Oct 19, 2016

saad-ali commented Oct 19, 2016

k8s-github-robot commented Oct 19, 2016

k8s-ci-robot commented Oct 19, 2016

k8s-github-robot commented Oct 19, 2016

screeley44 commented Oct 12, 2016 •

edited by pwittrock

eparis commented Oct 14, 2016 •

edited

eparis Oct 14, 2016 •

edited