Conversation
64b342a to
962b390
Compare
This was referenced Oct 2, 2024
962b390 to
3fe2e43
Compare
3fe2e43 to
d7f0a73
Compare
mauri870
reviewed
Oct 3, 2024
Co-authored-by: Mauri de Souza Meneguzzo <mauri870@gmail.com>
| var cachedObject runtime.Object | ||
|
|
||
| store = informer.GetStore() | ||
| queue = workqueue.NewNamed(name) |
There was a problem hiding this comment.
Suggested change
| queue = workqueue.NewNamed(name) | |
| queue = workqueue.NewWithConfig(QueueConfig{ | |
| Name: name, | |
| }) |
nit: since you touch this code maybe you wanna deal with deprecated calls as well
Member
Author
There was a problem hiding this comment.
I actually want to avoid touching it as much as possible. I think the recommended way here is to use typed queues, and I don't want to make that kind of change in this PR.
Contributor
There was a problem hiding this comment.
@swiatekm could you make an issue or PR (if it's quicker but I doubt that) to address the deprecated calls? That way we don't lose track of this work. Thanks.
pkoutsovasilis
approved these changes
Oct 3, 2024
pkoutsovasilis
left a comment
There was a problem hiding this comment.
Aside some nitpicking this LGTM
Contributor
💚 Build Succeeded
History
|
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
5 participants
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.
Introduce metadata-only watchers to the kubernetes package. These are useful if we only need to track metadata for a resource - a good example are ReplicaSets, for which we usually only care about the OwnerReferences. As a result, we only store the metadata, reducing steady-state memory consumption, but also only get updates involving metadata, reducing churn greatly in larger clusters.
The implementation introduces new constructors for the Watcher, allowing an informer to be passed in. Existing constructors are implemented using the new constructor, though none of the code actually changes. As a result, it is now possible to unit test the watcher, and I've added some basic unit tests for it.
We also add two helper functions:
GetKubernetesMetadataClientcreates a metadata-only kubernetes client, and is very similar to the existingGetKubernetesClientRemoveUnnecessaryReplicaSetDatais a transform function that can be passed into an informer so it only stores the metadata we actually useI tested these new functions in both beats and agent, in a kind cluster as well as one of our staging clusters.
This is part of the solution to elastic/elastic-agent#5580.