Merge pull request #99 from vatesfr/fix/oos-taint-vm-shutting-down

fix(node-out-of-service): taint node when VM is shutting down

Author: gCyrille

Makefile

@@ -71,7 +71,7 @@ build: ## Build
 
 .PHONY: run
 run: build ## Run
-	./bin/xenorchestra-cloud-controller-manager-$(ARCH) --v=5 --kubeconfig=$(KUBECONFIG) --cloud-config=xo-config.yaml --controllers=cloud-node,cloud-node-lifecycle,cloud-node-label-sync \
+	./bin/xenorchestra-cloud-controller-manager-$(ARCH) --v=5 --kubeconfig=$(KUBECONFIG) --cloud-config=xo-config.yaml --controllers=cloud-node,cloud-node-lifecycle,cloud-node-label-sync,cloud-node-out-of-service \
 		--use-service-account-credentials --leader-elect=false --bind-address=127.0.0.1 --authorization-always-allow-paths=/healthz,/livez,/readyz,/metrics
 
 .PHONY: lint

docs/node-out-of-service.md

@@ -2,7 +2,8 @@
 
 The `cloud-node-out-of-service` controller applies the
 `node.kubernetes.io/out-of-service=nodeshutdown:NoExecute` taint to nodes whose
-Xen Orchestra VM is no longer running. This is the Kubernetes
+Xen Orchestra VM is no longer running, or is currently shutting down
+(suspend, clean or hard shutdown in progress). This is the Kubernetes
 [Non-Graceful Node Shutdown](https://kubernetes.io/docs/concepts/cluster-administration/node-shutdown/#non-graceful-node-shutdown)
 procedure, implemented by the CCM because it owns the cloud instance state.
 
@@ -18,6 +19,15 @@ error`, so a simple pod restart takes 8 to 10 minutes.
 The 6 minute timer itself is not configurable. The supported way to bypass it is
 the out-of-service taint: KCM then detaches the volume immediately.
 
+The same wait applies to a VM that is being deliberately shut down: a clean
+shutdown or suspend started from Xen Orchestra stops the guest without draining
+the node, but the VM's power state stays `Running` for the whole duration of the
+OS shutdown. The controller therefore also looks at the in-flight XAPI
+operations of the VM and treats a running VM that has a `suspend`,
+`clean-shutdown` or `hard-shutdown` operation in progress as down, so the grace
+period starts as soon as the shutdown begins instead of after the power state
+flips.
+
 ## Who reads the taint
 
 Only the **kube-controller-manager of the workload cluster** reads it, in three
@@ -50,19 +60,22 @@ Every `--node-out-of-service-sync-period` (default `10s`) it lists the nodes and
    initialized yet (`node.cloudprovider.kubernetes.io/uninitialized`);
 2. **fast path**: skips Ready nodes that do not carry the taint, so a healthy
    cluster makes no Xen Orchestra API call;
-3. queries Xen Orchestra with `InstancesV2.InstanceExists` and
-   `InstanceShutdown`;
-4. applies the taint when the VM is down **and** the node is `NotReady`:
+3. queries Xen Orchestra with `GetInstance`, which returns the VM's power state
+   and in-flight operations;
+4. applies the taint when the VM is down or shutting down **and** the node is
+   `NotReady`:
    * the VM was deleted from Xen Orchestra → immediate, it can never come back;
-   * the VM is only powered off → only after
-     `--node-out-of-service-grace-period` (default `30s`), because a clean reboot
-     must not trigger a force detach;
+   * the VM is not running (halted, paused or suspended), or a `suspend`,
+     `clean-shutdown` or `hard-shutdown` operation is in progress → only after
+     `--node-out-of-service-grace-period` (default `30s`), because a clean
+     reboot (no operation in flight) must not trigger a force detach;
 5. removes the taint once the VM runs again and the node is `Ready`.
 
 Applying the taint to a node that is not actually shut down can corrupt a
-filesystem, so the two conditions (VM down *and* node `NotReady`) and the grace
-period are required. See the upstream warning in the Node Shutdowns
-documentation.
+filesystem, so the two conditions (VM down or shutting down *and* node
+`NotReady`) and the grace period are required. The grace period also covers a
+shutdown operation that is cancelled before it completes. See the upstream
+warning in the Node Shutdowns documentation.
 
 ## Enabling the controller
 
@@ -75,7 +88,7 @@ the `docs/deploy` manifests enable it by default.
 | flag | default | description |
 |---|---|---|
 | `--node-out-of-service-sync-period` | `10s` | reconciliation period |
-| `--node-out-of-service-grace-period` | `30s` | how long a powered-off VM must stay down (with a NotReady node) before tainting |
+| `--node-out-of-service-grace-period` | `30s` | how long a VM must stay down or have a shutdown operation in progress (with a NotReady node) before tainting |
 
 With the Helm chart these are set with the `nodeOutOfServiceSyncPeriod` and
 `nodeOutOfServiceGracePeriod` values (empty by default, which keeps the built-in

go.mod

@@ -6,8 +6,8 @@ require (
 	github.com/gofrs/uuid v4.4.0+incompatible
 	github.com/jarcoal/httpmock v1.4.1
 	github.com/spf13/pflag v1.0.10
-	github.com/stretchr/testify v1.12.0
-	github.com/vatesfr/xenorchestra-go-sdk v1.19.0
+	github.com/stretchr/testify v1.12.1
+	github.com/vatesfr/xenorchestra-go-sdk v1.20.0
 	github.com/vatesfr/xenorchestra-k8s-common v0.3.0
 	go.uber.org/mock v0.6.0
 	k8s.io/api v0.36.3
@@ -65,7 +65,7 @@ require (
 	github.com/prometheus/client_model v0.6.2 // indirect
 	github.com/prometheus/common v0.67.5 // indirect
 	github.com/prometheus/procfs v0.19.2 // indirect
-	github.com/sourcegraph/jsonrpc2 v0.2.2 // indirect
+	github.com/sourcegraph/jsonrpc2 v0.2.3 // indirect
 	github.com/spf13/cobra v1.10.2 // indirect
 	github.com/stoewer/go-strcase v1.3.0 // indirect
 	github.com/subosito/gotenv v1.6.0 // indirect
@@ -86,7 +86,7 @@ require (
 	go.uber.org/multierr v1.11.0 // indirect
 	go.uber.org/zap v1.28.0 // indirect
 	go.yaml.in/yaml/v2 v2.4.3 // indirect
-	go.yaml.in/yaml/v3 v3.0.4 // indirect
+	go.yaml.in/yaml/v3 v3.0.5 // indirect
 	golang.org/x/crypto v0.47.0 // indirect
 	golang.org/x/exp v0.0.0-20251219203646-944ab1f22d93 // indirect
 	golang.org/x/net v0.49.0 // indirect

go.sum

@@ -71,7 +71,7 @@ github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX
 github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg=
 github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0=
 github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
-github.com/gorilla/websocket v1.4.1/go.mod h1:YR8l580nyteQvAITg2hZ9XVh4b55+EU/adAjf1fMHhE=
+github.com/gorilla/websocket v1.5.3/go.mod h1:YR8l580nyteQvAITg2hZ9XVh4b55+EU/adAjf1fMHhE=
 github.com/gorilla/websocket v1.5.4-0.20250319132907-e064f32e3674 h1:JeSE6pjso5THxAzdVpqr6/geYxZytqFMBCOtn/ujyeo=
 github.com/gorilla/websocket v1.5.4-0.20250319132907-e064f32e3674/go.mod h1:r4w70xmWCQKmi1ONH4KIaBptdivuRPyosB9RmPlGEwA=
 github.com/grpc-ecosystem/go-grpc-middleware/providers/prometheus v1.1.0 h1:QGLs/O40yoNK9vmy4rhUGBVyMf1lISBGtXRpsu/Qu/o=
@@ -134,8 +134,8 @@ github.com/sirupsen/logrus v1.9.3 h1:dueUQJ1C2q9oE3F7wvmSGAaVtTmUizReu6fjN8uqzbQ
 github.com/sirupsen/logrus v1.9.3/go.mod h1:naHLuLoDiP4jHNo9R0sCBMtWGeIprob74mVsIT4qYEQ=
 github.com/soheilhy/cmux v0.1.5 h1:jjzc5WVemNEDTLwv9tlmemhC73tI08BNOIGwBOo10Js=
 github.com/soheilhy/cmux v0.1.5/go.mod h1:T7TcVDs9LWfQgPlPsdngu6I6QIoyIFZDDC6sNE1GqG0=
-github.com/sourcegraph/jsonrpc2 v0.2.2 h1:fCyU80iidEwcF9kWaj4ylOO1h8pT8P8sFGv8EKemLaw=
-github.com/sourcegraph/jsonrpc2 v0.2.2/go.mod h1:ZafdZgk/axhT1cvZAPOhw+95nz2I/Ra5qMlU4gTRwIo=
+github.com/sourcegraph/jsonrpc2 v0.2.3 h1:0VYp5WZ2irQvRK8OKKxZAbbplj6Pmda07MXm+Mdz3GQ=
+github.com/sourcegraph/jsonrpc2 v0.2.3/go.mod h1:0jBvyko0tdLpe6ntNSF/mvFalb/RPRXcmogoV47cEWc=
 github.com/spf13/cobra v1.10.2 h1:DMTTonx5m65Ic0GOoRY2c16WCbHxOOw6xxezuLaBpcU=
 github.com/spf13/cobra v1.10.2/go.mod h1:7C1pvHqHw5A4vrJfjNwvOdzYu0Gml16OCs2GRiTUUS4=
 github.com/spf13/pflag v1.0.9/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg=
@@ -152,14 +152,14 @@ github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UV
 github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
 github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU=
 github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4=
-github.com/stretchr/testify v1.12.0 h1:K6Mr6jO9JICuend/5xzTM03ydSV3vdNRYAdPSukj8uI=
-github.com/stretchr/testify v1.12.0/go.mod h1:bOYBZb5qJ00vPzWfIqBUZPaxK8jWiXc6d3ErP4Ca9Gw=
+github.com/stretchr/testify v1.12.1 h1:EuwCh5fleGS7H32xRwO3wRGT7DxrDhLAT6FF8MpWDWE=
+github.com/stretchr/testify v1.12.1/go.mod h1:MDEgiDPPsNp5cuIrHPPCyornHKgEVbtFUmoNlxoYthg=
 github.com/subosito/gotenv v1.6.0 h1:9NlTDc1FTs4qu0DDq7AEtTPNw6SVm7uBMsUCUjABIf8=
 github.com/subosito/gotenv v1.6.0/go.mod h1:Dk4QP5c2W3ibzajGcXpNraDfq2IrhjMIvMSWPKKo0FU=
 github.com/tmc/grpc-websocket-proxy v0.0.0-20220101234140-673ab2c3ae75 h1:6fotK7otjonDflCTK0BCfls4SPy3NcCVb5dqqmbRknE=
 github.com/tmc/grpc-websocket-proxy v0.0.0-20220101234140-673ab2c3ae75/go.mod h1:KO6IkyS8Y3j8OdNO85qEYBsRPuteD+YciPomcXdrMnk=
-github.com/vatesfr/xenorchestra-go-sdk v1.19.0 h1:QGFi9h6CioQFeNOLOmPfGiBt84uT1hDG/PtYkSZFsMA=
-github.com/vatesfr/xenorchestra-go-sdk v1.19.0/go.mod h1:lkAhxh9gLFZNLIubuMcjDKP49G2/H/h6wfWA3nWpO1o=
+github.com/vatesfr/xenorchestra-go-sdk v1.20.0 h1:zLNv4l8YhRnwamrXlcC65URJ7cfSh08wCHs6aTht2kc=
+github.com/vatesfr/xenorchestra-go-sdk v1.20.0/go.mod h1:pOG7n9YM9sxJc4EyE/uPf8JbMi/mEHkVLkQAzhL0jbM=
 github.com/vatesfr/xenorchestra-k8s-common v0.3.0 h1:nIALjBoPmtbQei8SezQCj4jNQ2umFiTuFw8cmwj4GFg=
 github.com/vatesfr/xenorchestra-k8s-common v0.3.0/go.mod h1:nBjpo85ypPajWO4zpDKAP6/k58y88zjGX445N/wzLDU=
 github.com/x448/float16 v0.8.4 h1:qLwI1I70+NjRFUR3zs1JPUCgaCXSh3SW62uAKT1mSBM=
@@ -214,8 +214,9 @@ go.uber.org/zap v1.28.0 h1:IZzaP1Fv73/T/pBMLk4VutPl36uNC+OSUh3JLG3FIjo=
 go.uber.org/zap v1.28.0/go.mod h1:rDLpOi171uODNm/mxFcuYWxDsqWSAVkFdX4XojSKg/Q=
 go.yaml.in/yaml/v2 v2.4.3 h1:6gvOSjQoTB3vt1l+CU+tSyi/HOjfOjRLJ4YwYZGwRO0=
 go.yaml.in/yaml/v2 v2.4.3/go.mod h1:zSxWcmIDjOzPXpjlTTbAsKokqkDNAVtZO0WOMiT90s8=
-go.yaml.in/yaml/v3 v3.0.4 h1:tfq32ie2Jv2UxXFdLJdh3jXuOzWiL1fo0bu/FbuKpbc=
 go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg=
+go.yaml.in/yaml/v3 v3.0.5 h1:N6y/pJk8buWs9NY5ERU2HSMfm+IuD/OtfdAnq6kESPw=
+go.yaml.in/yaml/v3 v3.0.5/go.mod h1:HVTZu1O7/Vkt2N+BFy8Zza+lnLsABggaTM2ZpNIGuKg=
 golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
 golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
 golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=

pkg/controllers/nodeoutofservice/node_out_of_service_controller.go

@@ -59,9 +59,9 @@ const (
 var (
 	// SyncPeriod is how often the controller watches all nodes.
 	SyncPeriod = DefaultSyncPeriod
-	// GracePeriod is how long a powered-off VM must stay down while its node is
-	// NotReady before the out-of-service taint is applied. A missing VM (deleted
-	// from Xen Orchestra) is tainted immediately.
+	// GracePeriod is how long a VM must be down or shutting down while its node
+	// is NotReady before the out-of-service taint is applied. A missing VM
+	// (deleted from Xen Orchestra) is tainted immediately.
 	GracePeriod = DefaultGracePeriod
 )
 
@@ -84,8 +84,9 @@ type Controller struct {
 	i           xenorchestra.XOInstances
 	gracePeriod time.Duration
 
-	// firstObserved records, per node UID, when the "VM down and node NotReady"
-	// condition was first observed. Only touched by the single sync goroutine.
+	// firstObserved records, per node UID, when the "VM down or shutting down
+	// and node NotReady" condition was first observed. Only touched by the
+	// single sync goroutine.
 	firstObserved map[string]time.Time
 }
 
@@ -224,9 +225,9 @@ func (c *Controller) SyncNodes(ctx context.Context) error {
 			instanceState = vm.PowerState
 			instanceRunning = vm.PowerState == payloads.PowerStateRunning
 		}
-		instanceDown := !instanceRunning
+		instanceDownOrShuttingDown := !instanceRunning || (!instanceMissing && vm.IsShuttingDown())
 
-		if instanceDown && !nodeReady {
+		if instanceDownOrShuttingDown && !nodeReady {
 			if _, seen := c.firstObserved[key]; !seen {
 				c.firstObserved[key] = now
 			}
@@ -235,8 +236,8 @@ func (c *Controller) SyncNodes(ctx context.Context) error {
 		}
 
 		switch {
-		case shouldApplyOutOfServiceTaint(node, instanceDown, !instanceMissing, nodeReady, c.firstObserved[key], now, c.gracePeriod):
-			klog.InfoS("Applying out-of-service taint: VM is not running and node is not Ready",
+		case shouldApplyOutOfServiceTaint(node, instanceDownOrShuttingDown, !instanceMissing, nodeReady, c.firstObserved[key], now, c.gracePeriod):
+			klog.InfoS("Applying out-of-service taint: VM is down or shutting down and node is not Ready",
 				"node", klog.KObj(node), "instanceState", instanceState)
 			if err := addOutOfServiceTaint(c.kubeClient, node); err != nil {
 				klog.ErrorS(err, "Failed to apply out-of-service taint", "node", klog.KObj(node))

pkg/controllers/nodeoutofservice/node_out_of_service_controller_test.go

@@ -122,6 +122,24 @@ func TestSyncNodesDoesNotTaintRunningNode(t *testing.T) {
 	assert.False(t, hasOutOfServiceTaint(getNode(t, client, node.Name)))
 }
 
+func TestSyncNodesAppliesTaintAfterGraceWhenShutdownOperationIsInProgress(t *testing.T) {
+	ctx := context.Background()
+	node := testNode(notReady)
+	inst := vmWithState(payloads.PowerStateRunning)
+	inst.vm.CurrentOperations = map[string]payloads.VMOperation{
+		"operation-id": payloads.VMOperationSuspend,
+	}
+	c, client := newTestController(t, node, inst, time.Minute)
+
+	require.NoError(t, c.SyncNodes(ctx))
+	assert.False(t, hasOutOfServiceTaint(getNode(t, client, node.Name)))
+	assert.Contains(t, c.firstObserved, string(node.UID))
+
+	c.firstObserved[string(node.UID)] = time.Now().Add(-2 * time.Minute)
+	require.NoError(t, c.SyncNodes(ctx))
+	assert.True(t, hasOutOfServiceTaint(getNode(t, client, node.Name)))
+}
+
 func TestSyncNodesDoesNotTaintReadyNode(t *testing.T) {
 	ctx := context.Background()
 	node := testNode() // Ready