diff --git a/operator/internal/controller/simplyblockstoragenodeset_controller.go b/operator/internal/controller/simplyblockstoragenodeset_controller.go index 59574e09..0da618db 100644 --- a/operator/internal/controller/simplyblockstoragenodeset_controller.go +++ b/operator/internal/controller/simplyblockstoragenodeset_controller.go @@ -119,7 +119,7 @@ var ( // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterroles,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterrolebindings,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=cert-manager.io,resources=certificates,verbs=get;list;watch;create;update;patch;delete -// +kubebuilder:rbac:groups="",resources=events,verbs=get;list;watch +// +kubebuilder:rbac:groups="",resources=events,verbs=get;list;watch;create;patch // +kubebuilder:rbac:groups=storage.simplyblock.io,resources=volumemigrations,verbs=get;list;watch;create;delete // +kubebuilder:rbac:groups="",resources=persistentvolumes,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=persistentvolumeclaims,verbs=get;list;watch;update;patch @@ -1848,7 +1848,7 @@ func (r *StorageNodeSetReconciler) performNodeAction( } else if inProgress := actionInProgressStatus(snCR.Spec.Action); inProgress != "" && cur == inProgress { log.Info("Backend action already in progress; waiting for completion instead of re-triggering", "action", snCR.Spec.Action, "nodeUUID", snCR.Spec.NodeUUID, "status", cur) - return r.waitForActionCompletion(ctx, apiClient, clusterUUID, snCR.Spec.NodeUUID, snCR.Spec.Action) + return r.waitForActionCompletion(ctx, apiClient, clusterUUID, snCR) } else if target, ok := actionTargetStatus(snCR.Spec.Action); ok && cur == target && actionAlreadyTriggered(snCR) { log.Info("Backend action already completed; nothing to do", "action", snCR.Spec.Action, "nodeUUID", snCR.Spec.NodeUUID, "status", cur) @@ -1953,8 +1953,7 @@ func (r *StorageNodeSetReconciler) performNodeAction( ctx, apiClient, clusterUUID, - snCR.Spec.NodeUUID, - snCR.Spec.Action, + snCR, ); err != nil { return fmt.Errorf( "node did not reach expected state after action %s: %w", @@ -2082,12 +2081,14 @@ func (r *StorageNodeSetReconciler) waitForActionCompletion( ctx context.Context, apiClient *webapi.Client, clusterUUID string, - nodeUUID string, - action string, + snCR *simplyblockv1alpha1.StorageNodeSet, ) error { log := logf.FromContext(ctx) + nodeUUID := snCR.Spec.NodeUUID + action := snCR.Spec.Action + expectedStatus := map[string]string{ utils.NodeActionSuspend: "suspended", utils.NodeActionResume: "online", @@ -2152,7 +2153,7 @@ func (r *StorageNodeSetReconciler) waitForActionCompletion( if resp.Status == targetStatus { log.Info("Node reached expected status", "nodeUUID", nodeUUID, "status", resp.Status) - return nil + return r.verifyWorkerAssignment(ctx, snCR, resp) } waitForActionCompletionSleepFn(waitForActionCompletionWaitInterval) @@ -2165,3 +2166,55 @@ func (r *StorageNodeSetReconciler) waitForActionCompletion( action, ) } + +// verifyWorkerAssignment confirms that a migration actually landed the storage +// node on the requested worker. It only applies to a restart that targets a +// specific worker (spec.workerNode) — the migration flow. +// +// A failed migration can silently report success on the ORIGINAL worker: the +// webapp resets the node to OFFLINE without committing the new api_endpoint, so +// the tasks-runner picks up the offline node, restarts it against the stale +// (old-worker) endpoint, and the node comes back online where it started. +// Checking only status == "online" passes in that case. To catch it we compare +// the node's reported mgmt_ip against the requested worker's InternalIP; a +// mismatch means the node came online on the wrong worker. +func (r *StorageNodeSetReconciler) verifyWorkerAssignment( + ctx context.Context, + snCR *simplyblockv1alpha1.StorageNodeSet, + resp utils.NodeStatusResponse, +) error { + if snCR.Spec.Action != "restart" || snCR.Spec.WorkerNode == "" { + return nil + } + + log := logf.FromContext(ctx) + + expectedIP, err := getNodeInternalIP(ctx, r.Client, snCR.Spec.WorkerNode) + if err != nil { + return fmt.Errorf( + "cannot verify worker assignment for node %s: %w", + snCR.Spec.NodeUUID, err, + ) + } + + if resp.IP == expectedIP { + log.Info( + "Node came online on the expected worker", + "nodeUUID", snCR.Spec.NodeUUID, + "workerNode", snCR.Spec.WorkerNode, + "mgmtIp", resp.IP, + ) + return nil + } + + r.Recorder.Eventf( + snCR, corev1.EventTypeWarning, "WorkerAssignmentMismatch", + "Node %s came online on the wrong worker after migration: expected %s (%s), got mgmt_ip %s", + snCR.Spec.NodeUUID, snCR.Spec.WorkerNode, expectedIP, resp.IP, + ) + + return fmt.Errorf( + "node %s online on wrong worker: expected %s (%s), got mgmt_ip %s", + snCR.Spec.NodeUUID, snCR.Spec.WorkerNode, expectedIP, resp.IP, + ) +} diff --git a/operator/internal/controller/simplyblockstoragenodeset_controller_unit_test.go b/operator/internal/controller/simplyblockstoragenodeset_controller_unit_test.go index 772234d2..90875ddf 100644 --- a/operator/internal/controller/simplyblockstoragenodeset_controller_unit_test.go +++ b/operator/internal/controller/simplyblockstoragenodeset_controller_unit_test.go @@ -24,6 +24,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -65,7 +66,10 @@ func TestEnsureNodeStatus(t *testing.T) { func TestWaitForActionCompletionUnknownAction(t *testing.T) { r := &StorageNodeSetReconciler{} c := webapi.NewClient("http://127.0.0.1:1") - err := r.waitForActionCompletion(context.Background(), c, "cluster", "node", "invalid-action") + sn := &simplyblockv1alpha1.StorageNodeSet{ + Spec: simplyblockv1alpha1.StorageNodeSetSpec{NodeUUID: "node", Action: "invalid-action"}, + } + err := r.waitForActionCompletion(context.Background(), c, "cluster", sn) if err == nil { t.Fatalf("expected error for unknown action") } @@ -126,7 +130,10 @@ func TestWaitForActionCompletionValidTransitions(t *testing.T) { r := &StorageNodeSetReconciler{} c := webapi.NewClient(srv.URL) - err := r.waitForActionCompletion(context.Background(), c, "cluster", "node", tc.action) + sn := &simplyblockv1alpha1.StorageNodeSet{ + Spec: simplyblockv1alpha1.StorageNodeSetSpec{NodeUUID: "node", Action: tc.action}, + } + err := r.waitForActionCompletion(context.Background(), c, "cluster", sn) if err != nil { t.Fatalf("waitForActionCompletion returned error: %v", err) } @@ -134,6 +141,81 @@ func TestWaitForActionCompletionValidTransitions(t *testing.T) { } } +func TestVerifyWorkerAssignment(t *testing.T) { + scheme := newTestScheme(t, simplyblockv1alpha1.AddToScheme, corev1.AddToScheme) + + worker4 := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "worker-4"}, + Status: corev1.NodeStatus{ + Addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: "192.168.10.247"}, + }, + }, + } + + newReconciler := func() (*StorageNodeSetReconciler, *record.FakeRecorder) { + rec := record.NewFakeRecorder(8) + return &StorageNodeSetReconciler{ + Client: newTestClient(t, scheme, nil, worker4), + Scheme: scheme, + Recorder: rec, + }, rec + } + + newSN := func(action, workerNode string) *simplyblockv1alpha1.StorageNodeSet { + return &simplyblockv1alpha1.StorageNodeSet{ + ObjectMeta: metav1.ObjectMeta{Name: "sn", Namespace: "default"}, + Spec: simplyblockv1alpha1.StorageNodeSetSpec{ + Action: action, + NodeUUID: "node-1", + WorkerNode: workerNode, + }, + } + } + + t.Run("node online on the requested worker passes", func(t *testing.T) { + r, rec := newReconciler() + err := r.verifyWorkerAssignment(context.Background(), newSN("restart", "worker-4"), + utils.NodeStatusResponse{Status: statusOnline, IP: "192.168.10.247"}) + if err != nil { + t.Fatalf("expected nil for matching worker, got %v", err) + } + select { + case e := <-rec.Events: + t.Fatalf("expected no event, got %q", e) + default: + } + }) + + t.Run("node online on the wrong worker errors and emits an event", func(t *testing.T) { + r, rec := newReconciler() + // The migration targeted worker-4 (.247) but the node came back on + // worker-3 (.246) — the silent-success case from SFAM-2764. + err := r.verifyWorkerAssignment(context.Background(), newSN("restart", "worker-4"), + utils.NodeStatusResponse{Status: statusOnline, IP: "192.168.10.246"}) + if err == nil { + t.Fatalf("expected an error when the node landed on the wrong worker") + } + select { + case e := <-rec.Events: + if !strings.Contains(e, "WorkerAssignmentMismatch") { + t.Fatalf("unexpected event: %q", e) + } + default: + t.Fatalf("expected a WorkerAssignmentMismatch event") + } + }) + + t.Run("restart without a target worker is not verified", func(t *testing.T) { + r, _ := newReconciler() + err := r.verifyWorkerAssignment(context.Background(), newSN("restart", ""), + utils.NodeStatusResponse{Status: statusOnline, IP: "10.0.0.1"}) + if err != nil { + t.Fatalf("expected nil when no worker is targeted, got %v", err) + } + }) +} + func TestHandleNodeActionTransitions(t *testing.T) { t.Run("does not re-enter terminal success for same action and node", func(t *testing.T) { sn := &simplyblockv1alpha1.StorageNodeSet{ @@ -1758,8 +1840,9 @@ func TestWaitForActionCompletionRetryBehavior(t *testing.T) { context.Background(), webapi.NewClient(srv.URL), "cluster-a", - "node-a", - "restart", + &simplyblockv1alpha1.StorageNodeSet{ + Spec: simplyblockv1alpha1.StorageNodeSetSpec{NodeUUID: "node-a", Action: "restart"}, + }, ) if err == nil { t.Fatalf("expected terminal status error") @@ -1794,8 +1877,9 @@ func TestWaitForActionCompletionRetryBehavior(t *testing.T) { context.Background(), webapi.NewClient(srv.URL), "cluster-b", - "node-b", - "restart", + &simplyblockv1alpha1.StorageNodeSet{ + Spec: simplyblockv1alpha1.StorageNodeSetSpec{NodeUUID: "node-b", Action: "restart"}, + }, ) if err != nil { t.Fatalf("expected eventual success, got error: %v", err)