fix(test): wait for control-plane recovery before retrying deploys - #12608
Merged
Conversation
The corerp-cloud functional tests intermittently fail with "connection reset by peer" / EOF (#12297). Re-analyzing a recurrence showed the failure starts at the storage layer inside the kind cluster: UCP surfaced "etcdserver: request timed out" about 3.5 minutes before kube-apiserver, kube-controller-manager, and kube-scheduler restarted. etcd itself never restarted and no Radius pod restarted. The existing retry could not cover that. A fixed "2 retries, 30s apart" is roughly 65 seconds of budget, and once the kube-apiserver is down each retry fails instantly against a dead socket, so the budget is spent long before the control plane returns. Replace the fixed attempt count with a wall-clock retry budget (10 minutes, about 3x the observed outage) and gate each retry on control- plane readiness. The gate probes /readyz and then the aggregated /apis/api.ucp.dev/v1alpha3 path, because the kube-apiserver can report ready while the UCP APIService behind the aggregation layer still returns 503, and that aggregated path is what rad connects to. MaxRetries becomes an optional cap so existing WithRetry callers keep their limit while inheriting the budget and the readiness gate. Also correct the comments in deployexecutor.go: the rad deploy path connects directly to the kind-published apiserver port from the kubeconfig, not through a kubectl port-forward tunnel. Port-forwarding is used elsewhere in the suite (the gateway tests) and breaks with the same errors, but describing this failure as a port-forward problem points the fix at the wrong layer. Diagnosing the run was limited by what CI collects: there were no etcd logs, no events, and no Last State blocks explaining why the containers were killed. Add a shared collect-cluster-diagnostics.sh used by the cloud, noncloud, and long-running-azure workflows that captures the previous instance's logs for every kube-system container, cluster events sorted by time, pods as YAML (which preserves lastState.terminated .exitCode, unlike kubectl describe), node conditions, and verbose livez/readyz. Collection is best effort so a cluster already tearing down cannot fail the job. Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f742ea9-6aeb-46f1-9e91-5adfe24694c0 Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR targets the recurring corerp-cloud kind control-plane restart flake by (1) making rad deploy retries deadline-based with an explicit control-plane readiness gate, and (2) improving CI post-mortem diagnostics so the next recurrence has enough evidence to identify the underlying resource-pressure mechanism.
Changes:
- Reworked
DeployExecutorretry behavior to be wall-clock budget based (with an optional retry cap), and added a readiness gate that blocks retries until the apiserver + Radius aggregated API are serving again. - Added unit tests for the new retry/budget/readiness behavior and for the control-plane readiness probing.
- Replaced inline “pod details” diagnostics in multiple workflows with a shared cluster diagnostics script.
Review notes (requires follow-up in this PR):
- The new diagnostics script writes
${PREFIX}-pods.yamlvia thecapturewrapper, which prepends non-YAML headers and makes the file invalid YAML (breaking the intended “machine-readable pod state” artifact).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/step/deployexecutor.go | Deadline-based retry budget + optional retry cap; readiness-gated retries using K8s discovery REST client. |
| test/step/deployexecutor_test.go | Added cases validating budget behavior, uncapped retries bounded by budget, and readiness gating. |
| test/step/controlplane.go | New readiness polling helper probing /readyz and the Radius aggregated API discovery path. |
| test/step/controlplane_test.go | Unit tests for readiness probe behavior and polling timeout reporting. |
| .github/workflows/long-running-azure.yaml | Switched from inline pod snapshot to shared cluster diagnostics script. |
| .github/workflows/functional-test-noncloud.yaml | Switched from inline pod snapshot to shared cluster diagnostics script. |
| .github/workflows/functional-test-cloud.yaml | Switched from inline pod snapshot to shared cluster diagnostics script. |
| .github/scripts/collect-cluster-diagnostics.sh | New best-effort diagnostics collector (pods/events/nodes/health/kube-system logs). |
brooke-hamilton
marked this pull request as draft
August 4, 2026 15:53
brooke-hamilton
changed the title
Addresses review feedback and removes complexity that was not carrying its weight. Collapse MaxRetries into RetryBudget. Two knobs bounding the same thing was the main source of complexity in the retry loop: it needed a separate cap check, manual deadline arithmetic, and a retryEnabled helper. A single wall-clock budget expresses the same intent, so WithRetry now takes a budget instead of an attempt count and WithRetryBudget is gone. Both existing callers are updated to a 3 minute budget with the same 60s delay, preserving their previous ~2 attempt behavior. Note that leaving them on the old signature would have silently compiled WithRetry(2, ...) as a 2 nanosecond budget. Drop the /readyz probe from the readiness gate. A successful request to the aggregated /apis/api.ucp.dev/v1alpha3 path cannot happen unless the kube-apiserver and its aggregation layer are both serving, so the separate probe added a round trip and a second failure mode without adding signal. This removes checkControlPlaneReady entirely. Unexport WaitForReady, which is only ever populated by Execute, and trim comments that had grown longer than the code they describe. Review feedback: - pods.yaml was written through capture(), which prepends separator and command lines. That made the file invalid YAML and defeated its whole purpose of preserving lastState/exitCode for post-mortem parsing. It is now written with a raw redirect. Verified with yaml.safe_load against a live cluster. - The RetryBudget doc claimed to bound total retry time, but the budget only gates the pre-retry wait. Clarified the comment rather than bounding the deploy call, since killing a deployment that is making progress would turn a slow success into a failure. Net 108 lines removed, and the unit tests drop from 5.9s to 1.1s. Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f742ea9-6aeb-46f1-9e91-5adfe24694c0 Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (3)
test/step/controlplane_test.go:59
- This test appends to a shared slice from the httptest server goroutine and then reads it from the test goroutine without synchronization, which will trip the race detector. Use a channel (or a mutex) to capture the probed path safely.
var paths []string
client := newTestRESTClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
paths = append(paths, r.URL.Path)
w.WriteHeader(http.StatusOK)
}))
require.NoError(t, waitForControlPlaneReady(context.Background(), t, client))
// A single probe of the aggregated path is enough: it cannot succeed unless
// the kube-apiserver and its aggregation layer are both serving.
assert.Equal(t, []string{radiusAggregatedAPIPath}, paths)
test/step/deployexecutor_test.go:227
- Test name/comment suggest the retry waits through multiple readiness probes, but the stubbed waitForReady returns an error immediately, which makes executeWithRetry stop after the first failure. Either make the stub actually block/poll until ready, or rename the test to reflect that an error from waitForReady disables retries.
func Test_ExecuteWithRetry_WaitsForControlPlaneBeforeRetrying(t *testing.T) {
t.Parallel()
var calls atomic.Int32
var waits atomic.Int32
.github/scripts/collect-cluster-diagnostics.sh:83
- The pods YAML dump suppresses stderr to /dev/null, so if
kubectl get pods -A -o yamlfails for a reason not already captured by the earlier commands, the failure is silently lost and you just get a missing/empty YAML file. Keep the YAML file header-free, but append stderr (and the exit code) to the pod-states log for post-mortem debugging.
# Written raw, with no headers, so the file stays parseable by a YAML
# reader. Failures land in the pod-states log above, so a broken dump here
# only means an absent or empty file.
kubectl get pods -A -o yaml >"${OUTPUT_DIR}/${PREFIX}-pods.yaml" 2>/dev/null || true
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12608 +/- ##
=======================================
Coverage 54.20% 54.20%
=======================================
Files 770 770
Lines 51085 51085
=======================================
Hits 27690 27690
Misses 20789 20789
Partials 2606 2606 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Self-review found a regression risk in the retry budget that was worse than the flake it was meant to fix. The budget is per deploy step, and it applies to all 105 NewDeployExecutor call sites across the functional suites, not just the corerp-cloud tests in #12297. A control plane that never recovers makes every test pay the full budget. corerp-noncloud runs 52 tests at -parallel 10, so a broken cluster costs roughly six times the budget. At the previous 10 minute default that is about an hour of waiting, which blows both the job timeout and `go test -timeout` and replaces clear per-test failures with an opaque panic. It would also risk losing the diagnostics this same PR adds, which are collected after the tests run. No single constant fixes this, because the suites do not share a deadline: FUNCTIONALTEST_TIMEOUT is 15m for noncloud and 60m for cloud. A budget large enough to cover a multi-minute outage in the cloud suite would blow the noncloud deadline. Cap the budget at half the time the test binary has left instead. Each subsequent batch of parallel tests then gets half of what remains, so the deadline is approached but never reached, and no per-suite constant is needed. Lower the default from 10 to 5 minutes, which still covers the ~3.5 minute outage observed in CI. Verified empirically: under `go test -timeout 5m` a never-recovering control plane waits exactly 2m30s rather than the configured budget. Also drop the package-level poll interval variable in favor of passing the interval as a parameter, so no test mutates shared state. Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f742ea9-6aeb-46f1-9e91-5adfe24694c0 Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (2)
test/step/controlplane_test.go:59
- This test records request paths by appending to a slice from the httptest server handler goroutine, then reads it from the test goroutine. Under
-racethis is a data race. Use an atomic to record the (single) observed path and request count instead of mutating a shared slice.
var paths []string
client := newTestRESTClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
paths = append(paths, r.URL.Path)
w.WriteHeader(http.StatusOK)
}))
require.NoError(t, waitForControlPlaneReady(context.Background(), t, client, time.Millisecond))
// A single probe of the aggregated path is enough: it cannot succeed unless
// the kube-apiserver and its aggregation layer are both serving.
assert.Equal(t, []string{radiusAggregatedAPIPath}, paths)
.github/scripts/collect-cluster-diagnostics.sh:83
kubectl get pods -A -o yamlredirects stderr to/dev/null, so failures do not actually "land in the pod-states log" as the comment claims. Redirect stderr to the pod-states log instead so YAML stays valid but errors are still captured for post-mortem.
# Written raw, with no headers, so the file stays parseable by a YAML
# reader. Failures land in the pod-states log above, so a broken dump here
# only means an absent or empty file.
kubectl get pods -A -o yaml >"${OUTPUT_DIR}/${PREFIX}-pods.yaml" 2>/dev/null || true
The cloud workflow gave `go test` a 60m timeout inside a job whose timeout-minutes was also 60. Those cannot both be reached: the job additionally pays for about 10 minutes of setup before the tests start (KinD cluster, Radius install, Helm charts, Azure and AWS logins), so a hung suite hits the job timeout first. GitHub then cancels the job and the post-test steps never run - including the cluster diagnostics this PR adds, which is precisely the evidence needed to debug a hang. Lower FUNCTIONALTEST_TIMEOUT to 30m and add a 35m step-level timeout on the test step. A step timeout, unlike a job timeout, still allows the `if: always()` steps that follow to run, so diagnostics are collected either way. The ordering is now `go test` < step < job, matching the pattern already used in long-running-azure.yaml. The new values are generous. Measured over recent runs, the cloud test jobs complete in 14.5-16.6 minutes end to end, of which `go test` is about 5 minutes, so 30m leaves roughly a six-fold margin. The noncloud workflow already orders these correctly (15m `go test` inside a 90m job) and is left alone. Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f742ea9-6aeb-46f1-9e91-5adfe24694c0 Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (5)
test/step/deployexecutor_test.go:212
- Test name is misleading: this case asserts that a non-nil control-plane readiness error stops retries immediately (no retry is attempted), not that the executor waits until the control plane becomes ready. Rename the test to reflect the behavior being validated.
func Test_ExecuteWithRetry_WaitsForControlPlaneBeforeRetrying(t *testing.T) {
.github/workflows/functional-test-cloud.yaml:1067
- This diagnostics step runs on successful jobs too ("if: always()"), but the PR description scopes diagnostics to failures and the new script collects potentially large kube-system logs. Consider running it only when the job is failing to reduce runtime and artifact size on green runs.
if: always()
.github/workflows/functional-test-noncloud.yaml:714
- This diagnostics step runs on successful jobs too ("if: always()"), but the PR description scopes diagnostics to failures and the new script collects potentially large kube-system logs. Consider running it only when the job is failing to reduce runtime and artifact size on green runs.
if: always()
.github/workflows/long-running-azure.yaml:455
- This diagnostics step runs on successful jobs too ("if: always()"), but the PR description scopes diagnostics to failures and the new script collects potentially large kube-system logs. Consider running it only when the job is failing to reduce runtime and artifact size on green runs.
if: always()
test/step/controlplane_test.go:59
- This test appends to a shared slice from the httptest server handler goroutine and then reads it in the test goroutine. Under
-racethis can be reported as a data race because there’s no explicit synchronization. Use a channel (or a mutex) to record request paths safely.
var paths []string
client := newTestRESTClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
paths = append(paths, r.URL.Path)
w.WriteHeader(http.StatusOK)
}))
brooke-hamilton
marked this pull request as ready for review
August 4, 2026 18:38
brooke-hamilton
enabled auto-merge
August 4, 2026 18:39
Three small fixes from the Copilot review: Capture stderr from the pods YAML dump into the pod-states log instead of discarding it. The previous comment claimed failures landed there, but 2>/dev/null meant a failed dump produced a silently empty file. The YAML stays header-free and parseable either way. Guard the recorded request paths in the readiness gate test with a mutex. The slice was appended from the httptest handler goroutine and read from the test goroutine; the single request in this test gives it a happens-before edge today, but the pattern is a hazard if the test ever probes more than once. Rename Test_ExecuteWithRetry_WaitsForControlPlaneBeforeRetrying to Test_ExecuteWithRetry_UnreadyControlPlaneStopsRetrying. The stub returned an error on its first call, so the executor stopped immediately and never waited through repeated probes as the old name and comment implied. The stub's unreachable branch is dropped and the surfaced error is now asserted. Also normalize controlplane.go and controlplane_test.go to LF in the working tree. Git already stored them as LF, so there is no content change, but the CRLF copies made local gofmt report them as unformatted. Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f742ea9-6aeb-46f1-9e91-5adfe24694c0 Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
test/step/deployexecutor.go:322
- When RetryDelay is 0 and waitForReady is nil (e.g., no K8s client), this returns nil without checking ctx. That means the retry loop can keep starting new deploy attempts even after retryCtx’s deadline (RetryBudget) has elapsed, potentially spinning until the parent ctx/test timeout. Return ctx.Err() here so the budget/context always gates retries, even in the delay-only fast-path.
if d.waitForReady == nil {
return nil
}
waitBeforeRetry returned a bare nil when there was no readiness gate. If RetryDelay was also zero, nothing on that path ever consulted the context, so executeWithRetry kept starting new attempts after the retry budget had elapsed and only stopped at the parent context or the test binary's timeout. That defeats the guarantee effectiveRetryBudget exists to provide. Return ctx.Err() instead. It is nil while budget remains, so the normal path is unchanged, and it surfaces the deadline once the budget is spent. The combination is reachable through the public API: RetryDelay is an exported field and WithRetry takes a delay, so zero is a legal value, and waitForReady is nil whenever no Kubernetes client is available. Adds a regression test. Without the fix it hangs until the test timeout panics; with it, the loop stops at the budget. Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f742ea9-6aeb-46f1-9e91-5adfe24694c0 Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/scripts/collect-cluster-diagnostics.sh:166
- Argument parsing assumes
-o/--output-dirand-p/--prefixalways have a following value; if a caller accidentally supplies the flag as the final arg,shift 2will fail underset -ewith a generic "shift count out of range". Add an explicit[[ $# -ge 2 ]]check for these options and return a clear error message.
while [[ $# -gt 0 ]]; do
case $1 in
-o | --output-dir)
OUTPUT_DIR="$2"
shift 2
;;
-p | --prefix)
PREFIX="$2"
shift 2
;;
PR #12618 landed on main while this branch was open. It targets the same flake (#12297) in the same file with a different approach, so the two had to be reconciled rather than one side simply taken. Kept from #12618: the error classification. It adds IsTransientAPIServerRestartError, which recognizes the second phase of a control-plane restart - the apiserver accepting connections again but answering 503 "before all known HTTP paths have been installed", and 403 "cannot get path /apis/api.ucp.dev" while RBAC reconciles. This branch had no equivalent, and without it the readiness gate here is unreachable in that phase: ShouldRetry returns false for those errors, so the retry loop is never entered at all. Its two tests are kept for the same reason. Kept from this branch: the retry mechanics. RetryBudget bounds retrying by wall-clock time and is capped by the test binary's deadline, and the readiness gate waits on the aggregated API rather than sleeping blindly. Dropped from #12618: MaxRetries (raised there 2 -> 3), retryDelayForAttempt and maxTransientRetryDelay, plus Test_RetryDelayForAttempt. Its exponential backoff exists to widen the recovery window from ~66s to 3m30s, which the budget already does - and does so bounded by the test deadline, which a fixed schedule cannot be. Keeping both would reinstate two mechanisms bounding the same thing, the complexity this branch set out to remove. Also adopt t.Context() in the tests added here. The usetesting linter is enabled with context-background: true, so the context.Background() calls this branch introduced would now fail lint. Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4f742ea9-6aeb-46f1-9e91-5adfe24694c0 Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Note
This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com>
brooke-hamilton
changed the title
Radius functional test overviewClick here to see the test run details
Test Status⌛ Building Radius and pushing container images for functional tests... |
sk593
approved these changes
Aug 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes functional-test deployment retries deadline-based and waits for the Radius aggregated API to recover before starting another attempt. It also ensures cloud test timeouts leave time for diagnostics and applies the shared diagnostics collector to the long-running workflow.
Current evidence
The problem still exists after #12618 added restart-response classification/backoff and #12627 reduced
corerp-cloudparallelism from 10 to 4.Run 31804889581, on
mainafter both fixes merged, failed threecorerp-cloudtests together on August 14. Each sawconnection reset by peerfollowed byEOF, then exhausted the four-attempt 30s/60s/120s schedule. The downloaded diagnostics show etcd requests slowing and timing out, followed by restarts of kube-apiserver, kube-controller-manager, and kube-scheduler. The apiserver restarted about four minutes into the affected tests, after the fixed attempt schedule had already been consumed.This establishes that reducing parallelism lowered the frequency but did not remove the control-plane outage, and that the remaining failure is still the one this PR targets.
Relationship to changes already on main
IsTransientAPIServerRestartErrorand its tests. The readiness gate is unreachable unless restart-phase 503/403 responses remain retryable.-parallel 4and the sharedkind export logsdiagnostics script. This PR no longer replaces or duplicates that script./apis/api.ucp.dev/v1alpha3. Attempts are no longer spent against an unavailable apiserver.Additional changes after reassessment
IsTransientDeployErrorwith Azure-specific retry markers. Previously those two callers replaced the default predicate and would still fail on the exact connection-reset/EOF scenario.go testtimeout inside a 35-minute step timeout and 60-minute job timeout, preserving time for post-test diagnostics and artifact upload.Validation
go test -race ./test/step/... -count=1go test ./test/functional-portable/corerp/cloud/resources -run 'Test_isTransientCloudDeployError$' -count=1The local functional harness could not complete a representative deployment because current
mainrejects Radius.Core deployments in the credential-free local environment without an Azure provider, while a legacy fallback test fails against the currently published Bicep types. Those failures occur before retry execution and are unrelated to this patch; the new PR-head GitHub functional runs are the authoritative end-to-end validation.