Add successful condition for Ansible-based operators (#5262)

fabianvf · web-flow · commit eccd2408c8aa · 2021-10-14T12:12:32.000-04:00
* Add and set successful condition, never remove conditions

Signed-off-by: Fabian von Feilitzsch &lt;fabian@fabianism.us&gt;
diff --git a/changelog/fragments/ansible-successful-condition.yaml b/changelog/fragments/ansible-successful-condition.yaml
@@ -0,0 +1,8 @@
+entries:
+  - description: >
+      For Ansible-based Operators, adds documented `Successful` condition, and no longer
+      removes conditions from the status in updates. Users can now wait for a successful
+      reconciliation by waiting for the `Successful` type condition to be `True`.
+
+    kind: "bugfix"
+    breaking: false
diff --git a/hack/generate/samples/internal/ansible/constants.go b/hack/generate/samples/internal/ansible/constants.go
@@ -69,8 +69,7 @@ const moleculeTaskFragment = `- name: Load CR
     wait: yes
     wait_timeout: 300
     wait_condition:
-      type: Running
-      reason: Successful
+      type: Successful
       status: "True"
 
 - name: Wait 2 minutes for memcached deployment
@@ -259,8 +258,7 @@ const originaMemcachedMoleculeTask = `- name: Create the cache.example.com/v1alp
     wait: yes
     wait_timeout: 300
     wait_condition:
-      type: Running
-      reason: Successful
+      type: Successful
       status: "True"
   vars:
     cr_file: 'cache_v1alpha1_memcached.yaml'
diff --git a/hack/generate/samples/internal/ansible/testdata/tasks/argstest_test.yml b/hack/generate/samples/internal/ansible/testdata/tasks/argstest_test.yml
@@ -13,8 +13,7 @@
     wait: yes
     wait_timeout: 300
     wait_condition:
-      type: Running
-      reason: Successful
+      type: Successful
       status: "True"
   register: args_test
 
diff --git a/hack/generate/samples/internal/ansible/testdata/tasks/casetest_test.yml b/hack/generate/samples/internal/ansible/testdata/tasks/casetest_test.yml
@@ -13,8 +13,7 @@
     wait: yes
     wait_timeout: 300
     wait_condition:
-      type: Running
-      reason: Successful
+      type: Successful
       status: "True"
   register: case_test
 
diff --git a/hack/generate/samples/internal/ansible/testdata/tasks/clusterannotationtest_test.yml b/hack/generate/samples/internal/ansible/testdata/tasks/clusterannotationtest_test.yml
@@ -7,8 +7,7 @@
     wait: yes
     wait_timeout: 300
     wait_condition:
-      type: Running
-      reason: Successful
+      type: Successful
       status: "True"
   vars:
     cr_file: 'test_v1alpha1_clusterannotationtest.yaml'
diff --git a/hack/generate/samples/internal/ansible/testdata/tasks/collectiontest_test.yml b/hack/generate/samples/internal/ansible/testdata/tasks/collectiontest_test.yml
@@ -11,8 +11,7 @@
     wait: yes
     wait_timeout: 300
     wait_condition:
-      type: Running
-      reason: Successful
+      type: Successful
       status: "True"
 
 - name: Assert ConfigMap has been created by collection Role
diff --git a/hack/generate/samples/internal/ansible/testdata/tasks/inventorytest_test.yml b/hack/generate/samples/internal/ansible/testdata/tasks/inventorytest_test.yml
@@ -7,8 +7,7 @@
     wait: yes
     wait_timeout: 300
     wait_condition:
-      type: Running
-      reason: Successful
+      type: Successful
       status: "True"
   vars:
     custom_resource: "{{ lookup('template', '/'.join([
diff --git a/hack/generate/samples/internal/ansible/testdata/tasks/reconciliationtest_test.yml b/hack/generate/samples/internal/ansible/testdata/tasks/reconciliationtest_test.yml
@@ -13,8 +13,7 @@
     wait: yes
     wait_timeout: 300
     wait_condition:
-      type: Running
-      reason: Successful
+      type: Successful
       status: "True"
   register: reconciliation_test
 - name: Retreive the number of iterations on the ConfigMap
diff --git a/hack/generate/samples/internal/ansible/testdata/tasks/selectortest_test.yml b/hack/generate/samples/internal/ansible/testdata/tasks/selectortest_test.yml
@@ -15,8 +15,7 @@
     wait: yes
     wait_timeout: 300
     wait_condition:
-      type: Running
-      reason: Successful
+      type: Successful
       status: "True"
   register: selector_test
 
diff --git a/hack/generate/samples/internal/ansible/testdata/tasks/subresourcestest_test.yml b/hack/generate/samples/internal/ansible/testdata/tasks/subresourcestest_test.yml
@@ -14,8 +14,7 @@
     wait: yes
     wait_timeout: 300
     wait_condition:
-      type: Running
-      reason: Successful
+      type: Successful
       status: "True"
   register: subresources_test
 
diff --git a/internal/ansible/controller/reconcile.go b/internal/ansible/controller/reconcile.go
@@ -303,6 +303,11 @@ func (r *AnsibleOperatorReconciler) markRunning(ctx context.Context, nn types.Na
 		errCond.Status = v1.ConditionFalse
 		ansiblestatus.SetCondition(&crStatus, *errCond)
 	}
+	successCond := ansiblestatus.GetCondition(crStatus, ansiblestatus.SuccessfulConditionType)
+	if successCond != nil {
+		successCond.Status = v1.ConditionFalse
+		ansiblestatus.SetCondition(&crStatus, *successCond)
+	}
 	// If the condition is currently running, making sure that the values are correct.
 	// If they are the same a no-op, if they are different then it is a good thing we
 	// are updating it.
@@ -338,7 +343,12 @@ func (r *AnsibleOperatorReconciler) markError(ctx context.Context, nn types.Name
 	}
 	crStatus := getStatus(u)
 
-	sc := ansiblestatus.GetCondition(crStatus, ansiblestatus.RunningConditionType)
+	rc := ansiblestatus.GetCondition(crStatus, ansiblestatus.RunningConditionType)
+	if rc != nil {
+		rc.Status = v1.ConditionFalse
+		ansiblestatus.SetCondition(&crStatus, *rc)
+	}
+	sc := ansiblestatus.GetCondition(crStatus, ansiblestatus.SuccessfulConditionType)
 	if sc != nil {
 		sc.Status = v1.ConditionFalse
 		ansiblestatus.SetCondition(&crStatus, *sc)
@@ -375,33 +385,55 @@ func (r *AnsibleOperatorReconciler) markDone(ctx context.Context, nn types.Names
 	runSuccessful := len(failureMessages) == 0
 	ansibleStatus := ansiblestatus.NewAnsibleResultFromStatusJobEvent(statusEvent)
 
-	if !runSuccessful {
+	if runSuccessful {
+		metrics.ReconcileSucceeded(r.GVK.String())
+		deprecatedRunningCondition := ansiblestatus.NewCondition(
+			ansiblestatus.RunningConditionType,
+			v1.ConditionTrue,
+			ansibleStatus,
+			ansiblestatus.SuccessfulReason,
+			ansiblestatus.AwaitingMessage,
+		)
+		failureCondition := ansiblestatus.NewCondition(
+			ansiblestatus.FailureConditionType,
+			v1.ConditionFalse,
+			nil,
+			"",
+			"",
+		)
+		successfulCondition := ansiblestatus.NewCondition(
+			ansiblestatus.SuccessfulConditionType,
+			v1.ConditionTrue,
+			nil,
+			ansiblestatus.SuccessfulReason,
+			ansiblestatus.SuccessfulMessage,
+		)
+		ansiblestatus.SetCondition(&crStatus, *deprecatedRunningCondition)
+		ansiblestatus.SetCondition(&crStatus, *successfulCondition)
+		ansiblestatus.SetCondition(&crStatus, *failureCondition)
+	} else {
 		metrics.ReconcileFailed(r.GVK.String())
 		sc := ansiblestatus.GetCondition(crStatus, ansiblestatus.RunningConditionType)
 		if sc != nil {
 			sc.Status = v1.ConditionFalse
 			ansiblestatus.SetCondition(&crStatus, *sc)
 		}
-		c := ansiblestatus.NewCondition(
+		failureCondition := ansiblestatus.NewCondition(
 			ansiblestatus.FailureConditionType,
 			v1.ConditionTrue,
 			ansibleStatus,
 			ansiblestatus.FailedReason,
 			strings.Join(failureMessages, "\n"),
 		)
-		ansiblestatus.SetCondition(&crStatus, *c)
-	} else {
-		metrics.ReconcileSucceeded(r.GVK.String())
-		c := ansiblestatus.NewCondition(
-			ansiblestatus.RunningConditionType,
-			v1.ConditionTrue,
-			ansibleStatus,
-			ansiblestatus.SuccessfulReason,
-			ansiblestatus.SuccessfulMessage,
+		successfulCondition := ansiblestatus.NewCondition(
+			ansiblestatus.SuccessfulConditionType,
+			v1.ConditionFalse,
+			nil,
+			"",
+			"",
 		)
-		// Remove the failure condition if set, because this completed successfully.
-		ansiblestatus.RemoveCondition(&crStatus, ansiblestatus.FailureConditionType)
-		ansiblestatus.SetCondition(&crStatus, *c)
+		ansiblestatus.SetCondition(&crStatus, *failureCondition)
+		ansiblestatus.SetCondition(&crStatus, *successfulCondition)
 	}
 	// This needs the status subresource to be enabled by default.
 	u.Object["status"] = crStatus.GetJSONMap()
diff --git a/internal/ansible/controller/reconcile_test.go b/internal/ansible/controller/reconcile_test.go
@@ -127,6 +127,16 @@ func TestReconcile(t *testing.T) {
 								"message": "Awaiting next reconciliation",
 								"reason":  "Successful",
 							},
+							map[string]interface{}{
+								"status":  "True",
+								"type":    "Successful",
+								"message": "Last reconciliation succeeded",
+								"reason":  "Successful",
+							},
+							map[string]interface{}{
+								"status": "False",
+								"type":   "Failure",
+							},
 						},
 					},
 				},
@@ -200,6 +210,10 @@ func TestReconcile(t *testing.T) {
 								"message": "new failure message",
 								"reason":  "Failed",
 							},
+							map[string]interface{}{
+								"status": "False",
+								"type":   "Successful",
+							},
 						},
 					},
 				},
@@ -313,6 +327,16 @@ func TestReconcile(t *testing.T) {
 								"message": "Awaiting next reconciliation",
 								"reason":  "Successful",
 							},
+							map[string]interface{}{
+								"status":  "True",
+								"type":    "Successful",
+								"message": "Last reconciliation succeeded",
+								"reason":  "Successful",
+							},
+							map[string]interface{}{
+								"status": "False",
+								"type":   "Failure",
+							},
 						},
 					},
 				},
@@ -555,15 +579,15 @@ func TestReconcile(t *testing.T) {
 					actualCond := ansiblestatus.GetCondition(actualStatus, c.Type)
 					if c.Reason != actualCond.Reason || c.Message != actualCond.Message || c.Status !=
 						actualCond.Status {
-						t.Fatalf("Message or reason did not match\nexpected: %v\nactual: %v", c, actualCond)
+						t.Fatalf("Message or reason did not match\nexpected: %+v\nactual: %+v", c, actualCond)
 					}
 					if c.AnsibleResult == nil && actualCond.AnsibleResult != nil {
-						t.Fatalf("Ansible result did not match expected: %v\nactual: %v", c.AnsibleResult,
+						t.Fatalf("Ansible result did not match\nexpected: %+v\nactual: %+v", c.AnsibleResult,
 							actualCond.AnsibleResult)
 					}
 					if c.AnsibleResult != nil {
 						if !reflect.DeepEqual(c.AnsibleResult, actualCond.AnsibleResult) {
-							t.Fatalf("Ansible result did not match expected: %v\nactual: %v", c.AnsibleResult,
+							t.Fatalf("Ansible result did not match\nexpected: %+v\nactual: %+v", c.AnsibleResult,
 								actualCond.AnsibleResult)
 						}
 					}
diff --git a/internal/ansible/controller/status/types.go b/internal/ansible/controller/status/types.go
@@ -93,6 +93,8 @@ const (
 	RunningConditionType ConditionType = "Running"
 	// FailureConditionType - condition type of failure.
 	FailureConditionType ConditionType = "Failure"
+	// SuccessfulConditionType - condition type of success.
+	SuccessfulConditionType ConditionType = "Successful"
 )
 
 // Condition - the condition for the ansible operator.
@@ -118,11 +120,11 @@ func createConditionFromMap(cm map[string]interface{}) Condition {
 	}
 	reason, ok := cm["reason"].(string)
 	if !ok {
-		reason = RunningReason
+		reason = ""
 	}
 	message, ok := cm["message"].(string)
 	if !ok {
-		message = RunningMessage
+		message = ""
 	}
 	asm, ok := cm["ansibleResult"].(map[string]interface{})
 	var ansibleResult *AnsibleResult
diff --git a/internal/ansible/controller/status/utils.go b/internal/ansible/controller/status/utils.go
@@ -34,7 +34,9 @@ const (
 	// RunningMessage - message for running reason.
 	RunningMessage = "Running reconciliation"
 	// SuccessfulMessage - message for successful reason.
-	SuccessfulMessage = "Awaiting next reconciliation"
+	AwaitingMessage = "Awaiting next reconciliation"
+	// SuccessfulMessage - message for successful condition.
+	SuccessfulMessage = "Last reconciliation succeeded"
 )
 
 // NewCondition -  condition
diff --git a/internal/plugins/ansible/v1/scaffolds/internal/templates/molecule/mdefault/tasks_test_resource.go b/internal/plugins/ansible/v1/scaffolds/internal/templates/molecule/mdefault/tasks_test_resource.go
@@ -50,8 +50,7 @@ const resourceTestTemplate = `---
     wait: yes
     wait_timeout: 300
     wait_condition:
-      type: Running
-      reason: Successful
+      type: Successful
       status: "True"
   vars:
     cr_file: '{{ .SampleFile }}'
diff --git a/testdata/ansible/memcached-operator/molecule/default/tasks/memcached_test.yml b/testdata/ansible/memcached-operator/molecule/default/tasks/memcached_test.yml
@@ -13,8 +13,7 @@
     wait: yes
     wait_timeout: 300
     wait_condition:
-      type: Running
-      reason: Successful
+      type: Successful
       status: "True"
 
 - name: Wait 2 minutes for memcached deployment
