Retry failed runs with ObserveAndDelete policy (#301)

* retry failed ansibleruns with ObserverAndDelete policy Signed-off-by: Dasha Komsa <komsa.darya@gmail.com> * add unit tests for retrying failed ObserveAndDelete runs Signed-off-by: Dasha Komsa <komsa.darya@gmail.com> * linting fixes Signed-off-by: Dasha Komsa <komsa.darya@gmail.com> * handleLastApplied cyclomatic complexity reduced, remove nolint comment Signed-off-by: Dasha Komsa <komsa.darya@gmail.com> --------- Signed-off-by: Dasha Komsa <komsa.darya@gmail.com>
crossplane-contrib · Feb 2, 2024 · 4d74aa6 · 4d74aa6
1 parent 153108a
commit 4d74aa6
Show file tree

Hide file tree

Showing 2 changed files with 106 additions and 30 deletions.
diff --git a/internal/controller/ansibleRun/ansibleRun.go b/internal/controller/ansibleRun/ansibleRun.go
@@ -441,41 +441,39 @@ func getLastAppliedParameters(observed *v1alpha1.AnsibleRun) (*v1alpha1.AnsibleR
 	return lastParameters, nil
 }
 
-// nolint: gocyclo
-// TODO reduce cyclomatic complexity
 func (c *external) handleLastApplied(ctx context.Context, lastParameters *v1alpha1.AnsibleRunParameters, desired *v1alpha1.AnsibleRun) (managed.ExternalObservation, error) {
-	isUpToDate := false
-	if lastParameters != nil {
-		if equality.Semantic.DeepEqual(*lastParameters, desired.Spec.ForProvider) {
-			// Mark as up-to-date since last is equal to desired
-			isUpToDate = true
-		}
+	// Mark as up-to-date if last is equal to desired
+	isUpToDate := (lastParameters != nil && equality.Semantic.DeepEqual(*lastParameters, desired.Spec.ForProvider))
+
+	isLastSyncOK := (desired.GetCondition(xpv1.TypeSynced).Status == v1.ConditionTrue)
+
+	if isUpToDate && isLastSyncOK {
+		// nothing to do for this run
+		return managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil
 	}
 
-	if !isUpToDate {
-		out, err := json.Marshal(desired.Spec.ForProvider)
-		if err != nil {
-			return managed.ExternalObservation{}, err
-		}
-		// set LastAppliedConfig Annotation to avoid useless cmd run
-		meta.AddAnnotations(desired, map[string]string{
-			v1.LastAppliedConfigAnnotation: string(out),
-		})
+	out, err := json.Marshal(desired.Spec.ForProvider)
+	if err != nil {
+		return managed.ExternalObservation{}, err
+	}
+	// set LastAppliedConfig Annotation to avoid useless cmd run
+	meta.AddAnnotations(desired, map[string]string{
+		v1.LastAppliedConfigAnnotation: string(out),
+	})
 
-		if err := c.kube.Update(ctx, desired); err != nil {
-			return managed.ExternalObservation{}, err
-		}
-		stateVar := make(map[string]string)
-		stateVar["state"] = "present"
-		nestedMap := make(map[string]interface{})
-		nestedMap[desired.GetName()] = stateVar
-		if err := c.runner.WriteExtraVar(nestedMap); err != nil {
-			return managed.ExternalObservation{}, err
-		}
+	if err := c.kube.Update(ctx, desired); err != nil {
+		return managed.ExternalObservation{}, err
+	}
+	stateVar := make(map[string]string)
+	stateVar["state"] = "present"
+	nestedMap := make(map[string]interface{})
+	nestedMap[desired.GetName()] = stateVar
+	if err := c.runner.WriteExtraVar(nestedMap); err != nil {
+		return managed.ExternalObservation{}, err
+	}
 
-		if err := c.runAnsible(desired); err != nil {
-			return managed.ExternalObservation{}, fmt.Errorf("running ansible: %w", err)
-		}
+	if err := c.runAnsible(desired); err != nil {
+		return managed.ExternalObservation{}, fmt.Errorf("running ansible: %w", err)
 	}
 
 	// The crossplane runtime is not aware of the external resource created by ansible content.

diff --git a/internal/controller/ansibleRun/ansibleRun_test.go b/internal/controller/ansibleRun/ansibleRun_test.go
@@ -30,6 +30,7 @@ import (
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/spf13/afero"
+	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -537,6 +538,26 @@ func TestObserve(t *testing.T) {
 		conditions []xpv1.Condition
 	}
 
+	testPlaybook := "fake playbook"
+	testRun := v1alpha1.AnsibleRun{
+		ObjectMeta: metav1.ObjectMeta{
+			Annotations: map[string]string{
+				v1.LastAppliedConfigAnnotation: fmt.Sprintf(`{"playbookInline":"%s"}`, testPlaybook),
+			},
+		},
+		Spec: v1alpha1.AnsibleRunSpec{
+			ForProvider: v1alpha1.AnsibleRunParameters{
+				PlaybookInline: &testPlaybook,
+			},
+		},
+	}
+
+	testRunWithReconcileSuccess := testRun.DeepCopy()
+	testRunWithReconcileSuccess.SetConditions(xpv1.ReconcileSuccess())
+
+	testRunWithReconcileError := testRun.DeepCopy()
+	testRunWithReconcileError.SetConditions(xpv1.ReconcileError(errors.New("fake error")))
+
 	cases := map[string]struct {
 		reason string
 		fields fields
@@ -586,6 +607,63 @@ func TestObserve(t *testing.T) {
 				conditions: []xpv1.Condition{xpv1.Unavailable()},
 			},
 		},
+		"UnchangedWithObserveAndDeletePolicy": {
+			reason: "We should not run ansible when spec has not changed and last sync was successful",
+			fields: fields{
+				kube: &test.MockClient{
+					MockGet:    test.NewMockGetFn(nil),
+					MockUpdate: test.NewMockUpdateFn(nil),
+				},
+				runner: &MockRunner{
+					MockAnsibleRunPolicy: func() *ansible.RunPolicy {
+						return &ansible.RunPolicy{
+							Name: "ObserveAndDelete",
+						}
+					},
+					MockWriteExtraVar: func(extraVar map[string]interface{}) error {
+						return nil
+					},
+					MockRun: func() (*exec.Cmd, io.Reader, error) {
+						return nil, nil, fmt.Errorf("run should not have been called")
+					},
+				},
+			},
+			args: args{
+				mg: testRunWithReconcileSuccess,
+			},
+			want: want{
+				o: managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true},
+			},
+		},
+		"RetryFailedWithObserveAndDeletePolicy": {
+			reason: "We should run ansible when spec has not changed but last sync was unsuccessful",
+			fields: fields{
+				kube: &test.MockClient{
+					MockGet:    test.NewMockGetFn(nil),
+					MockUpdate: test.NewMockUpdateFn(nil),
+				},
+				runner: &MockRunner{
+					MockAnsibleRunPolicy: func() *ansible.RunPolicy {
+						return &ansible.RunPolicy{
+							Name: "ObserveAndDelete",
+						}
+					},
+					MockWriteExtraVar: func(extraVar map[string]interface{}) error {
+						return nil
+					},
+					MockRun: func() (*exec.Cmd, io.Reader, error) {
+						cmd := exec.Command("ls")
+						return cmd, nil, cmd.Start()
+					},
+				},
+			},
+			args: args{
+				mg: testRunWithReconcileError,
+			},
+			want: want{
+				o: managed.ExternalObservation{ResourceExists: true, ResourceUpToDate: true},
+			},
+		},
 		"GetObservedErrorWhenCheckWhenObservePolicy": {
 			reason: "We should return any error we encounter getting observed resource",
 			fields: fields{