Skip to content

Add integration tests for machine preservation - #1124

Open
aaronfern wants to merge 25 commits into
gardener:masterfrom
aaronfern:int_tests
Open

Add integration tests for machine preservation#1124
aaronfern wants to merge 25 commits into
gardener:masterfrom
aaronfern:int_tests

Conversation

@aaronfern

@aaronfern aaronfern commented Jul 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

What this PR does / why we need it:
This PR adds integration tests for machine preservation.

The following cases have been covered:

Auto Machine Preservation:

  • Ensure failed machine is preserved and rejoins the cluster on recovery
  • Failed machines that cross the max threshold are not preserved
  • machinePreserveTimeout is honoured for failing machines
  • Machine should not be preserved when the preserve=false annotation is present
  • Preserved machine should stop being preserved when preserve=false annotation is added
  • When AutoPreserveFailedMachineMax is reduced, failed machines that exceed the new AutoPreserveFailedMachineMax count are moved to terminating

Manual Machine Preservation:

  • Machine in an mcd without any preservation fields should be preserved when failed if it has the when-failed annotation and move back to Running phase when it recovers
  • Machine should be preserved when it's corresponding node is annotated with when-failed
  • A machine marked for preservation using the when-failed annotation should be preserved when failed even if autoPreserveMax is crossed
  • Preserved machine should terminate if the preservation annotation is removed

Which issue(s) this PR fixes:
Fixes partially #1123

Special notes for your reviewer:
To trigger machine failures, this PR takes the approach of applying a validating admission policy to block all kubelet updates. This causes kubelet not to be able to update it's node lease or it's node status and results in the node going into the Unknown state.
This is a k8s native way of achieving node failures and has no dependency on the provider or any other node/worker-pool setting

This might not work with the virtual provider of the virtual provider does not use kubelets, but imo this can be considered a special case and can be tackled in a separate PR

Release note:

Added integration tests for machine preservation

@aaronfern
aaronfern requested a review from a team as a code owner July 27, 2026 07:05
@gardener-prow gardener-prow Bot added do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 27, 2026
@aaronfern aaronfern mentioned this pull request Jul 27, 2026
Open
6 tasks
@aaronfern aaronfern added the kind/test Test label Jul 27, 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 Jul 27, 2026
@thiyyakat

thiyyakat commented Jul 27, 2026

Copy link
Copy Markdown
Member

/assign

gagan16k
gagan16k reviewed Aug 4, 2026
View reviewed changes

@gagan16k gagan16k 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 PR!

Comment thread pkg/test/integration/common/helpers/admission_policy.go
Comment thread pkg/test/integration/common/helpers/admission_policy.go
Comment thread pkg/test/integration/common/helpers/admission_policy.go Outdated
Comment thread go.mod Outdated
Comment thread pkg/test/integration/common/helpers/machine_resources.go
Comment thread pkg/test/integration/common/framework.go Outdated
Comment thread pkg/test/integration/common/framework.go Outdated
Comment thread pkg/test/integration/common/framework.go Outdated
Comment thread pkg/test/integration/common/framework.go Outdated
Comment thread pkg/test/integration/common/framework.go Outdated
thiyyakat
thiyyakat reviewed Aug 5, 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 adding these much-needed tests! 👍

Comment thread pkg/test/integration/common/helpers/admission_policy.go Outdated
Comment thread pkg/test/integration/common/helpers/admission_policy.go Outdated
Comment thread pkg/test/integration/common/helpers/admission_policy.go Outdated
Comment thread pkg/test/integration/common/helpers/machine_resources.go Outdated
Comment thread pkg/test/integration/common/helpers/machine_resources.go Outdated
Comment thread pkg/test/integration/common/framework.go Outdated
Comment thread pkg/test/integration/common/framework.go Outdated
Comment thread pkg/test/integration/common/framework.go Outdated
Comment thread pkg/test/integration/common/framework.go Outdated
Comment thread pkg/test/integration/common/framework.go
@thiyyakat

thiyyakat commented Aug 5, 2026

Copy link
Copy Markdown
Member

@aaronfern , should we have some IT to test reduction of AutoPreserveFailedMachineMax? If the number of preserved failed machines is equal to AutoPreserveFailedMachineMax already, and the value is reduced by 1, a machine should move to Terminating.

@gardener-prow

gardener-prow Bot commented Aug 5, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from thiyyakat. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

@aaronfern

aaronfern commented Aug 5, 2026

Copy link
Copy Markdown
Member Author

@aaronfern , should we have some IT to test reduction of AutoPreserveFailedMachineMax? If the number of preserved failed machines is equal to AutoPreserveFailedMachineMax already, and the value is reduced by 1, a machine should move to Terminating.

@thiyyakat Okay, sure, makes sense. I will add this test

thiyyakat
thiyyakat reviewed Aug 6, 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 adding the test and addressing the comments. Just a few nits.

Comment thread pkg/test/integration/common/helpers/machine_resources.go Outdated
Comment thread pkg/test/integration/common/helpers/machine_resources.go Outdated
Comment thread pkg/test/integration/common/helpers/machine_resources.go Outdated
Comment thread pkg/test/integration/common/framework.go Outdated
Comment thread pkg/test/integration/common/framework.go Outdated
Comment thread pkg/test/integration/common/framework.go Outdated
thiyyakat
thiyyakat reviewed Aug 6, 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.

There are a few if-else if blocks that can be converted to if-else blocks. PTAL.

Comment thread pkg/test/integration/common/helpers/admission_policy.go Outdated
Comment thread pkg/test/integration/common/framework.go Outdated
Comment thread pkg/test/integration/common/framework.go Outdated
Comment thread pkg/test/integration/common/framework.go Outdated
Comment thread pkg/test/integration/common/framework.go Outdated
@aaronfern
aaronfern force-pushed the int_tests branch 2 times, most recently from bc0917f to 573b83f Compare August 6, 2026 09:21
gagan16k
gagan16k reviewed Aug 6, 2026
View reviewed changes

@gagan16k gagan16k 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.

Some more comments, PTAL

Comment thread pkg/test/integration/common/framework.go Outdated
Comment thread pkg/test/integration/common/framework.go Outdated
Comment thread pkg/test/integration/common/framework.go
Comment thread pkg/test/integration/common/helpers/machine_resources.go Outdated
Comment thread pkg/test/integration/common/framework.go Outdated
Comment thread pkg/test/integration/common/helpers/machine_resources.go Outdated
thiyyakat
thiyyakat reviewed Aug 10, 2026
View reviewed changes
Comment thread pkg/test/integration/common/framework.go Outdated
gagan16k

This comment was marked as resolved.

Sign in to view

r4mek
r4mek reviewed Aug 18, 2026
View reviewed changes

@r4mek r4mek 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 PR!

Comment thread pkg/test/integration/common/helpers/machine_resources.go Outdated
Comment thread pkg/test/integration/common/helpers/machine_resources.go Outdated
r4mek
r4mek reviewed Aug 18, 2026
View reviewed changes
Comment thread pkg/test/integration/common/helpers/admission_policy.go Outdated
r4mek
r4mek reviewed Aug 18, 2026
View reviewed changes
Comment thread pkg/test/integration/common/helpers/admission_policy.go
@gardener-prow gardener-prow Bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 18, 2026
@aaronfern aaronfern changed the title Add integration tests for auto machine preservation Add integration tests for machine preservation Aug 18, 2026
@aaronfern aaronfern mentioned this pull request Aug 18, 2026
Closed
4 tasks
aaronfern added 19 commits August 31, 2026 15:45
@aaronfern
Add IT to ensure number of preserved machines reduce if AutoPreserveF…
0a9b6df 
…ailedMachineMax is reduced

Signed-off-by: aaronfern <aaron.francis.fernandes@sap.com>
@aaronfern
Address review comments by Meghana - 2
0123ad3 
Signed-off-by: aaronfern <aaron.francis.fernandes@sap.com>
@aaronfern
Add removal of vap/vapb to cleanup and remove vap/vapb via defers in …
84e8191 
…tests

Signed-off-by: aaronfern <aaron.francis.fernandes@sap.com>
@aaronfern
Address review comments by Gagan - 2
1a2215d 
Signed-off-by: aaronfern <aaron.francis.fernandes@sap.com>
@aaronfern
Reorder IT to have the orphan resource tests at the end
67c6758 
Signed-off-by: aaronfern <aaron.francis.fernandes@sap.com>
@aaronfern
Run make check
82f44ac 
Signed-off-by: aaronfern <aaron.francis.fernandes@sap.com>
@aaronfern
Add DeferCleanup before vap/vapb resources are created
c6bd838 
Signed-off-by: aaronfern <aaron.francis.fernandes@sap.com>
@aaronfern
Add manual preservation tests
3cf25f7 
Signed-off-by: aaronfern <aaron.francis.fernandes@sap.com>
@aaronfern
Address review comments from r4mek
bf2c1b8 
Signed-off-by: aaronfern <aaron.francis.fernandes@sap.com>
@aaronfern
Remove commented out function
ea5c6e3 
Signed-off-by: aaronfern <aaron.francis.fernandes@sap.com>
@aaronfern
Run make check
0568197 
Signed-off-by: aaronfern <aaron.francis.fernandes@sap.com>
@aaronfern
Address review comments
ae11763 
Signed-off-by: aaronfern <aaron.francis.fernandes@sap.com>
@aaronfern
Run make check
2323b33 
Signed-off-by: aaronfern <aaron.francis.fernandes@sap.com>
@aaronfern
Address review comments
343d4b0 
Signed-off-by: aaronfern <aaron.francis.fernandes@sap.com>
@aaronfern
Use node label key constant instead of hardcoded string
a4c4c85 
Signed-off-by: aaronfern <aaron.francis.fernandes@sap.com>
@aaronfern
Adding optimizations to speed up tests
a56035b 
Signed-off-by: aaronfern <aaron.francis.fernandes@sap.com>
@aaronfern
Requeue machine when phase updated to Failed
952f322 
Signed-off-by: aaronfern <aaron.francis.fernandes@sap.com>
@aaronfern
Update test descriptions
d77beb3 
Signed-off-by: aaronfern <aaron.francis.fernandes@sap.com>
@aaronfern
Address review comments
08fdaed 
Signed-off-by: aaronfern <aaron.francis.fernandes@sap.com>
@gagan16k

gagan16k commented Sep 1, 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 1, 2026
@gardener-prow

gardener-prow Bot commented Sep 1, 2026

Copy link
Copy Markdown

LGTM label has been added.

DetailsGit tree hash: 9738282a5d4a8fa09e00c07d376b4d8fa15ea996

thiyyakat
thiyyakat reviewed Sep 1, 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.

Just a few minor nits. Looks good to me otherwise. 👍

Comment thread pkg/test/integration/common/framework.go Outdated
Comment thread pkg/test/integration/common/framework.go Outdated
Comment thread pkg/test/integration/common/framework.go Outdated
Comment thread pkg/test/integration/common/framework.go Outdated
Comment thread pkg/test/integration/common/framework.go Outdated
@aaronfern
Address review comments
5eaf897 
Signed-off-by: aaronfern <aaron.francis.fernandes@sap.com>
@gardener-prow gardener-prow Bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 1, 2026
@gardener-prow
gardener-prow Bot requested a review from thiyyakat September 1, 2026 10:52
@thiyyakat

thiyyakat commented Sep 1, 2026

Copy link
Copy Markdown
Member

Thanks for the changes and for addressing multiple rounds of review comments!

/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: f27c3ea5e1015be55bacf9df2942f6de4928ce95

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
@r4mek r4mek r4mek left review comments
@thiyyakat thiyyakat Awaiting requested review from thiyyakat

Assignees

@thiyyakat thiyyakat

Labels

cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. kind/test Test lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@aaronfern @thiyyakat @gagan16k @r4mek