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

Parameterise ASG Termination Policies #1107

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mf-lit
Copy link

@mf-lit mf-lit commented Apr 21, 2023

This change adds a Parameter for setting the ASG Termination Policies, with defaults to keep it backward-compatible.

FWIW, our particular use case for this is so we can proritise the NewestInstance policy, meaning that we keep instances for longer in order to maintain a warm docker cache (our queue isn't busy enough to justify a dedicated queue for this).

@triarius
Copy link
Contributor

Hi @mf-lit. Thanks for making this PR. Unfortunately, we think it's not likely to work as expected.

So the way the instances in the elastic stack are terminated is not controlled by the ASG, because the ASG has NewInstancesProtectedFromScaleIn set to true. The instances terminate themselves when their agents are no longer running jobs (and ScaleInIdlePeriod has passed).

Furthermore, the configuration of buildkite-agent-scaler does not decrease the ASG's desired capacity. It's only the instances terminating themselves that decrease the desired capacity.

With that in mind, we don't think the termination policies will be used, as the scale in pathway does not involve the ASG evaluating them.

The reason for this admittedly convoluted situation is that we don't want instance to be terminated while they are running jobs. If that's not something you are concerned with, we can consider a mode of operation where the terminations policies will have an effect, but you will likely need to make significant changes to achieve that.

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.

2 participants