Support eventual consistency in conformance tests #1080

nathancoleman · 2022-03-29T19:18:44Z

What type of PR is this?

/kind bug
(best effort ^, is there a better option?)

What this PR does / why we need it:
We discussed in the sig meeting recently that "accepted" (or "ready" in the future) can only really indicate that the controller has synced all necessary configuration and that routes will begin working "soon".

In order for the conformance tests to support this, they must accept that HTTP requests to new routes may fail for some period of time but should become eventually consistent.

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2022-03-29T19:18:52Z

Hi @nathancoleman. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

conformance/utils/http/http.go

evankanderson · 2022-03-29T21:59:29Z

Any chance that we could instead require a conservative Ready status for the HttpRoute, rather than expecting each client to build in eventual consistency handling?

I ask because making the Gateway API eventually consistent (rather than conservatively Ready) means that every development team writing an end-to-end test through the Gateway API will need to add this logic to their tests and potentially have to debug the "was it a Gateway-induced eventual consistency failure or an application-level failure" when their tests fail or time out. As an end-user developer writing tests, being able to distinguish between "the Gateway controller didn't finish" and "my application fails for 30s at startup" is pretty useful.

nathancoleman · 2022-03-29T22:06:29Z

Hi @evankanderson ! There was some discussion about adding a Ready status to the HTTPRoute, and it sounds like that will be coming in the near term; however, I believe even then, things are expected to be eventually consistent due to the way these underlying systems work.

The context from the discussion at yesterday's meeting might be helpful. The recording is up on YouTube here (linked to appropriate timestamp) and the meeting notes are available here.

skriss

FYI I've been able to run the conformance tests using this PR against Contour's implementation successfully, whereas the tests in master where flaky for us. Thanks!

howardjohn · 2022-03-31T16:24:18Z

conformance/utils/http/http.go

+		return true
+	}, maxConsistencyPeriodPerRequest, 1*time.Second, "error making request, never got expected status")
+
+	// Once we've made a successful request and gotten a response with the


Sorry to nitpick this a lot... a bit worried about the logic here. Typically what I have seen is that config changes will go from "all old config" to "mix of old and new" before arriving at "new config". So if we assert that once we see the 'new' state its immediately always 'new', we may see issues.

For example, if we have 2 pods implementing the gateway and 1 gets the new config first.

Here is how the Istio logic for this works: https://github.com/istio/istio/blob/30a866fdb5ad65eccde3dafe21d19f455addbd72/pkg/test/util/retry/retry.go#L182.

Basically we test that we get a success N times in a row -- but if we fail part way through the check we continue on retrying.

Also a bit of nitpick, but I am not sure we need the consistency_check_%d to show up in the test result? Doing the above would make that pretty hard to retain anyways

I agree with the logic suggested here by @howardjohn. Maybe a fairly low timeout like 30 seconds to get 3 consecutive successful requests would work?

👍 Not nitpicking IMO @howardjohn ! This makes a lot of sense and will help us avoid similar issues when we make the number of pods implementing the gateway configurable beyond 1 in the near future.

@howardjohn I've pushed some updates to handle intermittent success in the style of what you linked. Let me know what ya think!

You can see an example run w/ log output under Run tests here.

robscott

Thanks for the work on this @nathancoleman! Agree with John's suggestion but otherwise this LGTM.

robscott · 2022-04-01T05:41:33Z

conformance/utils/http/http.go

+		return true
+	}, maxConsistencyPeriodPerRequest, 1*time.Second, "error making request, never got expected status")
+
+	// Once we've made a successful request and gotten a response with the


I agree with the logic suggested here by @howardjohn. Maybe a fairly low timeout like 30 seconds to get 3 consecutive successful requests would work?

robscott · 2022-04-01T05:46:58Z

Any chance that we could instead require a conservative Ready status for the HttpRoute, rather than expecting each client to build in eventual consistency handling?

@evankanderson I really wish that was possible. That was my original intent here, but unfortunately I think it was a bit too optimistic. Many (maybe most) of the implementations of this API simply don't have a way to know when the config they've written has actually been picked up and implemented. If there are good ways around this, I'm very open to alternatives, because I agree that a reliable "ready" condition on Routes would be more helpful than a condition that indicated that config had been updated and should be ready to use soon.

robscott · 2022-04-01T05:47:06Z

/ok-to-test

evankanderson · 2022-04-01T13:52:14Z

Two thoughts:

Does it make sense to call this condition Accepted or Programmed rather than Ready to make the distinction clearer to users?
Conservative here could include adding a 30s delay after Accepted, assuming that's something like 2.5 stddev beyond the mean.

nathancoleman · 2022-04-01T20:13:54Z

@evankanderson @robscott I believe that future status additions for HTTPRoute -- "ready" vs. "accepted" vs. "programmed", etc., none of which exists today -- would be a good topic for the next kube-sig meeting on Monday, April 4.

Those evolutions in the Gateway API are only tangentially related to this PR. My goal with this change is just to unblock several implementations that are unable to run the conformance tests successfully today.

Would it make sense to you guys to move this discussion over to that venue? I think that would allow for more participation as well.

robscott

Thanks @nathancoleman! A few more tiny nits but otherwise this LGTM.

conformance/utils/http/http.go

robscott · 2022-04-05T17:43:16Z

Thanks @nathancoleman!

/lgtm
/approve

k8s-ci-robot · 2022-04-05T17:43:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nathancoleman, robscott, skriss

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

nathancoleman added 2 commits March 29, 2022 15:12

Support eventual consistency in request-response helper

2fb0f78

Use request-response helper for all tests

85d58a1

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 29, 2022

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 29, 2022

k8s-ci-robot requested review from robscott and youngnick March 29, 2022 19:18

howardjohn reviewed Mar 29, 2022

View reviewed changes

conformance/utils/http/http.go Outdated Show resolved Hide resolved

howardjohn reviewed Mar 29, 2022

View reviewed changes

conformance/utils/http/http.go Outdated Show resolved Hide resolved

nathancoleman commented Mar 29, 2022

View reviewed changes

conformance/utils/http/http.go Outdated Show resolved Hide resolved

nathancoleman changed the title ~~Support eventually consistency in conformance tests~~ Support eventual consistency in conformance tests Mar 29, 2022

nathancoleman added 2 commits March 29, 2022 16:26

Log expected vs. actual while waiting for consistency

824539b

Assert consistency once initial request succeeds

d01bd47

nathancoleman marked this pull request as ready for review March 29, 2022 21:50

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 29, 2022

k8s-ci-robot requested review from hbagdi and jpeach March 29, 2022 21:51

skriss mentioned this pull request Mar 30, 2022

run Gateway API conformance tests as part of CI projectcontour/contour#4429

Closed

skriss approved these changes Mar 31, 2022

View reviewed changes

howardjohn reviewed Mar 31, 2022

View reviewed changes

robscott reviewed Apr 1, 2022

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 1, 2022

nathancoleman mentioned this pull request Apr 1, 2022

Add conformance testing workflow for GitHub Actions hashicorp/consul-api-gateway#139

Merged

2 tasks

nathancoleman added 2 commits April 1, 2022 15:12

Allow for intermittent success prior to reaching consistency

41097d5

Log when we've reached the consistency threshold for a request

736fd46

evankanderson mentioned this pull request Apr 4, 2022

Update HTTPRoute and other status definitions and documentation #1077

Closed

robscott reviewed Apr 5, 2022

View reviewed changes

conformance/utils/http/http.go Outdated Show resolved Hide resolved

conformance/utils/http/http.go Outdated Show resolved Hide resolved

conformance/utils/http/http.go Show resolved Hide resolved

Improve constant names + add docstrings

7d3c918

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

k8s-ci-robot assigned robscott Apr 5, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 5, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 5, 2022

k8s-ci-robot merged commit df0cc90 into kubernetes-sigs:master Apr 5, 2022

nathancoleman deleted the eventually-consistent-conformance branch April 5, 2022 17:47

nathancoleman restored the eventually-consistent-conformance branch April 5, 2022 17:47

nathancoleman mentioned this pull request Apr 5, 2022

Stop using fork of gateway-api for conformance testing hashicorp/consul-api-gateway#141

Closed

2 tasks

nathancoleman deleted the eventually-consistent-conformance branch April 5, 2022 18:05

nathancoleman mentioned this pull request Apr 5, 2022

Add conformance tests for ReferencePolicy #1081

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support eventual consistency in conformance tests #1080

Support eventual consistency in conformance tests #1080

nathancoleman commented Mar 29, 2022

k8s-ci-robot commented Mar 29, 2022

evankanderson commented Mar 29, 2022

nathancoleman commented Mar 29, 2022 •

edited

Loading

skriss left a comment

howardjohn Mar 31, 2022

robscott Apr 1, 2022

nathancoleman Apr 1, 2022

nathancoleman Apr 1, 2022 •

edited

Loading

robscott left a comment

robscott Apr 1, 2022

robscott commented Apr 1, 2022 •

edited

Loading

robscott commented Apr 1, 2022

evankanderson commented Apr 1, 2022

nathancoleman commented Apr 1, 2022

robscott left a comment •

edited

Loading

robscott commented Apr 5, 2022

k8s-ci-robot commented Apr 5, 2022

Support eventual consistency in conformance tests #1080

Support eventual consistency in conformance tests #1080

Conversation

nathancoleman commented Mar 29, 2022

k8s-ci-robot commented Mar 29, 2022

evankanderson commented Mar 29, 2022

nathancoleman commented Mar 29, 2022 • edited Loading

skriss left a comment

Choose a reason for hiding this comment

howardjohn Mar 31, 2022

Choose a reason for hiding this comment

robscott Apr 1, 2022

Choose a reason for hiding this comment

nathancoleman Apr 1, 2022

Choose a reason for hiding this comment

nathancoleman Apr 1, 2022 • edited Loading

Choose a reason for hiding this comment

robscott left a comment

Choose a reason for hiding this comment

robscott Apr 1, 2022

Choose a reason for hiding this comment

robscott commented Apr 1, 2022 • edited Loading

robscott commented Apr 1, 2022

evankanderson commented Apr 1, 2022

nathancoleman commented Apr 1, 2022

robscott left a comment • edited Loading

Choose a reason for hiding this comment

robscott commented Apr 5, 2022

k8s-ci-robot commented Apr 5, 2022

nathancoleman commented Mar 29, 2022 •

edited

Loading

nathancoleman Apr 1, 2022 •

edited

Loading

robscott commented Apr 1, 2022 •

edited

Loading

robscott left a comment •

edited

Loading