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

feat: maintain idle pool of runners #1038

Merged
merged 4 commits into from
Mar 7, 2024
Merged

feat: maintain idle pool of runners #1038

merged 4 commits into from
Mar 7, 2024

Conversation

matt2e
Copy link
Collaborator

@matt2e matt2e commented Mar 7, 2024

Adds --idle-runners arg to define how large the idle pool should be.

Fixes #1036
Fixes #1030

Previous notes

Currently a draft because this PR makes #1036 more likely to be hit.
Before this change, killing all deployments would mean there are 0 runners, leading to no runner id collisions when you bring up more deployments
After this change, killing all deployments means that there will still be runners which will cause collisions if the idle runner ids aren't the lowest possible [R00000000000000000000002000, R00000000000000000000004000 ... ]

I've been testing with a hacky fix replacing line bankend/controller/scaling/local_scaling.go:96 to:

binary.BigEndian.PutUint32(ulid[10:], rand.Uint32())

@alecthomas alecthomas mentioned this pull request Mar 7, 2024
@matt2e
Copy link
Collaborator Author

matt2e commented Mar 7, 2024

Is 1 a good default idle pool size?

@alecthomas
Copy link
Collaborator

I think it would be best fix #1036 in this PR.

@@ -825,7 +826,7 @@ func (s *Service) reconcileRunners(ctx context.Context) (time.Duration, error) {
return 0, fmt.Errorf("%s: %w", "failed to get deployments needing reconciliation", err)
}

totalRunners := 0
totalRunners := s.config.IdleRunners
Copy link
Collaborator

Choose a reason for hiding this comment

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

Haha wow that was easy 😂

@matt2e
Copy link
Collaborator Author

matt2e commented Mar 7, 2024

I think it would be best fix #1036 in this PR.

Will give it a go

@alecthomas
Copy link
Collaborator

It might be best just randomly generate them like it does in production.

@matt2e
Copy link
Collaborator Author

matt2e commented Mar 7, 2024

Ah, just did a an ever increasing version. Happy to switch it over to what we do in prod though if you prefer. Have the simpler ids been helpful in development up to now? Otherwise keeping it closer to prod seems like a better choice

@matt2e matt2e marked this pull request as ready for review March 7, 2024 04:36
@alecthomas
Copy link
Collaborator

Ah, just did a an ever increasing version. Happy to switch it over to what we do in prod though if you prefer. Have the simpler ids been helpful in development up to now? Otherwise keeping it closer to prod seems like a better choice

I think the idea was that it would make it slightly easier to debug, but I don't recall in practice ever relying on it or even caring.

@matt2e matt2e merged commit 2b8f713 into main Mar 7, 2024
11 checks passed
@matt2e matt2e deleted the matt2e/idle-runner branch March 7, 2024 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants