From c3e0e5a99c58cd46a9b9f08c9db3ece085b672c7 Mon Sep 17 00:00:00 2001 From: Scot Schuchert-Wells Date: Wed, 11 Nov 2026 15:37:18 +0000 Subject: [PATCH] fix: patch the Workload for a location restart A location restart replaced the whole Workload it had read earlier, so any concurrent edit made the command fail with a conflict. It now sends a merge patch of the one annotation, which applies cleanly over other changes. Key changes: - patch only the location restart annotation instead of updating the Workload - document that the annotation stays after the restart and that removing it rolls the location once more - test a restart that races another edit, and an annotation naming a location the workload does not place in --- api/v1alpha/annotations.go | 5 +++ internal/cmd/compute/restart/restart.go | 3 +- internal/cmd/compute/restart/restart_test.go | 18 ++++++++ .../controller/workload_controller_test.go | 44 +++++++++++++++++++ 4 files changed, 69 insertions(+), 1 deletion(-) diff --git a/api/v1alpha/annotations.go b/api/v1alpha/annotations.go index 7bb7706c..100dca8a 100644 --- a/api/v1alpha/annotations.go +++ b/api/v1alpha/annotations.go @@ -31,6 +31,11 @@ const ( // location only, so a restart there needs only permission to update the // Workload. Deployments are owned by the workload controller, and a direct // edit to their template is overwritten on its next reconcile. + // + // The annotation is left in place after the restart, since a value that + // does not change does not roll again. Removing it changes the template of + // that location's deployments, which rolls the location once more. An + // annotation naming a location the workload does not place in is ignored. LocationRestartAnnotationPrefix = "location-restart." + AnnotationNamespace + "/" // LocationRestartedAtAnnotation is set by the workload controller on the diff --git a/internal/cmd/compute/restart/restart.go b/internal/cmd/compute/restart/restart.go index a589f041..013c0e71 100644 --- a/internal/cmd/compute/restart/restart.go +++ b/internal/cmd/compute/restart/restart.go @@ -110,12 +110,13 @@ func restartLocation(ctx context.Context, c client.Client, workload *computev1al return fmt.Errorf("no deployment found for workload %q in location %q", workload.Name, location) } + base := workload.DeepCopy() if workload.Annotations == nil { workload.Annotations = make(map[string]string) } workload.Annotations[annotation] = restartedAt - if err := c.Update(ctx, workload); err != nil { + if err := c.Patch(ctx, workload, client.MergeFrom(base)); err != nil { return fmt.Errorf("updating workload: %w", err) } diff --git a/internal/cmd/compute/restart/restart_test.go b/internal/cmd/compute/restart/restart_test.go index 0f229a03..f10e5840 100644 --- a/internal/cmd/compute/restart/restart_test.go +++ b/internal/cmd/compute/restart/restart_test.go @@ -100,3 +100,21 @@ func TestRestartLocation_UnknownLocation(t *testing.T) { require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(workload), &got)) assert.Empty(t, got.Annotations) } + +func TestRestartLocation_SurvivesConcurrentWorkloadEdit(t *testing.T) { + t.Parallel() + ctx := context.Background() + c, workload := newFixture(t) + + var concurrent computev1alpha.Workload + require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(workload), &concurrent)) + concurrent.Labels = map[string]string{"edited": "true"} + require.NoError(t, c.Update(ctx, &concurrent)) + + require.NoError(t, restartLocation(ctx, c, workload, testLocation, testRestartedAt)) + + var got computev1alpha.Workload + require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(workload), &got)) + assert.Equal(t, testRestartedAt, got.Annotations[computev1alpha.LocationRestartAnnotationPrefix+testLocation]) + assert.Equal(t, "true", got.Labels["edited"]) +} diff --git a/internal/controller/workload_controller_test.go b/internal/controller/workload_controller_test.go index 96283799..7c270bbd 100644 --- a/internal/controller/workload_controller_test.go +++ b/internal/controller/workload_controller_test.go @@ -865,3 +865,47 @@ func TestGetDeploymentsForWorkload_LocationRestartReachesOnlyThatLocation(t *tes assert.NotContains(t, byLocation[otherLocation].Spec.Template.Annotations, computev1alpha.LocationRestartedAtAnnotation) assert.Empty(t, workload.Spec.Template.Annotations, "the workload's own template must not change") } + +func TestGetDeploymentsForWorkload_LocationRestartForUnplacedLocationIgnored(t *testing.T) { + t.Parallel() + + workload := &computev1alpha.Workload{ + ObjectMeta: metav1.ObjectMeta{ + Name: rdTestWorkloadName, + Namespace: testDefaultNamespace, + UID: types.UID("workload-uid"), + Annotations: map[string]string{ + computev1alpha.LocationRestartAnnotationPrefix + "ord": "2026-01-02T15:04:05Z", + }, + }, + Spec: computev1alpha.WorkloadSpec{ + Placements: []computev1alpha.WorkloadPlacement{ + { + Name: testDefaultPlacement, + Locations: []locationsv1alpha1.LocationReference{{Name: testLocationName}}, + ScaleSettings: computev1alpha.HorizontalScaleSettings{MinReplicas: 1}, + }, + }, + }, + } + + s := newNetworkingScheme() + require.NoError(t, locationsv1alpha1.AddToScheme(s)) + cl := fake.NewClientBuilder(). + WithScheme(s). + WithObjects( + newLocationsServiceLocation(testLocationName, "DFW"), + newLocationsServiceLocation("ord", "ORD"), + newTestComputeAvailability(testLocationName), + newTestComputeAvailability("ord"), + ). + WithIndex(&computev1alpha.WorkloadDeployment{}, deploymentWorkloadUIDIndex, deploymentWorkloadUIDIndexFunc). + Build() + r := &WorkloadReconciler{} + + desired, _, err := r.getDeploymentsForWorkload(context.Background(), cl, workload) + require.NoError(t, err) + require.Len(t, desired, 1) + assert.Equal(t, testLocationName, desired[0].Spec.LocationRef.Name) + assert.NotContains(t, desired[0].Spec.Template.Annotations, computev1alpha.LocationRestartedAtAnnotation) +}