Add support for hook to suspend VM deletion by CaptainIRS · Pull Request #1158 · gardener/machine-controller-manager

CaptainIRS · 2026-09-22T03:36:32Z

What this PR does / why we need it:

Adds support for suspending VM deletion after node draining, allowing external controllers to perform pre-termination actions such as removing an etcd member before the VM is deleted.

When a terminating Machine reaches the VM deletion stage and has a suspension annotation, MCM:

skips the provider VM deletion;
sets the InstanceDeletionSuspended=True condition;
reports the responsible owner and purpose in the condition message;

Once the annotation is removed by the external controller, MCM proceeds to start deleting the VM in the reconciliation triggered by the annotation removal.

The annotation format is:

suspend-instance-deletion.node.machine.sapcloud.io/<purpose>: <owner>

For example:

suspend-instance-deletion.node.machine.sapcloud.io/etcd-member-removal: my-controller

Once the external controller removes the annotation, MCM clears the condition and resumes VM deletion.

Which issue(s) this PR fixes:

Fixes #1131

Special notes for your reviewer:

No new MachinePhase is introduced; the existing Terminating phase and deletion flow are preserved.
The annotation intentionally differs from the format suggested in the issue comments. node.machine.sapcloud.io/suspend-instance-deletion/<purpose> is not a valid Kubernetes annotation key because it contains two / separators. The implemented format uses suspend-instance-deletion.node.machine.sapcloud.io as the DNS prefix.
Multiple suspension annotations are supported, with deterministic condition messages.
Tests cover annotation parsing, suspension, condition updates, and resuming deletion.
Verified with:
- go test ./pkg/util/annotations
- go test ./pkg/util/provider/machinecontroller

Testing with g/g:

This patch can be utilized.

diff --git a/example/provider-local/shoot.yaml b/example/provider-local/shoot.yaml
index e5731ed90a..f105440a09 100644
--- a/example/provider-local/shoot.yaml
+++ b/example/provider-local/shoot.yaml
@@ -25,12 +25,14 @@ spec:
     type: local
     workers:
     - name: local
+      annotations:
+        suspend-instance-deletion.node.machine.sapcloud.io/etcd-member-removal: my-controller
       machine:
         type: local
       cri:
         name: containerd
-      minimum: 1
-      maximum: 2
+      minimum: 3
+      maximum: 4
       maxSurge: 1
       maxUnavailable: 0
   kubernetes:
diff --git a/extensions/pkg/controller/worker/genericactuator/actuator_reconcile.go b/extensions/pkg/controller/worker/genericactuator/actuator_reconcile.go
index a9051893f7..6dca6f6973 100644
--- a/extensions/pkg/controller/worker/genericactuator/actuator_reconcile.go
+++ b/extensions/pkg/controller/worker/genericactuator/actuator_reconcile.go
@@ -304,7 +304,8 @@ func deployMachineDeployment(
 		machineDeployment.Spec.AutoPreserveFailedMachineMax = deployment.AutoPreserveFailedMachineMax
 		machineDeployment.Spec.Template = machinev1alpha1.MachineTemplateSpec{
 			ObjectMeta: metav1.ObjectMeta{
-				Labels: getMachineLabels(deployment.Strategy, labels, worker.Name),
+				Labels:      getMachineLabels(deployment.Strategy, labels, worker.Name),
+				Annotations: deployment.Annotations,
 			},
 			Spec: machinev1alpha1.MachineSpec{
 				Class: machinev1alpha1.ClassSpec{
diff --git a/go.mod b/go.mod
index 76f81bb4c2..9c215dd007 100644
--- a/go.mod
+++ b/go.mod
@@ -373,3 +373,5 @@ require (
 	sigs.k8s.io/randfill v1.0.0 // indirect
 	sigs.k8s.io/structured-merge-diff/v6 v6.4.2 // indirect
 )
+
+replace github.com/gardener/machine-controller-manager => ../machine-controller-manager
diff --git a/go.sum b/go.sum
index bd6f6f4334..57cb66fee4 100644
--- a/go.sum
+++ b/go.sum
@@ -270,8 +270,6 @@ github.com/gardener/dependency-watchdog v1.8.0 h1:+3FE8sR1V6YM9JsGqX7m9oUjhr1wXh
 github.com/gardener/dependency-watchdog v1.8.0/go.mod h1:jRIyBZ4ySWy+EdQjfLm/N+90vBPdWJ7khiBN7CGQs18=
 github.com/gardener/etcd-druid/api v0.38.2 h1:ltaqC9PV5rd39kj766UW+ZsN7LputyF1JplgcohsRHE=
 github.com/gardener/etcd-druid/api v0.38.2/go.mod h1:HsqEM/h4XuxPC4cFDm20V9TPAUqv0tjtnzds/nZ5p7Y=
-github.com/gardener/machine-controller-manager v0.62.1 h1:TQ4Qs9aaYTqSkqFownIaEAvnrYoNpBMcp7ZMYVEgUWM=
-github.com/gardener/machine-controller-manager v0.62.1/go.mod h1:ICsymXK4VHm7NVr8Mpl6USvYcTZjUNSk6+XaVu/dd74=
 github.com/gardener/pvc-autoscaler v0.3.0 h1:vneqFW7WESecbucng0Ravuz825BzKLI3jSKmiopccrs=
 github.com/gardener/pvc-autoscaler v0.3.0/go.mod h1:GVA7q8qiMi2UD/vcCBVbHEUijZvxVqZ1SGqiwKGHTE4=
 github.com/gardener/terminal-controller-manager v0.39.0 h1:w+aKFvM5k5EobILX9vrxgs6VbA5gd88Gyx/3a5m7nVc=

In local gardener setup:

SKAFFOLD_CACHE_ARTIFACTS=false make kind-up && SKAFFOLD_CACHE_ARTIFACTS=false make gardener-up && KUBECONFIG=dev-setup/kubeconfigs/virtual-garden/kubeconfig kubectl apply -f example/provider-local/shoot.yaml

(This uses MCM checked out to this branch. Make sure skaffold is not using the cached MCM local provider image)

KUBECONFIG=dev-setup/kubeconfigs/virtual-garden/kubeconfig kubectl annotate shoot/local -n garden-local gardener.cloud/operation=rollout-workers=local
# Do the following for the machines in the MCD to let termination proceed.
k annotate --overwrite -n shoot--local--local machine/shoot--local--local-local-z1-<machine> suspend-instance-deletion.node.machine.sapcloud.io/etcd-member-removal-

Release note:

Add support for suspending VM deletion using suspend-instance-deletion.node.machine.sapcloud.io annotations.

Signed-off-by: Rinish Sam <rinish.sam@sap.com>

gardener-prow · 2026-09-22T03:36:38Z

[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 assign aaronfern for approval. 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

CaptainIRS · 2026-09-22T03:37:27Z

/kind enhancement

takoverflow

Thanks for the PR, just took a cursory glance and had some initial comments PTAL

Signed-off-by: Rinish Sam <rinish.sam@sap.com>

…ables Signed-off-by: Rinish Sam <rinish.sam@sap.com>

…ad of ShortRetry Signed-off-by: Rinish Sam <rinish.sam@sap.com>

Signed-off-by: Rinish Sam <rinish.sam@sap.com>

takoverflow · 2026-09-22T11:10:42Z

Can you add an intergration test for this annotation as well
At

machine-controller-manager/pkg/test/integration/common/framework.go

Line 701 in 1a7592c

ginkgo.Context("deletion", func() {

Which tests machine deletion for a machine with this annotation, waits for condition to be added, then the annotation is removed by the test to allow VM deletion to proceed.

Signed-off-by: Rinish Sam <rinish.sam@sap.com>

CaptainIRS · 2026-09-23T04:02:23Z

I've added integration tests, I've also verified with simulated provider. PTAL, thanks.

logs

Machine controllers test machine resource deletion when machines available should suspend deletion until the suspension annotation is removed
/Users/whatever/go/src/github.com/gardener/machine-controller-manager/pkg/test/integration/common/framework.go:741
  > Enter [BeforeEach] Machine controllers test @ 09/23/26 09:20:50.389
  STEP: Checking machineController process is running @ 09/23/26 09:20:50.389
  STEP: Checking machineControllerManager process is running @ 09/23/26 09:20:50.389
  STEP: Checking nodes in target cluster are healthy @ 09/23/26 09:20:50.389
  < Exit [BeforeEach] Machine controllers test @ 09/23/26 09:20:50.394 (5ms)
  > Enter [It] should suspend deletion until the suspension annotation is removed @ 09/23/26 09:20:50.394
  STEP: Creating a machine for the suspension test @ 09/23/26 09:20:50.394
  STEP: Adding the instance deletion suspension annotation @ 09/23/26 09:20:56.421
  STEP: Deleting the machine @ 09/23/26 09:20:56.434
  STEP: Waiting for InstanceDeletionSuspended=True @ 09/23/26 09:20:56.442
  STEP: Removing the instance deletion suspension annotation @ 09/23/26 09:21:08.473
  STEP: Waiting until the machine object is deleted @ 09/23/26 09:21:08.487
  < Exit [It] should suspend deletion until the suspension annotation is removed @ 09/23/26 09:21:24.529 (34.135s)
• [34.140 seconds]

takoverflow

/lgtm

Thanks for the PR!

gardener-prow · 2026-09-23T07:53:40Z

LGTM label has been added.

Details

Git tree hash: c39d6cece40af10e51cd05485a14f10b602dbcef

Signed-off-by: Rinish Sam <rinish.sam@sap.com>

…exceeding the max length Signed-off-by: Rinish Sam <rinish.sam@sap.com>

aaronfern

Last nit, otherwise lgtm

aaronfern · 2026-09-23T10:18:56Z

  - Long term: Please set more appropriate PDBs which allow disruption of at least one pod.
 - Expired cloud credentials can block the deletion of the machine from infrastructure.
 - Cloud provider can't delete the machine due to internal errors. Such situations are best debugged by using cloud provider specific CLI or cloud console.
+- A controller may intentionally suspend VM deletion by adding an annotation such as `suspend-instance-deletion.node.machine.sapcloud.io/my-reason: my-controller`. Check the `Machine`'s annotations and `.status.conditions` for `type: InstanceDeletionSuspended` with `status: "True"`; remove the suspension annotation once the controller's work is complete so MCM can continue deletion.


nit: just to make it clear that my-reason isn't actually part of the annotation

Suggested change

- A controller may intentionally suspend VM deletion by adding an annotation such as `suspend-instance-deletion.node.machine.sapcloud.io/my-reason: my-controller`. Check the `Machine`'s annotations and `.status.conditions` for `type: InstanceDeletionSuspended` with `status: "True"`; remove the suspension annotation once the controller's work is complete so MCM can continue deletion.

- A controller may intentionally suspend VM deletion by adding an annotation such as `suspend-instance-deletion.node.machine.sapcloud.io/<my-reason>: <my-controller>`. Check the `Machine`'s annotations and `.status.conditions` for `type: InstanceDeletionSuspended` with `status: "True"`; remove the suspension annotation once the controller's work is complete so MCM can continue deletion.

Sure, fixed in 4e95259.

Signed-off-by: Rinish Sam <rinish.sam@sap.com>

aaronfern

Thanks for the PR and for making the changes!
/lgtm

gardener-prow · 2026-09-23T10:46:52Z

LGTM label has been added.

Details

Git tree hash: 4cacb8c13a5ff82b04bebce9559f684ff4e06adf

gagan16k

Thanks for the PR!

gagan16k · 2026-09-23T10:07:50Z

+					Expect(actualCondition).ToNot(BeNil())
+					Expect(actualCondition.Type).To(Equal(v1alpha1.InstanceDeletionSuspended))
+					Expect(actualCondition.Status).To(Equal(corev1.ConditionTrue))
+					Expect(actualCondition.Message).To(Equal("Instance Deletion suspended by my-controller for etcd-member-removal."))


Could we abstract my-controller and etcd-member-removal as consts, so that any future tests can use it instead of a string?

gagan16k · 2026-09-23T10:12:54Z

+	}
+
+	var suspensions []instanceDeletionSuspension
+	if owner, exists := machine.Annotations[v1alpha1.AnnotationKeySuspendInstanceDeletionPrefix]; exists {


Could we have validation for owner to be a non empty string? Or else we can get malformed messages like "Instance Deletion suspended by for xxx-reason."

gagan16k · 2026-09-23T10:17:35Z

+func IsInstanceDeletionSuspended(machine *v1alpha1.Machine) (string, bool) {
+	suspensions := getInstanceDeletionSuspensions(machine)
+	if len(suspensions) == 0 {
+		return "", false


Since we return false only when the returned string is "", do we need the boolean return value?

gagan16k · 2026-09-23T10:36:37Z

 	}
+	oldSuspensionMessage, oldSuspended := annotationsutils.IsInstanceDeletionSuspended(oldMachine)
+	newSuspensionMessage, newSuspended := annotationsutils.IsInstanceDeletionSuspended(newMachine)
+	if (oldSuspended != newSuspended || oldSuspensionMessage != newSuspensionMessage) && c.shouldMachineBeMovedToTerminatingQueue(newMachine) {


Can we gate annotation checks on c.shouldMachineBeMovedToTerminatingQueue(newMachine) so that we do not have to do all these things for every machine update?
Future pre delete hooks(if any) can also reuse this block to run their checks only in the case the machine is marked for termination.

Add support for hook to suspend VM deletion

76dd161

Signed-off-by: Rinish Sam <rinish.sam@sap.com>

CaptainIRS requested a review from a team as a code owner September 22, 2026 03:36

gardener-prow Bot added do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 22, 2026

gardener-prow Bot added kind/enhancement Enhancement, improvement, extension and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Sep 22, 2026

takoverflow requested changes Sep 22, 2026

View reviewed changes

takoverflow reviewed Sep 22, 2026

View reviewed changes

Comment thread pkg/apis/machine/v1alpha1/constants.go Outdated

Comment thread pkg/apis/machine/v1alpha1/constants.go Outdated

Comment thread pkg/util/annotations/annotations_test.go Outdated

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

thiyyakat reviewed Sep 22, 2026

View reviewed changes

Comment thread pkg/util/annotations/annotations.go Outdated

gardener-prow Bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 22, 2026

Address review comments from Prashant

eace893

Signed-off-by: Rinish Sam <rinish.sam@sap.com>

CaptainIRS force-pushed the suspend-deletion branch from 3498b20 to eace893 Compare September 22, 2026 08:45

CaptainIRS added 2 commits September 22, 2026 14:19

Address review comments from Prashant: drop condition prefix for vari…

d32f452

…ables Signed-off-by: Rinish Sam <rinish.sam@sap.com>

Address review comments from Prashant: enqueue on updateMachine inste…

2785e0d

…ad of ShortRetry Signed-off-by: Rinish Sam <rinish.sam@sap.com>

CaptainIRS requested a review from takoverflow September 22, 2026 10:19

takoverflow reviewed Sep 22, 2026

View reviewed changes

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

Address review comments from Prashant: remove unnecessary bool

da38a58

Signed-off-by: Rinish Sam <rinish.sam@sap.com>

Add integration tests

fe2b2b0

Signed-off-by: Rinish Sam <rinish.sam@sap.com>

CaptainIRS requested a review from takoverflow September 23, 2026 04:06

aaronfern reviewed Sep 23, 2026

View reviewed changes

Comment thread pkg/apis/machine/v1alpha1/constants.go Outdated

Comment thread pkg/util/annotations/annotations.go

Comment thread pkg/test/integration/common/framework.go Outdated

takoverflow reviewed Sep 23, 2026

View reviewed changes

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

gagan16k mentioned this pull request Sep 23, 2026

Add an integration test for node deletion #1126

Merged

Address review comments from Aaron

f77fe6d

Signed-off-by: Rinish Sam <rinish.sam@sap.com>

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

CaptainIRS requested a review from aaronfern September 23, 2026 08:47

takoverflow mentioned this pull request Sep 23, 2026

Bug fixes and address integration test flakes #1154

Open

aaronfern reviewed Sep 23, 2026

View reviewed changes

Comment thread pkg/util/annotations/annotations.go Outdated

Address review comments from Aaron: fix handling of first annotation …

222befa

…exceeding the max length Signed-off-by: Rinish Sam <rinish.sam@sap.com>

CaptainIRS requested a review from aaronfern September 23, 2026 10:24

aaronfern reviewed Sep 23, 2026

View reviewed changes

Address review comments from Aaron: fix doc

4e95259

Signed-off-by: Rinish Sam <rinish.sam@sap.com>

CaptainIRS requested a review from aaronfern September 23, 2026 10:46

aaronfern reviewed Sep 23, 2026

View reviewed changes

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

gagan16k reviewed Sep 23, 2026

View reviewed changes

	- A controller may intentionally suspend VM deletion by adding an annotation such as `suspend-instance-deletion.node.machine.sapcloud.io/my-reason: my-controller`. Check the `Machine`'s annotations and `.status.conditions` for `type: InstanceDeletionSuspended` with `status: "True"`; remove the suspension annotation once the controller's work is complete so MCM can continue deletion.
	- A controller may intentionally suspend VM deletion by adding an annotation such as `suspend-instance-deletion.node.machine.sapcloud.io/<my-reason>: <my-controller>`. Check the `Machine`'s annotations and `.status.conditions` for `type: InstanceDeletionSuspended` with `status: "True"`; remove the suspension annotation once the controller's work is complete so MCM can continue deletion.

Conversation

CaptainIRS commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gardener-prow Bot commented Sep 22, 2026

Uh oh!

CaptainIRS commented Sep 22, 2026

Uh oh!

takoverflow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

takoverflow commented Sep 22, 2026

Uh oh!

CaptainIRS commented Sep 23, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

takoverflow left a comment

Choose a reason for hiding this comment

Uh oh!

gardener-prow Bot commented Sep 23, 2026

Uh oh!

Uh oh!

aaronfern left a comment

Choose a reason for hiding this comment

Uh oh!

aaronfern Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

CaptainIRS Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

aaronfern left a comment

Choose a reason for hiding this comment

Uh oh!

gardener-prow Bot commented Sep 23, 2026

Uh oh!

gagan16k left a comment

Choose a reason for hiding this comment

Uh oh!

gagan16k Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

gagan16k Sep 23, 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!

gagan16k Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

gagan16k Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CaptainIRS commented Sep 22, 2026 •

edited

Loading

gagan16k Sep 23, 2026 •

edited

Loading