From 87d7f4ca7bab0fa9780b548af197bd479278adfb Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Mon, 16 Sep 2024 12:01:04 +0200 Subject: [PATCH] review fixes --- .../machinedeployment_controller.go | 5 +++-- .../machinedeployment_controller_test.go | 22 ++++--------------- .../machineset/machineset_controller.go | 3 ++- .../machineset/machineset_controller_test.go | 20 ++++++----------- util/log/log.go | 16 +++++++++----- 5 files changed, 27 insertions(+), 39 deletions(-) diff --git a/internal/controllers/machinedeployment/machinedeployment_controller.go b/internal/controllers/machinedeployment/machinedeployment_controller.go index d7e26a78989f..fe9e663b442f 100644 --- a/internal/controllers/machinedeployment/machinedeployment_controller.go +++ b/internal/controllers/machinedeployment/machinedeployment_controller.go @@ -313,7 +313,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, md *clusterv1.MachineD for _, ms := range msList { if ms.DeletionTimestamp.IsZero() { log.Info("Deleting MachineSet", "MachineSet", klog.KObj(ms)) - if err := r.Client.Delete(ctx, ms); err != nil { + if err := r.Client.Delete(ctx, ms); err != nil && !apierrors.IsNotFound(err) { return errors.Wrapf(err, "failed to delete MachineSet %s", klog.KObj(ms)) } } @@ -335,7 +335,8 @@ func (r *Reconciler) getAndAdoptMachineSetsForDeployment(ctx context.Context, md filtered := make([]*clusterv1.MachineSet, 0, len(machineSets.Items)) for idx := range machineSets.Items { ms := &machineSets.Items[idx] - log.WithValues("MachineSet", klog.KObj(ms)) + log := log.WithValues("MachineSet", klog.KObj(ms)) + ctx := ctrl.LoggerInto(ctx, log) selector, err := metav1.LabelSelectorAsSelector(&md.Spec.Selector) if err != nil { log.Error(err, "Skipping MachineSet, failed to get label selector from spec selector") diff --git a/internal/controllers/machinedeployment/machinedeployment_controller_test.go b/internal/controllers/machinedeployment/machinedeployment_controller_test.go index ca9bfd99ad33..b3911d2cb8a3 100644 --- a/internal/controllers/machinedeployment/machinedeployment_controller_test.go +++ b/internal/controllers/machinedeployment/machinedeployment_controller_test.go @@ -18,7 +18,6 @@ package machinedeployment import ( "context" - "sort" "testing" "time" @@ -1001,12 +1000,15 @@ func TestReconciler_reconcileDelete(t *testing.T) { "some": "labelselector", } md := builder.MachineDeployment("default", "md0").WithClusterName("test").Build() + md.Finalizers = []string{ + clusterv1.MachineDeploymentFinalizer, + } md.DeletionTimestamp = ptr.To(metav1.Now()) md.Spec.Selector = metav1.LabelSelector{ MatchLabels: labels, } mdWithoutFinalizer := md.DeepCopy() - mdWithoutFinalizer.Finalizers = nil + mdWithoutFinalizer.Finalizers = []string{} tests := []struct { name string machineDeployment *clusterv1.MachineDeployment @@ -1057,7 +1059,6 @@ func TestReconciler_reconcileDelete(t *testing.T) { Client: c, recorder: record.NewFakeRecorder(32), } - // Mark machineDeployment to be in deletion. err := r.reconcileDelete(ctx, tt.machineDeployment) if tt.expectError { @@ -1070,21 +1071,6 @@ func TestReconciler_reconcileDelete(t *testing.T) { machineSetList := &clusterv1.MachineSetList{} g.Expect(c.List(ctx, machineSetList, client.InNamespace("default"))).ToNot(HaveOccurred()) - - // Remove ResourceVersion so we can actually compare. - for i, m := range machineSetList.Items { - m.ResourceVersion = "" - machineSetList.Items[i] = m - } - - sort.Slice(machineSetList.Items, func(i, j int) bool { - return machineSetList.Items[i].GetName() < machineSetList.Items[j].GetName() - }) - sort.Slice(tt.wantMachineSets, func(i, j int) bool { - return tt.wantMachineSets[i].GetName() < tt.wantMachineSets[j].GetName() - }) - - g.Expect(machineSetList.Items).To(BeComparableTo(tt.wantMachineSets)) }) } } diff --git a/internal/controllers/machineset/machineset_controller.go b/internal/controllers/machineset/machineset_controller.go index a221750b010e..27b9682adf9e 100644 --- a/internal/controllers/machineset/machineset_controller.go +++ b/internal/controllers/machineset/machineset_controller.go @@ -348,7 +348,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, machineSet *clusterv1. for _, machine := range machineList { if machine.DeletionTimestamp.IsZero() { log.Info("Deleting Machine", "Machine", klog.KObj(machine)) - if err := r.Client.Delete(ctx, machine); err != nil { + if err := r.Client.Delete(ctx, machine); err != nil && !apierrors.IsNotFound(err) { return errors.Wrapf(err, "failed to delete Machine %s", klog.KObj(machine)) } } @@ -381,6 +381,7 @@ func (r *Reconciler) getAndAdoptMachinesForMachineSet(ctx context.Context, machi for idx := range allMachines.Items { machine := &allMachines.Items[idx] log := log.WithValues("Machine", klog.KObj(machine)) + ctx := ctrl.LoggerInto(ctx, log) if shouldExcludeMachine(machineSet, machine) { continue } diff --git a/internal/controllers/machineset/machineset_controller_test.go b/internal/controllers/machineset/machineset_controller_test.go index 3f9352bd3f67..f1c866c42740 100644 --- a/internal/controllers/machineset/machineset_controller_test.go +++ b/internal/controllers/machineset/machineset_controller_test.go @@ -18,7 +18,6 @@ package machineset import ( "fmt" - "sort" "testing" "time" @@ -2136,12 +2135,15 @@ func TestReconciler_reconcileDelete(t *testing.T) { "some": "labelselector", } ms := builder.MachineSet("default", "ms0").WithClusterName("test").Build() + ms.Finalizers = []string{ + clusterv1.MachineSetFinalizer, + } ms.DeletionTimestamp = ptr.To(metav1.Now()) ms.Spec.Selector = metav1.LabelSelector{ MatchLabels: labels, } msWithoutFinalizer := ms.DeepCopy() - msWithoutFinalizer.Finalizers = nil + msWithoutFinalizer.Finalizers = []string{} tests := []struct { name string machineSet *clusterv1.MachineSet @@ -2207,19 +2209,11 @@ func TestReconciler_reconcileDelete(t *testing.T) { g.Expect(c.List(ctx, machineList, client.InNamespace("default"))).ToNot(HaveOccurred()) // Remove ResourceVersion so we can actually compare. - for i, m := range machineList.Items { - m.ResourceVersion = "" - machineList.Items[i] = m + for i := range machineList.Items { + machineList.Items[i].ResourceVersion = "" } - sort.Slice(machineList.Items, func(i, j int) bool { - return machineList.Items[i].GetName() < machineList.Items[j].GetName() - }) - sort.Slice(tt.wantMachines, func(i, j int) bool { - return tt.wantMachines[i].GetName() < tt.wantMachines[j].GetName() - }) - - g.Expect(machineList.Items).To(BeComparableTo(tt.wantMachines)) + g.Expect(machineList.Items).To(ConsistOf(tt.wantMachines)) }) } } diff --git a/util/log/log.go b/util/log/log.go index 45678cb6bf37..11a41c460361 100644 --- a/util/log/log.go +++ b/util/log/log.go @@ -111,14 +111,20 @@ func getOwners(ctx context.Context, c client.Client, obj metav1.Object) ([]owner // five objects. On more than five objects it outputs the first five objects and // adds information about how much more are in the given list. func ObjNamesString[T client.Object](objs []T) string { - objNames := make([]string, len(objs)) + shortenedBy := 0 + if len(objs) > 5 { + shortenedBy = len(objs) - 5 + objs = objs[:5] + } + + stringList := []string{} for _, obj := range objs { - objNames = append(objNames, obj.GetName()) + stringList = append(stringList, obj.GetName()) } - if len(objNames) <= 5 { - return strings.Join(objNames, ", ") + if shortenedBy > 0 { + stringList = append(stringList, fmt.Sprintf("... (%d more)", shortenedBy)) } - return fmt.Sprintf("%s and %d more", strings.Join(objNames[:5], ", "), len(objNames)-5) + return strings.Join(stringList, ", ") }