connector: Use a separate goroutine for update destination #3391
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dnephin
requested review from
ssoroka,
BruceMacD,
jmorganca and
pdevine
as code owners
dnephin
force-pushed
the
dnephin/connector-goroutines
branch
from
aa0f657
to
41c3ed3
Compare
dnephin
commented
Oct 4, 2022
internal/connector/connector.go
Outdated
Comment on lines
217
to
221
| if err := syncDestination(con); err != nil { | ||
| logging.Errorf("failed to update destination in infra: %v", err) | ||
| } | ||
| // TODO: should this exit after the first successful update? | ||
| wait(ctx, 30*time.Second) |
There was a problem hiding this comment.
I'm wondering how this should work. I think there's a few different cases:
- the sync failed - we should probably retry pretty quickly, but maybe with some exponential backoff (start at 2s retry, with a max backoff of 1 minute?)
- the sync was successful, but there's no
Connection.URLyet (relevant once we merge feat(connector): create destination as soon as possible #3384) - probably the same behaviour as above. Short delay with some backoff - the sync was successful, and there is a
Connection.URL- do we actually want to continue to check for updates in this scenario? I guess it needs to detect new namespaces and roles being added to the cluster? So maybe a 5 minute sleep?
There was a problem hiding this comment.
The URL could also change. If the service is using NodePort, the original node may have scaled down and replaced with another node. If the service is LoadBalancer, the load balancer may have been updated.
dnephin
force-pushed
the
dnephin/connector-goroutines
branch
2 times, most recently
from
a17c3da
to
22c1322
Compare
mxyng
approved these changes
Oct 4, 2022
This errgroup allows us to shutdown everything if one of the critical operations fails.
So that we can block on list grants
dnephin
force-pushed
the
dnephin/connector-goroutines
branch
from
22c1322
to
82ee11b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This should allow us to have different life-cycles for each of these operations.