plugin/kubernetes: remove skipApiCleanup boolean #4244
Conversation
Signed-off-by: Miek Gieben <miek@miek.nl>
miekg
requested review from
bradbeam,
chrisohaver,
johnbelamaric,
rajansandeep,
yongtang and
zouyee
as code owners
Presumably because the object is still referenced by the informer, so “zeroing” it out at least gets the things it referenced to be GCd. (I’m just presuming. I don’t know for sure) |
|
[ Quoting <notifications@github.com> in "Re: [coredns/coredns] plugin/kubern..." ]
IIRC, you had originally added the clean up step at the end of the To
functions to help with memory footprint reduction.
hmm, good point, but wrapped in an if-statement iirc?
|
|
yes, zeroing those values makes tests fail; this means that those tests are using code that's not used in prod. I'm not sure who much memory we are saving with doing this, or if it can be GCed... ? |
FWIW, I think this is the squashed commit message for adding that (from 830e97f#diff-642b4aa397e466d410076c727182863bf09f5724a41f20ad14431bc3e86599ae)... Unclear if it was done based on test results. |
|
... and the PR comment: #2128 (comment) |
|
[ Quoting <notifications@github.com> in "Re: [coredns/coredns] plugin/kubern..." ]
yes, zeroing those values makes tests fail; this means that those tests are
using code that's not used in prod.
I'm not sure who much memory we are saving with doing this, or if it can be
GCed... ?
FWIW, I think this is the squashed commit message for adding that (from 830e97f
#diff-642b4aa397e466d410076c727182863bf09f5724a41f20ad14431bc3e86599ae)...
* Set incoming object to nil
Explicataliy discard the converted object; we did a deep copy it's
not needed anymore.
Signed-off-by: Miek Gieben ***@***.***>
Unclear if it was done based on test results.
it wasn't, difficult to test properly as well, setting to `nil` is save and quarantees the
object is gone and not referenced anymore (and thus gc-ed)
|
|
Actually - running through the PR conversation - it looks like memory footprint shrank significantly with this change. |
Signed-off-by: Miek Gieben <miek@miek.nl>
|
[ Quoting <notifications@github.com> in "Re: [coredns/coredns] plugin/kubern..." ]
Actually - running through the PR conversation - it looks like memory footprint
shrank significantly with this change.
footprint reduced by 10% prior to nilling, and then 27% with the nilling.
thanks for digging that up.
Then we left with the question that tests need a flag the specifically changes how
production works. I don't think that are good tests.
|
It's not ideal, but I think better than not having tests at all. The valid concern is that the latency metric feature itself (not just the tests) might depend on the original api objects being left intact. The original defense of adding the We might need to test this in coredns/ci ... in the kubernetes cluster tests. We would not be able to check for specific latency values, like we do in these unit tests, but we could test that the metric produces "something reasonable". Or perhaps we could if we manually modify the endpoint annotations (assuming that doesn't break stuff). |
|
[ Quoting <notifications@github.com> in "Re: [coredns/coredns] plugin/kubern..." ]
We might need to test this in coredns/ci ... in the kubernetes cluster tests.
I think that's a better approach, cleaner code and and better e2e testing.
/Miek
…--
Miek Gieben
|
This was purely added to facilitate some testing, meaning the tests don't test production code. Remove the metrics and informer tests, these should be moved to coredns/ci to be properly e2e tested. Closes: #4244 Signed-off-by: Miek Gieben <miek@miek.nl>
Signed-off-by: Miek Gieben miek@miek.nl