Conditions on podupdater functions of kubernetes autodiscovery #71
Conversation
gizas
requested review from
belimawr,
fearful-symmetry,
gsantoro,
MichaelKatsoulis,
tetianakravchenko and
constanca-m
and removed request for
a team
There was a problem hiding this comment.
I left this comment elastic/beats#37338 (comment) on the issue. It seems to work, so I am approving it.
kubernetes/eventhandler.go
Outdated
| @@ -134,19 +138,25 @@ type podUpdaterStore interface { | |||
| List() []interface{} | |||
| } | |||
|
|
|||
| type UpdateWatcher interface { | |||
There was a problem hiding this comment.
@gizas could you please add Doc comments for the exported type?
kubernetes/eventhandler.go
Outdated
|
|
||
| // Slice includes the old and new version of caching object that changes in the current update event. slice[0] is the old version and slice[1] the new updated one | ||
| slice := n.nodewatcher.Deltaobjects() | ||
| cachednodeold, ok := slice[0].(*Node) |
There was a problem hiding this comment.
slice[1] seems to be not used and instead the old version (slice[0]) is compared with the obj.(*Node) passed to the OnUpdate. The same for the namespace
Do we actually need the updated version?
There was a problem hiding this comment.
I don't get the meaning of checking the current namespace name is the same as the cached version. Maybe you can clarify that in a comment. Should we instead check the new name has not changed compared to the cached version?
There was a problem hiding this comment.
@gsantoro the one you call "current" is the one that triggers the OnUpdate. So still the "current" is not saved in the cache until we make the checks. The actual checks to make the replacement is the ones that come from isEqualMetadata function. And if we have diffrences in metadata we proceed with the Pod updates
FYI : The check we make ns.Name == cachednamespaceold.Name is just for safety in order to make sure that we talk for the same object in cache
@tetianakravchenko indeed only the old version is needed (former slice[0]). I initially thought the wather to rerutn only the old version. Then I changed my mind that probably we will need in the future the deltas if we need to make more checks. I dont think that it is a big diff to return both? But tbh I have not tested the memory in scale here
There was a problem hiding this comment.
@tetianakravchenko indeed only the old version is needed (former slice[0]). I initially thought the wather to rerutn only the old version. Then I changed my mind that probably we will need in the future the deltas if we need to make more checks. I don't think that it is a big diff to return both? But tbh I have not tested the memory in scale here
since obj.(*Node) passed to the OnUpdate is equal to the slice[1] (new version) - then we are just duplicating data here and I can't imagine any use case to use this slice[1] for future (instead of obj.(*Node)), do you have anything in mind?
I also don't think that it is a big diff to return, but I think it is better to not introduce obscurity/duplication
There was a problem hiding this comment.
@tetianakravchenko I was biased due to this and was thinking that we might use it in the future. But not needed for now for sure.
I chanhed again the code only to return old object.
Co-authored-by: Tetiana Kravchenko <[email protected]>
Co-authored-by: Tetiana Kravchenko <[email protected]>
Co-authored-by: Tetiana Kravchenko <[email protected]>
kubernetes/eventhandler.go
Outdated
| locker: locker, | ||
| handler: handler, | ||
| store: store, | ||
| namespacewatcher: namespacewatcher, |
There was a problem hiding this comment.
can we use MixedCaps (aka camelCase) here and for all other variables? as suggested at https://go.dev/doc/effective_go?
There was a problem hiding this comment.
Update in 16b0fca
Let me know If I missed anything
Co-authored-by: Giuseppe Santoro <[email protected]>
Co-authored-by: Giuseppe Santoro <[email protected]>
kubernetes/eventhandler.go
Outdated
| @@ -209,16 +208,16 @@ func (*namespacePodUpdater) OnDelete(interface{}) {} | |||
| type nodePodUpdater struct { | |||
| handler podUpdaterHandlerFunc | |||
| store podUpdaterStore | |||
| nodewatcher OldobjectWatcher | |||
| nodeWatcher oldObjectWatcher | |||
There was a problem hiding this comment.
sorry I meant to say to replace any other reference to oldObject -> cachedObject
There was a problem hiding this comment.
Done @gsantoro
thank for insisting on this
Also relevant updates done in tests of elastic/beats#37431
Could you also have a look and approve there as well?
|
@fearful-symmetry or @belimawr can I have a review here as codeowners? |
Co-authored-by: Tetiana Kravchenko <[email protected]>
💚 Build Succeeded
History
|
Proposed commit message
How to test this PR locally
See info elastic/beats#37338
Related issues