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.

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

[VC-35738] Allow DataGatherer.Run to return without stopping the other errgroup Go Routines #605

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

wallrj
Copy link
Member

@wallrj wallrj commented Nov 6, 2024

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.Run implementations.

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:

$ ./hack/e2e/test.sh
...
{
  "ts": 1730909040560.407,
  "caller": "logs/logs.go:153",
  "msg": "successfully gathered 10 items from \"k8s/namespaces\" datagatherer",
  "source": "agent",
  "v": 0
}
{
  "ts": 1730909040560.5063,
  "caller": "logs/logs.go:153",
  "msg": "successfully gathered 0 items from \"k8s/googlecasclusterissuers\" datagatherer",
  "source": "agent",
  "v": 0
}
{
  "ts": 1730909040560.5884,
  "caller": "logs/logs.go:153",
  "msg": "successfully gathered 0 items from \"k8s/gateways\" datagatherer",
  "source": "agent",
  "v": 0
}
{
  "ts": 1730909040560.6826,
  "caller": "logs/logs.go:153",
  "msg": "Posting data to: ",
  "source": "agent",
  "v": 0
}
{"ts":1730909041447.9111,"caller":"logs/logs.go:153","msg":"Data sent successfully.","source":"agent","v":0}

@wallrj wallrj requested a review from tfadeyi November 6, 2024 16:13
@wallrj wallrj changed the title Allow DataGatherer.Run to return without stopping the other errgroup Go Routines [VC-35738] Allow DataGatherer.Run to return without stopping the other errgroup Go Routines Nov 6, 2024
@wallrj wallrj mentioned this pull request Nov 6, 2024
12 tasks
@@ -183,11 +183,14 @@ func Run(cmd *cobra.Command, args []string) error {

// start the data gatherers and wait for the cache sync
group.Go(func() error {
// Most implementations of `DataGatherer.Run` return immediately.
Copy link
Member

Choose a reason for hiding this comment

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

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
}

@wallrj wallrj merged commit 1778ed5 into VC-35738/feature Nov 7, 2024
2 checks passed
@wallrj wallrj deleted the VC-35738/errgroup-return-error branch November 7, 2024 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants