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.

Sign up for GitHub

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

Jump to bottom

Add new priority modes to --spawn-with-priority #1967

Closed
wants to merge 5 commits into from
Closed

Add new priority modes to --spawn-with-priority #1967

wants to merge 5 commits into from

Conversation

DrJosh9000
Copy link
Contributor

@DrJosh9000 DrJosh9000 commented Feb 23, 2023 •
edited

In #1929 it was pointed out that, when using --spawn-with-priority, the workload tends to be unevenly spread. Agents with fewer spawn will not have any spawn with as high a priority as agents with more spawn. Because work is assigned to available agents with the highest priority first, work ends up assigned only to agents with more spawn, and no work is assigned to the agents with fewer spawn (until the agents with more spawn become busy). This PR changes --spawn-with-priority to be a string flag, letting the user choose the priority assignment.

Example commands:

# Existing behaviour: Start 5 agents with default priority.
buildkite-agent start --build-path=~/tmp --spawn=5

# No longer valid (no value provided for --spawn-with-priority)
buildkite-agent start --build-path=~/tmp --spawn=5 --spawn-with-priority

# New behaviour: Start 5 agents with priorities [0, 1, 2, 3, 4].
buildkite-agent start --build-path=~/tmp --spawn=5 --spawn-with-priority=ascending

# New behaviour: Start 5 agents with priorities [0, -1, -2, -3, -4].
buildkite-agent start --build-path=~/tmp --spawn=5 --spawn-with-priority=descending

# New behaviour: Start 5 agents with priorities [42, 43, 44, 45, 46].
buildkite-agent start --build-path=~/tmp --spawn=5 --spawn-with-priority=ascending --priority=42

# New behaviour: Start 5 agents with priorities [42, 41, 40, 39, 38].
buildkite-agent start --build-path=~/tmp --spawn=5 --spawn-with-priority=descending --priority=42

Fixes #1929

Edit: changed from using an experiment to a single flag
Edit 2: changed from inverse to a variety of options.
Edit 3: fixed the example commands
Edit 4: changed away from the bool-like flag (fully breaking change)

@DrJosh9000 DrJosh9000 requested a review from a team February 23, 2023 01:56
triarius
triarius reviewed Feb 23, 2023
View reviewed changes
clicommand/agent_start.go Outdated Show resolved Hide resolved
@DrJosh9000 DrJosh9000 changed the title Add inverse-spawn-priority experiment Add an inverse mode to --spawn-with-priority Feb 23, 2023
@DrJosh9000 DrJosh9000 force-pushed the inverse-spawn-priority branch 4 times, most recently from f0e31b6 to 332ac7a Compare February 23, 2023 07:15
moskyb
moskyb reviewed Feb 23, 2023
View reviewed changes
clicommand/agent_start.go Outdated Show resolved Hide resolved
@DrJosh9000 DrJosh9000 changed the title Add an inverse mode to --spawn-with-priority Add new priority modes to --spawn-with-priority Feb 26, 2023
@sj26
Copy link
Member

sj26 commented Feb 27, 2023

Wonderful!

Just clarifying, when I started an agent on this branch like this:

go run main.go start --token ... --spawn 5 --spawn-with-priority

or

go run main.go start --token ... --spawn 5 --spawn-with-priority ascending

I get:

2023-02-27 17:27:08 INFO   Registering agent 1 of 5 with Buildkite...
2023-02-27 17:27:08 INFO   Assigning priority 0 for agent 1
2023-02-27 17:27:09 INFO   Successfully registered agent "...-1" with tags [queue=default]
2023-02-27 17:27:09 INFO   Registering agent 2 of 5 with Buildkite...
2023-02-27 17:27:09 INFO   Assigning priority 1 for agent 2
2023-02-27 17:27:09 INFO   Successfully registered agent "...-2" with tags [queue=default]
2023-02-27 17:27:09 INFO   Registering agent 3 of 5 with Buildkite...
2023-02-27 17:27:09 INFO   Assigning priority 2 for agent 3
2023-02-27 17:27:09 INFO   Successfully registered agent "...-3" with tags [queue=default]
2023-02-27 17:27:09 INFO   Registering agent 4 of 5 with Buildkite...
2023-02-27 17:27:09 INFO   Assigning priority 3 for agent 4
2023-02-27 17:27:10 INFO   Successfully registered agent "...-4" with tags [queue=default]
2023-02-27 17:27:10 INFO   Registering agent 5 of 5 with Buildkite...
2023-02-27 17:27:10 INFO   Assigning priority 4 for agent 5
2023-02-27 17:27:11 INFO   Successfully registered agent "...-5" with tags [queue=default]

Not what's in the PR message:

# Equivalent new behaviour: Start 5 agents with priorities [1, 2, 3, 4, 5].
buildkite-agent start --build-path=~/tmp --spawn=5 --spawn-with-priority=ascending

I think that's correct, just wanting to document the difference.

@sj26
Copy link
Member

sj26 commented Feb 27, 2023

This seems to work great, with the equals signs combining the flags with the arguments:

go run main.go start --spawn=5 --spawn-with-priority=descending

But this doesn't, where arguments are separated:

go run main.go start --spawn 5 --spawn-with-priority descending

It still spawns 5 agents, but with priorities ascending.

(That said, it doesn't complain about unknown arguments, so something's happening.)

Can this work as well?

@DrJosh9000
Copy link
Contributor Author

DrJosh9000 commented Feb 27, 2023 •
edited

There's a limitation to the flag parser. Flags are either IsBoolFlag, or not. In the IsBoolFlag case, the flag can be present without a value (it's like a bool), so while it understands both --flag (i.e. true) and --flag=value (in order to accept --flag=true, --flag=false), the parser won't parse the next space-separated arg as the associated value (if it did, should --flag --another be parsed as --flag=true --another, or as --flag='--another'?). In the non-IsBoolFlag case, the flag is supposed to always have a value, so it always parses the next arg as the value.

Perhaps it should just be a string flag?

@DrJosh9000 DrJosh9000 added this to the v3.45 milestone Feb 28, 2023
@DrJosh9000
Copy link
Contributor Author

I made it a string flag (value required).

@DrJosh9000 DrJosh9000 added the breaking Changes to existing behaviour users might rely on label Mar 2, 2023
@DrJosh9000 DrJosh9000 removed this from the v3.45 milestone Mar 9, 2023
@DrJosh9000 DrJosh9000 mentioned this pull request Mar 9, 2023
Merged
@nick-f nick-f mentioned this pull request Mar 19, 2023
Open
@DrJosh9000
Copy link
Contributor Author

I'm closing this with an eye to making descending-spawn-priority the default, as suggested by @dabarrell.

@DrJosh9000 DrJosh9000 closed this Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sj26 sj26 sj26 left review comments

@triarius triarius triarius left review comments

@dabarrell dabarrell dabarrell left review comments

@moskyb moskyb moskyb left review comments

Assignees
No one assigned
Labels
breaking Changes to existing behaviour users might rely on
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Distribute jobs more evenly across hosts
5 participants
@DrJosh9000 @sj26 @triarius @dabarrell @moskyb