fix(test): wait for control-plane recovery before retrying deploys

.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
@@ -983,6 +991,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
 

.github/workflows/long-running-azure.yaml

@@ -452,21 +452,16 @@ 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 post-mortem snapshot. The script records each command's
+        # failure inline and keeps going, because the failures worth debugging are
+        # usually the ones where the control plane is unreachable.
         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}"
+        env:
+          RADIUS_CONTAINER_LOG_PATH: ${{ github.workspace }}/${{ env.RADIUS_CONTAINER_LOG_BASE }}
+          DIAGNOSTICS_NAME: all
+        run: ./.github/scripts/collect-cluster-diagnostics.sh
 
       - name: Upload container logs
         if: always()

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 5 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(5*time.Minute, 60*time.Second, isTransientCloudDeployError),
 			SkipObjectValidation: true,
 			RPResources: &validation.RPResourceSet{
 				Resources: []validation.RPResource{
@@ -188,15 +189,15 @@ var transientAzureErrorMarkers = []string{
 	"ContainerGroupQuotaReached",
 }
 
-// isTransientAzureError returns true if the error is a known transient Azure
-// error that may succeed on retry. It delegates to step.ErrorContainsAny, which
-// flattens the nested ARM error details rad surfaces inside a CLIError so the
-// match covers causes such as the ACI container group quota error.
-func isTransientAzureError(err error) bool {
-	return step.ErrorContainsAny(err, transientAzureErrorMarkers...)
+// isTransientCloudDeployError extends the default deployment predicate with
+// Azure-specific failures. Custom retry configuration must retain the default
+// image-pull and control-plane recovery behavior.
+func isTransientCloudDeployError(err error) bool {
+	return step.IsTransientDeployError(err) ||
+		step.ErrorContainsAny(err, transientAzureErrorMarkers...)
 }
 
-func Test_isTransientAzureError(t *testing.T) {
+func Test_isTransientCloudDeployError(t *testing.T) {
 	// aciQuotaError mirrors how rad surfaces an ACI quota failure: the quota
 	// error code only appears inside a deeply nested details[].message field,
 	// while the top-level code/message returned by CLIError.Error() is the
@@ -258,13 +259,15 @@ func Test_isTransientAzureError(t *testing.T) {
 		{name: "nested ACI quota error", err: aciQuotaError, expected: true},
 		{name: "nested managed identity error", err: msiError, expected: true},
 		{name: "plain transient error string", err: errors.New("deployment failed: ManagedServiceIdentityNotFound"), expected: true},
+		{name: "apiserver connection reset", err: errors.New("read: connection reset by peer"), expected: true},
+		{name: "apiserver restart response", err: errors.New("before all known HTTP paths have been installed"), expected: true},
 		{name: "non-transient CLIError", err: nonTransientError, expected: false},
 		{name: "unrelated error", err: errors.New("connection refused"), expected: false},
 	}
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
-			require.Equal(t, tc.expected, isTransientAzureError(tc.err))
+			require.Equal(t, tc.expected, isTransientCloudDeployError(tc.err))
 		})
 	}
 }

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(5*time.Minute, 60*time.Second, isTransientCloudDeployError),
 			RPResources: &validation.RPResourceSet{
 				Resources: []validation.RPResource{
 					{Name: recipePackName, Type: validation.CoreRecipePacksResource},

test/step/controlplane.go

@@ -0,0 +1,101 @@
+/*
+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 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 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
+// 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, 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, pollInterval time.Duration) error {
+	for {
+		lastErr := probe(ctx, client, radiusAggregatedAPIPath)
+		if lastErr == nil {
+			return nil
+		}
+
+		t.Logf("waiting for the Radius control plane to become ready: %v", lastErr)
+
+		timer := time.NewTimer(pollInterval)
+		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)
+		}
+	}
+}
+
+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()
+}

test/step/controlplane_test.go

@@ -0,0 +1,107 @@
+/*
+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"
+	"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_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)
+	}))
+
+	require.NoError(t, waitForControlPlaneReady(t.Context(), 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.
+	mu.Lock()
+	defer mu.Unlock()
+	assert.Equal(t, []string{radiusAggregatedAPIPath}, paths)
+}
+
+func Test_WaitForControlPlaneReady_PollsUntilHealthy(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) {
+		if requests.Add(1) < 3 {
+			w.WriteHeader(http.StatusServiceUnavailable)
+			return
+		}
+		w.WriteHeader(http.StatusOK)
+	}))
+
+	ctx, cancel := context.WithTimeout(t.Context(), 30*time.Second)
+	defer cancel()
+
+	require.NoError(t, waitForControlPlaneReady(ctx, t, client, time.Millisecond))
+	assert.Equal(t, int32(3), requests.Load())
+}
+
+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(t.Context(), 50*time.Millisecond)
+	defer cancel()
+
+	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")
+}
+
+func Test_NewControlPlaneReadyWaiter_NilClientDisablesGate(t *testing.T) {
+	t.Parallel()
+	assert.Nil(t, newControlPlaneReadyWaiter(t, nil))
+}