Reverses Gateway Condition Polarity #104

danehans · 2020-02-17T17:35:30Z

The official guidance is being updated for conditions to reflect a positive polarity. This PR iterates on #47 to update the Gateway condition types accordingly.

/assign @robscott
/cc @bowei @ironcladlou @Miciah

k8s-ci-robot · 2020-02-17T17:35:35Z

@danehans: GitHub didn't allow me to request PR reviews from the following users: ironcladlou, Miciah.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

The official guidance is being updated for conditions to reflect a positive polarity. This PR iterates on #47 to update the Gateway condition types accordingly.

/assign @robscott
/cc @bowei @ironcladlou @Miciah

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.

k8s-ci-robot · 2020-02-17T17:35:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: danehans
To complete the pull request process, please assign thockin
You can assign the PR to them by writing /assign @thockin in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

robscott

Thanks for this PR! Some of these are pretty trick to turn into positive conditions. As an example, all of the conditions covering multiple objects become more complicated when they are positive. InvalidListeners indicated that at least one listener was invalid. ValidListeners as proposed would indicate if at least one listener was valid, but does not properly indicate if any listeners are invalid. We can change this to indicate that all listeners are valid, but it's not as clear from the condition name that that's what that means.

robscott · 2020-02-18T19:37:58Z

api/v1alpha1/gateway_types.go

-	// routes is invalid.
-	ConditionInvalidRoutes GatewayConditionType = "InvalidRoutes"
+	ConditionValidListeners GatewayConditionType = "ValidListeners"
+	// ConditionRoutesReady indicates that at least one of the specified routes


Suggested change

// ConditionRoutesReady indicates that at least one of the specified routes

// ConditionRoutesReady indicates that all of the specified routes

robscott · 2020-02-18T19:38:30Z

api/v1alpha1/gateway_types.go

+	// ConditionRoutesReady indicates that at least one of the specified routes
+	// is ready.
+	ConditionRoutesReady GatewayConditionType = "RoutesReady"
+	// ConditionValidRoutes indicates that at least one of the specified


Suggested change

// ConditionValidRoutes indicates that at least one of the specified

// ConditionValidRoutes indicates that all of the specified

robscott · 2020-02-18T19:39:17Z

api/v1alpha1/gateway_types.go

-	// ConditionInvalidListeners indicates that at least one of the specified
-	// listeners is invalid. If this condition has a status of True, a more
+	ConditionListenersReady GatewayConditionType = "ListenersReady"
+	// ConditionValidListeners indicates that at least one of the specified


Suggested change

// ConditionValidListeners indicates that at least one of the specified

// ConditionValidListeners indicates that all of the specified

robscott · 2020-03-18T18:55:53Z

@danehans thanks for the work on this! I think this is a good translation of the existing conditions to a positive polarity. This PR looks good to me for what it aims to do, but I'd personally like to see more activity/confirmation on kubernetes/community#4521 before merging this just so we're sure that this is something the broader community will fully adopt.

bowei · 2020-03-18T19:27:15Z

api/v1alpha1/gateway_types.go

-	// routes is invalid.
-	ConditionInvalidRoutes GatewayConditionType = "InvalidRoutes"
+	// ConditionGatewayClassExists indicates that the specified GatewayClass exists.
+	ConditionGatewayClassExists GatewayConditionType = "GatewayClassExists"


Can we start off with GatewayClassInvalid?

@bowei GatewayClassInvalid is a negative condition type. This PR aims to use positive conditions per kubernetes/community#4521. I can update "GatewayClassExists" to "GatewayClassValid" with the same meaning that the GatewayClass referenced in gateway.spec.class exists, unless I'm missing your point.

bowei · 2020-03-18T19:28:23Z

api/v1alpha1/gateway_types.go

+	// ConditionGatewayClassExists indicates that the specified GatewayClass exists.
+	ConditionGatewayClassExists GatewayConditionType = "GatewayClassExists"
+	// ConditionGatewayScheduled indicates that the Gateway has been scheduled.
+	ConditionGatewayScheduled GatewayConditionType = "GatewayScheduled"


You have to be more specific here. "Scheduled" means what for the end user?
For stuff that is informational, e.g. automation cannot act on it, has no semantic API meaning, it's worth thinking about whether it requires its own constant or just put the information into the human readable form.

You have to be more specific here. "Scheduled" means what for the end user?

@bowei I tried to make minimal changes to the docs to support the intent of this PR, which is to reverse the polarity of the existing conditions. This was an existing condition. Do you prefer I make changes to the previously created condition types in this PR or have this PR strictly focus on reversing the polarity?

bowei · 2020-03-18T19:28:36Z

api/v1alpha1/gateway_types.go

+	// are valid. If this condition has a status of False, a more detailed
+	// ListenerCondition should be present in the corresponding ListenerStatus.
+	ConditionListenersValid GatewayConditionType = "ListenersValid"
+	// ConditionRoutesReady indicates that all of the specified routes are ready.


Can we combine routes ready with routes valid? (can you give a good example why we want to distinguish these two states)

@bowei this comes from https://kubernetes.slack.com/archives/CR0H13KGA/p1584484692070800. A valid route means the config has passed validation. A ready route means some type of readiness probe has passed. I'm not opposed to sticking with valid listener/route now to simplify the conditions until a concrete use case is presented for adding a route ready state status condition. cc: @robscott

My comments here are mostly around making the comments more descriptive. (e.g. can you take your responses and put them in the comments, so when people read it, they know exactly what the conditions mean?)

bowei · 2020-03-18T19:30:09Z

api/v1alpha1/gateway_types.go

+	// ConditionListenerReady indicates the listener is ready to receive traffic.
+	ConditionListenerReady ListenerConditionType = "ListenerReady"
+	// ConditionValidAddress indicates the listener Address is valid.
+	ConditionValidAddress ListenerConditionType = "ValidAddress"


Can we clarify that these will always overlap:

Valid implies Ready?
Or Valid && Ready == can serve traffic?

danehans · 2020-04-23T01:34:11Z

/hold until kubernetes/community#4521 merges.

k8s-ci-robot · 2020-04-23T23:30:05Z

@danehans: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-service-apis-verify	`446d0a2`	link	`/test pull-service-apis-verify`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

danehans · 2020-04-30T18:19:45Z

@robscott and @bowei PTAL at these comments from @jpeach:

I’d guess that the condition type proposed in #1624 is likely to land, and it seems reasonable to just use it directly if we can.

Should I update this PR and pursue reversing polarity of the conditions?

k8s-ci-robot · 2020-04-30T18:19:52Z

@danehans: 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.

jpeach · 2020-04-30T22:54:06Z

@robscott and @bowei PTAL at these comments from @jpeach:

I’d guess that the condition type proposed in #1624 is likely to land, and it seems reasonable to just use it directly if we can.

Should I update this PR and pursue reversing polarity of the conditions?

I'd suggest we sit tight for a bit longer and wait for the converged condition type in kubernetes/enhancements#1624 to land. That doesn't address polarity, which I don't think will be resolved in the near term.

Practically, the only general purpose package I know of to expose status through Conditions is status, https://github.com/kubernetes-sigs/cli-utils/tree/master/pkg/kstatus

I'd like it if we followed their recommendations, so our conditions work out of the box.

fejta-bot · 2020-07-29T23:34:08Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

robscott · 2020-08-13T22:27:46Z

/cc @jpeach

k8s-ci-robot · 2020-08-13T22:27:47Z

@robscott: GitHub didn't allow me to request PR reviews from the following users: jpeach.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @jpeach

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 · 2020-09-10T01:28:43Z

@jpeach @danehans @robscott Given the work underway on conflict resolution and status & condition cleanup, should we close this?

jpeach · 2020-09-10T01:34:15Z

On 10 Sep 2020, at 11:28 am, Harry ***@***.***> wrote: @jpeach @danehans @robscott Given the work underway on conflict resolution and status & condition cleanup, should we close this?

Yes, that’s fine with me.

robscott · 2020-09-10T04:05:42Z

👍 I'm fine with that as well. Feel free to reopen in the future if we want to revisit any part of this.

/close

k8s-ci-robot · 2020-09-10T04:05:49Z

@robscott: Closed this PR.

In response to this:

👍 I'm fine with that as well. Feel free to reopen in the future if we want to revisit any part of 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.

k8s-ci-robot assigned robscott Feb 17, 2020

k8s-ci-robot requested a review from bowei February 17, 2020 17:35

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 17, 2020

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 17, 2020

robscott reviewed Feb 18, 2020

View reviewed changes

Reverses gateway condition polarity

446d0a2

danehans force-pushed the gw_positive_conditions branch from d93ead8 to 446d0a2 Compare March 17, 2020 23:43

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 17, 2020

bowei reviewed Mar 18, 2020

View reviewed changes

danehans mentioned this pull request Mar 18, 2020

Update Condition guidance kubernetes/community#4521

Merged

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 23, 2020

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

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 29, 2020

k8s-ci-robot closed this Sep 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reverses Gateway Condition Polarity #104

Reverses Gateway Condition Polarity #104

danehans commented Feb 17, 2020

k8s-ci-robot commented Feb 17, 2020

k8s-ci-robot commented Feb 17, 2020

robscott left a comment

robscott Feb 18, 2020

robscott Feb 18, 2020

robscott Feb 18, 2020

robscott commented Mar 18, 2020

bowei Mar 18, 2020

danehans Mar 18, 2020

bowei Mar 18, 2020

danehans Mar 18, 2020

bowei Mar 18, 2020

danehans Mar 18, 2020

bowei Mar 19, 2020

bowei Mar 18, 2020

danehans commented Apr 23, 2020

k8s-ci-robot commented Apr 23, 2020

danehans commented Apr 30, 2020

k8s-ci-robot commented Apr 30, 2020

jpeach commented Apr 30, 2020

fejta-bot commented Jul 29, 2020

robscott commented Aug 13, 2020

k8s-ci-robot commented Aug 13, 2020

hbagdi commented Sep 10, 2020

jpeach commented Sep 10, 2020 via email

robscott commented Sep 10, 2020

k8s-ci-robot commented Sep 10, 2020

	// ConditionRoutesReady indicates that at least one of the specified routes
	// ConditionRoutesReady indicates that all of the specified routes

	// ConditionValidRoutes indicates that at least one of the specified
	// ConditionValidRoutes indicates that all of the specified

	// ConditionValidListeners indicates that at least one of the specified
	// ConditionValidListeners indicates that all of the specified

Reverses Gateway Condition Polarity #104

Reverses Gateway Condition Polarity #104

Conversation

danehans commented Feb 17, 2020

k8s-ci-robot commented Feb 17, 2020

k8s-ci-robot commented Feb 17, 2020

robscott left a 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

robscott commented Mar 18, 2020

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

danehans commented Apr 23, 2020

k8s-ci-robot commented Apr 23, 2020

danehans commented Apr 30, 2020

k8s-ci-robot commented Apr 30, 2020

jpeach commented Apr 30, 2020

fejta-bot commented Jul 29, 2020

robscott commented Aug 13, 2020

k8s-ci-robot commented Aug 13, 2020

hbagdi commented Sep 10, 2020

jpeach commented Sep 10, 2020 via email

robscott commented Sep 10, 2020

k8s-ci-robot commented Sep 10, 2020