Skip to content
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

Support spawn setting of 0 to mean number of cpus #2711

Merged
merged 2 commits into from Apr 17, 2024

Conversation

mmlb
Copy link
Contributor

@mmlb mmlb commented Apr 3, 2024

Description

When setting up the agent config some setups may not have the number of CPUs available but want to spawn one worker per CPU. In this case we can interpret 0 to mean all CPUs.

Other options I considered were treating spawn as a string and doing some simple math but this seems saner (not to mention simpler).

Context

I'd like to run 1 agent worker per process running under NixOS, declaratively so can't use nproc other means.

Testing

  • Tests have run locally (with go test ./...). Buildkite employees may check this if the pipeline has run automatically.
  • Code is formatted (with go fmt ./...)

moskyb
moskyb previously requested changes Apr 10, 2024
Copy link
Contributor

@moskyb moskyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey! i like the idea of this feature. the one issue i have is that i think that the behaviour of --spawn 0 would be a bit surprising having not read the help text - ie if you read it in a config file or similar.

how would you feel about implementing this as a separate flag called something like --spawn-per-cpu <some number>, and have the agent spawn ceil(cfg.SpawnPerCPU * runtime.NumCPU()) children, and make this flag mutually exclusive with --spawn?

@mmlb
Copy link
Contributor Author

mmlb commented Apr 10, 2024

Yep that works for me!

…r of cpus

When setting up the agent config some setups may not have the number of
CPUs available but want to spawn one worker per CPU, --spawn-per-cpu 1
will do this. We can even do more than 1-per-cpu if this host is meant
for light weight/io bound tasks for example.

Signed-off-by: Manuel Mendez <github@i.m.mmlb.dev>
@mmlb
Copy link
Contributor Author

mmlb commented Apr 10, 2024

@moskyb I've updated the PR. I'm not sure I follow your use case for ceil when the cli flag already implies scaling by the number of cpus. Can you clarify?

In the meantime here's the output of some manual testing:

[17:31:06]-[~/c/g/b/agent]-[manny@c3mnix]
./bk-agent start --build-path ../build-path --token some-uuid 2>&1 | grep -e  Registering -e fatal | grep -v POST
2024-04-10 17:31:06 INFO   Registering agent with Buildkite...
[17:31:12]-[~/c/g/b/agent]-[manny@c3mnix]
./bk-agent start --build-path ../build-path --token some-uuid --spawn 2 2>&1 | grep -e  Registering -e fatal | grep -v POST
2024-04-10 17:31:12 INFO   Registering agent 1 of 2 with Buildkite...
[17:31:20]-[~/c/g/b/agent]-[manny@c3mnix]
./bk-agent start --build-path ../build-path --token some-uuid --spawn-per-cpu 1 2>&1 | grep -e  Registering -e fatal | grep -v POST
2024-04-10 17:31:20 INFO   Registering agent 1 of 48 with Buildkite...
[17:31:25]-[~/c/g/b/agent]-[manny@c3mnix]
./bk-agent start --build-path ../build-path --token some-uuid --spawn-per-cpu 2 2>&1 | grep -e  Registering -e fatal | grep -v POST
2024-04-10 17:31:25 INFO   Registering agent 1 of 96 with Buildkite...
[17:31:35]-[~/c/g/b/agent]-[manny@c3mnix]
./bk-agent start --build-path ../build-path --token some-uuid --spawn 2 --spawn-per-cpu 2 2>&1 | grep -e  Registering -e fatal | grep -v POST
fatal: You can't specify spawn and spawn-per-cpu ath the same time

@mmlb mmlb requested a review from moskyb April 10, 2024 22:08
clicommand/agent_start.go Outdated Show resolved Hide resolved
Copy link
Contributor

@dabarrell dabarrell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making that change, @mmlb! The reference to ceil was an idea that we could make the flag a float, allowing for --spawn-per-cpu 0.5 etc, rounding up to ensure the result is an int. We'll get this merged in though, and we can potentially make that float change in the future!

@dabarrell dabarrell merged commit ba14a12 into buildkite:main Apr 17, 2024
1 check passed
@mmlb mmlb deleted the spawn-0-workers branch April 17, 2024 15:34
@mmlb
Copy link
Contributor Author

mmlb commented Apr 17, 2024

🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants