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{