[chore][k8sattributesprocessor] Start informers in the provider function #39301
Conversation
swiatekm
requested review from
dmitryax,
fatsheep9146,
TylerHelmuth,
ChrsMark and
a team
as code owners
swiatekm
force-pushed
the
chore/k8sattributes/refactor-informer-start
branch
2 times, most recently
from
0bbecfc to
278adbc
Compare
swiatekm
changed the title
Also remove handlers when stopping the processor. # Conflicts: # processor/k8sattributesprocessor/internal/kube/client.go
swiatekm
force-pushed
the
chore/k8sattributes/refactor-informer-start
branch
from
278adbc to
722c85f
Compare
|
Please address conflict |
| @@ -168,13 +180,13 @@ func New( | |||
| return nil, err | |||
| } | |||
|
|
|||
| c.namespaceInformer = newNamespaceInformer(c.kc) | |||
| c.namespaceInformer = newNamespaceInformer(c.kc, c.stopCh) | |||
There was a problem hiding this comment.
What bugs me with the current state of this change is that the new*Informer() methods not only create a new informer but also start it. These are also called from the New() method that creates the k8s Client.
Is the plan to further refactor this in the upcoming follow-ups? The constructors should not be responsible for starting the Informers.
My understanding of the end goal is that the kube Client and its informers will be initialised once, and then get started once when the first processor's instance gets started (the rest will re-use the already started one), right?
If we agree on this, let's add some todo comments here so as to make it clear for the future PRs.
There was a problem hiding this comment.
The idea is that the client gets an informer that is already running, and by closing the stop channel, it signals that it doesn't want to use it anymore. There is no idempotent Start method for informers, I'd have to add our own wrapper around them, which I'd rather avoid.
I could move obtaining the informers completely into Start. There aren't really a lot of benefits to doing it in New, as the construction methods can't return errors anyway. Does that sound better?
There was a problem hiding this comment.
Can we clarify at which point the informers get started, and how they end up being attached to a specific client of a specific k8sattributes processor instance?
If I don't miss any details here it sounds like the shared informers should be created and already be in running state when the Start will be called. Or, the first Start that will be called takes the responsibility to start them and then the following ones re-use the existing informers (when applicable). Is this correct?
Can we decide on how we plan to apply this logic, to avoid back and forth in follow up PRs?
There was a problem hiding this comment.
Sure, let's move this discussion to the issue. I'll describe what I have in mind there.
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Description
We'd like instances of the k8sattributes processor to share local K8s resource caches (represented by the
informerconcept from client-go). See #36234 for the justification and a broad overview. This change is the first in a series of refactors necessary to make this possible.In order to share informers, the processor cannot be responsible for starting them. This PR moves that logic to informer provider functions. Informers are still stopped by closing a channel provided at construction.
The processor also makes no assumptions about whether the informers will keep running after
Stopis called. As a result, it needs to explicitly track its own event handler registration, and unregister in an orderly matter when stopping.See #36604 for all the refactoring changes put together. I've split this PR out to make review easier.
Link to tracking issue
Part of #36234
Split out from #36604
Testing
Modified existing tests.