Repository navigation
test(e2e): apply the health check event log BTP before its route - #10415
Conversation
BackendHealthCheckEventLog applied the HTTPRoute before the BackendTrafficPolicy that adds its health check. EG translated the route alone first, so Envoy received the route's cluster without a health check and then an update that added one. Envoy warms the updated cluster until it receives the cluster's endpoints, and as they had not changed, no new ClusterLoadAssignment arrived: the update waited for EDS until initial_fetch_timeout (15s), and only then did Envoy run the first check. Envoy flushes the logged event to stdout every 10s, and Loki ingests it after that, so on CI the test waited 16-29s for the first event, or failed after its 30s. Apply the BTP first, so that Envoy receives the cluster with its health check right away and checks it as soon as the endpoints arrive. Signed-off-by: Alexej Disterhoft <alexej@disterhoft.de>
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10415 +/- ##
==========================================
+ Coverage 82.12% 82.13% +0.01%
==========================================
Files 272 272
Lines 43156 43156
==========================================
+ Hits 35440 35446 +6
+ Misses 7715 7709 -6
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Does applying the BTP first actually close the race? The BTP and HTTPRoute arrive on separate informers, so the route can still be reconciled before the BTP is cached, and we're back to the 15s wait. |
not 100%, but on my local tests reordering was good enough for the test to no longer fail. Proper fix would probably need changes similar to #10405 |
What this PR does / why we need it:
TestE2E/BackendHealthCheckEventLog/health_check_events_appear_in_logsis flaky. On #10406, whose change is a provider unit test, it failed in 4 of the 7 e2e jobs that ran it: Loki showed no health check event for the whole 30s wait. The other 3 passed after 21.3s, 26.2s and 29.2s. On main's latest green run, 7 of the 8 e2e jobs needed 16–25s of the 30s.The test applies the health checked route and its BTP from one file, route first:
This PR applies the BTP first, so Envoy receives the cluster with its health check right away:
#10405 uses the same order for
BackendHealthCheckActiveGRPC.Evidence, from a kind cluster on main (bb149bb), running the e2e test 10 times per order, alternating:
init_fetch_timeoutcounter go up in 5 of 5 runs, and the first check ran 14.8–14.9s after the route and BTP were created.init_fetch_timeoutdid not change in 5 of 5 runs, and the first check ran as the route and BTP were created.Locally, all 20 of these runs passed the 30s budget. CI runners take longer, as the CI numbers above show, so there the route-first wait reaches the 30s.
Merge danger: a two-way door that changes only the order of one test manifest. The blast radius is this one e2e test.
Two observations outside this test:
queryLokicounts matches over the last 10 minutes. When the test reruns within 10 minutes, events from the earlier run can age out during the wait, so the count can stay at the baseline although new events arrive. That failed 9 of my first 20 local runs, in both orders. The runs above narrow the window to the run, which gives the baseline of 0 that CI has: CI runs the test once per cluster.Which issue(s) this PR fixes:
No issue filed. The test came in with #9651.
PR Checklist
git commit -s). See DCO: Sign your work./api), the API was discussed and agreed before the implementation. The API change can be in a separate PR, or in the same PR, but the API must be agreed before implementation. N/A if this PR does not contain API changes. (N/A: no API changes.)make generate gen-check,make lint, and the unit-test/coverage build pass. (Flaky e2e failures are not considered breakages, butgen-check,lint, and coverage MUST pass.) (make lintpasses locally. The PR changes only an e2e testdata file, sogen-checkand coverage are left to CI.)release-notes/current/<section>/<pr-number>-<slug>.md(seerelease-notes/current/README.mdfor sections and naming). N/A if this PR does not contain non-trivial changes. (N/A: test-only.)make gen-checkand committed the result if API/helm charts/modules changed. (N/A: nothing generated changed.)release-notes/current/breaking_changes/.