diff --git a/changelog/fragments/ansible-successful-condition.yaml b/changelog/fragments/ansible-successful-condition.yaml new file mode 100644 index 00000000000..180e6f1f98d --- /dev/null +++ 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 index bbe525c60b0..f2d2b71c290 100644 --- 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 index aa91e3b9836..180e1ffc4d4 100644 --- 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 index 1250fe795ec..427a440b2d7 100644 --- 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 index 84aa74b8df5..37aedb0fa34 100644 --- 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 index f7053f24bd3..9623d2ad692 100644 --- 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 index acb7e7e34d8..649713e3924 100644 --- 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 index b2c2bec9f8d..17f5747f50e 100644 --- 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 index 9254657cfe9..e766d87f6e1 100644 --- 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 index 65381a760d4..4a175a4e32e 100644 --- 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 index 28a18b4327a..0ccd5206a99 100644 --- 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 index 1a2b511d815..fc6f7ea3aaf 100644 --- 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 index 54f5dd4959d..f03eda74876 100644 --- 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 index 4516827f35a..cbf26082212 100644 --- 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 index 2824b5a30ee..bfb13461891 100644 --- 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 index 1fda30ae63c..b13cac0760a 100644 --- 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