Trigger node deletion when machine is deleted by gagan16k · Pull Request #1102 · gardener/machine-controller-manager

gagan16k · 2026-05-18T12:33:47Z

What this PR does / why we need it:

Machine controller now explicitly deletes the backing node from the target cluster on machine delete events, and removes finalizers from nodes that are being deleted with no backing machine.
The machine status description during creation has been updated from "Creating machine on cloud provider" to "VM created on cloud provider. Waiting for node registration", so the status accurately reflects which stage the machine is in.

Which issue(s) this PR fixes:
Fixes #1044, #1064

Tests:
Passed unit tests

Manual tests performed by deleting node manually

(⎈|garden--aws-ha-external:garden shoot--i765230--demo)➜  machine-controller-manager git:(pr/gagan16k/1102) k get mc
NAME                                              STATUS    AGE   NODE
shoot--i765230--demo-worker-cpu-z1-7988d-sdclq   Running    9m   ip-10-180-30-115.eu-west-1.compute.internal
shoot--i765230--demo-worker-etcd-z1-5dc7f-vpt7k   Running   41m   ip-10-180-134-208.eu-west-1.compute.internal

(⎈|garden-i765230--demo-external:garden-i765230 default)➜  machine-controller-manager git:(pr/gagan16k/1102) k get no
NAME                                           STATUS   ROLES    AGE   VERSION
ip-10-180-134-208.eu-west-1.compute.internal   Ready    worker   40m   v1.34.9
ip-10-180-30-115.eu-west-1.compute.internal    Ready    worker   10m   v1.34.9



# Force-delete machine by removing finalizer
(⎈|garden--aws-ha-external:garden shoot--i765230--demo)➜  machine-controller-manager git:(pr/gagan16k/1102) k -n shoot--i765230--demo patch machine shoot--i765230--demo-worker-cpu-z1-7988d-sdclq --type=merge -p '{"metadata":{"finalizers":null}}'
machine.machine.sapcloud.io/shoot--i765230--demo-worker-cpu-z1-7988d-sdclq patched
(⎈|garden--aws-ha-external:garden shoot--i765230--demo)➜  machine-controller-manager git:(pr/gagan16k/1102) k -n shoot--i765230--demo delete machine shoot--i765230--demo-worker-cpu-z1-7988d-sdclq
machine.machine.sapcloud.io "shoot--i765230--demo-worker-cpu-z1-7988d-sdclq" deleted



# Machine and node deleted
(⎈|garden-i765230--demo-external:garden-i765230 default)➜  machine-controller-manager git:(pr/gagan16k/1102) k get no
NAME                                           STATUS   ROLES    AGE   VERSION
ip-10-180-134-208.eu-west-1.compute.internal   Ready    worker   40m   v1.34.9

(⎈|garden--aws-ha-external:garden shoot--i765230--demo)➜  machine-controller-manager git:(pr/gagan16k/1102) k get mc
NAME                                              STATUS    AGE   NODE
shoot--i765230--demo-worker-cpu-z1-7988d-nvls2    Pending   15s   ip-10-180-3-36.eu-west-1.compute.internal
shoot--i765230--demo-worker-etcd-z1-5dc7f-vpt7k   Running   41m   ip-10-180-134-208.eu-west-1.compute.internal

For #1064, machine during creation has message

Last Operation:
    Description:       VM created on cloud provider. Waiting for node registration
    Last Update Time:  2026-08-12T06:20:01Z
    State:             Processing
    Type:              Create

Release note:

MCM now deletes the backing node when its machine is force-deleted, and clears finalizers on nodes terminating without a backing machine. The machine creation status is also modified to distinguish cloud-provider failures from node-registration failures.

thiyyakat

Thanks for the changes. Just two comments. PTAL.

gardener-ci-robot · 2026-07-04T10:03:27Z

The Gardener project currently lacks enough active contributors to adequately respond to all PRs.
This bot triages PRs according to the following rules:

After 30d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 14d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as active with /lifecycle active
Mark this PR as fresh with /remove-lifecycle stale
Mark this PR as rotten with /lifecycle rotten
Close this PR with /close

/lifecycle stale

gardener-ci-robot · 2026-08-03T10:25:55Z

The Gardener project currently lacks enough active contributors to adequately respond to all PRs.
This bot triages PRs according to the following rules:

After 30d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 14d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as active with /lifecycle active
Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close

/lifecycle rotten

…riptions

thiyyakat

Just one small nit.

thiyyakat · 2026-08-12T06:05:28Z

Thanks for addressing the comments.
/lgtm

gardener-prow · 2026-08-12T06:05:34Z

LGTM label has been added.

Details

Git tree hash: 7c6a71242f80f6ef4851c138c0bece6fce8dbd6c

takoverflow · 2026-08-12T06:06:19Z

/assign

gardener-prow · 2026-08-12T06:44:23Z

[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 takoverflow. 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:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

thiyyakat · 2026-08-12T06:48:29Z

/lgtm

gardener-prow · 2026-08-12T06:48:35Z

LGTM label has been added.

Details

Git tree hash: 5518b0ad517494d0016de9dc24f7dd9aec43f5b1

takoverflow

Thanks for the PR, have some queries/suggestion PTAL

takoverflow · 2026-08-12T06:18:49Z

 	}
 	c.enqueueMachineTermination(machine, "handling terminating machine object DELETE event")
+
+	// Node retains finalizer here, removed by reconcileClusterNodeKey once DeletionTimestamp is set


Why is the finalizer being retained here, wouldn't it make sense to speed up cleanup by removing it as well? Why wait for node reconciliation to happen?

Is it for convenience or there's some value in preserving the finalizer still?

There was a pre-existing mechanism to handle the finalizer removal (So that any nodes that do not have a machine do not get a finalizer, added as part of #1065).
So I did not add it here, as the finalizer is removed anyways

Yes but AFAICS the current reconcileClusterNodeKey doesn't have any finalizer removal logic

machine-controller-manager/pkg/util/provider/machinecontroller/node.go

Line 133 in 580d98f

func (c *controller) reconcileClusterNodeKey(key string) error {

Please correct me if I'm wrong but the only place we currently actively remove the finalizer from the node and issue deletion is deleteNodeObject which is part of triggerDeletionFlow.

Oh, really sorry for the confusion, #1065 had no finalizer removal steps (The change was to never add it in the first place).

But we do have it now now as part of this PR

takoverflow · 2026-08-12T06:51:53Z

-	// transient fetch errors will be eventually requeued by the update handler due to kubelet updates.
+	// Ignore node updates without an associated machine, except when the node is being
+	// deleted, then remove the MCM finalizer to unblock deletion. Retry only for
+	// errors other than errNoMachineMatch; transient fetch errors will be eventually


If there's a transient error while fetching the machine for the node, and node deletion was triggered via kubectl delete node, we can prematurely remove the finalizers now right?

Or there's no possibility of getting no machine match (when there is a machine) because we're relying on listers?

NoMachineMatch (when there is a machine) is theoretically possible but is probably very rare.
Could add an extra API server fetch to actually check if the machine is truly gone, but this might be unnecessary? Not sure, WDYT

That's why I thought maybe the finalizer removal and node deletion could happen as a unit as part of machine on delete event similar to how its done in deleteNodeObject. So all this checking here becomes unnecessary.

Not proposing that this is how it should be done, just pitching an alternative to see if there's any pros/cons of doing it either way.

gagan16k requested a review from a team as a code owner May 18, 2026 12:33

aaronfern added the kind/bug Bug label May 22, 2026

gardener-prow Bot removed the do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label May 22, 2026

thiyyakat reviewed Jun 3, 2026

View reviewed changes

Comment thread pkg/util/provider/machinecontroller/machine.go Outdated

Comment thread pkg/util/provider/machinecontroller/node.go

r4mek reviewed Jun 4, 2026

View reviewed changes

Comment thread pkg/util/provider/machinecontroller/machine_test.go Outdated

gardener-prow Bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 4, 2026

gardener-prow Bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 3, 2026

aaronfern removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Aug 3, 2026

gardener-prow Bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 7, 2026

gagan16k added 2 commits August 7, 2026 13:18

Trigger node deletion when machine is deleted and update machine desc…

da4359c

…riptions

Address review comments

9864805

gagan16k force-pushed the fix/machine-delete branch from 3a45864 to 9864805 Compare August 7, 2026 08:00

Fix test

e02a450

thiyyakat reviewed Aug 12, 2026

View reviewed changes

Comment thread pkg/util/provider/machinecontroller/node.go Outdated

gagan16k added 2 commits August 12, 2026 11:17

Remove redundant else

70001bf

Change log message

92ba83b

gardener-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Aug 12, 2026

gardener-prow Bot assigned takoverflow Aug 12, 2026

Improve error handling and logging for node deletion

2cf292e

gardener-prow Bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 12, 2026

gardener-prow Bot requested a review from takoverflow August 12, 2026 06:44

gardener-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Aug 12, 2026

takoverflow reviewed Aug 12, 2026

View reviewed changes

Uh oh!

Conversation

gagan16k commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thiyyakat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gardener-ci-robot commented Jul 4, 2026

Uh oh!

gardener-ci-robot commented Aug 3, 2026

Uh oh!

thiyyakat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thiyyakat commented Aug 12, 2026

Uh oh!

gardener-prow Bot commented Aug 12, 2026

Uh oh!

takoverflow commented Aug 12, 2026

Uh oh!

gardener-prow Bot commented Aug 12, 2026

Uh oh!

thiyyakat commented Aug 12, 2026

Uh oh!

gardener-prow Bot commented Aug 12, 2026

Uh oh!

takoverflow left a comment

Choose a reason for hiding this comment

Uh oh!

takoverflow Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

gagan16k Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

takoverflow Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

gagan16k Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

takoverflow Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

gagan16k Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

takoverflow Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gagan16k commented May 18, 2026 •

edited

Loading

gagan16k Aug 12, 2026 •

edited

Loading

takoverflow Aug 12, 2026 •

edited

Loading