From c9427d9cd8a411d97f234aae8291a21eba026750 Mon Sep 17 00:00:00 2001 From: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com> Date: Tue, 4 Aug 2026 11:42:31 -0400 Subject: [PATCH 1/7] Make deploy retries deadline-based and capture control-plane diagnostics 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> --- .../scripts/collect-cluster-diagnostics.sh | 171 ++++++++++++++++ .github/workflows/functional-test-cloud.yaml | 21 +- .../workflows/functional-test-noncloud.yaml | 21 +- .github/workflows/long-running-azure.yaml | 21 +- test/step/controlplane.go | 122 +++++++++++ test/step/controlplane_test.go | 126 ++++++++++++ test/step/deployexecutor.go | 189 +++++++++++++----- test/step/deployexecutor_test.go | 131 +++++++++++- 8 files changed, 713 insertions(+), 89 deletions(-) create mode 100755 .github/scripts/collect-cluster-diagnostics.sh create mode 100644 test/step/controlplane.go create mode 100644 test/step/controlplane_test.go diff --git a/.github/scripts/collect-cluster-diagnostics.sh b/.github/scripts/collect-cluster-diagnostics.sh new file mode 100755 index 00000000000..29617100599 --- /dev/null +++ b/.github/scripts/collect-cluster-diagnostics.sh @@ -0,0 +1,171 @@ +#!/bin/bash + +# ============================================================================ +# Collect Kubernetes cluster diagnostics for functional test post-mortems. +# +# Functional tests intermittently fail with `connection reset by peer` or `EOF` +# against the Radius API. Diagnosing those runs requires distinguishing a +# control-plane outage (etcd slowness, a kube-apiserver liveness kill) from a +# Radius bug, and the pod snapshot alone cannot do that. This script captures +# the missing evidence: +# +# * pod state as YAML, which preserves `lastState.terminated.reason` and +# `exitCode` for containers that already restarted (`kubectl describe` has +# been observed to omit the `Last State` block entirely) +# * cluster-wide events, which record liveness/readiness probe failures and +# the resulting kills +# * node conditions, which record disk/memory/PID pressure +# * the current and previous logs of every kube-system container, including +# etcd - the previous instance's log is where an apiserver crash reason and +# etcd's `apply request took too long` / `slow fdatasync` warnings live +# * verbose /livez and /readyz output, which names the specific failing +# apiserver health check +# +# Collection is best effort by design: it runs after a test job that may have +# already started tearing the cluster down, so an individual kubectl failure is +# recorded in the output and never aborts the rest of the collection. +# ============================================================================ + +set -euo pipefail + +readonly SCRIPT_NAME="$(basename "$0")" + +OUTPUT_DIR="" +PREFIX="cluster" + +usage() { + echo "Usage: ${SCRIPT_NAME} --output-dir DIR [--prefix NAME]" + echo "Options:" + echo " -o, --output-dir Directory to write diagnostics into (required)" + echo " -p, --prefix Filename prefix for the output files" + echo " (default: cluster)" + echo " -h, --help Show this help" + exit 0 +} + +validate_requirements() { + if [[ -z "${OUTPUT_DIR}" ]]; then + echo "Error: --output-dir is required" >&2 + exit 1 + fi + + if ! command -v kubectl >/dev/null; then + echo "Error: kubectl is required but not installed" >&2 + exit 1 + fi +} + +# capture appends the command line and its combined output to a file. A failing +# command is recorded rather than propagated so one unavailable resource does +# not stop the remaining collection. +capture() { + local output_file="$1" + shift + + { + echo "==============================================================" + echo "\$ $*" + echo "==============================================================" + "$@" 2>&1 || echo "(command exited with code $?)" + echo + } >>"${output_file}" +} + +collect_pod_state() { + # The plain-text snapshot keeps the historical pod-states file intact, and + # the YAML dump adds the machine-readable lastState/exitCode fields. + capture "${OUTPUT_DIR}/${PREFIX}-pod-states.log" kubectl get pods -A -o wide + capture "${OUTPUT_DIR}/${PREFIX}-pod-states.log" kubectl describe pods -A + capture "${OUTPUT_DIR}/${PREFIX}-pods.yaml" kubectl get pods -A -o yaml +} + +collect_events() { + capture "${OUTPUT_DIR}/${PREFIX}-events.log" \ + kubectl get events -A --sort-by=.lastTimestamp +} + +collect_node_state() { + capture "${OUTPUT_DIR}/${PREFIX}-nodes.log" kubectl get nodes -o wide + capture "${OUTPUT_DIR}/${PREFIX}-nodes.log" kubectl describe nodes +} + +collect_control_plane_health() { + local output_file="${OUTPUT_DIR}/${PREFIX}-control-plane-health.log" + + capture "${output_file}" kubectl get --raw "/livez?verbose" + capture "${output_file}" kubectl get --raw "/readyz?verbose" + capture "${output_file}" kubectl get apiservices -o wide +} + +# collect_kube_system_logs writes the current and previous logs of every +# kube-system container. Timestamps are included so the logs can be correlated +# with Radius pod logs and with the test output. +collect_kube_system_logs() { + local logs_dir="${OUTPUT_DIR}/${PREFIX}-kube-system-logs" + mkdir -p "${logs_dir}" + + local pods + if ! pods="$(kubectl get pods -n kube-system \ + -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}')"; then + echo "Warning: unable to list kube-system pods" >&2 + return 0 + fi + + local pod container containers previous_file + for pod in ${pods}; do + if ! containers="$(kubectl get pod "${pod}" -n kube-system \ + -o jsonpath='{range .spec.initContainers[*]}{.name}{"\n"}{end}{range .spec.containers[*]}{.name}{"\n"}{end}')"; then + continue + fi + + for container in ${containers}; do + kubectl logs "${pod}" -n kube-system -c "${container}" \ + --timestamps >"${logs_dir}/${pod}.${container}.log" 2>&1 || true + + # A previous instance only exists once a container has restarted, + # and it holds the reason it died. Drop the file when there is no + # previous instance so the artifact only contains real restarts. + previous_file="${logs_dir}/${pod}.${container}.previous.log" + if ! kubectl logs "${pod}" -n kube-system -c "${container}" \ + --previous --timestamps >"${previous_file}" 2>&1; then + rm -f "${previous_file}" + fi + done + done +} + +main() { + validate_requirements + + mkdir -p "${OUTPUT_DIR}" + + echo "Collecting cluster diagnostics into ${OUTPUT_DIR}" + collect_pod_state + collect_events + collect_node_state + collect_control_plane_health + collect_kube_system_logs + echo "Cluster diagnostics collection complete" +} + +while [[ $# -gt 0 ]]; do + case $1 in + -o | --output-dir) + OUTPUT_DIR="$2" + shift 2 + ;; + -p | --prefix) + PREFIX="$2" + shift 2 + ;; + -h | --help) + usage + ;; + *) + echo "Unknown option: $1" >&2 + exit 1 + ;; + esac +done + +main "$@" diff --git a/.github/workflows/functional-test-cloud.yaml b/.github/workflows/functional-test-cloud.yaml index ab746e4e42d..1076deefdf2 100644 --- a/.github/workflows/functional-test-cloud.yaml +++ b/.github/workflows/functional-test-cloud.yaml @@ -1049,23 +1049,20 @@ jobs: result_directory: dist/functional_test/ comment_mode: failures - - name: Collect Pod details + - name: Collect cluster diagnostics if: always() - # Diagnostic-only snapshot of cluster-wide pod state for post-mortem. A - # `kubectl describe pods -A` can race pod teardown and return a transient - # NotFound, so never let it fail the job. + # Diagnostic-only snapshot for post-mortem: pod state (including the + # previous container instance's termination reason), events, node + # conditions, control-plane health, and kube-system container logs. + # These commands can race cluster teardown and return a transient + # NotFound, so never let them fail the job. continue-on-error: true env: MATRIX_NAME: ${{ matrix.name }} run: | - POD_STATE_LOG_FILENAME="${RADIUS_CONTAINER_LOG_BASE}/${MATRIX_NAME}-tests-pod-states.log" - mkdir -p "$(dirname "${POD_STATE_LOG_FILENAME}")" - { - echo "kubectl get pods -A" - kubectl get pods -A - echo "kubectl describe pods -A" - kubectl describe pods -A - } >> "${POD_STATE_LOG_FILENAME}" + ./.github/scripts/collect-cluster-diagnostics.sh \ + --output-dir "${RADIUS_CONTAINER_LOG_BASE}" \ + --prefix "${MATRIX_NAME}-tests" - name: Upload container logs if: always() diff --git a/.github/workflows/functional-test-noncloud.yaml b/.github/workflows/functional-test-noncloud.yaml index 90b02c93633..4d7fd95ef6b 100644 --- a/.github/workflows/functional-test-noncloud.yaml +++ b/.github/workflows/functional-test-noncloud.yaml @@ -710,23 +710,20 @@ jobs: retention-days: 30 if-no-files-found: error - - name: Collect Pod details + - name: Collect cluster diagnostics if: always() - # Diagnostic-only snapshot of cluster-wide pod state for post-mortem. A - # `kubectl describe pods -A` can race pod teardown and return a transient - # NotFound, so never let it fail the job. + # Diagnostic-only snapshot for post-mortem: pod state (including the + # previous container instance's termination reason), events, node + # conditions, control-plane health, and kube-system container logs. + # These commands can race cluster teardown and return a transient + # NotFound, so never let them fail the job. continue-on-error: true env: MATRIX_NAME: ${{ matrix.name }} run: | - POD_STATE_LOG_FILENAME="${RADIUS_CONTAINER_LOG_BASE}/${MATRIX_NAME}-tests-pod-states.log" - mkdir -p "$(dirname "$POD_STATE_LOG_FILENAME")" - { - echo "kubectl get pods -A" - kubectl get pods -A - echo "kubectl describe pods -A" - kubectl describe pods -A - } >> "${POD_STATE_LOG_FILENAME}" + ./.github/scripts/collect-cluster-diagnostics.sh \ + --output-dir "${RADIUS_CONTAINER_LOG_BASE}" \ + --prefix "${MATRIX_NAME}-tests" - name: Upload container logs if: always() diff --git a/.github/workflows/long-running-azure.yaml b/.github/workflows/long-running-azure.yaml index b73a61c6f5a..677ee3c6cc8 100644 --- a/.github/workflows/long-running-azure.yaml +++ b/.github/workflows/long-running-azure.yaml @@ -451,21 +451,18 @@ jobs: make test-functional-all-cloud make test-functional-all-noncloud - - name: Collect Pod details + - name: Collect cluster diagnostics if: always() - # Diagnostic-only snapshot of cluster-wide pod state for post-mortem. A - # `kubectl describe pods -A` can race pod teardown and return a transient - # NotFound, so never let it fail the job. + # Diagnostic-only snapshot for post-mortem: pod state (including the + # previous container instance's termination reason), events, node + # conditions, control-plane health, and kube-system container logs. + # These commands can race cluster teardown and return a transient + # NotFound, so never let them fail the job. continue-on-error: true run: | - POD_STATE_LOG_FILENAME="${RADIUS_CONTAINER_LOG_BASE}/all-tests-pod-states.log" - mkdir -p $(dirname $POD_STATE_LOG_FILENAME) - { - echo "kubectl get pods -A" - kubectl get pods -A - echo "kubectl describe pods -A" - kubectl describe pods -A - } >> "${POD_STATE_LOG_FILENAME}" + ./.github/scripts/collect-cluster-diagnostics.sh \ + --output-dir "${RADIUS_CONTAINER_LOG_BASE}" \ + --prefix "all-tests" - name: Upload container logs if: always() diff --git a/test/step/controlplane.go b/test/step/controlplane.go new file mode 100644 index 00000000000..e560549481c --- /dev/null +++ b/test/step/controlplane.go @@ -0,0 +1,122 @@ +/* +Copyright 2023 The Radius Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package step + +import ( + "context" + "fmt" + "testing" + "time" + + k8s "k8s.io/client-go/kubernetes" + "k8s.io/client-go/rest" +) + +const ( + // controlPlaneReadyPollInterval is how long to wait between readiness + // probes while the control plane is recovering. + controlPlaneReadyPollInterval = 5 * time.Second + + // controlPlaneProbeTimeout bounds a single readiness probe so a hung + // kube-apiserver cannot consume the caller's entire retry budget in one + // request. + controlPlaneProbeTimeout = 10 * time.Second + + // radiusAggregatedAPIPath is the aggregated API discovery path that rad + // itself requests when it opens a workspace connection (see + // pkg/cli/workspaces.Connection). Probing the same path exercises the whole + // chain the deployment depends on: the kube-apiserver, its aggregation + // layer, and the UCP APIService behind it. + radiusAggregatedAPIPath = "/apis/api.ucp.dev/v1alpha3" +) + +// newControlPlaneReadyWaiter returns a function that blocks until the +// Kubernetes control plane and the Radius aggregated API are both serving +// again, or until ctx is done. +// +// It exists because a fixed sleep is not a useful gate between deployment +// retries. When the kind control plane goes down, every queued retry fails +// instantly against a dead socket and burns the retry budget in seconds while +// the outage lasts minutes. Waiting for readiness instead makes each retry +// attempt meaningful. +// +// It returns nil when no client is available, which disables the gate and +// leaves the retry loop with its delay-only behavior. +func newControlPlaneReadyWaiter(t *testing.T, client k8s.Interface) func(context.Context) error { + if client == nil { + return nil + } + + restClient := client.Discovery().RESTClient() + if restClient == nil { + return nil + } + + return func(ctx context.Context) error { + return waitForControlPlaneReady(ctx, t, restClient) + } +} + +// waitForControlPlaneReady polls until the control plane is ready or ctx is +// done. The error returned on timeout includes the last probe failure so the +// test log records which component was still unavailable. +func waitForControlPlaneReady(ctx context.Context, t *testing.T, client rest.Interface) error { + var lastErr error + for { + lastErr = checkControlPlaneReady(ctx, client) + if lastErr == nil { + return nil + } + + t.Logf("waiting for the Radius control plane to become ready: %v", lastErr) + + timer := time.NewTimer(controlPlaneReadyPollInterval) + select { + case <-timer.C: + case <-ctx.Done(): + timer.Stop() + return fmt.Errorf("control plane did not become ready: %w (last probe failure: %v)", ctx.Err(), lastErr) + } + } +} + +// checkControlPlaneReady performs a single readiness probe. +func checkControlPlaneReady(ctx context.Context, client rest.Interface) error { + // /readyz reports whether the kube-apiserver has finished starting and its + // health checks - including the etcd backend check - are passing. It is + // served to any authenticated caller by the built-in + // system:public-info-viewer role, so it needs no extra RBAC. + if err := probe(ctx, client, "/readyz"); err != nil { + return fmt.Errorf("kube-apiserver is not ready: %w", err) + } + + // The kube-apiserver can be ready before the aggregated API it proxies to + // is, and a request to an unavailable APIService fails with 503. Probing it + // avoids retrying a deployment that is guaranteed to fail. + if err := probe(ctx, client, radiusAggregatedAPIPath); err != nil { + return fmt.Errorf("the Radius aggregated API is not reachable: %w", err) + } + + return nil +} + +func probe(ctx context.Context, client rest.Interface, path string) error { + probeCtx, cancel := context.WithTimeout(ctx, controlPlaneProbeTimeout) + defer cancel() + + return client.Get().AbsPath(path).Do(probeCtx).Error() +} diff --git a/test/step/controlplane_test.go b/test/step/controlplane_test.go new file mode 100644 index 00000000000..a82103173f9 --- /dev/null +++ b/test/step/controlplane_test.go @@ -0,0 +1,126 @@ +/* +Copyright 2023 The Radius Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package step + +import ( + "context" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/rest" +) + +// newTestRESTClient returns a REST client pointed at a test HTTP server that +// serves the given handler. +func newTestRESTClient(t *testing.T, handler http.Handler) rest.Interface { + t.Helper() + + server := httptest.NewServer(handler) + t.Cleanup(server.Close) + + client, err := kubernetes.NewForConfig(&rest.Config{Host: server.URL}) + require.NoError(t, err) + + return client.Discovery().RESTClient() +} + +func Test_CheckControlPlaneReady_ReadyWhenBothProbesSucceed(t *testing.T) { + t.Parallel() + 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, checkControlPlaneReady(context.Background(), client)) + assert.Equal(t, []string{"/readyz", radiusAggregatedAPIPath}, paths) +} + +func Test_CheckControlPlaneReady_NotReadyWhenAPIServerIsUnhealthy(t *testing.T) { + t.Parallel() + client := newTestRESTClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + + err := checkControlPlaneReady(context.Background(), client) + require.Error(t, err) + assert.Contains(t, err.Error(), "kube-apiserver is not ready") +} + +func Test_CheckControlPlaneReady_NotReadyWhenAggregatedAPIIsUnavailable(t *testing.T) { + t.Parallel() + // The kube-apiserver can be healthy while the UCP APIService behind the + // aggregation layer is still unavailable, which the apiserver reports as a + // 503. Retrying a deployment in that window cannot succeed. + client := newTestRESTClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == radiusAggregatedAPIPath { + w.WriteHeader(http.StatusServiceUnavailable) + return + } + w.WriteHeader(http.StatusOK) + })) + + err := checkControlPlaneReady(context.Background(), client) + require.Error(t, err) + assert.Contains(t, err.Error(), "the Radius aggregated API is not reachable") +} + +func Test_WaitForControlPlaneReady_ReturnsOnceHealthy(t *testing.T) { + t.Parallel() + var requests atomic.Int32 + client := newTestRESTClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Fail the first /readyz probe so the wait loop has to poll at least + // twice before succeeding. + if r.URL.Path == "/readyz" && requests.Add(1) == 1 { + w.WriteHeader(http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusOK) + })) + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + require.NoError(t, waitForControlPlaneReady(ctx, t, client)) + assert.Greater(t, requests.Load(), int32(1)) +} + +func Test_WaitForControlPlaneReady_ReportsLastProbeFailureOnTimeout(t *testing.T) { + t.Parallel() + client := newTestRESTClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + + err := waitForControlPlaneReady(ctx, t, client) + require.Error(t, err) + assert.ErrorIs(t, err, context.DeadlineExceeded) + assert.Contains(t, err.Error(), "kube-apiserver is not ready") +} + +func Test_NewControlPlaneReadyWaiter_NilClientDisablesGate(t *testing.T) { + t.Parallel() + assert.Nil(t, newControlPlaneReadyWaiter(t, nil)) +} diff --git a/test/step/deployexecutor.go b/test/step/deployexecutor.go index 942ac3599bb..4e18dbb0da6 100644 --- a/test/step/deployexecutor.go +++ b/test/step/deployexecutor.go @@ -46,16 +46,28 @@ type DeployExecutor struct { // the environment is not defined in bicep. Environment string - // MaxRetries is the maximum number of retry attempts after the initial deployment fails. - // Zero means no retries (default behavior). + // MaxRetries caps the number of retry attempts after the initial deployment + // fails. Zero means uncapped: retries continue until RetryBudget is spent. MaxRetries int - // RetryDelay is the duration to wait between retry attempts. + // RetryDelay is the minimum time to wait between retry attempts. When + // WaitForReady is set the actual wait is this delay plus however long the + // control plane takes to become ready again. RetryDelay time.Duration + // RetryBudget bounds the total wall-clock time spent retrying, measured + // from the moment the first attempt fails. Zero disables retries. + RetryBudget time.Duration + // ShouldRetry is a predicate that determines whether a failed deployment should be retried. - // If nil, no retries are attempted regardless of MaxRetries. + // If nil, no retries are attempted regardless of RetryBudget. ShouldRetry func(error) bool + + // WaitForReady, when set, is called before each retry attempt and must + // block until the Radius control plane is serving again or the context is + // done. Execute populates it from the test's Kubernetes client; a nil value + // disables the readiness gate and falls back to RetryDelay alone. + WaitForReady func(context.Context) error } // Default retry behavior applied by NewDeployExecutor for transient deployment @@ -63,11 +75,23 @@ type DeployExecutor struct { // container image pulls from shared registries (for example the // ghcr.io/radius-project/* images) that occasionally fail due to registry or // network blips, and UCP connection resets/EOFs when the kind control-plane -// restarts under runner resource pressure and drops the port-forward tunnel. -// Callers can override these defaults with WithRetry. +// restarts under runner resource pressure. +// +// Retrying is bounded by wall-clock time rather than by a fixed attempt count. +// A fixed count does not work for a control-plane outage: once the +// kube-apiserver is down every retry fails instantly against a dead socket, so +// a budget of "2 retries, 30s apart" is spent in about a minute while the +// outage lasts several. Pairing a deadline with the readiness gate in +// WaitForReady means the retry loop waits for the control plane to return +// instead of burning attempts against it. Callers can override these defaults +// with WithRetry. const ( - defaultTransientMaxRetries = 2 defaultTransientRetryDelay = 30 * time.Second + + // defaultTransientRetryBudget is roughly three times the longest + // control-plane outage observed in CI (about 3.5 minutes between the first + // etcd request timeout and the kube-apiserver serving again). + defaultTransientRetryBudget = 10 * time.Minute ) // transientImagePullErrorMarkers are substrings that indicate a container image @@ -102,31 +126,35 @@ func IsTransientImagePullError(err error) bool { // failed because the connection between rad and the UCP API server was reset or // closed mid-request, rather than because the deployment itself was invalid. // -// In CI the Radius workspace reaches UCP through a local `kubectl port-forward` -// tunnel that proxies through the kind cluster's kube-apiserver. When the -// GitHub-hosted runner is under resource pressure the kind static control-plane -// pods (kube-apiserver/controller-manager/scheduler) restart, which drops every -// in-flight port-forward tunnel at once and resets all parallel `rad deploy` -// connections simultaneously. UCP and the Radius pods do not crash, so -// re-running the deployment once the control-plane recovers typically succeeds. +// rad reaches UCP over the aggregated API path +// (/apis/api.ucp.dev/v1alpha3/...) using the connection built from the test +// kubeconfig, which for kind points at the apiserver port published on +// 127.0.0.1. When the control plane goes down - observed in CI as etcd failing +// to serve requests, followed a few minutes later by the kube-apiserver being +// restarted - every in-flight connection is reset at once, so all parallel +// `rad deploy` invocations fail together. Other parts of the suite (for example +// the gateway tests) reach workloads through a `kubectl port-forward` tunnel +// that proxies through the same kube-apiserver, and those tunnels break with +// the same errors. UCP and the Radius pods do not crash in either case, so +// re-running the deployment once the control plane recovers typically +// succeeds - which is what the readiness gate in WaitForReady waits for. var transientConnectionErrorMarkers = []string{ - // The socket to the port-forward tunnel was reset when the kube-apiserver - // (which the tunnel proxies through) bounced, e.g. + // The socket to the apiserver was reset when it bounced, e.g. // `read tcp 127.0.0.1:38764->127.0.0.1:37481: read: connection reset by peer`. "connection reset by peer", - // rad's HTTP client observed a clean close of the tunnel mid-response, e.g. + // rad's HTTP client observed a clean close mid-response, e.g. // `Get "https://127.0.0.1:37481/.../operationStatuses/...": EOF`. ": EOF", // The pod log-stream tailers and larger response bodies surface this variant - // when the tunnel closes partway through a read. + // when the connection closes partway through a read. "unexpected EOF", - // The write side of the tunnel was torn down while rad was still sending. + // The write side was torn down while rad was still sending. "broken pipe", } // IsTransientConnectionError reports whether err was caused by a transient // network disruption between rad and the UCP API server (a reset or closed -// port-forward tunnel) that is likely to succeed on retry. See +// connection) that is likely to succeed on retry. See // transientConnectionErrorMarkers for the environmental root cause. // // A connection reset/EOF is a transport-level failure that rad surfaces as a @@ -154,15 +182,16 @@ func IsTransientDeployError(err error) bool { // // By default the executor retries a deployment that fails with a transient // error - either a container image pull blip or a UCP connection reset/EOF (see -// IsTransientDeployError). Use WithRetry to override the retry count, delay, and -// predicate. +// IsTransientDeployError) - for up to defaultTransientRetryBudget, waiting for +// the control plane to become ready again before each attempt. Use WithRetry to +// override the attempt cap, delay, and predicate. func NewDeployExecutor(template string, parameters ...string) *DeployExecutor { return &DeployExecutor{ Description: fmt.Sprintf("deploy %s", template), Template: template, Parameters: parameters, - MaxRetries: defaultTransientMaxRetries, RetryDelay: defaultTransientRetryDelay, + RetryBudget: defaultTransientRetryBudget, ShouldRetry: IsTransientDeployError, } } @@ -180,10 +209,12 @@ func (d *DeployExecutor) WithEnvironment(environment string) *DeployExecutor { } // WithRetry configures retry behavior for transient deployment failures, -// replacing the default transient image pull retry set by NewDeployExecutor. -// maxRetries is the number of additional attempts after the first failure. -// delay is the wait time between attempts. shouldRetry determines whether -// a given error is eligible for retry. +// replacing the default transient-error retry set by NewDeployExecutor. +// maxRetries caps the number of additional attempts after the first failure; +// zero leaves the attempt count uncapped so only RetryBudget bounds it. delay +// is the minimum wait between attempts. shouldRetry determines whether a given +// error is eligible for retry. The overall retry budget is unchanged; use +// WithRetryBudget to adjust it. func (d *DeployExecutor) WithRetry(maxRetries int, delay time.Duration, shouldRetry func(error) bool) *DeployExecutor { d.MaxRetries = maxRetries d.RetryDelay = delay @@ -191,6 +222,13 @@ func (d *DeployExecutor) WithRetry(maxRetries int, delay time.Duration, shouldRe return d } +// WithRetryBudget sets the total wall-clock time the executor may spend +// retrying a transient failure and returns the same instance. +func (d *DeployExecutor) WithRetryBudget(budget time.Duration) *DeployExecutor { + d.RetryBudget = budget + return d +} + // GetDescription returns the Description field of the DeployExecutor instance. func (d *DeployExecutor) GetDescription() string { return d.Description @@ -205,6 +243,10 @@ func (d *DeployExecutor) Execute(ctx context.Context, t *testing.T, options test t.Logf("deploying %s from file %s", d.Description, d.Template) cli := radcli.NewCLI(t, options.ConfigFilePath) + if d.WaitForReady == nil && options.K8sClient != nil { + d.WaitForReady = newControlPlaneReadyWaiter(t, options.K8sClient) + } + deployFunc := func() error { return cli.Deploy(ctx, templateFilePath, d.Environment, d.Application, d.Parameters...) } @@ -214,40 +256,87 @@ func (d *DeployExecutor) Execute(ctx context.Context, t *testing.T, options test t.Logf("finished deploying %s from file %s", d.Description, d.Template) } -// executeWithRetry runs the deploy function with optional retry logic. +// retryEnabled reports whether the executor is configured to retry at all. +func (d *DeployExecutor) retryEnabled() bool { + return d.ShouldRetry != nil && d.RetryBudget > 0 +} + +// executeWithRetry runs the deploy function, retrying transient failures until +// the retry budget is exhausted, the attempt cap (if any) is reached, or the +// context is cancelled. +// +// The budget is wall-clock based on purpose. The failure this guards against is +// a control-plane outage, during which a deployment fails immediately rather +// than slowly, so a fixed attempt count is consumed long before the control +// plane returns. Between attempts the loop waits for the delay and then blocks +// on WaitForReady, which means time is spent waiting for recovery rather than +// on attempts that cannot succeed. func (d *DeployExecutor) executeWithRetry(ctx context.Context, t *testing.T, deployFunc func() error) error { - maxAttempts := 1 - if d.MaxRetries > 0 && d.ShouldRetry != nil { - maxAttempts = d.MaxRetries + 1 + err := deployFunc() + if err == nil || !d.retryEnabled() { + return err } - var lastErr error - for attempt := 1; attempt <= maxAttempts; attempt++ { - if attempt > 1 { - t.Logf("waiting %s before retry attempt %d/%d", d.RetryDelay, attempt, maxAttempts) - timer := time.NewTimer(d.RetryDelay) - select { - case <-timer.C: - case <-ctx.Done(): - timer.Stop() - lastErr = ctx.Err() - } + deadline := time.Now().Add(d.RetryBudget) + + for attempt := 1; ; attempt++ { + if !d.ShouldRetry(err) { + return err + } + + if d.MaxRetries > 0 && attempt > d.MaxRetries { + t.Logf("deployment failed after %d retries: %v", d.MaxRetries, err) + return err + } + + remaining := time.Until(deadline) + if remaining <= 0 { + t.Logf("deployment retry budget of %s is exhausted after %d attempts: %v", d.RetryBudget, attempt, err) + return err + } + + t.Logf("deployment attempt %d failed with retryable error (%s of the %s retry budget remaining): %v", + attempt, remaining.Round(time.Second), d.RetryBudget, err) + + retryCtx, cancel := context.WithDeadline(ctx, deadline) + waitErr := d.waitBeforeRetry(retryCtx, t) + cancel() + + if waitErr != nil { + // A cancelled parent context is a caller-driven abort and is + // reported as such. Exhausting the budget is not: the deployment + // error is the meaningful failure to surface. if ctx.Err() != nil { - break + return ctx.Err() } + + t.Logf("giving up on retrying the deployment: %v", waitErr) + return err } - lastErr = deployFunc() - if lastErr == nil { - break + err = deployFunc() + if err == nil { + return nil } + } +} - if attempt == maxAttempts || !d.ShouldRetry(lastErr) { - break +// waitBeforeRetry waits out the retry delay and then blocks until the control +// plane is ready, bounded by ctx. +func (d *DeployExecutor) waitBeforeRetry(ctx context.Context, t *testing.T) error { + if d.RetryDelay > 0 { + timer := time.NewTimer(d.RetryDelay) + select { + case <-timer.C: + case <-ctx.Done(): + timer.Stop() + return ctx.Err() } + } - t.Logf("deployment attempt %d/%d failed with retryable error: %v", attempt, maxAttempts, lastErr) + if d.WaitForReady == nil { + return nil } - return lastErr + return d.WaitForReady(ctx) } diff --git a/test/step/deployexecutor_test.go b/test/step/deployexecutor_test.go index 8cdbf258c3f..935bcfd9e2c 100644 --- a/test/step/deployexecutor_test.go +++ b/test/step/deployexecutor_test.go @@ -135,8 +135,9 @@ func Test_ExecuteWithRetry_DefaultRetriesTransientImagePullError(t *testing.T) { func Test_ExecuteWithRetry_DefaultRetriesTransientConnectionError(t *testing.T) { t.Parallel() var calls atomic.Int32 - // NewDeployExecutor retries transient UCP connection resets by default, which - // occur when the kind control-plane restarts and drops the port-forward tunnel. + // NewDeployExecutor retries transient UCP connection resets by default, + // which occur when the kind control plane restarts and drops every + // in-flight connection to the API server. d := NewDeployExecutor("test.bicep") d.RetryDelay = 10 * time.Millisecond // shorten the delay for the test @@ -190,6 +191,130 @@ func Test_ExecuteWithRetry_NilShouldRetryDisablesRetries(t *testing.T) { assert.Equal(t, int32(1), calls.Load()) } +func Test_ExecuteWithRetry_ZeroBudgetDisablesRetries(t *testing.T) { + t.Parallel() + var calls atomic.Int32 + d := NewDeployExecutor("test.bicep") + d.RetryDelay = 0 + d.RetryBudget = 0 + d.ShouldRetry = func(error) bool { return true } + + err := d.executeWithRetry(context.Background(), t, func() error { + calls.Add(1) + return errors.New("transient") + }) + + require.Error(t, err) + assert.Equal(t, int32(1), calls.Load()) +} + +func Test_ExecuteWithRetry_UncappedAttemptsBoundedByBudget(t *testing.T) { + t.Parallel() + var calls atomic.Int32 + // MaxRetries is zero, so only the budget bounds the loop. A short budget + // with no delay still terminates. + d := NewDeployExecutor("test.bicep") + d.MaxRetries = 0 + d.RetryDelay = time.Millisecond + d.RetryBudget = 50 * time.Millisecond + d.ShouldRetry = func(error) bool { return true } + + err := d.executeWithRetry(context.Background(), t, func() error { + calls.Add(1) + return errors.New("always fails") + }) + + require.Error(t, err) + assert.Equal(t, "always fails", err.Error()) + // The initial attempt plus at least one retry, and the loop must have + // stopped rather than run forever. + assert.Greater(t, calls.Load(), int32(1)) +} + +func Test_ExecuteWithRetry_WaitsForControlPlaneBeforeRetrying(t *testing.T) { + t.Parallel() + var calls atomic.Int32 + var waits atomic.Int32 + + d := NewDeployExecutor("test.bicep") + d.RetryDelay = time.Millisecond + d.ShouldRetry = func(error) bool { return true } + // The gate stands in for a control plane that is down for the first two + // probes: the retry must not be attempted until it reports ready. + d.WaitForReady = func(ctx context.Context) error { + if waits.Add(1) < 3 { + return errors.New("kube-apiserver is not ready") + } + return nil + } + + err := d.executeWithRetry(context.Background(), t, func() error { + n := calls.Add(1) + if n == 1 { + return errors.New("connection reset by peer") + } + return nil + }) + + // A failing gate ends the loop with the deployment error rather than + // retrying against an unavailable control plane. + require.Error(t, err) + assert.Equal(t, int32(1), calls.Load()) + assert.Equal(t, int32(1), waits.Load()) +} + +func Test_ExecuteWithRetry_RetriesOnceControlPlaneIsReady(t *testing.T) { + t.Parallel() + var calls atomic.Int32 + var waits atomic.Int32 + + d := NewDeployExecutor("test.bicep") + d.RetryDelay = time.Millisecond + d.ShouldRetry = func(error) bool { return true } + d.WaitForReady = func(ctx context.Context) error { + waits.Add(1) + return nil + } + + err := d.executeWithRetry(context.Background(), t, func() error { + n := calls.Add(1) + if n == 1 { + return errors.New("connection reset by peer") + } + return nil + }) + + require.NoError(t, err) + assert.Equal(t, int32(2), calls.Load()) + assert.Equal(t, int32(1), waits.Load()) +} + +func Test_ExecuteWithRetry_BudgetBoundsTheReadinessWait(t *testing.T) { + t.Parallel() + var calls atomic.Int32 + + d := NewDeployExecutor("test.bicep") + d.RetryDelay = 0 + d.RetryBudget = 50 * time.Millisecond + d.ShouldRetry = func(error) bool { return true } + // A control plane that never recovers must not block past the budget. + d.WaitForReady = func(ctx context.Context) error { + <-ctx.Done() + return ctx.Err() + } + + start := time.Now() + err := d.executeWithRetry(context.Background(), t, func() error { + calls.Add(1) + return errors.New("connection reset by peer") + }) + + require.Error(t, err) + assert.Equal(t, "connection reset by peer", err.Error()) + assert.Equal(t, int32(1), calls.Load()) + assert.Less(t, time.Since(start), 5*time.Second) +} + func Test_IsTransientImagePullError(t *testing.T) { // imagePullError mirrors how rad surfaces a transient image pull failure: // the ErrImagePull/timeout cause only appears inside a deeply nested @@ -282,7 +407,7 @@ func Test_IsTransientConnectionError(t *testing.T) { }{ {name: "nil error", err: nil, expected: false}, { - name: "port-forward connection reset", + name: "apiserver connection reset", err: errors.New(`Get "https://127.0.0.1:37481/.../operationStatuses/...": read tcp 127.0.0.1:38764->127.0.0.1:37481: read: connection reset by peer`), expected: true, }, From 8e1a963e1ee805566745737f38f33925721c8f30 Mon Sep 17 00:00:00 2001 From: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com> Date: Tue, 4 Aug 2026 11:57:06 -0400 Subject: [PATCH 2/7] Simplify the retry API and fix the pods.yaml diagnostics dump 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> --- .../scripts/collect-cluster-diagnostics.sh | 7 +- .../corerp/cloud/resources/aci_test.go | 15 +- .../radiuscore_azure_mysql_portallink_test.go | 2 +- test/step/controlplane.go | 46 ++--- test/step/controlplane_test.go | 55 ++---- test/step/deployexecutor.go | 157 ++++++------------ test/step/deployexecutor_test.go | 46 ++--- 7 files changed, 110 insertions(+), 218 deletions(-) diff --git a/.github/scripts/collect-cluster-diagnostics.sh b/.github/scripts/collect-cluster-diagnostics.sh index 29617100599..8fc2253357b 100755 --- a/.github/scripts/collect-cluster-diagnostics.sh +++ b/.github/scripts/collect-cluster-diagnostics.sh @@ -76,7 +76,11 @@ collect_pod_state() { # the YAML dump adds the machine-readable lastState/exitCode fields. capture "${OUTPUT_DIR}/${PREFIX}-pod-states.log" kubectl get pods -A -o wide capture "${OUTPUT_DIR}/${PREFIX}-pod-states.log" kubectl describe pods -A - capture "${OUTPUT_DIR}/${PREFIX}-pods.yaml" kubectl get pods -A -o yaml + + # 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 } collect_events() { @@ -97,6 +101,7 @@ collect_control_plane_health() { capture "${output_file}" kubectl get apiservices -o wide } + # collect_kube_system_logs writes the current and previous logs of every # kube-system container. Timestamps are included so the logs can be correlated # with Radius pod logs and with the test output. diff --git a/test/functional-portable/corerp/cloud/resources/aci_test.go b/test/functional-portable/corerp/cloud/resources/aci_test.go index 949b7383c82..e9950327550 100644 --- a/test/functional-portable/corerp/cloud/resources/aci_test.go +++ b/test/functional-portable/corerp/cloud/resources/aci_test.go @@ -40,12 +40,13 @@ import ( func Test_ACI(t *testing.T) { // Disabled: this test dominates the corerp-cloud functional leg's wall-clock // time. It provisions real Azure Container Instances (plus a VNet/NSG/ILB) and - // is wrapped in a 2x retry with 60s backoff to tolerate the subscription-shared - // 'StandardCores' ACI quota (ContainerGroupQuotaReached). When the quota is - // exhausted by concurrent CI runs the deploy is retried end-to-end, so a single - // run can take ~11-12 minutes (vs <1 minute for every other test in the leg) and - // roughly doubles the cloud workflow's total time. Re-enable once the ACI tests - // are isolated onto their own quota-aware lane. See #12044 and #12163. + // is wrapped in a 3 minute retry budget with 60s backoff to tolerate the + // subscription-shared 'StandardCores' ACI quota (ContainerGroupQuotaReached). + // When the quota is exhausted by concurrent CI runs the deploy is retried + // end-to-end, so a single run can take ~11-12 minutes (vs <1 minute for every + // other test in the leg) and roughly doubles the cloud workflow's total time. + // Re-enable once the ACI tests are isolated onto their own quota-aware lane. + // See #12044 and #12163. t.Skip("Test_ACI is temporarily disabled: real ACI provisioning + quota retries dominate corerp-cloud CI time. See #12044, #12163.") name := "aci-app" @@ -56,7 +57,7 @@ func Test_ACI(t *testing.T) { test := rp.NewRPTest(t, name, []rp.TestStep{ { - Executor: step.NewDeployExecutor(template).WithRetry(2, 60*time.Second, isTransientAzureError), + Executor: step.NewDeployExecutor(template).WithRetry(3*time.Minute, 60*time.Second, isTransientAzureError), SkipObjectValidation: true, RPResources: &validation.RPResourceSet{ Resources: []validation.RPResource{ diff --git a/test/functional-portable/corerp/cloud/resources/radiuscore_azure_mysql_portallink_test.go b/test/functional-portable/corerp/cloud/resources/radiuscore_azure_mysql_portallink_test.go index 4763fc45dc1..85dd1b366a6 100644 --- a/test/functional-portable/corerp/cloud/resources/radiuscore_azure_mysql_portallink_test.go +++ b/test/functional-portable/corerp/cloud/resources/radiuscore_azure_mysql_portallink_test.go @@ -127,7 +127,7 @@ func Test_RadiusCore_AzureMySql_PortalLink(t *testing.T) { "azureSubscriptionId="+azureSubscriptionID, "azureResourceGroupName="+azureResourceGroupName, "password=not-prod-password", - ).WithRetry(2, 60*time.Second, isTransientAzureError), + ).WithRetry(3*time.Minute, 60*time.Second, isTransientAzureError), RPResources: &validation.RPResourceSet{ Resources: []validation.RPResource{ {Name: recipePackName, Type: validation.CoreRecipePacksResource}, diff --git a/test/step/controlplane.go b/test/step/controlplane.go index e560549481c..ca0e4f8e332 100644 --- a/test/step/controlplane.go +++ b/test/step/controlplane.go @@ -27,10 +27,6 @@ import ( ) const ( - // controlPlaneReadyPollInterval is how long to wait between readiness - // probes while the control plane is recovering. - controlPlaneReadyPollInterval = 5 * time.Second - // controlPlaneProbeTimeout bounds a single readiness probe so a hung // kube-apiserver cannot consume the caller's entire retry budget in one // request. @@ -38,15 +34,20 @@ const ( // radiusAggregatedAPIPath is the aggregated API discovery path that rad // itself requests when it opens a workspace connection (see - // pkg/cli/workspaces.Connection). Probing the same path exercises the whole - // chain the deployment depends on: the kube-apiserver, its aggregation - // layer, and the UCP APIService behind it. + // pkg/cli/workspaces.Connection). Probing it exercises the whole chain a + // deployment depends on: the kube-apiserver, its aggregation layer, and the + // UCP APIService behind it. A healthy response here implies the + // kube-apiserver is serving, so no separate /readyz probe is needed. radiusAggregatedAPIPath = "/apis/api.ucp.dev/v1alpha3" ) -// newControlPlaneReadyWaiter returns a function that blocks until the -// Kubernetes control plane and the Radius aggregated API are both serving -// again, or until ctx is done. +// controlPlaneReadyPollInterval is how long to wait between readiness probes +// while the control plane is recovering. It is a variable so tests can shorten +// it. +var controlPlaneReadyPollInterval = 5 * time.Second + +// newControlPlaneReadyWaiter returns a function that blocks until the Radius +// aggregated API is serving again, or until ctx is done. // // It exists because a fixed sleep is not a useful gate between deployment // retries. When the kind control plane goes down, every queued retry fails @@ -73,11 +74,10 @@ func newControlPlaneReadyWaiter(t *testing.T, client k8s.Interface) func(context // waitForControlPlaneReady polls until the control plane is ready or ctx is // done. The error returned on timeout includes the last probe failure so the -// test log records which component was still unavailable. +// test log records what was still unavailable. func waitForControlPlaneReady(ctx context.Context, t *testing.T, client rest.Interface) error { - var lastErr error for { - lastErr = checkControlPlaneReady(ctx, client) + lastErr := probe(ctx, client, radiusAggregatedAPIPath) if lastErr == nil { return nil } @@ -94,26 +94,6 @@ func waitForControlPlaneReady(ctx context.Context, t *testing.T, client rest.Int } } -// checkControlPlaneReady performs a single readiness probe. -func checkControlPlaneReady(ctx context.Context, client rest.Interface) error { - // /readyz reports whether the kube-apiserver has finished starting and its - // health checks - including the etcd backend check - are passing. It is - // served to any authenticated caller by the built-in - // system:public-info-viewer role, so it needs no extra RBAC. - if err := probe(ctx, client, "/readyz"); err != nil { - return fmt.Errorf("kube-apiserver is not ready: %w", err) - } - - // The kube-apiserver can be ready before the aggregated API it proxies to - // is, and a request to an unavailable APIService fails with 503. Probing it - // avoids retrying a deployment that is guaranteed to fail. - if err := probe(ctx, client, radiusAggregatedAPIPath); err != nil { - return fmt.Errorf("the Radius aggregated API is not reachable: %w", err) - } - - return nil -} - func probe(ctx context.Context, client rest.Interface, path string) error { probeCtx, cancel := context.WithTimeout(ctx, controlPlaneProbeTimeout) defer cancel() diff --git a/test/step/controlplane_test.go b/test/step/controlplane_test.go index a82103173f9..7076773294f 100644 --- a/test/step/controlplane_test.go +++ b/test/step/controlplane_test.go @@ -44,7 +44,7 @@ func newTestRESTClient(t *testing.T, handler http.Handler) rest.Interface { return client.Discovery().RESTClient() } -func Test_CheckControlPlaneReady_ReadyWhenBothProbesSucceed(t *testing.T) { +func Test_WaitForControlPlaneReady_ReturnsWhenAggregatedAPIServes(t *testing.T) { t.Parallel() var paths []string client := newTestRESTClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -52,47 +52,26 @@ func Test_CheckControlPlaneReady_ReadyWhenBothProbesSucceed(t *testing.T) { w.WriteHeader(http.StatusOK) })) - require.NoError(t, checkControlPlaneReady(context.Background(), client)) - assert.Equal(t, []string{"/readyz", radiusAggregatedAPIPath}, paths) -} - -func Test_CheckControlPlaneReady_NotReadyWhenAPIServerIsUnhealthy(t *testing.T) { - t.Parallel() - client := newTestRESTClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusInternalServerError) - })) + require.NoError(t, waitForControlPlaneReady(context.Background(), t, client)) - err := checkControlPlaneReady(context.Background(), client) - require.Error(t, err) - assert.Contains(t, err.Error(), "kube-apiserver is not ready") + // 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) } -func Test_CheckControlPlaneReady_NotReadyWhenAggregatedAPIIsUnavailable(t *testing.T) { - t.Parallel() - // The kube-apiserver can be healthy while the UCP APIService behind the - // aggregation layer is still unavailable, which the apiserver reports as a - // 503. Retrying a deployment in that window cannot succeed. - client := newTestRESTClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == radiusAggregatedAPIPath { - w.WriteHeader(http.StatusServiceUnavailable) - return - } - w.WriteHeader(http.StatusOK) - })) +func Test_WaitForControlPlaneReady_PollsUntilHealthy(t *testing.T) { + // Not parallel: this test shortens the package-level poll interval. + original := controlPlaneReadyPollInterval + controlPlaneReadyPollInterval = time.Millisecond + t.Cleanup(func() { controlPlaneReadyPollInterval = original }) - err := checkControlPlaneReady(context.Background(), client) - require.Error(t, err) - assert.Contains(t, err.Error(), "the Radius aggregated API is not reachable") -} - -func Test_WaitForControlPlaneReady_ReturnsOnceHealthy(t *testing.T) { - t.Parallel() var requests atomic.Int32 + // The kube-apiserver can be up while the UCP APIService behind the + // aggregation layer is still unavailable, which surfaces as a 503. The gate + // must keep polling rather than let a retry proceed in that window. client := newTestRESTClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Fail the first /readyz probe so the wait loop has to poll at least - // twice before succeeding. - if r.URL.Path == "/readyz" && requests.Add(1) == 1 { - w.WriteHeader(http.StatusInternalServerError) + if requests.Add(1) < 3 { + w.WriteHeader(http.StatusServiceUnavailable) return } w.WriteHeader(http.StatusOK) @@ -102,7 +81,7 @@ func Test_WaitForControlPlaneReady_ReturnsOnceHealthy(t *testing.T) { defer cancel() require.NoError(t, waitForControlPlaneReady(ctx, t, client)) - assert.Greater(t, requests.Load(), int32(1)) + assert.Equal(t, int32(3), requests.Load()) } func Test_WaitForControlPlaneReady_ReportsLastProbeFailureOnTimeout(t *testing.T) { @@ -117,7 +96,7 @@ func Test_WaitForControlPlaneReady_ReportsLastProbeFailureOnTimeout(t *testing.T err := waitForControlPlaneReady(ctx, t, client) require.Error(t, err) assert.ErrorIs(t, err, context.DeadlineExceeded) - assert.Contains(t, err.Error(), "kube-apiserver is not ready") + assert.Contains(t, err.Error(), "last probe failure") } func Test_NewControlPlaneReadyWaiter_NilClientDisablesGate(t *testing.T) { diff --git a/test/step/deployexecutor.go b/test/step/deployexecutor.go index 4e18dbb0da6..d8dd0c787c3 100644 --- a/test/step/deployexecutor.go +++ b/test/step/deployexecutor.go @@ -46,45 +46,33 @@ type DeployExecutor struct { // the environment is not defined in bicep. Environment string - // MaxRetries caps the number of retry attempts after the initial deployment - // fails. Zero means uncapped: retries continue until RetryBudget is spent. - MaxRetries int - - // RetryDelay is the minimum time to wait between retry attempts. When - // WaitForReady is set the actual wait is this delay plus however long the - // control plane takes to become ready again. + // RetryDelay is the minimum time to wait between retry attempts. RetryDelay time.Duration - // RetryBudget bounds the total wall-clock time spent retrying, measured - // from the moment the first attempt fails. Zero disables retries. + // RetryBudget bounds how long the executor keeps starting new attempts, + // measured from the moment the first attempt fails. Once it elapses no + // further attempt begins, but an attempt already in flight runs to + // completion under the caller's context. Zero disables retries. RetryBudget time.Duration // ShouldRetry is a predicate that determines whether a failed deployment should be retried. // If nil, no retries are attempted regardless of RetryBudget. ShouldRetry func(error) bool - // WaitForReady, when set, is called before each retry attempt and must - // block until the Radius control plane is serving again or the context is - // done. Execute populates it from the test's Kubernetes client; a nil value - // disables the readiness gate and falls back to RetryDelay alone. - WaitForReady func(context.Context) error + // waitForReady, when set, blocks before each retry until the Radius control + // plane is serving again. Execute populates it from the test's Kubernetes + // client; a nil value falls back to RetryDelay alone. + waitForReady func(context.Context) error } // Default retry behavior applied by NewDeployExecutor for transient deployment -// failures. Functional tests hit two classes of environmental flake in CI: -// container image pulls from shared registries (for example the -// ghcr.io/radius-project/* images) that occasionally fail due to registry or -// network blips, and UCP connection resets/EOFs when the kind control-plane -// restarts under runner resource pressure. +// failures: image pull blips from shared registries, and UCP connection +// resets/EOFs when the kind control plane restarts under runner pressure. // -// Retrying is bounded by wall-clock time rather than by a fixed attempt count. -// A fixed count does not work for a control-plane outage: once the -// kube-apiserver is down every retry fails instantly against a dead socket, so -// a budget of "2 retries, 30s apart" is spent in about a minute while the -// outage lasts several. Pairing a deadline with the readiness gate in -// WaitForReady means the retry loop waits for the control plane to return -// instead of burning attempts against it. Callers can override these defaults -// with WithRetry. +// The bound is wall-clock time rather than an attempt count because a +// control-plane outage makes every attempt fail instantly against a dead +// socket, so "2 retries, 30s apart" is spent in about a minute while the outage +// lasts several. const ( defaultTransientRetryDelay = 30 * time.Second @@ -126,18 +114,14 @@ func IsTransientImagePullError(err error) bool { // failed because the connection between rad and the UCP API server was reset or // closed mid-request, rather than because the deployment itself was invalid. // -// rad reaches UCP over the aggregated API path -// (/apis/api.ucp.dev/v1alpha3/...) using the connection built from the test -// kubeconfig, which for kind points at the apiserver port published on -// 127.0.0.1. When the control plane goes down - observed in CI as etcd failing -// to serve requests, followed a few minutes later by the kube-apiserver being -// restarted - every in-flight connection is reset at once, so all parallel -// `rad deploy` invocations fail together. Other parts of the suite (for example -// the gateway tests) reach workloads through a `kubectl port-forward` tunnel -// that proxies through the same kube-apiserver, and those tunnels break with -// the same errors. UCP and the Radius pods do not crash in either case, so -// re-running the deployment once the control plane recovers typically -// succeeds - which is what the readiness gate in WaitForReady waits for. +// rad reaches UCP over the aggregated API path (/apis/api.ucp.dev/v1alpha3/...) +// using the connection built from the test kubeconfig, which for kind points at +// the apiserver port published on 127.0.0.1. When the control plane goes down - +// observed in CI as etcd failing to serve requests, followed a few minutes +// later by the kube-apiserver restarting - every in-flight connection is reset +// at once, so all parallel `rad deploy` invocations fail together. UCP and the +// Radius pods do not crash, so re-running the deployment once the control plane +// recovers typically succeeds. var transientConnectionErrorMarkers = []string{ // The socket to the apiserver was reset when it bounced, e.g. // `read tcp 127.0.0.1:38764->127.0.0.1:37481: read: connection reset by peer`. @@ -181,10 +165,9 @@ func IsTransientDeployError(err error) bool { // NewDeployExecutor creates a new DeployExecutor instance with the given template and parameters. // // By default the executor retries a deployment that fails with a transient -// error - either a container image pull blip or a UCP connection reset/EOF (see -// IsTransientDeployError) - for up to defaultTransientRetryBudget, waiting for -// the control plane to become ready again before each attempt. Use WithRetry to -// override the attempt cap, delay, and predicate. +// error (see IsTransientDeployError) for up to defaultTransientRetryBudget, +// waiting for the control plane to become ready again before each attempt. Use +// WithRetry to override the budget, delay, and predicate. func NewDeployExecutor(template string, parameters ...string) *DeployExecutor { return &DeployExecutor{ Description: fmt.Sprintf("deploy %s", template), @@ -209,26 +192,16 @@ func (d *DeployExecutor) WithEnvironment(environment string) *DeployExecutor { } // WithRetry configures retry behavior for transient deployment failures, -// replacing the default transient-error retry set by NewDeployExecutor. -// maxRetries caps the number of additional attempts after the first failure; -// zero leaves the attempt count uncapped so only RetryBudget bounds it. delay -// is the minimum wait between attempts. shouldRetry determines whether a given -// error is eligible for retry. The overall retry budget is unchanged; use -// WithRetryBudget to adjust it. -func (d *DeployExecutor) WithRetry(maxRetries int, delay time.Duration, shouldRetry func(error) bool) *DeployExecutor { - d.MaxRetries = maxRetries +// replacing the default set by NewDeployExecutor. budget bounds how long new +// attempts keep being started, delay is the minimum wait between them, and +// shouldRetry decides which errors are eligible. +func (d *DeployExecutor) WithRetry(budget time.Duration, delay time.Duration, shouldRetry func(error) bool) *DeployExecutor { + d.RetryBudget = budget d.RetryDelay = delay d.ShouldRetry = shouldRetry return d } -// WithRetryBudget sets the total wall-clock time the executor may spend -// retrying a transient failure and returns the same instance. -func (d *DeployExecutor) WithRetryBudget(budget time.Duration) *DeployExecutor { - d.RetryBudget = budget - return d -} - // GetDescription returns the Description field of the DeployExecutor instance. func (d *DeployExecutor) GetDescription() string { return d.Description @@ -243,8 +216,8 @@ func (d *DeployExecutor) Execute(ctx context.Context, t *testing.T, options test t.Logf("deploying %s from file %s", d.Description, d.Template) cli := radcli.NewCLI(t, options.ConfigFilePath) - if d.WaitForReady == nil && options.K8sClient != nil { - d.WaitForReady = newControlPlaneReadyWaiter(t, options.K8sClient) + if d.waitForReady == nil && options.K8sClient != nil { + d.waitForReady = newControlPlaneReadyWaiter(t, options.K8sClient) } deployFunc := func() error { @@ -256,53 +229,27 @@ func (d *DeployExecutor) Execute(ctx context.Context, t *testing.T, options test t.Logf("finished deploying %s from file %s", d.Description, d.Template) } -// retryEnabled reports whether the executor is configured to retry at all. -func (d *DeployExecutor) retryEnabled() bool { - return d.ShouldRetry != nil && d.RetryBudget > 0 -} - // executeWithRetry runs the deploy function, retrying transient failures until -// the retry budget is exhausted, the attempt cap (if any) is reached, or the -// context is cancelled. +// the retry budget is spent or the context is cancelled. // -// The budget is wall-clock based on purpose. The failure this guards against is -// a control-plane outage, during which a deployment fails immediately rather -// than slowly, so a fixed attempt count is consumed long before the control -// plane returns. Between attempts the loop waits for the delay and then blocks -// on WaitForReady, which means time is spent waiting for recovery rather than -// on attempts that cannot succeed. +// The bound is wall-clock time because a control-plane outage makes a +// deployment fail immediately rather than slowly, so a fixed attempt count is +// consumed long before the control plane returns. Between attempts the loop +// waits for the delay and then for readiness, spending the budget on recovery +// rather than on attempts that cannot succeed. func (d *DeployExecutor) executeWithRetry(ctx context.Context, t *testing.T, deployFunc func() error) error { err := deployFunc() - if err == nil || !d.retryEnabled() { + if err == nil || d.ShouldRetry == nil || d.RetryBudget <= 0 { return err } - deadline := time.Now().Add(d.RetryBudget) - - for attempt := 1; ; attempt++ { - if !d.ShouldRetry(err) { - return err - } - - if d.MaxRetries > 0 && attempt > d.MaxRetries { - t.Logf("deployment failed after %d retries: %v", d.MaxRetries, err) - return err - } - - remaining := time.Until(deadline) - if remaining <= 0 { - t.Logf("deployment retry budget of %s is exhausted after %d attempts: %v", d.RetryBudget, attempt, err) - return err - } - - t.Logf("deployment attempt %d failed with retryable error (%s of the %s retry budget remaining): %v", - attempt, remaining.Round(time.Second), d.RetryBudget, err) + retryCtx, cancel := context.WithTimeout(ctx, d.RetryBudget) + defer cancel() - retryCtx, cancel := context.WithDeadline(ctx, deadline) - waitErr := d.waitBeforeRetry(retryCtx, t) - cancel() + for attempt := 1; d.ShouldRetry(err); attempt++ { + t.Logf("deployment attempt %d failed with a retryable error: %v", attempt, err) - if waitErr != nil { + if waitErr := d.waitBeforeRetry(retryCtx, t); waitErr != nil { // A cancelled parent context is a caller-driven abort and is // reported as such. Exhausting the budget is not: the deployment // error is the meaningful failure to surface. @@ -310,15 +257,16 @@ func (d *DeployExecutor) executeWithRetry(ctx context.Context, t *testing.T, dep return ctx.Err() } - t.Logf("giving up on retrying the deployment: %v", waitErr) + t.Logf("no longer retrying the deployment after %d attempts: %v", attempt, waitErr) return err } - err = deployFunc() - if err == nil { + if err = deployFunc(); err == nil { return nil } } + + return err } // waitBeforeRetry waits out the retry delay and then blocks until the control @@ -326,17 +274,18 @@ func (d *DeployExecutor) executeWithRetry(ctx context.Context, t *testing.T, dep func (d *DeployExecutor) waitBeforeRetry(ctx context.Context, t *testing.T) error { if d.RetryDelay > 0 { timer := time.NewTimer(d.RetryDelay) + defer timer.Stop() + select { case <-timer.C: case <-ctx.Done(): - timer.Stop() return ctx.Err() } } - if d.WaitForReady == nil { + if d.waitForReady == nil { return nil } - return d.WaitForReady(ctx) + return d.waitForReady(ctx) } diff --git a/test/step/deployexecutor_test.go b/test/step/deployexecutor_test.go index 935bcfd9e2c..f2bde069ca2 100644 --- a/test/step/deployexecutor_test.go +++ b/test/step/deployexecutor_test.go @@ -33,7 +33,7 @@ import ( func Test_ExecuteWithRetry_SucceedsOnFirstAttempt(t *testing.T) { t.Parallel() var calls atomic.Int32 - d := NewDeployExecutor("test.bicep").WithRetry(2, 10*time.Millisecond, func(error) bool { return true }) + d := NewDeployExecutor("test.bicep").WithRetry(time.Second, 10*time.Millisecond, func(error) bool { return true }) err := d.executeWithRetry(context.Background(), t, func() error { calls.Add(1) @@ -48,7 +48,7 @@ func Test_ExecuteWithRetry_RetriesOnTransientThenSucceeds(t *testing.T) { t.Parallel() var calls atomic.Int32 transientErr := errors.New("ManagedServiceIdentityNotFound") - d := NewDeployExecutor("test.bicep").WithRetry(2, 10*time.Millisecond, func(err error) bool { + d := NewDeployExecutor("test.bicep").WithRetry(time.Second, 10*time.Millisecond, func(err error) bool { return err.Error() == "ManagedServiceIdentityNotFound" }) @@ -67,7 +67,7 @@ func Test_ExecuteWithRetry_RetriesOnTransientThenSucceeds(t *testing.T) { func Test_ExecuteWithRetry_DoesNotRetryNonTransientError(t *testing.T) { t.Parallel() var calls atomic.Int32 - d := NewDeployExecutor("test.bicep").WithRetry(2, 10*time.Millisecond, func(err error) bool { + d := NewDeployExecutor("test.bicep").WithRetry(time.Second, 10*time.Millisecond, func(err error) bool { return err.Error() == "transient" }) @@ -81,10 +81,10 @@ func Test_ExecuteWithRetry_DoesNotRetryNonTransientError(t *testing.T) { assert.Equal(t, int32(1), calls.Load()) } -func Test_ExecuteWithRetry_ExhaustsAllRetries(t *testing.T) { +func Test_ExecuteWithRetry_StopsWhenBudgetIsExhausted(t *testing.T) { t.Parallel() var calls atomic.Int32 - d := NewDeployExecutor("test.bicep").WithRetry(2, 10*time.Millisecond, func(error) bool { return true }) + d := NewDeployExecutor("test.bicep").WithRetry(50*time.Millisecond, 10*time.Millisecond, func(error) bool { return true }) err := d.executeWithRetry(context.Background(), t, func() error { calls.Add(1) @@ -93,7 +93,9 @@ func Test_ExecuteWithRetry_ExhaustsAllRetries(t *testing.T) { require.Error(t, err) assert.Equal(t, "always fails", err.Error()) - assert.Equal(t, int32(3), calls.Load()) // 1 initial + 2 retries + // The initial attempt plus at least one retry, and the loop must have + // stopped once the budget elapsed rather than run forever. + assert.Greater(t, calls.Load(), int32(1)) } func Test_ExecuteWithRetry_DefaultDoesNotRetryNonTransientError(t *testing.T) { @@ -158,7 +160,7 @@ func Test_ExecuteWithRetry_ContextCancelledDuringDelay(t *testing.T) { t.Parallel() var calls atomic.Int32 ctx, cancel := context.WithCancel(context.Background()) - d := NewDeployExecutor("test.bicep").WithRetry(2, 5*time.Second, func(error) bool { return true }) + d := NewDeployExecutor("test.bicep").WithRetry(time.Minute, 5*time.Second, func(error) bool { return true }) // Cancel context immediately after first deploy attempt err := d.executeWithRetry(ctx, t, func() error { @@ -179,7 +181,6 @@ func Test_ExecuteWithRetry_NilShouldRetryDisablesRetries(t *testing.T) { t.Parallel() var calls atomic.Int32 d := NewDeployExecutor("test.bicep") - d.MaxRetries = 3 d.ShouldRetry = nil // nil predicate err := d.executeWithRetry(context.Background(), t, func() error { @@ -208,29 +209,6 @@ func Test_ExecuteWithRetry_ZeroBudgetDisablesRetries(t *testing.T) { assert.Equal(t, int32(1), calls.Load()) } -func Test_ExecuteWithRetry_UncappedAttemptsBoundedByBudget(t *testing.T) { - t.Parallel() - var calls atomic.Int32 - // MaxRetries is zero, so only the budget bounds the loop. A short budget - // with no delay still terminates. - d := NewDeployExecutor("test.bicep") - d.MaxRetries = 0 - d.RetryDelay = time.Millisecond - d.RetryBudget = 50 * time.Millisecond - d.ShouldRetry = func(error) bool { return true } - - err := d.executeWithRetry(context.Background(), t, func() error { - calls.Add(1) - return errors.New("always fails") - }) - - require.Error(t, err) - assert.Equal(t, "always fails", err.Error()) - // The initial attempt plus at least one retry, and the loop must have - // stopped rather than run forever. - assert.Greater(t, calls.Load(), int32(1)) -} - func Test_ExecuteWithRetry_WaitsForControlPlaneBeforeRetrying(t *testing.T) { t.Parallel() var calls atomic.Int32 @@ -241,7 +219,7 @@ func Test_ExecuteWithRetry_WaitsForControlPlaneBeforeRetrying(t *testing.T) { d.ShouldRetry = func(error) bool { return true } // The gate stands in for a control plane that is down for the first two // probes: the retry must not be attempted until it reports ready. - d.WaitForReady = func(ctx context.Context) error { + d.waitForReady = func(ctx context.Context) error { if waits.Add(1) < 3 { return errors.New("kube-apiserver is not ready") } @@ -271,7 +249,7 @@ func Test_ExecuteWithRetry_RetriesOnceControlPlaneIsReady(t *testing.T) { d := NewDeployExecutor("test.bicep") d.RetryDelay = time.Millisecond d.ShouldRetry = func(error) bool { return true } - d.WaitForReady = func(ctx context.Context) error { + d.waitForReady = func(ctx context.Context) error { waits.Add(1) return nil } @@ -298,7 +276,7 @@ func Test_ExecuteWithRetry_BudgetBoundsTheReadinessWait(t *testing.T) { d.RetryBudget = 50 * time.Millisecond d.ShouldRetry = func(error) bool { return true } // A control plane that never recovers must not block past the budget. - d.WaitForReady = func(ctx context.Context) error { + d.waitForReady = func(ctx context.Context) error { <-ctx.Done() return ctx.Err() } From 58a0a886047bef7b895e2d763f7746838b5e4ca9 Mon Sep 17 00:00:00 2001 From: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com> Date: Tue, 4 Aug 2026 12:18:57 -0400 Subject: [PATCH 3/7] Bound the retry budget by the test binary deadline 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> --- test/step/controlplane.go | 15 +++++------ test/step/controlplane_test.go | 12 +++------ test/step/deployexecutor.go | 46 +++++++++++++++++++++++++++----- test/step/deployexecutor_test.go | 27 +++++++++++++++++++ 4 files changed, 78 insertions(+), 22 deletions(-) diff --git a/test/step/controlplane.go b/test/step/controlplane.go index ca0e4f8e332..98ed8c32fae 100644 --- a/test/step/controlplane.go +++ b/test/step/controlplane.go @@ -27,6 +27,10 @@ import ( ) const ( + // controlPlaneReadyPollInterval is how long to wait between readiness + // probes while the control plane is recovering. + controlPlaneReadyPollInterval = 5 * time.Second + // controlPlaneProbeTimeout bounds a single readiness probe so a hung // kube-apiserver cannot consume the caller's entire retry budget in one // request. @@ -41,11 +45,6 @@ const ( radiusAggregatedAPIPath = "/apis/api.ucp.dev/v1alpha3" ) -// controlPlaneReadyPollInterval is how long to wait between readiness probes -// while the control plane is recovering. It is a variable so tests can shorten -// it. -var controlPlaneReadyPollInterval = 5 * time.Second - // newControlPlaneReadyWaiter returns a function that blocks until the Radius // aggregated API is serving again, or until ctx is done. // @@ -68,14 +67,14 @@ func newControlPlaneReadyWaiter(t *testing.T, client k8s.Interface) func(context } return func(ctx context.Context) error { - return waitForControlPlaneReady(ctx, t, restClient) + return waitForControlPlaneReady(ctx, t, restClient, controlPlaneReadyPollInterval) } } // waitForControlPlaneReady polls until the control plane is ready or ctx is // done. The error returned on timeout includes the last probe failure so the // test log records what was still unavailable. -func waitForControlPlaneReady(ctx context.Context, t *testing.T, client rest.Interface) error { +func waitForControlPlaneReady(ctx context.Context, t *testing.T, client rest.Interface, pollInterval time.Duration) error { for { lastErr := probe(ctx, client, radiusAggregatedAPIPath) if lastErr == nil { @@ -84,7 +83,7 @@ func waitForControlPlaneReady(ctx context.Context, t *testing.T, client rest.Int t.Logf("waiting for the Radius control plane to become ready: %v", lastErr) - timer := time.NewTimer(controlPlaneReadyPollInterval) + timer := time.NewTimer(pollInterval) select { case <-timer.C: case <-ctx.Done(): diff --git a/test/step/controlplane_test.go b/test/step/controlplane_test.go index 7076773294f..dba3039d0a8 100644 --- a/test/step/controlplane_test.go +++ b/test/step/controlplane_test.go @@ -52,7 +52,7 @@ func Test_WaitForControlPlaneReady_ReturnsWhenAggregatedAPIServes(t *testing.T) w.WriteHeader(http.StatusOK) })) - require.NoError(t, waitForControlPlaneReady(context.Background(), t, client)) + 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. @@ -60,11 +60,7 @@ func Test_WaitForControlPlaneReady_ReturnsWhenAggregatedAPIServes(t *testing.T) } func Test_WaitForControlPlaneReady_PollsUntilHealthy(t *testing.T) { - // Not parallel: this test shortens the package-level poll interval. - original := controlPlaneReadyPollInterval - controlPlaneReadyPollInterval = time.Millisecond - t.Cleanup(func() { controlPlaneReadyPollInterval = original }) - + t.Parallel() var requests atomic.Int32 // The kube-apiserver can be up while the UCP APIService behind the // aggregation layer is still unavailable, which surfaces as a 503. The gate @@ -80,7 +76,7 @@ func Test_WaitForControlPlaneReady_PollsUntilHealthy(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() - require.NoError(t, waitForControlPlaneReady(ctx, t, client)) + require.NoError(t, waitForControlPlaneReady(ctx, t, client, time.Millisecond)) assert.Equal(t, int32(3), requests.Load()) } @@ -93,7 +89,7 @@ func Test_WaitForControlPlaneReady_ReportsLastProbeFailureOnTimeout(t *testing.T ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) defer cancel() - err := waitForControlPlaneReady(ctx, t, client) + err := waitForControlPlaneReady(ctx, t, client, time.Millisecond) require.Error(t, err) assert.ErrorIs(t, err, context.DeadlineExceeded) assert.Contains(t, err.Error(), "last probe failure") diff --git a/test/step/deployexecutor.go b/test/step/deployexecutor.go index d8dd0c787c3..ad56ff1b6f2 100644 --- a/test/step/deployexecutor.go +++ b/test/step/deployexecutor.go @@ -76,10 +76,11 @@ type DeployExecutor struct { const ( defaultTransientRetryDelay = 30 * time.Second - // defaultTransientRetryBudget is roughly three times the longest - // control-plane outage observed in CI (about 3.5 minutes between the first - // etcd request timeout and the kube-apiserver serving again). - defaultTransientRetryBudget = 10 * time.Minute + // defaultTransientRetryBudget covers the longest control-plane outage + // observed in CI (about 3.5 minutes between the first etcd request timeout + // and the kube-apiserver serving again) with a little margin. It is an upper + // bound: effectiveRetryBudget shortens it to fit the test binary's deadline. + defaultTransientRetryBudget = 5 * time.Minute ) // transientImagePullErrorMarkers are substrings that indicate a container image @@ -239,11 +240,16 @@ func (d *DeployExecutor) Execute(ctx context.Context, t *testing.T, options test // rather than on attempts that cannot succeed. func (d *DeployExecutor) executeWithRetry(ctx context.Context, t *testing.T, deployFunc func() error) error { err := deployFunc() - if err == nil || d.ShouldRetry == nil || d.RetryBudget <= 0 { + if err == nil || d.ShouldRetry == nil { return err } - retryCtx, cancel := context.WithTimeout(ctx, d.RetryBudget) + budget := d.effectiveRetryBudget(t) + if budget <= 0 { + return err + } + + retryCtx, cancel := context.WithTimeout(ctx, budget) defer cancel() for attempt := 1; d.ShouldRetry(err); attempt++ { @@ -269,6 +275,34 @@ func (d *DeployExecutor) executeWithRetry(ctx context.Context, t *testing.T, dep return err } +// effectiveRetryBudget returns RetryBudget shortened so that retrying can never +// run the test binary out of time. +// +// The budget is per deploy step, so a control plane that never recovers makes +// every test pay it. The suites run in parallel batches - corerp-noncloud is 52 +// tests at -parallel 10 - and their `go test -timeout` is as low as 15 minutes +// (FUNCTIONALTEST_TIMEOUT in functional-test-noncloud.yaml, versus 60 minutes +// for the cloud suites). A fixed budget large enough to cover a multi-minute +// outage in one suite would blow the deadline in another, replacing clear +// per-test failures with an opaque panic and risking the loss of the +// diagnostics collected on failure. +// +// Spending at most half the remaining time leaves each subsequent batch half of +// what is left, so the deadline is approached but never reached, without +// needing a per-suite constant. +func (d *DeployExecutor) effectiveRetryBudget(t *testing.T) time.Duration { + deadline, ok := t.Deadline() + if !ok { + return d.RetryBudget + } + + if half := time.Until(deadline) / 2; half < d.RetryBudget { + return half + } + + return d.RetryBudget +} + // waitBeforeRetry waits out the retry delay and then blocks until the control // plane is ready, bounded by ctx. func (d *DeployExecutor) waitBeforeRetry(ctx context.Context, t *testing.T) error { diff --git a/test/step/deployexecutor_test.go b/test/step/deployexecutor_test.go index f2bde069ca2..c2796e31b80 100644 --- a/test/step/deployexecutor_test.go +++ b/test/step/deployexecutor_test.go @@ -293,6 +293,33 @@ func Test_ExecuteWithRetry_BudgetBoundsTheReadinessWait(t *testing.T) { assert.Less(t, time.Since(start), 5*time.Second) } +func Test_EffectiveRetryBudget_CappedByTestDeadline(t *testing.T) { + t.Parallel() + deadline, ok := t.Deadline() + if !ok { + t.Skip("go test was run without -timeout, so there is no deadline to cap against") + } + + // A budget far larger than the time the test binary has left must be cut + // down, so a broken cluster produces per-test failures rather than an + // opaque `go test -timeout` panic. + d := NewDeployExecutor("test.bicep") + d.RetryBudget = time.Hour + + budget := d.effectiveRetryBudget(t) + + assert.Less(t, budget, time.Until(deadline)) + assert.Positive(t, budget) +} + +func Test_EffectiveRetryBudget_UnchangedWhenItFitsTheDeadline(t *testing.T) { + t.Parallel() + d := NewDeployExecutor("test.bicep") + d.RetryBudget = time.Millisecond + + assert.Equal(t, time.Millisecond, d.effectiveRetryBudget(t)) +} + func Test_IsTransientImagePullError(t *testing.T) { // imagePullError mirrors how rad surfaces a transient image pull failure: // the ErrImagePull/timeout cause only appears inside a deeply nested From fe78092e97b65ea8c3f0f3c66ac8357332b21f70 Mon Sep 17 00:00:00 2001 From: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com> Date: Tue, 4 Aug 2026 12:55:12 -0400 Subject: [PATCH 4/7] Fix the cloud functional test timeout ordering 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> --- .github/workflows/functional-test-cloud.yaml | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/.github/workflows/functional-test-cloud.yaml b/.github/workflows/functional-test-cloud.yaml index 1076deefdf2..4fc253e43ab 100644 --- a/.github/workflows/functional-test-cloud.yaml +++ b/.github/workflows/functional-test-cloud.yaml @@ -64,8 +64,16 @@ env: CONTAINER_REGISTRY: ${{ vars.FUNCTIONAL_TEST_CONTAINER_REGISTRY }} # Container registry for storing Bicep recipe artifacts BICEP_RECIPE_REGISTRY: ${{ vars.FUNCTIONAL_TEST_BICEP_RECIPE_REGISTRY }} - # The radius functional test timeout - FUNCTIONALTEST_TIMEOUT: 60m + # The radius functional test timeout. This bounds `go test` and must stay + # comfortably below the job's timeout-minutes: the job also pays for ~10 + # minutes of setup (KinD cluster, Radius install, Helm charts, cloud logins) + # before the tests start, and must still have time afterwards to collect + # cluster diagnostics and upload artifacts. If `go test` is allowed to run to + # the job timeout, GitHub cancels the job mid-test and the post-test steps - + # including diagnostics collection - never run, which is exactly the evidence + # needed to debug a hang. The cloud legs normally spend about 5 minutes in + # `go test`, so this leaves a wide margin. + FUNCTIONALTEST_TIMEOUT: 30m # The Azure Location to store test resources AZURE_LOCATION: ${{ vars.AZURE_LOCATION }} # The base directory for storing test logs @@ -981,6 +989,12 @@ jobs: # Restore AWS Bicep types bicep restore ./test/functional-portable/corerp/cloud/resources/testdata/aws-logs-loggroup.bicep --force - name: Run functional tests + # A step-level timeout, unlike the job-level one, still lets the + # `if: always()` steps below run - so a hung suite is followed by + # diagnostics collection rather than a cancelled job. Kept above + # FUNCTIONALTEST_TIMEOUT so `go test` times out first and prints its + # goroutine dump. Mirrors the pattern in long-running-azure.yaml. + timeout-minutes: 35 run: | set -euo pipefail From e43e67b7ca3d080731c2713410ac7a6a9150bdd0 Mon Sep 17 00:00:00 2001 From: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com> Date: Tue, 4 Aug 2026 14:48:17 -0400 Subject: [PATCH 5/7] Address review feedback on tests and diagnostics stderr 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> --- .github/scripts/collect-cluster-diagnostics.sh | 7 ++++--- test/step/controlplane_test.go | 6 ++++++ test/step/deployexecutor_test.go | 17 ++++++++--------- 3 files changed, 18 insertions(+), 12 deletions(-) diff --git a/.github/scripts/collect-cluster-diagnostics.sh b/.github/scripts/collect-cluster-diagnostics.sh index 8fc2253357b..4af4bcd548e 100755 --- a/.github/scripts/collect-cluster-diagnostics.sh +++ b/.github/scripts/collect-cluster-diagnostics.sh @@ -78,9 +78,10 @@ collect_pod_state() { capture "${OUTPUT_DIR}/${PREFIX}-pod-states.log" kubectl describe pods -A # 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 + # reader. stderr goes to the pod-states log so a failed dump is still + # explained there rather than silently producing an empty file. + kubectl get pods -A -o yaml >"${OUTPUT_DIR}/${PREFIX}-pods.yaml" \ + 2>>"${OUTPUT_DIR}/${PREFIX}-pod-states.log" || true } collect_events() { diff --git a/test/step/controlplane_test.go b/test/step/controlplane_test.go index dba3039d0a8..07c2ae83b42 100644 --- a/test/step/controlplane_test.go +++ b/test/step/controlplane_test.go @@ -20,6 +20,7 @@ import ( "context" "net/http" "net/http/httptest" + "sync" "sync/atomic" "testing" "time" @@ -46,9 +47,12 @@ func newTestRESTClient(t *testing.T, handler http.Handler) rest.Interface { func Test_WaitForControlPlaneReady_ReturnsWhenAggregatedAPIServes(t *testing.T) { t.Parallel() + var mu sync.Mutex var paths []string client := newTestRESTClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() paths = append(paths, r.URL.Path) + mu.Unlock() w.WriteHeader(http.StatusOK) })) @@ -56,6 +60,8 @@ func Test_WaitForControlPlaneReady_ReturnsWhenAggregatedAPIServes(t *testing.T) // A single probe of the aggregated path is enough: it cannot succeed unless // the kube-apiserver and its aggregation layer are both serving. + mu.Lock() + defer mu.Unlock() assert.Equal(t, []string{radiusAggregatedAPIPath}, paths) } diff --git a/test/step/deployexecutor_test.go b/test/step/deployexecutor_test.go index c2796e31b80..627e8041bd0 100644 --- a/test/step/deployexecutor_test.go +++ b/test/step/deployexecutor_test.go @@ -209,7 +209,7 @@ func Test_ExecuteWithRetry_ZeroBudgetDisablesRetries(t *testing.T) { assert.Equal(t, int32(1), calls.Load()) } -func Test_ExecuteWithRetry_WaitsForControlPlaneBeforeRetrying(t *testing.T) { +func Test_ExecuteWithRetry_UnreadyControlPlaneStopsRetrying(t *testing.T) { t.Parallel() var calls atomic.Int32 var waits atomic.Int32 @@ -217,13 +217,11 @@ func Test_ExecuteWithRetry_WaitsForControlPlaneBeforeRetrying(t *testing.T) { d := NewDeployExecutor("test.bicep") d.RetryDelay = time.Millisecond d.ShouldRetry = func(error) bool { return true } - // The gate stands in for a control plane that is down for the first two - // probes: the retry must not be attempted until it reports ready. + // The gate stands in for a control plane that never came back within the + // budget. Retrying against it cannot succeed, so the loop must stop. d.waitForReady = func(ctx context.Context) error { - if waits.Add(1) < 3 { - return errors.New("kube-apiserver is not ready") - } - return nil + waits.Add(1) + return errors.New("kube-apiserver is not ready") } err := d.executeWithRetry(context.Background(), t, func() error { @@ -234,9 +232,10 @@ func Test_ExecuteWithRetry_WaitsForControlPlaneBeforeRetrying(t *testing.T) { return nil }) - // A failing gate ends the loop with the deployment error rather than - // retrying against an unavailable control plane. + // The deployment error is surfaced rather than the gate's error, and no + // retry is attempted against an unavailable control plane. require.Error(t, err) + assert.Equal(t, "connection reset by peer", err.Error()) assert.Equal(t, int32(1), calls.Load()) assert.Equal(t, int32(1), waits.Load()) } From ca8f99b81d281737c1da7796e4bf84f585632846 Mon Sep 17 00:00:00 2001 From: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com> Date: Tue, 4 Aug 2026 15:12:25 -0400 Subject: [PATCH 6/7] Correct a stale timeout value in the retry budget comment The effectiveRetryBudget comment still cited 60 minutes as the cloud suites' `go test -timeout`, but a later commit in this branch lowered FUNCTIONALTEST_TIMEOUT in functional-test-cloud.yaml to 30m. Comment only; the reasoning is unchanged, since the suites still do not share a deadline (15m noncloud versus 30m cloud). 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> --- test/step/deployexecutor.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/step/deployexecutor.go b/test/step/deployexecutor.go index ad56ff1b6f2..f36276b7167 100644 --- a/test/step/deployexecutor.go +++ b/test/step/deployexecutor.go @@ -281,7 +281,7 @@ func (d *DeployExecutor) executeWithRetry(ctx context.Context, t *testing.T, dep // The budget is per deploy step, so a control plane that never recovers makes // every test pay it. The suites run in parallel batches - corerp-noncloud is 52 // tests at -parallel 10 - and their `go test -timeout` is as low as 15 minutes -// (FUNCTIONALTEST_TIMEOUT in functional-test-noncloud.yaml, versus 60 minutes +// (FUNCTIONALTEST_TIMEOUT in functional-test-noncloud.yaml, versus 30 minutes // for the cloud suites). A fixed budget large enough to cover a multi-minute // outage in one suite would blow the deadline in another, replacing clear // per-test failures with an opaque panic and risking the loss of the From d03a3b5405b1a3f3a8f3c347b25f9a83830a862b Mon Sep 17 00:00:00 2001 From: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com> Date: Tue, 4 Aug 2026 15:23:08 -0400 Subject: [PATCH 7/7] Make the retry budget gate the delay-free fast path 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> --- test/step/deployexecutor.go | 6 +++++- test/step/deployexecutor_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/test/step/deployexecutor.go b/test/step/deployexecutor.go index f36276b7167..e52d32018df 100644 --- a/test/step/deployexecutor.go +++ b/test/step/deployexecutor.go @@ -318,7 +318,11 @@ func (d *DeployExecutor) waitBeforeRetry(ctx context.Context, t *testing.T) erro } if d.waitForReady == nil { - return nil + // ctx.Err() is nil while budget remains. Returning it rather than a + // bare nil matters when RetryDelay is also zero: with neither a delay + // nor a readiness gate to block on, nothing else would consult the + // deadline and the loop would keep starting attempts past the budget. + return ctx.Err() } return d.waitForReady(ctx) diff --git a/test/step/deployexecutor_test.go b/test/step/deployexecutor_test.go index 627e8041bd0..4c115c7ae08 100644 --- a/test/step/deployexecutor_test.go +++ b/test/step/deployexecutor_test.go @@ -319,6 +319,35 @@ func Test_EffectiveRetryBudget_UnchangedWhenItFitsTheDeadline(t *testing.T) { assert.Equal(t, time.Millisecond, d.effectiveRetryBudget(t)) } +func Test_ExecuteWithRetry_BudgetBoundsRetriesWithoutDelayOrReadinessGate(t *testing.T) { + t.Parallel() + var calls atomic.Int32 + + // With neither a retry delay nor a readiness gate, the budget is the only + // thing that can stop the loop, so waitBeforeRetry has to consult it on + // that path too. Without this the loop runs until the parent context or the + // test binary's timeout, not the budget. + d := NewDeployExecutor("test.bicep") + d.RetryDelay = 0 + d.RetryBudget = 20 * time.Millisecond + d.ShouldRetry = func(error) bool { return true } + d.waitForReady = nil + + start := time.Now() + err := d.executeWithRetry(context.Background(), t, func() error { + calls.Add(1) + // Keeps the attempt count (and the log volume) bounded while the + // budget elapses. + time.Sleep(5 * time.Millisecond) + return errors.New("connection reset by peer") + }) + + require.Error(t, err) + assert.Equal(t, "connection reset by peer", err.Error()) + assert.Less(t, time.Since(start), 5*time.Second) + assert.Greater(t, calls.Load(), int32(1)) +} + func Test_IsTransientImagePullError(t *testing.T) { // imagePullError mirrors how rad surfaces a transient image pull failure: // the ErrImagePull/timeout cause only appears inside a deeply nested