Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Disconnect call in Reconcile #296

Merged
merged 2 commits into from
Dec 2, 2021
Merged

Add Disconnect call in Reconcile #296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bf53464
Add Disconnect call in Reconcile
vaspahomov Oct 1, 2021
a5ff67d
remove named returns; disconnect error should not requeue reconcile
vaspahomov Oct 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 74 additions & 3 deletions pkg/reconciler/managed/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -269,6 +320,11 @@ func (c *NopConnecter) Connect(_ context.Context, _ resource.Managed) (ExternalC
return &NopClient{}, nil
}

// Disconnect do nothing. It never returns an error.
vaspahomov marked this conversation as resolved.
Show resolved Hide resolved
func (c *NopConnecter) Disconnect(_ context.Context) error {
return nil
}

// A NopClient does nothing.
type NopClient struct{}

Expand Down Expand Up @@ -408,12 +464,12 @@ func defaultMRManaged(m manager.Manager) mrManaged {
}

type mrExternal struct {
ExternalConnecter
ExternalConnectDisconnecter
}

func defaultMRExternal() mrExternal {
return mrExternal{
ExternalConnecter: &NopConnecter{},
ExternalConnectDisconnecter: &NopConnecter{},
}
}

Expand Down Expand Up @@ -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
}
}

Expand Down Expand Up @@ -677,6 +741,13 @@ 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))
managed.SetConditions(xpv1.ReconcileError(errors.Wrap(disconnectErr, errReconcileDisconnect)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this SetCondition call might cause flapping/overwritten conditions in cases where the deferred function is called because we're returning from another error case where we would also update the same condition. I'll fix this up in a future PR though.

}
}()
negz marked this conversation as resolved.
Show resolved Hide resolved

observation, err := external.Observe(externalCtx, managed)
if err != nil {
Expand Down
77 changes: 77 additions & 0 deletions pkg/reconciler/managed/reconciler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,83 @@ func TestReconciler(t *testing.T) {
},
want: want{result: reconcile.Result{Requeue: true}},
},
"ExternalDisconnectError": {
reason: "Error disconnecting from the provider should not trigger requeue.",
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}},
},
"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}},
},
"ExternalObserveError": {
reason: "Errors observing the external resource should trigger a requeue after a short wait.",
args: args{
Expand Down