feat: Run workloads with long names in every location

internal/controller/workload_controller.go

@@ -33,6 +33,7 @@ import (
 	computev1alpha "go.datum.net/compute/api/v1alpha"
 	"go.datum.net/compute/internal/features"
 	"go.datum.net/compute/internal/locations"
+	"go.datum.net/compute/internal/naming"
 	"go.datum.net/compute/pkg/runtimeclass"
 	networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha"
 	locationsv1alpha1 "go.miloapis.com/locations/api/v1alpha1"
@@ -208,30 +209,7 @@ func (r *WorkloadReconciler) Reconcile(ctx context.Context, req mcreconcile.Requ
 	for _, desiredDeployment := range desired {
 		logger.Info("ensuring workload deployment", "deployment_name", desiredDeployment.Name)
 
-		deployment := &computev1alpha.WorkloadDeployment{
-			ObjectMeta: metav1.ObjectMeta{
-				Namespace: desiredDeployment.Namespace,
-				Name:      desiredDeployment.Name,
-			},
-		}
-
-		_, err := controllerutil.CreateOrUpdate(ctx, cl.GetClient(), deployment, func() error {
-			if deployment.CreationTimestamp.IsZero() {
-				logger.Info("creating deployment", "deployment_name", deployment.Name)
-				if err := controllerutil.SetControllerReference(&workload, deployment, cl.GetScheme()); err != nil {
-					return fmt.Errorf("failed to set controller on workload deployment: %w", err)
-				}
-			} else {
-				logger.Info("updating deployment", "deployment_name", deployment.Name)
-			}
-
-			mergeDeploymentMetadata(deployment, &desiredDeployment)
-
-			// TODO(jreese) consider how this plays well with autoscaling
-			deployment.Spec = desiredDeployment.Spec
-			return nil
-		})
-
+		deployment, err := upsertWorkloadDeployment(ctx, cl.GetClient(), &workload, &desiredDeployment)
 		if err != nil {
 			return ctrl.Result{}, fmt.Errorf("failed mutating workload deployment: %w", err)
 		}
@@ -245,6 +223,50 @@ func (r *WorkloadReconciler) Reconcile(ctx context.Context, req mcreconcile.Requ
 	return ctrl.Result{}, r.reconcileWorkloadStatus(ctx, cl.GetClient(), &workload, placementDeployments)
 }
 
+// upsertWorkloadDeployment creates or updates the deployment named by desired. It
+// refuses to update a deployment that belongs to a different workload.
+func upsertWorkloadDeployment(
+	ctx context.Context,
+	upstreamClient client.Client,
+	workload *computev1alpha.Workload,
+	desired *computev1alpha.WorkloadDeployment,
+) (*computev1alpha.WorkloadDeployment, error) {
+	logger := log.FromContext(ctx)
+
+	deployment := &computev1alpha.WorkloadDeployment{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: desired.Namespace,
+			Name:      desired.Name,
+		},
+	}
+
+	_, err := controllerutil.CreateOrUpdate(ctx, upstreamClient, deployment, func() error {
+		if owner := deployment.Spec.WorkloadRef.UID; owner != "" && owner != workload.UID {
+			return fmt.Errorf("workload deployment %q belongs to workload UID %q, not %q",
+				deployment.Name, owner, workload.UID)
+		}
+
+		if deployment.CreationTimestamp.IsZero() {
+			logger.Info("creating deployment", "deployment_name", deployment.Name)
+			if err := controllerutil.SetControllerReference(workload, deployment, upstreamClient.Scheme()); err != nil {
+				return fmt.Errorf("failed to set controller on workload deployment: %w", err)
+			}
+		} else {
+			logger.Info("updating deployment", "deployment_name", deployment.Name)
+		}
+
+		mergeDeploymentMetadata(deployment, desired)
+
+		// TODO(jreese) consider how this plays well with autoscaling
+		deployment.Spec = desired.Spec
+		return nil
+	})
+	if err != nil {
+		return nil, err
+	}
+	return deployment, nil
+}
+
 func (r *WorkloadReconciler) reconcileWorkloadStatus(
 	ctx context.Context,
 	upstreamClient client.Client,
@@ -521,13 +543,8 @@ func (r *WorkloadReconciler) getDeploymentsForWorkload(
 		return nil, nil, err
 	}
 
-	existingDeployments := sets.Set[string]{}
 	desiredDeployments := sets.Set[string]{}
 
-	for _, deployment := range deployments.Items {
-		existingDeployments.Insert(deployment.Name)
-	}
-
 	placementLocations, err := locations.ListPlacementLocations(ctx, upstreamClient, r.LocationSource)
 	if err != nil {
 		return nil, nil, err
@@ -554,11 +571,7 @@ func (r *WorkloadReconciler) getDeploymentsForWorkload(
 		}
 
 		for _, locationName := range locationNames {
-			// TODO(jreese) should we use GenerateName for deployments and identify
-			// them via labels instead? Would help with race conditions on workload
-			// recreation.
-
-			deploymentName := fmt.Sprintf("%s-%s-%s", workload.Name, placement.Name, strings.ToLower(locationName))
+			deploymentName := naming.DeploymentName(workload.Name, workload.UID, placement.Name, locationName)
 			desiredDeployments.Insert(deploymentName)
 
 			desired = append(desired, computev1alpha.WorkloadDeployment{
@@ -588,12 +601,9 @@ func (r *WorkloadReconciler) getDeploymentsForWorkload(
 		}
 	}
 
-	// Collect orphans
-	for _, name := range existingDeployments.Difference(desiredDeployments).UnsortedList() {
-		for _, deployment := range deployments.Items {
-			if name == deployment.Name {
-				orphaned = append(orphaned, deployment)
-			}
+	for _, deployment := range deployments.Items {
+		if !desiredDeployments.Has(deployment.Name) {
+			orphaned = append(orphaned, deployment)
 		}
 	}
 

internal/controller/workload_controller_test.go

@@ -17,6 +17,7 @@ import (
 
 	computev1alpha "go.datum.net/compute/api/v1alpha"
 	"go.datum.net/compute/internal/locations"
+	"go.datum.net/compute/internal/naming"
 	networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha"
 	locationsv1alpha1 "go.miloapis.com/locations/api/v1alpha1"
 	servicesv1alpha1 "go.miloapis.com/service-catalog/api/v1alpha1"
@@ -408,7 +409,7 @@ func TestGetDeploymentsForWorkload_LocationSelector(t *testing.T) {
 	require.Len(t, desired, 2)
 	assert.Equal(t, "dfw-a", desired[0].Spec.LocationRef.Name)
 	assert.Equal(t, "dfw-b", desired[1].Spec.LocationRef.Name)
-	assert.Equal(t, rdTestWorkloadName+"-"+testDefaultPlacement+"-dfw-a", desired[0].Name)
+	assert.Equal(t, naming.DeploymentName(rdTestWorkloadName, "workload-uid", testDefaultPlacement, "dfw-a"), desired[0].Name)
 	assert.Equal(t, "dfw-a", desired[0].Labels[computev1alpha.LocationLabel])
 }
 
@@ -566,3 +567,135 @@ func TestGetDeploymentsForWorkload_RequiresComputeAvailability(t *testing.T) {
 		assert.Equal(t, testLocationName, deployment.Spec.LocationRef.Name)
 	}
 }
+
+func newDeploymentMatchingTestWorkload() *computev1alpha.Workload {
+	return &computev1alpha.Workload{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      rdTestWorkloadName,
+			Namespace: testDefaultNamespace,
+			UID:       types.UID("workload-uid"),
+		},
+		Spec: computev1alpha.WorkloadSpec{
+			Placements: []computev1alpha.WorkloadPlacement{{
+				Name:          testDefaultPlacement,
+				Locations:     []locationsv1alpha1.LocationReference{{Name: testLocationName}},
+				ScaleSettings: computev1alpha.HorizontalScaleSettings{MinReplicas: 1},
+			}},
+		},
+	}
+}
+
+func newExistingDeployment(name string, workloadUID types.UID, placement string) *computev1alpha.WorkloadDeployment {
+	return &computev1alpha.WorkloadDeployment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: testDefaultNamespace,
+			Labels:    map[string]string{computev1alpha.WorkloadUIDLabel: string(workloadUID)},
+		},
+		Spec: computev1alpha.WorkloadDeploymentSpec{
+			WorkloadRef:   computev1alpha.WorkloadReference{Name: rdTestWorkloadName, UID: workloadUID},
+			PlacementName: placement,
+			LocationRef:   locationsv1alpha1.LocationReference{Name: testLocationName},
+		},
+	}
+}
+
+// TestGetDeploymentsForWorkload_OrphansOtherNames verifies that only the
+// deployment carrying the expected name is kept for a placement and location.
+// A deployment of the same workload under any other name, such as one created
+// before the naming change, is orphaned and replaced.
+func TestGetDeploymentsForWorkload_OrphansOtherNames(t *testing.T) {
+	t.Parallel()
+
+	workload := newDeploymentMatchingTestWorkload()
+	expectedName := naming.DeploymentName(workload.Name, workload.UID, testDefaultPlacement, testLocationName)
+	current := newExistingDeployment(expectedName, workload.UID, testDefaultPlacement)
+	oldStyle := newExistingDeployment(rdTestWorkloadName+"-"+testDefaultPlacement+"-"+testLocationName, workload.UID, testDefaultPlacement)
+
+	cl := fake.NewClientBuilder().
+		WithScheme(newNetworkingScheme()).
+		WithObjects(
+			newTestLocationBinding(testLocationName, "DFW"),
+			newTestComputeAvailability(testLocationName),
+			current, oldStyle,
+		).
+		WithIndex(&computev1alpha.WorkloadDeployment{}, deploymentWorkloadUIDIndex, deploymentWorkloadUIDIndexFunc).
+		Build()
+	r := &WorkloadReconciler{}
+
+	desired, orphaned, err := r.getDeploymentsForWorkload(context.Background(), cl, workload)
+	require.NoError(t, err)
+	require.Len(t, desired, 1)
+	assert.Equal(t, expectedName, desired[0].Name)
+	require.Len(t, orphaned, 1)
+	assert.Equal(t, oldStyle.Name, orphaned[0].Name)
+}
+
+// TestGetDeploymentsForWorkload_ReplacesOldName verifies that a deployment
+// under an old-style name is orphaned and the expected name is desired in its
+// place.
+func TestGetDeploymentsForWorkload_ReplacesOldName(t *testing.T) {
+	t.Parallel()
+
+	workload := newDeploymentMatchingTestWorkload()
+	oldStyle := newExistingDeployment(rdTestWorkloadName+"-"+testDefaultPlacement+"-"+testLocationName, workload.UID, testDefaultPlacement)
+
+	cl := fake.NewClientBuilder().
+		WithScheme(newNetworkingScheme()).
+		WithObjects(
+			newTestLocationBinding(testLocationName, "DFW"),
+			newTestComputeAvailability(testLocationName),
+			oldStyle,
+		).
+		WithIndex(&computev1alpha.WorkloadDeployment{}, deploymentWorkloadUIDIndex, deploymentWorkloadUIDIndexFunc).
+		Build()
+	r := &WorkloadReconciler{}
+
+	desired, orphaned, err := r.getDeploymentsForWorkload(context.Background(), cl, workload)
+	require.NoError(t, err)
+	require.Len(t, desired, 1)
+	assert.Equal(t, naming.DeploymentName(workload.Name, workload.UID, testDefaultPlacement, testLocationName), desired[0].Name)
+	require.Len(t, orphaned, 1)
+	assert.Equal(t, oldStyle.Name, orphaned[0].Name)
+}
+
+// TestUpsertWorkloadDeployment_RefusesForeignWorkload verifies that a
+// deployment belonging to another workload is never overwritten.
+func TestUpsertWorkloadDeployment_RefusesForeignWorkload(t *testing.T) {
+	t.Parallel()
+
+	workload := newDeploymentMatchingTestWorkload()
+	name := naming.DeploymentName(workload.Name, workload.UID, testDefaultPlacement, testLocationName)
+	foreign := newExistingDeployment(name, types.UID("other-uid"), "other-placement")
+
+	cl := fake.NewClientBuilder().
+		WithScheme(newNetworkingScheme()).
+		WithObjects(foreign).
+		Build()
+
+	desired := newExistingDeployment(name, workload.UID, testDefaultPlacement)
+	_, err := upsertWorkloadDeployment(context.Background(), cl, workload, desired)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "other-uid")
+
+	var stored computev1alpha.WorkloadDeployment
+	require.NoError(t, cl.Get(context.Background(), client.ObjectKeyFromObject(foreign), &stored))
+	assert.Equal(t, types.UID("other-uid"), stored.Spec.WorkloadRef.UID)
+	assert.Equal(t, "other-placement", stored.Spec.PlacementName)
+}
+
+// TestUpsertWorkloadDeployment_CreatesOwnedDeployment verifies a new
+// deployment is created and controlled by its workload.
+func TestUpsertWorkloadDeployment_CreatesOwnedDeployment(t *testing.T) {
+	t.Parallel()
+
+	workload := newDeploymentMatchingTestWorkload()
+	name := naming.DeploymentName(workload.Name, workload.UID, testDefaultPlacement, testLocationName)
+	cl := fake.NewClientBuilder().WithScheme(newNetworkingScheme()).Build()
+
+	desired := newExistingDeployment(name, workload.UID, testDefaultPlacement)
+	deployment, err := upsertWorkloadDeployment(context.Background(), cl, workload, desired)
+	require.NoError(t, err)
+	assert.Equal(t, name, deployment.Name)
+	assert.True(t, metav1.IsControlledBy(deployment, workload))
+}

internal/naming/naming.go

@@ -0,0 +1,37 @@
+// SPDX-License-Identifier: AGPL-3.0-only
+
+// Package naming builds the names of the objects compute derives from a
+// workload.
+package naming
+
+import (
+	"crypto/sha256"
+	"encoding/hex"
+	"strings"
+
+	"k8s.io/apimachinery/pkg/types"
+)
+
+const (
+	deploymentNamePrefixLength = 30
+	deploymentNameHashLength   = 10
+
+	// MaxDeploymentNameLength is the longest name DeploymentName returns.
+	MaxDeploymentNameLength = deploymentNamePrefixLength + 1 + deploymentNameHashLength
+)
+
+// DeploymentName returns the name of the WorkloadDeployment that runs a
+// workload's placement at a location. The name is a readable prefix of the
+// workload name followed by a hash of the workload UID, placement and
+// location, so its length never depends on the placement or location and two
+// workloads never share a name.
+func DeploymentName(workloadName string, workloadUID types.UID, placement, location string) string {
+	prefix := workloadName
+	if len(prefix) > deploymentNamePrefixLength {
+		prefix = prefix[:deploymentNamePrefixLength]
+	}
+	prefix = strings.TrimRight(prefix, "-")
+
+	sum := sha256.Sum256([]byte(string(workloadUID) + "\x00" + placement + "\x00" + strings.ToLower(location)))
+	return prefix + "-" + hex.EncodeToString(sum[:])[:deploymentNameHashLength]
+}

internal/naming/naming_test.go

@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: AGPL-3.0-only
+
+package naming
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"k8s.io/apimachinery/pkg/api/validation"
+	"k8s.io/apimachinery/pkg/types"
+)
+
+const testUID = types.UID("0f7c9a52-3b1e-4d8e-9a61-2f4c7b8e1d05")
+
+func TestDeploymentName_Deterministic(t *testing.T) {
+	a := DeploymentName("checkout-api", testUID, "production", "us-east-1")
+	b := DeploymentName("checkout-api", testUID, "production", "us-east-1")
+	assert.Equal(t, a, b)
+	assert.True(t, strings.HasPrefix(a, "checkout-api-"))
+	assert.Len(t, a, len("checkout-api-")+deploymentNameHashLength)
+}
+
+func TestDeploymentName_LocationCaseInsensitive(t *testing.T) {
+	assert.Equal(t,
+		DeploymentName("app", testUID, "default", "US-EAST-1"),
+		DeploymentName("app", testUID, "default", "us-east-1"),
+	)
+}
+
+func TestDeploymentName_LengthBound(t *testing.T) {
+	long63 := strings.Repeat("a", 63)
+	name := DeploymentName(long63, testUID, long63, long63)
+	assert.Len(t, name, MaxDeploymentNameLength)
+	assert.LessOrEqual(t, MaxDeploymentNameLength, 41)
+	assert.Empty(t, validation.NameIsDNSLabel(name, false))
+}
+
+func TestDeploymentName_DistinctInputs(t *testing.T) {
+	assert.NotEqual(t,
+		DeploymentName("app-a", testUID, "b", "loc"),
+		DeploymentName("app", testUID, "a-b", "loc"),
+		"joining with a dash must not make different placements collide",
+	)
+	assert.NotEqual(t,
+		DeploymentName("app", testUID, "default", "loc-a"),
+		DeploymentName("app", testUID, "default", "loc-b"),
+	)
+	assert.NotEqual(t,
+		DeploymentName("app", testUID, "default", "loc"),
+		DeploymentName("app", types.UID("another-uid"), "default", "loc"),
+		"a recreated workload must get fresh names",
+	)
+}
+
+func TestDeploymentName_TrimsTrailingDash(t *testing.T) {
+	workloadName := strings.Repeat("a", 29) + "-bcd"
+	name := DeploymentName(workloadName, testUID, "default", "loc")
+	assert.True(t, strings.HasPrefix(name, strings.Repeat("a", 29)+"-"))
+	assert.NotContains(t, name, "--")
+	assert.Empty(t, validation.NameIsDNSLabel(name, false))
+}