feat: add storage node drain-remove workflow by geoffrey1330 · Pull Request #294 · simplyblock/simplyblock-operator

geoffrey1330 · 2026-07-01T09:18:13Z

Summary

Implements Issue #131 — safe storage node removal with a non-blocking, restartable drain workflow.

Previously action: remove expected the node to already be offline. This PR extends the remove action with a multi-step drain phase driven by the existing StorageNodeSetReconciler, so all logical volumes are migrated before the node is shut down.

Changes

New: drain-remove state machine (`simplyblockstoragenodeset_drain.go`)

The reconciler advances through six sub-phases on each reconcile invocation — no blocking loops:

Sub-phase	What happens
Validating	Classifies all volumes on the node (PV-managed, pinned, system, unmanaged). Blocks with a `Warning` event if pinned or unmanaged volumes are found. Node is untouched until validation passes.
Suspending	POSTs `suspend`, persists `Triggered=true` to guard against duplicate calls across restarts, polls until `status == suspended`.
Migrating	Creates one `VolumeMigration` CR per PV-managed volume. Watches owned CRs via `.Owns(&VolumeMigration{})`. Updates `volumesMigrated`/`volumesPending` counters in `ActionStatus`.
Verifying	Lists volumes remaining on the node, filters system volumes by regex. Requeues until empty.
Removing	Calls the backend remove API.
Cancellation	If the user clears `spec.action` mid-drain, the node is automatically resumed before `ActionStatus` is cleared.

On any failure after Suspending, resumeAndFail performs a best-effort resume so the node returns to online.

New spec fields

spec:
  systemVolumeFilterRegex: "^sb-fio-baseline-.*"  # skip benchmark/system volumes (default)

New status fields

status:
  actionStatus:
    subPhase: Migrating           # persisted across operator restarts
    volumesMigrated: 3
    volumesPending: 4

Other changes

NodeActionShutdown/Restart/Suspend/Resume/Remove constants added to utils/constants.go; all string literals in the controller replaced (fixes goconst lint violation)
RBAC annotations added for volumemigrations, persistentvolumes, persistentvolumeclaims
case "remove" in performNodeAction now returns an explicit error — the action is intercepted in reconcileAction before it can reach that path

Test plan

action: remove on a node with PV-managed volumes — verify VolumeMigration CRs are created, node suspends, volumes migrate, node is removed
Node with pinned PVC — verify drain blocks with PinnedVolumeBlocking event and node stays online until annotation is manually removed
Node with unmanaged volume — verify drain blocks with UnmanagedVolumeBlocking event
Clear spec.action mid-drain (during Migrating) — verify node is resumed and returns to online
Operator restart during Migrating sub-phase — verify drain resumes from the correct sub-phase without duplicate migrations
VolumeMigration CR fails — verify node is resumed and actionStatus.state = failed
fio randrw workload running on PVCs during drain — verify no I/O interruption from node suspend through final removal confirmation

Output

  status:
    actionStatus:
      action: remove
      message: node removed successfully
      nodeUUID: 0ac0b63f-8d03-4f99-9593-c24fb5db6111
      state: success
      triggered: true
      updatedAt: "2026-07-02T13:19:09Z"
      volumesMigrated: 6

2026-07-02T14:33:38Z	DEBUG	events	drain blocked: pinned volumes a41c1711-5ad1-4de2-9a08-e3aca1f87e23 must be unpinned before drain	{"type": "Warning", "object": {"kind":"StorageNodeSet","namespace":"simplyblock","name":"simplyblock-node","uid":"0ce68b7f-6f76-4f97-a002-92eb905cb3db","apiVersion":"storage.simplyblock.io/v1alpha1","resourceVersion":"584125"}, "reason": "PinnedVolumeBlocking"}
2026-07-02T14:33:46Z	DEBUG	events	drain blocked: pinned volumes a41c1711-5ad1-4de2-9a08-e3aca1f87e23 must be unpinned before drain	{"type": "Warning", "object": {"kind":"StorageNodeSet","namespace":"simplyblock","name":"simplyblock-node","uid":"0ce68b7f-6f76-4f97-a002-92eb905cb3db","apiVersion":"storage.simplyblock.io/v1alpha1","resourceVersion":"584125"}, "reason": "PinnedVolumeBlocking"}

2026-07-02T15:09:39Z	DEBUG	events	drain blocked: unmanaged volumes 9f73b083-7ce9-4b55-8702-54b50c22afb5 must be removed before drain	{"type": "Warning", "object": {"kind":"StorageNodeSet","namespace":"simplyblock","name":"simplyblock-node","uid":"dc26fade-ed28-4d7a-b8e5-e86ebf1e6913","apiVersion":"storage.simplyblock.io/v1alpha1","resourceVersion":"594289"}, "reason": "UnmanagedVolumeBlocking"}
2026-07-02T15:09:40Z	DEBUG	events	drain blocked: unmanaged volumes 9f73b083-7ce9-4b55-8702-54b50c22afb5 must be removed before drain	{"type": "Warning", "object": {"kind":"StorageNodeSet","namespace":"simplyblock","name":"simplyblock-node","uid":"dc26fade-ed28-4d7a-b8e5-e86ebf1e6913","apiVersion":"storage.simplyblock.io/v1alpha1","resourceVersion":"594289"}, "reason": "UnmanagedVolumeBlocking"}
2026-07-02T15:09:40Z	DEBUG	events	drain blocked: unmanaged volumes 9f73b083-7ce9-4b55-8702-54b50c22afb5 must be removed before drain	{"type": "Warning", "object": {"kind":"StorageNodeSet","namespace":"simplyblock","name":"simplyblock-node","uid":"dc26fade-ed28-4d7a-b8e5-e86ebf1e6913","apiVersion":"storage.simplyblock.io/v1alpha1","resourceVersion":"594291"}, "reason": "UnmanagedVolumeBlocking"}

[INFO] Triggering drain on 0fa4216a-b640-4ea9-980f-500e218bb178 while fio is running
storagenodeset.storage.simplyblock.io/simplyblock-node patched
[INFO] Waiting for drain to complete while monitoring fio pods (timeout: 600s)...
[INFO]   drain: state=running subPhase=Migrating migrated= pending=4
[INFO]   drain: state=running subPhase=Migrating migrated= pending=4
[INFO]   drain: state=running subPhase=Migrating migrated=2 pending=2
[INFO]   drain: state=success subPhase= migrated=4 pending=
[PASS] Test 6: Drain completed successfully under fio load
[INFO] Waiting for node 0fa4216a-b640-4ea9-980f-500e218bb178 to show 'removed' status in StorageNodeSet (timeout: 120s)...
[INFO]   node backend status=in_shutdown
[INFO]   node backend status=removed
[INFO]   fio pods still running after drain: 6/6
[PASS] Test 6: All 6 fio pods still running after drain
[PASS] Test 6: No fio I/O errors detected in pod logs
[PASS] Test 6: fio ran uninterrupted throughout drain
[INFO] Cleaning up drain-remove-test...

════════════════════════════════════════════
 Results: 6 passed, 0 failed
════════════════════════════════════════════

We get event for both blocking condition at the same time

2026-07-07T08:13:12Z	DEBUG	events	drain blocked: pinned volumes 3ac646d8-0a73-48e1-b778-2a08508f0ae5 must be unpinned before drain	{"type": "Warning", "object": {"kind":"StorageNodeSet","namespace":"simplyblock","name":"simplyblock-node","uid":"48be621b-5d61-48f6-997a-11e699916665","apiVersion":"storage.simplyblock.io/v1alpha1","resourceVersion":"787866"}, "reason": "PinnedVolumeBlocking"}
2026-07-07T08:13:12Z	DEBUG	events	drain blocked: unmanaged volumes d851287a-2c6a-44c4-bef8-bf85afd39855 must be removed before drain	{"type": "Warning", "object": {"kind":"StorageNodeSet","namespace":"simplyblock","name":"simplyblock-node","uid":"48be621b-5d61-48f6-997a-11e699916665","apiVersion":"storage.simplyblock.io/v1alpha1","resourceVersion":"787866"}, "reason": "UnmanagedVolumeBlocking"}
2026-07-07T08:13:12Z	DEBUG	events	drain blocked: pinned volumes 3ac646d8-0a73-48e1-b778-2a08508f0ae5 must be unpinned before drain	{"type": "Warning", "object": {"kind":"StorageNodeSet","namespace":"simplyblock","name":"simplyblock-node","uid":"48be621b-5d61-48f6-997a-11e699916665","apiVersion":"storage.simplyblock.io/v1alpha1","resourceVersion":"787867"}, "reason": "PinnedVolumeBlocking"}
2026-07-07T08:13:12Z	DEBUG	events	drain blocked: unmanaged volumes d851287a-2c6a-44c4-bef8-bf85afd39855 must be removed before drain	{"type": "Warning", "object": {"kind":"StorageNodeSet","namespace":"simplyblock","name":"simplyblock-node","uid":"48be621b-5d61-48f6-997a-11e699916665","apiVersion":"storage.simplyblock.io/v1alpha1","resourceVersion":"787867"}, "reason": "UnmanagedVolumeBlocking"}

  Warning  PinnedVolumeBlocking     99s                storagenodeset-controller  drain blocked: pinned volumes 3ac646d8-0a73-48e1-b778-2a08508f0ae5 must be unpinned before drain
  Warning  UnmanagedVolumeBlocking  99s                storagenodeset-controller  drain blocked: unmanaged volumes d851287a-2c6a-44c4-bef8-bf85afd39855 must be removed before drain

…tervention

…g PVs

… drain

…ellation

…edundant calls

…terals

…status

…gnment

…l state

…Migrating phase

noctarius · 2026-07-06T12:55:45Z

I think we need a lot more test scenarios. Testing is not just about testing ones to ensure the basics work. We need more behavior tests on specific code segments and functions. Not only for us to ensure it works today, but also to ensure that stuff isn't breaking tomorrow by a thought-off unrelated change.

Many should be possible as unit tests which just ensure that the code itself behaves correctly now, and fails to fail in the future (with code changes). Some may require integration tests which should be considered e2e tests (as far as possible) or laying out a test concept for @RaunakJalan.

The suspended storage node itself fails mid-draining
The cluster becomes degraded during draining
The cluster falls below FTT threshold during draining
Then after the cluster comes back, it is in rebalancing which needs to pause (?) draining until finished?
No draining target is available
The storage node never reaches suspended (timeout, connection refused, 4xx, 5xx, ...)
General drainVerify failure scenarios
Operator restart during suspending before triggered persisted (idempotency)
Already started VolumeMigration CR before operator restart should not be recreated
Operator restart during storage node removal (idempotency)
Control plane rejects storage node suspension
Control plane rejects storage node removal
Unmanaged volume fails pre-validation
Cluster in rebalancing fails pre-validation
An empty storage node skips migration
A storage node with only system-volumes skips migration
If pinned and unmanaged volumes are found, both failures should be user visible
A PVC's owning pod is in deletion
A PVC's owning pod is not running
Validation that a failed VolumeMigration CR is handled correctly
Rapid action toggling (set/clear/set/clear/...)
The draining target (the storage node uuid which is drained) is changed during a draining operation in the CR

geoffrey1330 · 2026-07-06T14:10:17Z

I think we need a lot more test scenarios. Testing is not just about testing ones to ensure the basics work. We need more behavior tests on specific code segments and functions. Not only for us to ensure it works today, but also to ensure that stuff isn't breaking tomorrow by a thought-off unrelated change.

Many should be possible as unit tests which just ensure that the code itself behaves correctly now, and fails to fail in the future (with code changes). Some may require integration tests which should be considered e2e tests (as far as possible) or laying out a test concept for @RaunakJalan.

The suspended storage node itself fails mid-draining

The cluster becomes degraded during draining

The cluster falls below FTT threshold during draining

Then after the cluster comes back, it is in rebalancing which needs to pause (?) draining until finished?

No draining target is available

The storage node never reaches suspended (timeout, connection refused, 4xx, 5xx, ...)

General drainVerify failure scenarios

Operator restart during suspending before triggered persisted (idempotency)

Already started VolumeMigration CR before operator restart should not be recreated

Operator restart during storage node removal (idempotency)

Control plane rejects storage node suspension

Control plane rejects storage node removal

Unmanaged volume fails pre-validation

Cluster in rebalancing fails pre-validation

An empty storage node skips migration

A storage node with only system-volumes skips migration

If pinned and unmanaged volumes are found, both failures should be user visible

A PVC's owning pod is in deletion

A PVC's owning pod is not running

Validation that a failed VolumeMigration CR is handled correctly

Rapid action toggling (set/clear/set/clear/...)

The draining target (the storage node uuid which is drained) is changed during a draining operation in the CR

I have added unit-test that covers the outlined scenarios

noctarius · 2026-07-06T15:15:25Z

I have added unit-test that covers the outlined scenarios

I see that some off them are implemented as very very basic tests. Like for example that if pinned and unmanaged volumes are found. You added a unit tests to check that both are return from the validation function, but not they both are reconciled into events on the StorageNodeSet which tells the user about both issues. This is what I actually meant. Only the first check (which is if len(pinned) > 0) is reconciled. If there are also unmanaged volumes, they would require the user to run another iteration just to realize it still doesn't work. Bad user experience.

We should validate everything and mention all at once to prevent the user makes circles.

For all the above tests, it's good to test them as soon as possible, but please validate if there are high-up location where they would have impact and we need to ensure the user understands what's happening. Not sure how many really are affected but I saw this specific one since I thought about it specifically as feedback to the user.

noctarius · 2026-07-06T15:16:50Z

Also, what is with the open questions? Are those answered? If so, what are the answers and how are they respected in the design? I typically strike them out with a quick explanation when I answered one and reconcile the answer into the design itself to ensure consistency.

…it_test.go

…le events and status

… deletes

noctarius

There are a few comments like enhanced error handling, a small helper extraction and one or two questions regarding sub-process fails (like a failing volume migration).

noctarius · 2026-07-07T08:34:22Z

+	}
+
+	// All existing CRs are completed — update counters, clean up and advance.
+	if inProgress == 0 && completed == len(vmigList.Items) {


What if a migration fails? Seems like there is no terminal operation for this and it would spin forever since inProgress would be >1 and completed not matching the migration list?

noctarius · 2026-07-07T08:36:25Z

 		body, status, err := apiClient.Do(ctx, http.MethodGet, endpoint, nil)

-		if action == "remove" && status == http.StatusNotFound {
+		if action == utils.NodeActionRemove && status == http.StatusNotFound {


Do we need to reconcile the status StorageNodeSet status field (removing the node) or does it happen automatically? Not sure if we should also update the workers property since it's not true anymore.

status.nodes is refreshed automatically via syncTrackedNodesStatus. We intentionally leave spec.workerNodes unchanged. it's user-declared and the stale status.nodes entry also prevents reconcileWorkerNode from re-provisioning the removed node.

noctarius · 2026-07-07T08:40:09Z

This isn't great error handling. HTTP statuses above 3xx mean different things. Just blindly retrying is wrong here. If a node is already removed the control plane would most probably answer with 409 (conflict) or 404 (not found). A retry would not solve this issue. See the new error classification system in the CSI driver for hints which is retry-able or not (https://github.com/simplyblock/simplyblock-csi/blob/master/pkg/spdk/errorclass.go). I know this isn't directly part of this PR, but since we already hit this part, we should fix it.

I'm happy to extract the error classification already and make it available here, too.

Sure, maybe i can extract and use it as part of my PR

I've extracted the error classification into the operator errorclass.go

noctarius · 2026-07-07T08:45:57Z

+		return ctrl.Result{}, nil
+	}
+
+	// 5xx: transient backend error — retry without resuming the node.


Not every 5xx error is retry-able. See the other comment.

noctarius · 2026-07-07T08:47:10Z

+	}
+
+	// 4xx (other than 404): permanent backend rejection — resume and fail.
+	return r.resumeAndFail(ctx, apiClient, clusterUUID, snCR,


Not every 4xx is a permanent error, like 429 is too many requests.

noctarius · 2026-07-07T08:47:41Z

+	nodeUUID := snCR.Spec.NodeUUID
+
+	resumeEndpoint := fmt.Sprintf("/api/v2/clusters/%s/storage-nodes/%s/resume", clusterUUID, nodeUUID)
+	if _, _, err := apiClient.Do(ctx, http.MethodPost, resumeEndpoint, nil); err != nil {


Why is resume a best effort only?

You're right, if resume fails the node stays suspended which is bad. Fixed — we now retry on transient errors and only give up on permanent ones.

noctarius · 2026-07-07T08:50:48Z

+			Namespace: pv.Spec.ClaimRef.Namespace,
+			Name:      pv.Spec.ClaimRef.Name,
+		}, &pvc); err != nil {
+			log.Error(err, "matchVolumesToPVs: failed to get PVC, treating volume as unmanaged",


I think this is not a perfect solution but it's better than ignoring the error. Maybe it's worth adding to the functions documentation that a failed PV request / mapping will temporarily mark all volumes as unmanaged.

noctarius · 2026-07-07T08:54:52Z

+		}
+	}
+
+	re, reErr := regexp.Compile(filterRegex)


I think this should be precompiled at bootup. That way it will also fast fail when there is an error in the filter expression.

noctarius · 2026-07-07T08:58:07Z

+	clusterUUID string,
+	nodeUUID string,
+) ([]webapi.VolumeInfo, error) {
+	pools, err := apiClient.GetStoragePools(ctx, clusterUUID)


This API call and the following will hammer the control plane every second (drainRequeueImmediate) with at least two calls.

noctarius · 2026-07-07T08:59:47Z

+		return ctrl.Result{RequeueAfter: drainRequeueValidate}, nil
+	}
+
+	filterRegex := simplyblockv1alpha1.DefaultSystemVolumeFilterRegex


This snippet is used at least 4 times. It should be extracted into a helper such as resolveSystemFlter(spec).

…drain flow

…oPVs

…ache custom patterns

… API calls

geoffrey1330 added 19 commits July 1, 2026 10:17

feat: add storage node drain-remove workflow

7151c48

refactor: replace node action string literals with constants

0b7af84

fix: update tests for remove action routing to drain workflow

a9dbe0b

feat: remove UnpinBeforeDrain field, pinned volumes require manual in…

46533be

…tervention

refactor: merge storagenodeset constants into a single block

0bf3d24

fix: use pool volumes API to list node volumes during drain

6c25e6b

fix: parse clusterUUID:poolUUID:volumeUUID handle format when matchin…

142fac7

…g PVs

fix: pick online target node when creating VolumeMigration CRs during…

ea60cdb

… drain

fix: re-fetch CR before resume to prevent stale cache triggering canc…

8340297

…ellation

fix: guard suspend and cancellation resume against stale cache races

9c36feb

fix: check backend node status before suspend and resume to prevent r…

760053e

…edundant calls

fix: add NodeStatusSuspended constant and replace suspended string li…

2c3debc

…terals

fix: accept 204 No Content as success response from node DELETE endpoint

5861c24

fix: ignore not found error when patching deleted VolumeMigration CR …

259d054

…status

feat: distribute drain migrations across nodes using round-robin assi…

27ca08b

…gnment

fix: skip drain re-initialization when action already reached termina…

664fa7d

…l state

fix: update migration counters to final values before advancing from …

8764c66

…Migrating phase

feat: block drain if cluster is rebalancing during validation

1af90b7

added drain test script

f54f1ce

geoffrey1330 requested review from boddumanohar, mxsrc and noctarius and removed request for boddumanohar and noctarius July 6, 2026 11:49

geoffrey1330 added 3 commits July 6, 2026 14:10

updated storage-node daemonset hostpath to var-run-simplyblock

85445ed

feat: add enum validation to ActionStatus.SubPhase field

a0f41d4

test: add drain-remove unit tests and test plan document

beb5ebc

fix: resolve golangci-lint errors in drain unit tests

ce9b98c

geoffrey1330 added 10 commits July 6, 2026 16:24

fix: surface all drain validation blockers in a single reconcile

a0e5b42

formatted file internal/controller/simplyblockstoragenodeset_drain_un…

209f8a4

…it_test.go

test: add reconciler-level drain validation tests covering user-visib…

0bf101a

…le events and status

fix: suppress unparam lint warnings on test helper functions

3068e7d

fix: delete in-flight VolumeMigration CRs on drain cancellation

4284ec8

fix: retry drain node removal on transient network and 5xx errors

f99e763

feat: delete system volumes in drainVerify before node removal

2438b58

fix: skip in_deletion volumes in listNodeVolumes to prevent duplicate…

8755237

… deletes

added doc design-node-removal-draining.md

e0a920b

updated test script test_drain_remove.sh

ee6b7ac

noctarius reviewed Jul 7, 2026

View reviewed changes

geoffrey1330 added 4 commits July 7, 2026 10:25

feat: extract HTTP error classification from CSI driver and apply to …

fe74da2

…drain flow

docs: document transient PVC fetch failure behaviour in matchVolumesT…

79651f0

…oPVs

refactor: precompile system volume filter regex at package init and c…

ff4d9b5

…ache custom patterns

refactor: fetch pool list once per reconcile in drainVerify to reduce…

295aba3

… API calls

Uh oh!

Conversation

geoffrey1330 commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

New: drain-remove state machine (simplyblockstoragenodeset_drain.go)

New spec fields

New status fields

Other changes

Test plan

Output

Uh oh!

noctarius commented Jul 6, 2026

Uh oh!

geoffrey1330 commented Jul 6, 2026

Uh oh!

noctarius commented Jul 6, 2026

Uh oh!

noctarius commented Jul 6, 2026

Uh oh!

noctarius left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

geoffrey1330 commented Jul 1, 2026 •

edited

Loading

New: drain-remove state machine (`simplyblockstoragenodeset_drain.go`)