Target worker node verification after storage node migration

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,
+	)
+}

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,14 +130,92 @@ 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)
 			}
 		})
 	}
 }
 
+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)