[VC-35738] Allow DataGatherer.Run to return without stopping the other errgroup Go Routines#605
Merged
wallrj merged 1 commit intoVC-35738/featurefrom Nov 7, 2024
Merged
Conversation
…Go Routines Signed-off-by: Richard Wall <richard.wall@venafi.com>
maelvls
reviewed
Nov 7, 2024
|
|
||
| // start the data gatherers and wait for the cache sync | ||
| group.Go(func() error { | ||
| // Most implementations of `DataGatherer.Run` return immediately. |
Member
There was a problem hiding this comment.
Side-note, you don't need to change anything:
I find it weird that the Run func's implementation varies from data gatherers to data gatherers...
The specification of the Run func (an interface, I assume) seems to be defined as "Run blocks until the channel is closed".
But the local datagatherer, for example, is implemented like this:
func (g *DataGatherer) Run(stopCh <-chan struct{}) error {
// no async functionality, see Fetch
return nil
}It should be:
func (g *DataGatherer) Run(stopCh <-chan struct{}) error {
// no async functionality, see Fetch
<-stopCh
return nil
}
maelvls
approved these changes
Nov 7, 2024
This file contains hidden or 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
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.
In #601 (comment) I introduced a bug which caused the API server and VenafiConnection helper to exit as a result of the early return of some
DataGatherer.Runimplementations.Tested using
hack/e2e/test.sh.Before this change, the venctl installation part of the script failed,
because the healthz server gets quickly shutdown as a result of the Discovery DataGatherer.Run function returning and cancelling the errgroup.
This causes the Liveness check to fail, and the Helm install never succeeds.
After this change, the test script works again: