-
Notifications
You must be signed in to change notification settings - Fork 392
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
watcher: Refactor K8sWatcher to reuse config and factories #3413
Open
lambdanis
wants to merge
6
commits into
cilium:main
Choose a base branch
from
lambdanis:pr/lambdanis/factory
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+435
−350
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
There are no functional changes in this commit, only moving code around. It's a preparation to further refactoring of the k8s watcher that will decouple the core k8s watcher and resource-specific functionalities, and allow for reusing of informer factories. Also removed unused global constants. Signed-off-by: Anna Kapuscinska <[email protected]>
K8sResourceWatcher interface covers two distinct functionalities: setting up Kubernetes informers and accessing information about pods. These make sense as separate interfaces, so the K8sResourceWatcher methods for accessing pods are split out into a dedicated interface, PodAccessor, and setup methods - into Watcher interface. The K8sResourceWatcher interface stays in place for now, embedding both. Signed-off-by: Anna Kapuscinska <[email protected]>
The previous K8sWatcher implementation allowed extending it with extra informers via AddInformers method, but didn't provide a way to reuse shared informer factories between AddInformers calls. That's wasteful, as the whole point of a factory is to be reused. This commit adds shared informer factories to the K8sWatcher struct: one for global k8s built-in resources, one for node-local k8s built-in resources, and one for CRDs. They are initialized in NewK8sWatcher function. The AddInformers method is replaced by AddInformer, which simply adds a single informer with indexers to the list. The Start method starts all informer factories - thanks to storing them, there is no more need to dynamically extend a start function. Initialization of the pod informer is moved out of NewK8sWatcher to a dedicated function called from main, AddPodInformer. The idea is that K8sWatcher provides a config and interface for adding informers, and different packages can use it to watch specific resourced. A following commit will use this functionality for watching TracingPolicy CRD. Signed-off-by: Anna Kapuscinska <[email protected]>
Let's use a more descriptive name. Also renamed crd.go to tracingpolicy.go. There are no functional changes in this commit. Signed-off-by: Anna Kapuscinska <[email protected]>
Unexport informer factories in the K8sWatcher struct and implement exported Get* methods instead. This will make it easier to reuse them and test. Signed-off-by: Anna Kapuscinska <[email protected]>
12ec831
to
76ec341
Compare
✅ Deploy Preview for tetragon ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Before, to watch TracingPolicy(Namespaced) custom resources, we were reading Kubernetes config, creating an informer factory and starting it, all in the WatchTracePolicy function. As now the watcher.K8sWatcher struct initializes informer factories, we can reuse it instead of configuring everything from scratch. This commit replaces the WatchTracePolicy function with AddTracingPolicyInformer, which only adds informers to the K8sWatcher. Initialization and start of the K8sWatcher happen in main. Signed-off-by: Anna Kapuscinska <[email protected]>
76ec341
to
a051c49
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.
The previous K8sWatcher implementation allowed extending it with extra
informers via AddInformers method, but didn't provide a way to reuse shared
informer factories between AddInformers calls. That's wasteful, as the whole
point of a factory is to be reused.
This PR adds shared informer factories to the K8sWatcher struct: one for
global k8s built-in resources, one for node-local k8s built-in resources, and
one for CRDs. They are initialized in NewK8sWatcher function. The AddInformers
method is replaced by AddInformer, which simply adds a single informer with
indexers to the list. The Start method starts all informer factories - thanks
to storing them, there is no more need to dynamically extend a start function.
Initialization of the pod informer is moved out of NewK8sWatcher to a dedicated
function called from main, AddPodInformer.
Watching TracingPolicy(Namespaced) now reuses the K8sWatcher. Before, we were
reading Kubernetes config, creating an informer factory and starting it, all in
the WatchTracePolicy function. Now it's replaced by AddTracingPolicyInformer,
which only adds informers to the K8sWatcher.