windows: fix inefficient gathering of task processes #20619
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
shoenig
force-pushed
the
process-stats-pace
branch
from
c3df21a
to
a0390be
Compare
shoenig
force-pushed
the
process-stats-pace
branch
from
a0390be
to
3a40893
Compare
tgross
added
the
stage/needs-rebase
shoenig
force-pushed
the
process-stats-pace
branch
from
3a40893
to
8005339
Compare
shoenig
force-pushed
the
process-stats-pace
branch
from
8005339
to
106d2e9
Compare
shoenig
force-pushed
the
process-stats-pace
branch
from
106d2e9
to
5f89ecb
Compare
shoenig
added
backport/ent/1.7.x+ent
tgross
approved these changes
May 17, 2024
|
|
||
| all, err := processes() | ||
| if err != nil { | ||
| return set.New[ProcessID](0) |
There was a problem hiding this comment.
Could we at least return the executor PID here? Or is the thinking that if this call fails we can't trust any of the results?
There was a problem hiding this comment.
Oh good idea, let me add that
|
@shoenig you don't need a 1.8.x backport label on this, as we'll cut the RC release from main |
Yeah we could definitely add base cases for:
But at that point you're optimizing microseconds when the original problem is this horrible process table snapshot syscall that takes 10's of milliseconds. I'd rather the code just be readable. |
tgross
added a commit
that referenced
this pull request
Oct 14, 2024
In #20619 we overhauled how we were gathering stats for Windows processes. Unlike in Linux where we can ask for processes in a cgroup, on Windows we have to make a single expensive syscall to get all the processes and then build the tree ourselves. Our algorithm to do so is recursive and quadratic in both steps and space with the number of processes on the host. For busy hosts this hit the stack limit and panic the Nomad client. We already build a map of parent PID to PID, so modify this to be a map of parent PID to slice of children and then traverse that tree only from the root we care about (the executor PID). This moves the allocations to the heap but makes the stats gathering linear in steps and space required. Fixes: #23984
tgross
added a commit
that referenced
this pull request
Oct 14, 2024
…ar space (#24182) In #20619 we overhauled how we were gathering stats for Windows processes. Unlike in Linux where we can ask for processes in a cgroup, on Windows we have to make a single expensive syscall to get all the processes and then build the tree ourselves. Our algorithm to do so is recursive and quadratic in both steps and space with the number of processes on the host. For busy hosts this hits the stack limit and panics the Nomad client. We already build a map of parent PID to PID, so modify this to be a map of parent PID to slice of children and then traverse that tree only from the root we care about (the executor PID). This moves the allocations to the heap but makes the stats gathering linear in steps and space required. This changeset also moves as much of this code as possible into an area not conditionally-compiled by OS, as the tagged test file was not being run in CI. Fixes: #23984
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 PR overhauls the way processes are scanned on Windows for the exec driver(s).
A regression was introduced in Nomad 1.7 where we would invoke an expensive syscall for every descendant of a process to build a tree of the processes associated with a task. Instead, we restore the behavior of scanning the process table once and rebuild the process tree manually.
Fixes #20042