Track lifecycle and skip terminating already terminating instances #5411

alam0rt · 2023-01-11T05:52:18Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

We encountered an issue where the curSize cached value would be decremented every time an already terminating instance was signaled for termination again. This caused the desired size of the ASG to dwindle to min size and was made worse when we introduced a change that kept instances in a terminating state for longer than we previously did.

The cause of this it seems is because cluster-autoscaler doesn't take any heed of already terminating instances and blindly decrements the curSize value,

This PR addresses that by caching the lifecycle state of the instance.

Which issue(s) this PR fixes:

Fixes #4095

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2023-01-11T05:52:26Z

Welcome @alam0rt!

It looks like this is your first PR to kubernetes/autoscaler 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/autoscaler has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

grosser

🎉

cluster-autoscaler/cloudprovider/aws/auto_scaling_groups.go

x13n · 2023-02-28T16:21:00Z

Looks like this was already lgtmed by @grosser . Assigning aws owners for approval.

/assign @jaypipes @gjtempleton

gjtempleton

One query.

cluster-autoscaler/cloudprovider/aws/auto_scaling_groups.go

gjtempleton · 2023-05-09T06:57:24Z

Can you squash this down to one or two commits please, and then happy to get this merged.

Avoid deadlocking by locking mutex twice Instantiate test instance with lifecycle state Make method private

grosser · 2023-05-11T06:02:30Z

@gjtempleton update was pushed

gjtempleton · 2023-05-18T13:22:10Z

Thanks!
/lgtm
/approve

k8s-ci-robot · 2023-05-18T13:22:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alam0rt, gjtempleton, grosser

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~cluster-autoscaler/cloudprovider/aws/OWNERS~~ [gjtempleton]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

… instances (kubernetes#5411) Merge pull request kubernetes#5411 from alam0rt/fix-issue-where-terminating-instances-scale-down-asg-to-min Track lifecycle and skip terminating already terminating instances

…nating-instances-scale-down-asg-to-min Track lifecycle and skip terminating already terminating instances

… instances (kubernetes#5411) Merge pull request kubernetes#5411 from alam0rt/fix-issue-where-terminating-instances-scale-down-asg-to-min Track lifecycle and skip terminating already terminating instances

Patch comes from upstream: 43c511d 21e1f5b kubernetes#5411 This has been modified to apply cleanly to 1.22.3

…nating-instances-scale-down-asg-to-min Track lifecycle and skip terminating already terminating instances

Brings in: kubernetes#5411 kubernetes#6166 But the code is so different I manually brought it up to speed.

bring up AWS patch for termination Brings in: kubernetes#5411 kubernetes#6166 But the code is so different I manually brought it up to speed. Also, I added a patch to make tests pass in the latest version of Go.

…nating-instances-scale-down-asg-to-min Track lifecycle and skip terminating already terminating instances

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 11, 2023

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 11, 2023

k8s-ci-robot requested a review from gjtempleton January 11, 2023 05:52

k8s-ci-robot added the area/cluster-autoscaler label Jan 11, 2023

k8s-ci-robot requested a review from jaypipes January 11, 2023 05:52

alam0rt marked this pull request as ready for review January 11, 2023 05:57

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 11, 2023

alam0rt marked this pull request as draft January 11, 2023 06:02

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 11, 2023

grosser approved these changes Jan 11, 2023

View reviewed changes

cluster-autoscaler/cloudprovider/aws/auto_scaling_groups.go Outdated Show resolved Hide resolved

alam0rt marked this pull request as ready for review January 11, 2023 21:50

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 11, 2023

alam0rt mentioned this pull request Jan 12, 2023

Track lifecycle and skip terminating already terminating instances grosser/autoscaler#4

Open

grosser mentioned this pull request Jan 13, 2023

cluster-autoscaler: terminating an instance does not mean there will be less desired instances #5420

Closed

alam0rt mentioned this pull request Jan 18, 2023

[cluster-autoscaler][AWS] Repeated deletion every 10s of same node takes AWS ASG down to min size killing multiple running pods #4095

Closed

k8s-ci-robot assigned gjtempleton and jaypipes Feb 28, 2023

gjtempleton reviewed Mar 6, 2023

View reviewed changes

cluster-autoscaler/cloudprovider/aws/auto_scaling_groups.go Show resolved Hide resolved

alam0rt added 2 commits May 11, 2023 15:37

Track lifecycle and skip terminating already terminating instances

43c511d

Avoid deadlocking by locking mutex twice Instantiate test instance with lifecycle state Make method private

Add test to assert terminating instances aren't terminated again

21e1f5b

alam0rt force-pushed the fix-issue-where-terminating-instances-scale-down-asg-to-min branch from 7cdba40 to 21e1f5b Compare May 11, 2023 05:39

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 18, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 18, 2023

k8s-ci-robot merged commit 47af9cc into kubernetes:master May 18, 2023

abhinavdahiya mentioned this pull request Sep 28, 2023

Cherry Pick: Track lifecycle and skip terminating already terminating instances lyft/autoscaler#2

Merged

abhinavdahiya pushed a commit to lyft/autoscaler that referenced this pull request Sep 28, 2023

Merge pull request kubernetes#5411 from alam0rt/fix-issue-where-termi…

2295fe7

…nating-instances-scale-down-asg-to-min Track lifecycle and skip terminating already terminating instances

abhinavdahiya mentioned this pull request Sep 28, 2023

Merge pull request #5411 from alam0rt/fix-issue-where-terminating-instances-scale-down-asg-to-min lyft/autoscaler#3

Merged

tomwans added a commit to lyft/autoscaler that referenced this pull request Sep 29, 2023

Track lifecycle and skip terminating already terminating instances

1987a28

Patch comes from upstream: 43c511d 21e1f5b kubernetes#5411 This has been modified to apply cleanly to 1.22.3

abhinavdahiya pushed a commit to lyft/autoscaler that referenced this pull request Oct 2, 2023

Merge pull request kubernetes#5411 from alam0rt/fix-issue-where-termi…

84d2bdd

…nating-instances-scale-down-asg-to-min Track lifecycle and skip terminating already terminating instances

tomwans mentioned this pull request Oct 3, 2023

aws: check for all possible Terminated states #6166

Merged

tomwans added a commit to lyft/autoscaler that referenced this pull request Oct 9, 2023

bring up AWS patch for termination

e3df53c

Brings in: kubernetes#5411 kubernetes#6166 But the code is so different I manually brought it up to speed.

tomwans mentioned this pull request Oct 9, 2023

backporting: AWS Termination handling fix for 1.16 release lyft/autoscaler#6

Merged

murongl-lyft pushed a commit to lyft/autoscaler that referenced this pull request Dec 4, 2023

Merge pull request kubernetes#5411 from alam0rt/fix-issue-where-termi…

5e5fd71

…nating-instances-scale-down-asg-to-min Track lifecycle and skip terminating already terminating instances

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Track lifecycle and skip terminating already terminating instances #5411

Track lifecycle and skip terminating already terminating instances #5411

Uh oh!

alam0rt commented Jan 11, 2023 •

edited

Loading

Uh oh!

k8s-ci-robot commented Jan 11, 2023

Uh oh!

grosser left a comment

Uh oh!

Uh oh!

x13n commented Feb 28, 2023

Uh oh!

gjtempleton left a comment

Uh oh!

Uh oh!

gjtempleton commented May 9, 2023

Uh oh!

grosser commented May 11, 2023

Uh oh!

gjtempleton commented May 18, 2023

Uh oh!

k8s-ci-robot commented May 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Track lifecycle and skip terminating already terminating instances #5411

Track lifecycle and skip terminating already terminating instances #5411

Uh oh!

Conversation

alam0rt commented Jan 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

k8s-ci-robot commented Jan 11, 2023

Uh oh!

grosser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

x13n commented Feb 28, 2023

Uh oh!

gjtempleton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gjtempleton commented May 9, 2023

Uh oh!

grosser commented May 11, 2023

Uh oh!

gjtempleton commented May 18, 2023

Uh oh!

k8s-ci-robot commented May 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

alam0rt commented Jan 11, 2023 •

edited

Loading