Skip to content

Refine defaults and validation for worker with ManualInPlaceUpdate strategy #12607

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Aug 6, 2025
Merged

Refine defaults and validation for worker with ManualInPlaceUpdate strategy #12607

merged 10 commits into from
Aug 6, 2025

Conversation

acumino
Copy link
Member

@acumino acumino commented Jul 25, 2025 •
edited
Loading

How to categorize this PR?

/area control-plane
/kind cleanup

What this PR does / why we need it:
Updated validation to enforce that both MaxSurge and MaxUnavailable must not be set for ManualInPlaceUpdate, as MaxUnavailable is not relevant for workers using this strategy nodes selected by the user receive in-place updates simultaneously, regardless of MaxUnavailable. Similarly, MaxSurge is not applicable, as new node creation is not allowed.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
/cc @ary1992 @shafeeqes

Release note:

Setting shoot's `.spec.providers.workers[].{maxSurge, maxUnavailable}` will be denied in future versions of Gardener for workers with updateStrategy `ManualInPlaceUpdate`. Users should unset these values with this version of Gardener.

@gardener-prow gardener-prow bot requested review from ary1992 and shafeeqes July 25, 2025 08:14
@gardener-prow gardener-prow bot added area/control-plane Control plane related kind/cleanup Something that is not needed anymore and can be cleaned up size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. labels Jul 25, 2025
@acumino
Copy link
Member Author

acumino commented Jul 28, 2025

/retest

ary1992
ary1992 reviewed Jul 29, 2025
View reviewed changes
@acumino acumino force-pushed the man-inp-maxU branch 2 times, most recently from 13a8f8d to 79f7f8d Compare July 29, 2025 09:58
acumino
acumino commented Jul 29, 2025
View reviewed changes
@gardener-prow gardener-prow bot requested a review from hendrikKahl July 29, 2025 10:00
ary1992
ary1992 reviewed Jul 29, 2025
View reviewed changes
Copy link
Contributor

@ary1992 ary1992 left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-prow gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2025
@gardener-prow Gardener Prow
Copy link
Contributor

gardener-prow bot commented Jul 29, 2025

LGTM label has been added.

Git tree hash: 5fc838fd90559ce8be3af3903012f97914b13ade

@gardener-prow gardener-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 31, 2025
@gardener-prow gardener-prow bot requested a review from ary1992 July 31, 2025 08:08
@ary1992
Copy link
Contributor

ary1992 commented Jul 31, 2025

/lgtm

@gardener-prow gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 31, 2025
@gardener-prow Gardener Prow
Copy link
Contributor

gardener-prow bot commented Jul 31, 2025

LGTM label has been added.

Git tree hash: b969d19626695f2ee1274dca3577bfb1abf02e9e

shafeeqes
shafeeqes reviewed Aug 1, 2025
View reviewed changes
@gardener-prow gardener-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 1, 2025
shafeeqes
shafeeqes reviewed Aug 1, 2025
View reviewed changes
Copy link
Contributor

@shafeeqes shafeeqes left a comment

Choose a reason for hiding this comment

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

Thanks!
/lgtm

@gardener-prow gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2025
@gardener-prow Gardener Prow
Copy link
Contributor

gardener-prow bot commented Aug 1, 2025

LGTM label has been added.

Git tree hash: 085d8a6d1a985aef20c1d2bb8b124cced7ebf3b2

@gardener-prow gardener-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 4, 2025
@gardener-prow gardener-prow bot requested a review from shafeeqes August 4, 2025 12:40
shafeeqes
shafeeqes approved these changes Aug 6, 2025
View reviewed changes
Copy link
Contributor

@shafeeqes shafeeqes left a comment

Choose a reason for hiding this comment

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

Thanks!
/lgtm

@gardener-prow gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label Aug 6, 2025
@gardener-prow Gardener Prow
Copy link
Contributor

gardener-prow bot commented Aug 6, 2025

LGTM label has been added.

Git tree hash: 788f9f0e52b63523e0ca197a5fe5b78ff0bc0e28

@gardener-prow Gardener Prow
Copy link
Contributor

gardener-prow bot commented Aug 6, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shafeeqes

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:

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 Aug 6, 2025
@gardener-prow Gardener Prow
Copy link
Contributor

gardener-prow bot commented Aug 6, 2025 •
edited
Loading

@acumino: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-gardener-apidiff 5e77629 link false /test pull-gardener-apidiff

Full PR test history. Your PR dashboard. Command help for this repository.
Please help us cut down on flakes by linking this test failure to an open flake report or filing a new flake report if you can't find an existing one. Also see our testing guideline for how to avoid and hunt flakes.

Instructions 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. I understand the commands that are listed here.

@gardener-prow gardener-prow bot merged commit 94b6f57 into gardener:master Aug 6, 2025
18 of 19 checks passed
@acumino acumino deleted the man-inp-maxU branch August 6, 2025 12:31
Duciwuci pushed a commit to stackitcloud/gardener that referenced this pull request Sep 1, 2025
@acumino @Duciwuci
Refine defaults and validation for worker with ManualInPlaceUpdate st…
8eb633d 
…rategy (gardener#12607)

* Refine defaults and validation for worker with ManualInPlaceUpdate strategy

* Address Review

* Rename flag for more clarity

* Allow setting machine image version during shoot creation

* MaxSurge and MaxUnavailable can be nil

* Don't default maxSurge and maxUnavailable for ManualInPlaceUpdate workers

* Add validation that MaxSurge and MaxUnavilable should not be set for ManualInPlaceUpdate

* Fix e2e test for in-place updates

* If only patch version is updated we don't need to check for inplace constranits

* Address Review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shafeeqes shafeeqes shafeeqes approved these changes

@hendrikKahl hendrikKahl Awaiting requested review from hendrikKahl

@ary1992 ary1992 Awaiting requested review from ary1992

Assignees

@ary1992 ary1992

@shafeeqes shafeeqes

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/control-plane Control plane related cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. kind/cleanup Something that is not needed anymore and can be cleaned up 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
@acumino @ary1992 @shafeeqes