Fix flaky TestMinScaleAnnotationChange by waiting for scale convergence#16668
Fix flaky TestMinScaleAnnotationChange by waiting for scale convergence#16668brucearctor wants to merge 1 commit into
Conversation
The test calls ensureDesiredScale (which asserts stability for 30s) immediately after the Configuration reports Ready. However, the KPA reconciles scale asynchronously — the deployment may not have reached the target replica count yet when Ready is reported. Add a waitForDesiredScale call before ensureDesiredScale to first wait for the scale to converge (up to 3 min), then verify it holds steady. This matches the pattern already used in the for-loop body on line 443. This test has been auto-reported as flaky twice (knative#16527, knative#16653) and auto-closed both times without a real fix. Fixes: knative#16653
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: brucearctor The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @brucearctor! It looks like this is your first PR to knative/serving 🎉 |
|
Hi @brucearctor. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
|
/ok-to-test |
|
@linkvt -- the build job in the kind-e2e workflow is failing across PRs (e.g. #16668, #16655) with Get "https://registry.local:5000/v2/": http: server gave HTTP response to HTTPS client. Looks like ko is trying HTTPS despite the *.local insecure override in go-containerregistry. Might be a ko version / registry:3.0.0 interaction. Happy to dig in if helpful. |
|
Sure! It just seems like your change shouldn't have caused this and other recent PRs dont fail, I thought we usually pin all versions. /retest |
|
Thanks for the retest @linkvt! Same failure on retry — looks persistent, not a flake. I dug in a bit:
This is probably a |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #16668 +/- ##
=======================================
Coverage 80.29% 80.29%
=======================================
Files 217 217
Lines 13568 13568
=======================================
Hits 10895 10895
Misses 2308 2308
Partials 365 365 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@brucearctor I created PR #16669 to work around the issue, CI passes and the PR now waits for a maintainer review. |
What
Fix the recurring flaky
TestMinScaleAnnotationChangee2e test by adding awaitForDesiredScalecall beforeensureDesiredScale.Why
The test calls
ensureDesiredScale(which asserts scale stability for 30s) immediately after the Configuration reportsReady. However, the KPA reconciles scale asynchronously — the deployment may not have reached the target replica count yet whenReadyis reported. If the scale is briefly at 0 during KPA convergence,ensureDesiredScalecatches that transient state and hard-fails.This has been auto-reported as flaky twice and auto-closed both times without a real fix:
Fix
Add
waitForDesiredScale(polls up to 3 min for convergence) beforeensureDesiredScale(asserts stability for 30s). This matches the pattern already used in the annotation-change loop body (line 443), which does not exhibit this flake.Other call sites
Lines 134, 203, 308, and 341 in the same file have the same
ensureDesiredScale-without-convergence-wait pattern. These could also be flaky but are not currently failing. Happy to fix those in a follow-up PR if maintainers agree./kind bug