Skip to content

Fix: Align auto-preservation eviction order with scale-down deletion priority - #1138

Merged
gardener-prow[bot] merged 4 commits into
gardener:masterfrom
aaronfern:preserve_mc_sort
Sep 21, 2026
Merged

Fix: Align auto-preservation eviction order with scale-down deletion priority#1138
gardener-prow[bot] merged 4 commits into
gardener:masterfrom
aaronfern:preserve_mc_sort

Conversation

@aaronfern

@aaronfern aaronfern commented Aug 25, 2026

Copy link
Copy Markdown
Member

What this PR does / why we need it:
We noticed an inconsistency between how auto-preserved failed machines are chosen for termination due to mcs's spec.replica reduction and how auto-preserve failed machines are chosen for termination due to mcs's spec.autoPreserveFailedMachineMax reduction.

Currently when spec.autoPreserveFailedMachineMax is reduced, auto-preserved failed machines that have a nearer PreserveExpiryTime are chosen for deletion, whereas when spec.replica is reduced auto-preserved failed machines that have an earlier CreationTimestamp are chosen for deletion.

This PR aligns both these orderings to ensure that now auto-preserved failed machines are only ordered based on their PreserveExpiryTimes

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

Ensure auto-preserve failed machines are ordered by their `PreserveExpiryTime` in case of deletion due to reduction is mcs.spec.Replica or mcs.spec.autoPreserveFailedMachineMax

@gardener-prow gardener-prow Bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 25, 2026
@aaronfern
aaronfern marked this pull request as ready for review August 26, 2026 04:49
@aaronfern
aaronfern requested a review from a team as a code owner August 26, 2026 04:49
@gardener-prow gardener-prow Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 26, 2026
@aaronfern aaronfern added the kind/bug Bug label Aug 26, 2026
@gardener-prow gardener-prow Bot removed the do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Aug 26, 2026
thiyyakat
thiyyakat reviewed Aug 31, 2026
View reviewed changes

@thiyyakat thiyyakat left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the quick fix, @aaronfern, and for updating the FAQ! Just a few nits from me.

Also, if it's not too much trouble, could you please add a similar test for the sorting performed in the case of mcd replica reduction as well?

Comment thread docs/faq.md Outdated
Comment thread docs/faq.md Outdated
Comment thread docs/faq.md
@gardener-prow gardener-prow Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 31, 2026
@aaronfern

aaronfern commented Aug 31, 2026

Copy link
Copy Markdown
Member Author

Thanks for reviewing @thiyyakat
I've addressed your comments. PTAL

thiyyakat
thiyyakat reviewed Sep 1, 2026
View reviewed changes
Comment thread pkg/controller/machineset_test.go Outdated
@thiyyakat

thiyyakat commented Sep 1, 2026

Copy link
Copy Markdown
Member

Thanks! The changes look good to me. Just wanted to add a comment here for completeness:

When autoPreserveFailedMachineMax is reduced, we stop auto-preservation based solely on PreserveExpiryTime (removing the machine whose preservation expires soonest first), unlike the more general ActiveMachine sort. There's no value in considering other criteria such as MachinePriority here because when PreserveExpiryTime values differ by only milliseconds, and we choose the machine with the earlier PreserveExpiryTime the choice is effectively arbitrary.
More criteria would imply a meaningful preference when PreserveExpiryTime values are equal, where none really exists. 😅

/lgtm

@gardener-prow gardener-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Sep 1, 2026
@gardener-prow

gardener-prow Bot commented Sep 1, 2026

Copy link
Copy Markdown

LGTM label has been added.

DetailsGit tree hash: e812c9df4139341aebda490726c9493e8529b893

gagan16k
gagan16k reviewed Sep 2, 2026
View reviewed changes
Comment thread pkg/controller/machineset.go Outdated
Comment thread pkg/controller/machineset_test.go Outdated
Comment thread pkg/controller/machineset_test.go Outdated
@gardener-prow gardener-prow Bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 17, 2026
@aaronfern
Fix ordering inconsistency in stopAutoPreservationForMachines to use …
16522d5 
…PreserveExpiryTime instead of CreationTimestamp

Signed-off-by: aaronfern <aaron.francis.fernandes@sap.com>
@aaronfern
Address review comments
96ddcde 
Signed-off-by: aaronfern <aaron.francis.fernandes@sap.com>
@aaronfern
Address review comments - 2
73db83a 
Signed-off-by: aaronfern <aaron.francis.fernandes@sap.com>
@aaronfern
Address review feedback
a62e1c9 
Signed-off-by: aaronfern <aaron.francis.fernandes@sap.com>
gagan16k
gagan16k reviewed Sep 21, 2026
View reviewed changes
Comment thread pkg/controller/machineset_test.go
@gagan16k

gagan16k commented Sep 21, 2026

Copy link
Copy Markdown
Member

/lgtm

@gardener-prow gardener-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Sep 21, 2026
@gardener-prow

gardener-prow Bot commented Sep 21, 2026

Copy link
Copy Markdown

LGTM label has been added.

DetailsGit tree hash: 932259ec620c29380ce68120f35073c4c7b88f95

@thiyyakat

thiyyakat commented Sep 21, 2026

Copy link
Copy Markdown
Member

/lgtm

@gardener-prow

gardener-prow Bot commented Sep 21, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thiyyakat

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@gardener-prow gardener-prow Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 21, 2026
@gardener-prow
gardener-prow Bot merged commit 1a7592c into gardener:master Sep 21, 2026
41 of 45 checks passed
This was referenced Sep 23, 2026
Merged
Merged
Merged
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gagan16k gagan16k gagan16k left review comments
@thiyyakat thiyyakat thiyyakat approved these changes

Assignees

No one assigned

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Bug lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@aaronfern @thiyyakat @gagan16k