Skip to content

[GEP-31] Adapt Worker reconciler for in-place update #11713

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
Apr 17, 2025
Merged

[GEP-31] Adapt Worker reconciler for in-place update #11713

merged 10 commits into from
Apr 17, 2025

Conversation

shafeeqes
Copy link
Contributor

@shafeeqes shafeeqes commented Mar 20, 2025 •
edited
Loading

How to categorize this PR?

/area control-plane delivery open-source scalability
/kind enhancement

What this PR does / why we need it:

  • Adapt the worker controller to add an additional hash (omitted in the node-agent-secret name) to the hash calculation for the machine class.
  • Introduce new field in the Worker status API to keep track of the worker pool hash
    • Handle the health check in worker to pass even if there are pending manual in-place updates

Which issue(s) this PR fixes:
Part of #10219

Special notes for your reviewer:

Release note:

NONE

@gardener-prow gardener-prow bot added the area/control-plane Control plane related label Mar 20, 2025
@gardener-prow gardener-prow bot requested review from plkokanov and timebertt March 20, 2025 08:34
@gardener-prow gardener-prow bot added area/delivery Delivery related area/open-source Open Source (community, enablement, contributions, conferences, CNCF, etc.) related area/scalability Scalability related kind/enhancement Enhancement, improvement, extension cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 20, 2025
@shafeeqes shafeeqes mentioned this pull request Mar 17, 2025
Open
40 tasks
@shafeeqes
Copy link
Contributor Author

/hold
until MCM is released with the required constants and the replace directive is dropped.

@gardener-prow gardener-prow bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 20, 2025
ary1992
ary1992 reviewed Mar 24, 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.

Thanks for the PR!
Made it till half way.

ary1992
ary1992 reviewed Mar 25, 2025
View reviewed changes
ary1992
ary1992 reviewed Mar 25, 2025
View reviewed changes
acumino
acumino requested changes Mar 25, 2025
View reviewed changes
@shafeeqes shafeeqes force-pushed the inplace-updates-worker branch from 368faf5 to 0d9efa4 Compare March 25, 2025 10:35
@gardener-prow gardener-prow bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 25, 2025
acumino
acumino reviewed Mar 28, 2025
View reviewed changes
@plkokanov
Copy link
Contributor

/assign

@gardener-prow gardener-prow bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 31, 2025
@shafeeqes shafeeqes force-pushed the inplace-updates-worker branch from cdd8249 to 236bc1c Compare April 3, 2025 10:15
@gardener-prow gardener-prow bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 3, 2025
plkokanov
plkokanov requested changes Apr 4, 2025
View reviewed changes
Copy link
Contributor

@plkokanov plkokanov left a comment

Choose a reason for hiding this comment

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

I'm half-way through the commits. So far it's a very nicely split PR and a pleasure to review, thanks!

@shafeeqes shafeeqes force-pushed the inplace-updates-worker branch 2 times, most recently from 45ae14f to 30a0287 Compare April 6, 2025 13:35
ary1992
ary1992 reviewed Apr 8, 2025
View reviewed changes
@gardener-prow gardener-prow bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 16, 2025
shafeeqes and others added 10 commits April 16, 2025 14:49
@shafeeqes @ary1992 @acumino
Add helper functions
1a5013d 
Co-Authored-By: Ashish Ranjan Yadav <ashish.ranjan.yadav@sap.com>
Co-Authored-By: Sonu Kumar Singh <sonu.kumar.singh02@sap.com>
@shafeeqes @ary1992 @acumino
Adapt worker controller to add worker pool hash for inplace update
83127c9 
Co-Authored-By: Ashish Ranjan Yadav <ashish.ranjan.yadav@sap.com>
Co-Authored-By: Sonu Kumar Singh <sonu.kumar.singh02@sap.com>
@shafeeqes @ary1992 @acumino
Adapt Worker API
59ae59d 
Co-Authored-By: Ashish Ranjan Yadav <ashish.ranjan.yadav@sap.com>
Co-Authored-By: Sonu Kumar Singh <sonu.kumar.singh02@sap.com>
@shafeeqes @ary1992 @acumino
Adapt worker controller to set worker status with in-place update hash
dd6cdae 
Co-Authored-By: Ashish Ranjan Yadav <ashish.ranjan.yadav@sap.com>
Co-Authored-By: Sonu Kumar Singh <sonu.kumar.singh02@sap.com>
@shafeeqes @ary1992 @acumino
Handle health check in worker for manual in-place update
ed3c892 
Co-Authored-By: Ashish Ranjan Yadav <ashish.ranjan.yadav@sap.com>
Co-Authored-By: Sonu Kumar Singh <sonu.kumar.singh02@sap.com>
@shafeeqes @ary1992 @acumino
Run make generate
3663fad 
Co-Authored-By: Ashish Ranjan Yadav <ashish.ranjan.yadav@sap.com>
Co-Authored-By: Sonu Kumar Singh <sonu.kumar.singh02@sap.com>
@shafeeqes
Overwrite whole strategy while setting
f313ce3 
RollingUpdate configuration  will be non-nil otherwise
@shafeeqes
Address PR review feedback from rfranzke
19f4ade
@shafeeqes
Address PR review feedback from plkokanov
ad742c1
@shafeeqes
Address PR review feedback from ary1992
d5fad11
@shafeeqes shafeeqes force-pushed the inplace-updates-worker branch from 3e14a0b to d5fad11 Compare April 16, 2025 09:53
@gardener-prow gardener-prow bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 16, 2025
plkokanov
plkokanov reviewed Apr 17, 2025
View reviewed changes
Copy link
Contributor

@plkokanov plkokanov left a comment

Choose a reason for hiding this comment

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

/lgtm
From my PoV. I'm still not confident enough to also approve it but I wanted to unblock it as I will be in vacation until mid next week.

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

gardener-prow bot commented Apr 17, 2025

LGTM label has been added.

Git tree hash: 9d6f02b318e931c4b755ab316233e209593a6c2d

@rfranzke
Copy link
Member

/lgtm

@rfranzke rfranzke requested a review from acumino April 17, 2025 07:31
ary1992
ary1992 reviewed Apr 17, 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

acumino
acumino reviewed Apr 17, 2025
View reviewed changes
acumino
acumino reviewed Apr 17, 2025
View reviewed changes
Copy link
Member

@acumino acumino left a comment

Choose a reason for hiding this comment

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

/approve

@gardener-prow Gardener Prow
Copy link
Contributor

gardener-prow bot commented Apr 17, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: acumino

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 Apr 17, 2025
@gardener-prow gardener-prow bot merged commit 8080039 into gardener:master Apr 17, 2025
19 checks passed
@shafeeqes shafeeqes deleted the inplace-updates-worker branch April 17, 2025 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ary1992 ary1992 ary1992 left review comments

@acumino acumino acumino requested changes

@plkokanov plkokanov plkokanov requested changes

@timebertt timebertt Awaiting requested review from timebertt

@rfranzke rfranzke Awaiting requested review from rfranzke

Assignees

@ary1992 ary1992

@rfranzke rfranzke

@acumino acumino

@plkokanov plkokanov

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/control-plane Control plane related area/delivery Delivery related area/open-source Open Source (community, enablement, contributions, conferences, CNCF, etc.) related area/scalability Scalability related cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. kind/enhancement Enhancement, improvement, extension 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.
5 participants
@shafeeqes @plkokanov @rfranzke @ary1992 @acumino