Skip to content

Commit 4a5969d

Browse files
cadenmarchesecadenmarchese
authored
handle empty failedProvisioningState during update or adminUpdate (#4794)
1 parent 3534314 commit 4a5969d

4 files changed

Lines changed: 138 additions & 5 deletions

File tree

‎pkg/backend/openshiftcluster.go‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -305,9 +305,15 @@ func (ocb *openShiftClusterBackend) endLease(ctx context.Context, log *logrus.En
305305
_, err = ocb.dbOpenShiftClusters.EndLease(ctx, doc.Key, provisioningState, failedProvisioningState, adminUpdateError)
306306
}()
307307

308-
if initialProvisioningState != api.ProvisioningStateAdminUpdating &&
309-
provisioningState == api.ProvisioningStateFailed {
310-
failedProvisioningState = initialProvisioningState
308+
if provisioningState == api.ProvisioningStateFailed {
309+
if initialProvisioningState == api.ProvisioningStateAdminUpdating {
310+
failedProvisioningState = doc.OpenShiftCluster.Properties.FailedProvisioningState
311+
if failedProvisioningState == "" {
312+
failedProvisioningState = api.ProvisioningStateAdminUpdating
313+
}
314+
} else {
315+
failedProvisioningState = initialProvisioningState
316+
}
311317
}
312318

313319
// If cluster is in the non-terminal state we are still in the same
@@ -327,6 +333,9 @@ func (ocb *openShiftClusterBackend) endLease(ctx context.Context, log *logrus.En
327333
provisioningState = doc.OpenShiftCluster.Properties.LastProvisioningState
328334
}
329335
failedProvisioningState = doc.OpenShiftCluster.Properties.FailedProvisioningState
336+
if failedProvisioningState == "" && backendErr != nil {
337+
failedProvisioningState = api.ProvisioningStateAdminUpdating
338+
}
330339

331340
if backendErr == nil {
332341
adminUpdateError = pointerutils.ToPtr("")

‎pkg/backend/openshiftcluster_test.go‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -510,6 +510,72 @@ func TestBackendTry(t *testing.T) {
510510
manager.EXPECT().AdminUpdate(gomock.Any()).Return(errors.New("oh no!"))
511511
},
512512
},
513+
{
514+
name: "StateAdminUpdating run failure with empty failedProvisioningState sets it to AdminUpdating",
515+
fixture: func(f *testdatabase.Fixture) {
516+
f.AddOpenShiftClusterDocuments(&api.OpenShiftClusterDocument{
517+
Key: strings.ToLower(resourceID),
518+
OpenShiftCluster: &api.OpenShiftCluster{
519+
ID: resourceID,
520+
Name: "resourceName",
521+
Type: "Microsoft.RedHatOpenShift/OpenShiftClusters",
522+
Location: "location",
523+
Properties: api.OpenShiftClusterProperties{
524+
ProvisioningState: api.ProvisioningStateAdminUpdating,
525+
LastProvisioningState: api.ProvisioningStateSucceeded,
526+
FailedProvisioningState: "",
527+
MaintenanceTask: api.MaintenanceTaskEverything,
528+
MaintenanceState: api.MaintenanceStateUnplanned,
529+
NetworkProfile: api.NetworkProfile{
530+
PodCIDR: "10.128.0.0/14",
531+
ServiceCIDR: "172.30.0.0/16",
532+
PreconfiguredNSG: api.PreconfiguredNSGDisabled,
533+
OutboundType: api.OutboundTypeLoadbalancer,
534+
LoadBalancerProfile: &api.LoadBalancerProfile{
535+
ManagedOutboundIPs: &api.ManagedOutboundIPs{
536+
Count: 0,
537+
},
538+
},
539+
},
540+
},
541+
},
542+
})
543+
f.AddSubscriptionDocuments(&api.SubscriptionDocument{
544+
ID: mockSubID,
545+
})
546+
},
547+
checker: func(c *testdatabase.Checker) {
548+
c.AddOpenShiftClusterDocuments(&api.OpenShiftClusterDocument{
549+
Key: strings.ToLower(resourceID),
550+
OpenShiftCluster: &api.OpenShiftCluster{
551+
ID: resourceID,
552+
Name: "resourceName",
553+
Type: "Microsoft.RedHatOpenShift/OpenShiftClusters",
554+
Location: "location",
555+
Properties: api.OpenShiftClusterProperties{
556+
ProvisioningState: api.ProvisioningStateSucceeded,
557+
FailedProvisioningState: api.ProvisioningStateAdminUpdating,
558+
LastAdminUpdateError: "admin update failed!",
559+
MaintenanceState: api.MaintenanceStateUnplanned,
560+
NetworkProfile: api.NetworkProfile{
561+
PodCIDR: "10.128.0.0/14",
562+
ServiceCIDR: "172.30.0.0/16",
563+
PreconfiguredNSG: api.PreconfiguredNSGDisabled,
564+
OutboundType: api.OutboundTypeLoadbalancer,
565+
LoadBalancerProfile: &api.LoadBalancerProfile{
566+
ManagedOutboundIPs: &api.ManagedOutboundIPs{
567+
Count: 0,
568+
},
569+
},
570+
},
571+
},
572+
},
573+
})
574+
},
575+
mocks: func(manager *mock_cluster.MockInterface, dbOpenShiftClusters database.OpenShiftClusters) {
576+
manager.EXPECT().AdminUpdate(gomock.Any()).Return(errors.New("admin update failed!"))
577+
},
578+
},
513579
{
514580
name: "StateDeleting success deletes the document",
515581
fixture: func(f *testdatabase.Fixture) {

‎pkg/frontend/openshiftcluster_putorpatch.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -150,9 +150,9 @@ func (f *frontend) _putOrPatchOpenShiftCluster(ctx context.Context, log *logrus.
150150
switch doc.OpenShiftCluster.Properties.FailedProvisioningState {
151151
case api.ProvisioningStateCreating:
152152
return nil, api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeRequestNotAllowed, "", "Request is not allowed on cluster whose creation failed. Delete the cluster.")
153-
case api.ProvisioningStateUpdating:
153+
case api.ProvisioningStateAdminUpdating, api.ProvisioningStateUpdating, "":
154154
// allow: a previous failure to update should not prevent a new
155-
// operation.
155+
// operation. Empty failedProvisioningState is treated as update failure
156156
case api.ProvisioningStateDeleting:
157157
return nil, api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeRequestNotAllowed, "", "Request is not allowed on cluster whose deletion failed. Delete the cluster.")
158158
default:

‎pkg/frontend/openshiftcluster_putorpatch_test.go‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -977,6 +977,39 @@ func TestPutorPatchOpenShiftClusterUpdatePut(t *testing.T) {
977977
},
978978
wantError: "400: RequestNotAllowed: : Request is not allowed on cluster whose deletion failed. Delete the cluster.",
979979
},
980+
{
981+
name: "update a cluster from failed with empty failedProvisioningState",
982+
request: func() *v20240812preview.OpenShiftCluster {
983+
cluster := getServicePrincipalOpenShiftClusterRequest()
984+
cluster.Properties.NetworkProfile.OutboundType = v20240812preview.OutboundTypeLoadbalancer
985+
cluster.Properties.NetworkProfile.PreconfiguredNSG = v20240812preview.PreconfiguredNSGDisabled
986+
cluster.Properties.NetworkProfile.LoadBalancerProfile = &v20240812preview.LoadBalancerProfile{
987+
ManagedOutboundIPs: &v20240812preview.ManagedOutboundIPs{
988+
Count: 1,
989+
},
990+
}
991+
return cluster
992+
},
993+
fixture: func(f *testdatabase.Fixture) {
994+
f.AddSubscriptionDocuments(mockSubscriptionDocument)
995+
f.AddOpenShiftClusterDocuments(getExistingServicePrincipalOpenShiftClusterDocument(api.ProvisioningStateFailed, "", ""))
996+
},
997+
wantSystemDataEnriched: true,
998+
wantAsync: true,
999+
wantStatusCode: http.StatusOK,
1000+
wantDocuments: func(checker *testdatabase.Checker) {
1001+
checker.AddAsyncOperationDocuments(getAsynchronousOperationDocument(api.ProvisioningStateUpdating, api.ProvisioningStateUpdating))
1002+
doc := getExistingServicePrincipalOpenShiftClusterDocument(api.ProvisioningStateUpdating, "", "")
1003+
doc.OpenShiftCluster.Properties.NetworkProfile.LoadBalancerProfile.EffectiveOutboundIPs = []api.EffectiveOutboundIP{}
1004+
doc.OpenShiftCluster.Properties.LastProvisioningState = api.ProvisioningStateFailed
1005+
checker.AddOpenShiftClusterDocuments(doc)
1006+
},
1007+
wantResponse: func() *v20240812preview.OpenShiftCluster {
1008+
response := getExistingServicePrincipalOpenShiftClusterResponse()
1009+
response.Properties.ProvisioningState = v20240812preview.ProvisioningStateUpdating
1010+
return response
1011+
},
1012+
},
9801013
{
9811014
name: "update a Workload Identity cluster from succeeded",
9821015
request: func() *v20240812preview.OpenShiftCluster {
@@ -1570,6 +1603,31 @@ func TestPutorPatchOpenShiftClusterUpdatePatch(t *testing.T) {
15701603
},
15711604
wantError: "400: RequestNotAllowed: : Request is not allowed on cluster whose deletion failed. Delete the cluster.",
15721605
},
1606+
{
1607+
name: "update a cluster from failed with empty failedProvisioningState",
1608+
request: func() *v20240812preview.OpenShiftCluster {
1609+
return &v20240812preview.OpenShiftCluster{}
1610+
},
1611+
fixture: func(f *testdatabase.Fixture) {
1612+
f.AddSubscriptionDocuments(mockSubscriptionDocument)
1613+
f.AddOpenShiftClusterDocuments(getExistingServicePrincipalOpenShiftClusterDocument(api.ProvisioningStateFailed, "", ""))
1614+
},
1615+
wantSystemDataEnriched: true,
1616+
wantAsync: true,
1617+
wantStatusCode: http.StatusOK,
1618+
wantDocuments: func(checker *testdatabase.Checker) {
1619+
checker.AddAsyncOperationDocuments(getAsynchronousOperationDocument(api.ProvisioningStateUpdating, api.ProvisioningStateUpdating))
1620+
doc := getExistingServicePrincipalOpenShiftClusterDocument(api.ProvisioningStateUpdating, "", "")
1621+
doc.OpenShiftCluster.Properties.NetworkProfile.LoadBalancerProfile.EffectiveOutboundIPs = []api.EffectiveOutboundIP{}
1622+
doc.OpenShiftCluster.Properties.LastProvisioningState = api.ProvisioningStateFailed
1623+
checker.AddOpenShiftClusterDocuments(doc)
1624+
},
1625+
wantResponse: func() *v20240812preview.OpenShiftCluster {
1626+
response := getExistingServicePrincipalOpenShiftClusterResponse()
1627+
response.Properties.ProvisioningState = v20240812preview.ProvisioningStateUpdating
1628+
return response
1629+
},
1630+
},
15731631
} {
15741632
t.Run(tt.name, func(t *testing.T) {
15751633
ti := newTestInfra(t).

0 commit comments

Comments
 (0)