From a03f50bef2fd569dfbf3c37ff65706284b9bd174 Mon Sep 17 00:00:00 2001 From: rpotla Date: Thu, 10 Jul 2025 16:50:57 -0400 Subject: [PATCH 1/7] add support for label to LinodeMachine --- api/v1alpha2/linodemachine_types.go | 6 +++ ...cture.cluster.x-k8s.io_linodemachines.yaml | 5 ++ ...uster.x-k8s.io_linodemachinetemplates.yaml | 5 ++ docs/src/reference/out.md | 1 + .../controller/linodemachine_controller.go | 10 ++-- .../linodemachine_controller_helpers.go | 27 +++++++++++ .../linodemachine_controller_helpers_test.go | 48 +++++++++++++++++++ .../linodemachinetemplate_controller.go | 30 +++++++----- .../linodemachinetemplate_controller_test.go | 2 +- 9 files changed, 116 insertions(+), 18 deletions(-) diff --git a/api/v1alpha2/linodemachine_types.go b/api/v1alpha2/linodemachine_types.go index 20821eeae..11e649d5e 100644 --- a/api/v1alpha2/linodemachine_types.go +++ b/api/v1alpha2/linodemachine_types.go @@ -33,6 +33,12 @@ const ( // LinodeMachineSpec defines the desired state of LinodeMachine type LinodeMachineSpec struct { + + // name of the Linode instance associated with this machine. + // the instance name is set to the name of LinodeMachine if the Label is not set / empty. + // +optional + Label string `json:"label,omitempty"` + // ProviderID is the unique identifier as specified by the cloud provider. // +optional ProviderID *string `json:"providerID,omitempty"` diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachines.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachines.yaml index 0b0bb5d64..1ba37cc5d 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachines.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachines.yaml @@ -264,6 +264,11 @@ spec: x-kubernetes-validations: - message: Value is immutable rule: self == oldSelf + label: + description: |- + name of the Linode instance associated with this machine. + the instance name is set to the name of LinodeMachine if the Label is not set / empty. + type: string osDisk: description: |- OSDisk is configuration for the root disk that includes the OS, diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachinetemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachinetemplates.yaml index 4b0577d11..b1f1eb23a 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachinetemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachinetemplates.yaml @@ -256,6 +256,11 @@ spec: x-kubernetes-validations: - message: Value is immutable rule: self == oldSelf + label: + description: |- + name of the Linode instance associated with this machine. + the instance name is set to the name of LinodeMachine if the Label is not set / empty. + type: string osDisk: description: |- OSDisk is configuration for the root disk that includes the OS, diff --git a/docs/src/reference/out.md b/docs/src/reference/out.md index fb958d16e..f8561f53d 100644 --- a/docs/src/reference/out.md +++ b/docs/src/reference/out.md @@ -597,6 +597,7 @@ _Appears in:_ | Field | Description | Default | Validation | | --- | --- | --- | --- | +| `label` _string_ | name of the Linode instance associated with this machine.
the instance name is set to the name of LinodeMachine if the Label is not set / empty. | | | | `providerID` _string_ | ProviderID is the unique identifier as specified by the cloud provider. | | | | `instanceID` _integer_ | InstanceID is the Linode instance ID for this machine. | | | | `region` _string_ | | | Required: \{\}
| diff --git a/internal/controller/linodemachine_controller.go b/internal/controller/linodemachine_controller.go index 740afe252..a92661403 100644 --- a/internal/controller/linodemachine_controller.go +++ b/internal/controller/linodemachine_controller.go @@ -21,7 +21,6 @@ import ( "errors" "fmt" "net/http" - "slices" "strings" "time" @@ -745,12 +744,11 @@ func (r *LinodeMachineReconciler) reconcileUpdate(ctx context.Context, logger lo }) } - // update the tags if needed - machineTags := getTags(machineScope, linodeInstance.Tags) - if !slices.Equal(machineTags, linodeInstance.Tags) { - _, err = machineScope.LinodeClient.UpdateInstance(ctx, instanceID, linodego.InstanceUpdateOptions{Tags: &machineTags}) + isUpdated, updateOptions := instanceHasToBeUpdated(machineScope, linodeInstance) + if isUpdated { + _, err = machineScope.LinodeClient.UpdateInstance(ctx, instanceID, updateOptions) if err != nil { - logger.Error(err, "Failed to update tags for Linode instance") + logger.Error(err, "Failed to update Linode instance") return ctrl.Result{RequeueAfter: reconciler.DefaultMachineControllerWaitForRunningDelay}, nil } } diff --git a/internal/controller/linodemachine_controller_helpers.go b/internal/controller/linodemachine_controller_helpers.go index dabc34ace..54cffb39d 100644 --- a/internal/controller/linodemachine_controller_helpers.go +++ b/internal/controller/linodemachine_controller_helpers.go @@ -25,6 +25,7 @@ import ( "fmt" "net/http" "net/netip" + "reflect" "slices" "strings" "text/template" @@ -569,6 +570,7 @@ func linodeMachineSpecToInstanceCreateConfig(machineSpec infrav1alpha2.LinodeMac privateIP = *machineSpec.PrivateIP } return &linodego.InstanceCreateOptions{ + Label: machineSpec.Label, Region: machineSpec.Region, Type: machineSpec.Type, AuthorizedKeys: machineSpec.AuthorizedKeys, @@ -1021,3 +1023,28 @@ func getTags(machineScope *scope.MachineScope, instanceTags []string) []string { machineScope.LinodeMachine.Status.Tags = slices.Clone(machineScope.LinodeMachine.Spec.Tags) return outTags } + +// This function is a placeholder for any logic that checks if the instance needs to be updated. +// It compares the current instance tags and label with the desired state from the MachineScope. +// If there are differences, it returns true and the update options that should be applied. +func instanceHasToBeUpdated(machineScope *scope.MachineScope, linodeInstance *linodego.Instance) (bool, linodego.InstanceUpdateOptions) { + + updateOptions := linodego.InstanceUpdateOptions{} + + machineTags := getTags(machineScope, linodeInstance.Tags) + if !slices.Equal(machineTags, linodeInstance.Tags) { + updateOptions.Tags = &machineTags + } + + if machineScope.LinodeMachine.Spec.Label != linodeInstance.Label { + if machineScope.LinodeMachine.Spec.Label == "" { + updateOptions.Label = machineScope.LinodeMachine.Name + } else { + updateOptions.Label = machineScope.LinodeMachine.Spec.Label + } + } + + // TODO: remove after testing + fmt.Println(reflect.DeepEqual(updateOptions, linodego.InstanceUpdateOptions{}), updateOptions) + return !reflect.DeepEqual(updateOptions, linodego.InstanceUpdateOptions{}), updateOptions +} diff --git a/internal/controller/linodemachine_controller_helpers_test.go b/internal/controller/linodemachine_controller_helpers_test.go index 884b11f36..eecee87e1 100644 --- a/internal/controller/linodemachine_controller_helpers_test.go +++ b/internal/controller/linodemachine_controller_helpers_test.go @@ -1301,3 +1301,51 @@ func TestGetTags(t *testing.T) { }) } } + +func TestInstanceHasToBeUpdated(t *testing.T) { + t.Parallel() + + // Setup test cases + testCases := []struct { + name string + machine *infrav1alpha2.LinodeMachine + instance *linodego.Instance + expectToUpdate bool + expectedUpdateOptions *linodego.InstanceUpdateOptions + }{ + { + name: "No changes - no update needed", + machine: &infrav1alpha2.LinodeMachine{ + Spec: infrav1alpha2.LinodeMachineSpec{ + Type: "g6-standard-1", + }, + }, + instance: &linodego.Instance{ + Type: "g6-standard-1", + }, + expectToUpdate: false, + }, + { + name: "Type change - update needed", + machine: &infrav1alpha2.LinodeMachine{ + Spec: infrav1alpha2.LinodeMachineSpec{ + Type: "g6-standard-2", + }, + }, + instance: &linodego.Instance{ + Type: "g6-standard-1", + }, + expectToUpdate: true, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + toUpdate := instanceHasToBeUpdated(tc.machine, tc.instance) + require.Equal(t, tc.expectToUpdate, toUpdate) + }) + } +} diff --git a/internal/controller/linodemachinetemplate_controller.go b/internal/controller/linodemachinetemplate_controller.go index f28b85eaf..74f6c38df 100644 --- a/internal/controller/linodemachinetemplate_controller.go +++ b/internal/controller/linodemachinetemplate_controller.go @@ -126,14 +126,12 @@ func (lmtr *LinodeMachineTemplateReconciler) reconcile(ctx context.Context, lmtS } machinesFoundForTemplate = true - if !slices.Equal(lmtScope.LinodeMachineTemplate.Spec.Template.Spec.Tags, lmtScope.LinodeMachineTemplate.Status.Tags) { - err := lmtr.reconcileTags(ctx, lmtScope.LinodeMachineTemplate, &machine) - if err != nil { - lmtr.Logger.Error(err, "Failed to add tags to LinodeMachine", "template", lmtScope.LinodeMachineTemplate.Name, "machine", machine.Name) - outErr = errors.Join(outErr, err) - - failureReason = "FailedToPatchLinodeMachine" - } + err := lmtr.reconcileUpdates(ctx, lmtScope.LinodeMachineTemplate, &machine) + if err != nil { + lmtr.Logger.Error(err, "Failed to add tags to LinodeMachine", "template", lmtScope.LinodeMachineTemplate.Name, "machine", machine.Name) + outErr = errors.Join(outErr, err) + + failureReason = "FailedToPatchLinodeMachine" } } @@ -152,18 +150,28 @@ func (lmtr *LinodeMachineTemplateReconciler) reconcile(ctx context.Context, lmtS return ctrl.Result{}, outErr } -func (lmtr *LinodeMachineTemplateReconciler) reconcileTags(ctx context.Context, lmt *infrav1alpha2.LinodeMachineTemplate, machine *infrav1alpha2.LinodeMachine) error { +func (lmtr *LinodeMachineTemplateReconciler) reconcileUpdates(ctx context.Context, lmt *infrav1alpha2.LinodeMachineTemplate, machine *infrav1alpha2.LinodeMachine) error { + helper, err := patch.NewHelper(machine, lmtr.Client) if err != nil { return fmt.Errorf("failed to init patch helper: %w", err) } - machine.Spec.Tags = lmt.Spec.Template.Spec.Tags + if !slices.Equal(lmt.Spec.Template.Spec.Tags, lmt.Status.Tags) { + machine.Spec.Tags = lmt.Spec.Template.Spec.Tags + lmtr.Logger.Info("Update LinodeMachine with new tags", "machine", machine.Name, "tags", lmt.Spec.Template.Spec.Tags) + + } + + if machine.Spec.Label != lmt.Spec.Template.Spec.Label { + machine.Spec.Label = lmt.Spec.Template.Spec.Label + lmtr.Logger.Info("Update LinodeMachine with new label", "machine", machine.Name, "label", lmt.Spec.Template.Spec.Label) + + } if err := helper.Patch(ctx, machine); err != nil { return fmt.Errorf("failed to patch LinodeMachine %s with new tags: %w", machine.Name, err) } - lmtr.Logger.Info("Patched LinodeMachine with new tags", "machine", machine.Name, "tags", lmt.Spec.Template.Spec.Tags) return nil } diff --git a/internal/controller/linodemachinetemplate_controller_test.go b/internal/controller/linodemachinetemplate_controller_test.go index 94df095f8..5eb826924 100644 --- a/internal/controller/linodemachinetemplate_controller_test.go +++ b/internal/controller/linodemachinetemplate_controller_test.go @@ -166,7 +166,7 @@ var _ = Describe("lifecycle", Ordered, Label("LinodeMachineTemplateReconciler", res, err := reconciler.reconcile(ctx, lmtScope) Expect(err).NotTo(HaveOccurred()) Expect(res).To(Equal(ctrl.Result{})) - Expect(mck.Logs()).To(ContainSubstring("Patched LinodeMachine with new tags")) + Expect(mck.Logs()).To(ContainSubstring("Update LinodeMachine with new tags")) // get the updated machineTemplate updatedMachineTemplate := &infrav1alpha2.LinodeMachineTemplate{} From f444fb87896ff38d8e4a66beb2345de35dcdb9a4 Mon Sep 17 00:00:00 2001 From: rpotla Date: Sun, 13 Jul 2025 16:43:55 -0400 Subject: [PATCH 2/7] add tests --- Makefile | 2 +- .../lmt-tags/assert-lm-label-addition.yaml | 8 + .../lmt-tags/assert-lm-label-removal.yaml | 6 + .../lmt-tags/chainsaw-test.yaml | 34 +++- .../lmt-tags/lmt-add-label.yaml | 9 ++ .../lmt-tags/lmt-remove-label.yaml | 9 ++ .../linodemachine_controller_helpers.go | 6 +- .../linodemachine_controller_helpers_test.go | 37 +++-- .../linodemachine_controller_test.go | 30 ++++ .../linodemachinetemplate_controller.go | 3 - .../linodemachinetemplate_controller_test.go | 152 +++++++++++------- 11 files changed, 220 insertions(+), 76 deletions(-) create mode 100644 e2e/linodemachinetemplate-controller/lmt-tags/assert-lm-label-addition.yaml create mode 100644 e2e/linodemachinetemplate-controller/lmt-tags/assert-lm-label-removal.yaml create mode 100644 e2e/linodemachinetemplate-controller/lmt-tags/lmt-add-label.yaml create mode 100644 e2e/linodemachinetemplate-controller/lmt-tags/lmt-remove-label.yaml diff --git a/Makefile b/Makefile index 3b986b4ec..bbe1f1786 100644 --- a/Makefile +++ b/Makefile @@ -360,7 +360,7 @@ S5CMD ?= $(CACHE_BIN)/s5cmd ## Tool Versions KUSTOMIZE_VERSION ?= v5.4.3 -CTLPTL_VERSION ?= v0.8.29 +CTLPTL_VERSION ?= v0.8.42 CLUSTERCTL_VERSION ?= v1.7.2 CRD_REF_DOCS_VERSION ?= v0.1.0 KUBECTL_VERSION ?= v1.28.0 diff --git a/e2e/linodemachinetemplate-controller/lmt-tags/assert-lm-label-addition.yaml b/e2e/linodemachinetemplate-controller/lmt-tags/assert-lm-label-addition.yaml new file mode 100644 index 000000000..c1c3b3d74 --- /dev/null +++ b/e2e/linodemachinetemplate-controller/lmt-tags/assert-lm-label-addition.yaml @@ -0,0 +1,8 @@ +--- +apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2 +kind: LinodeMachine +metadata: + labels: + cluster.x-k8s.io/cluster-name: ($cluster) +spec: + label: test-label diff --git a/e2e/linodemachinetemplate-controller/lmt-tags/assert-lm-label-removal.yaml b/e2e/linodemachinetemplate-controller/lmt-tags/assert-lm-label-removal.yaml new file mode 100644 index 000000000..ed56fa4c6 --- /dev/null +++ b/e2e/linodemachinetemplate-controller/lmt-tags/assert-lm-label-removal.yaml @@ -0,0 +1,6 @@ +--- +apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2 +kind: LinodeMachine +metadata: + labels: + cluster.x-k8s.io/cluster-name: ($cluster) diff --git a/e2e/linodemachinetemplate-controller/lmt-tags/chainsaw-test.yaml b/e2e/linodemachinetemplate-controller/lmt-tags/chainsaw-test.yaml index 58a6fe7af..28a2f3299 100644 --- a/e2e/linodemachinetemplate-controller/lmt-tags/chainsaw-test.yaml +++ b/e2e/linodemachinetemplate-controller/lmt-tags/chainsaw-test.yaml @@ -5,7 +5,7 @@ metadata: name: lmt-e2e labels: all: - linodemachine: + linodemachinetemplate: spec: bindings: # A short identifier for the E2E test run @@ -74,6 +74,38 @@ spec: - assert: file: assert-lm-tags-removal.yaml catch: + - describe: + apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2 + kind: LinodeMachine + - name: Add Label to template + try: + - apply: + file: lmt-add-label.yaml + catch: + - describe: + apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2 + kind: LinodeMachineTemplate + - name: Ensure label is added to machine + try: + - assert: + file: assert-lm-label-addition.yaml + catch: + - describe: + apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2 + kind: LinodeMachine + - name: Remove label from template + try: + - apply: + file: lmt-remove-label.yaml + catch: + - describe: + apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2 + kind: LinodeMachineTemplate + - name: Ensure label is removed from machine + try: + - assert: + file: assert-lm-label-removal.yaml + catch: - describe: apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2 kind: LinodeMachine \ No newline at end of file diff --git a/e2e/linodemachinetemplate-controller/lmt-tags/lmt-add-label.yaml b/e2e/linodemachinetemplate-controller/lmt-tags/lmt-add-label.yaml new file mode 100644 index 000000000..2b02a4967 --- /dev/null +++ b/e2e/linodemachinetemplate-controller/lmt-tags/lmt-add-label.yaml @@ -0,0 +1,9 @@ +apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2 +kind: LinodeMachineTemplate +metadata: + name: ($cluster) +spec: + template: + spec: + label: test-label +--- \ No newline at end of file diff --git a/e2e/linodemachinetemplate-controller/lmt-tags/lmt-remove-label.yaml b/e2e/linodemachinetemplate-controller/lmt-tags/lmt-remove-label.yaml new file mode 100644 index 000000000..c9435a9ba --- /dev/null +++ b/e2e/linodemachinetemplate-controller/lmt-tags/lmt-remove-label.yaml @@ -0,0 +1,9 @@ +apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2 +kind: LinodeMachineTemplate +metadata: + name: ($cluster) +spec: + template: + spec: + label: "" +--- \ No newline at end of file diff --git a/internal/controller/linodemachine_controller_helpers.go b/internal/controller/linodemachine_controller_helpers.go index 54cffb39d..428e9424a 100644 --- a/internal/controller/linodemachine_controller_helpers.go +++ b/internal/controller/linodemachine_controller_helpers.go @@ -25,7 +25,6 @@ import ( "fmt" "net/http" "net/netip" - "reflect" "slices" "strings" "text/template" @@ -1028,7 +1027,6 @@ func getTags(machineScope *scope.MachineScope, instanceTags []string) []string { // It compares the current instance tags and label with the desired state from the MachineScope. // If there are differences, it returns true and the update options that should be applied. func instanceHasToBeUpdated(machineScope *scope.MachineScope, linodeInstance *linodego.Instance) (bool, linodego.InstanceUpdateOptions) { - updateOptions := linodego.InstanceUpdateOptions{} machineTags := getTags(machineScope, linodeInstance.Tags) @@ -1044,7 +1042,5 @@ func instanceHasToBeUpdated(machineScope *scope.MachineScope, linodeInstance *li } } - // TODO: remove after testing - fmt.Println(reflect.DeepEqual(updateOptions, linodego.InstanceUpdateOptions{}), updateOptions) - return !reflect.DeepEqual(updateOptions, linodego.InstanceUpdateOptions{}), updateOptions + return updateOptions.Tags != nil || updateOptions.Label != "", updateOptions } diff --git a/internal/controller/linodemachine_controller_helpers_test.go b/internal/controller/linodemachine_controller_helpers_test.go index eecee87e1..0f0732968 100644 --- a/internal/controller/linodemachine_controller_helpers_test.go +++ b/internal/controller/linodemachine_controller_helpers_test.go @@ -1308,34 +1308,46 @@ func TestInstanceHasToBeUpdated(t *testing.T) { // Setup test cases testCases := []struct { name string - machine *infrav1alpha2.LinodeMachine + machineScope *scope.MachineScope instance *linodego.Instance expectToUpdate bool expectedUpdateOptions *linodego.InstanceUpdateOptions }{ { - name: "No changes - no update needed", - machine: &infrav1alpha2.LinodeMachine{ - Spec: infrav1alpha2.LinodeMachineSpec{ - Type: "g6-standard-1", + name: "no update needed: No changes", + machineScope: &scope.MachineScope{ + LinodeMachine: &infrav1alpha2.LinodeMachine{ + Spec: infrav1alpha2.LinodeMachineSpec{ + Type: "g6-standard-1", + }, }, }, instance: &linodego.Instance{ Type: "g6-standard-1", }, - expectToUpdate: false, + expectToUpdate: false, + expectedUpdateOptions: &linodego.InstanceUpdateOptions{}, }, { - name: "Type change - update needed", - machine: &infrav1alpha2.LinodeMachine{ - Spec: infrav1alpha2.LinodeMachineSpec{ - Type: "g6-standard-2", + name: "update needed: Changes in label and tags", + machineScope: &scope.MachineScope{ + LinodeMachine: &infrav1alpha2.LinodeMachine{ + Spec: infrav1alpha2.LinodeMachineSpec{ + Type: "g6-standard-2", + Label: "new-instance-label", + Tags: []string{"new-instance-tag"}, + }, }, }, instance: &linodego.Instance{ - Type: "g6-standard-1", + Type: "g6-standard-1", + Label: "test-instance", }, expectToUpdate: true, + expectedUpdateOptions: &linodego.InstanceUpdateOptions{ + Label: "new-instance-label", + Tags: &[]string{"new-instance-tag"}, + }, }, } @@ -1344,8 +1356,9 @@ func TestInstanceHasToBeUpdated(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - toUpdate := instanceHasToBeUpdated(tc.machine, tc.instance) + toUpdate, updateOptions := instanceHasToBeUpdated(tc.machineScope, tc.instance) require.Equal(t, tc.expectToUpdate, toUpdate) + require.Equal(t, *tc.expectedUpdateOptions, updateOptions) }) } } diff --git a/internal/controller/linodemachine_controller_test.go b/internal/controller/linodemachine_controller_test.go index ccb94805c..b82ff040a 100644 --- a/internal/controller/linodemachine_controller_test.go +++ b/internal/controller/linodemachine_controller_test.go @@ -1711,6 +1711,36 @@ var _ = Describe("machine-update", Ordered, Label("machine", "machine-update"), Expect(linodeMachine.Status.Tags).To(Equal([]string{"test-tag"})) }), ), + Path( + Call("machine label is updated", func(ctx context.Context, mck Mock) { + mck.LinodeClient.EXPECT().GetInstance(ctx, 11111).Return( + &linodego.Instance{ + ID: 11111, + IPv4: []*net.IP{ptr.To(net.IPv4(192, 168, 0, 2))}, + IPv6: "fd00::", + Tags: []string{"test-cluster-2"}, + Status: linodego.InstanceRunning, + Updated: util.Pointer(time.Now()), + }, nil) + mck.LinodeClient.EXPECT().UpdateInstance(ctx, 11111, gomock.Any()).Return( + &linodego.Instance{ + ID: 11111, + IPv4: []*net.IP{ptr.To(net.IPv4(192, 168, 0, 2))}, + IPv6: "fd00::", + Tags: []string{"test-cluster-2"}, + Status: linodego.InstanceRunning, + Updated: util.Pointer(time.Now()), + Label: "test-label", + }, nil) + }), + Result("machine label is updated", func(ctx context.Context, mck Mock) { + linodeMachine.Spec.ProviderID = util.Pointer("linode://11111") + linodeMachine.Status.InstanceState = util.Pointer(linodego.InstanceRunning) + linodeMachine.Spec.Label = "test-label" + _, err := reconciler.reconcile(ctx, logr.Logger{}, mScope) + Expect(err).NotTo(HaveOccurred()) + }), + ), ) }) diff --git a/internal/controller/linodemachinetemplate_controller.go b/internal/controller/linodemachinetemplate_controller.go index 74f6c38df..43556e3ae 100644 --- a/internal/controller/linodemachinetemplate_controller.go +++ b/internal/controller/linodemachinetemplate_controller.go @@ -151,7 +151,6 @@ func (lmtr *LinodeMachineTemplateReconciler) reconcile(ctx context.Context, lmtS } func (lmtr *LinodeMachineTemplateReconciler) reconcileUpdates(ctx context.Context, lmt *infrav1alpha2.LinodeMachineTemplate, machine *infrav1alpha2.LinodeMachine) error { - helper, err := patch.NewHelper(machine, lmtr.Client) if err != nil { return fmt.Errorf("failed to init patch helper: %w", err) @@ -160,13 +159,11 @@ func (lmtr *LinodeMachineTemplateReconciler) reconcileUpdates(ctx context.Contex if !slices.Equal(lmt.Spec.Template.Spec.Tags, lmt.Status.Tags) { machine.Spec.Tags = lmt.Spec.Template.Spec.Tags lmtr.Logger.Info("Update LinodeMachine with new tags", "machine", machine.Name, "tags", lmt.Spec.Template.Spec.Tags) - } if machine.Spec.Label != lmt.Spec.Template.Spec.Label { machine.Spec.Label = lmt.Spec.Template.Spec.Label lmtr.Logger.Info("Update LinodeMachine with new label", "machine", machine.Name, "label", lmt.Spec.Template.Spec.Label) - } if err := helper.Patch(ctx, machine); err != nil { diff --git a/internal/controller/linodemachinetemplate_controller_test.go b/internal/controller/linodemachinetemplate_controller_test.go index 5eb826924..72302c69d 100644 --- a/internal/controller/linodemachinetemplate_controller_test.go +++ b/internal/controller/linodemachinetemplate_controller_test.go @@ -33,6 +33,45 @@ import ( . "github.com/onsi/gomega" ) +func generateMachine(templateName, label string) infrav1alpha2.LinodeMachine { + out := infrav1alpha2.LinodeMachine{ + ObjectMeta: metav1.ObjectMeta{ + Name: templateName, + Namespace: "default", + Annotations: map[string]string{ + clusterv1.TemplateClonedFromNameAnnotation: templateName, + }, + }, + Spec: infrav1alpha2.LinodeMachineSpec{}, + } + + if label != "" { + out.Spec.Label = label + } + + return out +} + +func generateMachineTemplate(templateName string, tags []string, label string, statusTags []string) infrav1alpha2.LinodeMachineTemplate { + return infrav1alpha2.LinodeMachineTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Name: templateName, + Namespace: "default", + }, + Spec: infrav1alpha2.LinodeMachineTemplateSpec{ + Template: infrav1alpha2.LinodeMachineTemplateResource{ + Spec: infrav1alpha2.LinodeMachineSpec{ + Tags: tags, + Label: label, + }, + }, + }, + Status: infrav1alpha2.LinodeMachineTemplateStatus{ + Tags: statusTags, + }, + } +} + var _ = Describe("lifecycle", Ordered, Label("LinodeMachineTemplateReconciler", "lifecycle"), func() { suite := NewControllerSuite(GinkgoT(), mock.MockLinodeClient{}) @@ -42,63 +81,18 @@ var _ = Describe("lifecycle", Ordered, Label("LinodeMachineTemplateReconciler", // var linodeMT infrav1alpha2.LinodeMachineTemplate machineTemplates := []infrav1alpha2.LinodeMachineTemplate{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "machine-template-no-machines", - Namespace: "default", - }, - Spec: infrav1alpha2.LinodeMachineTemplateSpec{}, - }, - { - ObjectMeta: metav1.ObjectMeta{ - Name: "machine-template-with-spec-tags", - Namespace: "default", - }, - Spec: infrav1alpha2.LinodeMachineTemplateSpec{ - Template: infrav1alpha2.LinodeMachineTemplateResource{ - Spec: infrav1alpha2.LinodeMachineSpec{ - Tags: []string{"test-tag"}, - }, - }, - }, - }, - { - ObjectMeta: metav1.ObjectMeta{ - Name: "machine-template-no-tags-change", - Namespace: "default", - }, - Spec: infrav1alpha2.LinodeMachineTemplateSpec{ - Template: infrav1alpha2.LinodeMachineTemplateResource{ - Spec: infrav1alpha2.LinodeMachineSpec{ - Tags: []string{"test-tag1"}, - }, - }, - }, - Status: infrav1alpha2.LinodeMachineTemplateStatus{ - Tags: []string{"test-tag1"}, - }, - }, + generateMachineTemplate("machine-template-no-machines", nil, "", nil), + generateMachineTemplate("machine-template-with-spec-tags", []string{"test-tag"}, "", nil), + generateMachineTemplate("machine-template-no-tags-change", []string{"test-tag1"}, "test-label", []string{"test-tag1"}), + generateMachineTemplate("machine-template-label-change", nil, "test-label", nil), + generateMachineTemplate("machine-template-label-change-to-empty", nil, "", nil), } linodeMachines := []infrav1alpha2.LinodeMachine{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "machine-1", - Namespace: "default", - Annotations: map[string]string{ - clusterv1.TemplateClonedFromNameAnnotation: "machine-template-with-spec-tags", - }, - }, - }, - { - ObjectMeta: metav1.ObjectMeta{ - Name: "machine-2", - Namespace: "default", - Annotations: map[string]string{ - clusterv1.TemplateClonedFromNameAnnotation: "machine-template-no-tags-change", - }, - }, - }, + generateMachine("machine-template-with-spec-tags", ""), + generateMachine("machine-template-no-tags-change", ""), + generateMachine("machine-template-label-change", ""), + generateMachine("machine-template-label-change-to-empty", "old-label"), } BeforeAll(func(ctx SpecContext) { @@ -198,6 +192,56 @@ var _ = Describe("lifecycle", Ordered, Label("LinodeMachineTemplateReconciler", Expect(mck.Logs()).NotTo(ContainSubstring("Patched LinodeMachine with new tags")) }), ), + Path( + Call("machine template machine label update to non-empty value", func(ctx context.Context, mck Mock) {}), + Result("success", func(ctx context.Context, mck Mock) { + patchHelper, err := patch.NewHelper(&machineTemplates[3], k8sClient) + Expect(err).NotTo(HaveOccurred()) + + lmtScope = scope.MachineTemplateScope{ + PatchHelper: patchHelper, + LinodeMachineTemplate: &machineTemplates[3], + } + reconciler = LinodeMachineTemplateReconciler{ + Logger: mck.Logger(), + Client: k8sClient, + } + + res, err := reconciler.reconcile(ctx, &lmtScope) + Expect(err).NotTo(HaveOccurred()) + Expect(res).To(Equal(ctrl.Result{})) + Expect(mck.Logs()).To(ContainSubstring("Update LinodeMachine with new label")) + }), + ), + Path( + Call("machine template machine label update to empty value", func(ctx context.Context, mck Mock) {}), + Result("success", func(ctx context.Context, mck Mock) { + patchHelper, err := patch.NewHelper(&machineTemplates[4], k8sClient) + Expect(err).NotTo(HaveOccurred()) + + lmtScope = scope.MachineTemplateScope{ + PatchHelper: patchHelper, + LinodeMachineTemplate: &machineTemplates[4], + } + reconciler = LinodeMachineTemplateReconciler{ + Logger: mck.Logger(), + Client: k8sClient, + } + + res, err := reconciler.reconcile(ctx, &lmtScope) + Expect(err).NotTo(HaveOccurred()) + Expect(res).To(Equal(ctrl.Result{})) + Expect(mck.Logs()).To(ContainSubstring("Update LinodeMachine with new label")) + + // get the updated machine + updatedMachine := &infrav1alpha2.LinodeMachine{} + Expect(k8sClient.Get(ctx, client.ObjectKey{ + Name: linodeMachines[3].Name, + Namespace: linodeMachines[3].Namespace, + }, updatedMachine)).To(Succeed()) + Expect(updatedMachine.Spec.Label).To(Equal("")) + }), + ), ), ) From b30f741cd8dc00a03781d5d0accee151d2a7abb3 Mon Sep 17 00:00:00 2001 From: rpotla Date: Mon, 14 Jul 2025 11:14:19 -0400 Subject: [PATCH 3/7] tmp commit delete before push --- api/v1alpha2/linodemachine_types.go | 8 +- ...cture.cluster.x-k8s.io_linodemachines.yaml | 6 +- ...uster.x-k8s.io_linodemachinetemplates.yaml | 6 +- docs/src/reference/out.md | 2 +- .../linodemachine_controller_helpers.go | 45 ++++++++-- .../linodemachine_controller_helpers_test.go | 86 ++++++++++++++++++- .../linodemachine_controller_test.go | 2 +- .../linodemachinetemplate_controller.go | 6 +- .../linodemachinetemplate_controller_test.go | 20 ++--- 9 files changed, 145 insertions(+), 36 deletions(-) diff --git a/api/v1alpha2/linodemachine_types.go b/api/v1alpha2/linodemachine_types.go index 11e649d5e..b6e092820 100644 --- a/api/v1alpha2/linodemachine_types.go +++ b/api/v1alpha2/linodemachine_types.go @@ -34,10 +34,12 @@ const ( // LinodeMachineSpec defines the desired state of LinodeMachine type LinodeMachineSpec struct { - // name of the Linode instance associated with this machine. - // the instance name is set to the name of LinodeMachine if the Label is not set / empty. + // LabelPrefix is the prefix to use for the Linode instance label. + // If not specified, defaults are applied. + // If specified but a Machine doesn't have a owner reference, the prefix is added to the Machine name. + // If specified and a Machine has a owner reference, owner reference name is replaced with the prefix. // +optional - Label string `json:"label,omitempty"` + LabelPrefix string `json:"labelPrefix,omitempty"` // ProviderID is the unique identifier as specified by the cloud provider. // +optional diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachines.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachines.yaml index 1ba37cc5d..5b9497e74 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachines.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachines.yaml @@ -264,10 +264,10 @@ spec: x-kubernetes-validations: - message: Value is immutable rule: self == oldSelf - label: + labelPrefix: description: |- - name of the Linode instance associated with this machine. - the instance name is set to the name of LinodeMachine if the Label is not set / empty. + LabelPrefix is the prefix to use for the Linode instance label. + If not specified, defaults are applied. type: string osDisk: description: |- diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachinetemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachinetemplates.yaml index b1f1eb23a..fb547e476 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachinetemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachinetemplates.yaml @@ -256,10 +256,10 @@ spec: x-kubernetes-validations: - message: Value is immutable rule: self == oldSelf - label: + labelPrefix: description: |- - name of the Linode instance associated with this machine. - the instance name is set to the name of LinodeMachine if the Label is not set / empty. + LabelPrefix is the prefix to use for the Linode instance label. + If not specified, defaults are applied. type: string osDisk: description: |- diff --git a/docs/src/reference/out.md b/docs/src/reference/out.md index 34635e191..ebf072740 100644 --- a/docs/src/reference/out.md +++ b/docs/src/reference/out.md @@ -597,7 +597,7 @@ _Appears in:_ | Field | Description | Default | Validation | | --- | --- | --- | --- | -| `label` _string_ | name of the Linode instance associated with this machine.
the instance name is set to the name of LinodeMachine if the Label is not set / empty. | | | +| `labelPrefix` _string_ | LabelPrefix is the prefix to use for the Linode instance label.
If not specified, defaults are applied. | | | | `providerID` _string_ | ProviderID is the unique identifier as specified by the cloud provider. | | | | `instanceID` _integer_ | InstanceID is the Linode instance ID for this machine. | | | | `region` _string_ | | | Required: \{\}
| diff --git a/internal/controller/linodemachine_controller_helpers.go b/internal/controller/linodemachine_controller_helpers.go index 428e9424a..2d27b46ee 100644 --- a/internal/controller/linodemachine_controller_helpers.go +++ b/internal/controller/linodemachine_controller_helpers.go @@ -105,7 +105,7 @@ func fillCreateConfig(createConfig *linodego.InstanceCreateOptions, machineScope func newCreateConfig(ctx context.Context, machineScope *scope.MachineScope, gzipCompressionEnabled bool, logger logr.Logger) (*linodego.InstanceCreateOptions, error) { var err error - createConfig := linodeMachineSpecToInstanceCreateConfig(machineScope.LinodeMachine.Spec, getTags(machineScope, []string{})) + createConfig := linodeMachineSpecToInstanceCreateConfig(machineScope, getTags(machineScope, []string{})) if createConfig == nil { err = errors.New("failed to convert machine spec to create instance config") logger.Error(err, "Panic! Struct of LinodeMachineSpec is different than InstanceCreateOptions") @@ -552,7 +552,8 @@ func getVPCInterfaceConfigFromDirectID(ctx context.Context, machineScope *scope. }, nil } -func linodeMachineSpecToInstanceCreateConfig(machineSpec infrav1alpha2.LinodeMachineSpec, machineTags []string) *linodego.InstanceCreateOptions { +func linodeMachineSpecToInstanceCreateConfig(machineScope *scope.MachineScope, machineTags []string) *linodego.InstanceCreateOptions { + machineSpec := machineScope.LinodeMachine.Spec interfaces := make([]linodego.InstanceConfigInterfaceCreateOptions, len(machineSpec.Interfaces)) for idx, iface := range machineSpec.Interfaces { interfaces[idx] = linodego.InstanceConfigInterfaceCreateOptions{ @@ -569,7 +570,7 @@ func linodeMachineSpecToInstanceCreateConfig(machineSpec infrav1alpha2.LinodeMac privateIP = *machineSpec.PrivateIP } return &linodego.InstanceCreateOptions{ - Label: machineSpec.Label, + Label: getDesiredLinodeInstanceLabel(machineScope), Region: machineSpec.Region, Type: machineSpec.Type, AuthorizedKeys: machineSpec.AuthorizedKeys, @@ -1034,13 +1035,41 @@ func instanceHasToBeUpdated(machineScope *scope.MachineScope, linodeInstance *li updateOptions.Tags = &machineTags } - if machineScope.LinodeMachine.Spec.Label != linodeInstance.Label { - if machineScope.LinodeMachine.Spec.Label == "" { - updateOptions.Label = machineScope.LinodeMachine.Name + computedInstanceLabel := getDesiredLinodeInstanceLabel(machineScope) + if computedInstanceLabel != linodeInstance.Label { + updateOptions.Label = computedInstanceLabel + } + + return updateOptions.Tags != nil || updateOptions.Label != "", updateOptions +} + +func getDesiredLinodeInstanceLabel(machineScope *scope.MachineScope) string { + // If no label prefix is specified, use the machine name as the label + if machineScope.LinodeMachine.Spec.LabelPrefix == "" { + return machineScope.LinodeMachine.Name + } + + // if machine is created by a deployment / control-plane, it's name will be prefixed with the label of linode. + outLabel := machineScope.LinodeMachine.Name + machineOwners := machineScope.Machine.GetOwnerReferences() + + // get the longest prefix match from machine owner names. + longestPrefix := "" + for _, owner := range machineOwners { + if strings.HasPrefix(machineScope.LinodeMachine.Name, owner.Name) && len(owner.Name) > len(longestPrefix) { + longestPrefix = owner.Name + } + } + + // If no owner name matches the prefix, use the machine name as the label + if machineScope.LinodeMachine.Spec.LabelPrefix != "" { + if longestPrefix == "" { + // If no owner name matches the prefix, use the label prefix + outLabel = machineScope.LinodeMachine.Spec.LabelPrefix + "-" + machineScope.Machine.Name } else { - updateOptions.Label = machineScope.LinodeMachine.Spec.Label + outLabel = strings.Replace(machineScope.Machine.Name, longestPrefix, machineScope.LinodeMachine.Spec.LabelPrefix, 1) } } - return updateOptions.Tags != nil || updateOptions.Label != "", updateOptions + return outLabel } diff --git a/internal/controller/linodemachine_controller_helpers_test.go b/internal/controller/linodemachine_controller_helpers_test.go index 0f0732968..fa3f7b501 100644 --- a/internal/controller/linodemachine_controller_helpers_test.go +++ b/internal/controller/linodemachine_controller_helpers_test.go @@ -24,6 +24,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" "sigs.k8s.io/cluster-api/api/v1beta1" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/controller-runtime/pkg/client" infrav1alpha2 "github.com/linode/cluster-api-provider-linode/api/v1alpha2" @@ -64,7 +65,11 @@ func TestLinodeMachineSpecToCreateInstanceConfig(t *testing.T) { PrivateIP: util.Pointer(true), } - createConfig := linodeMachineSpecToInstanceCreateConfig(machineSpec, []string{"tag"}) + createConfig := linodeMachineSpecToInstanceCreateConfig(&scope.MachineScope{ + LinodeMachine: &infrav1alpha2.LinodeMachine{ + Spec: machineSpec, + }, + }, []string{"tag"}) assert.NotNil(t, createConfig, "Failed to convert LinodeMachineSpec to InstanceCreateOptions") } @@ -1333,9 +1338,9 @@ func TestInstanceHasToBeUpdated(t *testing.T) { machineScope: &scope.MachineScope{ LinodeMachine: &infrav1alpha2.LinodeMachine{ Spec: infrav1alpha2.LinodeMachineSpec{ - Type: "g6-standard-2", - Label: "new-instance-label", - Tags: []string{"new-instance-tag"}, + Type: "g6-standard-2", + LabelPrefix: "new-instance-label", + Tags: []string{"new-instance-tag"}, }, }, }, @@ -1362,3 +1367,76 @@ func TestInstanceHasToBeUpdated(t *testing.T) { }) } } + +func TestGetDesiredLinodeInstanceLabel(t *testing.T) { + t.Parallel() + + // Setup test cases + testCases := []struct { + name string + machineScope *scope.MachineScope + expectedLabel string + }{ + { + name: "Success - Default label", + machineScope: &scope.MachineScope{ + LinodeMachine: &infrav1alpha2.LinodeMachine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-machine", + Namespace: "default", + }, + }, + }, + expectedLabel: "test-machine", + }, + { + name: "Success - Custom label prefix", + machineScope: &scope.MachineScope{ + LinodeMachine: &infrav1alpha2.LinodeMachine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-machine", + Namespace: "default", + }, + Spec: infrav1alpha2.LinodeMachineSpec{ + LabelPrefix: "custom-prefix", + }, + }, + }, + expectedLabel: "custom-prefix-test-machine", + }, + { + name: "Success - Custom label prefix with owner reference", + machineScope: &scope.MachineScope{ + LinodeMachine: &infrav1alpha2.LinodeMachine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-machine", + Namespace: "default", + }, + Spec: infrav1alpha2.LinodeMachineSpec{ + LabelPrefix: "custom-prefix", + }, + }, + Machine: &clusterv1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "MachineDeployment", + }, + }, + }, + }, + }, + expectedLabel: "custom-prefix-test-machine", + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + label := getDesiredLinodeInstanceLabel(tc.machineScope) + require.Equal(t, tc.expectedLabel, label) + }) + } +} diff --git a/internal/controller/linodemachine_controller_test.go b/internal/controller/linodemachine_controller_test.go index b82ff040a..8c9851b97 100644 --- a/internal/controller/linodemachine_controller_test.go +++ b/internal/controller/linodemachine_controller_test.go @@ -1736,7 +1736,7 @@ var _ = Describe("machine-update", Ordered, Label("machine", "machine-update"), Result("machine label is updated", func(ctx context.Context, mck Mock) { linodeMachine.Spec.ProviderID = util.Pointer("linode://11111") linodeMachine.Status.InstanceState = util.Pointer(linodego.InstanceRunning) - linodeMachine.Spec.Label = "test-label" + linodeMachine.Spec.LabelPrefix = "test-label" _, err := reconciler.reconcile(ctx, logr.Logger{}, mScope) Expect(err).NotTo(HaveOccurred()) }), diff --git a/internal/controller/linodemachinetemplate_controller.go b/internal/controller/linodemachinetemplate_controller.go index 43556e3ae..2d5a7e1f4 100644 --- a/internal/controller/linodemachinetemplate_controller.go +++ b/internal/controller/linodemachinetemplate_controller.go @@ -161,9 +161,9 @@ func (lmtr *LinodeMachineTemplateReconciler) reconcileUpdates(ctx context.Contex lmtr.Logger.Info("Update LinodeMachine with new tags", "machine", machine.Name, "tags", lmt.Spec.Template.Spec.Tags) } - if machine.Spec.Label != lmt.Spec.Template.Spec.Label { - machine.Spec.Label = lmt.Spec.Template.Spec.Label - lmtr.Logger.Info("Update LinodeMachine with new label", "machine", machine.Name, "label", lmt.Spec.Template.Spec.Label) + if machine.Spec.LabelPrefix != lmt.Spec.Template.Spec.LabelPrefix { + machine.Spec.LabelPrefix = lmt.Spec.Template.Spec.LabelPrefix + lmtr.Logger.Info("Update LinodeMachine with new label prefix", "machine", machine.Name, "labelPrefix", lmt.Spec.Template.Spec.LabelPrefix) } if err := helper.Patch(ctx, machine); err != nil { diff --git a/internal/controller/linodemachinetemplate_controller_test.go b/internal/controller/linodemachinetemplate_controller_test.go index 72302c69d..af156113d 100644 --- a/internal/controller/linodemachinetemplate_controller_test.go +++ b/internal/controller/linodemachinetemplate_controller_test.go @@ -33,7 +33,7 @@ import ( . "github.com/onsi/gomega" ) -func generateMachine(templateName, label string) infrav1alpha2.LinodeMachine { +func generateMachine(templateName, labelPrefix string) infrav1alpha2.LinodeMachine { out := infrav1alpha2.LinodeMachine{ ObjectMeta: metav1.ObjectMeta{ Name: templateName, @@ -45,14 +45,14 @@ func generateMachine(templateName, label string) infrav1alpha2.LinodeMachine { Spec: infrav1alpha2.LinodeMachineSpec{}, } - if label != "" { - out.Spec.Label = label + if labelPrefix != "" { + out.Spec.LabelPrefix = labelPrefix } return out } -func generateMachineTemplate(templateName string, tags []string, label string, statusTags []string) infrav1alpha2.LinodeMachineTemplate { +func generateMachineTemplate(templateName string, tags []string, labelPrefix string, statusTags []string) infrav1alpha2.LinodeMachineTemplate { return infrav1alpha2.LinodeMachineTemplate{ ObjectMeta: metav1.ObjectMeta{ Name: templateName, @@ -61,8 +61,8 @@ func generateMachineTemplate(templateName string, tags []string, label string, s Spec: infrav1alpha2.LinodeMachineTemplateSpec{ Template: infrav1alpha2.LinodeMachineTemplateResource{ Spec: infrav1alpha2.LinodeMachineSpec{ - Tags: tags, - Label: label, + Tags: tags, + LabelPrefix: labelPrefix, }, }, }, @@ -189,7 +189,7 @@ var _ = Describe("lifecycle", Ordered, Label("LinodeMachineTemplateReconciler", res, err := reconciler.reconcile(ctx, &lmtScope) Expect(err).NotTo(HaveOccurred()) Expect(res).To(Equal(ctrl.Result{})) - Expect(mck.Logs()).NotTo(ContainSubstring("Patched LinodeMachine with new tags")) + Expect(mck.Logs()).NotTo(ContainSubstring("Update LinodeMachine with new tags")) }), ), Path( @@ -210,7 +210,7 @@ var _ = Describe("lifecycle", Ordered, Label("LinodeMachineTemplateReconciler", res, err := reconciler.reconcile(ctx, &lmtScope) Expect(err).NotTo(HaveOccurred()) Expect(res).To(Equal(ctrl.Result{})) - Expect(mck.Logs()).To(ContainSubstring("Update LinodeMachine with new label")) + Expect(mck.Logs()).To(ContainSubstring("Update LinodeMachine with new label prefix")) }), ), Path( @@ -231,7 +231,7 @@ var _ = Describe("lifecycle", Ordered, Label("LinodeMachineTemplateReconciler", res, err := reconciler.reconcile(ctx, &lmtScope) Expect(err).NotTo(HaveOccurred()) Expect(res).To(Equal(ctrl.Result{})) - Expect(mck.Logs()).To(ContainSubstring("Update LinodeMachine with new label")) + Expect(mck.Logs()).To(ContainSubstring("Update LinodeMachine with new label prefix")) // get the updated machine updatedMachine := &infrav1alpha2.LinodeMachine{} @@ -239,7 +239,7 @@ var _ = Describe("lifecycle", Ordered, Label("LinodeMachineTemplateReconciler", Name: linodeMachines[3].Name, Namespace: linodeMachines[3].Namespace, }, updatedMachine)).To(Succeed()) - Expect(updatedMachine.Spec.Label).To(Equal("")) + Expect(updatedMachine.Spec.LabelPrefix).To(Equal("")) }), ), ), From 030710c81f9c5a08d08c69f11c7e66057def3826 Mon Sep 17 00:00:00 2001 From: rpotla Date: Mon, 14 Jul 2025 17:38:29 -0400 Subject: [PATCH 4/7] Fix tests and linter --- ...cture.cluster.x-k8s.io_linodemachines.yaml | 2 ++ ...uster.x-k8s.io_linodemachinetemplates.yaml | 2 ++ docs/src/reference/out.md | 2 +- .../linodemachine_controller_helpers.go | 4 ++-- .../linodemachine_controller_helpers_test.go | 19 +++++++++++++++---- .../linodemachine_controller_test.go | 2 ++ 6 files changed, 24 insertions(+), 7 deletions(-) diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachines.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachines.yaml index 5b9497e74..ec58604a1 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachines.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachines.yaml @@ -268,6 +268,8 @@ spec: description: |- LabelPrefix is the prefix to use for the Linode instance label. If not specified, defaults are applied. + If specified but a Machine doesn't have a owner reference, the prefix is added to the Machine name. + If specified and a Machine has a owner reference, owner reference name is replaced with the prefix. type: string osDisk: description: |- diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachinetemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachinetemplates.yaml index fb547e476..cfccb38a8 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachinetemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachinetemplates.yaml @@ -260,6 +260,8 @@ spec: description: |- LabelPrefix is the prefix to use for the Linode instance label. If not specified, defaults are applied. + If specified but a Machine doesn't have a owner reference, the prefix is added to the Machine name. + If specified and a Machine has a owner reference, owner reference name is replaced with the prefix. type: string osDisk: description: |- diff --git a/docs/src/reference/out.md b/docs/src/reference/out.md index ebf072740..c5f591b44 100644 --- a/docs/src/reference/out.md +++ b/docs/src/reference/out.md @@ -597,7 +597,7 @@ _Appears in:_ | Field | Description | Default | Validation | | --- | --- | --- | --- | -| `labelPrefix` _string_ | LabelPrefix is the prefix to use for the Linode instance label.
If not specified, defaults are applied. | | | +| `labelPrefix` _string_ | LabelPrefix is the prefix to use for the Linode instance label.
If not specified, defaults are applied.
If specified but a Machine doesn't have a owner reference, the prefix is added to the Machine name.
If specified and a Machine has a owner reference, owner reference name is replaced with the prefix. | | | | `providerID` _string_ | ProviderID is the unique identifier as specified by the cloud provider. | | | | `instanceID` _integer_ | InstanceID is the Linode instance ID for this machine. | | | | `region` _string_ | | | Required: \{\}
| diff --git a/internal/controller/linodemachine_controller_helpers.go b/internal/controller/linodemachine_controller_helpers.go index 2d27b46ee..76ca8b9e8 100644 --- a/internal/controller/linodemachine_controller_helpers.go +++ b/internal/controller/linodemachine_controller_helpers.go @@ -1065,9 +1065,9 @@ func getDesiredLinodeInstanceLabel(machineScope *scope.MachineScope) string { if machineScope.LinodeMachine.Spec.LabelPrefix != "" { if longestPrefix == "" { // If no owner name matches the prefix, use the label prefix - outLabel = machineScope.LinodeMachine.Spec.LabelPrefix + "-" + machineScope.Machine.Name + outLabel = machineScope.LinodeMachine.Spec.LabelPrefix + "-" + machineScope.LinodeMachine.Name } else { - outLabel = strings.Replace(machineScope.Machine.Name, longestPrefix, machineScope.LinodeMachine.Spec.LabelPrefix, 1) + outLabel = strings.Replace(machineScope.LinodeMachine.Name, longestPrefix, machineScope.LinodeMachine.Spec.LabelPrefix, 1) } } diff --git a/internal/controller/linodemachine_controller_helpers_test.go b/internal/controller/linodemachine_controller_helpers_test.go index fa3f7b501..b12ac5fd4 100644 --- a/internal/controller/linodemachine_controller_helpers_test.go +++ b/internal/controller/linodemachine_controller_helpers_test.go @@ -24,7 +24,6 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" "sigs.k8s.io/cluster-api/api/v1beta1" - clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/controller-runtime/pkg/client" infrav1alpha2 "github.com/linode/cluster-api-provider-linode/api/v1alpha2" @@ -1326,6 +1325,7 @@ func TestInstanceHasToBeUpdated(t *testing.T) { Type: "g6-standard-1", }, }, + Machine: &v1beta1.Machine{}, }, instance: &linodego.Instance{ Type: "g6-standard-1", @@ -1337,12 +1337,16 @@ func TestInstanceHasToBeUpdated(t *testing.T) { name: "update needed: Changes in label and tags", machineScope: &scope.MachineScope{ LinodeMachine: &infrav1alpha2.LinodeMachine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-instance", + }, Spec: infrav1alpha2.LinodeMachineSpec{ Type: "g6-standard-2", LabelPrefix: "new-instance-label", Tags: []string{"new-instance-tag"}, }, }, + Machine: &v1beta1.Machine{}, }, instance: &linodego.Instance{ Type: "g6-standard-1", @@ -1350,7 +1354,7 @@ func TestInstanceHasToBeUpdated(t *testing.T) { }, expectToUpdate: true, expectedUpdateOptions: &linodego.InstanceUpdateOptions{ - Label: "new-instance-label", + Label: "new-instance-label-test-instance", Tags: &[]string{"new-instance-tag"}, }, }, @@ -1386,6 +1390,7 @@ func TestGetDesiredLinodeInstanceLabel(t *testing.T) { Namespace: "default", }, }, + Machine: &v1beta1.Machine{}, }, expectedLabel: "test-machine", }, @@ -1401,6 +1406,7 @@ func TestGetDesiredLinodeInstanceLabel(t *testing.T) { LabelPrefix: "custom-prefix", }, }, + Machine: &v1beta1.Machine{}, }, expectedLabel: "custom-prefix-test-machine", }, @@ -1416,17 +1422,22 @@ func TestGetDesiredLinodeInstanceLabel(t *testing.T) { LabelPrefix: "custom-prefix", }, }, - Machine: &clusterv1.Machine{ + Machine: &v1beta1.Machine{ ObjectMeta: metav1.ObjectMeta{ OwnerReferences: []metav1.OwnerReference{ { Kind: "MachineDeployment", + Name: "te", + }, + { + Kind: "MachineSet", + Name: "test", }, }, }, }, }, - expectedLabel: "custom-prefix-test-machine", + expectedLabel: "custom-prefix-machine", }, } diff --git a/internal/controller/linodemachine_controller_test.go b/internal/controller/linodemachine_controller_test.go index 8c9851b97..185ed6b09 100644 --- a/internal/controller/linodemachine_controller_test.go +++ b/internal/controller/linodemachine_controller_test.go @@ -1651,6 +1651,7 @@ var _ = Describe("machine-update", Ordered, Label("machine", "machine-update"), mck.LinodeClient.EXPECT().GetInstance(ctx, 11111).Return( &linodego.Instance{ ID: 11111, + Label: "machine-update", IPv4: []*net.IP{ptr.To(net.IPv4(192, 168, 0, 2))}, IPv6: "fd00::", Tags: []string{"test-cluster-2"}, @@ -1668,6 +1669,7 @@ var _ = Describe("machine-update", Ordered, Label("machine", "machine-update"), mck.LinodeClient.EXPECT().GetInstance(ctx, 11111).Return( &linodego.Instance{ + Label: "machine-update", ID: 11111, IPv4: []*net.IP{ptr.To(net.IPv4(192, 168, 0, 2))}, IPv6: "fd00::", From f67191b94b61138063a2474ca11f2d55dbd75acc Mon Sep 17 00:00:00 2001 From: rpotla Date: Tue, 15 Jul 2025 19:04:06 -0400 Subject: [PATCH 5/7] revert propagating labelprefix --- .../controller/linodemachine_controller.go | 9 +- .../linodemachine_controller_helpers.go | 42 +++-- .../linodemachine_controller_helpers_test.go | 66 -------- .../linodemachine_controller_test.go | 32 ---- .../linodemachinetemplate_controller.go | 27 ++- .../linodemachinetemplate_controller_test.go | 156 +++++++----------- 6 files changed, 91 insertions(+), 241 deletions(-) diff --git a/internal/controller/linodemachine_controller.go b/internal/controller/linodemachine_controller.go index a92661403..b8c2bb745 100644 --- a/internal/controller/linodemachine_controller.go +++ b/internal/controller/linodemachine_controller.go @@ -744,11 +744,12 @@ func (r *LinodeMachineReconciler) reconcileUpdate(ctx context.Context, logger lo }) } - isUpdated, updateOptions := instanceHasToBeUpdated(machineScope, linodeInstance) - if isUpdated { - _, err = machineScope.LinodeClient.UpdateInstance(ctx, instanceID, updateOptions) + // update the tags if needed + machineTags := getTags(machineScope, linodeInstance.Tags) + if !areSlicesEqual(machineTags, linodeInstance.Tags) { + _, err = machineScope.LinodeClient.UpdateInstance(ctx, instanceID, linodego.InstanceUpdateOptions{Tags: &machineTags}) if err != nil { - logger.Error(err, "Failed to update Linode instance") + logger.Error(err, "Failed to update tags for Linode instance") return ctrl.Result{RequeueAfter: reconciler.DefaultMachineControllerWaitForRunningDelay}, nil } } diff --git a/internal/controller/linodemachine_controller_helpers.go b/internal/controller/linodemachine_controller_helpers.go index 76ca8b9e8..ab921c67e 100644 --- a/internal/controller/linodemachine_controller_helpers.go +++ b/internal/controller/linodemachine_controller_helpers.go @@ -1024,23 +1024,21 @@ func getTags(machineScope *scope.MachineScope, instanceTags []string) []string { return outTags } -// This function is a placeholder for any logic that checks if the instance needs to be updated. -// It compares the current instance tags and label with the desired state from the MachineScope. -// If there are differences, it returns true and the update options that should be applied. -func instanceHasToBeUpdated(machineScope *scope.MachineScope, linodeInstance *linodego.Instance) (bool, linodego.InstanceUpdateOptions) { - updateOptions := linodego.InstanceUpdateOptions{} - - machineTags := getTags(machineScope, linodeInstance.Tags) - if !slices.Equal(machineTags, linodeInstance.Tags) { - updateOptions.Tags = &machineTags - } - - computedInstanceLabel := getDesiredLinodeInstanceLabel(machineScope) - if computedInstanceLabel != linodeInstance.Label { - updateOptions.Label = computedInstanceLabel +func areSlicesEqual(a, b []string) bool { + if len(a) != len(b) { + return false + } + aSet := constructSet(a) + bSet := constructSet(b) + if len(aSet) != len(bSet) { + return false + } + for key := range aSet { + if _, ok := bSet[key]; !ok { + return false + } } - - return updateOptions.Tags != nil || updateOptions.Label != "", updateOptions + return true } func getDesiredLinodeInstanceLabel(machineScope *scope.MachineScope) string { @@ -1062,13 +1060,11 @@ func getDesiredLinodeInstanceLabel(machineScope *scope.MachineScope) string { } // If no owner name matches the prefix, use the machine name as the label - if machineScope.LinodeMachine.Spec.LabelPrefix != "" { - if longestPrefix == "" { - // If no owner name matches the prefix, use the label prefix - outLabel = machineScope.LinodeMachine.Spec.LabelPrefix + "-" + machineScope.LinodeMachine.Name - } else { - outLabel = strings.Replace(machineScope.LinodeMachine.Name, longestPrefix, machineScope.LinodeMachine.Spec.LabelPrefix, 1) - } + if longestPrefix == "" { + // If no owner name matches the prefix, use the label prefix + outLabel = machineScope.LinodeMachine.Spec.LabelPrefix + "-" + machineScope.LinodeMachine.Name + } else { + outLabel = strings.Replace(machineScope.LinodeMachine.Name, longestPrefix, machineScope.LinodeMachine.Spec.LabelPrefix, 1) } return outLabel diff --git a/internal/controller/linodemachine_controller_helpers_test.go b/internal/controller/linodemachine_controller_helpers_test.go index b12ac5fd4..91b542b63 100644 --- a/internal/controller/linodemachine_controller_helpers_test.go +++ b/internal/controller/linodemachine_controller_helpers_test.go @@ -1306,72 +1306,6 @@ func TestGetTags(t *testing.T) { } } -func TestInstanceHasToBeUpdated(t *testing.T) { - t.Parallel() - - // Setup test cases - testCases := []struct { - name string - machineScope *scope.MachineScope - instance *linodego.Instance - expectToUpdate bool - expectedUpdateOptions *linodego.InstanceUpdateOptions - }{ - { - name: "no update needed: No changes", - machineScope: &scope.MachineScope{ - LinodeMachine: &infrav1alpha2.LinodeMachine{ - Spec: infrav1alpha2.LinodeMachineSpec{ - Type: "g6-standard-1", - }, - }, - Machine: &v1beta1.Machine{}, - }, - instance: &linodego.Instance{ - Type: "g6-standard-1", - }, - expectToUpdate: false, - expectedUpdateOptions: &linodego.InstanceUpdateOptions{}, - }, - { - name: "update needed: Changes in label and tags", - machineScope: &scope.MachineScope{ - LinodeMachine: &infrav1alpha2.LinodeMachine{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-instance", - }, - Spec: infrav1alpha2.LinodeMachineSpec{ - Type: "g6-standard-2", - LabelPrefix: "new-instance-label", - Tags: []string{"new-instance-tag"}, - }, - }, - Machine: &v1beta1.Machine{}, - }, - instance: &linodego.Instance{ - Type: "g6-standard-1", - Label: "test-instance", - }, - expectToUpdate: true, - expectedUpdateOptions: &linodego.InstanceUpdateOptions{ - Label: "new-instance-label-test-instance", - Tags: &[]string{"new-instance-tag"}, - }, - }, - } - - for _, tc := range testCases { - tc := tc - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - - toUpdate, updateOptions := instanceHasToBeUpdated(tc.machineScope, tc.instance) - require.Equal(t, tc.expectToUpdate, toUpdate) - require.Equal(t, *tc.expectedUpdateOptions, updateOptions) - }) - } -} - func TestGetDesiredLinodeInstanceLabel(t *testing.T) { t.Parallel() diff --git a/internal/controller/linodemachine_controller_test.go b/internal/controller/linodemachine_controller_test.go index 185ed6b09..ccb94805c 100644 --- a/internal/controller/linodemachine_controller_test.go +++ b/internal/controller/linodemachine_controller_test.go @@ -1651,7 +1651,6 @@ var _ = Describe("machine-update", Ordered, Label("machine", "machine-update"), mck.LinodeClient.EXPECT().GetInstance(ctx, 11111).Return( &linodego.Instance{ ID: 11111, - Label: "machine-update", IPv4: []*net.IP{ptr.To(net.IPv4(192, 168, 0, 2))}, IPv6: "fd00::", Tags: []string{"test-cluster-2"}, @@ -1669,7 +1668,6 @@ var _ = Describe("machine-update", Ordered, Label("machine", "machine-update"), mck.LinodeClient.EXPECT().GetInstance(ctx, 11111).Return( &linodego.Instance{ - Label: "machine-update", ID: 11111, IPv4: []*net.IP{ptr.To(net.IPv4(192, 168, 0, 2))}, IPv6: "fd00::", @@ -1713,36 +1711,6 @@ var _ = Describe("machine-update", Ordered, Label("machine", "machine-update"), Expect(linodeMachine.Status.Tags).To(Equal([]string{"test-tag"})) }), ), - Path( - Call("machine label is updated", func(ctx context.Context, mck Mock) { - mck.LinodeClient.EXPECT().GetInstance(ctx, 11111).Return( - &linodego.Instance{ - ID: 11111, - IPv4: []*net.IP{ptr.To(net.IPv4(192, 168, 0, 2))}, - IPv6: "fd00::", - Tags: []string{"test-cluster-2"}, - Status: linodego.InstanceRunning, - Updated: util.Pointer(time.Now()), - }, nil) - mck.LinodeClient.EXPECT().UpdateInstance(ctx, 11111, gomock.Any()).Return( - &linodego.Instance{ - ID: 11111, - IPv4: []*net.IP{ptr.To(net.IPv4(192, 168, 0, 2))}, - IPv6: "fd00::", - Tags: []string{"test-cluster-2"}, - Status: linodego.InstanceRunning, - Updated: util.Pointer(time.Now()), - Label: "test-label", - }, nil) - }), - Result("machine label is updated", func(ctx context.Context, mck Mock) { - linodeMachine.Spec.ProviderID = util.Pointer("linode://11111") - linodeMachine.Status.InstanceState = util.Pointer(linodego.InstanceRunning) - linodeMachine.Spec.LabelPrefix = "test-label" - _, err := reconciler.reconcile(ctx, logr.Logger{}, mScope) - Expect(err).NotTo(HaveOccurred()) - }), - ), ) }) diff --git a/internal/controller/linodemachinetemplate_controller.go b/internal/controller/linodemachinetemplate_controller.go index 2d5a7e1f4..f28b85eaf 100644 --- a/internal/controller/linodemachinetemplate_controller.go +++ b/internal/controller/linodemachinetemplate_controller.go @@ -126,12 +126,14 @@ func (lmtr *LinodeMachineTemplateReconciler) reconcile(ctx context.Context, lmtS } machinesFoundForTemplate = true - err := lmtr.reconcileUpdates(ctx, lmtScope.LinodeMachineTemplate, &machine) - if err != nil { - lmtr.Logger.Error(err, "Failed to add tags to LinodeMachine", "template", lmtScope.LinodeMachineTemplate.Name, "machine", machine.Name) - outErr = errors.Join(outErr, err) - - failureReason = "FailedToPatchLinodeMachine" + if !slices.Equal(lmtScope.LinodeMachineTemplate.Spec.Template.Spec.Tags, lmtScope.LinodeMachineTemplate.Status.Tags) { + err := lmtr.reconcileTags(ctx, lmtScope.LinodeMachineTemplate, &machine) + if err != nil { + lmtr.Logger.Error(err, "Failed to add tags to LinodeMachine", "template", lmtScope.LinodeMachineTemplate.Name, "machine", machine.Name) + outErr = errors.Join(outErr, err) + + failureReason = "FailedToPatchLinodeMachine" + } } } @@ -150,25 +152,18 @@ func (lmtr *LinodeMachineTemplateReconciler) reconcile(ctx context.Context, lmtS return ctrl.Result{}, outErr } -func (lmtr *LinodeMachineTemplateReconciler) reconcileUpdates(ctx context.Context, lmt *infrav1alpha2.LinodeMachineTemplate, machine *infrav1alpha2.LinodeMachine) error { +func (lmtr *LinodeMachineTemplateReconciler) reconcileTags(ctx context.Context, lmt *infrav1alpha2.LinodeMachineTemplate, machine *infrav1alpha2.LinodeMachine) error { helper, err := patch.NewHelper(machine, lmtr.Client) if err != nil { return fmt.Errorf("failed to init patch helper: %w", err) } - if !slices.Equal(lmt.Spec.Template.Spec.Tags, lmt.Status.Tags) { - machine.Spec.Tags = lmt.Spec.Template.Spec.Tags - lmtr.Logger.Info("Update LinodeMachine with new tags", "machine", machine.Name, "tags", lmt.Spec.Template.Spec.Tags) - } - - if machine.Spec.LabelPrefix != lmt.Spec.Template.Spec.LabelPrefix { - machine.Spec.LabelPrefix = lmt.Spec.Template.Spec.LabelPrefix - lmtr.Logger.Info("Update LinodeMachine with new label prefix", "machine", machine.Name, "labelPrefix", lmt.Spec.Template.Spec.LabelPrefix) - } + machine.Spec.Tags = lmt.Spec.Template.Spec.Tags if err := helper.Patch(ctx, machine); err != nil { return fmt.Errorf("failed to patch LinodeMachine %s with new tags: %w", machine.Name, err) } + lmtr.Logger.Info("Patched LinodeMachine with new tags", "machine", machine.Name, "tags", lmt.Spec.Template.Spec.Tags) return nil } diff --git a/internal/controller/linodemachinetemplate_controller_test.go b/internal/controller/linodemachinetemplate_controller_test.go index af156113d..94df095f8 100644 --- a/internal/controller/linodemachinetemplate_controller_test.go +++ b/internal/controller/linodemachinetemplate_controller_test.go @@ -33,45 +33,6 @@ import ( . "github.com/onsi/gomega" ) -func generateMachine(templateName, labelPrefix string) infrav1alpha2.LinodeMachine { - out := infrav1alpha2.LinodeMachine{ - ObjectMeta: metav1.ObjectMeta{ - Name: templateName, - Namespace: "default", - Annotations: map[string]string{ - clusterv1.TemplateClonedFromNameAnnotation: templateName, - }, - }, - Spec: infrav1alpha2.LinodeMachineSpec{}, - } - - if labelPrefix != "" { - out.Spec.LabelPrefix = labelPrefix - } - - return out -} - -func generateMachineTemplate(templateName string, tags []string, labelPrefix string, statusTags []string) infrav1alpha2.LinodeMachineTemplate { - return infrav1alpha2.LinodeMachineTemplate{ - ObjectMeta: metav1.ObjectMeta{ - Name: templateName, - Namespace: "default", - }, - Spec: infrav1alpha2.LinodeMachineTemplateSpec{ - Template: infrav1alpha2.LinodeMachineTemplateResource{ - Spec: infrav1alpha2.LinodeMachineSpec{ - Tags: tags, - LabelPrefix: labelPrefix, - }, - }, - }, - Status: infrav1alpha2.LinodeMachineTemplateStatus{ - Tags: statusTags, - }, - } -} - var _ = Describe("lifecycle", Ordered, Label("LinodeMachineTemplateReconciler", "lifecycle"), func() { suite := NewControllerSuite(GinkgoT(), mock.MockLinodeClient{}) @@ -81,18 +42,63 @@ var _ = Describe("lifecycle", Ordered, Label("LinodeMachineTemplateReconciler", // var linodeMT infrav1alpha2.LinodeMachineTemplate machineTemplates := []infrav1alpha2.LinodeMachineTemplate{ - generateMachineTemplate("machine-template-no-machines", nil, "", nil), - generateMachineTemplate("machine-template-with-spec-tags", []string{"test-tag"}, "", nil), - generateMachineTemplate("machine-template-no-tags-change", []string{"test-tag1"}, "test-label", []string{"test-tag1"}), - generateMachineTemplate("machine-template-label-change", nil, "test-label", nil), - generateMachineTemplate("machine-template-label-change-to-empty", nil, "", nil), + { + ObjectMeta: metav1.ObjectMeta{ + Name: "machine-template-no-machines", + Namespace: "default", + }, + Spec: infrav1alpha2.LinodeMachineTemplateSpec{}, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "machine-template-with-spec-tags", + Namespace: "default", + }, + Spec: infrav1alpha2.LinodeMachineTemplateSpec{ + Template: infrav1alpha2.LinodeMachineTemplateResource{ + Spec: infrav1alpha2.LinodeMachineSpec{ + Tags: []string{"test-tag"}, + }, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "machine-template-no-tags-change", + Namespace: "default", + }, + Spec: infrav1alpha2.LinodeMachineTemplateSpec{ + Template: infrav1alpha2.LinodeMachineTemplateResource{ + Spec: infrav1alpha2.LinodeMachineSpec{ + Tags: []string{"test-tag1"}, + }, + }, + }, + Status: infrav1alpha2.LinodeMachineTemplateStatus{ + Tags: []string{"test-tag1"}, + }, + }, } linodeMachines := []infrav1alpha2.LinodeMachine{ - generateMachine("machine-template-with-spec-tags", ""), - generateMachine("machine-template-no-tags-change", ""), - generateMachine("machine-template-label-change", ""), - generateMachine("machine-template-label-change-to-empty", "old-label"), + { + ObjectMeta: metav1.ObjectMeta{ + Name: "machine-1", + Namespace: "default", + Annotations: map[string]string{ + clusterv1.TemplateClonedFromNameAnnotation: "machine-template-with-spec-tags", + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "machine-2", + Namespace: "default", + Annotations: map[string]string{ + clusterv1.TemplateClonedFromNameAnnotation: "machine-template-no-tags-change", + }, + }, + }, } BeforeAll(func(ctx SpecContext) { @@ -160,7 +166,7 @@ var _ = Describe("lifecycle", Ordered, Label("LinodeMachineTemplateReconciler", res, err := reconciler.reconcile(ctx, lmtScope) Expect(err).NotTo(HaveOccurred()) Expect(res).To(Equal(ctrl.Result{})) - Expect(mck.Logs()).To(ContainSubstring("Update LinodeMachine with new tags")) + Expect(mck.Logs()).To(ContainSubstring("Patched LinodeMachine with new tags")) // get the updated machineTemplate updatedMachineTemplate := &infrav1alpha2.LinodeMachineTemplate{} @@ -189,57 +195,7 @@ var _ = Describe("lifecycle", Ordered, Label("LinodeMachineTemplateReconciler", res, err := reconciler.reconcile(ctx, &lmtScope) Expect(err).NotTo(HaveOccurred()) Expect(res).To(Equal(ctrl.Result{})) - Expect(mck.Logs()).NotTo(ContainSubstring("Update LinodeMachine with new tags")) - }), - ), - Path( - Call("machine template machine label update to non-empty value", func(ctx context.Context, mck Mock) {}), - Result("success", func(ctx context.Context, mck Mock) { - patchHelper, err := patch.NewHelper(&machineTemplates[3], k8sClient) - Expect(err).NotTo(HaveOccurred()) - - lmtScope = scope.MachineTemplateScope{ - PatchHelper: patchHelper, - LinodeMachineTemplate: &machineTemplates[3], - } - reconciler = LinodeMachineTemplateReconciler{ - Logger: mck.Logger(), - Client: k8sClient, - } - - res, err := reconciler.reconcile(ctx, &lmtScope) - Expect(err).NotTo(HaveOccurred()) - Expect(res).To(Equal(ctrl.Result{})) - Expect(mck.Logs()).To(ContainSubstring("Update LinodeMachine with new label prefix")) - }), - ), - Path( - Call("machine template machine label update to empty value", func(ctx context.Context, mck Mock) {}), - Result("success", func(ctx context.Context, mck Mock) { - patchHelper, err := patch.NewHelper(&machineTemplates[4], k8sClient) - Expect(err).NotTo(HaveOccurred()) - - lmtScope = scope.MachineTemplateScope{ - PatchHelper: patchHelper, - LinodeMachineTemplate: &machineTemplates[4], - } - reconciler = LinodeMachineTemplateReconciler{ - Logger: mck.Logger(), - Client: k8sClient, - } - - res, err := reconciler.reconcile(ctx, &lmtScope) - Expect(err).NotTo(HaveOccurred()) - Expect(res).To(Equal(ctrl.Result{})) - Expect(mck.Logs()).To(ContainSubstring("Update LinodeMachine with new label prefix")) - - // get the updated machine - updatedMachine := &infrav1alpha2.LinodeMachine{} - Expect(k8sClient.Get(ctx, client.ObjectKey{ - Name: linodeMachines[3].Name, - Namespace: linodeMachines[3].Namespace, - }, updatedMachine)).To(Succeed()) - Expect(updatedMachine.Spec.LabelPrefix).To(Equal("")) + Expect(mck.Logs()).NotTo(ContainSubstring("Patched LinodeMachine with new tags")) }), ), ), From 7781fec95f8f312554b50d8b56289fcd6eeebb79 Mon Sep 17 00:00:00 2001 From: rpotla Date: Tue, 15 Jul 2025 19:38:42 -0400 Subject: [PATCH 6/7] revert test changes --- .../lmt-tags/assert-lm-label-addition.yaml | 8 ----- .../lmt-tags/assert-lm-label-removal.yaml | 6 ---- .../lmt-tags/chainsaw-test.yaml | 32 ------------------- .../lmt-tags/lmt-add-label.yaml | 9 ------ .../lmt-tags/lmt-remove-label.yaml | 9 ------ .../linodemachine_controller_helpers_test.go | 2 ++ 6 files changed, 2 insertions(+), 64 deletions(-) delete mode 100644 e2e/linodemachinetemplate-controller/lmt-tags/assert-lm-label-addition.yaml delete mode 100644 e2e/linodemachinetemplate-controller/lmt-tags/assert-lm-label-removal.yaml delete mode 100644 e2e/linodemachinetemplate-controller/lmt-tags/lmt-add-label.yaml delete mode 100644 e2e/linodemachinetemplate-controller/lmt-tags/lmt-remove-label.yaml diff --git a/e2e/linodemachinetemplate-controller/lmt-tags/assert-lm-label-addition.yaml b/e2e/linodemachinetemplate-controller/lmt-tags/assert-lm-label-addition.yaml deleted file mode 100644 index c1c3b3d74..000000000 --- a/e2e/linodemachinetemplate-controller/lmt-tags/assert-lm-label-addition.yaml +++ /dev/null @@ -1,8 +0,0 @@ ---- -apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2 -kind: LinodeMachine -metadata: - labels: - cluster.x-k8s.io/cluster-name: ($cluster) -spec: - label: test-label diff --git a/e2e/linodemachinetemplate-controller/lmt-tags/assert-lm-label-removal.yaml b/e2e/linodemachinetemplate-controller/lmt-tags/assert-lm-label-removal.yaml deleted file mode 100644 index ed56fa4c6..000000000 --- a/e2e/linodemachinetemplate-controller/lmt-tags/assert-lm-label-removal.yaml +++ /dev/null @@ -1,6 +0,0 @@ ---- -apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2 -kind: LinodeMachine -metadata: - labels: - cluster.x-k8s.io/cluster-name: ($cluster) diff --git a/e2e/linodemachinetemplate-controller/lmt-tags/chainsaw-test.yaml b/e2e/linodemachinetemplate-controller/lmt-tags/chainsaw-test.yaml index 28a2f3299..acc7129ad 100644 --- a/e2e/linodemachinetemplate-controller/lmt-tags/chainsaw-test.yaml +++ b/e2e/linodemachinetemplate-controller/lmt-tags/chainsaw-test.yaml @@ -77,35 +77,3 @@ spec: - describe: apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2 kind: LinodeMachine - - name: Add Label to template - try: - - apply: - file: lmt-add-label.yaml - catch: - - describe: - apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2 - kind: LinodeMachineTemplate - - name: Ensure label is added to machine - try: - - assert: - file: assert-lm-label-addition.yaml - catch: - - describe: - apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2 - kind: LinodeMachine - - name: Remove label from template - try: - - apply: - file: lmt-remove-label.yaml - catch: - - describe: - apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2 - kind: LinodeMachineTemplate - - name: Ensure label is removed from machine - try: - - assert: - file: assert-lm-label-removal.yaml - catch: - - describe: - apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2 - kind: LinodeMachine \ No newline at end of file diff --git a/e2e/linodemachinetemplate-controller/lmt-tags/lmt-add-label.yaml b/e2e/linodemachinetemplate-controller/lmt-tags/lmt-add-label.yaml deleted file mode 100644 index 2b02a4967..000000000 --- a/e2e/linodemachinetemplate-controller/lmt-tags/lmt-add-label.yaml +++ /dev/null @@ -1,9 +0,0 @@ -apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2 -kind: LinodeMachineTemplate -metadata: - name: ($cluster) -spec: - template: - spec: - label: test-label ---- \ No newline at end of file diff --git a/e2e/linodemachinetemplate-controller/lmt-tags/lmt-remove-label.yaml b/e2e/linodemachinetemplate-controller/lmt-tags/lmt-remove-label.yaml deleted file mode 100644 index c9435a9ba..000000000 --- a/e2e/linodemachinetemplate-controller/lmt-tags/lmt-remove-label.yaml +++ /dev/null @@ -1,9 +0,0 @@ -apiVersion: infrastructure.cluster.x-k8s.io/v1alpha2 -kind: LinodeMachineTemplate -metadata: - name: ($cluster) -spec: - template: - spec: - label: "" ---- \ No newline at end of file diff --git a/internal/controller/linodemachine_controller_helpers_test.go b/internal/controller/linodemachine_controller_helpers_test.go index 91b542b63..921e9bafe 100644 --- a/internal/controller/linodemachine_controller_helpers_test.go +++ b/internal/controller/linodemachine_controller_helpers_test.go @@ -44,6 +44,7 @@ func TestLinodeMachineSpecToCreateInstanceConfig(t *testing.T) { RootPass: "rootPass", AuthorizedKeys: []string{"key"}, AuthorizedUsers: []string{"user"}, + LabelPrefix: "test-label-prefix", BackupID: 1, Image: "image", Interfaces: []infrav1alpha2.InstanceConfigInterfaceCreateOptions{ @@ -68,6 +69,7 @@ func TestLinodeMachineSpecToCreateInstanceConfig(t *testing.T) { LinodeMachine: &infrav1alpha2.LinodeMachine{ Spec: machineSpec, }, + Machine: &v1beta1.Machine{}, }, []string{"tag"}) assert.NotNil(t, createConfig, "Failed to convert LinodeMachineSpec to InstanceCreateOptions") } From 7db0ffe93f35e7f7f0f9c57d2a533f529a589d61 Mon Sep 17 00:00:00 2001 From: rpotla Date: Tue, 15 Jul 2025 21:12:03 -0400 Subject: [PATCH 7/7] simplify code --- internal/controller/linodemachine_controller_helpers.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/internal/controller/linodemachine_controller_helpers.go b/internal/controller/linodemachine_controller_helpers.go index ab921c67e..c62b1613e 100644 --- a/internal/controller/linodemachine_controller_helpers.go +++ b/internal/controller/linodemachine_controller_helpers.go @@ -1048,7 +1048,6 @@ func getDesiredLinodeInstanceLabel(machineScope *scope.MachineScope) string { } // if machine is created by a deployment / control-plane, it's name will be prefixed with the label of linode. - outLabel := machineScope.LinodeMachine.Name machineOwners := machineScope.Machine.GetOwnerReferences() // get the longest prefix match from machine owner names. @@ -1062,10 +1061,8 @@ func getDesiredLinodeInstanceLabel(machineScope *scope.MachineScope) string { // If no owner name matches the prefix, use the machine name as the label if longestPrefix == "" { // If no owner name matches the prefix, use the label prefix - outLabel = machineScope.LinodeMachine.Spec.LabelPrefix + "-" + machineScope.LinodeMachine.Name + return machineScope.LinodeMachine.Spec.LabelPrefix + "-" + machineScope.LinodeMachine.Name } else { - outLabel = strings.Replace(machineScope.LinodeMachine.Name, longestPrefix, machineScope.LinodeMachine.Spec.LabelPrefix, 1) + return strings.Replace(machineScope.LinodeMachine.Name, longestPrefix, machineScope.LinodeMachine.Spec.LabelPrefix, 1) } - - return outLabel }