clear conditions on loop start and ensure synced (#61)

This is the first patch that looks at cleaning up our handling of
Condition objects for resources. We make a change to the common runtime
to clear all Condition objects from the desired resource's
Status.Conditions collection at the beginning of the reconciliation loop
and add a deferred call to a function that ensures that an
ACK.ResourceSynced condition has been placed on the resource if the
resource manager has failed to add one.

Future patches will add functionality to the code generator to allow
programmatic control over when generated resource managers set
ACK.ResourceSynced conditions on their resources.

Issue: aws-controllers-k8s/community#1030

Signed-off-by: Jay Pipes <[email protected]>

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

Author: jaypipes

pkg/condition/condition.go

@@ -157,3 +157,10 @@ func LateInitializationInProgress(subject acktypes.ConditionManager) bool {
 	c := LateInitialized(subject)
 	return c != nil && c.Status == corev1.ConditionFalse
 }
+
+// Clear resets the resource's collection of Conditions to an empty list.
+func Clear(
+	subject acktypes.ConditionManager,
+) {
+	subject.ReplaceConditions([]*ackv1alpha1.Condition{})
+}

pkg/runtime/reconciler.go

@@ -29,6 +29,7 @@ import (
 	ackv1alpha1 "github.com/aws-controllers-k8s/runtime/apis/core/v1alpha1"
 	ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"
 	"github.com/aws-controllers-k8s/runtime/pkg/condition"
+	ackcondition "github.com/aws-controllers-k8s/runtime/pkg/condition"
 	ackcfg "github.com/aws-controllers-k8s/runtime/pkg/config"
 	ackerr "github.com/aws-controllers-k8s/runtime/pkg/errors"
 	ackmetrics "github.com/aws-controllers-k8s/runtime/pkg/metrics"
@@ -204,6 +205,9 @@ func (r *resourceReconciler) Sync(
 
 	var latest acktypes.AWSResource // the newly created or mutated resource
 
+	r.resetConditions(ctx, desired)
+	defer r.ensureConditions(ctx, latest, err)
+
 	isAdopted := IsAdopted(desired)
 	rlog.WithValues("is_adopted", isAdopted)
 
@@ -237,6 +241,56 @@ func (r *resourceReconciler) Sync(
 	return r.handleRequeues(ctx, latest)
 }
 
+// resetConditions strips the supplied resource of all objects in its
+// Status.Conditions collection. We do this at the start of each reconciliation
+// loop in order to ensure that the objects in the Status.Conditions collection
+// represent the state transitions that occurred in the last reconciliation
+// loop. In other words, Status.Conditions should refer to the latest observed
+// state read.
+func (r *resourceReconciler) resetConditions(
+	ctx context.Context,
+	res acktypes.AWSResource,
+) {
+	var err error
+	rlog := ackrtlog.FromContext(ctx)
+	exit := rlog.Trace("r.resetConditions")
+	defer exit(err)
+
+	ackcondition.Clear(res)
+}
+
+// ensureConditions examines the supplied resource's collection of Condition
+// objects and ensures that an ACK.ResourceSynced condition is present.
+func (r *resourceReconciler) ensureConditions(
+	ctx context.Context,
+	res acktypes.AWSResource,
+	reconcileErr error,
+) {
+	if ackcompare.IsNil(res) {
+		return
+	}
+
+	var err error
+	rlog := ackrtlog.FromContext(ctx)
+	exit := rlog.Trace("r.ensureConditions")
+	defer exit(err)
+
+	if syncedCond := ackcondition.Synced(res); syncedCond == nil {
+		rlog.Debug("resource missing ACK.ResourceSynced condition")
+		// only the resource manager will know whether the resource is in a
+		// stable sync state. Even if we got no error back from the
+		// create/update operations, we can only set this to Unknown.
+		condStatus := corev1.ConditionUnknown
+		if reconcileErr == ackerr.Terminal {
+			// A terminal condition by its very nature indicates a stable state
+			// for a resource being synced. The resource is considered synced
+			// because its state will not change.
+			condStatus = corev1.ConditionTrue
+		}
+		ackcondition.SetSynced(res, condStatus, nil, nil)
+	}
+}
+
 // createResource marks the CR as managed by ACK, calls one or more AWS APIs to
 // create the backend AWS resource and patches the CR's Metadata, Spec and
 // Status back to the Kubernetes API.

pkg/runtime/reconciler_test.go

@@ -151,13 +151,29 @@ func TestReconcilerUpdate(t *testing.T) {
 	delta.Add("Spec.A", "val1", "val2")
 
 	desired, desiredRTObj, _ := resourceMocks()
+	desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return()
 
 	ids := &ackmocks.AWSResourceIdentifiers{}
 	ids.On("ARN").Return(&arn)
 
 	latest, latestRTObj, _ := resourceMocks()
 	latest.On("Identifiers").Return(ids)
+
+	// resourceReconciler.ensureConditions will ensure that if the resource
+	// manager has not set any Conditions on the resource, that at least an
+	// ACK.ResourceSynced condition with status Unknown will be set on the
+	// resource.
 	latest.On("Conditions").Return([]*ackv1alpha1.Condition{})
+	latest.On(
+		"ReplaceConditions",
+		mock.AnythingOfType("[]*v1alpha1.Condition"),
+	).Return().Run(func(args mock.Arguments) {
+		conditions := args.Get(0).([]*ackv1alpha1.Condition)
+		assert.Equal(t, 1, len(conditions))
+		cond := conditions[0]
+		assert.Equal(t, cond.Type, ackv1alpha1.ConditionTypeResourceSynced)
+		assert.Equal(t, cond.Status, corev1.ConditionUnknown)
+	})
 
 	rm := &ackmocks.AWSResourceManager{}
 	rm.On("ReadOne", ctx, desired).Return(
@@ -207,13 +223,18 @@ func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInMetadata(t *testing.T) {
 	delta.Add("Spec.A", "val1", "val2")
 
 	desired, desiredRTObj, _ := resourceMocks()
+	desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return()
 
 	ids := &ackmocks.AWSResourceIdentifiers{}
 	ids.On("ARN").Return(&arn)
 
 	latest, latestRTObj, latestMetaObj := resourceMocks()
 	latest.On("Identifiers").Return(ids)
 	latest.On("Conditions").Return([]*ackv1alpha1.Condition{})
+	latest.On(
+		"ReplaceConditions",
+		mock.AnythingOfType("[]*v1alpha1.Condition"),
+	).Return()
 
 	// Note the change in annotations
 	latestMetaObj.SetAnnotations(map[string]string{"a": "b"})
@@ -261,13 +282,18 @@ func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInSpec(t *testing.T) {
 	delta.Add("Spec.A", "val1", "val2")
 
 	desired, desiredRTObj, _ := resourceMocks()
+	desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return()
 
 	ids := &ackmocks.AWSResourceIdentifiers{}
 	ids.On("ARN").Return(&arn)
 
 	latest, latestRTObj, _ := resourceMocks()
 	latest.On("Identifiers").Return(ids)
 	latest.On("Conditions").Return([]*ackv1alpha1.Condition{})
+	latest.On(
+		"ReplaceConditions",
+		mock.AnythingOfType("[]*v1alpha1.Condition"),
+	).Return()
 	// Note no change to metadata...
 
 	rmf, rd := managedResourceManagerFactoryMocks(desired, latest)
@@ -310,13 +336,18 @@ func TestReconcilerHandleReconcilerError_PatchStatus_Latest(t *testing.T) {
 	delta.Add("Spec.A", "val1", "val2")
 
 	desired, desiredRTObj, _ := resourceMocks()
+	desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return()
 
 	ids := &ackmocks.AWSResourceIdentifiers{}
 	ids.On("ARN").Return(&arn)
 
 	latest, latestRTObj, latestMetaObj := resourceMocks()
 	latest.On("Identifiers").Return(ids)
 	latest.On("Conditions").Return([]*ackv1alpha1.Condition{})
+	latest.On(
+		"ReplaceConditions",
+		mock.AnythingOfType("[]*v1alpha1.Condition"),
+	).Return()
 
 	latestMetaObj.SetAnnotations(map[string]string{"a": "b"})
 
@@ -343,6 +374,7 @@ func TestReconcilerHandleReconcilerError_NoPatchStatus_NoLatest(t *testing.T) {
 	ctx := context.TODO()
 
 	desired, _, _ := resourceMocks()
+	desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return()
 
 	rmf, _ := managedResourceManagerFactoryMocks(desired, nil)
 	r, kc := reconcilerMocks(rmf)
@@ -371,13 +403,18 @@ func TestReconcilerUpdate_ErrorInLateInitialization(t *testing.T) {
 	delta.Add("Spec.A", "val1", "val2")
 
 	desired, desiredRTObj, _ := resourceMocks()
+	desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return()
 
 	ids := &ackmocks.AWSResourceIdentifiers{}
 	ids.On("ARN").Return(&arn)
 
 	latest, latestRTObj, _ := resourceMocks()
 	latest.On("Identifiers").Return(ids)
 	latest.On("Conditions").Return([]*ackv1alpha1.Condition{})
+	latest.On(
+		"ReplaceConditions",
+		mock.AnythingOfType("[]*v1alpha1.Condition"),
+	).Return()
 
 	rm := &ackmocks.AWSResourceManager{}
 	rm.On("ReadOne", ctx, desired).Return(
@@ -423,6 +460,7 @@ func TestReconcilerUpdate_ResourceNotManaged(t *testing.T) {
 	delta := ackcompare.NewDelta()
 
 	desired, _, _ := resourceMocks()
+	desired.On("ReplaceConditions", []*ackv1alpha1.Condition{}).Return()
 
 	ids := &ackmocks.AWSResourceIdentifiers{}
 	ids.On("ARN").Return(&arn)