Report terminating instances as gone in AWS HasInstance - #10149
Report terminating instances as gone in AWS HasInstance#10149ribhavjain wants to merge 1 commit into
Conversation
After NodeGroup.DeleteNodes(), terminated instances keep being listed by the ASG API in the Terminating lifecycle state until their termination completes, so HasInstance() kept returning true long after the instances were excluded from the target size. Per the quick-fix instructions in issue kubernetes#9877, this makes ClusterStateRegistry undercount upcoming nodes and trigger unnecessary extra scale-ups. Mark instances as Terminating in the lifecycle cache as soon as they are terminated in DeleteInstances(), report instances in any terminating lifecycle state as gone from HasInstance(), and don't let stale API responses overwrite the cached terminating state on regeneration.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ribhavjain The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
This issue is currently awaiting triage. If SIG Autoscaling contributors determines this is a relevant issue, they will accept it by applying the The DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Hi @ribhavjain. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe AWS provider now tracks terminating instances in the ASG cache. ChangesAWS terminating instance handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The AWS provider now treats terminating instances as gone sooner, preventing unnecessary scale-ups; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/assign @gjtempleton |
|
@gjtempleton gentle ping — anything you need from me to unlock CI here? This implements the AWS quick-fix from #9877 (same for the small kubemark build fix in #10148). |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Implements the AWS provider part of the
HasInstance()quick-fix instructions from #9877.Today the AWS
HasInstance()returns true for any instance present in the ASG cache, regardless of its state. AfterNodeGroup.DeleteNodes():HasInstance()keeps returning true until the next cache regeneration, andTerminatinglifecycle state until its termination completes, so every cache regeneration re-adds it andHasInstance()keeps returning true that whole time.Per #9877, this makes ClusterStateRegistry keep categorizing scaled-down Nodes as Ready/Unready after they are already excluded from the target size, which undercounts upcoming nodes and triggers the same unnecessary extra scale-ups as #9813.
This change:
DeleteInstances()now marks the instance asTerminatingin the lifecycle cache immediately after the successfulTerminateInstanceInAutoScalingGroupcall, soHasInstance()starts responding false right afterDeleteNodes()returns, before any cache regeneration.HasInstance()now returns false for instances whose cached lifecycle state isTerminating,Terminating:Wait,Terminating:ProceedorTerminated(the same setDeleteInstances()already treats as "already terminating"), so the false response survives cache regenerations for as long as the ASG API still lists the instance.Behavior for non-autoscaled nodes (not present in the cache) and for autoscaled nodes still included in the target size is unchanged.
Which issue(s) this PR fixes:
Part of #9877 (AWS provider quick-fix; not closing the umbrella issue)
Special notes for your reviewer:
Per the contributor guide's AI guidance: AI tooling was used in preparing this change. I have reviewed it and can explain every line.
Does this PR introduce a user-facing change?
Summary by CodeRabbit
Bug Fixes
Tests