[crashtracking] improve poll waiting logic #754
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
BenchmarksComparisonBenchmark execution time: 2024-11-25 20:51:20 Comparing candidate commit 8074728 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 51 metrics, 2 unstable metrics. CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
Group 12
BaselineOmitted due to size. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #754 +/- ##
==========================================
- Coverage 70.49% 70.47% -0.02%
==========================================
Files 297 297
Lines 43401 43411 +10
==========================================
Hits 30595 30595
- Misses 12806 12816 +10
|
pawelchcki
reviewed
Nov 25, 2024
| _ => Err(anyhow::anyhow!("poll returned unexpected result")), | ||
| }, | ||
| let mut poll_fds = [pollfd { | ||
| fd: target_fd, |
There was a problem hiding this comment.
+1 - BorrowedFd prefferably should be use in conjuction with OwnedFd. borrow_raw - without any guarantees of FD lifetime is problematic.
Probably the safest option would be to dup the fd - and own it within the context of this function.
Otherwise the code looks like correct but "C'ish" rust :)
pawelchcki
reviewed
Nov 25, 2024
| let reaping_allowed_ms = std::cmp::min( | ||
| timeout_ms.saturating_sub(start_time.elapsed().as_millis() as u32), | ||
| DD_CRASHTRACK_MINIMUM_REAP_TIME_MS, | ||
| ); | ||
|
|
||
| let _ = reap_child_non_blocking(receiver_pid_as_pid, reaping_allowed_ms); |
There was a problem hiding this comment.
In sidecar - we send kill and term. When the timeout ends.
And it looks that - we're not doing that here either way - so a non 0 timeout will only reduce the incidence of zombies. Not prevent them.
There was a problem hiding this comment.
Don't we do that just above? https://github.com/DataDog/libdatadog/blob/main/crashtracker/src/collector/crash_handler.rs#L483
pawelchcki
approved these changes
Nov 25, 2024
danielsn
reviewed
Nov 25, 2024
| _ => Err(anyhow::anyhow!("poll returned unexpected result")), | ||
| }, | ||
| let mut poll_fds = [pollfd { | ||
| fd: target_fd, |
| revents: 0, | ||
| }]; | ||
|
|
||
| match unsafe { poll(poll_fds.as_mut_ptr(), 1, timeout_ms) } { |
There was a problem hiding this comment.
style: this should be .len not constant 1
| revents if revents.contains(PollFlags::POLLHUP) => Ok(true), | ||
| _ => Err(anyhow::anyhow!("poll returned unexpected result")), | ||
| }, | ||
| let mut poll_fds = [pollfd { |
There was a problem hiding this comment.
comment to explain the meaning of the boolean result
| let reaping_allowed_ms = std::cmp::min( | ||
| timeout_ms.saturating_sub(start_time.elapsed().as_millis() as u32), | ||
| DD_CRASHTRACK_MINIMUM_REAP_TIME_MS, | ||
| ); | ||
|
|
||
| let _ = reap_child_non_blocking(receiver_pid_as_pid, reaping_allowed_ms); |
There was a problem hiding this comment.
Don't we do that just above? https://github.com/DataDog/libdatadog/blob/main/crashtracker/src/collector/crash_handler.rs#L483
This was referenced Nov 25, 2024
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.
What does this PR do?
The original implementation accidentally had a mutable array with immutable objects, causing the interface to always throw errors. Since this part of the code is in the critical path for handling zombie processes, this condition had an adverse side-effect on customer infrastructure.
This code also used a
BorrowedFd, which is supposed to track anOwnedFd. This was problematic in some conditions, since the underlying implementation would useprctl()to check file descriptor liveness and panic in some edge-cases. The code has been ported to libc, using exclusivelyRawFd, in order to prevent this condition.Finally, this patch grants some additional time to the act of reaping a PID. When a receiver process exceeds its timeout budget, it's sent a
SIGKILL. However, the old behavior was toSIGKILL, the immediatelywaitpid( pid, ..., WNOHANG). On a saturated system (i.e., precisely the kind of system where a timeout might be necessary!), it may take some time for the receiver PID to respond to theSIGKILL.In general, there's no way to provided a bounded guarantee for the duration of this reap operation, so an arbitrary number of scheduler slices is chosen as the maximum reaping wait duration.
Motivation
Fix zombies