Skip to content

Merge node finalizer removal and node deletion into one step #1097

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

Merged
gardener-prow merged 11 commits into from
Apr 24, 2026
Merged

Merge node finalizer removal and node deletion into one step #1097

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
84353c6
Atomically perform finalizer removal
gagan16k Apr 10, 2026
0e10a8e
update tests
gagan16k Apr 10, 2026
2d52801
Check machine deletion status before adding finalizers
gagan16k Apr 10, 2026
acfa4a1
Refactor node update logic
gagan16k Apr 10, 2026
439698b
cleanup
gagan16k Apr 10, 2026
dcaf05a
Address review comments
gagan16k Apr 17, 2026
6bb7293
test fix
gagan16k Apr 17, 2026
136039e
consistency changes
gagan16k Apr 17, 2026
582a6ec
Address review comments
gagan16k Apr 20, 2026
9899422
fix test
gagan16k Apr 20, 2026
c1f5e06
revert parameter name
gagan16k Apr 20, 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
3 changes: 0 additions & 3 deletions pkg/util/provider/machinecontroller/machine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -665,9 +665,6 @@ func (c *controller) triggerDeletionFlow(ctx context.Context, deleteMachineReque
case strings.Contains(machine.Status.LastOperation.Description, machineutils.InitiateVMDeletion):
return c.deleteVM(ctx, deleteMachineRequest)

case strings.Contains(machine.Status.LastOperation.Description, machineutils.RemoveNodeFinalizers):
return c.deleteNodeFinalizers(ctx, machine)

case strings.Contains(machine.Status.LastOperation.Description, machineutils.InitiateNodeDeletion):
return c.deleteNodeObject(ctx, machine)

Expand Down
114 changes: 6 additions & 108 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 @@ -3015,7 +3015,7 @@ var _ = Describe("machine", func() {
},
},
expect: expect{
err: fmt.Errorf("Machine deletion in process. VM deletion was successful. " + machineutils.RemoveNodeFinalizers),
err: fmt.Errorf("Machine deletion in process. VM deletion was successful. " + machineutils.InitiateNodeDeletion),
retry: machineutils.ShortRetry,
machine: newMachine(
&v1alpha1.MachineTemplateSpec{
Expand All @@ -3034,7 +3034,7 @@ var _ = Describe("machine", func() {
LastUpdateTime: metav1.Now(),
},
LastOperation: v1alpha1.LastOperation{
Description: fmt.Sprintf("VM deletion was successful. %s", machineutils.RemoveNodeFinalizers),
Description: fmt.Sprintf("VM deletion was successful. %s", machineutils.InitiateNodeDeletion),
State: v1alpha1.MachineStateProcessing,
Type: v1alpha1.MachineOperationDelete,
LastUpdateTime: metav1.Now(),
Expand All @@ -3052,7 +3052,7 @@ var _ = Describe("machine", func() {
),
},
}),
Entry("Delete node finalizers successfully", &data{
Entry("Delete node object successfully", &data{
setup: setup{
secrets: []*corev1.Secret{
{
Expand Down Expand Up @@ -3083,7 +3083,7 @@ var _ = Describe("machine", func() {
LastUpdateTime: metav1.Now(),
},
LastOperation: v1alpha1.LastOperation{
Description: fmt.Sprintf("VM deletion was successful. %s", machineutils.RemoveNodeFinalizers),
Description: fmt.Sprintf("VM deletion was successful. %s", machineutils.InitiateNodeDeletion),
State: v1alpha1.MachineStateProcessing,
Type: v1alpha1.MachineOperationDelete,
LastUpdateTime: metav1.Now(),
Expand Down Expand Up @@ -3120,109 +3120,7 @@ var _ = Describe("machine", func() {
},
},
expect: expect{
err: fmt.Errorf("machine deletion in process. Removal of finalizers from Node Object \"fakeID-0\" is successful. " + machineutils.InitiateNodeDeletion),
retry: machineutils.ShortRetry,
machine: newMachine(
&v1alpha1.MachineTemplateSpec{
ObjectMeta: *newObjectMeta(objMeta, 0),
Spec: v1alpha1.MachineSpec{
Class: v1alpha1.ClassSpec{
Kind: "MachineClass",
Name: "machine-0",
},
ProviderID: "fakeID",
},
},
&v1alpha1.MachineStatus{
CurrentStatus: v1alpha1.CurrentStatus{
Phase: v1alpha1.MachineTerminating,
LastUpdateTime: metav1.Now(),
},
LastOperation: v1alpha1.LastOperation{
Description: fmt.Sprintf("Removal of finalizers from Node Object %q is successful. %s", "fakeID-0", machineutils.InitiateNodeDeletion),
State: v1alpha1.MachineStateProcessing,
Type: v1alpha1.MachineOperationDelete,
LastUpdateTime: metav1.Now(),
},
},
nil,
map[string]string{
machineutils.MachinePriority: "3",
},
map[string]string{
v1alpha1.NodeLabelKey: "fakeID-0",
},
true,
metav1.Now(),
),
},
}),
Entry("Delete node object successfully", &data{
setup: setup{
secrets: []*corev1.Secret{
{
ObjectMeta: *newObjectMeta(objMeta, 0),
},
},
machineClasses: []*v1alpha1.MachineClass{
{
ObjectMeta: *newObjectMeta(objMeta, 0),
SecretRef: newSecretReference(objMeta, 0),
},
},
machines: newMachines(
1,
&v1alpha1.MachineTemplateSpec{
ObjectMeta: *newObjectMeta(objMeta, 0),
Spec: v1alpha1.MachineSpec{
Class: v1alpha1.ClassSpec{
Kind: "MachineClass",
Name: "machine-0",
},
ProviderID: "fakeID",
},
},
&v1alpha1.MachineStatus{
CurrentStatus: v1alpha1.CurrentStatus{
Phase: v1alpha1.MachineTerminating,
LastUpdateTime: metav1.Now(),
},
LastOperation: v1alpha1.LastOperation{
Description: fmt.Sprintf("Removal of finalizers from Node Object %q is successful. %s", "fakeID-0", machineutils.InitiateNodeDeletion),
State: v1alpha1.MachineStateProcessing,
Type: v1alpha1.MachineOperationDelete,
LastUpdateTime: metav1.Now(),
},
},
nil,
map[string]string{
machineutils.MachinePriority: "3",
},
map[string]string{
v1alpha1.NodeLabelKey: "fakeID-0",
},
true,
metav1.Now(),
),
nodes: []*corev1.Node{
{
ObjectMeta: metav1.ObjectMeta{
Name: "fakeID-0",
},
},
},
},
action: action{
machine: "machine-0",
fakeDriver: &driver.FakeDriver{
VMExists: true,
ProviderID: "fakeID-0",
NodeName: "fakeNode-0",
Err: nil,
},
},
expect: expect{
err: fmt.Errorf("Machine deletion in process. Deletion of node object was successful"),
err: fmt.Errorf("Machine deletion in process. Deletion of Node Object %q is successful. %s", "fakeID-0", machineutils.InitiateFinalizerRemoval),
retry: machineutils.ShortRetry,
nodeDeleted: true,
machine: newMachine(
Expand Down Expand Up @@ -3624,7 +3522,7 @@ var _ = Describe("machine", func() {
LastUpdateTime: metav1.Now(),
},
LastOperation: v1alpha1.LastOperation{
Description: fmt.Sprintf("Label %q not present on machine %q or no associated node object found, continuing deletion flow. %s", v1alpha1.NodeLabelKey, "machine-0", machineutils.InitiateFinalizerRemoval),
Description: fmt.Sprintf("Label %q not present on machine %q, continuing deletion flow. %s", v1alpha1.NodeLabelKey, "machine-0", machineutils.InitiateFinalizerRemoval),
State: v1alpha1.MachineStateProcessing,
Type: v1alpha1.MachineOperationDelete,
LastUpdateTime: metav1.Now(),
Expand Down
92 changes: 25 additions & 67 deletions pkg/util/provider/machinecontroller/machine_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1832,7 +1832,7 @@ func (c *controller) deleteVM(ctx context.Context, deleteMachineRequest *driver.
state = v1alpha1.MachineStateFailed
case codes.NotFound:
retryRequired = machineutils.ShortRetry
description = fmt.Sprintf("VM not found. Continuing deletion flow. %s", machineutils.RemoveNodeFinalizers)
description = fmt.Sprintf("VM not found. Continuing deletion flow. %s", machineutils.InitiateNodeDeletion)
state = v1alpha1.MachineStateProcessing
default:
retryRequired = machineutils.LongRetry
Expand All @@ -1847,7 +1847,7 @@ func (c *controller) deleteVM(ctx context.Context, deleteMachineRequest *driver.

} else {
retryRequired = machineutils.ShortRetry
description = fmt.Sprintf("VM deletion was successful. %s", machineutils.RemoveNodeFinalizers)
description = fmt.Sprintf("VM deletion was successful. %s", machineutils.InitiateNodeDeletion)
state = v1alpha1.MachineStateProcessing

err = fmt.Errorf("Machine deletion in process. %s", description)
Expand Down Expand Up @@ -1880,8 +1880,8 @@ func (c *controller) deleteVM(ctx context.Context, deleteMachineRequest *driver.
return retryRequired, err
}

// deleteNodeFinalizers attempts to remove finalizers from the node object backed by the machine object
func (c *controller) deleteNodeFinalizers(ctx context.Context, machine *v1alpha1.Machine) (machineutils.RetryPeriod, error) {
// deleteNodeObject attempts to remove finalizers from and delete the node object backed by the machine object
func (c *controller) deleteNodeObject(ctx context.Context, machine *v1alpha1.Machine) (machineutils.RetryPeriod, error) {
var (
err error
description string
Expand All @@ -1896,86 +1896,44 @@ func (c *controller) deleteNodeFinalizers(ctx context.Context, machine *v1alpha1
if err != nil {
switch {
case apierrors.IsNotFound(err):
description = fmt.Sprintf("No node object found for %q, skipping node finalizer removal. %s", nodeName, machineutils.InitiateNodeDeletion)
description = fmt.Sprintf("No node object found for %q, continuing deletion flow. %s", nodeName, machineutils.InitiateFinalizerRemoval)
klog.Warning(description)
state = v1alpha1.MachineStateProcessing
default:
description = fmt.Sprintf("Retrieval of Node Object %q failed due to error: %s, Retrying node finalizer removal. %s", nodeName, err, machineutils.RemoveNodeFinalizers)
description = fmt.Sprintf("Retrieval of Node Object %q failed due to error: %s, Retrying. %s", nodeName, err, machineutils.InitiateNodeDeletion)
klog.Errorf(description)
state = v1alpha1.MachineStateFailed
}
} else {
err = c.removeNodeFinalizers(ctx, node)
if err != nil {
description = fmt.Sprintf("Removal of finalizers from Node Object %q failed due to error: %s, Retrying node finalizer removal. %s", nodeName, err, machineutils.RemoveNodeFinalizers)
description = fmt.Sprintf("Removal of finalizers from Node Object %q failed due to error: %s, Retrying. %s", nodeName, err, machineutils.InitiateNodeDeletion)
klog.Errorf(description)
state = v1alpha1.MachineStateFailed
} else {
description = fmt.Sprintf("Removal of finalizers from Node Object %q is successful. %s", nodeName, machineutils.InitiateNodeDeletion)
state = v1alpha1.MachineStateProcessing
err = fmt.Errorf("machine deletion in process. %s", description)
// Delete node object
klog.V(3).Infof("Deleting node %q associated with machine %q", nodeName, machine.Name)
err = c.targetCoreClient.CoreV1().Nodes().Delete(ctx, nodeName, metav1.DeleteOptions{})
if err != nil && !apierrors.IsNotFound(err) {
description = fmt.Sprintf("Deletion of Node Object %q failed due to error: %s. %s", nodeName, err, machineutils.InitiateNodeDeletion)
klog.Error(description)
state = v1alpha1.MachineStateFailed
} else if err == nil {
description = fmt.Sprintf("Deletion of Node Object %q is successful. %s", nodeName, machineutils.InitiateFinalizerRemoval)
klog.V(3).Info(description)
state = v1alpha1.MachineStateProcessing
err = fmt.Errorf("Machine deletion in process. %s", description)
} else {
description = fmt.Sprintf("No node object found for %q, continuing deletion flow. %s", nodeName, machineutils.InitiateFinalizerRemoval)
klog.Warning(description)
state = v1alpha1.MachineStateProcessing
}
}
}
} else {
description = fmt.Sprintf("Label %q not present on machine %q, skipping node finalizer removal. %s", v1alpha1.NodeLabelKey, machine.Name, machineutils.InitiateNodeDeletion)
description = fmt.Sprintf("Label %q not present on machine %q, continuing deletion flow. %s", v1alpha1.NodeLabelKey, machine.Name, machineutils.InitiateFinalizerRemoval)
Comment thread
takoverflow marked this conversation as resolved.
Outdated
klog.Warning(description)
state = v1alpha1.MachineStateProcessing
}

updateRetryPeriod, updateErr := c.machineStatusUpdate(
ctx,
machine,
v1alpha1.LastOperation{
Description: description,
State: state,
Type: v1alpha1.MachineOperationDelete,
LastUpdateTime: metav1.Now(),
},
// Let the clone.Status.CurrentStatus (LastUpdateTime) be as it was before.
// This helps while computing when the drain timeout to determine if force deletion is to be triggered.
machine.Status.CurrentStatus,
machine.Status.LastKnownState,
)

if updateErr != nil {
return updateRetryPeriod, updateErr
}
return machineutils.ShortRetry, err
}

// deleteNodeObject attempts to delete the node object backed by the machine object
func (c *controller) deleteNodeObject(ctx context.Context, machine *v1alpha1.Machine) (machineutils.RetryPeriod, error) {
var (
err error
description string
state v1alpha1.MachineState
)

nodeName := machine.Labels[v1alpha1.NodeLabelKey]

if nodeName != "" {
// Delete node object
err = c.targetCoreClient.CoreV1().Nodes().Delete(ctx, nodeName, metav1.DeleteOptions{})
klog.V(3).Infof("Deleting node %q associated with machine %q", nodeName, machine.Name)
if err != nil && !apierrors.IsNotFound(err) {
// If its an error, and any other error than object not found
description = fmt.Sprintf("Deletion of Node Object %q failed due to error: %s. %s", nodeName, err, machineutils.InitiateNodeDeletion)
klog.Error(description)
state = v1alpha1.MachineStateFailed
} else if err == nil {
description = fmt.Sprintf("Deletion of Node Object %q is successful. %s", nodeName, machineutils.InitiateFinalizerRemoval)
klog.V(3).Info(description)
state = v1alpha1.MachineStateProcessing
err = fmt.Errorf("Machine deletion in process. Deletion of node object was successful")
} else {
description = fmt.Sprintf("No node object found for %q, continuing deletion flow. %s", nodeName, machineutils.InitiateFinalizerRemoval)
klog.Warning(description)
state = v1alpha1.MachineStateProcessing
}
} else {
description = fmt.Sprintf("Label %q not present on machine %q or no associated node object found, continuing deletion flow. %s", v1alpha1.NodeLabelKey, machine.Name, machineutils.InitiateFinalizerRemoval)
klog.Error(description)
state = v1alpha1.MachineStateProcessing
err = fmt.Errorf("Machine deletion in process. No node object found")
}

Expand Down
47 changes: 23 additions & 24 deletions pkg/util/provider/machinecontroller/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@ import (
"encoding/json"
"errors"
"fmt"
"github.com/gardener/machine-controller-manager/pkg/controller/autoscaler"
"time"

"github.com/gardener/machine-controller-manager/pkg/controller/autoscaler"

"github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1"
"github.com/gardener/machine-controller-manager/pkg/util/provider/machineutils"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -72,33 +73,29 @@ func (c *controller) updateNode(oldObj, newObj any) {
}

// delete the machine if the node is deleted
if node.DeletionTimestamp != nil {
switch {
case node.DeletionTimestamp != nil:
err := c.triggerMachineDeletion(context.Background(), node.Name)
if err != nil {
c.enqueueNodeAfter(node, time.Duration(machineutils.ShortRetry), fmt.Sprintf("handling node UPDATE event. Failed to trigger machine deletion for node %q, re-queuing", node.Name))
Comment thread
takoverflow marked this conversation as resolved.
Outdated
}
return
}

if !HasFinalizer(node, NodeFinalizerName) {
case !HasFinalizer(node, NodeFinalizerName):
c.enqueueNodeAfter(node, time.Duration(machineutils.MediumRetry), fmt.Sprintf("MCM finalizer missing from node %q, re-queuing", node.Name))
Comment thread
r4mek marked this conversation as resolved.
Outdated
Comment thread
takoverflow marked this conversation as resolved.
Outdated
return
}
// to reconcile on addition/removal of essential taints in machine lifecycle, example - critical component taint
if addedOrRemovedEssentialTaints(oldNode, node, machineutils.EssentialTaints) {
c.enqueueMachine(machine, fmt.Sprintf("handling node UPDATE event. Atleast one of essential taints on node %q has changed", getNodeName(machine)))
return
}
if inPlaceUpdateLabelsChanged(oldNode, node) {
c.enqueueMachine(machine, fmt.Sprintf("handling node UPDATE event. in-place update label added or updated for node %q", getNodeName(machine)))
return
}

isMachineCrashLooping := machine.Status.CurrentStatus.Phase == v1alpha1.MachineCrashLoopBackOff
isMachineTerminating := machine.Status.CurrentStatus.Phase == v1alpha1.MachineTerminating
_, _, nodeConditionsHaveChanged := nodeConditionsHaveChanged(machine.Status.Conditions, node.Status.Conditions)

// Enqueue machine if node conditions have changed and machine is not in crashloop or terminating state
if nodeConditionsHaveChanged && !(isMachineCrashLooping || isMachineTerminating) {
// to reconcile on addition/removal of essential taints in machine lifecycle, example - critical component taint
switch {
case addedOrRemovedEssentialTaints(oldNode, node, machineutils.EssentialTaints):
c.enqueueMachine(machine, fmt.Sprintf("handling node UPDATE event. Atleast one of essential taints on node %q has changed", node.Name))
case inPlaceUpdateLabelsChanged(oldNode, node):
c.enqueueMachine(machine, fmt.Sprintf("handling node UPDATE event. in-place update label added or updated for node %q", node.Name))
Comment thread
takoverflow marked this conversation as resolved.
case nodeConditionsHaveChanged && !(isMachineCrashLooping || isMachineTerminating):
// Enqueue machine if node conditions have changed and machine is not in crashloop or terminating state
c.enqueueMachine(machine, fmt.Sprintf("handling node UPDATE event. Conditions of node %q differ from machine status", node.Name))
}
// to reconcile on change in annotations related to preservation
Comment thread
r4mek marked this conversation as resolved.
Outdated
Expand Down Expand Up @@ -155,7 +152,8 @@ func (c *controller) reconcileClusterNodeKey(key string) error {

// 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.
if _, err := c.getMachineFromNode(node.Name); err != nil {
machine, err := c.getMachineFromNode(node.Name)
if err != nil {
if errors.Is(err, errNoMachineMatch) {
klog.Errorf("ClusterNode %q: No machine found matching node, skipping adding finalizers", key)
return nil
Expand All @@ -176,13 +174,14 @@ func (c *controller) reconcileClusterNodeKey(key string) error {
return nil
}

//Add finalizers to node object if not present
err = c.addNodeFinalizers(ctx, node)
if err != nil {
klog.Errorf("ClusterNode %q: error adding finalizers to node: %v", key, err)
c.enqueueNodeAfter(node, time.Duration(machineutils.ShortRetry), err.Error())
if machine.DeletionTimestamp == nil { // If machine is marked for deletion, ensure node finalizers are not added
err = c.addNodeFinalizers(ctx, node) // Add finalizers to node object if not present
Comment thread
takoverflow marked this conversation as resolved.
Outdated
if err != nil {
klog.Errorf("ClusterNode %q: error adding finalizers to node: %v", key, err)
c.enqueueNodeAfter(node, time.Duration(machineutils.ShortRetry), err.Error())
return nil
Comment thread
takoverflow marked this conversation as resolved.
Outdated
}
}

return nil
}

Expand Down
Loading
Loading