Merge pull request #391 from ulucinar/fix-sync-state

Cache the error from the last asynchronous reconciliation

pkg/controller/api.go

@@ -27,6 +27,13 @@ const (
 	errReconcileRequestFmt = "cannot request the reconciliation of the resource %s/%s after an async %s"
 )
 
+// crossplane-runtime error constants
+const (
+	errXPReconcileCreate = "create failed"
+	errXPReconcileUpdate = "update failed"
+	errXPReconcileDelete = "delete failed"
+)
+
 const (
 	rateLimiterCallback = "asyncCallback"
 )
@@ -119,6 +126,20 @@ func (ac *APICallbacks) callbackFn(name, op string) terraform.CallbackFn {
 		// to do so. So we keep the `LastAsyncOperation` condition.
 		// TODO: move this to the `Synced` condition.
 		tr.SetConditions(resource.LastAsyncOperationCondition(err))
+		if err != nil {
+			wrapMsg := ""
+			switch op {
+			case "create":
+				wrapMsg = errXPReconcileCreate
+			case "update":
+				wrapMsg = errXPReconcileUpdate
+			case "destroy":
+				wrapMsg = errXPReconcileDelete
+			}
+			tr.SetConditions(xpv1.ReconcileError(errors.Wrap(err, wrapMsg)))
+		} else {
+			tr.SetConditions(xpv1.ReconcileSuccess())
+		}
 		if ac.enableStatusUpdates {
 			tr.SetConditions(resource.AsyncOperationFinishedCondition())
 		}

pkg/controller/external_async_tfpluginfw.go

@@ -7,6 +7,7 @@ package controller
 import (
 	"context"
 
+	xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1"
 	"github.com/crossplane/crossplane-runtime/pkg/logging"
 	"github.com/crossplane/crossplane-runtime/pkg/meta"
 	"github.com/crossplane/crossplane-runtime/pkg/reconciler/managed"
@@ -116,14 +117,16 @@ func (n *terraformPluginFrameworkAsyncExternalClient) Observe(ctx context.Contex
 			ResourceUpToDate: true,
 		}, nil
 	}
-	n.opTracker.LastOperation.Flush()
+	n.opTracker.LastOperation.Clear(true)
 
 	o, err := n.terraformPluginFrameworkExternalClient.Observe(ctx, mg)
 	// clear any previously reported LastAsyncOperation error condition here,
 	// because there are no pending updates on the existing resource and it's
 	// not scheduled to be deleted.
 	if err == nil && o.ResourceExists && o.ResourceUpToDate && !meta.WasDeleted(mg) {
 		mg.(resource.Terraformed).SetConditions(resource.LastAsyncOperationCondition(nil))
+		mg.(resource.Terraformed).SetConditions(xpv1.ReconcileSuccess())
+		n.opTracker.LastOperation.Clear(false)
 	}
 	return o, err
 }
@@ -149,7 +152,7 @@ func (n *terraformPluginFrameworkAsyncExternalClient) Create(_ context.Context,
 		}
 	}()
 
-	return managed.ExternalCreation{}, nil
+	return managed.ExternalCreation{}, n.opTracker.LastOperation.Error()
 }
 
 func (n *terraformPluginFrameworkAsyncExternalClient) Update(_ context.Context, mg xpresource.Managed) (managed.ExternalUpdate, error) {
@@ -173,7 +176,7 @@ func (n *terraformPluginFrameworkAsyncExternalClient) Update(_ context.Context,
 		}
 	}()
 
-	return managed.ExternalUpdate{}, nil
+	return managed.ExternalUpdate{}, n.opTracker.LastOperation.Error()
 }
 
 func (n *terraformPluginFrameworkAsyncExternalClient) Delete(_ context.Context, mg xpresource.Managed) error {
@@ -200,5 +203,5 @@ func (n *terraformPluginFrameworkAsyncExternalClient) Delete(_ context.Context,
 		}
 	}()
 
-	return nil
+	return n.opTracker.LastOperation.Error()
 }

pkg/controller/external_async_tfpluginsdk.go

@@ -8,6 +8,7 @@ import (
 	"context"
 	"time"
 
+	xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1"
 	"github.com/crossplane/crossplane-runtime/pkg/logging"
 	"github.com/crossplane/crossplane-runtime/pkg/meta"
 	"github.com/crossplane/crossplane-runtime/pkg/reconciler/managed"
@@ -121,14 +122,16 @@ func (n *terraformPluginSDKAsyncExternal) Observe(ctx context.Context, mg xpreso
 			ResourceUpToDate: true,
 		}, nil
 	}
-	n.opTracker.LastOperation.Flush()
+	n.opTracker.LastOperation.Clear(true)
 
 	o, err := n.terraformPluginSDKExternal.Observe(ctx, mg)
 	// clear any previously reported LastAsyncOperation error condition here,
 	// because there are no pending updates on the existing resource and it's
 	// not scheduled to be deleted.
 	if err == nil && o.ResourceExists && o.ResourceUpToDate && !meta.WasDeleted(mg) {
 		mg.(resource.Terraformed).SetConditions(resource.LastAsyncOperationCondition(nil))
+		mg.(resource.Terraformed).SetConditions(xpv1.ReconcileSuccess())
+		n.opTracker.LastOperation.Clear(false)
 	}
 	return o, err
 }
@@ -154,7 +157,7 @@ func (n *terraformPluginSDKAsyncExternal) Create(_ context.Context, mg xpresourc
 		}
 	}()
 
-	return managed.ExternalCreation{}, nil
+	return managed.ExternalCreation{}, n.opTracker.LastOperation.Error()
 }
 
 func (n *terraformPluginSDKAsyncExternal) Update(_ context.Context, mg xpresource.Managed) (managed.ExternalUpdate, error) {
@@ -178,7 +181,7 @@ func (n *terraformPluginSDKAsyncExternal) Update(_ context.Context, mg xpresourc
 		}
 	}()
 
-	return managed.ExternalUpdate{}, nil
+	return managed.ExternalUpdate{}, n.opTracker.LastOperation.Error()
 }
 
 func (n *terraformPluginSDKAsyncExternal) Delete(_ context.Context, mg xpresource.Managed) error {
@@ -205,5 +208,5 @@ func (n *terraformPluginSDKAsyncExternal) Delete(_ context.Context, mg xpresourc
 		}
 	}()
 
-	return nil
+	return n.opTracker.LastOperation.Error()
 }

pkg/terraform/operation.go

@@ -43,14 +43,26 @@ func (o *Operation) MarkEnd() {
 	o.endTime = &now
 }
 
-// Flush cleans the operation information.
+// Flush cleans the operation information including the registered error from
+// the last reconciliation.
+// Deprecated: Please use Clear, which allows optionally preserving the error
+// from the last reconciliation to implement proper SYNC status condition for
+// the asynchronous external clients.
 func (o *Operation) Flush() {
+	o.Clear(false)
+}
+
+// Clear clears the operation information optionally preserving the last
+// registered error from the last reconciliation.
+func (o *Operation) Clear(preserveError bool) {
 	o.mu.Lock()
 	defer o.mu.Unlock()
 	o.Type = ""
 	o.startTime = nil
 	o.endTime = nil
-	o.err = nil
+	if !preserveError {
+		o.err = nil
+	}
 }
 
 // IsEnded returns whether the operation has ended, regardless of its result.

pkg/terraform/operation_test.go

@@ -8,9 +8,11 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/pkg/errors"
 )
 
 func TestOperation(t *testing.T) {
+	testErr := errors.New("test error")
 	type args struct {
 		calls func(o *Operation)
 	}
@@ -54,13 +56,46 @@ func TestOperation(t *testing.T) {
 			args: args{
 				calls: func(o *Operation) {
 					o.MarkStart("type")
+					o.SetError(testErr)
 					o.MarkEnd()
 					o.Flush()
 				},
 			},
 			want: want{
 				checks: func(o *Operation) bool {
-					return o.Type == "" && o.startTime == nil && o.endTime == nil
+					return o.Type == "" && o.startTime == nil && o.endTime == nil && o.err == nil
+				},
+				result: true,
+			},
+		},
+		"ClearedIncludingErrors": {
+			args: args{
+				calls: func(o *Operation) {
+					o.MarkStart("type")
+					o.SetError(testErr)
+					o.MarkEnd()
+					o.Clear(false)
+				},
+			},
+			want: want{
+				checks: func(o *Operation) bool {
+					return o.Type == "" && o.startTime == nil && o.endTime == nil && o.err == nil
+				},
+				result: true,
+			},
+		},
+		"ClearedPreservingErrors": {
+			args: args{
+				calls: func(o *Operation) {
+					o.MarkStart("type")
+					o.SetError(testErr)
+					o.MarkEnd()
+					o.Clear(true)
+				},
+			},
+			want: want{
+				checks: func(o *Operation) bool {
+					return o.Type == "" && o.startTime == nil && o.endTime == nil && errors.Is(o.err, testErr)
 				},
 				result: true,
 			},