You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Follow-up to #397, fixed by #400. That PR added set -e to every chainsaw script that chains 2+ metrics_test.py calls, and raised the per-check budget (export TIMEOUT=90) plus spec.timeouts.exec on the two progress-gated scripts in simple-nodewright.
Explicitly out of scope there, and still outstanding, is the per-test review of the other 8 chainsaw tests that call metrics_test.py. Quoting #400: "Whether any of them have the same progress-gated timing risk as simple-nodewright needs a per-test review of pod-startup timing, which is separate follow-up work, not blind timeout bumps."
Why this still matters
metrics_test.py waits TIMEOUT = float(os.environ.get('TIMEOUT', 30)) per check (k8s-tests/chainsaw/metrics_test.py:88). simple-nodewright is the only test in the repo that overrides it, so every other check across the suite still runs on the 30s default.
A check for a metric that only becomes true after a package advances a stage is really "wait up to 30s for the previous stage to finish". That duration is bounded by pod and container startup on the node, which is not bounded by 30s on a loaded runner. That is precisely the failure mode in #397.
Scope
107 metrics_test.py calls across 8 files, none of which set TIMEOUT:
The four tests on a 90s exec budget are the tightest: a single check that burns its full 30s leaves only 60s for the rest of its script block.
What "review" means here
Classify each check into one of three buckets, because only one of them is fixed by a larger timeout:
Terminal / latched. The gauge holds the asserted value once reached (for example a package's final stage). Safe on the 30s default. A bump costs nothing but failure latency.
Progress-gated. The assertion can only pass once a package finishes a prior stage, which is gated on pod and container startup. These carry the 30s cliff and want an explicit export TIMEOUT=<n> plus a spec.timeouts.exec that accommodates the worst case.
Genuinely transient. The assertion names a stage or state the package passes through. Per the metric-semantics note in Flaky e2e: simple-skyhook metrics checks time out on metrics_test.py 30s default #397: nodewright_package_stage_count is a point-in-time gauge republished only when ReportState() runs during a reconcile, and setAllMetrics zeroes all state and stage combinations first, so a stage transited between two reconciles may never be published at all. Raising TIMEOUT cannot fix these; they need rewriting against a latched value, or removing.
Bucket 3 is the reason this is a review rather than a blanket timeout bump.
Acceptance criteria
Every metrics_test.py check in the 8 files above classified as terminal, progress-gated, or transient
Progress-gated checks carry an explicit TIMEOUT and a spec.timeouts.exec that accommodates it
Any genuinely transient assertions rewritten against a latched metric, or removed
A decision recorded on per-test TIMEOUT overrides vs. exporting it once next to CHAINSAW_ARGS in operator/Makefile. The single export raises the floor everywhere at the cost of slowing down every genuine failure.
Summary
Follow-up to #397, fixed by #400. That PR added
set -eto every chainsaw script that chains 2+metrics_test.pycalls, and raised the per-check budget (export TIMEOUT=90) plusspec.timeouts.execon the two progress-gated scripts insimple-nodewright.Explicitly out of scope there, and still outstanding, is the per-test review of the other 8 chainsaw tests that call
metrics_test.py. Quoting #400: "Whether any of them have the same progress-gated timing risk assimple-nodewrightneeds a per-test review of pod-startup timing, which is separate follow-up work, not blind timeout bumps."Why this still matters
metrics_test.pywaitsTIMEOUT = float(os.environ.get('TIMEOUT', 30))per check (k8s-tests/chainsaw/metrics_test.py:88).simple-nodewrightis the only test in the repo that overrides it, so every other check across the suite still runs on the 30s default.A check for a metric that only becomes true after a package advances a stage is really "wait up to 30s for the previous stage to finish". That duration is bounded by pod and container startup on the node, which is not bounded by 30s on a loaded runner. That is precisely the failure mode in #397.
Scope
107
metrics_test.pycalls across 8 files, none of which setTIMEOUT:spec.timeouts.execdeployment-policy/legacy-compatibility/chainsaw-test.yamldeployment-policy/linear-strategy/chainsaw-test.yamldeployment-policy/multi-compartment/chainsaw-test.yamldeployment-policy/overlapping-selectors/chainsaw-test.yamlnodewright/delete-nodewright/chainsaw-test.yamlnodewright/failure-nodewright/chainsaw-test.yamlnodewright/strict-order/chainsaw-test.yamlnodewright/taint-scheduling/chainsaw-test.yamlThe four tests on a 90s
execbudget are the tightest: a single check that burns its full 30s leaves only 60s for the rest of its script block.What "review" means here
Classify each check into one of three buckets, because only one of them is fixed by a larger timeout:
export TIMEOUT=<n>plus aspec.timeouts.execthat accommodates the worst case.nodewright_package_stage_countis a point-in-time gauge republished only whenReportState()runs during a reconcile, andsetAllMetricszeroes all state and stage combinations first, so a stage transited between two reconciles may never be published at all. RaisingTIMEOUTcannot fix these; they need rewriting against a latched value, or removing.Bucket 3 is the reason this is a review rather than a blanket timeout bump.
Acceptance criteria
metrics_test.pycheck in the 8 files above classified as terminal, progress-gated, or transientTIMEOUTand aspec.timeouts.execthat accommodates itTIMEOUToverrides vs. exporting it once next toCHAINSAW_ARGSinoperator/Makefile. The single export raises the floor everywhere at the cost of slowing down every genuine failure.Repro
Same shrink-the-budget trick as #397, per pool: