Refine route list output

[zhanggbj]

Fixes #
issue 350

Proposed Changes

• Show tag for revisionName if any
• Make the traffic column shorter

With this PR, output of route list will be

$ ./kn route list
NAME          URL                                                                                AGE    CONDITIONS   TRAFFIC
gracecanary   http://gracecanary.default.zhanggbj-knative.us-south.containers.appdomain.cloud   5d1h   3 OK / 3     (current)gracecanary-blue=90, gracecanary-green=10
helloworld    http://helloworld.default.zhanggbj-knative.us-south.containers.appdomain.cloud    27d    3 OK / 3     (old)helloworld-kp2pk=50, (new)helloworld-fw5zj=50

Release Note

[knative-prow-robot]

@zhanggbj: 0 warnings.

In response to this:

• Show tag for revisionName if any
• Make the traffic colume shorter

issue 350

Fixes #

Proposed Changes

Release Note

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[knative-prow-robot]

Hi @zhanggbj. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[rhuss]

/ok-to-test

[navidshaikh]

Are AGE and CONDITIONS columns useful here ?
The readiness of a route can be seen in service status as well, can we compact the route list output to show the most relevant info ?

[rhuss]

I even question whether we want to have the detailed traffic splitting in a list output. This column can be unbounded and does not nicely fit on a single line in a general case. And if crammed into a single line it easily gets confusing.

IMO this kind of information belongs to kn route describe like it is for kn service describe, where kn service list does not reveal the traffic split (but 'describe' does).

[rhuss]

In general I would like to impose the rule not to show array/list-value on an overview view in kn ... list. It's really hard to keep that confined to a line's max lengths, without truncating. And if truncating happens the value of the column is very limited. So I would reserve such output for kn ... describe where we can easily print out list output via multiple rows (so it can be quite length as the number of lines is not so critical as the length of a line).

Said that we could add something like the main revision + the portion of traffic it gets if its not 100%

$ ./kn route list
NAME          URL                                                                               AGE    CONDITIONS   REVISION
gracecanary   http://gracecanary.default.zhanggbj-knative.us-south.containers.appdomain.cloud   5d1h   3 OK / 3     gracecanary-blue (90%)
helloworld    http://helloworld.default.zhanggbj-knative.us-south.containers.appdomain.cloud    27d    3 OK / 3     helloworld-kp2pk

Still not sure whether this is very helpful, but at least the length of the last column is then fixed to a single revision name. The tag name I would reserve as a detail for kn route describe.

[rhuss]

/retest

[maximilien]

I wonder if we should explicitly deal with cases where the API returns empty tag? Perhaps have a token like NOTAG or similar?

[rhuss]

I don't think that is necessary as an empty tag is not distinguishable from having no tag ("" is the empty string value, too, there is no nil string in go).

And when there is no tag there is no need to add something.

However, that tag should be added with a space between revision name and opening parentheses.

[maximilien]

I disagree. But I can live with it.

[zhanggbj]

Thanks @navidshaikh @rhuss @maximilien for your input. As there are lots of different suggestions of the route list output, I'm thinking we can take this PR as a place for further discussion. How about I list the options for voting and comments, and anyone can list more voting options if necessary. From Above comments, I think we have two items now. Please choose 👍(agree) or 👎 (disagree) and leave your comments if needed.

1. Keep AGE and CONDITIONS in route list or not
2. Show detailed traffic splitting in route list or just show "main revision + the portion of traffic it gets if its not 100%".

[zhanggbj]

Vote 1a):
Keep AGE and CONDITIONS in route list

[zhanggbj]

Vote 1b):
Remove AGE and CONDITIONS in route list

[zhanggbj]

Vote 2a):
Show detailed traffic splitting in route list like below

$ ./kn route list
NAME          URL                                                                                AGE    CONDITIONS   TRAFFIC
gracecanary   http://gracecanary.default.zhanggbj-knative.us-south.containers.appdomain.cloud   5d1h   3 OK / 3     (current)gracecanary-blue=90, gracecanary-green=10
helloworld    http://helloworld.default.zhanggbj-knative.us-south.containers.appdomain.cloud    27d    3 OK / 3     (old)helloworld-kp2pk=50, (new)helloworld-fw5zj=50

[zhanggbj]

Vote 2b):
just show "main revision + the portion of traffic it gets if it's not 100%" and leave details in route describe

$ ./kn route list
NAME          URL                                                                               AGE    CONDITIONS   REVISION
gracecanary   http://gracecanary.default.zhanggbj-knative.us-south.containers.appdomain.cloud   5d1h   3 OK / 3     gracecanary-blue (90%)
helloworld    http://helloworld.default.zhanggbj-knative.us-south.containers.appdomain.cloud    27d    3 OK / 3     helloworld-kp2pk

[sixolet]

Show the most-traffic revision and relabel it "MAIN REVISION" or "MOST TRAFFIC" or something like that.

[rhuss]

Maybe we can list a set of use cases for kn route list and kn route describe which doesn't fit to kn service list or kn service describe. We can then decide whether its worth to introduce a high-level command like 'kn route' or whether we can find other options.

To be clear, if we would have a kn route create for creating routes independently from services, then listing of routes makes total sense. But if this is supposed to be always a 1:1 relation I don't see the need for an additional command (everything could be covered by the service command, too).

Here is a use case, I could see for kn route list.

• As a user, for a given URL I want to find out which service is responsible for.
  Solution: kn route list | grep $url, but maybe also kn service describe --url $url ? (if this actually is an importan use case to cover. not sure about this)

For kn route describe I don't see anything which is not already covered by kn service describe.

Any other use case you can imaging for kn route commands ?

[rhuss]

@zhanggbj @toVersus @navidshaikh @sixolet I put the question whether we need kn route list and kn route describe at all on tomorrows WG agenda.

@zhanggbj sorry, I know it's very late for you (probably too late). We can also chat about it tomorrow on slack #cli (knative.slack.com). I will be there starting from 8am CEST.

[zhanggbj]

@rhuss @toVersus
I think for basic scenarios, Service has a 1:1 relation with Route, so we didn't see much value of output kn route list, but IMHO in some advanced scenarios as below, its not always 1:1 relation, so I think it's better to keep kn route list but refine the output to print more valuable and route related info.

1) Use custom url to access one Revision directly
As described in #407 (comment)

2) Routing across multiple Knative services
Details: https://knative.dev/docs/serving/samples/knative-routing-go/
In short, we can setup routing for different services using domain url as below

So in kn route list we may have

# kn route list
NAME          ROUTE                                                                              READY
gracecanary   http://gracecanary.default.zhanggbj-knative.containers.appdomain.cloud             true
gracecanary   http://zhanggbj-knative.containers.appdomain.cloud/login                           true
helloworld    http://helloworld.default.zhanggbj-knative.us-south.containers.appdomain.cloud     true
helloworld    http://zhanggbj-knative.us-south.containers.appdomain.cloud/test                   true

3) http-based routing for different Revisions of one Service
For now, Serving only supports routing based on weighted traffic, but I think in production users may have various routing requirements based on the incoming request, for e.g. based on path/user indentity/GET or Post request, So it's valuable for kn route list to show complex route rules to different Revisions of one service. Detailed discussion in knative/serving#4736, Knative Serving may support it after 1.0.

So in kn route list we may have

# kn route list
NAME          ROUTE                                                                                       REVISION                READY
gracecanary   http://gracecanary.default.zhanggbj-knative.containers.appdomain.cloud                      current-gracecanary     true
gracecanary   http://gracecanary.default.zhanggbj-knative.containers.appdomain.cloud Header=enduser:json  current-gracecanary     true
gracecanary   http://gracecanary.default.zhanggbj-knative.containers.appdomain.cloud Header=enduser:mike  candidate-gracecanary   true       

@rhuss It's great to discuss in WG on this, I may not able to attend it :-) And yes, we can discuss later on Slack. Thanks!

[rhuss]

@zhanggbj thanks a lot for your detailed exploration of possible route use cases. My comments on them following your enumeration above:

1. I see the point but you get the very same information also with kn service describe gracecanary and kn service helloworld, even much more important information like to which revision and especially to which image is an URL pointing to and how the traffic is split. The question I really had is important to see all URL from all Service at once (like in kn route list) ? My feeling this case 1. is already nicely covered with the kn service commands.

2. I don't think that this use case applies to Route resources at all. It's about how to configure an Istio VirtualService as in this routing.yaml. So, the gateway would not be managed by kn anyway (but by Istio or generic Kubernetes tooling).

3. Header based routing might a future use case for kn when it is available in Serving. It could be managed equally well with kn service commands like the traffics split. But more importantly, this is something in the future and its not clear to me how Serving is going to implement that. IMO we should wait with a client implementation (or user visible preparation like adding new commands) until this feature has landed server side.

[rhuss]

I still think that the decision whether to implement kn routes list boils down to two questions:

• Has the collective displays of all routes a substantial value which justifies the introduction of a new top-level noun (routes)?
• Do we want to allow to manage routes independently from Services, breaking the 1:1 relationship? If so, then we should add also create and update to kn route to make it complete.

[zhanggbj]

Hi All,
Thanks for all the discussions, based on above comments, I think another alternative option is we still keep the route list for now but with only NAME/URL/READY as below. And leave the desition to completely remove route list until we have more users and more scenarios, if they think route list is not meaningful, we can remove it completely then.

I've submit a new PR and please help to take a review. Thx!

$ ./kn route list   
NAME                URL                                                                                                                           READY
gracecanary         http://gracecanary.default.zhanggbj-knative.us-south.containers.appdomain.cloud         True
helloworld          http://helloworld.default.zhanggbj-knative.us-south.containers.appdomain.cloud          True

[rhuss]

thanks.

/lgtm
/approve

[knative-metrics-robot]

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/route/human_readable_flags.go	95.8%	93.8%	-2.1

[navidshaikh]

/lgtm

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: navidshaikh, rhuss, zhanggbj

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 [navidshaikh,rhuss]

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