fix(controller): roll instances by recreate so restart actually rolls them

A template-hash change (an image update, or a restartedAt annotation from
`datumctl compute restart`) previously resolved to an in-place Update of the
Instance. The unikraft provider bakes the pod at creation time and never
recomputes an existing pod's spec, so the in-place update silently failed to
roll the running workload — instances kept their old pod.

Emit a delete (recreate) for drifted Ready instances instead. The next
reconcile refills the slot via the create path with the new template, and the
provider's finalizer-gated teardown plus create-on-new-Instance roll the pod
with no provider changes. Ordered one-at-a-time pacing is preserved by the
existing descending-ordinal sort, skip-all-but-first, and the
DeletionTimestamp WaitAction.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: scotwells

internal/controller/instancecontrol/stateful/stateful_control.go

@@ -53,8 +53,10 @@ func (c *statefulControl) GetActions(
 	var createActions []instancecontrol.Action
 	var waitActions []instancecontrol.Action
 
-	// highest -> lowest
-	var updateActions []instancecontrol.Action
+	// highest -> lowest. Instances whose template hash has drifted from the
+	// desired template are deleted and recreated (not updated in place) so the
+	// change actually rolls the backing pod — see the recreate branch below.
+	var recreateActions []instancecontrol.Action
 
 	// highest -> lowest
 	var deleteActions []instancecontrol.Action
@@ -129,14 +131,19 @@ func (c *statefulControl) GetActions(
 			if !apimeta.IsStatusConditionTrue(instance.Status.Conditions, v1alpha.InstanceReady) {
 				waitActions = append(waitActions, instancecontrol.NewWaitAction(instance))
 			} else if needsUpdate(instance, instanceTemplateHash) {
-				updatedInstance := instance.DeepCopy()
-				updatedInstance.Annotations = deployment.Spec.Template.Annotations
-				updatedInstance.Labels = deployment.Spec.Template.Labels
-
-				addInstanceControllerLabels(updatedInstance, getInstanceOrdinal(updatedInstance.Name), deployment)
-
-				updatedInstance.Spec = deployment.Spec.Template.Spec
-				updateActions = append(updateActions, instancecontrol.NewUpdateAction(updatedInstance))
+				// The instance's template hash no longer matches the desired
+				// template — e.g. an image change, or a restart requested via the
+				// RestartedAtAnnotation, which is part of the template hash. The
+				// unikraft provider bakes the pod's runtime, rootfs, and file
+				// mounts at pod-creation time and never reconciles an existing
+				// pod's spec, so an in-place Instance update would silently fail to
+				// roll the running workload. Delete the instance instead; the next
+				// reconcile recreates it from the current template via the create
+				// path above, and the provider tears down the old pod
+				// (finalizer-gated) and boots a fresh one. Ordered, one-at-a-time
+				// pacing is preserved by the descending-ordinal sort, the
+				// skip-all-but-first logic, and the DeletionTimestamp WaitAction.
+				recreateActions = append(recreateActions, instancecontrol.NewDeleteAction(instance))
 			}
 		}
 	}
@@ -168,10 +175,10 @@ func (c *statefulControl) GetActions(
 		}
 	}
 
-	slices.SortFunc(updateActions, descendingOrdinal)
+	slices.SortFunc(recreateActions, descendingOrdinal)
 	slices.SortFunc(deleteActions, descendingOrdinal)
 
-	actions := make([]instancecontrol.Action, 0, len(createActions)+len(waitActions)+len(updateActions)+len(deleteActions)+len(patchLabelActions))
+	actions := make([]instancecontrol.Action, 0, len(createActions)+len(waitActions)+len(recreateActions)+len(deleteActions)+len(patchLabelActions))
 
 	switch deployment.Spec.ScaleSettings.InstanceManagementPolicy {
 	case v1alpha.OrderedReadyInstanceManagementPolicyType:
@@ -186,7 +193,7 @@ func (c *statefulControl) GetActions(
 
 		slices.SortFunc(actions, ascendingOrdinal)
 
-		actions = append(actions, updateActions...)
+		actions = append(actions, recreateActions...)
 		actions = append(actions, deleteActions...)
 
 		// Skip all actions except the first one.

internal/controller/instancecontrol/stateful/stateful_control_test.go

@@ -49,6 +49,11 @@ func TestFreshDeployment(t *testing.T) {
 	assert.True(t, actions[1].IsSkipped())
 }
 
+// TestUpdateWithAllReadyInstances verifies that a template change on Ready
+// instances rolls them by delete+recreate (not an in-place update), ordered
+// highest-ordinal-first with only the first action active. An in-place update
+// would never roll the backing pod, since the unikraft provider bakes the pod
+// at creation time and ignores spec changes on an existing pod.
 func TestUpdateWithAllReadyInstances(t *testing.T) {
 	ctx := context.Background()
 	control := New()
@@ -67,11 +72,11 @@ func TestUpdateWithAllReadyInstances(t *testing.T) {
 	assert.Len(t, actions, 2)
 
 	assert.Equal(t, "test-deploy-1", actions[0].Object.GetName())
-	assert.Equal(t, instancecontrol.ActionTypeUpdate, actions[0].ActionType())
+	assert.Equal(t, instancecontrol.ActionTypeDelete, actions[0].ActionType())
 	assert.False(t, actions[0].IsSkipped())
 
 	assert.Equal(t, "test-deploy-0", actions[1].Object.GetName())
-	assert.Equal(t, instancecontrol.ActionTypeUpdate, actions[1].ActionType())
+	assert.Equal(t, instancecontrol.ActionTypeDelete, actions[1].ActionType())
 	assert.True(t, actions[1].IsSkipped())
 }
 
@@ -244,38 +249,48 @@ func TestInstanceLabels_FourNewLabelsStamped(t *testing.T) {
 		"PlacementNameLabel must equal deployment.Spec.PlacementName")
 }
 
-// TestInstanceLabels_PropagatedOnUpdate verifies that when an existing instance
-// is updated (rolling update path), the four new labels are refreshed from the
-// deployment so they remain accurate after spec changes.
-func TestInstanceLabels_PropagatedOnUpdate(t *testing.T) {
+// TestInstanceLabels_RefreshedOnRecreate verifies that when a template change
+// rolls an instance, the recreated instance carries the four self-describing
+// labels sourced from the WorkloadDeployment. A template change no longer
+// updates the instance in place; it deletes the drifted instance and recreates
+// it via the create path on the following reconcile, which stamps the labels.
+func TestInstanceLabels_RefreshedOnRecreate(t *testing.T) {
 	ctx := context.Background()
 	control := New()
 
 	deployment := getWorkloadDeployment("test-labels-update", 1)
 
-	// Build a ready existing instance.
+	// A ready existing instance on the old template hash.
 	currentInstances := []v1alpha.Instance{*getInstanceForDeployment(deployment, 0)}
 
-	// Trigger a rolling update by changing the image.
+	// Trigger a roll by changing the image.
 	deployment.Spec.Template.Spec.Runtime.Sandbox.Containers[0].Image = "updated-image"
 
+	// First reconcile: the drifted instance is deleted (recreate), not updated.
 	actions, err := control.GetActions(ctx, scheme, deployment, currentInstances)
+	assert.NoError(t, err)
+	assert.Len(t, actions, 1)
+	assert.Equal(t, instancecontrol.ActionTypeDelete, actions[0].ActionType())
+	assert.Equal(t, "test-labels-update-0", actions[0].Object.GetName())
 
+	// Next reconcile, after the old instance has been fully deleted and is gone:
+	// the empty slot is refilled by the create path, which stamps the labels.
+	actions, err = control.GetActions(ctx, scheme, deployment, nil)
 	assert.NoError(t, err)
 	assert.Len(t, actions, 1)
-	assert.Equal(t, instancecontrol.ActionTypeUpdate, actions[0].ActionType())
+	assert.Equal(t, instancecontrol.ActionTypeCreate, actions[0].ActionType())
 
 	instance, ok := actions[0].Object.(*v1alpha.Instance)
 	assert.True(t, ok)
 
 	assert.Equal(t, deployment.GetName(), instance.Labels[v1alpha.WorkloadDeploymentNameLabel],
-		"WorkloadDeploymentNameLabel must be refreshed on update")
+		"WorkloadDeploymentNameLabel must be set on the recreated instance")
 	assert.Equal(t, deployment.Spec.CityCode, instance.Labels[v1alpha.CityCodeLabel],
-		"CityCodeLabel must be refreshed on update")
+		"CityCodeLabel must be set on the recreated instance")
 	assert.Equal(t, deployment.Spec.WorkloadRef.Name, instance.Labels[v1alpha.WorkloadNameLabel],
-		"WorkloadNameLabel must be refreshed on update")
+		"WorkloadNameLabel must be set on the recreated instance")
 	assert.Equal(t, deployment.Spec.PlacementName, instance.Labels[v1alpha.PlacementNameLabel],
-		"PlacementNameLabel must be refreshed on update")
+		"PlacementNameLabel must be set on the recreated instance")
 }
 
 // TestInstanceLocation_SetWhenDeploymentStatusLocationPresent verifies that when
@@ -331,7 +346,7 @@ func TestInstanceLocation_NilWhenDeploymentStatusLocationAbsent(t *testing.T) {
 
 // TestLabelBackfill_NotReadyMatchingHash verifies that a not-Ready instance
 // with an unchanged template hash receives a PatchLabels action when it is
-// missing controller-managed labels. The action must not be a rollout Update,
+// missing controller-managed labels. The action must not be a rollout recreate,
 // must not alter spec/template, and must not block subsequent instances.
 func TestLabelBackfill_NotReadyMatchingHash(t *testing.T) {
 	ctx := context.Background()
@@ -361,15 +376,15 @@ func TestLabelBackfill_NotReadyMatchingHash(t *testing.T) {
 	assert.NoError(t, err)
 
 	// Collect actions by type.
-	var waitActions, createActions, updateActions, patchActions []instancecontrol.Action
+	var waitActions, createActions, recreateActions, patchActions []instancecontrol.Action
 	for _, a := range actions {
 		switch a.ActionType() {
 		case instancecontrol.ActionTypeWait:
 			waitActions = append(waitActions, a)
 		case instancecontrol.ActionTypeCreate:
 			createActions = append(createActions, a)
-		case instancecontrol.ActionTypeUpdate:
-			updateActions = append(updateActions, a)
+		case instancecontrol.ActionTypeDelete:
+			recreateActions = append(recreateActions, a)
 		case instancecontrol.ActionTypePatchLabels:
 			patchActions = append(patchActions, a)
 		}
@@ -383,8 +398,8 @@ func TestLabelBackfill_NotReadyMatchingHash(t *testing.T) {
 	assert.Len(t, createActions, 1, "instance-1 create action must be present")
 	assert.True(t, createActions[0].IsSkipped(), "create for instance-1 must be skipped while instance-0 is waiting")
 
-	// No template Update actions must be produced.
-	assert.Empty(t, updateActions, "no template Update must be produced for a matching-hash instance")
+	// No rollout recreate actions must be produced.
+	assert.Empty(t, recreateActions, "no rollout recreate must be produced for a matching-hash instance")
 
 	// A PatchLabels action must be produced for instance-0.
 	assert.Len(t, patchActions, 1, "exactly one PatchLabels action for the label-drifted instance")
@@ -439,7 +454,7 @@ func TestLabelBackfill_Idempotent(t *testing.T) {
 
 // TestLabelBackfill_ReadyInstanceCorrected verifies that a Ready instance with
 // correct template hash but drifted labels receives a PatchLabels action
-// without triggering a template rollout Update.
+// without triggering a rollout recreate.
 func TestLabelBackfill_ReadyInstanceCorrected(t *testing.T) {
 	ctx := context.Background()
 	control := New()
@@ -456,18 +471,18 @@ func TestLabelBackfill_ReadyInstanceCorrected(t *testing.T) {
 
 	assert.NoError(t, err)
 
-	var updateActions, patchActions []instancecontrol.Action
+	var recreateActions, patchActions []instancecontrol.Action
 	for _, a := range actions {
 		switch a.ActionType() {
-		case instancecontrol.ActionTypeUpdate:
-			updateActions = append(updateActions, a)
+		case instancecontrol.ActionTypeDelete:
+			recreateActions = append(recreateActions, a)
 		case instancecontrol.ActionTypePatchLabels:
 			patchActions = append(patchActions, a)
 		}
 	}
 
-	// No template Update must be produced — template hash matches.
-	assert.Empty(t, updateActions, "no template Update must be produced for a matching-hash ready instance")
+	// No rollout recreate must be produced — template hash matches.
+	assert.Empty(t, recreateActions, "no rollout recreate must be produced for a matching-hash ready instance")
 
 	// A PatchLabels action must be produced.
 	assert.Len(t, patchActions, 1, "PatchLabels action must be produced for the label-drifted ready instance")
@@ -478,8 +493,9 @@ func TestLabelBackfill_ReadyInstanceCorrected(t *testing.T) {
 }
 
 // TestLabelBackfill_DoesNotAffectRollingUpdate verifies that a genuine template
-// change on a Ready instance still produces a normal ordered Update action and
-// that the PatchLabels path does not interfere with or duplicate it.
+// change on a Ready instance still produces the normal ordered roll (a recreate
+// Delete per instance) and that the PatchLabels path does not interfere with or
+// duplicate it.
 func TestLabelBackfill_DoesNotAffectRollingUpdate(t *testing.T) {
 	ctx := context.Background()
 	control := New()
@@ -516,23 +532,23 @@ func TestLabelBackfill_DoesNotAffectRollingUpdate(t *testing.T) {
 
 	assert.NoError(t, err)
 
-	var updateActions, patchActions []instancecontrol.Action
+	var recreateActions, patchActions []instancecontrol.Action
 	for _, a := range actions {
 		switch a.ActionType() {
-		case instancecontrol.ActionTypeUpdate:
-			updateActions = append(updateActions, a)
+		case instancecontrol.ActionTypeDelete:
+			recreateActions = append(recreateActions, a)
 		case instancecontrol.ActionTypePatchLabels:
 			patchActions = append(patchActions, a)
 		}
 	}
 
-	// Two Update actions expected (one per instance), ordered highest-to-lowest.
-	assert.Len(t, updateActions, 2, "both instances must produce Update actions on template change")
-	assert.Equal(t, "test-backfill-rolling-1", updateActions[0].Object.GetName(),
-		"Update actions must be ordered highest ordinal first")
-	assert.Equal(t, "test-backfill-rolling-0", updateActions[1].Object.GetName())
-	assert.False(t, updateActions[0].IsSkipped(), "first Update must be active")
-	assert.True(t, updateActions[1].IsSkipped(), "second Update must be skipped (ordered rollout)")
+	// Two recreate (Delete) actions expected (one per instance), ordered highest-to-lowest.
+	assert.Len(t, recreateActions, 2, "both instances must produce recreate actions on template change")
+	assert.Equal(t, "test-backfill-rolling-1", recreateActions[0].Object.GetName(),
+		"recreate actions must be ordered highest ordinal first")
+	assert.Equal(t, "test-backfill-rolling-0", recreateActions[1].Object.GetName())
+	assert.False(t, recreateActions[0].IsSkipped(), "first recreate must be active")
+	assert.True(t, recreateActions[1].IsSkipped(), "second recreate must be skipped (ordered rollout)")
 
 	// No PatchLabels — all labels are already correct.
 	assert.Empty(t, patchActions, "no PatchLabels when all labels are already correct")