From 9b80e448ceb05047b5cf98d996e89cf1b51582c8 Mon Sep 17 00:00:00 2001 From: Cliff Schomburg Date: Fri, 10 Jul 2026 15:39:16 -0700 Subject: [PATCH 1/2] fix: make frontend cluster create idempotent to prevent CS duplicate collisions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- ...uster_cluster_service_create_controller.go | 66 +------------------ ..._cluster_service_create_controller_test.go | 11 ++-- frontend/pkg/frontend/cluster.go | 52 ++++++++++----- internal/ocm/client.go | 50 ++++++++++++++ 4 files changed, 94 insertions(+), 85 deletions(-) diff --git a/backend/pkg/controllers/clustercreation/cluster_cluster_service_create_controller.go b/backend/pkg/controllers/clustercreation/cluster_cluster_service_create_controller.go index 5181c30c4b6..a1a2c52719b 100644 --- a/backend/pkg/controllers/clustercreation/cluster_cluster_service_create_controller.go +++ b/backend/pkg/controllers/clustercreation/cluster_cluster_service_create_controller.go @@ -17,7 +17,6 @@ package clustercreation import ( "context" "fmt" - "strings" "time" arohcpv1alpha1 "github.com/openshift-online/ocm-sdk-go/arohcp/v1alpha1" @@ -126,7 +125,7 @@ func (c *clusterClusterServiceCreateSyncer) SyncOnce(ctx context.Context, key co tenantID := *subscription.Properties.TenantId mrg := cluster.CustomerProperties.Platform.ManagedResourceGroup - csCluster, err := c.findAROHCPClusterByAzureInfo(ctx, key.SubscriptionID, key.ResourceGroupName, key.HCPClusterName, tenantID, mrg) + csCluster, err := ocm.FindClusterByAzureInfo(ctx, c.clustersServiceClient, key.SubscriptionID, key.ResourceGroupName, key.HCPClusterName, tenantID, mrg) if err != nil { return utils.TrackError(err) } @@ -169,69 +168,6 @@ func (c *clusterClusterServiceCreateSyncer) createPreconditionDesiredVersionReso return false, nil } -// findAROHCPClusterByAzureInfo returns the Cluster Service cluster whose Azure -// metadata matches the given subscription, resource group, ARM resource name, -// tenant ID, and managed resource group name (MRG). -// It returns (nil, nil) when no such cluster exists. -// An error is returned if more than one cluster is returned matching the Azure metadata, as it should be unique. -func (c *clusterClusterServiceCreateSyncer) findAROHCPClusterByAzureInfo(ctx context.Context, subscriptionID, resourceGroupName, resourceName, tenantID, managedResourceGroupName string) (*arohcpv1alpha1.Cluster, error) { - // Subscription ID, resource group, and cluster name are lowercased when building the Cluster Service - // cluster (see withImmutableAttributes in convert.go). - wantSub := strings.ToLower(subscriptionID) - wantRG := strings.ToLower(resourceGroupName) - wantName := strings.ToLower(resourceName) - // Tenant ID and managed resource group are not lowercased in the OCM CS - // builder (see withImmutableAttributes in convert.go), we keep the casing as it is. - wantTenant := tenantID - wantMRG := managedResourceGroupName - search := c.clustersServiceClusterByAzureInfoSearchString(wantSub, wantRG, wantName, wantTenant, wantMRG) - matches, err := c.csClustersMatchingClusterByAzureInfo(ctx, c.clustersServiceClient.ListClusters(search), wantSub, wantRG, wantName, wantTenant, wantMRG) - if err != nil { - return nil, err - } - if len(matches) > 1 { - return nil, fmt.Errorf( - "cluster service returned %d clusters for one Azure resource (expected exactly 1): "+ - "subscription_id=%q resource_group=%q resource_name=%q tenant_id=%q managed_resource_group=%q", - len(matches), wantSub, wantRG, wantName, wantTenant, wantMRG, - ) - } - if len(matches) == 1 { - return matches[0], nil - } - return nil, nil -} - -func (c *clusterClusterServiceCreateSyncer) clustersServiceClusterByAzureInfoSearchString(wantSub, wantRG, wantName, wantTenant, wantMRG string) string { - return fmt.Sprintf( - "azure.subscription_id = '%s' and azure.resource_group_name = '%s' and azure.resource_name = '%s' and "+ - "azure.tenant_id = '%s' and azure.managed_resource_group_name = '%s'", - wantSub, wantRG, wantName, wantTenant, wantMRG, - ) -} - -func (c *clusterClusterServiceCreateSyncer) csClustersMatchingClusterByAzureInfo(ctx context.Context, it ocm.ClusterListIterator, wantSub, wantRG, wantName, wantTenant, wantMRG string) ([]*arohcpv1alpha1.Cluster, error) { - var res []*arohcpv1alpha1.Cluster - for csCluster := range it.Items(ctx) { - az := csCluster.Azure() - if az == nil { - continue - } - if az.SubscriptionID() != wantSub || - az.ResourceGroupName() != wantRG || - az.ResourceName() != wantName || - az.TenantID() != wantTenant || - az.ManagedResourceGroupName() != wantMRG { - continue - } - res = append(res, csCluster) - } - if err := it.GetError(); err != nil { - return nil, err - } - return res, nil -} - func (c *clusterClusterServiceCreateSyncer) createClusterServiceCluster(ctx context.Context, cluster *api.HCPOpenShiftCluster, serviceProviderCluster *api.ServiceProviderCluster, tenantID string) (*arohcpv1alpha1.Cluster, error) { logger := utils.LoggerFromContext(ctx) diff --git a/backend/pkg/controllers/clustercreation/cluster_cluster_service_create_controller_test.go b/backend/pkg/controllers/clustercreation/cluster_cluster_service_create_controller_test.go index bb631db9742..d2d8055dcb2 100644 --- a/backend/pkg/controllers/clustercreation/cluster_cluster_service_create_controller_test.go +++ b/backend/pkg/controllers/clustercreation/cluster_cluster_service_create_controller_test.go @@ -201,7 +201,7 @@ func TestClusterClusterServiceCreate_SyncOnce(t *testing.T) { setupMockCS: func(ctrl *gomock.Controller) ocm.ClusterServiceClientSpec { mockCS := ocm.NewMockClusterServiceClientSpec(ctrl) // Build the CS cluster with Azure fields matching the test cluster so it - // passes the csClustersMatchingClusterByAzureInfo filter. + // passes the FindClusterByAzureInfo filter. csCluster, err := arohcpv1alpha1.NewCluster(). HREF(testClusterServiceIDStr). Azure(arohcpv1alpha1.NewAzure(). @@ -276,7 +276,7 @@ func TestClusterClusterServiceCreate_SyncOnce(t *testing.T) { } } -func TestClusterClusterServiceCreate_findAROHCPClusterByAzureInfo(t *testing.T) { +func TestFindClusterByAzureInfo(t *testing.T) { azureTestCluster := func(t *testing.T, sub, rg, name, tenant, mrg string) *arohcpv1alpha1.Cluster { t.Helper() c, err := arohcpv1alpha1.NewCluster(). @@ -300,7 +300,9 @@ func TestClusterClusterServiceCreate_findAROHCPClusterByAzureInfo(t *testing.T) defaultMRG := "arohcp-mycluster-uuid" searchString := func(sub, rg, name, tenant, mrg string) string { - return (&clusterClusterServiceCreateSyncer{}).clustersServiceClusterByAzureInfoSearchString( + return fmt.Sprintf( + "azure.subscription_id = '%s' and azure.resource_group_name = '%s' and azure.resource_name = '%s' and "+ + "azure.tenant_id = '%s' and azure.managed_resource_group_name = '%s'", strings.ToLower(sub), strings.ToLower(rg), strings.ToLower(name), tenant, mrg, ) } @@ -496,8 +498,7 @@ func TestClusterClusterServiceCreate_findAROHCPClusterByAzureInfo(t *testing.T) ctrl := gomock.NewController(t) mockCS, want := tt.setupMockCS(t, ctrl, wantSearch) - s := &clusterClusterServiceCreateSyncer{clustersServiceClient: mockCS} - got, err := s.findAROHCPClusterByAzureInfo(ctx, sub, rg, resName, tenant, mrg) + got, err := ocm.FindClusterByAzureInfo(ctx, mockCS, sub, rg, resName, tenant, mrg) if tt.wantErr { require.Error(t, err) diff --git a/frontend/pkg/frontend/cluster.go b/frontend/pkg/frontend/cluster.go index 3652e88b648..ef2166641a6 100644 --- a/frontend/pkg/frontend/cluster.go +++ b/frontend/pkg/frontend/cluster.go @@ -369,26 +369,48 @@ func (f *Frontend) createHCPCluster(writer http.ResponseWriter, request *http.Re tenantID = *subscription.Properties.TenantId } - initialClusterProperties := map[string]string{} - if len(f.clusterServiceProvisionShard) != 0 { - initialClusterProperties[ocm.CSPropertyProvisionShardID] = f.clusterServiceProvisionShard - } - if f.clusterServiceNoopProvision { - initialClusterProperties[ocm.CSPropertyNoopProvision] = ocm.CSPropertyEnabled - } - if f.clusterServiceNoopDeprovision { - initialClusterProperties[ocm.CSPropertyNoopDeprovision] = ocm.CSPropertyEnabled - } - newClusterServiceClusterBuilder, newClusterServiceAutoscalerBuilder, err := ocm.BuildCSCluster(newInternalCluster.ID, tenantID, newInternalCluster, initialClusterProperties, nil, nil) - if err != nil { - return utils.TrackError(err) - } logger.Info(fmt.Sprintf("creating resource %s", newInternalCluster.ID)) - resultingClusterServiceCluster, err := f.clusterServiceClient.PostCluster(ctx, newClusterServiceClusterBuilder, newClusterServiceAutoscalerBuilder) + + // Check if CS already has a cluster with matching Azure metadata. This + // handles the retry-after-partial-failure scenario: a prior attempt may + // have created the CS cluster but failed to write the Cosmos documents, + // leaving an orphan in CS that would cause PostCluster to reject with + // "Duplicate ARO-HCP cluster name" (see ARO-27951). + resultingClusterServiceCluster, err := ocm.FindClusterByAzureInfo( + ctx, f.clusterServiceClient, + newInternalCluster.ID.SubscriptionID, + newInternalCluster.ID.ResourceGroupName, + newInternalCluster.Name, + tenantID, + newInternalCluster.CustomerProperties.Platform.ManagedResourceGroup, + ) if err != nil { return utils.TrackError(err) } + if resultingClusterServiceCluster == nil { + initialClusterProperties := map[string]string{} + if len(f.clusterServiceProvisionShard) != 0 { + initialClusterProperties[ocm.CSPropertyProvisionShardID] = f.clusterServiceProvisionShard + } + if f.clusterServiceNoopProvision { + initialClusterProperties[ocm.CSPropertyNoopProvision] = ocm.CSPropertyEnabled + } + if f.clusterServiceNoopDeprovision { + initialClusterProperties[ocm.CSPropertyNoopDeprovision] = ocm.CSPropertyEnabled + } + newClusterServiceClusterBuilder, newClusterServiceAutoscalerBuilder, err := ocm.BuildCSCluster(newInternalCluster.ID, tenantID, newInternalCluster, initialClusterProperties, nil, nil) + if err != nil { + return utils.TrackError(err) + } + resultingClusterServiceCluster, err = f.clusterServiceClient.PostCluster(ctx, newClusterServiceClusterBuilder, newClusterServiceAutoscalerBuilder) + if err != nil { + return utils.TrackError(err) + } + } else { + logger.Info("Reusing existing Cluster Service cluster found by Azure metadata", "csClusterHREF", resultingClusterServiceCluster.HREF()) + } + csID, err := api.NewInternalID(resultingClusterServiceCluster.HREF()) if err != nil { return utils.TrackError(err) diff --git a/internal/ocm/client.go b/internal/ocm/client.go index 32fc69ae56a..4e4fc03b7ed 100644 --- a/internal/ocm/client.go +++ b/internal/ocm/client.go @@ -845,3 +845,53 @@ func ConvertOpenShiftVersionAddPrefix(v string) string { } return v } + +// FindClusterByAzureInfo searches Cluster Service for a cluster matching the +// given Azure metadata (subscription, resource group, resource name, tenant, +// managed resource group). Returns (nil, nil) when no match exists. Returns an +// error if more than one matching cluster is found. +func FindClusterByAzureInfo(ctx context.Context, client ClusterServiceClientSpec, subscriptionID, resourceGroupName, resourceName, tenantID, managedResourceGroupName string) (*arohcpv1alpha1.Cluster, error) { + wantSub := strings.ToLower(subscriptionID) + wantRG := strings.ToLower(resourceGroupName) + wantName := strings.ToLower(resourceName) + wantTenant := tenantID + wantMRG := managedResourceGroupName + + search := fmt.Sprintf( + "azure.subscription_id = '%s' and azure.resource_group_name = '%s' and azure.resource_name = '%s' and "+ + "azure.tenant_id = '%s' and azure.managed_resource_group_name = '%s'", + wantSub, wantRG, wantName, wantTenant, wantMRG, + ) + + it := client.ListClusters(search) + var matches []*arohcpv1alpha1.Cluster + for csCluster := range it.Items(ctx) { + az := csCluster.Azure() + if az == nil { + continue + } + if az.SubscriptionID() != wantSub || + az.ResourceGroupName() != wantRG || + az.ResourceName() != wantName || + az.TenantID() != wantTenant || + az.ManagedResourceGroupName() != wantMRG { + continue + } + matches = append(matches, csCluster) + } + if err := it.GetError(); err != nil { + return nil, err + } + + if len(matches) > 1 { + return nil, fmt.Errorf( + "cluster service returned %d clusters for one Azure resource (expected exactly 1): "+ + "subscription_id=%q resource_group=%q resource_name=%q tenant_id=%q managed_resource_group=%q", + len(matches), wantSub, wantRG, wantName, wantTenant, wantMRG, + ) + } + if len(matches) == 1 { + return matches[0], nil + } + return nil, nil +} From 62b9ed101f0988dcc80827b3dd3ec04ca18a7412 Mon Sep 17 00:00:00 2001 From: Cliff Schomburg Date: Mon, 13 Jul 2026 18:32:27 -0700 Subject: [PATCH 2/2] =?UTF-8?q?fix:=20address=20review=20feedback=20?= =?UTF-8?q?=E2=80=94=20mixed-result=20test=20case=20and=20log=20placement?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- ...er_cluster_service_create_controller_test.go | 17 +++++++++++++++++ frontend/pkg/frontend/cluster.go | 3 +-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/backend/pkg/controllers/clustercreation/cluster_cluster_service_create_controller_test.go b/backend/pkg/controllers/clustercreation/cluster_cluster_service_create_controller_test.go index d2d8055dcb2..2f694cbc53d 100644 --- a/backend/pkg/controllers/clustercreation/cluster_cluster_service_create_controller_test.go +++ b/backend/pkg/controllers/clustercreation/cluster_cluster_service_create_controller_test.go @@ -453,6 +453,23 @@ func TestFindClusterByAzureInfo(t *testing.T) { return mock, nil }, }, + { + name: "found among non-matching clusters", + setupMockCS: func(t *testing.T, ctrl *gomock.Controller, wantSearch string) (ocm.ClusterServiceClientSpec, *arohcpv1alpha1.Cluster) { + wantSub := strings.ToLower(defaultSub) + wantRG := strings.ToLower(defaultRG) + wantName := strings.ToLower(defaultResName) + match := azureTestCluster(t, wantSub, wantRG, wantName, defaultTenant, defaultMRG) + differentSub := azureTestCluster(t, "00000000-0000-0000-0000-000000000099", wantRG, wantName, defaultTenant, defaultMRG) + differentRG := azureTestCluster(t, wantSub, "other-rg", wantName, defaultTenant, defaultMRG) + differentName := azureTestCluster(t, wantSub, wantRG, "other-cluster", defaultTenant, defaultMRG) + mock := ocm.NewMockClusterServiceClientSpec(ctrl) + mock.EXPECT(). + ListClusters(wantSearch). + Return(ocm.NewSimpleClusterListIterator([]*arohcpv1alpha1.Cluster{differentSub, differentRG, match, differentName}, nil)) + return mock, match + }, + }, { name: "multiple matches error", setupMockCS: func(t *testing.T, ctrl *gomock.Controller, wantSearch string) (ocm.ClusterServiceClientSpec, *arohcpv1alpha1.Cluster) { diff --git a/frontend/pkg/frontend/cluster.go b/frontend/pkg/frontend/cluster.go index ef2166641a6..997d09ac3d3 100644 --- a/frontend/pkg/frontend/cluster.go +++ b/frontend/pkg/frontend/cluster.go @@ -369,8 +369,6 @@ func (f *Frontend) createHCPCluster(writer http.ResponseWriter, request *http.Re tenantID = *subscription.Properties.TenantId } - logger.Info(fmt.Sprintf("creating resource %s", newInternalCluster.ID)) - // Check if CS already has a cluster with matching Azure metadata. This // handles the retry-after-partial-failure scenario: a prior attempt may // have created the CS cluster but failed to write the Cosmos documents, @@ -389,6 +387,7 @@ func (f *Frontend) createHCPCluster(writer http.ResponseWriter, request *http.Re } if resultingClusterServiceCluster == nil { + logger.Info(fmt.Sprintf("creating resource %s", newInternalCluster.ID)) initialClusterProperties := map[string]string{} if len(f.clusterServiceProvisionShard) != 0 { initialClusterProperties[ocm.CSPropertyProvisionShardID] = f.clusterServiceProvisionShard