Skip to content

Conversation

inteon
Copy link
Contributor

@inteon inteon commented Jun 11, 2025

Use Contexts instead of stopCh when working with informers, such that client-go can use contextual logging.

See https://pkg.go.dev/k8s.io/client-go@v0.33.1/tools/cache#SharedInformer for more info.

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
@inteon inteon requested review from Copilot and wallrj June 11, 2025 13:11
Copilot

This comment was marked as outdated.

@inteon inteon requested a review from Copilot June 11, 2025 13:21
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR replaces stop-channel parameters with context.Context across data gatherer implementations to enable contextual logging and aligns informer usage with client-go’s context-based APIs.

  • Refactors Run and WaitForCacheSync signatures in local, discovery, dynamic, and dummy gatherers to accept context.Context.
  • Updates agent code and tests to pass contexts instead of channels.
  • Switches informer handler registration to AddEventHandlerWithOptions with a logger and uses RunWithContext.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
pkg/datagatherer/local/local.go Changed Run/WaitForCacheSync to accept context.Context.
pkg/datagatherer/k8s/dynamic.go Removed stored ctx field; added RunWithContext, HandlerOptions logger; updated WaitForCacheSync.
pkg/datagatherer/k8s/dynamic_test.go Adapted tests to use new signatures and removed ctx assertions.
pkg/datagatherer/k8s/discovery.go Changed Run/WaitForCacheSync to accept context.Context.
pkg/datagatherer/datagatherer.go Updated DataGatherer interface to use context.Context.
pkg/agent/run.go Updated calls to Run/WaitForCacheSync to pass contexts.
pkg/agent/dummy_data_gatherer.go Changed Run/WaitForCacheSync to accept context.Context.
Comments suppressed due to low confidence (2)

pkg/datagatherer/k8s/dynamic.go:271

  • The comment here still refers to stopCh, but Run now takes a context.Context. Please update the doc to reflect that it blocks until the context is done.
// until the stopCh is closed. 

pkg/agent/run.go:191

  • This comment refers to a channel; since DataGatherer.Run now uses context.Context, consider updating the wording to describe blocking on ctx.Done().
// blocks until the supplied channel is closed. 
Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

Looks good. I prefer the use of context arguments instead of done channels...it's more familiar to me these days.

But I'd like to see some example output, demonstrating the new logging that is generated by this change and what level it is logged at.

@inteon
Copy link
Contributor Author

inteon commented Jun 11, 2025

This change has no effects on the logs when there are not critical errors (crashes in OnAdd etc). Also, since we were already setting the global logger, json logging was already working for the messages that were logged by the informer.
This PR future proofs us a bit and allows us to add more attributes to the informer logs in the future.

@inteon inteon merged commit c080864 into master Jun 11, 2025
2 checks passed
@wallrj wallrj deleted the pass_context branch June 11, 2025 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants