Skip to content

Trigger node deletion when machine is deleted #1102

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

Open
gagan16k wants to merge 6 commits into
base: master
Choose a base branch
from
+35 −8
Open

Trigger node deletion when machine is deleted #1102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
da4359c
Trigger node deletion when machine is deleted and update machine desc…
gagan16k May 15, 2026
9864805
Address review comments
gagan16k Aug 7, 2026
e02a450
Fix test
gagan16k Aug 7, 2026
70001bf
Remove redundant else
gagan16k Aug 12, 2026
92ba83b
Change log message
gagan16k Aug 12, 2026
2cf292e
Improve error handling and logging for node deletion
gagan16k Aug 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion pkg/util/provider/machinecontroller/machine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,22 @@ func (c *controller) deleteMachine(obj any) {
}
}
c.enqueueMachineTermination(machine, "handling terminating machine object DELETE event")

// Node retains finalizer here, removed by reconcileClusterNodeKey once DeletionTimestamp is set

@takoverflow takoverflow Aug 12, 2026

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.

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?

@gagan16k gagan16k Aug 12, 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 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

@takoverflow takoverflow Aug 12, 2026

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.

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.

@gagan16k gagan16k Aug 12, 2026 •
edited
Loading

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.

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

if c.targetCoreClient != nil {
if nodeName := machine.Labels[v1alpha1.NodeLabelKey]; nodeName != "" {
err := c.targetCoreClient.CoreV1().Nodes().Delete(context.Background(), nodeName, metav1.DeleteOptions{})
if err != nil {
if apierrors.IsNotFound(err) {
klog.Errorf("backing node %q does not exist/already deleted for deleted machine %q: %v", nodeName, machine.Name, err)
} else {
klog.Errorf("failed to delete backing node %q of deleted machine %q: %v", nodeName, machine.Name, err)
}
} else {
klog.Infof("Successfully triggered deletion of backing node %q for deleted machine %q", nodeName, machine.Name)
}
}
}
}

// getKeyForObj returns key for object, else returns false
Expand Down Expand Up @@ -530,7 +546,7 @@ func (c *controller) triggerCreationFlow(ctx context.Context, createMachineReque
if machine.Status.CurrentStatus.Phase == "" || machine.Status.CurrentStatus.Phase == v1alpha1.MachineCrashLoopBackOff {
clone := clone.DeepCopy()
clone.Status.LastOperation = v1alpha1.LastOperation{
Description: "Creating machine on cloud provider",
Description: "VM created on cloud provider. Waiting for node registration",
Comment thread
takoverflow marked this conversation as resolved.
State: v1alpha1.MachineStateProcessing,
Type: v1alpha1.MachineOperationCreate,
LastUpdateTime: metav1.Now(),
Expand Down
4 changes: 2 additions & 2 deletions pkg/util/provider/machinecontroller/machine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -677,7 +677,7 @@ var _ = Describe("machine", func() {
LastUpdateTime: metav1.Now(),
},
LastOperation: v1alpha1.LastOperation{
Description: "Creating machine on cloud provider",
Description: "VM created on cloud provider. Waiting for node registration",
State: v1alpha1.MachineStateProcessing,
Type: v1alpha1.MachineOperationCreate,
LastUpdateTime: metav1.Now(),
Expand Down Expand Up @@ -722,7 +722,7 @@ var _ = Describe("machine", func() {
LastUpdateTime: metav1.Now(),
},
LastOperation: v1alpha1.LastOperation{
Description: "Creating machine on cloud provider",
Description: "VM created on cloud provider. Waiting for node registration",
State: v1alpha1.MachineStateProcessing,
Type: v1alpha1.MachineOperationCreate,
LastUpdateTime: metav1.Now(),
Expand Down
21 changes: 16 additions & 5 deletions pkg/util/provider/machinecontroller/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,16 @@ func (c *controller) updateNode(oldObj, newObj any) {
return
}

// Do not process node updates if there is no associated machine
// In case of transient errors while fetching machine, do not retry
// as the update handler will be triggered again due to kubelet updates.
// Do not process node updates if there is no associated machine, except
// when the node is being deleted, in that case enqueue it for finalizer removal.
// For transient fetch errors, do not retry; the update handler will be triggered
// again due to kubelet updates.
machine, err := c.getMachineFromNode(node.Name)
if err != nil {
if errors.Is(err, errNoMachineMatch) && node.DeletionTimestamp != nil {
c.enqueueNode(node, fmt.Sprintf("handling node UPDATE event. Node %q is being deleted with no backing machine", node.Name))
return
}
klog.Errorf("unable to handle update event for node %q, couldn't fetch associated machine. Error: %v", node.Name, err)
return
}
Expand Down Expand Up @@ -148,11 +153,17 @@ func (c *controller) reconcileClusterNodeKey(key string) error {
return nil
}

// Ignore node updates without an associated machine. Retry only for errors other than errNoMachineMatch;
// 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

@takoverflow takoverflow Aug 12, 2026

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.

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?

@gagan16k gagan16k Aug 12, 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.

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

@takoverflow takoverflow Aug 12, 2026 •
edited
Loading

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.

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.

// requeued by the update handler due to kubelet updates.
machine, err := c.getMachineFromNode(node.Name)
if err != nil {
if errors.Is(err, errNoMachineMatch) {
if node.DeletionTimestamp != nil {
Comment thread
thiyyakat marked this conversation as resolved.
klog.V(2).Infof("ClusterNode %q: Node is being deleted with no backing machine, removing finalizer", key)
return c.removeNodeFinalizers(ctx, node)
}
klog.Errorf("ClusterNode %q: No machine found matching node, skipping adding finalizers", key)
return nil
}
Expand Down
Loading