Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecating NoSuchGatewayClass status reason #635

Merged

Conversation

robscott
Copy link
Member

What type of PR is this?
/kind cleanup
/kind deprecation

What this PR does / why we need it:
This deprecates the NoSuchGatewayClass status reason. Although this was a useful concept, it was never clear who would actually set it. Although we could develop an official controller that watched Gateways and GatewayClasses and set this status, I don't think anyone has the capacity to maintain that. Alternatively, we could rely on every Gateway controller to write this status, but that could result in some fairly ugly race conditions.

I initially proposed a new condition to replace this, but I don't think that's necessary. After discussing this with @jpeach, it seemed like the existing ListenerConditionResolvedRefs and NotReconciled conditions were sufficient here.

Does this PR introduce a user-facing change?:

The NoSuchGatewayClass status reason has been deprecated.

/cc @jpeach @hbagdi

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 27, 2021
@k8s-ci-robot
Copy link
Contributor

[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:

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 27, 2021
@hbagdi
Copy link
Contributor

hbagdi commented Apr 27, 2021

+1 to this PR. I didn't add an lgtm so that another reviewer can review before we merge.

After discussing this with @jpeach, it seemed like the existing ListenerConditionResolvedRefs and NotReconciled conditions were sufficient here.

NoSuchGatewayClass was useful when you wanted to report status for the entire Gateway and not a specific listener. I think there is value in having a condition that is representative of status of the entire Gateway rather than all listeners reporting as "NotReconciled".

@youngnick
Copy link
Contributor

Yeah, I agree that it's very useful to have simple, easy-to-understand Conditions for critical errors (of which NoSuchGatewayClass) is one. Having that Condition means that, if you typo the GatewayClass, then you can clearly see why all the listeners are "NotReconciled".

/lgtm though.

@bowei
Copy link
Contributor

bowei commented Apr 28, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 28, 2021
@k8s-ci-robot k8s-ci-robot merged commit efd0246 into kubernetes-sigs:master Apr 28, 2021
@jpeach
Copy link
Contributor

jpeach commented Apr 28, 2021

@robscott I'm interpreting the comments from @hbagdi and @youngnick as being supportive of not deprecating this?

@youngnick
Copy link
Contributor

I'm okay with deprecating this Reason, I think there should be a separate, negative-polarity "NoGatewayClass" Condition.

@jpeach
Copy link
Contributor

jpeach commented Apr 28, 2021

I'm okay with deprecating this Reason, I think there should be a separate, negative-polarity "NoGatewayClass" Condition.

I think the fundamental issue that @robscott is trying to address is what actor could ever set that condition?

@youngnick
Copy link
Contributor

🤦 Yes, of course, that doesn't make any sense, thanks.

For Contour, since a Contour deployment must be 1:1 with a Gateway, we've experimented with setting a named Gateway directly rather than a GatewayClass (since that gets around having to handle the various dynamic discovery error cases like two matching Gateways, Gateway priority etc. But I think we're moving away from that idea now.)

@robscott robscott deleted the deprecate-nosuchgatewayclass branch January 8, 2022 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants