From bf53464ca5289b243b48a0c02d7ac5d7c299840f Mon Sep 17 00:00:00 2001 From: vaspahomov Date: Fri, 1 Oct 2021 13:22:44 +0500 Subject: [PATCH 1/2] Add Disconnect call in Reconcile Signed-off-by: vaspahomov --- pkg/reconciler/managed/reconciler.go | 85 +++++++++++++++++++++-- pkg/reconciler/managed/reconciler_test.go | 83 ++++++++++++++++++++++ 2 files changed, 164 insertions(+), 4 deletions(-) diff --git a/pkg/reconciler/managed/reconciler.go b/pkg/reconciler/managed/reconciler.go index e8793c56e..0a9daddfa 100644 --- a/pkg/reconciler/managed/reconciler.go +++ b/pkg/reconciler/managed/reconciler.go @@ -52,6 +52,7 @@ const ( errUpdateManagedAnnotations = "cannot update managed resource annotations" errCreateIncomplete = "cannot determine creation result - remove the " + meta.AnnotationKeyExternalCreatePending + " annotation if it is safe to proceed" errReconcileConnect = "connect failed" + errReconcileDisconnect = "disconnect failed" errReconcileObserve = "observe failed" errReconcileCreate = "create failed" errReconcileUpdate = "update failed" @@ -61,6 +62,7 @@ const ( // Event reasons. const ( reasonCannotConnect event.Reason = "CannotConnectToProvider" + reasonCannotDisconnect event.Reason = "CannotDisconnectFromProvider" reasonCannotInitialize event.Reason = "CannotInitializeManagedResource" reasonCannotResolveRefs event.Reason = "CannotResolveResourceReferences" reasonCannotObserve event.Reason = "CannotObserveExternalResource" @@ -187,6 +189,46 @@ type ExternalConnecter interface { Connect(ctx context.Context, mg resource.Managed) (ExternalClient, error) } +// An ExternalDisconnecter disconnect from provider. +type ExternalDisconnecter interface { + // Disconnect from the provider and close an ExternalClient. + Disconnect(ctx context.Context) error +} + +// NopDisconnect does nothing. Never returns error. +func NopDisconnect(_ context.Context) error { + return nil +} + +// externalConnectDisconnecter is wrap ExternalConnecter in externalConnectDisconnecter +// with nop Disconnect. +type externalConnectDisconnecter struct { + c ExternalConnecter + d ExternalDisconnecter +} + +// Connect call ExternalConnecter's Connect +func (c *externalConnectDisconnecter) Connect(ctx context.Context, mg resource.Managed) (ExternalClient, error) { + return c.c.Connect(ctx, mg) +} + +// Disconnect call ExternalDisconnecter's Disconnect +func (c *externalConnectDisconnecter) Disconnect(ctx context.Context) error { + return c.d.Disconnect(ctx) +} + +// NewExternalConnectDisconnecter wraps ExternalConnecter in ExternalConnectDisconnecter +func NewExternalConnectDisconnecter(c ExternalConnecter, d ExternalDisconnecter) ExternalConnectDisconnecter { + return &externalConnectDisconnecter{c, d} +} + +// An ExternalConnectDisconnecter produces a new ExternalClient given the supplied +// Managed resource. +type ExternalConnectDisconnecter interface { + ExternalConnecter + ExternalDisconnecter +} + // An ExternalConnectorFn is a function that satisfies the ExternalConnecter // interface. type ExternalConnectorFn func(ctx context.Context, mg resource.Managed) (ExternalClient, error) @@ -197,6 +239,15 @@ func (ec ExternalConnectorFn) Connect(ctx context.Context, mg resource.Managed) return ec(ctx, mg) } +// An ExternalDisconnectorFn is a function that satisfies the ExternalConnecter +// interface. +type ExternalDisconnectorFn func(ctx context.Context) error + +// Disconnect from provider and close an ExternalClient. +func (ed ExternalDisconnectorFn) Disconnect(ctx context.Context) error { + return ed(ctx) +} + // An ExternalClient manages the lifecycle of an external resource. // None of the calls here should be blocking. All of the calls should be // idempotent. For example, Create call should not return AlreadyExists error @@ -269,6 +320,11 @@ func (c *NopConnecter) Connect(_ context.Context, _ resource.Managed) (ExternalC return &NopClient{}, nil } +// Disconnect do nothing. It never returns an error. +func (c *NopConnecter) Disconnect(_ context.Context) error { + return nil +} + // A NopClient does nothing. type NopClient struct{} @@ -408,12 +464,12 @@ func defaultMRManaged(m manager.Manager) mrManaged { } type mrExternal struct { - ExternalConnecter + ExternalConnectDisconnecter } func defaultMRExternal() mrExternal { return mrExternal{ - ExternalConnecter: &NopConnecter{}, + ExternalConnectDisconnecter: &NopConnecter{}, } } @@ -456,7 +512,15 @@ func WithCreationGracePeriod(d time.Duration) ReconcilerOption { // used to sync and delete external resources. func WithExternalConnecter(c ExternalConnecter) ReconcilerOption { return func(r *Reconciler) { - r.external.ExternalConnecter = c + r.external.ExternalConnectDisconnecter = NewExternalConnectDisconnecter(c, ExternalDisconnectorFn(NopDisconnect)) + } +} + +// WithExternalConnectDisconnecter specifies how the Reconciler should connect and disconnect to the API +// used to sync and delete external resources. +func WithExternalConnectDisconnecter(c ExternalConnectDisconnecter) ReconcilerOption { + return func(r *Reconciler) { + r.external.ExternalConnectDisconnecter = c } } @@ -552,7 +616,7 @@ func NewReconciler(m manager.Manager, of resource.ManagedKind, o ...ReconcilerOp } // Reconcile a managed resource with an external resource. -func (r *Reconciler) Reconcile(_ context.Context, req reconcile.Request) (reconcile.Result, error) { // nolint:gocyclo +func (r *Reconciler) Reconcile(_ context.Context, req reconcile.Request) (result reconcile.Result, err error) { // nolint:gocyclo // NOTE(negz): This method is a well over our cyclomatic complexity goal. // Be wary of adding additional complexity. @@ -677,6 +741,19 @@ func (r *Reconciler) Reconcile(_ context.Context, req reconcile.Request) (reconc managed.SetConditions(xpv1.ReconcileError(errors.Wrap(err, errReconcileConnect))) return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus) } + defer func() { + if disconnectErr := r.external.Disconnect(ctx); disconnectErr != nil { + log.Debug("Cannot disconnect from provider", "error", disconnectErr) + record.Event(managed, event.Warning(reasonCannotDisconnect, disconnectErr)) + disconnectErr = errors.Wrap(disconnectErr, errReconcileDisconnect) + managed.SetConditions(xpv1.ReconcileError(disconnectErr)) + if err != nil { + err = errors.Wrap(err, disconnectErr.Error()) + } else { + err = disconnectErr + } + } + }() observation, err := external.Observe(externalCtx, managed) if err != nil { diff --git a/pkg/reconciler/managed/reconciler_test.go b/pkg/reconciler/managed/reconciler_test.go index 03d8423b5..ba16d3d11 100644 --- a/pkg/reconciler/managed/reconciler_test.go +++ b/pkg/reconciler/managed/reconciler_test.go @@ -286,6 +286,89 @@ func TestReconciler(t *testing.T) { }, want: want{result: reconcile.Result{Requeue: true}}, }, + "ExternalDisconnectError": { + reason: "Errors disconnecting from the provider should return error.", + args: args{ + m: &fake.Manager{ + Client: &test.MockClient{ + MockGet: test.NewMockGetFn(nil), + MockStatusUpdate: test.MockStatusUpdateFn(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error { + want := &fake.Managed{} + want.SetConditions(xpv1.ReconcileSuccess()) + if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { + reason := "A successful no-op reconcile should be reported as a conditioned status." + t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) + } + return nil + }), + }, + Scheme: fake.SchemeWith(&fake.Managed{}), + }, + mg: resource.ManagedKind(fake.GVK(&fake.Managed{})), + o: []ReconcilerOption{ + WithInitializers(), + WithReferenceResolver(ReferenceResolverFn(func(_ context.Context, _ resource.Managed) error { return nil })), + WithExternalConnectDisconnecter(NewExternalConnectDisconnecter( + ExternalConnectorFn(func(_ context.Context, mg resource.Managed) (ExternalClient, error) { + c := &ExternalClientFns{ + ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) { + return ExternalObservation{ResourceExists: true, ResourceUpToDate: true}, nil + }, + } + return c, nil + }), ExternalDisconnectorFn(func(_ context.Context) error { + return errBoom + })), + ), + WithConnectionPublishers(), + WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}), + }, + }, + want: want{ + result: reconcile.Result{RequeueAfter: defaultpollInterval}, + err: errors.Wrap(errors.New("boom"), "disconnect failed"), + }, + }, + "ExternalObserveErrorDisconnectError": { + reason: "Errors disconnecting from the provider after error observing the external resource should trigger a requeue after a short wait and return error.", + args: args{ + m: &fake.Manager{ + Client: &test.MockClient{ + MockGet: test.NewMockGetFn(nil), + MockStatusUpdate: test.MockStatusUpdateFn(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error { + want := &fake.Managed{} + want.SetConditions(xpv1.ReconcileError(errors.Wrap(errBoom, errReconcileObserve))) + if diff := cmp.Diff(want, obj, test.EquateConditions()); diff != "" { + reason := "Errors observing the managed resource should be reported as a conditioned status." + t.Errorf("\nReason: %s\n-want, +got:\n%s", reason, diff) + } + return nil + }), + }, + Scheme: fake.SchemeWith(&fake.Managed{}), + }, + mg: resource.ManagedKind(fake.GVK(&fake.Managed{})), + o: []ReconcilerOption{ + WithInitializers(), + WithExternalConnectDisconnecter(NewExternalConnectDisconnecter( + ExternalConnectorFn(func(_ context.Context, mg resource.Managed) (ExternalClient, error) { + c := &ExternalClientFns{ + ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) { + return ExternalObservation{}, errBoom + }, + } + return c, nil + }), ExternalDisconnectorFn(func(_ context.Context) error { + return errBoom + })), + ), + }, + }, + want: want{ + result: reconcile.Result{Requeue: true}, + err: errors.Wrap(errors.New("boom"), "disconnect failed"), + }, + }, "ExternalObserveError": { reason: "Errors observing the external resource should trigger a requeue after a short wait.", args: args{ From a5ff67d5a0dac4d80ee5996d112f97ad33306467 Mon Sep 17 00:00:00 2001 From: vaspahomov Date: Thu, 28 Oct 2021 15:22:10 +0500 Subject: [PATCH 2/2] remove named returns; disconnect error should not requeue reconcile Signed-off-by: vaspahomov --- pkg/reconciler/managed/reconciler.go | 10 ++-------- pkg/reconciler/managed/reconciler_test.go | 12 +++--------- 2 files changed, 5 insertions(+), 17 deletions(-) diff --git a/pkg/reconciler/managed/reconciler.go b/pkg/reconciler/managed/reconciler.go index 0a9daddfa..3a6a447b5 100644 --- a/pkg/reconciler/managed/reconciler.go +++ b/pkg/reconciler/managed/reconciler.go @@ -616,7 +616,7 @@ func NewReconciler(m manager.Manager, of resource.ManagedKind, o ...ReconcilerOp } // Reconcile a managed resource with an external resource. -func (r *Reconciler) Reconcile(_ context.Context, req reconcile.Request) (result reconcile.Result, err error) { // nolint:gocyclo +func (r *Reconciler) Reconcile(_ context.Context, req reconcile.Request) (reconcile.Result, error) { // nolint:gocyclo // NOTE(negz): This method is a well over our cyclomatic complexity goal. // Be wary of adding additional complexity. @@ -745,13 +745,7 @@ func (r *Reconciler) Reconcile(_ context.Context, req reconcile.Request) (result if disconnectErr := r.external.Disconnect(ctx); disconnectErr != nil { log.Debug("Cannot disconnect from provider", "error", disconnectErr) record.Event(managed, event.Warning(reasonCannotDisconnect, disconnectErr)) - disconnectErr = errors.Wrap(disconnectErr, errReconcileDisconnect) - managed.SetConditions(xpv1.ReconcileError(disconnectErr)) - if err != nil { - err = errors.Wrap(err, disconnectErr.Error()) - } else { - err = disconnectErr - } + managed.SetConditions(xpv1.ReconcileError(errors.Wrap(disconnectErr, errReconcileDisconnect))) } }() diff --git a/pkg/reconciler/managed/reconciler_test.go b/pkg/reconciler/managed/reconciler_test.go index ba16d3d11..525cfe52b 100644 --- a/pkg/reconciler/managed/reconciler_test.go +++ b/pkg/reconciler/managed/reconciler_test.go @@ -287,7 +287,7 @@ func TestReconciler(t *testing.T) { want: want{result: reconcile.Result{Requeue: true}}, }, "ExternalDisconnectError": { - reason: "Errors disconnecting from the provider should return error.", + reason: "Error disconnecting from the provider should not trigger requeue.", args: args{ m: &fake.Manager{ Client: &test.MockClient{ @@ -324,10 +324,7 @@ func TestReconciler(t *testing.T) { WithFinalizer(resource.FinalizerFns{AddFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}), }, }, - want: want{ - result: reconcile.Result{RequeueAfter: defaultpollInterval}, - err: errors.Wrap(errors.New("boom"), "disconnect failed"), - }, + want: want{result: reconcile.Result{RequeueAfter: defaultpollInterval}}, }, "ExternalObserveErrorDisconnectError": { reason: "Errors disconnecting from the provider after error observing the external resource should trigger a requeue after a short wait and return error.", @@ -364,10 +361,7 @@ func TestReconciler(t *testing.T) { ), }, }, - want: want{ - result: reconcile.Result{Requeue: true}, - err: errors.Wrap(errors.New("boom"), "disconnect failed"), - }, + want: want{result: reconcile.Result{Requeue: true}}, }, "ExternalObserveError": { reason: "Errors observing the external resource should trigger a requeue after a short wait.",