Skip to content

Conversation

@acumino
Copy link
Member

@acumino acumino commented Jul 22, 2025

What this PR does / why we need it:
This PR updates the transferMachinesFromOldToNewMachineSet function to skip machines in the InPlaceUpdating phase. This prevents the machine controller from failing to update the phase to InPlaceUpdateSuccessful, which relies on the node having the label node.machine.sapcloud.io/update-result. The deployment controller removes this label after a successful update, and the machine is moved to the new machine set, causing the machine phase to get stuck in InPlaceUpdating and eventually leading to the InPlaceUpdateFailed phase, as the machine controller never gets a chance to update the phase to InPlaceUpdateSuccessful.

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

Special notes for your reviewer:
/invite @aaronfern

Release note:

Fixed a bug where machines in the `InPlaceUpdating` phase were incorrectly transferred to the new machine set during inplace updates. This caused the machine controller to miss updating the phase to `InPlaceUpdateSuccessful`, resulting in machines getting stuck or marked as `InPlaceUpdateFailed`.

@acumino acumino requested a review from a team as a code owner July 22, 2025 18:48
@gardener-robot gardener-robot requested a review from aaronfern July 22, 2025 18:48
@gardener-robot gardener-robot added needs/review Needs review size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) labels Jul 22, 2025
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 22, 2025
@gardener-robot-ci-3 gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jul 22, 2025
Copy link
Member

@aaronfern aaronfern 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-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review labels Jul 23, 2025
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 23, 2025
@aaronfern aaronfern merged commit e605094 into gardener:master Jul 23, 2025
8 checks passed
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Jul 23, 2025
thiyyakat pushed a commit to thiyyakat/machine-controller-manager that referenced this pull request Jul 23, 2025
…to prevent update phase failures (gardener#1020)

* If machine is in phase `InPlaceUpdating` it should not be moved to new machine set

* Prevent negative value calculation
@acumino acumino deleted the inp/enh branch September 8, 2025 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants