Adding new AdmittedGateways field to Routes to improve kubectl output #587

robscott · 2021-03-18T18:05:03Z

What type of PR is this?
/kind cleanup
/kind api-change

What this PR does / why we need it:
This adds a new AdmittedGateways []string field in route status to list all Gateways that have admitted the route. This field is used in kubectl output to make it clearer which Gateways are bound to a given Route.

Which issue(s) this PR fixes:
Fixes #586

Does this PR introduce a user-facing change?:

Routes now include an admittedGateways field in status.

k8s-ci-robot · 2021-03-18T18:05:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: robscott

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

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

robscott · 2021-03-18T18:06:11Z

/cc @jpeach @danehans

robscott · 2021-03-18T18:21:23Z

apis/v1alpha1/shared_types.go

+	// by any Gateway.
+	//
+	// +kubebuilder:validation:MaxItems=100
+	AdmittedGateways []string `json:"admittedGateways"`


I'm not tied to this name. I also considered AdmittingGateways, ConnectedGateways, or ActiveGateways if any of those seem better. Also open to other names.

I guess the question is what are we trying to convey? If the status of the object was Admitted: false because there's some sort of error, should it still show in this list?

If that's the case then maybe admittedGateways isn't the right name (but was my first thought as well). =)

Possibly selectedGateways but that implies the opposite ordering (the route doesn't select the gateway).

Yeah I think the most useful thing we can show here for kubectl is the Gateways that are actively using this Route. I was thinking that if a user expects a Gateway to be in the list and it isn't they can dig into the full yaml output/status.

I agree that Admitted doesn't seem right, but I can't think of anything else that doesn't sound bad. InUseByGateways is probably accurate, but a bit of a mouthful.

bowei · 2021-03-29T17:14:53Z

apis/v1alpha1/shared_types.go

@@ -211,6 +211,19 @@ type RouteStatus struct {
 	//
 	// +kubebuilder:validation:MaxItems=100
 	Gateways []RouteGatewayStatus `json:"gateways"`
+
+	// AdmittedGateways is a list of Gateways that have admitted this route.


Should we make sure that there is one way language we talk about this relationship? Is it "admitted" or "bound" (both terms exist in the repo).

Ask: can we decide on one and change all references to be consistent?

I like bound more :)

Should we make sure that there is one way language we talk about this relationship?

+1, we currently use "Admitted" and should continue doing so IMO.

bowei · 2021-03-29T17:16:24Z

apis/v1alpha1/shared_types.go

+	// namespace/name format. This field is primarily used to provide kubectl
+	// output.
+	//
+	// A maximum of 100 Gateways will be represented in this list. If this list


Can we leave the #'s out of this comment? We should just say that this list may be truncated and also include an indicator for truncation:

AdmittedGatewaysTruncated bool

or something similar, maybe

AdmittedGatewaysCount int32

that people can see total even though it was truncated.

danehans · 2021-03-30T18:21:19Z

apis/v1alpha1/shared_types.go

@@ -211,6 +211,19 @@ type RouteStatus struct {
 	//
 	// +kubebuilder:validation:MaxItems=100
 	Gateways []RouteGatewayStatus `json:"gateways"`


Should Gateways still exist? It seems redundant.

The big difference is that this list of Gateways includes all the conditions/status for the associated Gateways. In this PR I'm proposing adding a simpler []string for the purpose of populating kubectl output, but the full status/conditions would still be valuable.

@robscott does kubectl provide a way to iterate through the gateway ref list and output gateways of "Admitted=True"?

Sadly not, that would simplify a lot of this. More examples of what I tried in #586 (comment). Unfortunately this is intentional behavior from upstream: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/tableconvertor/tableconvertor.go#L111. There's been some discussion around improving that experience, but it has unfortunately not gone anywhere. kubernetes/kubectl#517 tracks the issue, but as I understand it, this is actually something that would need to change in apiserver, not kubectl. I think kubernetes/kubernetes#67079 would have fixed this but unfortunately went stale and was closed.

Even if we were able to get a fix into Kubernetes 1.22, the kubectl experience would be quite poor for anyone on an older k8s version since the current implementation always returns the first element in the matching list.

So this is the names of the gateways that are included in the Gateways array and have an Admitted status of true? If so, it's redundant but I can see your point here.

The doc should specify the semantic described here. I don't think that the intention is to invent new semantics for this field; we just want to make existing semantics consumable by kubectl. We should avoid the word "bound" since that's not a term of art in the API.

k8s-ci-robot · 2021-04-09T04:33:31Z

@robscott: PR needs rebase.

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.

hbagdi · 2021-04-12T19:53:47Z

@robscott Did we land on a decision on if we want to include this feature or hold it?

robscott · 2021-04-12T21:18:12Z

@hbagdi Thanks for following up! This is on me to follow up with apimachinery to see what's possible/recommended for these kinds of things.

robscott · 2021-04-12T21:40:08Z

Sent this out to sig-api-machinery to see if they have any insight: https://groups.google.com/g/kubernetes-sig-api-machinery/c/GxXWe6T8DoM

robscott · 2021-04-16T23:50:18Z

I've created an upstream PR that should help with this. Of course that's probably only a realistic solution if we can also backport the fix to earlier k8s versions. For now I'm going to put a hold on this until there's more clarity upstream.

/hold

robscott · 2021-04-23T18:35:26Z

I think the correct approach is to try to solve this upstream, even though it will take more time. I'm closing this in favor of kubernetes/kubernetes#101205 for now. If I can't get momentum upstream I'll revisit making changes like this.

/close

k8s-ci-robot · 2021-04-23T18:35:32Z

@robscott: Closed this PR.

In response to this:

I think the correct approach is to try to solve this upstream, even though it will take more time. I'm closing this in favor of kubernetes/kubernetes#101205 for now. If I can't get momentum upstream I'll revisit making changes like this.

/close

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.

Adding new AdmittedGateways field to Routes to improve kubectl output

a31a9dc

k8s-ci-robot requested review from hbagdi and thockin March 18, 2021 18:05

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 18, 2021

k8s-ci-robot requested review from danehans and jpeach March 18, 2021 18:06

robscott commented Mar 18, 2021

View reviewed changes

robscott mentioned this pull request Mar 19, 2021

Add more columns to CRDs #588

Closed

robscott added this to the v0.3.0 milestone Mar 24, 2021

bowei reviewed Mar 29, 2021

View reviewed changes

danehans reviewed Mar 30, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2021

robscott added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Apr 14, 2021

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 16, 2021

k8s-ci-robot closed this Apr 23, 2021

hbagdi mentioned this pull request Apr 23, 2021

Summary of out-of-tree problems #629

Closed

robscott deleted the kubectl-gateway-status branch January 8, 2022 01:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding new AdmittedGateways field to Routes to improve kubectl output #587

Adding new AdmittedGateways field to Routes to improve kubectl output #587

robscott commented Mar 18, 2021 •

edited

Loading

k8s-ci-robot commented Mar 18, 2021

robscott commented Mar 18, 2021

robscott Mar 18, 2021

stevesloka Mar 29, 2021

robscott Mar 31, 2021

youngnick Apr 19, 2021

bowei Mar 29, 2021

SantoDE Mar 29, 2021

danehans Mar 30, 2021

bowei Mar 29, 2021

danehans Mar 30, 2021

robscott Mar 30, 2021

danehans Mar 31, 2021

robscott Mar 31, 2021

robscott Mar 31, 2021

jpeach Apr 19, 2021

k8s-ci-robot commented Apr 9, 2021

hbagdi commented Apr 12, 2021

robscott commented Apr 12, 2021

robscott commented Apr 12, 2021 •

edited

Loading

robscott commented Apr 16, 2021

robscott commented Apr 23, 2021

k8s-ci-robot commented Apr 23, 2021

Adding new AdmittedGateways field to Routes to improve kubectl output #587

Adding new AdmittedGateways field to Routes to improve kubectl output #587

Conversation

robscott commented Mar 18, 2021 • edited Loading

k8s-ci-robot commented Mar 18, 2021

robscott commented Mar 18, 2021

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

k8s-ci-robot commented Apr 9, 2021

hbagdi commented Apr 12, 2021

robscott commented Apr 12, 2021

robscott commented Apr 12, 2021 • edited Loading

robscott commented Apr 16, 2021

robscott commented Apr 23, 2021

k8s-ci-robot commented Apr 23, 2021

robscott commented Mar 18, 2021 •

edited

Loading

robscott commented Apr 12, 2021 •

edited

Loading