From 2864aef5a01672ed74269e5c63563e946035b3c9 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Fri, 25 Sep 2026 15:37:40 -0500 Subject: [PATCH] refactor: read instance identity from labels, not names WorkloadDeployment names are moving to a truncated workload name plus a hash, so nothing may recover an ordinal or a deployment by splitting an instance name. Code now reads identity from labels and owner references. Key changes: - The stateful strategy reads each existing instance's ordinal from the instance-index label and uses the loop index for instances it builds. - Instances with a missing or invalid ordinal label, or a duplicate ordinal, are deleted ahead of other actions so a replacement can take the name. Previously a non-numeric suffix indexed the slice at -1. - New instances get their own copy of the template labels. The shared map let the last ordinal written overwrite every other instance's label, which only went unnoticed because ordering came from the name. - The CLI resolves an instance's deployment from the workload-deployment-name label or the WorkloadDeployment owner reference, and drops the name-splitting fallback. --- internal/cmd/compute/instances/instances.go | 48 +++------ .../cmd/compute/instances/instances_test.go | 60 ++++++++++++ .../stateful/stateful_control.go | 34 ++++--- .../stateful/stateful_control_test.go | 97 +++++++++++++++++++ .../stateful/stateful_control_util.go | 24 ++--- .../stateful/stateful_control_util_test.go | 49 ++++++---- .../workloaddeployment_location_test.go | 5 +- 7 files changed, 239 insertions(+), 78 deletions(-) create mode 100644 internal/cmd/compute/instances/instances_test.go diff --git a/internal/cmd/compute/instances/instances.go b/internal/cmd/compute/instances/instances.go index 0a37ff84..e56dc7f7 100644 --- a/internal/cmd/compute/instances/instances.go +++ b/internal/cmd/compute/instances/instances.go @@ -5,7 +5,6 @@ import ( "fmt" "sort" "strings" - "unicode" "github.com/spf13/cobra" corev1 "k8s.io/api/core/v1" @@ -166,13 +165,7 @@ func runList(cmd *cobra.Command, opts *listOptions) error { wlName = labelWLName } else { // At least one label absent — fall back to WorkloadDeployment lookup. - // Prefer the explicit WorkloadDeploymentNameLabel; fall back to - // deriving the WD name from the Instance name for existing instances - // that predate the label. - depName := inst.Labels[computev1alpha.WorkloadDeploymentNameLabel] - if depName == "" { - depName = wdNameFromInstanceName(inst.Name) - } + depName := deploymentNameForInstance(&inst) if dep, ok := deploymentMap[depName]; ok { location = dep.Spec.LocationRef.Name if labelWLName != "" { @@ -325,12 +318,7 @@ func runDescribe(cmd *cobra.Command, args []string) error { placementName = labelPlacement } else { // At least one label absent — fall back to WorkloadDeployment Get. - // Prefer the WorkloadDeploymentNameLabel; fall back to deriving the WD - // name from the Instance name for existing instances that lack the label. - depName := inst.Labels[computev1alpha.WorkloadDeploymentNameLabel] - if depName == "" { - depName = wdNameFromInstanceName(inst.Name) - } + depName := deploymentNameForInstance(&inst) if depName != "" { var dep computev1alpha.WorkloadDeployment if err := c.Get(ctx, types.NamespacedName{Namespace: util.ResourceNamespace, Name: depName}, &dep); err == nil { @@ -450,30 +438,20 @@ func networkSummary(ifaces []computev1alpha.InstanceNetworkInterfaceStatus) stri return fmt.Sprintf("External: %s Internal: %s", extIP, intIP) } -// wdNameFromInstanceName derives the WorkloadDeployment name from an Instance -// name by stripping the trailing "-" suffix. Instance names follow the -// convention "-" (e.g. "my-api-default-dfw-0" → "my-api-default-dfw"). -// This is used as a fallback when WorkloadDeploymentNameLabel is absent on older -// instances that predate that label. -// -// If the name has no trailing numeric segment (not a standard instance name), -// the original name is returned unchanged so callers can handle it gracefully. -func wdNameFromInstanceName(instanceName string) string { - idx := strings.LastIndex(instanceName, "-") - if idx < 0 { - return instanceName +// deploymentNameForInstance returns the name of the WorkloadDeployment that +// owns the instance, read from WorkloadDeploymentNameLabel or, failing that, +// the instance's WorkloadDeployment owner reference. Returns "" when neither is +// set. +func deploymentNameForInstance(inst *computev1alpha.Instance) string { + if name := inst.Labels[computev1alpha.WorkloadDeploymentNameLabel]; name != "" { + return name } - suffix := instanceName[idx+1:] - // The suffix must be entirely numeric digits to qualify as an ordinal. - for _, r := range suffix { - if !unicode.IsDigit(r) { - return instanceName + for _, owner := range inst.OwnerReferences { + if owner.Kind == "WorkloadDeployment" && owner.APIVersion == computev1alpha.GroupVersion.String() { + return owner.Name } } - if suffix == "" { - return instanceName - } - return instanceName[:idx] + return "" } // formatEnvVar renders a single EnvVar for display. diff --git a/internal/cmd/compute/instances/instances_test.go b/internal/cmd/compute/instances/instances_test.go new file mode 100644 index 00000000..9c092b36 --- /dev/null +++ b/internal/cmd/compute/instances/instances_test.go @@ -0,0 +1,60 @@ +package instances + +import ( + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + computev1alpha "go.datum.net/compute/api/v1alpha" +) + +func TestDeploymentNameForInstance(t *testing.T) { + const deploymentName = "checkout-api-7c1e04b9a2" + + ownerRef := func(apiVersion, kind, name string) metav1.OwnerReference { + return metav1.OwnerReference{APIVersion: apiVersion, Kind: kind, Name: name} + } + + tests := []struct { + name string + labels map[string]string + owners []metav1.OwnerReference + want string + }{ + { + name: "label", + labels: map[string]string{computev1alpha.WorkloadDeploymentNameLabel: deploymentName}, + owners: []metav1.OwnerReference{ownerRef(computev1alpha.GroupVersion.String(), "WorkloadDeployment", "other")}, + want: deploymentName, + }, + { + name: "owner reference", + owners: []metav1.OwnerReference{ownerRef(computev1alpha.GroupVersion.String(), "WorkloadDeployment", deploymentName)}, + want: deploymentName, + }, + { + name: "owner of another kind", + owners: []metav1.OwnerReference{ownerRef("apps/v1", "WorkloadDeployment", deploymentName)}, + want: "", + }, + { + name: "name is never parsed", + want: "", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + inst := &computev1alpha.Instance{ + ObjectMeta: metav1.ObjectMeta{ + Name: deploymentName + "-0", + Labels: test.labels, + OwnerReferences: test.owners, + }, + } + if got := deploymentNameForInstance(inst); got != test.want { + t.Errorf("deploymentNameForInstance() = %q, want %q", got, test.want) + } + }) + } +} diff --git a/internal/controller/instancecontrol/stateful/stateful_control.go b/internal/controller/instancecontrol/stateful/stateful_control.go index ea497e6d..8e27e010 100644 --- a/internal/controller/instancecontrol/stateful/stateful_control.go +++ b/internal/controller/instancecontrol/stateful/stateful_control.go @@ -3,6 +3,7 @@ package stateful import ( "context" "fmt" + "maps" "slices" "strconv" @@ -74,15 +75,24 @@ func (c *statefulControl) GetActions( // highest -> lowest var deleteActions []instancecontrol.Action + // Instances that cannot hold a slot. Deleted before anything else so a + // replacement can take the name. + var strayActions []instancecontrol.Action + // Instances that are desired to exist. We do not currently support the // concept of a partition, so will fill the entire slice. desiredInstances := make([]*v1alpha.Instance, desiredReplicas) for _, instance := range currentInstances { - instanceIndex := getInstanceOrdinal(instance.Name) - if instanceIndex >= len(desiredInstances) { + instanceIndex := getInstanceOrdinal(&instance) + switch { + case instanceIndex < 0: + strayActions = append(strayActions, instancecontrol.NewDeleteAction(&instance)) + case instanceIndex >= len(desiredInstances): deleteActions = append(deleteActions, instancecontrol.NewDeleteAction(&instance)) - } else { + case desiredInstances[instanceIndex] != nil: + strayActions = append(strayActions, instancecontrol.NewDeleteAction(&instance)) + default: desiredInstances[instanceIndex] = &instance } } @@ -93,7 +103,7 @@ func (c *statefulControl) GetActions( if desiredInstances[i] == nil { desiredInstances[i] = &v1alpha.Instance{ ObjectMeta: metav1.ObjectMeta{ - Labels: deployment.Spec.Template.Labels, + Labels: maps.Clone(deployment.Spec.Template.Labels), Annotations: deployment.Spec.Template.Annotations, Name: fmt.Sprintf("%s-%d", deployment.Name, i), Namespace: deployment.Namespace, @@ -128,7 +138,7 @@ func (c *statefulControl) GetActions( SchedulingGates: gates, } - addInstanceControllerLabels(desiredInstances[i], getInstanceOrdinal(desiredInstances[i].Name), deployment) + addInstanceControllerLabels(desiredInstances[i], int(i), deployment) if err := controllerutil.SetControllerReference(deployment, desiredInstances[i], scheme); err != nil { return nil, fmt.Errorf("failed to set controller reference: %w", err) @@ -177,13 +187,13 @@ func (c *statefulControl) GetActions( // and is emitted outside the ordered rollout decision so it never gates or // reorders instance creation/updates. var patchLabelActions []instancecontrol.Action - for _, instance := range desiredInstances { + for i, instance := range desiredInstances { if instance.CreationTimestamp.IsZero() || !instance.DeletionTimestamp.IsZero() { // Skip instances that don't exist yet or are being deleted. continue } - desiredLabels := desiredControllerLabels(getInstanceOrdinal(instance.Name), deployment) + desiredLabels := desiredControllerLabels(i, deployment) if labelsNeedBackfill(instance.Labels, desiredLabels) { base := instance.DeepCopy() patched := instance.DeepCopy() @@ -200,7 +210,7 @@ func (c *statefulControl) GetActions( slices.SortFunc(recreateActions, descendingOrdinal) slices.SortFunc(deleteActions, descendingOrdinal) - actions := make([]instancecontrol.Action, 0, len(createActions)+len(waitActions)+len(recreateActions)+len(deleteActions)+len(patchLabelActions)) + actions := make([]instancecontrol.Action, 0, len(strayActions)+len(createActions)+len(waitActions)+len(recreateActions)+len(deleteActions)+len(patchLabelActions)) switch deployment.Spec.ScaleSettings.InstanceManagementPolicy { case v1alpha.OrderedReadyInstanceManagementPolicyType: @@ -210,11 +220,11 @@ func (c *statefulControl) GetActions( // // For instance, we may have instance 0 that needs to wait to be ready, but // instance 1 wants to be created. - actions = append(actions, createActions...) - actions = append(actions, waitActions...) - - slices.SortFunc(actions, ascendingOrdinal) + ordered := slices.Concat(createActions, waitActions) + slices.SortFunc(ordered, ascendingOrdinal) + actions = append(actions, strayActions...) + actions = append(actions, ordered...) actions = append(actions, recreateActions...) actions = append(actions, deleteActions...) diff --git a/internal/controller/instancecontrol/stateful/stateful_control_test.go b/internal/controller/instancecontrol/stateful/stateful_control_test.go index 3105db5d..4f5333bd 100644 --- a/internal/controller/instancecontrol/stateful/stateful_control_test.go +++ b/internal/controller/instancecontrol/stateful/stateful_control_test.go @@ -710,3 +710,100 @@ func getInstanceTemplate(name string, ordinal int) *v1alpha.Instance { return instance } + +// TestOrdinal_ReadFromLabelNotName verifies that an existing instance fills the +// slot recorded in its ordinal label, whatever its name ends with. +func TestOrdinal_ReadFromLabelNotName(t *testing.T) { + ctx := context.Background() + control := NewWithOptions(Options{}) + + deployment := getWorkloadDeployment("test-ordinal-label", 2) + + instance := getInstanceForDeployment(deployment, 1) + instance.Name = "test-ordinal-label-renamed" + + actions, err := control.GetActions(ctx, scheme, deployment, deployment.Spec.ScaleSettings.MinReplicas, []v1alpha.Instance{*instance}) + require.NoError(t, err) + + for _, a := range actions { + assert.NotEqual(t, instancecontrol.ActionTypeDelete, a.ActionType(), + "an instance with a valid ordinal label must keep its slot") + } + require.Len(t, actions, 1) + assert.Equal(t, instancecontrol.ActionTypeCreate, actions[0].ActionType()) + assert.Equal(t, "test-ordinal-label-0", actions[0].Object.GetName()) +} + +// TestOrdinal_InvalidLabelDeleted verifies that an instance without a usable +// ordinal label is deleted instead of being placed by its name. +func TestOrdinal_InvalidLabelDeleted(t *testing.T) { + for _, value := range []string{"", "foo", "-1"} { + t.Run(fmt.Sprintf("label=%q", value), func(t *testing.T) { + ctx := context.Background() + control := NewWithOptions(Options{}) + + deployment := getWorkloadDeployment("test-ordinal-invalid", 1) + + instance := getInstanceForDeployment(deployment, 0) + if value == "" { + delete(instance.Labels, v1alpha.InstanceIndexLabel) + } else { + instance.Labels[v1alpha.InstanceIndexLabel] = value + } + + actions, err := control.GetActions(ctx, scheme, deployment, deployment.Spec.ScaleSettings.MinReplicas, []v1alpha.Instance{*instance}) + require.NoError(t, err) + + // The delete must run first: the replacement reuses the same name. + require.Len(t, actions, 2) + assert.Equal(t, instancecontrol.ActionTypeDelete, actions[0].ActionType()) + assert.Equal(t, "test-ordinal-invalid-0", actions[0].Object.GetName()) + assert.False(t, actions[0].IsSkipped()) + assert.Equal(t, instancecontrol.ActionTypeCreate, actions[1].ActionType()) + assert.Equal(t, "test-ordinal-invalid-0", actions[1].Object.GetName()) + assert.True(t, actions[1].IsSkipped()) + }) + } +} + +// TestOrdinal_DuplicateLabelDeleted verifies that when two instances claim the +// same ordinal, only one keeps the slot and the other is deleted. +func TestOrdinal_DuplicateLabelDeleted(t *testing.T) { + ctx := context.Background() + control := NewWithOptions(Options{}) + + deployment := getWorkloadDeployment("test-ordinal-duplicate", 1) + + first := getInstanceForDeployment(deployment, 0) + second := getInstanceForDeployment(deployment, 0) + second.Name = "test-ordinal-duplicate-other" + + actions, err := control.GetActions(ctx, scheme, deployment, deployment.Spec.ScaleSettings.MinReplicas, []v1alpha.Instance{*first, *second}) + require.NoError(t, err) + + require.Len(t, actions, 1) + assert.Equal(t, instancecontrol.ActionTypeDelete, actions[0].ActionType()) + assert.Equal(t, "test-ordinal-duplicate-other", actions[0].Object.GetName()) + assert.False(t, actions[0].IsSkipped()) +} + +// TestOrdinal_NewInstancesDoNotShareTemplateLabels verifies that each new +// instance gets its own label map, so the ordinal stamped on one instance does +// not leak onto another or onto the deployment template. +func TestOrdinal_NewInstancesDoNotShareTemplateLabels(t *testing.T) { + ctx := context.Background() + control := NewWithOptions(Options{}) + + deployment := getWorkloadDeployment("test-ordinal-template", 3) + deployment.Spec.Template.Labels = map[string]string{"app": "checkout"} + + actions, err := control.GetActions(ctx, scheme, deployment, deployment.Spec.ScaleSettings.MinReplicas, nil) + require.NoError(t, err) + require.Len(t, actions, 3) + + for i, a := range actions { + assert.Equal(t, strconv.Itoa(i), a.Object.GetLabels()[v1alpha.InstanceIndexLabel]) + assert.Equal(t, "checkout", a.Object.GetLabels()["app"]) + } + assert.Equal(t, map[string]string{"app": "checkout"}, deployment.Spec.Template.Labels) +} diff --git a/internal/controller/instancecontrol/stateful/stateful_control_util.go b/internal/controller/instancecontrol/stateful/stateful_control_util.go index a6157d7c..10083713 100644 --- a/internal/controller/instancecontrol/stateful/stateful_control_util.go +++ b/internal/controller/instancecontrol/stateful/stateful_control_util.go @@ -2,7 +2,8 @@ package stateful import ( "strconv" - "strings" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "go.datum.net/compute/api/v1alpha" "go.datum.net/compute/internal/controller/instancecontrol" @@ -13,24 +14,25 @@ func needsUpdate(instance *v1alpha.Instance, instanceTemplateHash string) bool { instance.Spec.Controller.TemplateHash != instanceTemplateHash } -// getInstanceOrdinal returns the ordinal of the instance, or -1 if the instance -// does not have an ordinal. -func getInstanceOrdinal(name string) int { - lastDash := strings.LastIndex(name, "-") - if lastDash == -1 { +// getInstanceOrdinal returns the ordinal recorded in the instance's +// InstanceIndexLabel, or -1 if the label is absent or not a non-negative +// integer. +func getInstanceOrdinal(obj metav1.Object) int { + value, ok := obj.GetLabels()[v1alpha.InstanceIndexLabel] + if !ok { return -1 } - ordinal := -1 - if i, err := strconv.Atoi(name[lastDash+1:]); err == nil { - ordinal = i + ordinal, err := strconv.Atoi(value) + if err != nil || ordinal < 0 { + return -1 } return ordinal } func ascendingOrdinal(a, b instancecontrol.Action) int { - if getInstanceOrdinal(a.Object.GetName()) < getInstanceOrdinal(b.Object.GetName()) { + if getInstanceOrdinal(a.Object) < getInstanceOrdinal(b.Object) { return -1 } else { return 1 @@ -38,7 +40,7 @@ func ascendingOrdinal(a, b instancecontrol.Action) int { } func descendingOrdinal(a, b instancecontrol.Action) int { - if getInstanceOrdinal(a.Object.GetName()) > getInstanceOrdinal(b.Object.GetName()) { + if getInstanceOrdinal(a.Object) > getInstanceOrdinal(b.Object) { return -1 } else { return 1 diff --git a/internal/controller/instancecontrol/stateful/stateful_control_util_test.go b/internal/controller/instancecontrol/stateful/stateful_control_util_test.go index 48c7bd7b..7ee0559b 100644 --- a/internal/controller/instancecontrol/stateful/stateful_control_util_test.go +++ b/internal/controller/instancecontrol/stateful/stateful_control_util_test.go @@ -4,6 +4,7 @@ import ( "fmt" "math/rand/v2" "slices" + "strconv" "testing" "github.com/stretchr/testify/assert" @@ -15,37 +16,48 @@ import ( func TestGetInstanceOrdinal(t *testing.T) { tests := []struct { - name string - objectName string - want int + name string + labels map[string]string + want int }{ { - name: "instance with ordinal 0", - objectName: "my-instance-0", - want: 0, + name: "ordinal 0", + labels: map[string]string{v1alpha.InstanceIndexLabel: "0"}, + want: 0, }, { - name: "instance with ordinal 1", - objectName: "my-instance-1", - want: 1, + name: "ordinal 12", + labels: map[string]string{v1alpha.InstanceIndexLabel: "12"}, + want: 12, }, { - name: "instance with unexpected suffix", - objectName: "my-instance-foo", - want: -1, + name: "missing label", + labels: map[string]string{}, + want: -1, }, { - name: "instance with no dash in name", - objectName: "myinstance", - want: -1, + name: "non-numeric label", + labels: map[string]string{v1alpha.InstanceIndexLabel: "foo"}, + want: -1, + }, + { + name: "negative label", + labels: map[string]string{v1alpha.InstanceIndexLabel: "-1"}, + want: -1, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - got := getInstanceOrdinal(test.objectName) + instance := &v1alpha.Instance{ + ObjectMeta: metav1.ObjectMeta{ + Name: "checkout-api-7c1e04b9a2-3", + Labels: test.labels, + }, + } + got := getInstanceOrdinal(instance) if got != test.want { - t.Errorf("getInstanceOrdinal(%q) = %d, want %d", test.objectName, got, test.want) + t.Errorf("getInstanceOrdinal(%v) = %d, want %d", test.labels, got, test.want) } }) } @@ -59,7 +71,8 @@ func TestDescendingOrdinal(t *testing.T) { actions = append(actions, instancecontrol.NewWaitAction( &v1alpha.Instance{ ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("my-instance-%d", perm[i]), + Name: fmt.Sprintf("my-instance-%d", perm[i]), + Labels: map[string]string{v1alpha.InstanceIndexLabel: strconv.Itoa(perm[i])}, }, }, )) diff --git a/internal/controller/workloaddeployment_location_test.go b/internal/controller/workloaddeployment_location_test.go index 497a1dd9..e40b9d62 100644 --- a/internal/controller/workloaddeployment_location_test.go +++ b/internal/controller/workloaddeployment_location_test.go @@ -227,8 +227,8 @@ func newLocationTestReconcilableWD(name string) *computev1alpha.WorkloadDeployme } // newLocationTestInstance builds an instance shaped the way the instance-control -// strategy creates it: ordinal name, deployment UID label, and the scheduling -// gates stamped at creation. The CreationTimestamp (which the fake client does +// strategy creates it: ordinal name and label, deployment UID label, and the +// scheduling gates stamped at creation. The CreationTimestamp (which the fake client does // not stamp on Create) keeps the strategy in its wait path. func newLocationTestInstance(deployment *computev1alpha.WorkloadDeployment) *computev1alpha.Instance { return &computev1alpha.Instance{ @@ -238,6 +238,7 @@ func newLocationTestInstance(deployment *computev1alpha.WorkloadDeployment) *com CreationTimestamp: metav1.Now(), Labels: map[string]string{ computev1alpha.WorkloadDeploymentUIDLabel: string(deployment.UID), + computev1alpha.InstanceIndexLabel: "0", }, }, Spec: computev1alpha.InstanceSpec{