[Heartbeat] Fix incorrect 'Up' status for all mode #11895
Merged
Conversation
This file contains 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
andrewvc
added
in progress
|
Pinging @elastic/uptime |
houndci-bot
reviewed
Apr 23, 2019
houndci-bot
reviewed
Apr 23, 2019
andrewvc
added
review
and removed
in progress
andrewvc
added
bug
needs_backport
andrewvc
changed the title
ruflin
approved these changes
Apr 24, 2019
heartbeat/eventext/eventext.go
Outdated
| @@ -30,3 +30,22 @@ func MergeEventFields(e *beat.Event, merge common.MapStr) { | |||
| e.Fields = merge | |||
| } | |||
| } | |||
|
|
|||
| // EventCalledMetaKey is the full path to the key marking an event as cancelled. | |||
| const EventCancelledMetaKey = "meta.__hb_evt_cancel__" | |||
There was a problem hiding this comment.
Nit: Leave out the meta. as otherwise if you use Put with a . in the key it will create a tree and then the Get has to traverse the tree. Not having this would make the checks more efficient assuming this will happen on a lot of times.
There was a problem hiding this comment.
Yeah, it is unfortunate that this is more expensive, but otherwise it winds up in event.Fields which is even hackier. I'm not really worried about the perf here since that should be very fast, even though it isn't ideal. WDYT?
| logp.Err("Job %v failed with: ", err) | ||
| } | ||
|
|
||
| if event.Fields != nil && !eventext.IsEventCancelled(event) { |
There was a problem hiding this comment.
Previously there was an event != nil check here. I assume because of :138 this cannot happen?
There was a problem hiding this comment.
That is correct. My IDE actually flagged that as redundant.
There was a problem hiding this comment.
event is being passed as pointer to job which could nil it, but that's something we wont ever do as far as I am concerned
|
@andrewvc I'm getting a panic running the target scenario It seems that MapStr.DeepUpdate is not thread safe and that routines 114 and 115 are writing concurrently. |
|
Good catch @odacremolbap . I wasn't seeing the segfault consistently, but running I don't think we really introduced the bug here, we just sort of exposed it. After a lot of head scratching I found the root cause in dd9fe1168 , where we re-used maps across continuations. Tested this by running heartbeat.monitors:
- type: http
urls: ["http://elastic.co:9200"]
mode: all
schedule: '@every 10s' |
houndci-bot
reviewed
Apr 26, 2019
heartbeat/eventext/eventext.go
Outdated
| } | ||
| } | ||
|
|
||
| // EventCalledMetaKey is the path to the @metadata key marking an event as cancelled. |
There was a problem hiding this comment.
comment on exported const EventCancelledMetaKey should be of the form "EventCancelledMetaKey ..."
houndci-bot
reviewed
Apr 26, 2019
houndci-bot
reviewed
Apr 26, 2019
|
@ruflin @odacremolbap I've made a final push here to fix the concurrency in a more general way by just cloning in 237b8a8 . We could do it better in the future with #11941 which avoids some of the complexity of |
This reverts commit dd9fe116838bbbb5f60a2cc0921f1f5c7d071ce1.
ruflin
approved these changes
Apr 26, 2019
There was a problem hiding this comment.
Skimmed through it again and looks but (with few nits), but would be good if @odacremolbap could take a look again.
|
CI failures look related. |
|
Tests are now all passing. Can I get a final LGTM here @ruflin |
andrewvc
added a commit
to andrewvc/beats
that referenced
this pull request
May 1, 2019
This fixes elastic#11737 , where setting mode: all now spawns multiple sub-tasks, but the parent task still runs as a job, even though it tests very little, and will always succeed so long as the DNS query does. It is essentially a parent job testing that the DNS resolver works. From a certain strict POV the behavior before this patch is correct. We executed a discrete job (checking DNS) and it worked successfully. A second part of the job, actually hitting the endpoints, failed. The total job (as noted in the summary) did fail, but a sub-part did succeed. That said, this is too complex and a bad UX, so this patch makes sense. Maybe in the future we will be under different constraints. The fix here involved marking that job's emitted event as cancelled using job metadata. I'd discussed using event.private for that with @ruflin, but looking at the code metadata seemed more appropriate. I'd love to hear your $0.02 @ruflin if you think private is more appropriate. Metricbeat wraps all events in its own metadata struct, but I don't think we're there yet in heartbeat in terms of a major refactor being justified. Testing this manually is easy, just point at a domain with multiple A records, like elastic.co. Truly integration testing it programmatically is hard without setting up a DNS resolver. The compromise I reached here is unit tests for the individual bits of logic. Fixes elastic#11737 (cherry picked from commit 078612e)
andrewvc
added
v7.0.1
and removed
needs_backport
|
@andrewvc since this is a 6.6 to 6.7 regression will this be backported to 6.x ? Mostly asking at this point since I don’t know how this is decided. On a unrelated note, I also wanted to thank you for your work on this issue. |
andrewvc
added a commit
that referenced
this pull request
May 2, 2019
This fixes #11737 , where setting mode: all now spawns multiple sub-tasks, but the parent task still runs as a job, even though it tests very little, and will always succeed so long as the DNS query does. It is essentially a parent job testing that the DNS resolver works. From a certain strict POV the behavior before this patch is correct. We executed a discrete job (checking DNS) and it worked successfully. A second part of the job, actually hitting the endpoints, failed. The total job (as noted in the summary) did fail, but a sub-part did succeed. That said, this is too complex and a bad UX, so this patch makes sense. Maybe in the future we will be under different constraints. The fix here involved marking that job's emitted event as cancelled using job metadata. I'd discussed using event.private for that with @ruflin, but looking at the code metadata seemed more appropriate. I'd love to hear your $0.02 @ruflin if you think private is more appropriate. Metricbeat wraps all events in its own metadata struct, but I don't think we're there yet in heartbeat in terms of a major refactor being justified. Testing this manually is easy, just point at a domain with multiple A records, like elastic.co. Truly integration testing it programmatically is hard without setting up a DNS resolver. The compromise I reached here is unit tests for the individual bits of logic. Fixes #11737 (cherry picked from commit 078612e)
leweafan
pushed a commit
to leweafan/beats
that referenced
this pull request
Apr 28, 2023
…lastic#12007) This fixes elastic#11737 , where setting mode: all now spawns multiple sub-tasks, but the parent task still runs as a job, even though it tests very little, and will always succeed so long as the DNS query does. It is essentially a parent job testing that the DNS resolver works. From a certain strict POV the behavior before this patch is correct. We executed a discrete job (checking DNS) and it worked successfully. A second part of the job, actually hitting the endpoints, failed. The total job (as noted in the summary) did fail, but a sub-part did succeed. That said, this is too complex and a bad UX, so this patch makes sense. Maybe in the future we will be under different constraints. The fix here involved marking that job's emitted event as cancelled using job metadata. I'd discussed using event.private for that with @ruflin, but looking at the code metadata seemed more appropriate. I'd love to hear your $0.02 @ruflin if you think private is more appropriate. Metricbeat wraps all events in its own metadata struct, but I don't think we're there yet in heartbeat in terms of a major refactor being justified. Testing this manually is easy, just point at a domain with multiple A records, like elastic.co. Truly integration testing it programmatically is hard without setting up a DNS resolver. The compromise I reached here is unit tests for the individual bits of logic. Fixes elastic#11737 (cherry picked from commit 9e80988)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This fixes #11737 , where setting
mode: allnow spawns multiple sub-tasks, but the parent task still runs as a job, even though it tests very little, and will always succeed so long as the DNS query does. It is essentially a parent job testing that the DNS resolver works.From a certain strict POV the behavior before this patch is correct. We executed a discrete job (checking DNS) and it worked successfully. A second part of the job, actually hitting the endpoints, failed. The total job (as noted in the summary) did fail, but a sub-part did succeed. That said, this is too complex and a bad UX, so this patch makes sense. Maybe in the future we will be under different constraints.
The fix here involved marking that job's emitted event as cancelled using job metadata. I'd discussed using
event.privatefor that with @ruflin, but looking at the code metadata seemed more appropriate. I'd love to hear your $0.02 @ruflin if you think private is more appropriate. Metricbeat wraps all events in its own metadata struct, but I don't think we're there yet in heartbeat in terms of a major refactor being justified.Testing this manually is easy, just point at a domain with multiple A records, like
elastic.co. Truly integration testing it programmatically is hard without setting up a DNS resolver. The compromise I reached here is unit tests for the individual bits of logic.Fixes #11737