Merge pull request #4148 from nojnhuh/aso-no-wait-for-ready

don't wait for ASO resources to be ready for updates
kubernetes-sigs · Oct 17, 2023 · ec021f5 · ec021f5
2 parents 65eb3d5 + eae6093
commit ec021f5
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 3 deletions.
diff --git a/azure/services/aso/aso.go b/azure/services/aso/aso.go
@@ -81,6 +81,7 @@ func (r *reconciler[T]) CreateOrUpdateResource(ctx context.Context, spec azure.A
 
 	log = log.WithValues("service", serviceName, "resource", resourceName, "namespace", resourceNamespace)
 
+	var readyErr error
 	var adopt bool
 	var existing T
 	var zero T // holds the zero value, to be returned with non-nil errors.
@@ -105,7 +106,6 @@ func (r *reconciler[T]) CreateOrUpdateResource(ctx context.Context, spec azure.A
 		if !readyExists {
 			return zero, azure.WithTransientError(errors.New("ready status unknown"), requeueInterval)
 		}
-		var readyErr error
 		if cond := conds[i]; cond.Status != metav1.ConditionTrue {
 			switch {
 			case cond.Reason == conditions.ReasonAzureResourceNotFound.Name &&
@@ -133,9 +133,10 @@ func (r *reconciler[T]) CreateOrUpdateResource(ctx context.Context, spec azure.A
 
 			if readyErr != nil {
 				if conds[i].Severity == conditions.ConditionSeverityError {
-					return zero, azure.WithTerminalError(readyErr)
+					readyErr = azure.WithTerminalError(readyErr)
+				} else {
+					readyErr = azure.WithTransientError(readyErr, requeueInterval)
 				}
-				return zero, azure.WithTransientError(readyErr, requeueInterval)
 			}
 		}
 	}
@@ -200,6 +201,11 @@ func (r *reconciler[T]) CreateOrUpdateResource(ctx context.Context, spec azure.A
 
 	diff := cmp.Diff(existing, parameters)
 	if diff == "" {
+		if readyErr != nil {
+			// Only return this error when the resource is up to date in order to permit updates from
+			// Parameters which may fix the resource's current state.
+			return zero, readyErr
+		}
 		log.V(2).Info("resource up to date")
 		return existing, nil
 	}

diff --git a/azure/services/aso/aso_test.go b/azure/services/aso/aso_test.go
@@ -174,6 +174,10 @@ func TestCreateOrUpdateResource(t *testing.T) {
 				Namespace: "namespace",
 			},
 		})
+		specMock.EXPECT().Parameters(gomockinternal.AContext(), gomock.Not(gomock.Nil())).DoAndReturn(func(_ context.Context, group *asoresourcesv1.ResourceGroup) (*asoresourcesv1.ResourceGroup, error) {
+			return group, nil
+		})
+		specMock.EXPECT().WasManaged(gomock.Any()).Return(false)
 
 		ctx := context.Background()
 		g.Expect(c.Create(ctx, &asoresourcesv1.ResourceGroup{
@@ -183,6 +187,9 @@ func TestCreateOrUpdateResource(t *testing.T) {
 				Labels: map[string]string{
 					infrav1.OwnedByClusterLabelKey: clusterName,
 				},
+				Annotations: map[string]string{
+					asoannotations.PerResourceSecret: "cluster-aso-secret",
+				},
 			},
 			Status: asoresourcesv1.ResourceGroup_STATUS{
 				Conditions: []conditions.Condition{
@@ -202,6 +209,7 @@ func TestCreateOrUpdateResource(t *testing.T) {
 		var recerr azure.ReconcileError
 		g.Expect(errors.As(err, &recerr)).To(BeTrue())
 		g.Expect(recerr.IsTransient()).To(BeTrue())
+		g.Expect(recerr.IsTerminal()).To(BeFalse())
 	})
 
 	t.Run("resource is not ready in reconciling state", func(t *testing.T) {
@@ -222,6 +230,10 @@ func TestCreateOrUpdateResource(t *testing.T) {
 				Namespace: "namespace",
 			},
 		})
+		specMock.EXPECT().Parameters(gomockinternal.AContext(), gomock.Not(gomock.Nil())).DoAndReturn(func(_ context.Context, group *asoresourcesv1.ResourceGroup) (*asoresourcesv1.ResourceGroup, error) {
+			return group, nil
+		})
+		specMock.EXPECT().WasManaged(gomock.Any()).Return(false)
 
 		ctx := context.Background()
 		g.Expect(c.Create(ctx, &asoresourcesv1.ResourceGroup{
@@ -231,6 +243,9 @@ func TestCreateOrUpdateResource(t *testing.T) {
 				Labels: map[string]string{
 					infrav1.OwnedByClusterLabelKey: clusterName,
 				},
+				Annotations: map[string]string{
+					asoannotations.PerResourceSecret: "cluster-aso-secret",
+				},
 			},
 			Status: asoresourcesv1.ResourceGroup_STATUS{
 				Conditions: []conditions.Condition{
@@ -267,6 +282,10 @@ func TestCreateOrUpdateResource(t *testing.T) {
 				Namespace: "namespace",
 			},
 		})
+		specMock.EXPECT().Parameters(gomockinternal.AContext(), gomock.Not(gomock.Nil())).DoAndReturn(func(_ context.Context, group *asoresourcesv1.ResourceGroup) (*asoresourcesv1.ResourceGroup, error) {
+			return group, nil
+		})
+		specMock.EXPECT().WasManaged(gomock.Any()).Return(false)
 
 		ctx := context.Background()
 		g.Expect(c.Create(ctx, &asoresourcesv1.ResourceGroup{
@@ -276,6 +295,9 @@ func TestCreateOrUpdateResource(t *testing.T) {
 				Labels: map[string]string{
 					infrav1.OwnedByClusterLabelKey: clusterName,
 				},
+				Annotations: map[string]string{
+					asoannotations.PerResourceSecret: "cluster-aso-secret",
+				},
 			},
 			Status: asoresourcesv1.ResourceGroup_STATUS{
 				Conditions: []conditions.Condition{
@@ -295,6 +317,7 @@ func TestCreateOrUpdateResource(t *testing.T) {
 		var recerr azure.ReconcileError
 		g.Expect(errors.As(err, &recerr)).To(BeTrue())
 		g.Expect(recerr.IsTerminal()).To(BeTrue())
+		g.Expect(recerr.IsTransient()).To(BeFalse())
 	})
 
 	t.Run("error getting existing resource", func(t *testing.T) {