[go: nahoru, domu]
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

Disconnect ExternalClient #697

Open
denniskniep opened this issue May 25, 2024 · 0 comments
Open

Disconnect ExternalClient #697

denniskniep opened this issue May 25, 2024 · 0 comments
Labels
bug Something isn't working

Comments

@denniskniep
Copy link

Problem

I am currently struggling with appropriately disconnecting the external client after it was used.

My Reconciler is created with WithExternalConnectDisconnecter here

I implemented the connect and disconnect method. But disconnect does not have any further specific parameters, so it seems to be intended to close the whole Connector. But there is only one Connector per Reconciler. Meaning if the Connectorś disconnect method is called and then the Reconciler.Reconcile gets another call it will call Connect() on an already Disconnected connector.

crossplane-runtime/pkg/reconciler/managed/reconciler.go

Lines 892 to 912 in fc03661

external, err := r.external.Connect(externalCtx, managed)
if err != nil {
// We'll usually hit this case if our Provider or its secret are missing
// or invalid. If this is first time we encounter this issue we'll be
// requeued implicitly when we update our status with the new error
// condition. If not, we requeue explicitly, which will trigger
// backoff.
log.Debug("Cannot connect to provider", "error", err)
if kerrors.IsConflict(err) {
return reconcile.Result{Requeue: true}, nil
}
record.Event(managed, event.Warning(reasonCannotConnect, err))
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 err := r.external.Disconnect(ctx); err != nil {
log.Debug("Cannot disconnect from provider", "error", err)
record.Event(managed, event.Warning(reasonCannotDisconnect, err))
}
}()

Currently the Disconnect call is:
r.external.Disconnect(ctx);

Proposal

Wouldn´t it be better to pass either external as a parameter to disconnect
r.external.Disconnect(ctx, external);

or directly call Disconnect on external (without r.)
external.Disconnect(ctx)

Benefit is, that we can close precisely an reconcile call scoped client. And not disconnect the whole connector of the reconciler, though it is still in use and might run another reconcile
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@denniskniep