batches: use Docker CPU count as default parallelism, not GOMAXPROCS#786
Merged
Conversation
eseliger
approved these changes
Jun 15, 2022
| SkipErrors: opts.flags.skipErrors, | ||
| CleanArchives: opts.flags.cleanArchives, | ||
| Parallelism: opts.flags.parallelism, | ||
| Parallelism: parallelism, |
Member
There was a problem hiding this comment.
fun fact, it's unused in SSBC 😆 PR cleaning this mess here up coming soon :)
| defer cancel() | ||
|
|
||
| args := []string{"info", "--format", "{{ .NCPU }}"} | ||
| out, err := exec.CommandContext(dctx, "docker", args...).CombinedOutput() |
Contributor
Author
There was a problem hiding this comment.
Combined — Docker outputs all non-container-output on stderr.
Co-authored-by: Erik Seliger <erikseliger@me.com>
Piszmog
approved these changes
Jun 16, 2022
scjohns
pushed a commit
that referenced
this pull request
Apr 24, 2023
…786) Co-authored-by: Erik Seliger <erikseliger@me.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 changes the default parallelism of
src batch preview|applyruns to be based on the CPU cores reported bydocker info.Practically, this means that Docker Desktop users will now have their parallelism based on the VM Docker runs within, not their host machine. The trade off here is that Docker Desktop users will likely have their default parallelism cut significantly, but this is also likely to make issues caused by the Docker VM having its resources exhausted considerably less likely.
Implementation notes
docker versionpreflight to usedocker infoinstead, which means we can get the CPU count and preflight Docker at the same time.docker infoshould always be quick, this reuses the same timeout behaviour as docker: use a short timeout when inspecting images #783, which has been renamed from beingimage inspectspecific to being used for any "fast commands" issued to Docker. The behaviour is otherwise unchanged.os/execis inconsistent in terms of the error returned when a deadline is exceeded; I've updated both places we use the fast command context to look both at the returned error and the context error.Screenshot of the timeout
Test plan
Added unit tests, and tested this in various scenarios locally (Docker not running; Docker running but unresponsive; Docker running normally).