Add non-blocking storage-node migration with promote and worker-list reconcile#329
Open
noctarius wants to merge 3 commits into
Open
Add non-blocking storage-node migration with promote and worker-list reconcile#329noctarius wants to merge 3 commits into
noctarius wants to merge 3 commits into
Conversation
…reconcile Wire the operator's restart action to the new control-plane migration APIs and complete the post-migration reconciliation the backend restart alone does not do. - New action-scoped spec field `newSsdPcie`, passed to the restart call as `new_ssd_pcie` (applies only to the node named by `nodeUUID`, like `workerNode`/`reattachVolume`). - Restart/migration now runs as a non-blocking phase machine (reconcileRestartAction) instead of the in-line sleep + poll loop that held the reconcile goroutine for minutes. Each phase does one step and requeues: fire -> Migrating (await online) -> Promoting (POST /promote) -> ReconcilingWorkers -> success. - After the node is online, call the new /promote endpoint (make_sec_new_primary): activates the new-host devices, fails+migrates the removed origin-host devices (starting the rebalance and returning the cluster to active), sets primary, and re-homes lvols. - Reconcile spec.workerNodes on a completed migration: drop the vacated origin host and add the target, so steady-state reconciliation does not re-provision a phantom node on the origin. ObservedGeneration is re-affirmed after the spec edit so the action-idempotency gate still matches. - Preserve the storm guard (never re-fire an in_restart, or an online node this request already fired) and a bounded online/reachability deadline. - Extend the ActionStatus.SubPhase enum (Promoting, ReconcilingWorkers; Migrating shared with the drain flow, disambiguated by Action) and regenerate CRDs/deepcopy. Scope: only restart/migration goes non-blocking; suspend/resume/shutdown keep the existing in-line path. Adds unit tests for the phase machine (non-blocking progression, storm guard, idempotency) and the worker-list reconcile. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the StorageNodeSet controller’s restart/migration action to use a non-blocking, multi-phase reconciliation state machine, wires the restart call to pass the new new_ssd_pcie parameter, and updates CRDs/manifests accordingly so migrations can be promoted and post-migration worker lists can be reconciled.
Changes:
- Replace the inline restart/migration wait loop with
reconcileRestartActionphase machine (fire → Migrating → Promoting → ReconcilingWorkers → success). - Add action-scoped
spec.newSsdPcieand pass it through to the control-plane restart payload asnew_ssd_pcie. - Extend ActionStatus.SubPhase enum in CRDs/manifests and add unit tests for non-blocking restart behavior plus worker-list reconciliation helper.
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| operator/internal/controller/simplyblockstoragenodeset_controller.go | Adds non-blocking restart/migration phase machine, /promote call, and workerNodes reconciliation logic; passes new_ssd_pcie. |
| operator/internal/controller/simplyblockstoragenodeset_restart_migration_test.go | Adds unit tests for non-blocking restart + storm guard and the workerNodes reconciliation helper. |
| operator/api/v1alpha1/storagenodeset_types.go | Adds NewSsdPcie to the CRD Go type and extends SubPhase enum. |
| operator/api/v1alpha1/zz_generated.deepcopy.go | Regenerates deepcopy to include NewSsdPcie. |
| operator/config/crd/bases/storage.simplyblock.io_storagenodesets.yaml | Adds newSsdPcie schema and extends SubPhase enum. |
| operator/config/crd/bases/storage.simplyblock.io_storageclusters.yaml | Extends SubPhase enum documentation/schema. |
| operator/dist/install.yaml | Updates installed CRD schema: SubPhase description/enum and newSsdPcie. |
| helm-charts/charts/simplyblock-operator/crds/storage.simplyblock.io_storagenodesets.yaml | Helm CRD update for newSsdPcie and SubPhase enum. |
| helm-charts/charts/simplyblock-operator/crds/storage.simplyblock.io_storageclusters.yaml | Helm CRD update for SubPhase enum. |
Files not reviewed (1)
- operator/api/v1alpha1/zz_generated.deepcopy.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2070
to
+2077
| fail := func(msg string) (ctrl.Result, error) { | ||
| as.State = utils.ActionStateFailed | ||
| as.Message = msg | ||
| as.UpdatedAt = metav1.Now() | ||
| _ = r.Status().Update(ctx, snCR) | ||
| log.Info("Restart action failed", "nodeUUID", snCR.Spec.NodeUUID, "message", msg) | ||
| return ctrl.Result{}, nil | ||
| } |
Comment on lines
+2152
to
+2164
| case restartSubPhasePromoting: // ---- promote: start rebalance + primary switch ---- | ||
| if err := r.promoteNode(ctx, apiClient, clusterUUID, snCR.Spec.NodeUUID); err != nil { | ||
| log.Error(err, "promote failed; will retry", "nodeUUID", snCR.Spec.NodeUUID) | ||
| return ctrl.Result{RequeueAfter: restartActionRequeue}, nil | ||
| } | ||
| log.Info("Promoted migrated node; rebalance started", | ||
| "nodeUUID", snCR.Spec.NodeUUID, "workerNode", snCR.Spec.WorkerNode) | ||
| as.SubPhase = restartSubPhaseReconcilingWorkers | ||
| as.UpdatedAt = metav1.Now() | ||
| if err := r.Status().Update(ctx, snCR); err != nil { | ||
| return ctrl.Result{}, err | ||
| } | ||
| return ctrl.Result{RequeueAfter: restartPromoteRequeue}, nil |
Comment on lines
+2255
to
+2258
| // reconcileMigratedWorkerList mutates snCR.Spec.WorkerNodes in place to reflect a | ||
| // completed migration: the target workerNode is added (if absent) and the vacated | ||
| // sourceHost is removed (when known and distinct from the target). Returns true when | ||
| // the list changed. |
Comment on lines
+80
to
+106
| // A plain restart (no workerNode) drives fire -> Migrating -> success without | ||
| // blocking, firing exactly one /restart and no /promote. | ||
| func TestReconcileRestartAction_PlainRestartNonBlocking(t *testing.T) { | ||
| srv, restartPosts, promotePosts := restartActionTestServer(t, utils.NodeStatusOnline) | ||
| defer srv.Close() | ||
|
|
||
| sn := &simplyblockv1alpha1.StorageNodeSet{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "sn-restart", Namespace: "default"}, | ||
| Spec: simplyblockv1alpha1.StorageNodeSetSpec{Action: "restart", NodeUUID: "node-1"}, | ||
| } | ||
| r := newStorageNodeSetStateTestReconciler(t, sn) | ||
| apiClient := webapi.NewClient(srv.URL) | ||
| key := client.ObjectKeyFromObject(sn) | ||
|
|
||
| steps, state := drive(t, r, apiClient, key) | ||
| if state != utils.ActionStateSuccess { | ||
| t.Fatalf("expected success, got %q", state) | ||
| } | ||
| if steps < 2 { | ||
| t.Fatalf("expected the flow to span multiple requeues (non-blocking), got %d step(s)", steps) | ||
| } | ||
| if got := atomic.LoadInt32(restartPosts); got != 1 { | ||
| t.Fatalf("expected exactly one /restart POST, got %d", got) | ||
| } | ||
| if got := atomic.LoadInt32(promotePosts); got != 0 { | ||
| t.Fatalf("expected no /promote for a plain restart, got %d", got) | ||
| } |
… machine - Hoist the repeated "Action executed successfully" status message into a package constant (goconst). - Split reconcileRestartAction into per-phase helpers (fire / await-online / promote / reconcile-workers) plus failRestart/succeedRestart, bringing the dispatcher's cyclomatic complexity back under the gocyclo threshold. Behaviour is unchanged; the phase-machine tests still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ration test - failRestart now returns the error when persisting the Failed state fails, so controller-runtime requeues instead of leaving the action stuck in Running. - Bound the Promoting phase with restartPromoteDeadline (120s): a permanently failing /promote now terminates as Failed instead of looping every 5s forever. - Clarify the ensureWorkerInEndpointSlice doc comment: workerNode is not in workerNodes *during* the migration (it is added on success), not "never". - Add an end-to-end migration test (spec.workerNode set) driving fire -> Migrating -> Promoting -> ReconcilingWorkers -> success, asserting exactly one /restart, one /promote, and workerNodes reconciled (origin dropped, target added). Adds a checkNodeInfoReachableFn seam so the migration fire phase is testable without a live node-info endpoint. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Wire the operator's restart action to the new control-plane migration APIs and complete the post-migration reconciliation the backend restart alone does not do.
newSsdPcie, passed to the restart call asnew_ssd_pcie(applies only to the node named bynodeUUID, likeworkerNode/reattachVolume).Scope: only restart/migration goes non-blocking; suspend/resume/shutdown keep the existing in-line path. Adds unit tests for the phase machine (non-blocking progression, storm guard, idempotency) and the worker-list reconcile.