Improve kubectl get experience for service catalog resources #2091

luksa · 2018-06-04T14:02:24Z

Instead of using custom-columns in OpenAPI schema, this commit adds
server-side column printing by registering TableConvertors.

pmorie

Looks like a promising start, but I'm not sure of kubectl column naming conventions. A more fundamental question is what information to show for ServiceInstance re request and matched service/plan and all the different ways you can request a service/plan.

pmorie · 2018-06-12T14:40:16Z

pkg/registry/servicecatalog/binding/storage.go

+			[]metav1beta1.TableColumnDefinition{
+				{Name: "Name", Type: "string", Format: "name"},
+				{Name: "Instance", Type: "string"},
+				{Name: "Secret", Type: "string"},


This should be SecretName

OK, I've changed this to Secret-Name so it gets printed as SECRET-NAME, but I think a simple SECRET is perfectly fine. This is just the column header.

pmorie · 2018-06-12T14:41:30Z

pkg/registry/servicecatalog/clusterserviceclass/storage.go

+		TableConvertor: tableconvertor.NewTableConvertor(
+			[]metav1beta1.TableColumnDefinition{
+				{Name: "Name", Type: "string", Format: "name"},
+				{Name: "External name", Type: "string"},


Seems like this should be ExternalName

Changed to External-Name so it gets printed as EXTERNAL-NAME (otherwise, it would be printed as EXTERNALNAME).

pmorie · 2018-06-12T14:41:57Z

pkg/registry/servicecatalog/clusterserviceclass/storage.go

+				{Name: "External name", Type: "string"},
+				{Name: "Broker", Type: "string"},
+				{Name: "Bindable", Type: "bool"},
+				{Name: "Plan updatable", Type: "bool"},


Hrm - reading these, I'm not certain what the naming convention is for kubectl columns.

They use two different forms:

Cluster-IP, OS-Image, Kernel-Version, Container-Runtime, ...

Node Selector, Last Schedule, Access Modes,Reclaim Policy

See: https://github.com/kubernetes/kubernetes/blob/7eb88f11d23d2be7dc3a91f727a1a77a0abac5e8/pkg/printers/internalversion/printers.go#L76-L422

pmorie · 2018-06-12T14:42:18Z

pkg/registry/servicecatalog/clusterserviceplan/storage.go

+			[]metav1beta1.TableColumnDefinition{
+				{Name: "Name", Type: "string", Format: "name"},
+				{Name: "External name", Type: "string"},
+				{Name: "Broker", Type: "string"},


This should be ClusterServiceBrokerName

This will be the printed name, correct? I thought the point of tweaking the default printing was to get closer to what svcat does (and a huge part of that is making our field names shorter and more intuitive). What's wrong with just Broker?

I don't see the value of calling the column CLUSTER-SERVICE-BROKER-NAME, as it just makes the output unnecessarily wide:

NAME EXTERNAL-NAME CLUSTER-SERVICE-BROKER-NAME BINDABLE PLAN-UPDATABLE AGE 4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468 user-provided-service ups-broker true true 12m 5f6e6cf6-ffdd-425f-a2c7-3c9258ad2468 user-provided-service-single-plan ups-broker true true 12m 8a6229d4-239e-4790-ba1f-8367004d0473 user-provided-service-with-schemas ups-broker true true 12m

pmorie · 2018-06-12T14:43:26Z

pkg/registry/servicecatalog/instance/storage.go

+		TableConvertor: tableconvertor.NewTableConvertor(
+			[]metav1beta1.TableColumnDefinition{
+				{Name: "Name", Type: "string", Format: "name"},
+				{Name: "Class", Type: "string"},


We should think about how we want to differentiate ns/cluster scoped classes/plans and requested vs. matched service/plan here

I suggest not differentiating for now and just printing both cluster and ns-scoped classes/plans in the same column. I definitely don't want to see two different columns for this, as that would make the output too wide.

But we could add a prefix to the values in the column (e.g. csc/class-name for clusterserviceclasses and sc/class-namefor serviceclasses)

FYI, a couple of us talked about what this should look like in svcat at the last F2F, here are notes:

#1826 (comment)

After thinking about it more, I would much rather prefer that we display the namespace (and leave it empty for cluster level resources) than use the prefixes.

Yeah, I don't like the short prefixes idea either. But I also don't like displaying the namespace, since a ServiceInstance never references a ServiceClass/Plan in a different namespace. It's either the same namespace as the ServiceInstance or no namespace.

Actually, I've just remembered that in core k8s, a RoleBinding can reference either a ClusterRole or a namespaced Role. This is how kubectl prints the reference:

$ kubectl get rolebinding -o wide NAME AGE ROLE USERS ... test 1m Role/my-namespaced-role foo test2 2s ClusterRole/my-cluster-role foo

So, to be consistent, we need to prefix with ServiceClass/ and ClusterServiceClass/.

luksa · 2018-06-12T15:56:58Z

Here are the new outputs (these include the changes requested by @pmorie):

$ k get clusterservicebrokers
NAME         URL                                                         STATUS    AGE
ups-broker   http://ups-broker-ups-broker.ups-broker.svc.cluster.local   Ready     18m

$ k get clusterserviceclasses
NAME                                   EXTERNAL-NAME                        BROKER       BINDABLE   PLAN-UPDATABLE   AGE
4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468   user-provided-service                ups-broker   true       true             18m
5f6e6cf6-ffdd-425f-a2c7-3c9258ad2468   user-provided-service-single-plan    ups-broker   true       true             18m
8a6229d4-239e-4790-ba1f-8367004d0473   user-provided-service-with-schemas   ups-broker   true       true             18m

$ k get clusterserviceplans
NAME                                   EXTERNAL-NAME   BROKER       CLASS                                  AGE
4dbcd97c-c9d2-4c6b-9503-4401a789b558   default         ups-broker   8a6229d4-239e-4790-ba1f-8367004d0473   18m
86064792-7ea2-467b-af93-ac9694d96d52   default         ups-broker   4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468   18m
96064792-7ea2-467b-af93-ac9694d96d52   default         ups-broker   5f6e6cf6-ffdd-425f-a2c7-3c9258ad2468   18m
cc0d7529-18e8-416d-8946-6f7456acd589   premium         ups-broker   4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468   18m

$ k get serviceinstances
NAME           CLASS                   PLAN      STATUS    AGE
ups-instance   user-provided-service   default   Ready     18m

$ k get servicebindings
NAME          SERVICE-INSTANCE   SECRET-NAME   STATUS    AGE
ups-binding   ups-instance       ups-binding   Ready     18m

jboyd01 · 2018-06-13T21:45:00Z

LGTM. This only takes effect if the kubeclient is of a specific version right? Is that 1.10 or 1.11? And older clients get the older, non friendly output?

luksa · 2018-06-14T07:09:30Z

@jboyd01 yeah, the client needs to support server-side printing. Looks like this is only enabled by default in kubectl 1.11. In 1.10 you need to enable it explicitly with --experimental-server-print=true.

carolynvs · 2018-06-14T18:11:22Z

Due to a bug in our travis file, this PR must be rebased in order to get the Travis CI build to run for this PR.

luksa · 2018-06-15T08:33:24Z

Rebased.

carolynvs · 2018-06-21T21:23:44Z

pkg/registry/servicecatalog/binding/storage.go

+				{Name: "Status", Type: "string"},
+				{Name: "Age", Type: "string"},
+			},
+			func(obj runtime.Object, m metav1.Object, name, age string) ([]interface{}, error) {


I've got a bunch of logic just like this in the svcat cli too. Maybe we can consolidate in a follow-on issue?

https://github.com/kubernetes-incubator/service-catalog/blob/master/pkg/svcat/service-catalog/binding.go#L198

https://github.com/kubernetes-incubator/service-catalog/blob/master/cmd/svcat/output/binding.go#L29

carolynvs · 2018-06-21T21:26:35Z

pkg/registry/servicecatalog/clusterserviceclass/storage.go

+				{Name: "Name", Type: "string", Format: "name"},
+				{Name: "External-Name", Type: "string"},
+				{Name: "Broker", Type: "string"},
+				{Name: "Bindable", Type: "bool"},


Bindable and plan-updatable aren't great candidates to promote to columns. Maybe I'm missing a use case but that's never information I'm looking for. Description seems more useful to promote up, that's what we display in the svcat cli.

carolynvs · 2018-06-21T21:29:36Z

pkg/registry/servicecatalog/instance/storage.go

+		TableConvertor: tableconvertor.NewTableConvertor(
+			[]metav1beta1.TableColumnDefinition{
+				{Name: "Name", Type: "string", Format: "name"},
+				{Name: "Class", Type: "string"},


After thinking about it more, I would much rather prefer that we display the namespace (and leave it empty for cluster level resources) than use the prefixes.

carolynvs · 2018-06-21T21:31:46Z

pkg/registry/servicecatalog/instance/storage.go

+				instance := obj.(*servicecatalog.ServiceInstance)
+				cells := []interface{}{
+					name,
+					instance.Spec.GetSpecifiedServiceClass(),


FYI, @eriknelson 's first pass on this resulted in two separate functions being created: one for cluster level classes/plans and another for namespaced. We spoke at the time about a follow-on PR that consolidated the two when being used by the svcat CLI so that we don't need awkward if/else statements because our helpers aren't helpful enough.

Just wanted you to be aware that this won't work the way you expect right now.

@carolynvs Thanks. Looks like we still don't have a function like GetSpecifiedClusterServiceClassOrServiceClass(), so I created one myself. I've named it GetSpecifiedClass() (and GetSpecifiedPlan()) for now. Any better ideas?

Nevermind. I'll throw that function out, since I no longer need it. I'll let someone else deal with the pain of naming these two methods :)

jboyd01 · 2018-07-24T19:16:13Z

/retest

jboyd01 · 2018-07-24T19:25:54Z

@luksa looks like this needs a rebase and updating, seeing unit test failures now.

Instead of using custom-columns in OpenAPI schema, this commit adds server-side column printing by registering TableConvertors.

luksa · 2018-08-09T15:58:16Z

This is what the ServiceInstance columns look like after my latest changes:

$ kubectl get serviceinstances
NAME                 CLASS                                       PLAN                         STATUS    AGE
local-ups-instance   ServiceClass/user-provided-service          ServicePlan/default          Ready     6m
ups-instance         ClusterServiceClass/user-provided-service   ClusterServicePlan/default   Ready     12m

This is now consistent with RoleBindings:

$ kubectl get rolebinding -o wide
NAME       AGE       ROLE                               USERS     ...
test       1m        Role/my-namespaced-role            foo                 
test2      2s        ClusterRole/my-cluster-role        foo

Not sure how I feel about the duplication (ServiceClass & ServicePlan). We could maybe omit the prefix in the PLAN column?

carolynvs · 2018-08-09T16:38:29Z

@luksa In svcat I added a namespace column to the output so that if it's set, you know it's a ServicePlan, and if it's empty its a ClusterServicePlan. Maybe something like that would help?

luksa · 2018-08-10T07:22:58Z

@carolynvs Not sure if we're talking about the same thing. There are two different problems (I'll focus only on classes here to keep things simple; the same rules should apply to plans):

when running svcat get classes, the output needs to show whether each row represents a ClusterServiceClass or a ServiceClass.
when running svcat get instances or kubectl get serviceinstances, the output needs to show whether the referenced class is a ClusterServiceClass or a ServiceClass.

The first problem only applies to svcat. There's no such problem in kubectl, since you use two different commands (kubectl get clusterserviceclasses vs kubectl get serviceclasses). I'm only trying to solve the second problem.

Because a ServiceInstance can only reference a ServiceClass in the same namespace or a ClusterServiceClass, we only need to show whether the referenced class is namespaced or not. If it is, it's obviously in the same namespace as the instance.

I don't like using a NAMESPACE column to show whether an instance is referencing a namespaced or a global class for two reasons:

a namespace column might imply that an instance could also reference a class in a different namespace
kubectl get instances --all-namespaces already shows a NAMESPACE column, so we'd have to name our column REFERENCED-CLASS-NAMESPACE (or something similar) and it would only repeat what's shown in the NAMESPACE column (or show an empty value when referencing global classes).

BTW: I don't see svcat print the name of the referenced class/plan, when they are namespaced:

$ svcat get instances
         NAME          NAMESPACE           CLASS            PLAN     STATUS  
+--------------------+-----------+-----------------------+---------+--------+
  local-ups-instance   test-ns                                       Ready   
  ups-instance         test-ns     user-provided-service   default   Ready

But it does print out both namespaced and global classes:

$ svcat get classes
                 NAME                  NAMESPACE         DESCRIPTION        
+------------------------------------+-----------+-------------------------+
  user-provided-service                            A user provided service  
  user-provided-service-single-plan                A user provided service  
  user-provided-service-with-schemas               A user provided service  
  user-provided-service                test-ns     A user provided service  
  user-provided-service-single-plan    test-ns     A user provided service  
  user-provided-service-with-schemas   test-ns     A user provided service

carolynvs · 2018-08-10T13:05:23Z

Thanks for the clarification! Yes I was mixing up two different things.

FYI, svcat hasn't been updated all the way through to support namespaced brokers. So any weirdness, is just because we aren't done yet. 😀

luksa · 2018-08-10T14:11:07Z

@carolynvs :) So, what's your opinion about prefixing just the CLASS column, and leaving the PLAN column without it? I'm now very confident we should use the prefix for the CLASS column (to keep things consistent with how kubectl prints out RoleBindings). I think we can also safely skip the prefix in the PLAN column, because of the parent-child relationship between classes and plans (since plans are part of the class, it's redundant to repeat the information).

So, this is what I'm suggesting:

$ kubectl get serviceinstances
NAME                 CLASS                                       PLAN      STATUS    AGE
local-ups-instance   ServiceClass/user-provided-service          default   Ready     6m
ups-instance         ClusterServiceClass/user-provided-service   default   Ready     12m

carolynvs · 2018-08-10T14:12:36Z

I think that is solid, consistent and understandable.

luksa · 2018-08-10T15:44:06Z

@carolynvs Thanks. Done.

carolynvs

/lgtm

I am officially awarding you 1 MILLION ✨ internet points for getting this through. THANK YOU! ❤️

jboyd01 · 2018-08-10T17:16:20Z

I agree with #2091 (review) - a lot of work to get this in, it will make a great difference to those using kubectl vs svcat.
LGTM
/approve

k8s-ci-robot · 2018-08-10T17:16:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jboyd01

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~~ [jboyd01]

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

jboyd01 · 2018-08-10T17:18:42Z

Travis failed with this:

Verify that vendor/ is in sync with Gopkg.lock

docker run --security-opt label:disable --rm -v /home/travis/build/kubernetes-incubator/service-catalog:/go/src/github.com/kubernetes-incubator/service-catalog -v /home/travis/build/kubernetes-incubator/service-catalog/.cache:/root/.cache/ -v /home/travis/build/kubernetes-incubator/service-catalog/.pkg:/go/pkg --env AZURE_STORAGE_CONNECTION_STRING scbuildimage build/verify-vendor.sh
grouped write of manifest, lock and vendor: error while writing out vendor tree: failed to write dep tree: failed to export github.com/PuerkitoBio/urlesc: https://github.com/PuerkitoBio/urlesc does not exist in the local cache and fetching failed: unable to get repository: Cloning into '/go/pkg/dep/sources/https---github.com-PuerkitoBio-urlesc'...
fatal: unable to access 'https://github.com/PuerkitoBio/urlesc/': GnuTLS recv error (-54): Error in the pull function.

I've restarted the job.

jboyd01 · 2018-08-10T17:46:26Z

hmm, different failure this time:

Step 3/12 : RUN curl -sSL -o /usr/local/bin/dep https://github.com/golang/dep/releases/download/$DEP_VERSION/dep-linux-amd64 && chmod +x /usr/local/bin/dep
---> Running in dcd2953075f1
curl: (56) SSL read: error:00000000:lib(0):func(0):reason(0), errno 104
The command '/bin/sh -c curl -sSL -o /usr/local/bin/dep https://github.com/golang/dep/releases/download/$DEP_VERSION/dep-linux-amd64 && chmod +x /usr/local/bin/dep' returned a non-zero code: 56
make: *** [.scBuildImage] Error 56

restarted again

jboyd01 · 2018-08-10T18:23:43Z

travis, travis, travis...

Running full build
mkdir -p .cache
mkdir -p .pkg
sed "s/GO_VERSION/1.10/g" < build/build-image/Dockerfile |
docker build -t scbuildimage -f - .
Sending build context to Docker daemon 12.98MB
Step 1/12 : FROM golang:1.10
1.10: Pulling from library/golang
No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 4, 2018

luksa force-pushed the kubectl_columns_tableconvertor branch 3 times, most recently from 0befdfe to a2bed79 Compare June 5, 2018 05:53

jboyd01 mentioned this pull request Jun 8, 2018

Output for oc get clusterserviceclasses is no longer user friendly #1530

Closed

luksa mentioned this pull request Jun 11, 2018

Show AGE column for service catalog resources in kubectl #1598

Closed

pmorie reviewed Jun 12, 2018

View reviewed changes

carolynvs added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2018

luksa force-pushed the kubectl_columns_tableconvertor branch from d3ec96c to a5ab712 Compare June 15, 2018 08:32

luksa force-pushed the kubectl_columns_tableconvertor branch from a5ab712 to 5f39bc0 Compare June 15, 2018 09:09

carolynvs removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 15, 2018

carolynvs reviewed Jun 21, 2018

View reviewed changes

luksa force-pushed the kubectl_columns_tableconvertor branch from 5f39bc0 to 9c6c9fa Compare July 16, 2018 09:19

MHBauer added the awaiting-ci label Jul 24, 2018

jboyd01 mentioned this pull request Jul 30, 2018

Improve kubectl get for the service-catalog resources #1634

Closed

luksa force-pushed the kubectl_columns_tableconvertor branch 3 times, most recently from ded0b59 to a26eca5 Compare August 9, 2018 13:48

luksa added 3 commits August 9, 2018 16:09

Improve kubectl get experience for service catalog resources

6bc638d

Instead of using custom-columns in OpenAPI schema, this commit adds server-side column printing by registering TableConvertors.

Fix column names

485d94a

Use ServiceInstanceSpec.GetSpecifiedServiceClass/Plan()

7dd5552

luksa force-pushed the kubectl_columns_tableconvertor branch from a26eca5 to 892ce28 Compare August 9, 2018 14:09

luksa added 2 commits August 9, 2018 17:52

Remove columns "Bindable" and "Plan-Updatable"

cb4391f

Print either the ClusterServiceClass or the ServiceClass name (& Plan)

18921c4

Print only the plan name in the PLAN column

ce86709

luksa force-pushed the kubectl_columns_tableconvertor branch from 892ce28 to ce86709 Compare August 10, 2018 15:41

carolynvs approved these changes Aug 10, 2018

View reviewed changes

k8s-ci-robot assigned carolynvs Aug 10, 2018

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2018

k8s-ci-robot merged commit 7e4c88e into kubernetes-retired:master Aug 10, 2018

luksa deleted the kubectl_columns_tableconvertor branch August 11, 2018 06:54

Improve kubectl get experience for service catalog resources #2091

Improve kubectl get experience for service catalog resources #2091

Conversation

luksa commented Jun 4, 2018

pmorie left a comment • edited Loading

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

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

luksa Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

luksa commented Jun 12, 2018 • edited Loading

jboyd01 commented Jun 13, 2018

luksa commented Jun 14, 2018

carolynvs commented Jun 14, 2018 • edited Loading

luksa commented Jun 15, 2018

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

jboyd01 commented Jul 24, 2018

jboyd01 commented Jul 24, 2018

luksa commented Aug 9, 2018 • edited Loading

carolynvs commented Aug 9, 2018

luksa commented Aug 10, 2018

carolynvs commented Aug 10, 2018

luksa commented Aug 10, 2018

carolynvs commented Aug 10, 2018

luksa commented Aug 10, 2018

carolynvs left a comment

Choose a reason for hiding this comment

jboyd01 commented Aug 10, 2018

k8s-ci-robot commented Aug 10, 2018

jboyd01 commented Aug 10, 2018

Verify that vendor/ is in sync with Gopkg.lock

jboyd01 commented Aug 10, 2018

jboyd01 commented Aug 10, 2018

pmorie left a comment •

edited

Loading

luksa Aug 9, 2018 •

edited

Loading

luksa commented Jun 12, 2018 •

edited

Loading

carolynvs commented Jun 14, 2018 •

edited

Loading

luksa commented Aug 9, 2018 •

edited

Loading