feat: design ClusterOps CR to decouple cluster operations from the StorageCluster reconciler - #397
feat: design ClusterOps CR to decouple cluster operations from the StorageCluster reconciler#397geoffrey1330 wants to merge 26 commits into
Conversation
…l applies the CRD
…vel actions exclusively in StorageClusterOps
… spec to StorageClusterOps spec
…s.Status so node-recycle progress is visible on the ops CR
…rting, and rebalancing phase handlers
…eeding immediately after POST
boddumanohar
left a comment
There was a problem hiding this comment.
It also seems like we are removing the fields spec.action and spec.nodeRecycle. This will be a breaking change.
From 26.4 we should be careful not to include any breaking changes.
I think this is acceptable as no one is using the operator in production now. thus why the rush to implement this. cc @noctarius |
|
Okay, didn't know that this is going to be added to the release branch (26.3). Generally it's good to have this in this release make the CRDs more complete. Then in that case, it should be fine. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a dedicated StorageClusterOps one-shot CRD + controller to execute long-running, cluster-level operations (activate/expand/shutdown/start/restart/node-recycle) outside of the StorageCluster steady-state reconciler, aligning the design with the existing StorageNodeOps pattern.
Changes:
- Added
StorageClusterOpsAPI types, CRDs (operator + Helm), controller implementation (including node-recycle state machine), and controller registration. - Removed legacy imperative action handling (
spec.action,actionStatus, node-recycle logic) fromStorageClusterAPI + reconciler and deleted the old actions implementation file. - Updated RBAC/manifests and added unit tests, docs, and a shell regression test script for the new API.
Reviewed changes
Copilot reviewed 21 out of 24 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| operator/test/utils/test_storageclusterops.sh | Adds a manual regression/E2E script for StorageClusterOps behaviors (shortname, mutual exclusion, events, operations). |
| operator/internal/utils/constants.go | Adds a new finalizer constant for StorageClusterOps. |
| operator/internal/controller/storageclusterops_noderecycle.go | Implements node-recycle state machine for StorageClusterOps (including pod refresh helpers). |
| operator/internal/controller/storageclusterops_controller.go | New controller handling locking via activeOpsRef, action dispatch, finalizers, and operation lifecycles. |
| operator/internal/controller/storageclusterops_controller_unit_test.go | Adds unit tests for basic reconcile paths and lock behavior. |
| operator/internal/controller/simplyblockstoragecluster_controller.go | Removes inline cluster action dispatch from the StorageCluster reconciler. |
| operator/internal/controller/simplyblockstoragecluster_controller_unit_test.go | Removes unit tests tied to legacy inline cluster actions and node-recycle write-ahead behavior. |
| operator/internal/controller/simplyblockstoragecluster_actions.go | Deletes the legacy action-handling implementation previously used by StorageCluster. |
| operator/docs/tests/test-plan-storageclusterops.md | Adds a test plan document for StorageClusterOps. |
| operator/docs/designs/design-storageclusterops.md | Adds a design doc describing the CRD/controller architecture and migration strategy. |
| operator/dist/install.yaml | Updates bundled install manifests: adds StorageClusterOps CRD and removes legacy StorageCluster action fields; updates RBAC resources. |
| operator/config/rbac/role.yaml | Updates generated ClusterRole to include storageclusterops resources/subresources. |
| operator/config/crd/kustomization.yaml | Adds the new storageclusterops CRD base to kustomize inputs. |
| operator/config/crd/bases/storage.simplyblock.io_storageclusters.yaml | Removes legacy StorageCluster action-related schema and adds status.activeOpsRef. |
| operator/config/crd/bases/storage.simplyblock.io_storageclusterops.yaml | Adds the generated CRD for StorageClusterOps. |
| operator/cmd/main.go | Registers the new StorageClusterOpsReconciler with the manager. |
| operator/api/v1alpha1/zz_generated.deepcopy.go | Updates generated deepcopy code for new/removed API types. |
| operator/api/v1alpha1/storagenodeset_types.go | Removes the legacy ActionStatus type (now unused after action refactor). |
| operator/api/v1alpha1/storageclusterops_types.go | Adds the StorageClusterOps API types and status structs. |
| operator/api/v1alpha1/storagecluster_types.go | Removes legacy action fields/status from StorageCluster and adds ActiveOpsRef to status. |
| helm-charts/charts/simplyblock-operator/templates/roles/manager_role.yaml | Updates Helm role template to include storageclusterops resources/subresources. |
| helm-charts/charts/simplyblock-operator/crds/storage.simplyblock.io_storageclusters.yaml | Updates Helm-shipped StorageCluster CRD schema (remove action fields; add activeOpsRef). |
| helm-charts/charts/simplyblock-operator/crds/storage.simplyblock.io_storageclusterops.yaml | Adds Helm-shipped StorageClusterOps CRD. |
| atlas-lib/internal/cpapi/gen/main_test.go | Minor formatting-only adjustment (newline/indent). |
Files not reviewed (1)
- operator/api/v1alpha1/zz_generated.deepcopy.go: Generated file
Suppressed comments (2)
operator/internal/controller/storageclusterops_controller.go:566
releaseClusterLockdrops errors from the status patch (_ = r.Status().Patch(...)). If this patch fails (conflict, transient apiserver issue),activeOpsRefcan remain stuck and block all future ops with no logs to explain why.
patch := client.MergeFrom(cluster.DeepCopy())
cluster.Status.ActiveOpsRef = ""
_ = r.Status().Patch(ctx, cluster, patch)
operator/docs/tests/test-plan-storageclusterops.md:98
- The Node Recycle E2E scenarios describe
nodeUUID-scoped API calls (/node-recycle/{nodeUUID}), but the implementation recycles all nodes by listing them and then calling per-node/storage-nodes/{uuid}/shutdown|restartendpoints. The test plan should be updated to reflect the implemented behavior.
| Scenario | Expected | Classification |
|---|---|---|
| Create `StorageClusterOps(action=node-recycle, nodeUUID=<uuid>)` | Backend `/cluster/{uuid}/node-recycle/{nodeUUID}` called; ops `Succeeded` | E2E |
| Provide an invalid `nodeUUID` | Backend returns non-2xx; ops transitions to `Failed` with error message | E2E |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
I don't know if it will be added in this release. we can discuss it during the sync |
…lizer naming convention
…ef lock-release is actually verified
…ent concurrent lock races
|
|
||
| ## 7. Node-Recycle State Machine | ||
|
|
||
| Node-recycle is a multi-phase per-node state machine that iterates all storage nodes in |
There was a problem hiding this comment.
We should describe it the same way as Kubernetes names this operation, a "rolling restart" or "rollout restart"
There was a problem hiding this comment.
what do you mean here?
| @@ -0,0 +1,128 @@ | |||
| # Test Plan: StorageClusterOps | |||
There was a problem hiding this comment.
What happens when a storage node goes offline during a restart? In FTT=2, this would be valid but how would the code handle this situation?
There was a problem hiding this comment.
Good question. The current code doesn't handle this. it only checks the status of the node it's actively restarting, not the status of the rest of the nodes. i will add this condition to pause the restart until the rest of the nodes are online
There was a problem hiding this comment.
Added a pre-shutdown health check that verifies all other nodes are online before proceeding with each shutdown. If any peer is not online, the rolling restart holds and requeues every 30s until the cluster recovers, preventing us from pushing past the cluster's FTT.
…roller, CRDs, docs, and tests
…eding cluster FTT on concurrent node failure
…mplyblockstoragecluster_actions.go
Fixes: issue-392
Summary
Introduces
StorageClusterOps, a new one-shot CRD for driving cluster-level operations (activate, expand, shutdown, restart, node-recycle) to completion and recording the result. Modelled after the existingStorageNodeOpspattern.storageclusterops.storage.simplyblock.io(scopsshort name) withPending → Running → Succeeded/Failedlifecycle phasesactiveOpsRefonStorageClusterStatus— only oneStorageClusterOpsactive per cluster at a time; concurrent ops requeue until the lock is freeStorageClusterOpsStatusprevents duplicate backend POSTs when a status patch fails after a successful mutationreconcileActivate,reconcileExpand,reconcileSimplePost,reconcileNodeRecycle), each polling until the backend confirms completionsimplyblock-operator/crds/and example CR added tooperator_customresources.yamlconfig/rbac/role.yamlupdated tostorageclusterops(all three subresources: base,/status,/finalizers)nodeUUIDfor node-recycleoperator/docs/designs/design-storageclusterops.mdTest plan
go test ./internal/controller/ -run TestStorageClusterOps -v— all 12 tests passmake manifestsgeneratesstorage.simplyblock.io_storageclusterops.yamlwith correct kind, plural, and short name (scops)StorageClusterOpsCRs targeting the same cluster simultaneously — second one should stayPendingand requeue until the first completeskubectl get scopsworks via short nameScript Based Tests
Test Result