-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Remove unnecessary restarts of metricsets while using Node autodiscover #19974
Remove unnecessary restarts of metricsets while using Node autodiscover #19974
Conversation
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
1 similar comment
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
32d6c6c
to
f205ee0
Compare
Pinging @elastic/integrations-platforms (Team:Platforms) |
jenkins run the tests please |
Thank you for opening, I agree @ChrsMark we could do something similar with Pods (an the others). A note on using reflect: I've checked this discussion kubernetes-sigs/kubebuilder#592 and it seems that there are better alternatives: Option 1 (I think this one is preferred, as we probably have this package already):
Option 2: Using https://github.com/banzaicloud/k8s-objectmatcher, which doesn't look bad either WDYT? |
f205ee0
to
57af22b
Compare
@exekias done. |
💔 Tests FailedExpand to view the summary
Build stats
Test stats 🧪
Test errorsExpand to view the tests failures
Steps errorsExpand to view the steps failures
Log outputExpand to view the last 100 lines of log output
|
Test failure looks related: https://travis-ci.org/github/elastic/beats/jobs/709358439#L703 |
57af22b
to
bfc2c5c
Compare
@ChrsMark , fixed. |
newCopy.ResourceVersion = "" | ||
|
||
// If the old object and new object are different in either meta or spec then there is a valid change | ||
fmt.Println(equality.Semantic.DeepDerivative(oldCopy.Spec, newCopy.Spec), equality.Semantic.DeepDerivative(oldCopy.ObjectMeta, newCopy.ObjectMeta)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spurious Println
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apologies. removing
libbeat/common/kubernetes/watcher.go
Outdated
@@ -123,7 +125,8 @@ func NewWatcher(client kubernetes.Interface, resource Resource, opts WatchOption | |||
new, _ := accessor.ResourceVersion(n.(runtime.Object)) | |||
|
|||
// Only enqueue changes that have a different resource versions to avoid processing resyncs. | |||
if old != new { | |||
// Also if there is a registered function to check for updates, use it to check for updates. | |||
if old != new && opts.IsUpdated != nil && opts.IsUpdated(o, n) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isUpdated
will be nil for all other resources (Pod, Service...), so this will never be true for them.
Perhaps we should have a default isUpdated
function, when the option is not present that does just the old != new
check?
bfc2c5c
to
ba464e1
Compare
jenkins run the tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! thanks @vjsamuel!
There is a conflict in changelog we need to fix.
https://travis-ci.org/github/elastic/beats/builds/712100705 seems like a flake. |
jenkins run the tests please |
Failing tests seem unrelated, merging. Thanks for contributing this @vjsamuel ! |
…er (elastic#19974) (cherry picked from commit 06f05ca)
…ne-2.0 * upstream/master: (29 commits) Add an explicit system test for processes on unix systems (elastic#20320) [Autodiscovery] Ignore ErrInputNotFinished errors in autodiscover config checks (elastic#20305) [CI] Update README.md with CI references (elastic#20316) Add ECK doc links to Heartbeat docs (elastic#20284) [Filebeat] Add export tests to x-pack/filebeat (elastic#20156) feat(ci): support building docker images for PRs (elastic#20323) Update system tests dependency (elastic#20287) [Libbeat] Log debug message if the Kibana dashboard can not be imported from the archive (elastic#12211) (elastic#20150) [Filebeat][Gsuite] Transform all dates to timestamp with processor (elastic#20308) Infer types in Prometheus remote_write (elastic#19944) Remove unnecessary restarts of metricsets while using Node autodiscover (elastic#19974) docs: update changelog on master branch (elastic#20259) feat(ci): support storing artifacts for PRs in separate dirs (elastic#20282) [CI] Change upstream reference (elastic#20296) [Filebeat] Updates to Suricata module (elastic#20220) [docs] Fix Windows download link for agent (elastic#20258) [docs] Rename release highlights to what's new (elastic#20255) fix: update the display name of the multibranch job (elastic#20265) [Elastic Agent] Add basic protocol to control Elastic Agent. (elastic#20146) Cisco ASA: Fix message 106100 (elastic#20245) ...
Bug
What does this PR do?
Everytime the kubelet does a check for node health it posts an update to the node status. Each change even if overall status doesnt change then it causes the metricset to restart. This PR adds a watcher option to register a func that checks in detail if the update is valid or not.
Why is it important?
This makes sure that the kubelet status updates dont prompt a restart unless the overall status changes.
Checklist
I have made corresponding changes to the documentationI have made corresponding change to the default configuration filesCHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Author's Checklist
How to test this PR locally
Related issues