fix: make frontend cluster create idempotent to prevent CS duplicate collisions - #6025
Cliff Schomburg (cssjr) wants to merge 2 commits into
Conversation
…collisions The frontend's createHCPCluster() performs a two-phase write: it creates the cluster in Cluster Service (CS) first, then writes to Cosmos DB. If the Cosmos write fails, CS has the cluster but the RP doesn't — a split-brain state. When the Azure SDK automatically retries the failed 500, the retry calls PostCluster again and CS rejects with "Duplicate ARO-HCP cluster name" (400 InvalidRequestContent). This was hitting ~7.2% of CI E2E runs (ARO-27951) and was confirmed in a Stage incident (ARO-17939). Fix: before calling PostCluster, search CS by Azure metadata (subscription, resource group, resource name, tenant, managed resource group) via a new shared FindClusterByAzureInfo function. If an orphaned CS cluster from a prior failed attempt is found, reuse it instead of creating a duplicate. This makes the create path idempotent on retry. Also refactors the backend's ClusterClusterServiceCreate controller to use the same shared function, eliminating duplicated search logic. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: cssjr The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Skipping CI for Draft Pull Request. |
|
/test all |
|
/assign copilot |
|
Cliff Schomburg (@cssjr): GitHub didn't allow me to assign the following users: copilot. Note that only Azure members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/cc copilot |
|
/retest |
1 similar comment
|
/retest |
Simon Wiencki (swiencki)
left a comment
Simon Wiencki (swiencki)
left a comment
There was a problem hiding this comment.
backend/pkg/controllers/clustercreation/cluster_cluster_service_create_controller_test.go - A second cluster in a different bucket asserting no manifest is created would prove the bucketing logic actually excludes foreign work.
|
/retest |
There was a problem hiding this comment.
Pull request overview
This PR addresses intermittent CI/E2E failures caused by duplicate ARO-HCP cluster name collisions when a retry occurs after a partial create failure (CS create succeeds but Cosmos write fails). It makes the frontend create path idempotent by reusing an existing Cluster Service cluster when Azure-identifying metadata matches, and deduplicates the same lookup logic already used by the backend controller into a shared internal/ocm helper.
Changes:
- Add
ocm.FindClusterByAzureInfohelper to locate an existing CS cluster by Azure metadata (and error on unexpected duplicates). - Update the frontend
createHCPCluster()flow to “find-or-create” the CS cluster before proceeding with Cosmos writes, preventing duplicate-name collisions on retries. - Refactor the backend controller to use the shared helper and adjust tests accordingly.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/ocm/client.go | Adds shared FindClusterByAzureInfo helper for CS idempotency lookups by Azure metadata. |
| frontend/pkg/frontend/cluster.go | Uses the shared lookup to reuse an existing CS cluster before attempting PostCluster. |
| backend/pkg/controllers/clustercreation/cluster_cluster_service_create_controller.go | Replaces controller-local lookup logic with the shared ocm.FindClusterByAzureInfo. |
| backend/pkg/controllers/clustercreation/cluster_cluster_service_create_controller_test.go | Updates tests to validate the shared helper behavior and new call site. |
Add test case "found among non-matching clusters" that verifies FindClusterByAzureInfo correctly selects the matching cluster when CS returns a mix of matching and non-matching clusters in the same response. (swiencki review feedback) Move the "creating resource" log line inside the create branch so it only logs when actually creating, not when reusing an existing CS cluster. (Copilot review feedback) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Simon Wiencki (@swiencki) good call out. Added test "found among non-matching clusters" to address. |
|
/retest |
|
We are removing all cluster-service calls from the frontend. The PR doing so has tested green and is tagged for merge: #6121 /close |
|
David Eads (@deads2k): Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
| return utils.TrackError(err) | ||
| } | ||
| } else { | ||
| logger.Info("Reusing existing Cluster Service cluster found by Azure metadata", "csClusterHREF", resultingClusterServiceCluster.HREF()) |
There was a problem hiding this comment.
we don't need this PR anymore, but I am curious what you think should happen if hte content in the existing cluster-service instance does not match the new create call.
There was a problem hiding this comment.
If there wasn't a 100% match, it was going to create a new one (not reuse the old one). My understanding was a partial match was not duplicative so a new one could be created without conflict/idempotentcy issues.
Summary
createHCPCluster()path before callingPostCluster, preventing split-brain when Cosmos write fails after CS create succeedsFindClusterByAzureInfointo a sharedocmpackage function, deduplicating logic between frontend and backend controllerRoot Cause
The frontend's
createHCPCluster()performs a two-phase write that is not atomic:PostClusterto Cluster Service (CS) — creates the cluster recordIf Phase 2 fails (transient Cosmos error), the system enters a split-brain state: CS has the cluster, Cosmos does not. When the Azure SDK automatically retries the failed 500:
CreateOrUpdateHCPClusterchecks Cosmos → not found → routes tocreateHCPClusterPostClusterto CS → CS rejects: "Duplicate ARO-HCP cluster name" (HTTP 400)InvalidRequestContent→ test/request failsThis was confirmed by an identical Stage incident documented in ARO-17939 (comment by Mike Gahagan, 2026-05-20).
Fix
Before calling
PostCluster, the frontend now searches CS for an existing cluster with matching Azure metadata (subscription, resource group, resource name, tenant, managed resource group) viaocm.FindClusterByAzureInfo. If an orphaned CS cluster from a prior failed attempt is found, it is reused instead of creating a duplicate.The backend's
ClusterClusterServiceCreatecontroller already had this idempotency pattern — this PR extracts it into a shared function and applies it to the frontend as well.Test plan
go buildfor all affected packages (internal/ocm, frontend, backend)go test ./internal/ocm/...— passesgo test ./frontend/...— passesgo test ./backend/...— all 20+ controller test packages passmake lint— passesmake test-integration)branch-ci-Azure-ARO-HCP-main-e2e-integration-e2e-parallelsuccess rate after merge🤖 Generated with Claude Code