Skip to content

Fix bug which leaves stale machine class name on inplace updated machine - #1153

Merged
gardener-prow[bot] merged 2 commits into
gardener:masterfrom
thiyyakat:bug/inplace-machineclassname
Sep 21, 2026
Merged

gardener-prow[bot] merged 2 commits into
gardener:masterfrom
thiyyakat:bug/inplace-machineclassname

Conversation

@thiyyakat

Copy link
Copy Markdown
Member

What this PR does / why we need it:

Currently, transferMachinesFromOldToNewMachineSet does not set the new machine class's name on the machine object, causing machine.Spec.Class.Name to contain the old machine class's name.

This causes the old machine class to stick around indefinitely because there are machines still referencing it. Additionally, the stale machine class name in the machine's spec prevents changes in the virtual capacity from being picked up for the updated machine.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Please note that newMachineSet.Spec.MachineClass is always empty, which is why the patch uses the value newMachineSet.Spec.Template.Spec.Class.Name.

Before the fix (Running MCM v0.62.1) :

> k get mcc                                                                                                                                               
NAME                                              AGE
shoot--x--preserve-inplace-test2-z1-74949   30m
> k get mc -oyaml| grep class -i -1                                                
  spec:
    class:
      kind: MachineClass
      name: shoot--x--preserve-inplace-test2-z1-74949

After the inplace update:

> k get mcc                                                                            
NAME                                              AGE
shoot--x--preserve-inplace-test2-z1-74949   34m <-- sticks around
shoot--x--preserve-inplace-test2-z1-a3a40   3m6s
> k get mc -oyaml| grep class -i -1                                              

  spec:
    class:
      kind: MachineClass
      name: shoot--x--preserve-inplace-test2-z1-74949 <-- not updated

After the fix (Running PR branch), after inplace update:

> k get mcc         
                                                             
NAME                                              AGE
shoot--x--preserve-inplace-test2-z1-9d9f2   4m10s
> k get mc -oyaml| grep class -i -1                                                                                                                              
  spec:
    class:
      kind: MachineClass
      name: shoot--x--preserve-inplace-test2-z1-9d9f2

Release note:

Patch `machine.Spec.Class.Name` on machine object with the new machine class's name after it undergoes an inplace update.

@thiyyakat
thiyyakat requested a review from a team as a code owner September 15, 2026 11:07
@gardener-prow gardener-prow Bot added do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 15, 2026
…ew machineSet

Signed-off-by: thiyyakat <meghana.thiyyakat@sap.com>
@thiyyakat
thiyyakat force-pushed the bug/inplace-machineclassname branch from 2d2f68d to f2c8fb2 Compare September 15, 2026 11:10
@aaronfern aaronfern added the kind/bug Bug label Sep 16, 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 Sep 16, 2026

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

Is it possible to write a unit test for transferMachinesFromOldToNewMachineSet? To ensure that referenced mcs, labels, and mcc are updated

v1alpha1.SchemeGroupVersion.WithKind("MachineSet").Kind,
newMachineSet.GetName(), newMachineSet.GetUID(), string(labelsJSONBytes), oldMachine.UID)

newMachineSet.GetName(), newMachineSet.GetUID(), string(labelsJSONBytes), oldMachine.UID, newMachineSet.Spec.Template.Spec.Class.Name)

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.

Quick Q: Is there an unit-test or integration test for this ?

@thiyyakat thiyyakat Sep 16, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There weren't tests specifically for this. I've added unit tests now on @aaronfern 's suggestion.
Please see commit: a8e519b.

IT for inplace is under review: #1144

@gardener-prow gardener-prow Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 16, 2026
…label, ownerRef and machine class name updates.

Signed-off-by: thiyyakat <meghana.thiyyakat@sap.com>
@thiyyakat
thiyyakat force-pushed the bug/inplace-machineclassname branch from 8abf0b3 to a8e519b Compare September 16, 2026 08:25
@aaronfern

Copy link
Copy Markdown
Member

Thanks for the changes!
/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: c7ddf3d72cb9f3e710356c87ebd31f82a4e0801c

@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: elankath

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

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

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

Development

Successfully merging this pull request may close these issues.

3 participants