From 419aa5a9a9a2c592b123d169300484b85c291dd2 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Thu, 30 Oct 2025 13:15:47 +0100 Subject: [PATCH 01/19] pkg/util implement unit tests for CAPIMachineSetStatusEqual --- pkg/util/sync_test.go | 345 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 345 insertions(+) create mode 100644 pkg/util/sync_test.go diff --git a/pkg/util/sync_test.go b/pkg/util/sync_test.go new file mode 100644 index 000000000..a608bd418 --- /dev/null +++ b/pkg/util/sync_test.go @@ -0,0 +1,345 @@ +/* +Copyright 2025 Red Hat, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +package util + +import ( + "fmt" + "testing" + "time" + + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" + + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" +) + +//nolint:funlen +func TestCAPIMachineSetStatusEqual(t *testing.T) { + now := metav1.Now() + later := metav1.NewTime(now.Add(1 * time.Hour)) + + tests := []struct { + name string + a clusterv1.MachineSetStatus + b clusterv1.MachineSetStatus + want string + wantChanges bool + }{ + { + name: "no diff", + a: clusterv1.MachineSetStatus{}, + b: clusterv1.MachineSetStatus{}, + want: "", + wantChanges: false, + }, + { + name: "diff in ReadyReplicas", + a: clusterv1.MachineSetStatus{ + ReadyReplicas: 3, + }, + b: clusterv1.MachineSetStatus{ + ReadyReplicas: 5, + }, + want: "map[.readyReplicas:[3 != 5]]", + wantChanges: true, + }, + { + name: "diff in AvailableReplicas", + a: clusterv1.MachineSetStatus{ + AvailableReplicas: 2, + }, + b: clusterv1.MachineSetStatus{ + AvailableReplicas: 4, + }, + want: "map[.availableReplicas:[2 != 4]]", + wantChanges: true, + }, + { + name: "diff in ReadyReplicas and AvailableReplicas", + a: clusterv1.MachineSetStatus{ + ReadyReplicas: 3, + AvailableReplicas: 2, + }, + b: clusterv1.MachineSetStatus{ + ReadyReplicas: 5, + AvailableReplicas: 4, + }, + want: "map[.availableReplicas:[2 != 4] .readyReplicas:[3 != 5]]", + wantChanges: true, + }, + { + name: "same v1beta1 conditions", + a: clusterv1.MachineSetStatus{ + Conditions: []clusterv1.Condition{ + { + Type: clusterv1.ReadyCondition, + Status: corev1.ConditionTrue, + LastTransitionTime: now, + }, + }, + }, + b: clusterv1.MachineSetStatus{ + Conditions: []clusterv1.Condition{ + { + Type: clusterv1.ReadyCondition, + Status: corev1.ConditionTrue, + LastTransitionTime: now, + }, + }, + }, + want: "map[]", + wantChanges: false, + }, + { + name: "changed v1beta1 condition Status", + a: clusterv1.MachineSetStatus{ + Conditions: []clusterv1.Condition{ + { + Type: clusterv1.ReadyCondition, + Status: corev1.ConditionTrue, + LastTransitionTime: now, + }, + }, + }, + b: clusterv1.MachineSetStatus{ + Conditions: []clusterv1.Condition{ + { + Type: clusterv1.ReadyCondition, + Status: corev1.ConditionFalse, + LastTransitionTime: now, + }, + }, + }, + want: "map[.conditions:[Status: True != False]]", + wantChanges: true, + }, + { + name: "v1beta1 condition LastTransitionTime ignored", + a: clusterv1.MachineSetStatus{ + Conditions: []clusterv1.Condition{ + { + Type: clusterv1.ReadyCondition, + Status: corev1.ConditionTrue, + LastTransitionTime: now, + }, + }, + }, + b: clusterv1.MachineSetStatus{ + Conditions: []clusterv1.Condition{ + { + Type: clusterv1.ReadyCondition, + Status: corev1.ConditionTrue, + LastTransitionTime: later, + }, + }, + }, + want: "map[]", + wantChanges: false, + }, + { + name: "multiple v1beta1 conditions with one changed", + a: clusterv1.MachineSetStatus{ + Conditions: []clusterv1.Condition{ + { + Type: clusterv1.ReadyCondition, + Status: corev1.ConditionTrue, + LastTransitionTime: now, + }, + { + Type: clusterv1.MachinesReadyCondition, + Status: corev1.ConditionTrue, + LastTransitionTime: now, + }, + }, + }, + b: clusterv1.MachineSetStatus{ + Conditions: []clusterv1.Condition{ + { + Type: clusterv1.ReadyCondition, + Status: corev1.ConditionTrue, + LastTransitionTime: now, + }, + { + Type: clusterv1.MachinesReadyCondition, + Status: corev1.ConditionFalse, + LastTransitionTime: now, + }, + }, + }, + want: "map[.conditions:[Status: True != False]]", + wantChanges: true, + }, + { + name: "same v1beta2 conditions", + a: clusterv1.MachineSetStatus{ + V1Beta2: &clusterv1.MachineSetV1Beta2Status{ + Conditions: []metav1.Condition{ + { + Type: "Ready", + Status: metav1.ConditionTrue, + LastTransitionTime: now, + Reason: "AllReady", + Message: "All machines are ready", + }, + }, + }, + }, + b: clusterv1.MachineSetStatus{ + V1Beta2: &clusterv1.MachineSetV1Beta2Status{ + Conditions: []metav1.Condition{ + { + Type: "Ready", + Status: metav1.ConditionTrue, + LastTransitionTime: now, + Reason: "AllReady", + Message: "All machines are ready", + }, + }, + }, + }, + want: "map[]", + wantChanges: false, + }, + { + name: "changed v1beta2 condition Status", + a: clusterv1.MachineSetStatus{ + V1Beta2: &clusterv1.MachineSetV1Beta2Status{ + Conditions: []metav1.Condition{ + { + Type: "Ready", + Status: metav1.ConditionTrue, + LastTransitionTime: now, + }, + }, + }, + }, + b: clusterv1.MachineSetStatus{ + V1Beta2: &clusterv1.MachineSetV1Beta2Status{ + Conditions: []metav1.Condition{ + { + Type: "Ready", + Status: metav1.ConditionFalse, + LastTransitionTime: now, + }, + }, + }, + }, + want: "map[.v1beta2.conditions:[Status: True != False]]", + wantChanges: true, + }, + { + name: "v1beta2 condition LastTransitionTime ignored", + a: clusterv1.MachineSetStatus{ + V1Beta2: &clusterv1.MachineSetV1Beta2Status{ + Conditions: []metav1.Condition{ + { + Type: "Ready", + Status: metav1.ConditionTrue, + LastTransitionTime: now, + Reason: "AllReady", + Message: "All machines are ready", + }, + }, + }, + }, + b: clusterv1.MachineSetStatus{ + V1Beta2: &clusterv1.MachineSetV1Beta2Status{ + Conditions: []metav1.Condition{ + { + Type: "Ready", + Status: metav1.ConditionTrue, + LastTransitionTime: later, + Reason: "AllReady", + Message: "All machines are ready", + }, + }, + }, + }, + want: "map[]", + wantChanges: false, + }, + { + name: "multiple v1beta2 conditions with one changed", + a: clusterv1.MachineSetStatus{ + V1Beta2: &clusterv1.MachineSetV1Beta2Status{ + Conditions: []metav1.Condition{ + { + Type: "Ready", + Status: metav1.ConditionTrue, + LastTransitionTime: now, + }, + { + Type: "Available", + Status: metav1.ConditionTrue, + LastTransitionTime: now, + }, + }, + }, + }, + b: clusterv1.MachineSetStatus{ + V1Beta2: &clusterv1.MachineSetV1Beta2Status{ + Conditions: []metav1.Condition{ + { + Type: "Ready", + Status: metav1.ConditionTrue, + LastTransitionTime: now, + }, + { + Type: "Available", + Status: metav1.ConditionFalse, + LastTransitionTime: now, + }, + }, + }, + }, + want: "map[.v1beta2.conditions:[Status: True != False]]", + wantChanges: true, + }, + { + name: "v1beta2 nil vs non-nil", + a: clusterv1.MachineSetStatus{ + V1Beta2: nil, + }, + b: clusterv1.MachineSetStatus{ + V1Beta2: &clusterv1.MachineSetV1Beta2Status{ + ReadyReplicas: ptr.To[int32](3), + }, + }, + want: "map[.v1beta2.readyReplicas:[ != int32]]", + wantChanges: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + got := CAPIMachineSetStatusEqual(tt.a, tt.b) + + gotChanges := len(got) > 0 + + g.Expect(gotChanges).To(Equal(tt.wantChanges)) + + if tt.wantChanges { + gotString := fmt.Sprintf("%+v", got) + + g.Expect(gotString).To(Equal(tt.want)) + } + }) + } +} From 7fb617fae783ba4ad7303738c0adacf119f71c8c Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Fri, 10 Oct 2025 09:56:41 +0200 Subject: [PATCH 02/19] pkg/util/sync: first iteration for new differ approach --- .../machineset_sync_controller.go | 15 +- pkg/util/diff.go | 145 ++++++++++++++++++ pkg/util/diff_test.go | 142 +++++++++++++++++ pkg/util/sync.go | 57 +------ pkg/util/sync_test.go | 33 ++-- 5 files changed, 318 insertions(+), 74 deletions(-) create mode 100644 pkg/util/diff.go create mode 100644 pkg/util/diff_test.go diff --git a/pkg/controllers/machinesetsync/machineset_sync_controller.go b/pkg/controllers/machinesetsync/machineset_sync_controller.go index 1108ee77a..38d9ec045 100644 --- a/pkg/controllers/machinesetsync/machineset_sync_controller.go +++ b/pkg/controllers/machinesetsync/machineset_sync_controller.go @@ -820,7 +820,10 @@ func (r *MachineSetSyncReconciler) createOrUpdateCAPIMachineSet(ctx context.Cont // If there is an existing CAPI machine set, work out if it needs updating. // Compare the existing CAPI machine set with the converted CAPI machine set to check for changes. - capiMachineSetsDiff := compareCAPIMachineSets(existingCAPIMachineSet, convertedCAPIMachineSet) + capiMachineSetsDiff, err := compareCAPIMachineSets(existingCAPIMachineSet, convertedCAPIMachineSet) + if err != nil { + return fmt.Errorf("failed to compare CAPI machine sets: %w", err) + } // Make a deep copy of the converted CAPI machine set to avoid modifying the original. updatedOrCreatedCAPIMachineSet := convertedCAPIMachineSet.DeepCopy() @@ -1427,7 +1430,7 @@ func compareCAPIInfraMachineTemplates(platform configv1.PlatformType, infraMachi } // compareCAPIMachineSets compares CAPI machineSets a and b, and returns a list of differences, or none if there are none. -func compareCAPIMachineSets(capiMachineSet1, capiMachineSet2 *clusterv1.MachineSet) map[string]any { +func compareCAPIMachineSets(capiMachineSet1, capiMachineSet2 *clusterv1.MachineSet) (map[string]any, error) { diff := make(map[string]any) if diffSpec := deep.Equal(capiMachineSet1.Spec, capiMachineSet2.Spec); len(diffSpec) > 0 { @@ -1438,11 +1441,13 @@ func compareCAPIMachineSets(capiMachineSet1, capiMachineSet2 *clusterv1.MachineS diff[".metadata"] = diffObjectMeta } - if diffStatus := util.CAPIMachineSetStatusEqual(capiMachineSet1.Status, capiMachineSet2.Status); len(diffStatus) > 0 { - diff[".status"] = diffStatus + if diffStatus, err := util.CAPIMachineSetStatusEqual(capiMachineSet1.Status, capiMachineSet2.Status); err != nil { + return nil, fmt.Errorf("failed to compare CAPI machine set status: %w", err) + } else if diffStatus.Changed() { + diff[".status"] = diffStatus.String() } - return diff + return diff, nil } // compareMAPIMachineSets compares MAPI machineSets a and b, and returns a list of differences, or none if there are none. diff --git a/pkg/util/diff.go b/pkg/util/diff.go new file mode 100644 index 000000000..83cf41b25 --- /dev/null +++ b/pkg/util/diff.go @@ -0,0 +1,145 @@ +/* +Copyright 2025 Red Hat, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +package util + +import ( + "fmt" + "strings" + + "github.com/go-test/deep" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" +) + +// DiffResult is the interface that represents the result of a diff operation. +type DiffResult interface { + Changed() bool + String() string +} + +type diffResult struct { + diff []string +} + +// Changed returns true if the diff detected any changes. +func (d *diffResult) Changed() bool { + return len(d.diff) > 0 +} + +// String returns the diff as a string. +func (d *diffResult) String() string { + return strings.Join(d.diff, ", ") +} + +type differ struct { + ignoreConditionsLastTransitionTime bool + ignoredPath [][]string +} + +// Diff compares the objects a and b, and returns a DiffResult. +func (d *differ) Diff(a, b any) (DiffResult, error) { + unstructuredA, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&a) + if err != nil { + return nil, fmt.Errorf("failed to convert b to unstructured: %w", err) + } + + unstructuredB, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&b) + if err != nil { + return nil, fmt.Errorf("failed to convert b to unstructured: %w", err) + } + + var additionalIgnoredPaths [][]string + + if d.ignoreConditionsLastTransitionTime { + if err := removeConditionsLastTransitionTime(unstructuredA); err != nil { + return nil, fmt.Errorf("failed to remove conditions last transition time from a: %w", err) + } + + if err := removeConditionsLastTransitionTime(unstructuredB); err != nil { + return nil, fmt.Errorf("failed to remove conditions last transition time from b: %w", err) + } + } + + // Remove fields that we want to ignore. + for _, ignorePath := range append(d.ignoredPath, additionalIgnoredPaths...) { + unstructured.RemoveNestedField(unstructuredA, ignorePath...) + unstructured.RemoveNestedField(unstructuredB, ignorePath...) + } + + diff := deep.Equal(unstructuredA, unstructuredB) + + // Make the result deterministic. + sort.Strings(diff) + + return &diffResult{diff: diff}, nil +} + +func removeConditionsLastTransitionTime(a map[string]interface{}) error { + conditionPaths := [][]string{ + {"conditions"}, + {"v1beta2", "conditions"}, + {"deprecated", "v1beta1", "conditions"}, + } + + for _, conditionPath := range conditionPaths { + conditions, found, err := unstructured.NestedSlice(a, conditionPath...) + if !found || err != nil { + continue + } + + for i, condition := range conditions { + conditionMap, ok := condition.(map[string]interface{}) + if !ok { + continue + } + + conditionMap["lastTransitionTime"] = "ignored" + conditions[i] = conditionMap + } + + if err := unstructured.SetNestedField(a, conditions, conditionPath...); err != nil { + return fmt.Errorf("failed to set nested field %s: %w", strings.Join(conditionPath, "."), err) + } + } + + return nil +} + +type diffopts func(*differ) + +// NewDiffer creates a new differ with the given options. +func NewDiffer(opts ...diffopts) *differ { + d := &differ{} + for _, opt := range opts { + opt(d) + } + + return d +} + +// WithIgnoreField adds a path to the list of paths to ignore when executing Diff. +func WithIgnoreField(path ...string) diffopts { + return func(d *differ) { + d.ignoredPath = append(d.ignoredPath, path) + } +} + +// WithIgnoreConditionsLastTransitionTime configures the differ to ignore LastTransitionTime for conditions when executing Diff. +func WithIgnoreConditionsLastTransitionTime() diffopts { + return func(d *differ) { + d.ignoreConditionsLastTransitionTime = true + } +} diff --git a/pkg/util/diff_test.go b/pkg/util/diff_test.go new file mode 100644 index 000000000..e36d6db7c --- /dev/null +++ b/pkg/util/diff_test.go @@ -0,0 +1,142 @@ +/* +Copyright 2025 Red Hat, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +package util + +import ( + "testing" + + . "github.com/onsi/gomega" +) + +//nolint:funlen +func TestDiff_basic_operations(t *testing.T) { + tests := []struct { + name string + a map[string]any + b map[string]any + wantChanged bool + want string + }{ + { + name: "no diff on empty objects", + a: map[string]any{}, + b: map[string]any{}, + wantChanged: false, + want: "", + }, + { + name: "diff when adding a field", + a: map[string]any{ + "a": 1, + }, + b: map[string]any{ + "a": 1, + "b": 2, + }, + wantChanged: true, + want: "map[b]: != 2", + }, + { + name: "diff when removing a field", + a: map[string]any{ + "a": 1, + "b": 2, + }, + b: map[string]any{ + "a": 1, + }, + wantChanged: true, + want: "map[b]: 2 != ", + }, + { + name: "diff when changing a field", + a: map[string]any{ + "a": 1, + "b": 2, + }, + b: map[string]any{ + "a": 1, + "b": 3, + }, + wantChanged: true, + want: "map[b]: 2 != 3", + }, + { + name: "diff when adding a entry to a list", + a: map[string]any{ + "a": 1, + "b": []int{1, 2}, + }, + b: map[string]any{ + "a": 1, + "b": []int{1, 2, 3}, + }, + wantChanged: true, + want: "map[b].slice[2]: != 3", + }, + { + name: "diff when removing a entry from a list", + a: map[string]any{ + "a": 1, + "b": []int{1, 2, 3}, + }, + b: map[string]any{ + "a": 1, + "b": []int{1, 2}, + }, + wantChanged: true, + want: "map[b].slice[2]: 3 != ", + }, + { + name: "diff when changing a entry in a list", + a: map[string]any{ + "a": 1, + "b": []int{1, 2, 3}, + }, + b: map[string]any{ + "a": 1, + "b": []int{1, 2, 4}, + }, + wantChanged: true, + want: "map[b].slice[2]: 3 != 4", + }, + { + name: "diff when deleting a list", + a: map[string]any{ + "a": 1, + "b": []int{1, 2, 3}, + }, + b: map[string]any{ + "a": 1, + }, + wantChanged: true, + want: "map[b]: [1 2 3] != ", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + differ := NewDiffer() + + diff, err := differ.Diff(tt.a, tt.b) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(diff.Changed()).To(Equal(tt.wantChanged)) + g.Expect(diff.String()).To(Equal(tt.want)) + }) + } +} diff --git a/pkg/util/sync.go b/pkg/util/sync.go index 648c441e9..439ce39f7 100644 --- a/pkg/util/sync.go +++ b/pkg/util/sync.go @@ -88,59 +88,12 @@ func ObjectMetaEqual(a, b metav1.ObjectMeta) map[string]any { // CAPIMachineSetStatusEqual compares variables a and b, // and returns a list of differences, or nil if there are none, // for the fields we care about when synchronising MAPI and CAPI Machines. -func CAPIMachineSetStatusEqual(a, b clusterv1.MachineSetStatus) map[string]any { - diff := map[string]any{} - - if diffReadyReplicas := deep.Equal(a.ReadyReplicas, b.ReadyReplicas); len(diffReadyReplicas) > 0 { - diff[".readyReplicas"] = diffReadyReplicas - } - - if diffAvailableReplicas := deep.Equal(a.AvailableReplicas, b.AvailableReplicas); len(diffAvailableReplicas) > 0 { - diff[".availableReplicas"] = diffAvailableReplicas - } - - if diffFullyLabeledReplicas := deep.Equal(a.FullyLabeledReplicas, b.FullyLabeledReplicas); len(diffFullyLabeledReplicas) > 0 { - diff[".fullyLabeledReplicas"] = diffFullyLabeledReplicas - } - - if diffFailureReason := deep.Equal(a.FailureReason, b.FailureReason); len(diffFailureReason) > 0 { - diff[".failureReason"] = diffFailureReason - } +func CAPIMachineSetStatusEqual(a, b clusterv1.MachineSetStatus) (DiffResult, error) { + differ := newDiffer( + WithIgnoreConditionsLastTransitionTime(), + ) - if diffFailureMessage := deep.Equal(a.FailureMessage, b.FailureMessage); len(diffFailureMessage) > 0 { - diff[".failureMessage"] = diffFailureMessage - } - - if diffConditions := compareCAPIV1Beta1Conditions(a.Conditions, b.Conditions); len(diffConditions) > 0 { - diff[".conditions"] = diffConditions - } - - // Compare the v1beta2 fields. - if a.V1Beta2 == nil { - a.V1Beta2 = &clusterv1.MachineSetV1Beta2Status{} - } - - if b.V1Beta2 == nil { - b.V1Beta2 = &clusterv1.MachineSetV1Beta2Status{} - } - - if diffUpToDateReplicas := deep.Equal(a.V1Beta2.UpToDateReplicas, b.V1Beta2.UpToDateReplicas); len(diffUpToDateReplicas) > 0 { - diff[".v1beta2.upToDateReplicas"] = diffUpToDateReplicas - } - - if diffAvailableReplicas := deep.Equal(a.V1Beta2.AvailableReplicas, b.V1Beta2.AvailableReplicas); len(diffAvailableReplicas) > 0 { - diff[".v1beta2.availableReplicas"] = diffAvailableReplicas - } - - if diffReadyReplicas := deep.Equal(a.V1Beta2.ReadyReplicas, b.V1Beta2.ReadyReplicas); len(diffReadyReplicas) > 0 { - diff[".v1beta2.readyReplicas"] = diffReadyReplicas - } - - if diffConditions := compareCAPIV1Beta2Conditions(a.V1Beta2.Conditions, b.V1Beta2.Conditions); len(diffConditions) > 0 { - diff[".v1beta2.conditions"] = diffConditions - } - - return diff + return differ.Diff(a, b) } // CAPIMachineStatusEqual compares variables a and b, diff --git a/pkg/util/sync_test.go b/pkg/util/sync_test.go index a608bd418..e682fc88e 100644 --- a/pkg/util/sync_test.go +++ b/pkg/util/sync_test.go @@ -16,7 +16,6 @@ limitations under the License. package util import ( - "fmt" "testing" "time" @@ -55,7 +54,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { b: clusterv1.MachineSetStatus{ ReadyReplicas: 5, }, - want: "map[.readyReplicas:[3 != 5]]", + want: "map[readyReplicas]: 3 != 5", wantChanges: true, }, { @@ -66,7 +65,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { b: clusterv1.MachineSetStatus{ AvailableReplicas: 4, }, - want: "map[.availableReplicas:[2 != 4]]", + want: "map[availableReplicas]: 2 != 4", wantChanges: true, }, { @@ -79,7 +78,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { ReadyReplicas: 5, AvailableReplicas: 4, }, - want: "map[.availableReplicas:[2 != 4] .readyReplicas:[3 != 5]]", + want: "map[readyReplicas]: 3 != 5, map[availableReplicas]: 2 != 4", wantChanges: true, }, { @@ -102,7 +101,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, }, }, - want: "map[]", + want: "", wantChanges: false, }, { @@ -125,7 +124,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, }, }, - want: "map[.conditions:[Status: True != False]]", + want: "map[conditions].slice[0].map[status]: True != False", wantChanges: true, }, { @@ -148,7 +147,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, }, }, - want: "map[]", + want: "", wantChanges: false, }, { @@ -181,7 +180,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, }, }, - want: "map[.conditions:[Status: True != False]]", + want: "map[conditions].slice[1].map[status]: True != False", wantChanges: true, }, { @@ -212,7 +211,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, }, }, - want: "map[]", + want: "", wantChanges: false, }, { @@ -239,7 +238,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, }, }, - want: "map[.v1beta2.conditions:[Status: True != False]]", + want: "map[v1beta2].map[conditions].slice[0].map[status]: True != False", wantChanges: true, }, { @@ -270,7 +269,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, }, }, - want: "map[]", + want: "", wantChanges: false, }, { @@ -307,7 +306,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, }, }, - want: "map[.v1beta2.conditions:[Status: True != False]]", + want: "map[v1beta2].map[conditions].slice[1].map[status]: True != False", wantChanges: true, }, { @@ -320,7 +319,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { ReadyReplicas: ptr.To[int32](3), }, }, - want: "map[.v1beta2.readyReplicas:[ != int32]]", + want: "map[v1beta2]: != map[readyReplicas:3]", wantChanges: true, }, } @@ -329,15 +328,15 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - got := CAPIMachineSetStatusEqual(tt.a, tt.b) + got, err := CAPIMachineSetStatusEqual(tt.a, tt.b) + g.Expect(err).ToNot(HaveOccurred()) - gotChanges := len(got) > 0 + gotChanges := got.Changed() g.Expect(gotChanges).To(Equal(tt.wantChanges)) if tt.wantChanges { - gotString := fmt.Sprintf("%+v", got) - + gotString := got.String() g.Expect(gotString).To(Equal(tt.want)) } }) From 7cd9f851a19c104baad92d50903351141aa22d40 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Thu, 30 Oct 2025 14:04:36 +0100 Subject: [PATCH 03/19] pkg/util/sync: second iteration prettify the output string --- pkg/util/diff.go | 11 ++++++++++- pkg/util/diff_test.go | 14 +++++++------- pkg/util/sync_test.go | 16 ++++++++-------- 3 files changed, 25 insertions(+), 16 deletions(-) diff --git a/pkg/util/diff.go b/pkg/util/diff.go index 83cf41b25..b40d1d86c 100644 --- a/pkg/util/diff.go +++ b/pkg/util/diff.go @@ -17,6 +17,7 @@ package util import ( "fmt" + "sort" "strings" "github.com/go-test/deep" @@ -41,7 +42,15 @@ func (d *diffResult) Changed() bool { // String returns the diff as a string. func (d *diffResult) String() string { - return strings.Join(d.diff, ", ") + if !d.Changed() { + return "" + } + + out := "." + strings.Join(d.diff, ", .") + out = strings.ReplaceAll(out, ".slice[", "[") + out = strings.ReplaceAll(out, "map[", "[") + + return out } type differ struct { diff --git a/pkg/util/diff_test.go b/pkg/util/diff_test.go index e36d6db7c..9a4971645 100644 --- a/pkg/util/diff_test.go +++ b/pkg/util/diff_test.go @@ -47,7 +47,7 @@ func TestDiff_basic_operations(t *testing.T) { "b": 2, }, wantChanged: true, - want: "map[b]: != 2", + want: ".[b]: != 2", }, { name: "diff when removing a field", @@ -59,7 +59,7 @@ func TestDiff_basic_operations(t *testing.T) { "a": 1, }, wantChanged: true, - want: "map[b]: 2 != ", + want: ".[b]: 2 != ", }, { name: "diff when changing a field", @@ -72,7 +72,7 @@ func TestDiff_basic_operations(t *testing.T) { "b": 3, }, wantChanged: true, - want: "map[b]: 2 != 3", + want: ".[b]: 2 != 3", }, { name: "diff when adding a entry to a list", @@ -85,7 +85,7 @@ func TestDiff_basic_operations(t *testing.T) { "b": []int{1, 2, 3}, }, wantChanged: true, - want: "map[b].slice[2]: != 3", + want: ".[b][2]: != 3", }, { name: "diff when removing a entry from a list", @@ -98,7 +98,7 @@ func TestDiff_basic_operations(t *testing.T) { "b": []int{1, 2}, }, wantChanged: true, - want: "map[b].slice[2]: 3 != ", + want: ".[b][2]: 3 != ", }, { name: "diff when changing a entry in a list", @@ -111,7 +111,7 @@ func TestDiff_basic_operations(t *testing.T) { "b": []int{1, 2, 4}, }, wantChanged: true, - want: "map[b].slice[2]: 3 != 4", + want: ".[b][2]: 3 != 4", }, { name: "diff when deleting a list", @@ -123,7 +123,7 @@ func TestDiff_basic_operations(t *testing.T) { "a": 1, }, wantChanged: true, - want: "map[b]: [1 2 3] != ", + want: ".[b]: [1 2 3] != ", }, } diff --git a/pkg/util/sync_test.go b/pkg/util/sync_test.go index e682fc88e..8736fd07f 100644 --- a/pkg/util/sync_test.go +++ b/pkg/util/sync_test.go @@ -54,7 +54,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { b: clusterv1.MachineSetStatus{ ReadyReplicas: 5, }, - want: "map[readyReplicas]: 3 != 5", + want: ".[readyReplicas]: 3 != 5", wantChanges: true, }, { @@ -65,7 +65,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { b: clusterv1.MachineSetStatus{ AvailableReplicas: 4, }, - want: "map[availableReplicas]: 2 != 4", + want: ".[availableReplicas]: 2 != 4", wantChanges: true, }, { @@ -78,7 +78,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { ReadyReplicas: 5, AvailableReplicas: 4, }, - want: "map[readyReplicas]: 3 != 5, map[availableReplicas]: 2 != 4", + want: ".[availableReplicas]: 2 != 4, .[readyReplicas]: 3 != 5", wantChanges: true, }, { @@ -124,7 +124,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, }, }, - want: "map[conditions].slice[0].map[status]: True != False", + want: ".[conditions][0].[status]: True != False", wantChanges: true, }, { @@ -180,7 +180,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, }, }, - want: "map[conditions].slice[1].map[status]: True != False", + want: ".[conditions][1].[status]: True != False", wantChanges: true, }, { @@ -238,7 +238,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, }, }, - want: "map[v1beta2].map[conditions].slice[0].map[status]: True != False", + want: ".[v1beta2].[conditions][0].[status]: True != False", wantChanges: true, }, { @@ -306,7 +306,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, }, }, - want: "map[v1beta2].map[conditions].slice[1].map[status]: True != False", + want: ".[v1beta2].[conditions][1].[status]: True != False", wantChanges: true, }, { @@ -319,7 +319,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { ReadyReplicas: ptr.To[int32](3), }, }, - want: "map[v1beta2]: != map[readyReplicas:3]", + want: ".[v1beta2]: != [readyReplicas:3]", wantChanges: true, }, } From d86d09a33d819485345d1510d228015b0b8661ea Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Thu, 30 Oct 2025 15:15:48 +0100 Subject: [PATCH 04/19] use differ everywhere --- .../machineset_sync_controller.go | 137 +++++---- .../machinesync/machine_sync_controller.go | 68 +++-- .../machine_sync_mapi2capi_infrastructure.go | 79 +++--- pkg/util/sync.go | 261 ++++-------------- 4 files changed, 217 insertions(+), 328 deletions(-) diff --git a/pkg/controllers/machinesetsync/machineset_sync_controller.go b/pkg/controllers/machinesetsync/machineset_sync_controller.go index 38d9ec045..f2864ff00 100644 --- a/pkg/controllers/machinesetsync/machineset_sync_controller.go +++ b/pkg/controllers/machinesetsync/machineset_sync_controller.go @@ -40,7 +40,6 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" utilerrors "k8s.io/apimachinery/pkg/util/errors" - "github.com/go-test/deep" machinev1applyconfigs "github.com/openshift/client-go/machine/applyconfigurations/machine/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" @@ -822,7 +821,7 @@ func (r *MachineSetSyncReconciler) createOrUpdateCAPIMachineSet(ctx context.Cont // Compare the existing CAPI machine set with the converted CAPI machine set to check for changes. capiMachineSetsDiff, err := compareCAPIMachineSets(existingCAPIMachineSet, convertedCAPIMachineSet) if err != nil { - return fmt.Errorf("failed to compare CAPI machine sets: %w", err) + return fmt.Errorf("failed to compare Cluster API machine sets: %w", err) } // Make a deep copy of the converted CAPI machine set to avoid modifying the original. @@ -1353,6 +1352,12 @@ func initInfraMachineTemplateListAndInfraClusterListFromProvider(platform config // //nolint:funlen func compareCAPIInfraMachineTemplates(platform configv1.PlatformType, infraMachineTemplate1, infraMachineTemplate2 client.Object) (map[string]any, error) { + diff := make(map[string]any) + + var metadata1, metadata2 metav1.ObjectMeta + + var spec1, spec2, status1, status2 any + switch platform { case configv1.AWSPlatformType: typedInfraMachineTemplate1, ok := infraMachineTemplate1.(*awsv1.AWSMachineTemplate) @@ -1365,19 +1370,12 @@ func compareCAPIInfraMachineTemplates(platform configv1.PlatformType, infraMachi return nil, errAssertingCAPIAWSMachineTemplate } - diff := make(map[string]any) - - if diffSpec := deep.Equal(typedInfraMachineTemplate1.Spec, typedinfraMachineTemplate2.Spec); len(diffSpec) > 0 { - diff[".spec"] = diffSpec - } - - if diffObjectMeta := util.ObjectMetaEqual(typedInfraMachineTemplate1.ObjectMeta, typedinfraMachineTemplate2.ObjectMeta); len(diffObjectMeta) > 0 { - diff[".metadata"] = diffObjectMeta - } - - // TODO: Evaluate if we want to add status comparison if needed in the future (e.g. for scale from zero capacity). - - return diff, nil + spec1 = typedInfraMachineTemplate1.Spec + spec2 = typedinfraMachineTemplate2.Spec + metadata1 = typedInfraMachineTemplate1.ObjectMeta + metadata2 = typedinfraMachineTemplate2.ObjectMeta + status1 = typedInfraMachineTemplate1.Status + status2 = typedinfraMachineTemplate2.Status case configv1.OpenStackPlatformType: typedInfraMachineTemplate1, ok := infraMachineTemplate1.(*openstackv1.OpenStackMachineTemplate) if !ok { @@ -1389,17 +1387,10 @@ func compareCAPIInfraMachineTemplates(platform configv1.PlatformType, infraMachi return nil, errAssertingCAPIOpenStackMachineTemplate } - diff := make(map[string]any) - - if diffSpec := deep.Equal(typedInfraMachineTemplate1.Spec, typedinfraMachineTemplate2.Spec); len(diffSpec) > 0 { - diff[".spec"] = diffSpec - } - - if diffObjectMeta := deep.Equal(typedInfraMachineTemplate1.ObjectMeta, typedinfraMachineTemplate2.ObjectMeta); len(diffObjectMeta) > 0 { - diff[".metadata"] = diffObjectMeta - } - - return diff, nil + spec1 = typedInfraMachineTemplate1.Spec + spec2 = typedinfraMachineTemplate2.Spec + metadata1 = typedInfraMachineTemplate1.ObjectMeta + metadata2 = typedinfraMachineTemplate2.ObjectMeta case configv1.PowerVSPlatformType: typedInfraMachineTemplate1, ok := infraMachineTemplate1.(*ibmpowervsv1.IBMPowerVSMachineTemplate) if !ok { @@ -1411,38 +1402,61 @@ func compareCAPIInfraMachineTemplates(platform configv1.PlatformType, infraMachi return nil, errAssertingCAPIIBMPowerVSMachineTemplate } - diff := make(map[string]any) - - if diffSpec := deep.Equal(typedInfraMachineTemplate1.Spec, typedinfraMachineTemplate2.Spec); len(diffSpec) > 0 { - diff[".spec"] = diffSpec - } + spec1 = typedInfraMachineTemplate1.Spec + spec2 = typedinfraMachineTemplate2.Spec + metadata1 = typedInfraMachineTemplate1.ObjectMeta + metadata2 = typedinfraMachineTemplate2.ObjectMeta + status1 = typedInfraMachineTemplate1.Status + status2 = typedinfraMachineTemplate2.Status + default: + return nil, fmt.Errorf("%w: %s", errPlatformNotSupported, platform) + } - if diffObjectMeta := deep.Equal(typedInfraMachineTemplate1.ObjectMeta, typedinfraMachineTemplate2.ObjectMeta); len(diffObjectMeta) > 0 { - diff[".metadata"] = diffObjectMeta - } + // Compare metadata + if diffMetadata, err := util.ObjectMetaEqual(metadata1, metadata2); err != nil { + return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine metadata: %w", err) + } else if diffMetadata.Changed() { + diff[".metadata"] = diffMetadata.String() + } - // TODO: Evaluate if we want to add status comparison if needed in the future (e.g. for scale from zero capacity). + // Compare spec + if diffSpec, err := util.NewDiffer().Diff(spec1, spec2); err != nil { + return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine spec: %w", err) + } else if diffSpec.Changed() { + diff[".spec"] = diffSpec.String() + } - return diff, nil - default: - return nil, fmt.Errorf("%w: %s", errPlatformNotSupported, platform) + // Compare status + if diffStatus, err := util.NewDiffer(util.WithIgnoreConditionsLastTransitionTime()).Diff(status1, status2); err != nil { + return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine status: %w", err) + } else if diffStatus.Changed() { + diff[".status"] = diffStatus.String() } + + return diff, nil } // compareCAPIMachineSets compares CAPI machineSets a and b, and returns a list of differences, or none if there are none. func compareCAPIMachineSets(capiMachineSet1, capiMachineSet2 *clusterv1.MachineSet) (map[string]any, error) { diff := make(map[string]any) - if diffSpec := deep.Equal(capiMachineSet1.Spec, capiMachineSet2.Spec); len(diffSpec) > 0 { - diff[".spec"] = diffSpec + // Compare metadata + if diffMetadata, err := util.ObjectMetaEqual(capiMachineSet1.ObjectMeta, capiMachineSet2.ObjectMeta); err != nil { + return nil, fmt.Errorf("failed to compare Cluster sAPI machine set metadata: %w", err) + } else if diffMetadata.Changed() { + diff[".metadata"] = diffMetadata.String() } - if diffObjectMeta := util.ObjectMetaEqual(capiMachineSet1.ObjectMeta, capiMachineSet2.ObjectMeta); len(diffObjectMeta) > 0 { - diff[".metadata"] = diffObjectMeta + // Compare spec + if diffSpec, err := util.NewDiffer().Diff(capiMachineSet1.Spec, capiMachineSet2.Spec); err != nil { + return nil, fmt.Errorf("failed to compare Cluster API machine spec: %w", err) + } else if diffSpec.Changed() { + diff[".spec"] = diffSpec.String() } + // Compare status if diffStatus, err := util.CAPIMachineSetStatusEqual(capiMachineSet1.Status, capiMachineSet2.Status); err != nil { - return nil, fmt.Errorf("failed to compare CAPI machine set status: %w", err) + return nil, fmt.Errorf("failed to compare Cluster API machine set status: %w", err) } else if diffStatus.Changed() { diff[".status"] = diffStatus.String() } @@ -1478,27 +1492,34 @@ func compareMAPIMachineSets(a, b *mapiv1beta1.MachineSet) (map[string]any, error return ps2.Tags[i].Name < ps2.Tags[j].Name }) - if diffProviderSpec := deep.Equal(ps1, ps2); len(diffProviderSpec) > 0 { - diff[".providerSpec"] = diffProviderSpec + // Compare providerSpec + if diffProviderSpec, err := util.NewDiffer().Diff(ps1, ps2); err != nil { + return nil, fmt.Errorf("failed to compare Machine API machine spec: %w", err) + } else if diffProviderSpec.Changed() { + diff[".providerSpec"] = diffProviderSpec.String() } - // Remove the providerSpec from the Spec as we've already compared them. - aCopy := a.DeepCopy() - aCopy.Spec.Template.Spec.ProviderSpec.Value = nil - - bCopy := b.DeepCopy() - bCopy.Spec.Template.Spec.ProviderSpec.Value = nil - - if diffSpec := deep.Equal(aCopy.Spec, bCopy.Spec); len(diffSpec) > 0 { - diff[".spec"] = diffSpec + // Compare metadata + if diffMetadata, err := util.ObjectMetaEqual(a.ObjectMeta, b.ObjectMeta); err != nil { + return nil, fmt.Errorf("failed to compare Machine API machine set metadata: %w", err) + } else if diffMetadata.Changed() { + diff[".metadata"] = diffMetadata.String() } - if diffMetadata := util.ObjectMetaEqual(aCopy.ObjectMeta, bCopy.ObjectMeta); len(diffMetadata) > 0 { - diff[".metadata"] = diffMetadata + // Compare spec + if diffSpec, err := util.NewDiffer( + util.WithIgnoreField("template", "providerSpec", "value"), + ).Diff(a.Spec, b.Spec); err != nil { + return nil, fmt.Errorf("failed to compare Machine API machine spec: %w", err) + } else if diffSpec.Changed() { + diff[".spec"] = diffSpec.String() } - if diffStatus := util.MAPIMachineSetStatusEqual(a.Status, b.Status); len(diffStatus) > 0 { - diff[".status"] = diffStatus + // Compare status + if diffStatus, err := util.MAPIMachineSetStatusEqual(a.Status, b.Status); err != nil { + return nil, fmt.Errorf("failed to compare Machine API machine set status: %w", err) + } else if diffStatus.Changed() { + diff[".status"] = diffStatus.String() } return diff, nil diff --git a/pkg/controllers/machinesync/machine_sync_controller.go b/pkg/controllers/machinesync/machine_sync_controller.go index 36e280e1e..d5c7586ac 100644 --- a/pkg/controllers/machinesync/machine_sync_controller.go +++ b/pkg/controllers/machinesync/machine_sync_controller.go @@ -36,7 +36,6 @@ import ( "github.com/openshift/cluster-capi-operator/pkg/conversion/mapi2capi" "github.com/openshift/cluster-capi-operator/pkg/util" - "github.com/go-test/deep" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -727,7 +726,10 @@ func (r *MachineSyncReconciler) createOrUpdateCAPIMachine(ctx context.Context, s } // Compare the existing CAPI machine with the desired CAPI machine to check for changes. - capiMachinesDiff := compareCAPIMachines(existingCAPIMachine, convertedCAPIMachine) + capiMachinesDiff, err := compareCAPIMachines(existingCAPIMachine, convertedCAPIMachine) + if err != nil { + return nil, fmt.Errorf("failed to compare Cluster API machines: %w", err) + } // Don't try to update the spec of the machine if it was just created it. // The resourceVersion of the convertedCAPIMachine is empty and would lead to failure. @@ -1291,22 +1293,31 @@ func (r *MachineSyncReconciler) ensureSyncFinalizer(ctx context.Context, mapiMac } // compareCAPIMachines compares CAPI machines a and b, and returns a list of differences, or none if there are none. -func compareCAPIMachines(capiMachine1, capiMachine2 *clusterv1.Machine) map[string]any { +func compareCAPIMachines(capiMachine1, capiMachine2 *clusterv1.Machine) (map[string]any, error) { diff := make(map[string]any) - if diffSpec := deep.Equal(capiMachine1.Spec, capiMachine2.Spec); len(diffSpec) > 0 { - diff[".spec"] = diffSpec + // Compare metadata + if diffMetadata, err := util.ObjectMetaEqual(capiMachine1.ObjectMeta, capiMachine2.ObjectMeta); err != nil { + return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine metadata: %w", err) + } else if diffMetadata.Changed() { + diff[".metadata"] = diffMetadata.String() } - if diffObjectMeta := util.ObjectMetaEqual(capiMachine1.ObjectMeta, capiMachine2.ObjectMeta); len(diffObjectMeta) > 0 { - diff[".metadata"] = diffObjectMeta + // Compare spec + if diffSpec, err := util.NewDiffer().Diff(capiMachine1.Spec, capiMachine2.Spec); err != nil { + return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine spec: %w", err) + } else if diffSpec.Changed() { + diff[".spec"] = diffSpec.String() } - if diffStatus := util.CAPIMachineStatusEqual(capiMachine1.Status, capiMachine2.Status); len(diffStatus) > 0 { - diff[".status"] = diffStatus + // Compare status + if diffStatus, err := util.NewDiffer(util.WithIgnoreConditionsLastTransitionTime()).Diff(capiMachine1.Status, capiMachine2.Status); err != nil { + return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine status: %w", err) + } else if diffStatus.Changed() { + diff[".status"] = diffStatus.String() } - return diff + return diff, nil } // compareMAPIMachines compares MAPI machines a and b, and returns a list of differences, or none if there are none. @@ -1337,27 +1348,34 @@ func compareMAPIMachines(a, b *mapiv1beta1.Machine) (map[string]any, error) { return ps2.Tags[i].Name < ps2.Tags[j].Name }) - if diffProviderSpec := deep.Equal(ps1, ps2); len(diffProviderSpec) > 0 { - diff[".providerSpec"] = diffProviderSpec + // Compare providerSpec + if diffProviderSpec, err := util.NewDiffer().Diff(ps1, ps2); err != nil { + return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine metadata: %w", err) + } else if diffProviderSpec.Changed() { + diff[".providerSpec"] = diffProviderSpec.String() } - // Remove the providerSpec from the Spec as we've already compared them. - aCopy := a.DeepCopy() - aCopy.Spec.ProviderSpec.Value = nil - - bCopy := b.DeepCopy() - bCopy.Spec.ProviderSpec.Value = nil - - if diffSpec := deep.Equal(aCopy.Spec, bCopy.Spec); len(diffSpec) > 0 { - diff[".spec"] = diffSpec + // Compare metadata + if diffMetadata, err := util.ObjectMetaEqual(a.ObjectMeta, b.ObjectMeta); err != nil { + return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine metadata: %w", err) + } else if diffMetadata.Changed() { + diff[".metadata"] = diffMetadata.String() } - if diffObjectMeta := util.ObjectMetaEqual(aCopy.ObjectMeta, bCopy.ObjectMeta); len(diffObjectMeta) > 0 { - diff[".metadata"] = diffObjectMeta + // Compare spec + if diffSpec, err := util.NewDiffer( + util.WithIgnoreField("providerSpec", "value"), + ).Diff(a.Spec, b.Spec); err != nil { + return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine spec: %w", err) + } else if diffSpec.Changed() { + diff[".spec"] = diffSpec.String() } - if diffStatus := util.MAPIMachineStatusEqual(a.Status, b.Status); len(diffStatus) > 0 { - diff[".status"] = diffStatus + // Compare status + if diffStatus, err := util.MAPIMachineStatusEqual(a.Status, b.Status); err != nil { + return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine status: %w", err) + } else if diffStatus.Changed() { + diff[".status"] = diffStatus.String() } return diff, nil diff --git a/pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go b/pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go index 3edf96b28..37dd5750c 100644 --- a/pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go +++ b/pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go @@ -20,12 +20,12 @@ import ( "context" "fmt" - "github.com/go-test/deep" configv1 "github.com/openshift/api/config/v1" mapiv1beta1 "github.com/openshift/api/machine/v1beta1" "github.com/openshift/cluster-capi-operator/pkg/util" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" utilerrors "k8s.io/apimachinery/pkg/util/errors" awsv1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2" ibmpowervsv1 "sigs.k8s.io/cluster-api-provider-ibmcloud/api/v1beta2" @@ -247,10 +247,14 @@ func (r *MachineSyncReconciler) ensureCAPIInfraMachineDeleted(ctx context.Contex // compareCAPIInfraMachines compares Cluster API infra machines a and b, and returns a list of differences, or none if there are none. // -//nolint:funlen,gocognit +//nolint:funlen func compareCAPIInfraMachines(platform configv1.PlatformType, infraMachine1, infraMachine2 client.Object) (map[string]any, error) { diff := make(map[string]any) + var metadata1, metadata2 metav1.ObjectMeta + + var spec1, spec2, status1, status2 any + switch platform { case configv1.AWSPlatformType: typedInfraMachine1, ok := infraMachine1.(*awsv1.AWSMachine) @@ -263,17 +267,12 @@ func compareCAPIInfraMachines(platform configv1.PlatformType, infraMachine1, inf return nil, errAssertingCAPIAWSMachine } - if diffSpec := deep.Equal(typedInfraMachine1.Spec, typedinfraMachine2.Spec); len(diffSpec) > 0 { - diff[".spec"] = diffSpec - } - - if diffMetadata := util.ObjectMetaEqual(typedInfraMachine1.ObjectMeta, typedinfraMachine2.ObjectMeta); len(diffMetadata) > 0 { - diff[".metadata"] = diffMetadata - } - - if diffStatus := deep.Equal(typedInfraMachine1.Status, typedinfraMachine2.Status); len(diffStatus) > 0 { - diff[".status"] = diffStatus - } + spec1 = typedInfraMachine1.Spec + spec2 = typedinfraMachine2.Spec + metadata1 = typedInfraMachine1.ObjectMeta + metadata2 = typedinfraMachine2.ObjectMeta + status1 = typedInfraMachine1.Status + status2 = typedinfraMachine2.Status case configv1.OpenStackPlatformType: typedInfraMachine1, ok := infraMachine1.(*openstackv1.OpenStackMachine) if !ok { @@ -285,17 +284,12 @@ func compareCAPIInfraMachines(platform configv1.PlatformType, infraMachine1, inf return nil, errAssertingCAPIOpenStackMachine } - if diffSpec := deep.Equal(typedInfraMachine1.Spec, typedinfraMachine2.Spec); len(diffSpec) > 0 { - diff[".spec"] = diffSpec - } - - if diffMetadata := util.ObjectMetaEqual(typedInfraMachine1.ObjectMeta, typedinfraMachine2.ObjectMeta); len(diffMetadata) > 0 { - diff[".metadata"] = diffMetadata - } - - if diffStatus := deep.Equal(typedInfraMachine1.Status, typedinfraMachine2.Status); len(diffStatus) > 0 { - diff[".status"] = diffStatus - } + spec1 = typedInfraMachine1.Spec + spec2 = typedinfraMachine2.Spec + metadata1 = typedInfraMachine1.ObjectMeta + metadata2 = typedinfraMachine2.ObjectMeta + status1 = typedInfraMachine1.Status + status2 = typedinfraMachine2.Status case configv1.PowerVSPlatformType: typedInfraMachine1, ok := infraMachine1.(*ibmpowervsv1.IBMPowerVSMachine) if !ok { @@ -307,20 +301,35 @@ func compareCAPIInfraMachines(platform configv1.PlatformType, infraMachine1, inf return nil, errAssertingCAPIIBMPowerVSMachine } - if diffSpec := deep.Equal(typedInfraMachine1.Spec, typedinfraMachine2.Spec); len(diffSpec) > 0 { - diff[".spec"] = diffSpec - } + spec1 = typedInfraMachine1.Spec + spec2 = typedinfraMachine2.Spec + metadata1 = typedInfraMachine1.ObjectMeta + metadata2 = typedinfraMachine2.ObjectMeta + status1 = typedInfraMachine1.Status + status2 = typedinfraMachine2.Status + default: + return nil, fmt.Errorf("%w: %s", errPlatformNotSupported, platform) + } - if diffMetadata := util.ObjectMetaEqual(typedInfraMachine1.ObjectMeta, typedinfraMachine2.ObjectMeta); len(diffMetadata) > 0 { - diff[".metadata"] = diffMetadata - } + // Compare metadata + if diffMetadata, err := util.ObjectMetaEqual(metadata1, metadata2); err != nil { + return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine metadata: %w", err) + } else if diffMetadata.Changed() { + diff[".metadata"] = diffMetadata.String() + } - if diffStatus := deep.Equal(typedInfraMachine1.Status, typedinfraMachine2.Status); len(diffStatus) > 0 { - diff[".status"] = diffStatus - } + // Compare spec + if diffSpec, err := util.NewDiffer().Diff(spec1, spec2); err != nil { + return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine spec: %w", err) + } else if diffSpec.Changed() { + diff[".spec"] = diffSpec.String() + } - default: - return nil, fmt.Errorf("%w: %s", errPlatformNotSupported, platform) + // Compare status + if diffStatus, err := util.NewDiffer(util.WithIgnoreConditionsLastTransitionTime()).Diff(status1, status2); err != nil { + return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine status: %w", err) + } else if diffStatus.Changed() { + diff[".status"] = diffStatus.String() } return diff, nil diff --git a/pkg/util/sync.go b/pkg/util/sync.go index 439ce39f7..c5d8b17e1 100644 --- a/pkg/util/sync.go +++ b/pkg/util/sync.go @@ -18,7 +18,6 @@ package util import ( "reflect" - "github.com/go-test/deep" mapiv1beta1 "github.com/openshift/api/machine/v1beta1" machinev1applyconfigs "github.com/openshift/client-go/machine/applyconfigurations/machine/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -56,40 +55,35 @@ func hasSameState(i *mapiv1beta1.Condition, j *machinev1applyconfigs.ConditionAp // ObjectMetaEqual compares variables a and b, // and returns a list of differences, or nil if there are none, // for the fields we care about when synchronising MAPI and CAPI Machines. -func ObjectMetaEqual(a, b metav1.ObjectMeta) map[string]any { - objectMetaDiff := map[string]any{} - - if diffLabels := deep.Equal(a.Labels, b.Labels); len(diffLabels) > 0 { - objectMetaDiff[".labels"] = diffLabels - } - - // Ignore conversion-data because this data is managed by Cluster API for down conversion. - aAnnotations := a.DeepCopy().GetAnnotations() - delete(aAnnotations, "cluster.x-k8s.io/conversion-data") - - bAnnotations := b.DeepCopy().GetAnnotations() - delete(bAnnotations, "cluster.x-k8s.io/conversion-data") - - if diffAnnotations := deep.Equal(aAnnotations, bAnnotations); len(diffAnnotations) > 0 { - objectMetaDiff[".annotations"] = diffAnnotations - } - - // Ignore the differences in finalizers, as CAPI always put finalizers on its resources - // even when its controllers are paused: - // https://github.com/kubernetes-sigs/cluster-api/blob/c70dca0fc387b44457c69b71a719132a0d9bed58/internal/controllers/machine/machine_controller.go#L207-L210 - - if diffOwnerReferences := deep.Equal(a.OwnerReferences, b.OwnerReferences); len(diffOwnerReferences) > 0 { - objectMetaDiff[".ownerReferences"] = diffOwnerReferences - } +func ObjectMetaEqual(a, b metav1.ObjectMeta) (DiffResult, error) { + differ := NewDiffer( + // Special handling for CAPI's conversion-data label. + WithIgnoreField("annotations", "cluster.x-k8s.io/conversion-data"), + + WithIgnoreField("name"), + WithIgnoreField("generateName"), + WithIgnoreField("namespace"), + WithIgnoreField("selfLink"), + WithIgnoreField("uid"), + WithIgnoreField("resourceVersion"), + WithIgnoreField("generation"), + WithIgnoreField("creationTimestamp"), + WithIgnoreField("deletionTimestamp"), + WithIgnoreField("deletionGracePeriodSeconds"), + WithIgnoreField("finalizers"), + WithIgnoreField("managedFields"), + ) - return objectMetaDiff + return differ.Diff(a, b) } // CAPIMachineSetStatusEqual compares variables a and b, // and returns a list of differences, or nil if there are none, // for the fields we care about when synchronising MAPI and CAPI Machines. func CAPIMachineSetStatusEqual(a, b clusterv1.MachineSetStatus) (DiffResult, error) { - differ := newDiffer( + differ := NewDiffer( + // Changes to a condition's LastTransitionTime should not cause updates because it should only change + // if the status itself changes. WithIgnoreConditionsLastTransitionTime(), ) @@ -99,203 +93,50 @@ func CAPIMachineSetStatusEqual(a, b clusterv1.MachineSetStatus) (DiffResult, err // CAPIMachineStatusEqual compares variables a and b, // and returns a list of differences, or nil if there are none, // for the fields we care about when synchronising CAPI and MAPI Machines. -func CAPIMachineStatusEqual(a, b clusterv1.MachineStatus) map[string]any { - diff := map[string]any{} - - if diffFailureReason := deep.Equal(a.FailureReason, b.FailureReason); len(diffFailureReason) > 0 { - diff[".failureReason"] = diffFailureReason - } - - if diffFailureMessage := deep.Equal(a.FailureMessage, b.FailureMessage); len(diffFailureMessage) > 0 { - diff[".failureMessage"] = diffFailureMessage - } - - if diffLastUpdated := deep.Equal(a.LastUpdated, b.LastUpdated); len(diffLastUpdated) > 0 { - diff[".lastUpdated"] = diffLastUpdated - } - - if diffPhase := deep.Equal(a.Phase, b.Phase); len(diffPhase) > 0 { - diff[".phase"] = diffPhase - } - - if diffAddresses := deep.Equal(a.Addresses, b.Addresses); len(diffAddresses) > 0 { - diff[".addresses"] = diffAddresses - } - - if diffBootstrapReady := deep.Equal(a.BootstrapReady, b.BootstrapReady); len(diffBootstrapReady) > 0 { - diff[".bootstrapReady"] = diffBootstrapReady - } - - if diffInfrastructureReady := deep.Equal(a.InfrastructureReady, b.InfrastructureReady); len(diffInfrastructureReady) > 0 { - diff[".infrastructureReady"] = diffInfrastructureReady - } - - if diffNodeInfo := deep.Equal(a.NodeInfo, b.NodeInfo); len(diffNodeInfo) > 0 { - diff[".nodeInfo"] = diffNodeInfo - } - - if diffConditions := compareCAPIV1Beta1Conditions(a.Conditions, b.Conditions); len(diffConditions) > 0 { - diff[".conditions"] = diffConditions - } - - // Compare the v1beta2 fields. - if a.V1Beta2 == nil { - a.V1Beta2 = &clusterv1.MachineV1Beta2Status{} - } - - if b.V1Beta2 == nil { - b.V1Beta2 = &clusterv1.MachineV1Beta2Status{} - } - - if diffConditions := compareCAPIV1Beta2Conditions(a.V1Beta2.Conditions, b.V1Beta2.Conditions); len(diffConditions) > 0 { - diff[".v1beta2.conditions"] = diffConditions - } +func CAPIMachineStatusEqual(a, b clusterv1.MachineStatus) (DiffResult, error) { + differ := NewDiffer( + // Changes to a condition's LastTransitionTime should not cause updates because it should only change + // if the status itself changes. + WithIgnoreConditionsLastTransitionTime(), + ) - return diff + return differ.Diff(a, b) } // MAPIMachineStatusEqual compares variables a and b, // and returns a list of differences, or nil if there are none, // for the fields we care about when synchronising CAPI and MAPI Machines. -func MAPIMachineStatusEqual(a, b mapiv1beta1.MachineStatus) map[string]any { - diff := map[string]any{} - - if diffConditions := compareMAPIV1Beta1Conditions(a.Conditions, b.Conditions); len(diffConditions) > 0 { - diff[".conditions"] = diffConditions - } - - if diffErrorReason := deep.Equal(a.ErrorReason, b.ErrorReason); len(diffErrorReason) > 0 { - diff[".errorReason"] = diffErrorReason - } - - if diffErrorMessage := deep.Equal(a.ErrorMessage, b.ErrorMessage); len(diffErrorMessage) > 0 { - diff[".errorMessage"] = diffErrorMessage - } - - if diffPhase := deep.Equal(a.Phase, b.Phase); len(diffPhase) > 0 { - diff[".phase"] = diffPhase - } - - if diffAddresses := deep.Equal(a.Addresses, b.Addresses); len(diffAddresses) > 0 { - diff[".addresses"] = diffAddresses - } - - if diffNodeRef := deep.Equal(a.NodeRef, b.NodeRef); len(diffNodeRef) > 0 { - diff[".nodeRef"] = diffNodeRef - } - - if diffLastUpdated := deep.Equal(a.LastUpdated, b.LastUpdated); len(diffLastUpdated) > 0 { - diff[".lastUpdated"] = diffLastUpdated - } - - return diff -} - -// compareCAPIV1Beta1Conditions compares variables a and b, -// and returns a list of differences, or nil if there are none, -// for the fields we care about when synchronising CAPI v1beta1 and MAPI. -func compareCAPIV1Beta1Conditions(a, b []clusterv1.Condition) []string { - diff := []string{} - // Compare the conditions one by one. - // Ignore the differences in LastTransitionTime. - for _, condition := range a { - for _, conditionB := range b { - if condition.Type == conditionB.Type { - if condition.Status != conditionB.Status || - condition.Severity != conditionB.Severity || - condition.Reason != conditionB.Reason || - condition.Message != conditionB.Message { - diff = append(diff, deep.Equal(condition, conditionB)...) - } - - break // Break out of the inner loop once we have found a match. - } - } - } - - return diff -} - -// compareMAPIV1Beta1Conditions compares variables a and b, -// and returns a list of differences, or nil if there are none, -// for the fields we care about when synchronising MAPI v1beta1 and CAPI. -func compareMAPIV1Beta1Conditions(a, b []mapiv1beta1.Condition) []string { - diff := []string{} - // Compare the conditions one by one. - // Ignore the differences in LastTransitionTime. - for _, condition := range a { - for _, conditionB := range b { - if condition.Type == conditionB.Type { - if condition.Status != conditionB.Status || - condition.Severity != conditionB.Severity || - condition.Reason != conditionB.Reason || - condition.Message != conditionB.Message { - diff = append(diff, deep.Equal(condition, conditionB)...) - } - - break // Break out of the inner loop once we have found a match. - } - } - } - - return diff -} - -// compareCAPIV1Beta2Conditions compares variables a and b, -// and returns a list of differences, or nil if there are none, -// for the fields we care about when synchronising CAPI v1beta2 and MAPI. -func compareCAPIV1Beta2Conditions(a, b []metav1.Condition) []string { - diff := []string{} - // Compare the conditions one by one. - // Ignore the differences in LastTransitionTime. - for _, condition := range a { - for _, conditionB := range b { - if condition.Type == conditionB.Type { - if condition.Status != conditionB.Status || - condition.Reason != conditionB.Reason || - condition.Message != conditionB.Message { - diff = append(diff, deep.Equal(condition, conditionB)...) - } +func MAPIMachineStatusEqual(a, b mapiv1beta1.MachineStatus) (DiffResult, error) { + differ := NewDiffer( + // Changes to a condition's LastTransitionTime should not cause updates because it should only change + // if the status itself changes. + WithIgnoreConditionsLastTransitionTime(), - break // Break out of the inner loop once we have found a match. - } - } - } + WithIgnoreField("providerStatus"), + WithIgnoreField("lastOperation"), + WithIgnoreField("authoritativeAPI"), + WithIgnoreField("synchronizedGeneration"), + ) - return diff + return differ.Diff(a, b) } // MAPIMachineSetStatusEqual compares variables a and b, // and returns a list of differences, or nil if there are none, // for the fields we care about when synchronising MAPI and CAPI Machines. -func MAPIMachineSetStatusEqual(a, b mapiv1beta1.MachineSetStatus) map[string]any { - diff := map[string]any{} - - if diffReadyReplicas := deep.Equal(a.ReadyReplicas, b.ReadyReplicas); len(diffReadyReplicas) > 0 { - diff[".readyReplicas"] = diffReadyReplicas - } - - if diffAvailableReplicas := deep.Equal(a.AvailableReplicas, b.AvailableReplicas); len(diffAvailableReplicas) > 0 { - diff[".availableReplicas"] = diffAvailableReplicas - } - - if diffFullyLabeledReplicas := deep.Equal(a.FullyLabeledReplicas, b.FullyLabeledReplicas); len(diffFullyLabeledReplicas) > 0 { - diff[".fullyLabeledReplicas"] = diffFullyLabeledReplicas - } - - if diffErrorReason := deep.Equal(a.ErrorReason, b.ErrorReason); len(diffErrorReason) > 0 { - diff[".errorReason"] = diffErrorReason - } - - if diffErrorMessage := deep.Equal(a.ErrorMessage, b.ErrorMessage); len(diffErrorMessage) > 0 { - diff[".errorMessage"] = diffErrorMessage - } +func MAPIMachineSetStatusEqual(a, b mapiv1beta1.MachineSetStatus) (DiffResult, error) { + differ := NewDiffer( + // Changes to a condition's LastTransitionTime should not cause updates because it should only change + // if the status itself changes. + WithIgnoreConditionsLastTransitionTime(), - if diffConditions := compareMAPIV1Beta1Conditions(a.Conditions, b.Conditions); len(diffConditions) > 0 { - diff[".conditions"] = diffConditions - } + WithIgnoreField("replicas"), + WithIgnoreField("observedGeneration"), + WithIgnoreField("authoritativeAPI"), + WithIgnoreField("synchronizedGeneration"), + ) - return diff + return differ.Diff(a, b) } // GetResourceVersion returns the object ResourceVersion or the zero value for it. From 2faedc0b49479946da97d14cc357c94027ea4752 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Fri, 31 Oct 2025 09:04:20 +0100 Subject: [PATCH 05/19] always compare client.Object --- .../machineset_sync_controller.go | 179 ++++------------- .../machineset_sync_controller_test.go | 9 +- .../machineset_sync_controller_unit_test.go} | 29 +-- .../machinesync/machine_sync_controller.go | 132 +++---------- .../machine_sync_mapi2capi_infrastructure.go | 67 ++----- pkg/conversion/mapi2capi/util.go | 28 +++ pkg/util/diff.go | 186 ++++++++++++++++-- pkg/util/diff_test.go | 105 ++++++---- pkg/util/sync.go | 88 --------- 9 files changed, 358 insertions(+), 465 deletions(-) rename pkg/{util/sync_test.go => controllers/machinesetsync/machineset_sync_controller_unit_test.go} (91%) diff --git a/pkg/controllers/machinesetsync/machineset_sync_controller.go b/pkg/controllers/machinesetsync/machineset_sync_controller.go index f2864ff00..8bf1ae667 100644 --- a/pkg/controllers/machinesetsync/machineset_sync_controller.go +++ b/pkg/controllers/machinesetsync/machineset_sync_controller.go @@ -21,7 +21,6 @@ import ( "errors" "fmt" "slices" - "sort" "github.com/go-logr/logr" configv1 "github.com/openshift/api/config/v1" @@ -774,7 +773,7 @@ func (r *MachineSetSyncReconciler) createOrUpdateCAPIInfraMachineTemplate(ctx co return updateErr } - if len(capiInfraMachineTemplatesDiff) == 0 { + if capiInfraMachineTemplatesDiff.HasChanges() { logger.Info("No changes detected for CAPI infra machine template") return nil } @@ -875,11 +874,11 @@ func (r *MachineSetSyncReconciler) ensureCAPIMachineSet(ctx context.Context, sou } // ensureCAPIMachineSetSpecUpdated updates the CAPI machine set if changes are detected. -func (r *MachineSetSyncReconciler) ensureCAPIMachineSetSpecUpdated(ctx context.Context, mapiMachineSet *mapiv1beta1.MachineSet, capiMachineSetsDiff map[string]any, updatedOrCreatedCAPIMachineSet *clusterv1.MachineSet) (bool, error) { +func (r *MachineSetSyncReconciler) ensureCAPIMachineSetSpecUpdated(ctx context.Context, mapiMachineSet *mapiv1beta1.MachineSet, capiMachineSetsDiff util.DiffResult, updatedOrCreatedCAPIMachineSet *clusterv1.MachineSet) (bool, error) { logger := logf.FromContext(ctx) // If there are no spec changes, return early. - if !hasSpecOrMetadataOrProviderSpecChanges(capiMachineSetsDiff) { + if !(capiMachineSetsDiff.HasMetadataChanges() || capiMachineSetsDiff.HasSpecChanges() || capiMachineSetsDiff.HasProviderSpecChanges()) { return false, nil } @@ -901,11 +900,11 @@ func (r *MachineSetSyncReconciler) ensureCAPIMachineSetSpecUpdated(ctx context.C } // ensureCAPIMachineSetStatusUpdated updates the CAPI machine set status if changes are detected and conditions are met. -func (r *MachineSetSyncReconciler) ensureCAPIMachineSetStatusUpdated(ctx context.Context, mapiMachineSet *mapiv1beta1.MachineSet, existingCAPIMachineSet *clusterv1.MachineSet, convertedCAPIMachineSet *clusterv1.MachineSet, updatedOrCreatedCAPIMachineSet *clusterv1.MachineSet, capiMachineSetsDiff map[string]any, specUpdated bool) (bool, error) { +func (r *MachineSetSyncReconciler) ensureCAPIMachineSetStatusUpdated(ctx context.Context, mapiMachineSet *mapiv1beta1.MachineSet, existingCAPIMachineSet *clusterv1.MachineSet, convertedCAPIMachineSet *clusterv1.MachineSet, updatedOrCreatedCAPIMachineSet *clusterv1.MachineSet, capiMachineSetsDiff util.DiffResult, specUpdated bool) (bool, error) { logger := logf.FromContext(ctx) // If there are no status changes and the spec has not been updated, return early. - if !hasStatusChanges(capiMachineSetsDiff) && !specUpdated { + if !capiMachineSetsDiff.HasStatusChanges() && !specUpdated { return false, nil } @@ -952,11 +951,11 @@ func (r *MachineSetSyncReconciler) ensureCAPIMachineSetStatusUpdated(ctx context } // ensureMAPIMachineSetSpecUpdated updates the MAPI machine set if changes are detected. -func (r *MachineSetSyncReconciler) ensureMAPIMachineSetSpecUpdated(ctx context.Context, mapiMachineSet *mapiv1beta1.MachineSet, mapiMachineSetsDiff map[string]any, updatedMAPIMachineSet *mapiv1beta1.MachineSet) (bool, error) { +func (r *MachineSetSyncReconciler) ensureMAPIMachineSetSpecUpdated(ctx context.Context, mapiMachineSet *mapiv1beta1.MachineSet, mapiMachineSetsDiff util.DiffResult, updatedMAPIMachineSet *mapiv1beta1.MachineSet) (bool, error) { logger := logf.FromContext(ctx) // If there are no spec changes, return early. - if !hasSpecOrMetadataOrProviderSpecChanges(mapiMachineSetsDiff) { + if !(mapiMachineSetsDiff.HasMetadataChanges() || mapiMachineSetsDiff.HasSpecChanges() || mapiMachineSetsDiff.HasProviderSpecChanges()) { return false, nil } @@ -979,11 +978,11 @@ func (r *MachineSetSyncReconciler) ensureMAPIMachineSetSpecUpdated(ctx context.C } // ensureMAPIMachineSetStatusUpdated updates the MAPI machine set status if changes are detected and conditions are met. -func (r *MachineSetSyncReconciler) ensureMAPIMachineSetStatusUpdated(ctx context.Context, existingMAPIMachineSet *mapiv1beta1.MachineSet, convertedMAPIMachineSet *mapiv1beta1.MachineSet, updatedMAPIMachineSet *mapiv1beta1.MachineSet, sourceCAPIMachineSet *clusterv1.MachineSet, mapiMachineSetsDiff map[string]any, specUpdated bool) (bool, error) { +func (r *MachineSetSyncReconciler) ensureMAPIMachineSetStatusUpdated(ctx context.Context, existingMAPIMachineSet *mapiv1beta1.MachineSet, convertedMAPIMachineSet *mapiv1beta1.MachineSet, updatedMAPIMachineSet *mapiv1beta1.MachineSet, sourceCAPIMachineSet *clusterv1.MachineSet, mapiMachineSetsDiff util.DiffResult, specUpdated bool) (bool, error) { logger := logf.FromContext(ctx) // If there are no status changes and the spec has not been updated, return early. - if !hasStatusChanges(mapiMachineSetsDiff) && !specUpdated { + if !mapiMachineSetsDiff.HasStatusChanges() && !specUpdated { return false, nil } @@ -1053,7 +1052,7 @@ func (r *MachineSetSyncReconciler) updateMAPIMachineSet(ctx context.Context, exi // Here we always assume the existingMAPIMachineSet already exists, so we don't need to create it. - mapiMachineSetsDiff, err := compareMAPIMachineSets(existingMAPIMachineSet, convertedMAPIMachineSet) + mapiMachineSetsDiff, err := compareMAPIMachineSets(r.Platform, existingMAPIMachineSet, convertedMAPIMachineSet) if err != nil { return fmt.Errorf("unable to compare MAPI machine sets: %w", err) } @@ -1349,14 +1348,8 @@ func initInfraMachineTemplateListAndInfraClusterListFromProvider(platform config } // compareCAPIInfraMachineTemplates compares CAPI infra machine templates a and b, and returns a list of differences, or none if there are none. -// -//nolint:funlen -func compareCAPIInfraMachineTemplates(platform configv1.PlatformType, infraMachineTemplate1, infraMachineTemplate2 client.Object) (map[string]any, error) { - diff := make(map[string]any) - - var metadata1, metadata2 metav1.ObjectMeta - - var spec1, spec2, status1, status2 any +func compareCAPIInfraMachineTemplates(platform configv1.PlatformType, infraMachineTemplate1, infraMachineTemplate2 client.Object) (util.DiffResult, error) { + var obj1, obj2 client.Object switch platform { case configv1.AWSPlatformType: @@ -1370,12 +1363,8 @@ func compareCAPIInfraMachineTemplates(platform configv1.PlatformType, infraMachi return nil, errAssertingCAPIAWSMachineTemplate } - spec1 = typedInfraMachineTemplate1.Spec - spec2 = typedinfraMachineTemplate2.Spec - metadata1 = typedInfraMachineTemplate1.ObjectMeta - metadata2 = typedinfraMachineTemplate2.ObjectMeta - status1 = typedInfraMachineTemplate1.Status - status2 = typedinfraMachineTemplate2.Status + obj1 = typedInfraMachineTemplate1 + obj2 = typedinfraMachineTemplate2 case configv1.OpenStackPlatformType: typedInfraMachineTemplate1, ok := infraMachineTemplate1.(*openstackv1.OpenStackMachineTemplate) if !ok { @@ -1387,10 +1376,8 @@ func compareCAPIInfraMachineTemplates(platform configv1.PlatformType, infraMachi return nil, errAssertingCAPIOpenStackMachineTemplate } - spec1 = typedInfraMachineTemplate1.Spec - spec2 = typedinfraMachineTemplate2.Spec - metadata1 = typedInfraMachineTemplate1.ObjectMeta - metadata2 = typedinfraMachineTemplate2.ObjectMeta + obj1 = typedInfraMachineTemplate1 + obj2 = typedinfraMachineTemplate2 case configv1.PowerVSPlatformType: typedInfraMachineTemplate1, ok := infraMachineTemplate1.(*ibmpowervsv1.IBMPowerVSMachineTemplate) if !ok { @@ -1402,124 +1389,44 @@ func compareCAPIInfraMachineTemplates(platform configv1.PlatformType, infraMachi return nil, errAssertingCAPIIBMPowerVSMachineTemplate } - spec1 = typedInfraMachineTemplate1.Spec - spec2 = typedinfraMachineTemplate2.Spec - metadata1 = typedInfraMachineTemplate1.ObjectMeta - metadata2 = typedinfraMachineTemplate2.ObjectMeta - status1 = typedInfraMachineTemplate1.Status - status2 = typedinfraMachineTemplate2.Status + obj1 = typedInfraMachineTemplate1 + obj2 = typedinfraMachineTemplate2 default: return nil, fmt.Errorf("%w: %s", errPlatformNotSupported, platform) } - // Compare metadata - if diffMetadata, err := util.ObjectMetaEqual(metadata1, metadata2); err != nil { - return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine metadata: %w", err) - } else if diffMetadata.Changed() { - diff[".metadata"] = diffMetadata.String() - } - - // Compare spec - if diffSpec, err := util.NewDiffer().Diff(spec1, spec2); err != nil { - return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine spec: %w", err) - } else if diffSpec.Changed() { - diff[".spec"] = diffSpec.String() - } - - // Compare status - if diffStatus, err := util.NewDiffer(util.WithIgnoreConditionsLastTransitionTime()).Diff(status1, status2); err != nil { - return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine status: %w", err) - } else if diffStatus.Changed() { - diff[".status"] = diffStatus.String() + diff, err := util.NewDefaultDiffer().Diff(obj1, obj2) + if err != nil { + return nil, fmt.Errorf("failed to compare Cluster API infrastructure machine templates: %w", err) } return diff, nil } // compareCAPIMachineSets compares CAPI machineSets a and b, and returns a list of differences, or none if there are none. -func compareCAPIMachineSets(capiMachineSet1, capiMachineSet2 *clusterv1.MachineSet) (map[string]any, error) { - diff := make(map[string]any) - - // Compare metadata - if diffMetadata, err := util.ObjectMetaEqual(capiMachineSet1.ObjectMeta, capiMachineSet2.ObjectMeta); err != nil { - return nil, fmt.Errorf("failed to compare Cluster sAPI machine set metadata: %w", err) - } else if diffMetadata.Changed() { - diff[".metadata"] = diffMetadata.String() - } - - // Compare spec - if diffSpec, err := util.NewDiffer().Diff(capiMachineSet1.Spec, capiMachineSet2.Spec); err != nil { - return nil, fmt.Errorf("failed to compare Cluster API machine spec: %w", err) - } else if diffSpec.Changed() { - diff[".spec"] = diffSpec.String() - } - - // Compare status - if diffStatus, err := util.CAPIMachineSetStatusEqual(capiMachineSet1.Status, capiMachineSet2.Status); err != nil { - return nil, fmt.Errorf("failed to compare Cluster API machine set status: %w", err) - } else if diffStatus.Changed() { - diff[".status"] = diffStatus.String() +func compareCAPIMachineSets(capiMachineSet1, capiMachineSet2 *clusterv1.MachineSet) (util.DiffResult, error) { + diff, err := util.NewDefaultDiffer().Diff(capiMachineSet1, capiMachineSet2) + if err != nil { + return nil, fmt.Errorf("failed to compare Cluster API machinesets: %w", err) } return diff, nil } // compareMAPIMachineSets compares MAPI machineSets a and b, and returns a list of differences, or none if there are none. -func compareMAPIMachineSets(a, b *mapiv1beta1.MachineSet) (map[string]any, error) { - diff := make(map[string]any) +func compareMAPIMachineSets(platform configv1.PlatformType, a, b *mapiv1beta1.MachineSet) (util.DiffResult, error) { + diff, err := util.NewDefaultDiffer( + util.WithProviderSpec(platform, []string{"spec", "template", "providerSpec", "value"}, mapi2capi.ProviderSpecFromRawExtension), - ps1, err := mapi2capi.AWSProviderSpecFromRawExtension(a.Spec.Template.Spec.ProviderSpec.Value) - if err != nil { - return nil, fmt.Errorf("unable to parse first MAPI machine set providerSpec: %w", err) - } + // Other status fields to ignore + util.WithIgnoreField("status", "replicas"), + util.WithIgnoreField("status", "observedGeneration"), + util.WithIgnoreField("status", "authoritativeAPI"), + util.WithIgnoreField("status", "synchronizedGeneration"), + ).Diff(a, b) - ps2, err := mapi2capi.AWSProviderSpecFromRawExtension(b.Spec.Template.Spec.ProviderSpec.Value) if err != nil { - return nil, fmt.Errorf("unable to parse second MAPI machine set providerSpec: %w", err) - } - - // Sort the tags by name to ensure consistent ordering. - // On the CAPI side these tags are in a map, - // so the order is not guaranteed when converting back from a CAPI map to a MAPI slice. - sort.Slice(ps1.Tags, func(i, j int) bool { - return ps1.Tags[i].Name < ps1.Tags[j].Name - }) - - // Sort the tags by name to ensure consistent ordering. - // On the CAPI side these tags are in a map, - // so the order is not guaranteed when converting back from a CAPI map to a MAPI slice. - sort.Slice(ps2.Tags, func(i, j int) bool { - return ps2.Tags[i].Name < ps2.Tags[j].Name - }) - - // Compare providerSpec - if diffProviderSpec, err := util.NewDiffer().Diff(ps1, ps2); err != nil { - return nil, fmt.Errorf("failed to compare Machine API machine spec: %w", err) - } else if diffProviderSpec.Changed() { - diff[".providerSpec"] = diffProviderSpec.String() - } - - // Compare metadata - if diffMetadata, err := util.ObjectMetaEqual(a.ObjectMeta, b.ObjectMeta); err != nil { - return nil, fmt.Errorf("failed to compare Machine API machine set metadata: %w", err) - } else if diffMetadata.Changed() { - diff[".metadata"] = diffMetadata.String() - } - - // Compare spec - if diffSpec, err := util.NewDiffer( - util.WithIgnoreField("template", "providerSpec", "value"), - ).Diff(a.Spec, b.Spec); err != nil { - return nil, fmt.Errorf("failed to compare Machine API machine spec: %w", err) - } else if diffSpec.Changed() { - diff[".spec"] = diffSpec.String() - } - - // Compare status - if diffStatus, err := util.MAPIMachineSetStatusEqual(a.Status, b.Status); err != nil { - return nil, fmt.Errorf("failed to compare Machine API machine set status: %w", err) - } else if diffStatus.Changed() { - diff[".status"] = diffStatus.String() + return nil, fmt.Errorf("failed to compare Machine API machinesets: %w", err) } return diff, nil @@ -1577,22 +1484,6 @@ func restoreMAPIFields(existingMAPIMachineSet, convertedMAPIMachineSet *mapiv1be convertedMAPIMachineSet.SetFinalizers(existingMAPIMachineSet.GetFinalizers()) } -// hasStatusChanges returns true if there are changes to the status. -func hasStatusChanges(diff map[string]any) bool { - _, ok := diff[".status"] - - return ok -} - -// hasSpecOrMetadataOrProviderSpecChanges returns true if there are changes to the spec, metadata, or providerSpec. -func hasSpecOrMetadataOrProviderSpecChanges(diff map[string]any) bool { - _, ok1 := diff[".spec"] - _, ok2 := diff[".metadata"] - _, ok3 := diff[".providerSpec"] - - return ok1 || ok2 || ok3 -} - // isTerminalConfigurationError returns true if the provided error is // errInvalidClusterInfraReference or errInvalidInfraMachineTemplateReference. func isTerminalConfigurationError(err error) bool { diff --git a/pkg/controllers/machinesetsync/machineset_sync_controller_test.go b/pkg/controllers/machinesetsync/machineset_sync_controller_test.go index 59bfc1217..943d39508 100644 --- a/pkg/controllers/machinesetsync/machineset_sync_controller_test.go +++ b/pkg/controllers/machinesetsync/machineset_sync_controller_test.go @@ -1190,18 +1190,17 @@ var _ = Describe("compareMAPIMachineSets", func() { Context("when comparing MachineSets with different instance types", func() { It("should detect differences in providerSpec", func() { - diff, err := compareMAPIMachineSets(mapiMachineSet1, mapiMachineSet2) + diff, err := compareMAPIMachineSets(configv1.AWSPlatformType, mapiMachineSet1, mapiMachineSet2) Expect(err).NotTo(HaveOccurred()) - Expect(diff).To(HaveKey(".providerSpec")) - Expect(diff[".providerSpec"]).NotTo(BeEmpty()) + Expect(diff.HasProviderSpecChanges()).To(BeTrue()) }) }) Context("when comparing identical MachineSets", func() { It("should detect no differences", func() { - diff, err := compareMAPIMachineSets(mapiMachineSet1, mapiMachineSet1) + diff, err := compareMAPIMachineSets(configv1.AWSPlatformType, mapiMachineSet1, mapiMachineSet1) Expect(err).NotTo(HaveOccurred()) - Expect(diff).To(BeEmpty()) + Expect(diff.HasChanges()).To(BeFalse()) }) }) }) diff --git a/pkg/util/sync_test.go b/pkg/controllers/machinesetsync/machineset_sync_controller_unit_test.go similarity index 91% rename from pkg/util/sync_test.go rename to pkg/controllers/machinesetsync/machineset_sync_controller_unit_test.go index 8736fd07f..72d6ae28e 100644 --- a/pkg/util/sync_test.go +++ b/pkg/controllers/machinesetsync/machineset_sync_controller_unit_test.go @@ -13,7 +13,7 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. */ -package util +package machinesetsync import ( "testing" @@ -54,7 +54,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { b: clusterv1.MachineSetStatus{ ReadyReplicas: 5, }, - want: ".[readyReplicas]: 3 != 5", + want: ".[status].[readyReplicas]: 3 != 5", wantChanges: true, }, { @@ -65,7 +65,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { b: clusterv1.MachineSetStatus{ AvailableReplicas: 4, }, - want: ".[availableReplicas]: 2 != 4", + want: ".[status].[availableReplicas]: 2 != 4", wantChanges: true, }, { @@ -78,7 +78,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { ReadyReplicas: 5, AvailableReplicas: 4, }, - want: ".[availableReplicas]: 2 != 4, .[readyReplicas]: 3 != 5", + want: ".[status].[availableReplicas]: 2 != 4, .[status].[readyReplicas]: 3 != 5", wantChanges: true, }, { @@ -124,7 +124,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, }, }, - want: ".[conditions][0].[status]: True != False", + want: ".[status].[conditions][0].[status]: True != False", wantChanges: true, }, { @@ -180,7 +180,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, }, }, - want: ".[conditions][1].[status]: True != False", + want: ".[status].[conditions][1].[status]: True != False", wantChanges: true, }, { @@ -238,7 +238,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, }, }, - want: ".[v1beta2].[conditions][0].[status]: True != False", + want: ".[status].[v1beta2].[conditions][0].[status]: True != False", wantChanges: true, }, { @@ -306,7 +306,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, }, }, - want: ".[v1beta2].[conditions][1].[status]: True != False", + want: ".[status].[v1beta2].[conditions][1].[status]: True != False", wantChanges: true, }, { @@ -319,7 +319,7 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { ReadyReplicas: ptr.To[int32](3), }, }, - want: ".[v1beta2]: != [readyReplicas:3]", + want: ".[status].[v1beta2]: != [readyReplicas:3]", wantChanges: true, }, } @@ -328,10 +328,17 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - got, err := CAPIMachineSetStatusEqual(tt.a, tt.b) + a := &clusterv1.MachineSet{ + Status: tt.a, + } + b := &clusterv1.MachineSet{ + Status: tt.b, + } + + got, err := compareCAPIMachineSets(a, b) g.Expect(err).ToNot(HaveOccurred()) - gotChanges := got.Changed() + gotChanges := got.HasChanges() g.Expect(gotChanges).To(Equal(tt.wantChanges)) diff --git a/pkg/controllers/machinesync/machine_sync_controller.go b/pkg/controllers/machinesync/machine_sync_controller.go index d5c7586ac..e66f30efb 100644 --- a/pkg/controllers/machinesync/machine_sync_controller.go +++ b/pkg/controllers/machinesync/machine_sync_controller.go @@ -21,7 +21,6 @@ import ( "errors" "fmt" "slices" - "sort" "strings" "time" @@ -678,11 +677,11 @@ func (r *MachineSyncReconciler) ensureCAPIMachine(ctx context.Context, sourceMAP } // ensureCAPIMachineSpecUpdated updates the Cluster API machine if changes are detected to the spec, metadata or provider spec. -func (r *MachineSyncReconciler) ensureCAPIMachineSpecUpdated(ctx context.Context, mapiMachine *mapiv1beta1.Machine, capiMachinesDiff map[string]any, convertedCAPIMachine *clusterv1.Machine) (bool, *clusterv1.Machine, error) { +func (r *MachineSyncReconciler) ensureCAPIMachineSpecUpdated(ctx context.Context, mapiMachine *mapiv1beta1.Machine, capiMachinesDiff util.DiffResult, convertedCAPIMachine *clusterv1.Machine) (bool, *clusterv1.Machine, error) { logger := logf.FromContext(ctx) // If there are no spec changes, return early. - if !hasSpecOrMetadataOrProviderSpecChanges(capiMachinesDiff) { + if !(capiMachinesDiff.HasMetadataChanges() || capiMachinesDiff.HasSpecChanges()) { return false, nil, nil } @@ -780,7 +779,7 @@ func (r *MachineSyncReconciler) createOrUpdateMAPIMachine(ctx context.Context, e // There already is an existing MAPI machine, work out if it needs updating. // Compare the existing MAPI machine with the converted MAPI machine to check for changes. - mapiMachinesDiff, err := compareMAPIMachines(existingMAPIMachine, convertedMAPIMachine) + mapiMachinesDiff, err := compareMAPIMachines(r.Platform, existingMAPIMachine, convertedMAPIMachine) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to compare Machine API machines: %w", err) } @@ -1293,102 +1292,41 @@ func (r *MachineSyncReconciler) ensureSyncFinalizer(ctx context.Context, mapiMac } // compareCAPIMachines compares CAPI machines a and b, and returns a list of differences, or none if there are none. -func compareCAPIMachines(capiMachine1, capiMachine2 *clusterv1.Machine) (map[string]any, error) { - diff := make(map[string]any) - - // Compare metadata - if diffMetadata, err := util.ObjectMetaEqual(capiMachine1.ObjectMeta, capiMachine2.ObjectMeta); err != nil { - return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine metadata: %w", err) - } else if diffMetadata.Changed() { - diff[".metadata"] = diffMetadata.String() - } - - // Compare spec - if diffSpec, err := util.NewDiffer().Diff(capiMachine1.Spec, capiMachine2.Spec); err != nil { - return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine spec: %w", err) - } else if diffSpec.Changed() { - diff[".spec"] = diffSpec.String() - } - - // Compare status - if diffStatus, err := util.NewDiffer(util.WithIgnoreConditionsLastTransitionTime()).Diff(capiMachine1.Status, capiMachine2.Status); err != nil { - return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine status: %w", err) - } else if diffStatus.Changed() { - diff[".status"] = diffStatus.String() +func compareCAPIMachines(capiMachine1, capiMachine2 *clusterv1.Machine) (util.DiffResult, error) { + diff, err := util.NewDefaultDiffer().Diff(capiMachine1, capiMachine2) + if err != nil { + return nil, fmt.Errorf("failed to compare Cluster API machines: %w", err) } return diff, nil } // compareMAPIMachines compares MAPI machines a and b, and returns a list of differences, or none if there are none. -func compareMAPIMachines(a, b *mapiv1beta1.Machine) (map[string]any, error) { - diff := make(map[string]any) +func compareMAPIMachines(platform configv1.PlatformType, a, b *mapiv1beta1.Machine) (util.DiffResult, error) { + diff, err := util.NewDefaultDiffer( + util.WithProviderSpec(platform, []string{"spec", "providerSpec", "value"}, mapi2capi.ProviderSpecFromRawExtension), + // Other status fields to ignore + util.WithIgnoreField("status", "providerStatus"), + util.WithIgnoreField("status", "lastOperation"), + util.WithIgnoreField("status", "authoritativeAPI"), + util.WithIgnoreField("status", "synchronizedGeneration"), + ).Diff(a, b) - ps1, err := mapi2capi.AWSProviderSpecFromRawExtension(a.Spec.ProviderSpec.Value) if err != nil { - return nil, fmt.Errorf("unable to parse first Machine API machine set providerSpec: %w", err) - } - - ps2, err := mapi2capi.AWSProviderSpecFromRawExtension(b.Spec.ProviderSpec.Value) - if err != nil { - return nil, fmt.Errorf("unable to parse second Machine API machine set providerSpec: %w", err) - } - - // Sort the tags by name to ensure consistent ordering. - // On the CAPI side these tags are in a map, - // so the order is not guaranteed when converting back from a CAPI map to a MAPI slice. - sort.Slice(ps1.Tags, func(i, j int) bool { - return ps1.Tags[i].Name < ps1.Tags[j].Name - }) - - // Sort the tags by name to ensure consistent ordering. - // On the CAPI side these tags are in a map, - // so the order is not guaranteed when converting back from a CAPI map to a MAPI slice. - sort.Slice(ps2.Tags, func(i, j int) bool { - return ps2.Tags[i].Name < ps2.Tags[j].Name - }) - - // Compare providerSpec - if diffProviderSpec, err := util.NewDiffer().Diff(ps1, ps2); err != nil { - return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine metadata: %w", err) - } else if diffProviderSpec.Changed() { - diff[".providerSpec"] = diffProviderSpec.String() - } - - // Compare metadata - if diffMetadata, err := util.ObjectMetaEqual(a.ObjectMeta, b.ObjectMeta); err != nil { - return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine metadata: %w", err) - } else if diffMetadata.Changed() { - diff[".metadata"] = diffMetadata.String() - } - - // Compare spec - if diffSpec, err := util.NewDiffer( - util.WithIgnoreField("providerSpec", "value"), - ).Diff(a.Spec, b.Spec); err != nil { - return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine spec: %w", err) - } else if diffSpec.Changed() { - diff[".spec"] = diffSpec.String() - } - - // Compare status - if diffStatus, err := util.MAPIMachineStatusEqual(a.Status, b.Status); err != nil { - return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine status: %w", err) - } else if diffStatus.Changed() { - diff[".status"] = diffStatus.String() + return nil, fmt.Errorf("failed to compare Machine API machines: %w", err) } return diff, nil } // ensureCAPIMachineStatusUpdated updates the CAPI machine status if changes are detected and conditions are met. -func (r *MachineSyncReconciler) ensureCAPIMachineStatusUpdated(ctx context.Context, mapiMachine *mapiv1beta1.Machine, existingCAPIMachine, convertedCAPIMachine *clusterv1.Machine, capiMachinesDiff map[string]any, forceUpdate bool) (bool, error) { +func (r *MachineSyncReconciler) ensureCAPIMachineStatusUpdated(ctx context.Context, mapiMachine *mapiv1beta1.Machine, existingCAPIMachine, convertedCAPIMachine *clusterv1.Machine, capiMachinesDiff util.DiffResult, forceUpdate bool) (bool, error) { logger := logf.FromContext(ctx) // If there are spec changes we always want to update the status to sync the spec generation. // If there are status changes we always want to update the status. // If both the above are false, we can skip updating the status and return early. - if !forceUpdate && !hasStatusChanges(capiMachinesDiff) { + if !forceUpdate && !capiMachinesDiff.HasStatusChanges() { return false, nil } @@ -1463,11 +1401,11 @@ func (r *MachineSyncReconciler) ensureMAPIMachine(ctx context.Context, existingM } // ensureMAPIMachineStatusUpdated updates the MAPI machine status if changes are detected. -func (r *MachineSyncReconciler) ensureMAPIMachineStatusUpdated(ctx context.Context, existingMAPIMachine *mapiv1beta1.Machine, convertedMAPIMachine *mapiv1beta1.Machine, mapiMachinesDiff map[string]any, specUpdated bool) (bool, error) { +func (r *MachineSyncReconciler) ensureMAPIMachineStatusUpdated(ctx context.Context, existingMAPIMachine *mapiv1beta1.Machine, convertedMAPIMachine *mapiv1beta1.Machine, mapiMachinesDiff util.DiffResult, specUpdated bool) (bool, error) { logger := logf.FromContext(ctx) // If there are no status changes and the spec has not been updated, return early. - if !hasStatusChanges(mapiMachinesDiff) && !specUpdated { + if !mapiMachinesDiff.HasStatusChanges() && !specUpdated { return false, nil } @@ -1511,11 +1449,11 @@ func (r *MachineSyncReconciler) ensureMAPIMachineStatusUpdated(ctx context.Conte } // ensureMAPIMachineSpecUpdated updates the MAPI machine if changes are detected. -func (r *MachineSyncReconciler) ensureMAPIMachineSpecUpdated(ctx context.Context, existingMAPIMachine *mapiv1beta1.Machine, mapiMachinesDiff map[string]any, updatedMAPIMachine *mapiv1beta1.Machine) (bool, error) { +func (r *MachineSyncReconciler) ensureMAPIMachineSpecUpdated(ctx context.Context, existingMAPIMachine *mapiv1beta1.Machine, mapiMachinesDiff util.DiffResult, updatedMAPIMachine *mapiv1beta1.Machine) (bool, error) { logger := logf.FromContext(ctx) // If there are no spec changes, return early. - if !hasSpecOrMetadataOrProviderSpecChanges(mapiMachinesDiff) { + if !(mapiMachinesDiff.HasMetadataChanges() || mapiMachinesDiff.HasSpecChanges() || mapiMachinesDiff.HasProviderSpecChanges()) { return false, nil } @@ -1569,12 +1507,6 @@ func setChangedMAPIMachineStatusFields(existingMAPIMachine, convertedMAPIMachine existingMAPIMachine.Status = convertedMAPIMachine.Status } -// hasStatusChanges checks if there are status-related changes in the diff. -func hasStatusChanges(diff map[string]any) bool { - _, hasStatus := diff[".status"] - return hasStatus -} - // applySynchronizedConditionWithPatch updates the synchronized condition // using a server side apply patch. We do this to force ownership of the // 'Synchronized' condition and 'SynchronizedGeneration'. @@ -1585,24 +1517,6 @@ func (r *MachineSyncReconciler) applySynchronizedConditionWithPatch(ctx context. status, reason, message, generation) } -// hasSpecOrMetadataOrProviderSpecChanges checks if there are spec, metadata, or provider spec changes in the diff. -func hasSpecOrMetadataOrProviderSpecChanges(diff map[string]any) bool { - _, hasProviderSpec := diff[".providerSpec"] - return hasSpecChanges(diff) || hasMetadataChanges(diff) || hasProviderSpec -} - -// hasSpecChanges checks if there are spec changes in the diff. -func hasSpecChanges(diff map[string]any) bool { - _, hasSpec := diff[".spec"] - return hasSpec -} - -// hasSpecChanges checks if there are spec changes in the diff. -func hasMetadataChanges(diff map[string]any) bool { - _, hasSpec := diff[".metadata"] - return hasSpec -} - // isTerminalConfigurationError returns true if the provided error is // errInvalidClusterInfraReference or errInvalidInfraMachineReference. func isTerminalConfigurationError(err error) bool { diff --git a/pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go b/pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go index 37dd5750c..3f3dba2da 100644 --- a/pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go +++ b/pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go @@ -25,7 +25,6 @@ import ( "github.com/openshift/cluster-capi-operator/pkg/util" corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" utilerrors "k8s.io/apimachinery/pkg/util/errors" awsv1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2" ibmpowervsv1 "sigs.k8s.io/cluster-api-provider-ibmcloud/api/v1beta2" @@ -64,7 +63,7 @@ func (r *MachineSyncReconciler) createOrUpdateCAPIInfraMachine(ctx context.Conte // Infrastructure machines are immutable so we delete it for spec changes. // The next reconciliation will create it with the expected changes. // Note: this could be improved to only trigger deletion on known immutable changes. - if hasSpecChanges(diff) { + if diff.HasSpecChanges() { logger.Info("Deleting the corresponding Cluster API Infrastructure machine as it is out of date, it will be recreated", "diff", fmt.Sprintf("%+v", diff)) syncronizationIsProgressing, err := r.ensureCAPIInfraMachineDeleted(ctx, sourceMAPIMachine, existingCAPIInfraMachine) @@ -122,11 +121,11 @@ func (r *MachineSyncReconciler) ensureCAPIInfraMachine(ctx context.Context, sour } // ensureCAPIInfraMachineMetadataUpdated updates the Cluster API Infrastructure machine if changes are detected to the metadata or spec (if possible). -func (r *MachineSyncReconciler) ensureCAPIInfraMachineMetadataUpdated(ctx context.Context, mapiMachine *mapiv1beta1.Machine, diff map[string]any, updatedOrCreatedCAPIInfraMachine client.Object) (bool, error) { +func (r *MachineSyncReconciler) ensureCAPIInfraMachineMetadataUpdated(ctx context.Context, mapiMachine *mapiv1beta1.Machine, diff util.DiffResult, updatedOrCreatedCAPIInfraMachine client.Object) (bool, error) { logger := logf.FromContext(ctx) // If there are no spec changes, return early. - if !hasMetadataChanges(diff) { + if !diff.HasMetadataChanges() { return false, nil } @@ -147,11 +146,11 @@ func (r *MachineSyncReconciler) ensureCAPIInfraMachineMetadataUpdated(ctx contex return true, nil } -func (r *MachineSyncReconciler) ensureCAPIInfraMachineStatusUpdated(ctx context.Context, mapiMachine *mapiv1beta1.Machine, existingCAPIInfraMachine, convertedCAPIInfraMachine client.Object, diff map[string]any, specUpdated bool) (bool, error) { +func (r *MachineSyncReconciler) ensureCAPIInfraMachineStatusUpdated(ctx context.Context, mapiMachine *mapiv1beta1.Machine, existingCAPIInfraMachine, convertedCAPIInfraMachine client.Object, diff util.DiffResult, specUpdated bool) (bool, error) { logger := logf.FromContext(ctx) // If there are no status changes and the spec has not been updated, return early. - if !hasStatusChanges(diff) && !specUpdated { + if !diff.HasStatusChanges() && !specUpdated { return false, nil } @@ -246,14 +245,8 @@ func (r *MachineSyncReconciler) ensureCAPIInfraMachineDeleted(ctx context.Contex } // compareCAPIInfraMachines compares Cluster API infra machines a and b, and returns a list of differences, or none if there are none. -// -//nolint:funlen -func compareCAPIInfraMachines(platform configv1.PlatformType, infraMachine1, infraMachine2 client.Object) (map[string]any, error) { - diff := make(map[string]any) - - var metadata1, metadata2 metav1.ObjectMeta - - var spec1, spec2, status1, status2 any +func compareCAPIInfraMachines(platform configv1.PlatformType, infraMachine1, infraMachine2 client.Object) (util.DiffResult, error) { + var obj1, obj2 client.Object switch platform { case configv1.AWSPlatformType: @@ -267,12 +260,8 @@ func compareCAPIInfraMachines(platform configv1.PlatformType, infraMachine1, inf return nil, errAssertingCAPIAWSMachine } - spec1 = typedInfraMachine1.Spec - spec2 = typedinfraMachine2.Spec - metadata1 = typedInfraMachine1.ObjectMeta - metadata2 = typedinfraMachine2.ObjectMeta - status1 = typedInfraMachine1.Status - status2 = typedinfraMachine2.Status + obj1 = typedInfraMachine1 + obj2 = typedinfraMachine2 case configv1.OpenStackPlatformType: typedInfraMachine1, ok := infraMachine1.(*openstackv1.OpenStackMachine) if !ok { @@ -284,12 +273,8 @@ func compareCAPIInfraMachines(platform configv1.PlatformType, infraMachine1, inf return nil, errAssertingCAPIOpenStackMachine } - spec1 = typedInfraMachine1.Spec - spec2 = typedinfraMachine2.Spec - metadata1 = typedInfraMachine1.ObjectMeta - metadata2 = typedinfraMachine2.ObjectMeta - status1 = typedInfraMachine1.Status - status2 = typedinfraMachine2.Status + obj1 = typedInfraMachine1 + obj2 = typedinfraMachine2 case configv1.PowerVSPlatformType: typedInfraMachine1, ok := infraMachine1.(*ibmpowervsv1.IBMPowerVSMachine) if !ok { @@ -301,35 +286,15 @@ func compareCAPIInfraMachines(platform configv1.PlatformType, infraMachine1, inf return nil, errAssertingCAPIIBMPowerVSMachine } - spec1 = typedInfraMachine1.Spec - spec2 = typedinfraMachine2.Spec - metadata1 = typedInfraMachine1.ObjectMeta - metadata2 = typedinfraMachine2.ObjectMeta - status1 = typedInfraMachine1.Status - status2 = typedinfraMachine2.Status + obj1 = typedInfraMachine1 + obj2 = typedinfraMachine2 default: return nil, fmt.Errorf("%w: %s", errPlatformNotSupported, platform) } - // Compare metadata - if diffMetadata, err := util.ObjectMetaEqual(metadata1, metadata2); err != nil { - return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine metadata: %w", err) - } else if diffMetadata.Changed() { - diff[".metadata"] = diffMetadata.String() - } - - // Compare spec - if diffSpec, err := util.NewDiffer().Diff(spec1, spec2); err != nil { - return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine spec: %w", err) - } else if diffSpec.Changed() { - diff[".spec"] = diffSpec.String() - } - - // Compare status - if diffStatus, err := util.NewDiffer(util.WithIgnoreConditionsLastTransitionTime()).Diff(status1, status2); err != nil { - return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine status: %w", err) - } else if diffStatus.Changed() { - diff[".status"] = diffStatus.String() + diff, err := util.NewDefaultDiffer().Diff(obj1, obj2) + if err != nil { + return nil, fmt.Errorf("failed to compare Cluster API infrastructure machines: %w", err) } return diff, nil diff --git a/pkg/conversion/mapi2capi/util.go b/pkg/conversion/mapi2capi/util.go index acf61edcd..1b9163e78 100644 --- a/pkg/conversion/mapi2capi/util.go +++ b/pkg/conversion/mapi2capi/util.go @@ -17,17 +17,45 @@ limitations under the License. package mapi2capi import ( + "errors" "fmt" + "sort" + configv1 "github.com/openshift/api/config/v1" mapiv1beta1 "github.com/openshift/api/machine/v1beta1" "github.com/openshift/cluster-capi-operator/pkg/conversion/consts" "github.com/openshift/cluster-capi-operator/pkg/util" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) +var errUnsupportedPlatform = errors.New("unsupported platform") + +// ProviderSpecFromRawExtension converts a MAPI providerSpec to a CAPI providerSpec. +func ProviderSpecFromRawExtension(platform configv1.PlatformType, rawExtension *runtime.RawExtension) (any, error) { + switch platform { + case configv1.AWSPlatformType: + providerConfig, err := AWSProviderSpecFromRawExtension(rawExtension) + if err != nil { + return nil, fmt.Errorf("unable to parse AWS providerSpec: %w", err) + } + + // Sort the tags by name to ensure consistent ordering. + // On the CAPI side these tags are in a map, + // so the order is not guaranteed when converting back from a CAPI map to a MAPI slice. + sort.Slice(providerConfig.Tags, func(i, j int) bool { + return providerConfig.Tags[i].Name < providerConfig.Tags[j].Name + }) + + return providerConfig, nil + default: + return nil, fmt.Errorf("%w: %s", errUnsupportedPlatform, platform) + } +} + func convertMAPIMachineSetSelectorToCAPI(mapiSelector metav1.LabelSelector) metav1.LabelSelector { capiSelector := mapiSelector.DeepCopy() capiSelector.MatchLabels = convertMAPILabelsToCAPI(mapiSelector.MatchLabels) diff --git a/pkg/util/diff.go b/pkg/util/diff.go index b40d1d86c..7896c0a63 100644 --- a/pkg/util/diff.go +++ b/pkg/util/diff.go @@ -16,37 +16,92 @@ limitations under the License. package util import ( + "errors" "fmt" "sort" "strings" + configv1 "github.com/openshift/api/config/v1" + "github.com/go-test/deep" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/sets" + "sigs.k8s.io/controller-runtime/pkg/client" ) +var errTimedOutWaitingForFeatureGates = errors.New("objects to diff cannot be nil") + // DiffResult is the interface that represents the result of a diff operation. type DiffResult interface { - Changed() bool + HasChanges() bool String() string + HasMetadataChanges() bool + HasSpecChanges() bool + HasProviderSpecChanges() bool + HasStatusChanges() bool } type diffResult struct { - diff []string + diff map[string][]string + providerSpecPath string } -// Changed returns true if the diff detected any changes. -func (d *diffResult) Changed() bool { +// HasChanges returns true if the diff detected any changes. +func (d *diffResult) HasChanges() bool { return len(d.diff) > 0 } +// HasMetadataChanges returns true if the diff detected any changes to the metadata. +func (d *diffResult) HasMetadataChanges() bool { + _, ok := d.diff["metadata"] + return ok +} + +// HasSpecChanges returns true if the diff detected any changes to the spec. +func (d *diffResult) HasSpecChanges() bool { + _, ok := d.diff["spec"] + return ok +} + +// HasProviderSpecChanges returns true if the diff detected any changes to the providerSpec. +func (d *diffResult) HasProviderSpecChanges() bool { + if d.providerSpecPath == "" { + return false + } + + _, ok := d.diff[d.providerSpecPath] + + return ok +} + +// HasStatusChanges returns true if the diff detected any changes to the status. +func (d *diffResult) HasStatusChanges() bool { + _, ok := d.diff["status"] + return ok +} + // String returns the diff as a string. func (d *diffResult) String() string { - if !d.Changed() { + if !d.HasChanges() { return "" } - out := "." + strings.Join(d.diff, ", .") + diff := []string{} + + for k, v := range d.diff { + for _, d := range v { + if strings.Contains(d, "]: ") { + diff = append(diff, fmt.Sprintf("[%s].%s", k, d)) + } else { + diff = append(diff, fmt.Sprintf("[%s]: %s", k, d)) + } + } + } + + sort.Strings(diff) + + out := "." + strings.Join(diff, ", .") out = strings.ReplaceAll(out, ".slice[", "[") out = strings.ReplaceAll(out, "map[", "[") @@ -55,11 +110,19 @@ func (d *diffResult) String() string { type differ struct { ignoreConditionsLastTransitionTime bool + modifyFuncs map[string]func(obj map[string]interface{}) error ignoredPath [][]string + providerSpecPath string } // Diff compares the objects a and b, and returns a DiffResult. -func (d *differ) Diff(a, b any) (DiffResult, error) { +// +//nolint:funlen +func (d *differ) Diff(a, b client.Object) (DiffResult, error) { + if a == nil || b == nil { + return nil, errTimedOutWaitingForFeatureGates + } + unstructuredA, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&a) if err != nil { return nil, fmt.Errorf("failed to convert b to unstructured: %w", err) @@ -82,25 +145,52 @@ func (d *differ) Diff(a, b any) (DiffResult, error) { } } + for funcName, modifyFunc := range d.modifyFuncs { + if err := modifyFunc(unstructuredA); err != nil { + return nil, fmt.Errorf("failed to run modify function %s on a: %w", funcName, err) + } + + if err := modifyFunc(unstructuredB); err != nil { + return nil, fmt.Errorf("failed to run modify function %son b: %w", funcName, err) + } + } // Remove fields that we want to ignore. for _, ignorePath := range append(d.ignoredPath, additionalIgnoredPaths...) { unstructured.RemoveNestedField(unstructuredA, ignorePath...) unstructured.RemoveNestedField(unstructuredB, ignorePath...) } - diff := deep.Equal(unstructuredA, unstructuredB) + allKeys := sets.Set[string]{} - // Make the result deterministic. - sort.Strings(diff) + for k := range unstructuredA { + allKeys.Insert(k) + } + + for k := range unstructuredB { + allKeys.Insert(k) + } + + diff := map[string][]string{} - return &diffResult{diff: diff}, nil + for k := range allKeys { + d := deep.Equal(unstructuredA[k], unstructuredB[k]) + + // Make the result deterministic. + sort.Strings(d) + + if len(d) > 0 { + diff[k] = d + } + } + + return &diffResult{diff: diff, providerSpecPath: d.providerSpecPath}, nil } func removeConditionsLastTransitionTime(a map[string]interface{}) error { conditionPaths := [][]string{ - {"conditions"}, - {"v1beta2", "conditions"}, - {"deprecated", "v1beta1", "conditions"}, + {"status", "conditions"}, + {"status", "v1beta2", "conditions"}, + {"status", "deprecated", "v1beta1", "conditions"}, } for _, conditionPath := range conditionPaths { @@ -129,9 +219,10 @@ func removeConditionsLastTransitionTime(a map[string]interface{}) error { type diffopts func(*differ) -// NewDiffer creates a new differ with the given options. -func NewDiffer(opts ...diffopts) *differ { - d := &differ{} +func newDiffer(opts ...diffopts) *differ { + d := &differ{ + modifyFuncs: map[string]func(obj map[string]interface{}) error{}, + } for _, opt := range opts { opt(d) } @@ -139,6 +230,32 @@ func NewDiffer(opts ...diffopts) *differ { return d } +// NewDefaultDiffer creates a new default differ with the default options. +func NewDefaultDiffer(opts ...diffopts) *differ { + return newDiffer(append(opts, + // Options for handling of metadata fields. + + // Special handling for CAPI's conversion-data label. + WithIgnoreField("metadata", "annotations", "cluster.x-k8s.io/conversion-data"), + + WithIgnoreField("metadata", "name"), + WithIgnoreField("metadata", "generateName"), + WithIgnoreField("metadata", "namespace"), + WithIgnoreField("metadata", "selfLink"), + WithIgnoreField("metadata", "uid"), + WithIgnoreField("metadata", "resourceVersion"), + WithIgnoreField("metadata", "generation"), + WithIgnoreField("metadata", "creationTimestamp"), + WithIgnoreField("metadata", "deletionTimestamp"), + WithIgnoreField("metadata", "deletionGracePeriodSeconds"), + WithIgnoreField("metadata", "finalizers"), + WithIgnoreField("metadata", "managedFields"), + + // Options for handling of status fields. + WithIgnoreConditionsLastTransitionTime(), + )...) +} + // WithIgnoreField adds a path to the list of paths to ignore when executing Diff. func WithIgnoreField(path ...string) diffopts { return func(d *differ) { @@ -149,6 +266,39 @@ func WithIgnoreField(path ...string) diffopts { // WithIgnoreConditionsLastTransitionTime configures the differ to ignore LastTransitionTime for conditions when executing Diff. func WithIgnoreConditionsLastTransitionTime() diffopts { return func(d *differ) { - d.ignoreConditionsLastTransitionTime = true + d.modifyFuncs["RemoveConditionsLastTransitionTime"] = removeConditionsLastTransitionTime + } +} + +// WithProviderSpec configures the differ to separately diff .spec.providerSpec. +func WithProviderSpec(platform configv1.PlatformType, path []string, marshalProviderSpec func(platform configv1.PlatformType, rawExtension *runtime.RawExtension) (any, error)) diffopts { + return func(d *differ) { + d.providerSpecPath = strings.Join(path, ".") + + d.modifyFuncs["ProviderSpec"] = func(obj map[string]interface{}) error { + rawExtensionMap, found, err := unstructured.NestedMap(obj, path...) + if !found || err != nil { + return fmt.Errorf("failed to get providerSpec value: %w", err) + } + + rawExtension := &runtime.RawExtension{} + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(rawExtensionMap, rawExtension); err != nil { + return fmt.Errorf("failed to convert providerSpec value map to raw extension: %w", err) + } + + providerSpec, err := marshalProviderSpec(platform, rawExtension) + if err != nil { + return fmt.Errorf("failed to marshal providerSpec: %w", err) + } + + // unset the nested field + unstructured.RemoveNestedField(obj, path...) + // add it as top-level field + if err := unstructured.SetNestedField(obj, providerSpec, d.providerSpecPath); err != nil { + return fmt.Errorf("failed to set nested field %s: %w", d.providerSpecPath, err) + } + + return nil + } } } diff --git a/pkg/util/diff_test.go b/pkg/util/diff_test.go index 9a4971645..95eb123c8 100644 --- a/pkg/util/diff_test.go +++ b/pkg/util/diff_test.go @@ -19,111 +19,138 @@ import ( "testing" . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) //nolint:funlen func TestDiff_basic_operations(t *testing.T) { tests := []struct { name string - a map[string]any - b map[string]any + a unstructured.Unstructured + b unstructured.Unstructured wantChanged bool want string }{ { - name: "no diff on empty objects", - a: map[string]any{}, - b: map[string]any{}, + name: "no diff on empty objects", + a: unstructured.Unstructured{ + Object: map[string]any{}, + }, + b: unstructured.Unstructured{ + Object: map[string]any{}, + }, wantChanged: false, want: "", }, { name: "diff when adding a field", - a: map[string]any{ + a: unstructured.Unstructured{Object: map[string]any{ "a": 1, - }, - b: map[string]any{ + }}, + b: unstructured.Unstructured{Object: map[string]any{ "a": 1, "b": 2, - }, + }}, + wantChanged: true, + want: ".[b]: != 2", + }, + { + name: "diff when adding a field nested", + a: unstructured.Unstructured{Object: map[string]any{ + "foo": map[string]any{ + "a": 1, + "c": map[string]any{ + "d": 3, + }, + }, + }}, + b: unstructured.Unstructured{Object: map[string]any{ + "foo": map[string]any{ + "a": 1, + "b": 2, + "c": map[string]any{ + "d": 4, + }, + }, + }}, wantChanged: true, - want: ".[b]: != 2", + want: ".[foo].[b]: != 2, .[foo].[c].[d]: 3 != 4", }, { name: "diff when removing a field", - a: map[string]any{ + a: unstructured.Unstructured{Object: map[string]any{ "a": 1, "b": 2, - }, - b: map[string]any{ + }}, + b: unstructured.Unstructured{Object: map[string]any{ "a": 1, - }, + }}, wantChanged: true, - want: ".[b]: 2 != ", + want: ".[b]: 2 != ", }, { name: "diff when changing a field", - a: map[string]any{ + a: unstructured.Unstructured{Object: map[string]any{ "a": 1, "b": 2, - }, - b: map[string]any{ + }}, + b: unstructured.Unstructured{Object: map[string]any{ "a": 1, "b": 3, - }, + }}, wantChanged: true, want: ".[b]: 2 != 3", }, { name: "diff when adding a entry to a list", - a: map[string]any{ + a: unstructured.Unstructured{Object: map[string]any{ "a": 1, "b": []int{1, 2}, - }, - b: map[string]any{ + }}, + b: unstructured.Unstructured{Object: map[string]any{ "a": 1, "b": []int{1, 2, 3}, - }, + }}, wantChanged: true, want: ".[b][2]: != 3", }, { name: "diff when removing a entry from a list", - a: map[string]any{ + a: unstructured.Unstructured{Object: map[string]any{ "a": 1, "b": []int{1, 2, 3}, - }, - b: map[string]any{ + }}, + b: unstructured.Unstructured{Object: map[string]any{ "a": 1, "b": []int{1, 2}, - }, + }}, wantChanged: true, want: ".[b][2]: 3 != ", }, { name: "diff when changing a entry in a list", - a: map[string]any{ + a: unstructured.Unstructured{Object: map[string]any{ "a": 1, "b": []int{1, 2, 3}, - }, - b: map[string]any{ + }}, + b: unstructured.Unstructured{Object: map[string]any{ "a": 1, "b": []int{1, 2, 4}, - }, + }}, wantChanged: true, want: ".[b][2]: 3 != 4", }, { name: "diff when deleting a list", - a: map[string]any{ + a: unstructured.Unstructured{Object: map[string]any{ "a": 1, "b": []int{1, 2, 3}, - }, - b: map[string]any{ + }}, + b: unstructured.Unstructured{Object: map[string]any{ "a": 1, - }, + }}, wantChanged: true, - want: ".[b]: [1 2 3] != ", + want: ".[b]: [1 2 3] != ", }, } @@ -131,11 +158,11 @@ func TestDiff_basic_operations(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - differ := NewDiffer() + differ := newDiffer() - diff, err := differ.Diff(tt.a, tt.b) + diff, err := differ.Diff(&tt.a, &tt.b) g.Expect(err).ToNot(HaveOccurred()) - g.Expect(diff.Changed()).To(Equal(tt.wantChanged)) + g.Expect(diff.HasChanges()).To(Equal(tt.wantChanged)) g.Expect(diff.String()).To(Equal(tt.want)) }) } diff --git a/pkg/util/sync.go b/pkg/util/sync.go index c5d8b17e1..3804bf7c4 100644 --- a/pkg/util/sync.go +++ b/pkg/util/sync.go @@ -21,7 +21,6 @@ import ( mapiv1beta1 "github.com/openshift/api/machine/v1beta1" machinev1applyconfigs "github.com/openshift/client-go/machine/applyconfigurations/machine/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -52,93 +51,6 @@ func hasSameState(i *mapiv1beta1.Condition, j *machinev1applyconfigs.ConditionAp i.Status == *j.Status } -// ObjectMetaEqual compares variables a and b, -// and returns a list of differences, or nil if there are none, -// for the fields we care about when synchronising MAPI and CAPI Machines. -func ObjectMetaEqual(a, b metav1.ObjectMeta) (DiffResult, error) { - differ := NewDiffer( - // Special handling for CAPI's conversion-data label. - WithIgnoreField("annotations", "cluster.x-k8s.io/conversion-data"), - - WithIgnoreField("name"), - WithIgnoreField("generateName"), - WithIgnoreField("namespace"), - WithIgnoreField("selfLink"), - WithIgnoreField("uid"), - WithIgnoreField("resourceVersion"), - WithIgnoreField("generation"), - WithIgnoreField("creationTimestamp"), - WithIgnoreField("deletionTimestamp"), - WithIgnoreField("deletionGracePeriodSeconds"), - WithIgnoreField("finalizers"), - WithIgnoreField("managedFields"), - ) - - return differ.Diff(a, b) -} - -// CAPIMachineSetStatusEqual compares variables a and b, -// and returns a list of differences, or nil if there are none, -// for the fields we care about when synchronising MAPI and CAPI Machines. -func CAPIMachineSetStatusEqual(a, b clusterv1.MachineSetStatus) (DiffResult, error) { - differ := NewDiffer( - // Changes to a condition's LastTransitionTime should not cause updates because it should only change - // if the status itself changes. - WithIgnoreConditionsLastTransitionTime(), - ) - - return differ.Diff(a, b) -} - -// CAPIMachineStatusEqual compares variables a and b, -// and returns a list of differences, or nil if there are none, -// for the fields we care about when synchronising CAPI and MAPI Machines. -func CAPIMachineStatusEqual(a, b clusterv1.MachineStatus) (DiffResult, error) { - differ := NewDiffer( - // Changes to a condition's LastTransitionTime should not cause updates because it should only change - // if the status itself changes. - WithIgnoreConditionsLastTransitionTime(), - ) - - return differ.Diff(a, b) -} - -// MAPIMachineStatusEqual compares variables a and b, -// and returns a list of differences, or nil if there are none, -// for the fields we care about when synchronising CAPI and MAPI Machines. -func MAPIMachineStatusEqual(a, b mapiv1beta1.MachineStatus) (DiffResult, error) { - differ := NewDiffer( - // Changes to a condition's LastTransitionTime should not cause updates because it should only change - // if the status itself changes. - WithIgnoreConditionsLastTransitionTime(), - - WithIgnoreField("providerStatus"), - WithIgnoreField("lastOperation"), - WithIgnoreField("authoritativeAPI"), - WithIgnoreField("synchronizedGeneration"), - ) - - return differ.Diff(a, b) -} - -// MAPIMachineSetStatusEqual compares variables a and b, -// and returns a list of differences, or nil if there are none, -// for the fields we care about when synchronising MAPI and CAPI Machines. -func MAPIMachineSetStatusEqual(a, b mapiv1beta1.MachineSetStatus) (DiffResult, error) { - differ := NewDiffer( - // Changes to a condition's LastTransitionTime should not cause updates because it should only change - // if the status itself changes. - WithIgnoreConditionsLastTransitionTime(), - - WithIgnoreField("replicas"), - WithIgnoreField("observedGeneration"), - WithIgnoreField("authoritativeAPI"), - WithIgnoreField("synchronizedGeneration"), - ) - - return differ.Diff(a, b) -} - // GetResourceVersion returns the object ResourceVersion or the zero value for it. func GetResourceVersion(obj client.Object) string { if obj == nil || reflect.ValueOf(obj).IsNil() { From 1fb263d40f028fa55262c16ff84a8e82bcd03142 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Fri, 31 Oct 2025 11:39:29 +0100 Subject: [PATCH 06/19] fix MAPI MachineSet providerSpec diff --- pkg/controllers/machinesetsync/machineset_sync_controller.go | 2 +- pkg/util/diff.go | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/controllers/machinesetsync/machineset_sync_controller.go b/pkg/controllers/machinesetsync/machineset_sync_controller.go index 8bf1ae667..b9f6a6f7e 100644 --- a/pkg/controllers/machinesetsync/machineset_sync_controller.go +++ b/pkg/controllers/machinesetsync/machineset_sync_controller.go @@ -1416,7 +1416,7 @@ func compareCAPIMachineSets(capiMachineSet1, capiMachineSet2 *clusterv1.MachineS // compareMAPIMachineSets compares MAPI machineSets a and b, and returns a list of differences, or none if there are none. func compareMAPIMachineSets(platform configv1.PlatformType, a, b *mapiv1beta1.MachineSet) (util.DiffResult, error) { diff, err := util.NewDefaultDiffer( - util.WithProviderSpec(platform, []string{"spec", "template", "providerSpec", "value"}, mapi2capi.ProviderSpecFromRawExtension), + util.WithProviderSpec(platform, []string{"spec", "template", "spec", "providerSpec", "value"}, mapi2capi.ProviderSpecFromRawExtension), // Other status fields to ignore util.WithIgnoreField("status", "replicas"), diff --git a/pkg/util/diff.go b/pkg/util/diff.go index 7896c0a63..2515c119f 100644 --- a/pkg/util/diff.go +++ b/pkg/util/diff.go @@ -294,9 +294,7 @@ func WithProviderSpec(platform configv1.PlatformType, path []string, marshalProv // unset the nested field unstructured.RemoveNestedField(obj, path...) // add it as top-level field - if err := unstructured.SetNestedField(obj, providerSpec, d.providerSpecPath); err != nil { - return fmt.Errorf("failed to set nested field %s: %w", d.providerSpecPath, err) - } + obj[d.providerSpecPath] = providerSpec return nil } From d6d4ab7c44c6f875c797695d260c3d3f5907739d Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Fri, 31 Oct 2025 15:01:35 +0100 Subject: [PATCH 07/19] Use proper naming for Cluster API and Machine API --- .../machinesetsync/machineset_sync_controller.go | 2 +- pkg/conversion/mapi2capi/util.go | 6 +++--- pkg/util/diff.go | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/controllers/machinesetsync/machineset_sync_controller.go b/pkg/controllers/machinesetsync/machineset_sync_controller.go index b9f6a6f7e..924c19c9a 100644 --- a/pkg/controllers/machinesetsync/machineset_sync_controller.go +++ b/pkg/controllers/machinesetsync/machineset_sync_controller.go @@ -1403,7 +1403,7 @@ func compareCAPIInfraMachineTemplates(platform configv1.PlatformType, infraMachi return diff, nil } -// compareCAPIMachineSets compares CAPI machineSets a and b, and returns a list of differences, or none if there are none. +// compareCAPIMachineSets compares Cluster API machineSets a and b, and returns a list of differences, or none if there are none. func compareCAPIMachineSets(capiMachineSet1, capiMachineSet2 *clusterv1.MachineSet) (util.DiffResult, error) { diff, err := util.NewDefaultDiffer().Diff(capiMachineSet1, capiMachineSet2) if err != nil { diff --git a/pkg/conversion/mapi2capi/util.go b/pkg/conversion/mapi2capi/util.go index 1b9163e78..930ad39b0 100644 --- a/pkg/conversion/mapi2capi/util.go +++ b/pkg/conversion/mapi2capi/util.go @@ -34,7 +34,7 @@ import ( var errUnsupportedPlatform = errors.New("unsupported platform") -// ProviderSpecFromRawExtension converts a MAPI providerSpec to a CAPI providerSpec. +// ProviderSpecFromRawExtension converts a Machine API providerSpec to a Cluster API providerSpec. func ProviderSpecFromRawExtension(platform configv1.PlatformType, rawExtension *runtime.RawExtension) (any, error) { switch platform { case configv1.AWSPlatformType: @@ -44,8 +44,8 @@ func ProviderSpecFromRawExtension(platform configv1.PlatformType, rawExtension * } // Sort the tags by name to ensure consistent ordering. - // On the CAPI side these tags are in a map, - // so the order is not guaranteed when converting back from a CAPI map to a MAPI slice. + // On the Cluster API side these tags are in a map, + // so the order is not guaranteed when converting back from a Cluster API map to a Machine API slice. sort.Slice(providerConfig.Tags, func(i, j int) bool { return providerConfig.Tags[i].Name < providerConfig.Tags[j].Name }) diff --git a/pkg/util/diff.go b/pkg/util/diff.go index 2515c119f..139d6b139 100644 --- a/pkg/util/diff.go +++ b/pkg/util/diff.go @@ -235,7 +235,7 @@ func NewDefaultDiffer(opts ...diffopts) *differ { return newDiffer(append(opts, // Options for handling of metadata fields. - // Special handling for CAPI's conversion-data label. + // Special handling for Cluster API's conversion-data label. WithIgnoreField("metadata", "annotations", "cluster.x-k8s.io/conversion-data"), WithIgnoreField("metadata", "name"), From 3cbe8cdbe22a18dbb1ee0dd82944a5d31a2539c6 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Fri, 31 Oct 2025 15:27:19 +0100 Subject: [PATCH 08/19] self-review fixes --- pkg/conversion/mapi2capi/aws.go | 8 ++ pkg/conversion/mapi2capi/util.go | 8 -- pkg/util/diff.go | 130 ++++++++++++++++--------------- 3 files changed, 75 insertions(+), 71 deletions(-) diff --git a/pkg/conversion/mapi2capi/aws.go b/pkg/conversion/mapi2capi/aws.go index 159fcb992..fc0c30938 100644 --- a/pkg/conversion/mapi2capi/aws.go +++ b/pkg/conversion/mapi2capi/aws.go @@ -19,6 +19,7 @@ import ( "fmt" "reflect" "regexp" + "sort" "strings" configv1 "github.com/openshift/api/config/v1" @@ -436,6 +437,13 @@ func AWSProviderSpecFromRawExtension(rawExtension *runtime.RawExtension) (mapiv1 return mapiv1beta1.AWSMachineProviderConfig{}, fmt.Errorf("error unmarshalling providerSpec: %w", err) } + // Sort the tags by name to ensure consistent ordering. + // On the Cluster API side these tags are in a map, + // so the order is not guaranteed when converting back from a Cluster API map to a Machine API slice. + sort.Slice(spec.Tags, func(i, j int) bool { + return spec.Tags[i].Name < spec.Tags[j].Name + }) + return spec, nil } diff --git a/pkg/conversion/mapi2capi/util.go b/pkg/conversion/mapi2capi/util.go index 930ad39b0..30ceafdd9 100644 --- a/pkg/conversion/mapi2capi/util.go +++ b/pkg/conversion/mapi2capi/util.go @@ -19,7 +19,6 @@ package mapi2capi import ( "errors" "fmt" - "sort" configv1 "github.com/openshift/api/config/v1" mapiv1beta1 "github.com/openshift/api/machine/v1beta1" @@ -43,13 +42,6 @@ func ProviderSpecFromRawExtension(platform configv1.PlatformType, rawExtension * return nil, fmt.Errorf("unable to parse AWS providerSpec: %w", err) } - // Sort the tags by name to ensure consistent ordering. - // On the Cluster API side these tags are in a map, - // so the order is not guaranteed when converting back from a Cluster API map to a Machine API slice. - sort.Slice(providerConfig.Tags, func(i, j int) bool { - return providerConfig.Tags[i].Name < providerConfig.Tags[j].Name - }) - return providerConfig, nil default: return nil, fmt.Errorf("%w: %s", errUnsupportedPlatform, platform) diff --git a/pkg/util/diff.go b/pkg/util/diff.go index 139d6b139..d45770ef9 100644 --- a/pkg/util/diff.go +++ b/pkg/util/diff.go @@ -42,6 +42,7 @@ type DiffResult interface { HasStatusChanges() bool } +// diffResult is the implementation of the DiffResult interface. type diffResult struct { diff map[string][]string providerSpecPath string @@ -65,6 +66,7 @@ func (d *diffResult) HasSpecChanges() bool { } // HasProviderSpecChanges returns true if the diff detected any changes to the providerSpec. +// Only ever returns true if d.providerSpecPath was set. func (d *diffResult) HasProviderSpecChanges() bool { if d.providerSpecPath == "" { return false @@ -109,20 +111,22 @@ func (d *diffResult) String() string { } type differ struct { - ignoreConditionsLastTransitionTime bool - modifyFuncs map[string]func(obj map[string]interface{}) error - ignoredPath [][]string - providerSpecPath string + modifyFuncs map[string]func(obj map[string]interface{}) error + ignoredPath [][]string + + // providerSpecPath is a custom path to the providerSpec field which differs between + // Machine API Machines and MachineSets and is passed down to the result to enable HasProviderSpecChanges(). + // It is only used when set, so it does not make a difference for non Machine API objects. + providerSpecPath string } // Diff compares the objects a and b, and returns a DiffResult. -// -//nolint:funlen func (d *differ) Diff(a, b client.Object) (DiffResult, error) { if a == nil || b == nil { return nil, errTimedOutWaitingForFeatureGates } + // 1. Convert the objects to unstructured. unstructuredA, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&a) if err != nil { return nil, fmt.Errorf("failed to convert b to unstructured: %w", err) @@ -135,16 +139,9 @@ func (d *differ) Diff(a, b client.Object) (DiffResult, error) { var additionalIgnoredPaths [][]string - if d.ignoreConditionsLastTransitionTime { - if err := removeConditionsLastTransitionTime(unstructuredA); err != nil { - return nil, fmt.Errorf("failed to remove conditions last transition time from a: %w", err) - } - - if err := removeConditionsLastTransitionTime(unstructuredB); err != nil { - return nil, fmt.Errorf("failed to remove conditions last transition time from b: %w", err) - } - } - + // 2. Run the configured modify functions + // This allows customizing the diffing process, e.g. remove conditions last transition time to ignore them during diffing + // or separate handling for providerSpec. for funcName, modifyFunc := range d.modifyFuncs { if err := modifyFunc(unstructuredA); err != nil { return nil, fmt.Errorf("failed to run modify function %s on a: %w", funcName, err) @@ -154,12 +151,17 @@ func (d *differ) Diff(a, b client.Object) (DiffResult, error) { return nil, fmt.Errorf("failed to run modify function %son b: %w", funcName, err) } } - // Remove fields that we want to ignore. + + // 3. Remove fields configured to be ignored. for _, ignorePath := range append(d.ignoredPath, additionalIgnoredPaths...) { unstructured.RemoveNestedField(unstructuredA, ignorePath...) unstructured.RemoveNestedField(unstructuredB, ignorePath...) } + // 4. Diff both resulted unstructured objects. + // Record the result for each top-level key in the maps, so it can be used later on for the `Has*Changes` functions. + + // Collect all top-level keys. allKeys := sets.Set[string]{} for k := range unstructuredA { @@ -172,6 +174,7 @@ func (d *differ) Diff(a, b client.Object) (DiffResult, error) { diff := map[string][]string{} + // Diff each top-level key separately and record the output to the diff map. for k := range allKeys { d := deep.Equal(unstructuredA[k], unstructuredB[k]) @@ -183,51 +186,10 @@ func (d *differ) Diff(a, b client.Object) (DiffResult, error) { } } - return &diffResult{diff: diff, providerSpecPath: d.providerSpecPath}, nil -} - -func removeConditionsLastTransitionTime(a map[string]interface{}) error { - conditionPaths := [][]string{ - {"status", "conditions"}, - {"status", "v1beta2", "conditions"}, - {"status", "deprecated", "v1beta1", "conditions"}, - } - - for _, conditionPath := range conditionPaths { - conditions, found, err := unstructured.NestedSlice(a, conditionPath...) - if !found || err != nil { - continue - } - - for i, condition := range conditions { - conditionMap, ok := condition.(map[string]interface{}) - if !ok { - continue - } - - conditionMap["lastTransitionTime"] = "ignored" - conditions[i] = conditionMap - } - - if err := unstructured.SetNestedField(a, conditions, conditionPath...); err != nil { - return fmt.Errorf("failed to set nested field %s: %w", strings.Join(conditionPath, "."), err) - } - } - - return nil -} - -type diffopts func(*differ) - -func newDiffer(opts ...diffopts) *differ { - d := &differ{ - modifyFuncs: map[string]func(obj map[string]interface{}) error{}, - } - for _, opt := range opts { - opt(d) - } - - return d + return &diffResult{ + diff: diff, + providerSpecPath: d.providerSpecPath, + }, nil } // NewDefaultDiffer creates a new default differ with the default options. @@ -266,7 +228,36 @@ func WithIgnoreField(path ...string) diffopts { // WithIgnoreConditionsLastTransitionTime configures the differ to ignore LastTransitionTime for conditions when executing Diff. func WithIgnoreConditionsLastTransitionTime() diffopts { return func(d *differ) { - d.modifyFuncs["RemoveConditionsLastTransitionTime"] = removeConditionsLastTransitionTime + d.modifyFuncs["RemoveConditionsLastTransitionTime"] = func(a map[string]interface{}) error { + conditionPaths := [][]string{ + {"status", "conditions"}, + {"status", "v1beta2", "conditions"}, + {"status", "deprecated", "v1beta1", "conditions"}, + } + + for _, conditionPath := range conditionPaths { + conditions, found, err := unstructured.NestedSlice(a, conditionPath...) + if !found || err != nil { + continue + } + + for i, condition := range conditions { + conditionMap, ok := condition.(map[string]interface{}) + if !ok { + continue + } + + conditionMap["lastTransitionTime"] = "ignored" + conditions[i] = conditionMap + } + + if err := unstructured.SetNestedField(a, conditions, conditionPath...); err != nil { + return fmt.Errorf("failed to set nested field %s: %w", strings.Join(conditionPath, "."), err) + } + } + + return nil + } } } @@ -300,3 +291,16 @@ func WithProviderSpec(platform configv1.PlatformType, path []string, marshalProv } } } + +type diffopts func(*differ) + +func newDiffer(opts ...diffopts) *differ { + d := &differ{ + modifyFuncs: map[string]func(obj map[string]interface{}) error{}, + } + for _, opt := range opts { + opt(d) + } + + return d +} From 91414c4bc22e49e2eec4b8e3cec5c231b3ba1b0e Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Fri, 31 Oct 2025 16:00:16 +0100 Subject: [PATCH 09/19] rewrite tests in ginkgo style --- .../machineset_sync_controller_unit_test.go | 123 ++++++++---------- pkg/util/diff_test.go | 85 +++++------- pkg/util/suite_test.go | 28 ++++ 3 files changed, 115 insertions(+), 121 deletions(-) create mode 100644 pkg/util/suite_test.go diff --git a/pkg/controllers/machinesetsync/machineset_sync_controller_unit_test.go b/pkg/controllers/machinesetsync/machineset_sync_controller_unit_test.go index 72d6ae28e..7818b13b2 100644 --- a/pkg/controllers/machinesetsync/machineset_sync_controller_unit_test.go +++ b/pkg/controllers/machinesetsync/machineset_sync_controller_unit_test.go @@ -16,9 +16,9 @@ limitations under the License. package machinesetsync import ( - "testing" "time" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -27,27 +27,42 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) -//nolint:funlen -func TestCAPIMachineSetStatusEqual(t *testing.T) { +var _ = Describe("Unit tests for CAPIMachineSetStatusEqual", func() { now := metav1.Now() later := metav1.NewTime(now.Add(1 * time.Hour)) - tests := []struct { - name string + type testInput struct { a clusterv1.MachineSetStatus b clusterv1.MachineSetStatus want string wantChanges bool - }{ - { - name: "no diff", + } + + DescribeTable("when comparing Cluster API MachineSets", + func(tt testInput) { + a := &clusterv1.MachineSet{ + Status: tt.a, + } + b := &clusterv1.MachineSet{ + Status: tt.b, + } + + got, err := compareCAPIMachineSets(a, b) + Expect(err).ToNot(HaveOccurred()) + + Expect(got.HasChanges()).To(Equal(tt.wantChanges)) + + if tt.wantChanges { + Expect(got.String()).To(Equal(tt.want)) + } + }, + Entry("no diff", testInput{ a: clusterv1.MachineSetStatus{}, b: clusterv1.MachineSetStatus{}, want: "", wantChanges: false, - }, - { - name: "diff in ReadyReplicas", + }), + Entry("diff in ReadyReplicas", testInput{ a: clusterv1.MachineSetStatus{ ReadyReplicas: 3, }, @@ -56,9 +71,8 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, want: ".[status].[readyReplicas]: 3 != 5", wantChanges: true, - }, - { - name: "diff in AvailableReplicas", + }), + Entry("diff in AvailableReplicas", testInput{ a: clusterv1.MachineSetStatus{ AvailableReplicas: 2, }, @@ -67,9 +81,8 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, want: ".[status].[availableReplicas]: 2 != 4", wantChanges: true, - }, - { - name: "diff in ReadyReplicas and AvailableReplicas", + }), + Entry("diff in ReadyReplicas and AvailableReplicas", testInput{ a: clusterv1.MachineSetStatus{ ReadyReplicas: 3, AvailableReplicas: 2, @@ -80,9 +93,8 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, want: ".[status].[availableReplicas]: 2 != 4, .[status].[readyReplicas]: 3 != 5", wantChanges: true, - }, - { - name: "same v1beta1 conditions", + }), + Entry("same v1beta1 conditions", testInput{ a: clusterv1.MachineSetStatus{ Conditions: []clusterv1.Condition{ { @@ -103,9 +115,8 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, want: "", wantChanges: false, - }, - { - name: "changed v1beta1 condition Status", + }), + Entry("changed v1beta1 condition Status", testInput{ a: clusterv1.MachineSetStatus{ Conditions: []clusterv1.Condition{ { @@ -126,9 +137,8 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, want: ".[status].[conditions][0].[status]: True != False", wantChanges: true, - }, - { - name: "v1beta1 condition LastTransitionTime ignored", + }), + Entry("v1beta1 condition LastTransitionTime ignored", testInput{ a: clusterv1.MachineSetStatus{ Conditions: []clusterv1.Condition{ { @@ -149,9 +159,8 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, want: "", wantChanges: false, - }, - { - name: "multiple v1beta1 conditions with one changed", + }), + Entry("multiple v1beta1 conditions with one changed", testInput{ a: clusterv1.MachineSetStatus{ Conditions: []clusterv1.Condition{ { @@ -182,9 +191,8 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, want: ".[status].[conditions][1].[status]: True != False", wantChanges: true, - }, - { - name: "same v1beta2 conditions", + }), + Entry("same v1beta2 conditions", testInput{ a: clusterv1.MachineSetStatus{ V1Beta2: &clusterv1.MachineSetV1Beta2Status{ Conditions: []metav1.Condition{ @@ -213,9 +221,8 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, want: "", wantChanges: false, - }, - { - name: "changed v1beta2 condition Status", + }), + Entry("changed v1beta2 condition Status", testInput{ a: clusterv1.MachineSetStatus{ V1Beta2: &clusterv1.MachineSetV1Beta2Status{ Conditions: []metav1.Condition{ @@ -240,9 +247,8 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, want: ".[status].[v1beta2].[conditions][0].[status]: True != False", wantChanges: true, - }, - { - name: "v1beta2 condition LastTransitionTime ignored", + }), + Entry("v1beta2 condition LastTransitionTime ignored", testInput{ a: clusterv1.MachineSetStatus{ V1Beta2: &clusterv1.MachineSetV1Beta2Status{ Conditions: []metav1.Condition{ @@ -271,9 +277,8 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, want: "", wantChanges: false, - }, - { - name: "multiple v1beta2 conditions with one changed", + }), + Entry("multiple v1beta2 conditions with one changed", testInput{ a: clusterv1.MachineSetStatus{ V1Beta2: &clusterv1.MachineSetV1Beta2Status{ Conditions: []metav1.Condition{ @@ -308,9 +313,8 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, want: ".[status].[v1beta2].[conditions][1].[status]: True != False", wantChanges: true, - }, - { - name: "v1beta2 nil vs non-nil", + }), + Entry("v1beta2 nil vs non-nil", testInput{ a: clusterv1.MachineSetStatus{ V1Beta2: nil, }, @@ -321,31 +325,6 @@ func TestCAPIMachineSetStatusEqual(t *testing.T) { }, want: ".[status].[v1beta2]: != [readyReplicas:3]", wantChanges: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - a := &clusterv1.MachineSet{ - Status: tt.a, - } - b := &clusterv1.MachineSet{ - Status: tt.b, - } - - got, err := compareCAPIMachineSets(a, b) - g.Expect(err).ToNot(HaveOccurred()) - - gotChanges := got.HasChanges() - - g.Expect(gotChanges).To(Equal(tt.wantChanges)) - - if tt.wantChanges { - gotString := got.String() - g.Expect(gotString).To(Equal(tt.want)) - } - }) - } -} + }), + ) +}) diff --git a/pkg/util/diff_test.go b/pkg/util/diff_test.go index 95eb123c8..b2cf40b13 100644 --- a/pkg/util/diff_test.go +++ b/pkg/util/diff_test.go @@ -16,23 +16,31 @@ limitations under the License. package util import ( - "testing" - + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) -//nolint:funlen -func TestDiff_basic_operations(t *testing.T) { - tests := []struct { - name string +var _ = Describe("Unit test Diff", func() { + var differ *differ + + BeforeEach(func() { + differ = newDiffer() + }) + + type testInput struct { a unstructured.Unstructured b unstructured.Unstructured wantChanged bool want string - }{ - { - name: "no diff on empty objects", + } + DescribeTable("basic operations", func(tt testInput) { + diff, err := differ.Diff(&tt.a, &tt.b) + Expect(err).ToNot(HaveOccurred()) + Expect(diff.HasChanges()).To(Equal(tt.wantChanged)) + Expect(diff.String()).To(Equal(tt.want)) + }, + Entry("no diff on empty objects", testInput{ a: unstructured.Unstructured{ Object: map[string]any{}, }, @@ -41,9 +49,8 @@ func TestDiff_basic_operations(t *testing.T) { }, wantChanged: false, want: "", - }, - { - name: "diff when adding a field", + }), + Entry("diff when adding a field", testInput{ a: unstructured.Unstructured{Object: map[string]any{ "a": 1, }}, @@ -53,9 +60,8 @@ func TestDiff_basic_operations(t *testing.T) { }}, wantChanged: true, want: ".[b]: != 2", - }, - { - name: "diff when adding a field nested", + }), + Entry("diff when adding a field nested", testInput{ a: unstructured.Unstructured{Object: map[string]any{ "foo": map[string]any{ "a": 1, @@ -75,9 +81,8 @@ func TestDiff_basic_operations(t *testing.T) { }}, wantChanged: true, want: ".[foo].[b]: != 2, .[foo].[c].[d]: 3 != 4", - }, - { - name: "diff when removing a field", + }), + Entry("diff when removing a field", testInput{ a: unstructured.Unstructured{Object: map[string]any{ "a": 1, "b": 2, @@ -87,9 +92,8 @@ func TestDiff_basic_operations(t *testing.T) { }}, wantChanged: true, want: ".[b]: 2 != ", - }, - { - name: "diff when changing a field", + }), + Entry("diff when changing a field", testInput{ a: unstructured.Unstructured{Object: map[string]any{ "a": 1, "b": 2, @@ -100,9 +104,8 @@ func TestDiff_basic_operations(t *testing.T) { }}, wantChanged: true, want: ".[b]: 2 != 3", - }, - { - name: "diff when adding a entry to a list", + }), + Entry("diff when adding a entry to a list", testInput{ a: unstructured.Unstructured{Object: map[string]any{ "a": 1, "b": []int{1, 2}, @@ -113,9 +116,8 @@ func TestDiff_basic_operations(t *testing.T) { }}, wantChanged: true, want: ".[b][2]: != 3", - }, - { - name: "diff when removing a entry from a list", + }), + Entry("diff when removing a entry from a list", testInput{ a: unstructured.Unstructured{Object: map[string]any{ "a": 1, "b": []int{1, 2, 3}, @@ -126,9 +128,8 @@ func TestDiff_basic_operations(t *testing.T) { }}, wantChanged: true, want: ".[b][2]: 3 != ", - }, - { - name: "diff when changing a entry in a list", + }), + Entry("diff when changing a entry in a list", testInput{ a: unstructured.Unstructured{Object: map[string]any{ "a": 1, "b": []int{1, 2, 3}, @@ -139,9 +140,8 @@ func TestDiff_basic_operations(t *testing.T) { }}, wantChanged: true, want: ".[b][2]: 3 != 4", - }, - { - name: "diff when deleting a list", + }), + Entry("diff when deleting a list", testInput{ a: unstructured.Unstructured{Object: map[string]any{ "a": 1, "b": []int{1, 2, 3}, @@ -151,19 +151,6 @@ func TestDiff_basic_operations(t *testing.T) { }}, wantChanged: true, want: ".[b]: [1 2 3] != ", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - differ := newDiffer() - - diff, err := differ.Diff(&tt.a, &tt.b) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(diff.HasChanges()).To(Equal(tt.wantChanged)) - g.Expect(diff.String()).To(Equal(tt.want)) - }) - } -} + }), + ) +}) diff --git a/pkg/util/suite_test.go b/pkg/util/suite_test.go new file mode 100644 index 000000000..6a52aa41b --- /dev/null +++ b/pkg/util/suite_test.go @@ -0,0 +1,28 @@ +/* +Copyright 2025 Red Hat, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +package util + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestUtil(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Books Suite") +} From 7b3b8469832e0cbd79670fad3985965dc081fa82 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Fri, 31 Oct 2025 16:01:21 +0100 Subject: [PATCH 10/19] Review: coderabbit --- .../machineset_sync_controller.go | 2 +- pkg/util/diff.go | 31 +++++++++++-------- pkg/util/suite_test.go | 2 +- 3 files changed, 20 insertions(+), 15 deletions(-) diff --git a/pkg/controllers/machinesetsync/machineset_sync_controller.go b/pkg/controllers/machinesetsync/machineset_sync_controller.go index 924c19c9a..be42f7e72 100644 --- a/pkg/controllers/machinesetsync/machineset_sync_controller.go +++ b/pkg/controllers/machinesetsync/machineset_sync_controller.go @@ -773,7 +773,7 @@ func (r *MachineSetSyncReconciler) createOrUpdateCAPIInfraMachineTemplate(ctx co return updateErr } - if capiInfraMachineTemplatesDiff.HasChanges() { + if !capiInfraMachineTemplatesDiff.HasChanges() { logger.Info("No changes detected for CAPI infra machine template") return nil } diff --git a/pkg/util/diff.go b/pkg/util/diff.go index d45770ef9..7cd30c8f8 100644 --- a/pkg/util/diff.go +++ b/pkg/util/diff.go @@ -30,7 +30,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -var errTimedOutWaitingForFeatureGates = errors.New("objects to diff cannot be nil") +var ( + errObjectsToCompareCannotBeNil = errors.New("objects to diff cannot be nil") + errProviderSpecNotFound = errors.New("providerSpec not found") +) // DiffResult is the interface that represents the result of a diff operation. type DiffResult interface { @@ -123,16 +126,16 @@ type differ struct { // Diff compares the objects a and b, and returns a DiffResult. func (d *differ) Diff(a, b client.Object) (DiffResult, error) { if a == nil || b == nil { - return nil, errTimedOutWaitingForFeatureGates + return nil, errObjectsToCompareCannotBeNil } // 1. Convert the objects to unstructured. - unstructuredA, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&a) + unstructuredA, err := runtime.DefaultUnstructuredConverter.ToUnstructured(a) if err != nil { - return nil, fmt.Errorf("failed to convert b to unstructured: %w", err) + return nil, fmt.Errorf("failed to convert a to unstructured: %w", err) } - unstructuredB, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&b) + unstructuredB, err := runtime.DefaultUnstructuredConverter.ToUnstructured(b) if err != nil { return nil, fmt.Errorf("failed to convert b to unstructured: %w", err) } @@ -148,7 +151,7 @@ func (d *differ) Diff(a, b client.Object) (DiffResult, error) { } if err := modifyFunc(unstructuredB); err != nil { - return nil, fmt.Errorf("failed to run modify function %son b: %w", funcName, err) + return nil, fmt.Errorf("failed to run modify function %s on b: %w", funcName, err) } } @@ -172,22 +175,22 @@ func (d *differ) Diff(a, b client.Object) (DiffResult, error) { allKeys.Insert(k) } - diff := map[string][]string{} + diffByKey := map[string][]string{} // Diff each top-level key separately and record the output to the diff map. for k := range allKeys { - d := deep.Equal(unstructuredA[k], unstructuredB[k]) + diff := deep.Equal(unstructuredA[k], unstructuredB[k]) // Make the result deterministic. - sort.Strings(d) + sort.Strings(diff) - if len(d) > 0 { - diff[k] = d + if len(diff) > 0 { + diffByKey[k] = diff } } return &diffResult{ - diff: diff, + diff: diffByKey, providerSpecPath: d.providerSpecPath, }, nil } @@ -268,8 +271,10 @@ func WithProviderSpec(platform configv1.PlatformType, path []string, marshalProv d.modifyFuncs["ProviderSpec"] = func(obj map[string]interface{}) error { rawExtensionMap, found, err := unstructured.NestedMap(obj, path...) - if !found || err != nil { + if err != nil { return fmt.Errorf("failed to get providerSpec value: %w", err) + } else if !found { + return fmt.Errorf("%w at path %s", errProviderSpecNotFound, strings.Join(path, ".")) } rawExtension := &runtime.RawExtension{} diff --git a/pkg/util/suite_test.go b/pkg/util/suite_test.go index 6a52aa41b..a9f51f360 100644 --- a/pkg/util/suite_test.go +++ b/pkg/util/suite_test.go @@ -24,5 +24,5 @@ import ( func TestUtil(t *testing.T) { RegisterFailHandler(Fail) - RunSpecs(t, "Books Suite") + RunSpecs(t, "Util Suite") } From d50d2060f4c1b7ef002cbd2e5720912d72298069 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Mon, 3 Nov 2025 10:06:44 +0100 Subject: [PATCH 11/19] Review CodeRabbit --- pkg/util/diff.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pkg/util/diff.go b/pkg/util/diff.go index 7cd30c8f8..83e3b0ba0 100644 --- a/pkg/util/diff.go +++ b/pkg/util/diff.go @@ -140,8 +140,6 @@ func (d *differ) Diff(a, b client.Object) (DiffResult, error) { return nil, fmt.Errorf("failed to convert b to unstructured: %w", err) } - var additionalIgnoredPaths [][]string - // 2. Run the configured modify functions // This allows customizing the diffing process, e.g. remove conditions last transition time to ignore them during diffing // or separate handling for providerSpec. @@ -156,7 +154,7 @@ func (d *differ) Diff(a, b client.Object) (DiffResult, error) { } // 3. Remove fields configured to be ignored. - for _, ignorePath := range append(d.ignoredPath, additionalIgnoredPaths...) { + for _, ignorePath := range d.ignoredPath { unstructured.RemoveNestedField(unstructuredA, ignorePath...) unstructured.RemoveNestedField(unstructuredB, ignorePath...) } From d2b199f5f33d8982f33db3061237ec3627807ebb Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Fri, 7 Nov 2025 12:36:02 +0100 Subject: [PATCH 12/19] CodeRabbit nits --- pkg/controllers/machinesync/machine_sync_controller.go | 8 ++++---- .../machinesync/machine_sync_mapi2capi_infrastructure.go | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/controllers/machinesync/machine_sync_controller.go b/pkg/controllers/machinesync/machine_sync_controller.go index e66f30efb..e33186440 100644 --- a/pkg/controllers/machinesync/machine_sync_controller.go +++ b/pkg/controllers/machinesync/machine_sync_controller.go @@ -229,7 +229,7 @@ func (r *MachineSyncReconciler) Reconcile(ctx context.Context, req reconcile.Req capiMachineNotFound = true } else if err != nil { logger.Error(err, "Failed to get Cluster API Machine") - return ctrl.Result{}, fmt.Errorf("failed to get Cluster API machine:: %w", err) + return ctrl.Result{}, fmt.Errorf("failed to get Cluster API machine: %w", err) } if mapiMachineNotFound && capiMachineNotFound { @@ -481,7 +481,7 @@ func (r *MachineSyncReconciler) reconcileMAPIMachinetoCAPIMachine(ctx context.Co return ctrl.Result{}, fmt.Errorf("failed to convert Machine API machine owner references to Cluster API: %w", err) } - if err := r.validateMAPIToCAPIPlatfromSpecifics(ctx, sourceMAPIMachine); err != nil { + if err := r.validateMAPIToCAPIPlatformSpecifics(ctx, sourceMAPIMachine); err != nil { conversionErr := fmt.Errorf("failed to convert Machine API machine to Cluster API machine: %w", err) if condErr := r.applySynchronizedConditionWithPatch(ctx, sourceMAPIMachine, corev1.ConditionFalse, reasonFailedToConvertMAPIMachineToCAPI, conversionErr.Error(), nil); condErr != nil { return ctrl.Result{}, utilerrors.NewAggregate([]error{conversionErr, condErr}) @@ -597,8 +597,8 @@ func (r *MachineSyncReconciler) reconcileMAPIMachinetoCAPIMachine(ctx context.Co controllers.ReasonResourceSynchronized, messageSuccessfullySynchronizedMAPItoCAPI, &sourceMAPIMachine.Generation) } -// validateMAPIToCAPIPlatfromSpecifics verifies that shared CAPI resources are compatible before converting from MAPI -> CAPI. -func (r *MachineSyncReconciler) validateMAPIToCAPIPlatfromSpecifics(ctx context.Context, mapiMachine *mapiv1beta1.Machine) error { +// validateMAPIToCAPIPlatformSpecifics verifies that shared CAPI resources are compatible before converting from MAPI -> CAPI. +func (r *MachineSyncReconciler) validateMAPIToCAPIPlatformSpecifics(ctx context.Context, mapiMachine *mapiv1beta1.Machine) error { switch r.Platform { case configv1.AWSPlatformType: return r.validateMachineAWSLoadBalancers(ctx, mapiMachine) diff --git a/pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go b/pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go index 3f3dba2da..5655b48ad 100644 --- a/pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go +++ b/pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go @@ -84,9 +84,9 @@ func (r *MachineSyncReconciler) createOrUpdateCAPIInfraMachine(ctx context.Conte } if metadataUpdated || statusUpdated { - logger.Info("Successfully updated Cluster API machine") + logger.Info("Successfully updated Cluster API Infrastructure machine") } else { - logger.Info("No changes detected for Cluster API machine") + logger.Info("No changes detected for Cluster API Infrastructure machine") } return ctrl.Result{}, syncronizationIsProgressingFalse, nil From d53f7c02db73d3228e3d7a29debc69a718c418c0 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Thu, 13 Nov 2025 13:33:11 +0100 Subject: [PATCH 13/19] implement unit test implement test case for setting Security Groups without filters for AWS for OCPBUGS-63229 --- .../machine_sync_controller_test.go | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/pkg/controllers/machinesync/machine_sync_controller_test.go b/pkg/controllers/machinesync/machine_sync_controller_test.go index c16a6b6dd..1aed33017 100644 --- a/pkg/controllers/machinesync/machine_sync_controller_test.go +++ b/pkg/controllers/machinesync/machine_sync_controller_test.go @@ -336,6 +336,47 @@ var _ = Describe("With a running MachineSync Reconciler", func() { ) }) + Context("when the MAPI machine has MachineAuthority set to Machine API and the providerSpec has security groups", func() { + BeforeEach(func() { + By("Creating the MAPI machine") + providerSpecBuilder := machinev1resourcebuilder.AWSProviderSpec().WithLoadBalancers(nil). + WithSecurityGroups( + []mapiv1beta1.AWSResourceReference{ + { + Filters: []mapiv1beta1.Filter{{ + Name: "tag:Name", + Values: []string{"some-tag"}, + }}, + }, + { + ID: ptr.To("some-sg-id"), + }, + }, + ) + mapiMachine = mapiMachineBuilder.WithProviderSpecBuilder(providerSpecBuilder).Build() + + Eventually(k8sClient.Create(ctx, mapiMachine)).Should(Succeed(), "mapi machine should be able to be created") + + By("Setting the MAPI machine AuthoritativeAPI to MachineAPI") + Eventually(k.UpdateStatus(mapiMachine, func() { + mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI + })).Should(Succeed()) + }) + + Context("when the CAPI machine does not exist", func() { + It("should update the synchronized condition on the MAPI machine to True", func() { + Eventually(k.Object(mapiMachine), timeout).Should( + HaveField("Status.Conditions", ContainElement( + SatisfyAll( + HaveField("Type", Equal(consts.SynchronizedCondition)), + HaveField("Status", Equal(corev1.ConditionTrue)), + HaveField("Reason", Equal("ResourceSynchronized")), + HaveField("Message", Equal("Successfully synchronized MAPI Machine to CAPI")), + ))), + ) + }) + }) + }) Context("when the MAPI machine has MachineAuthority set to Machine API", func() { BeforeEach(func() { By("Creating the MAPI machine") From c8f5931ca7acb4c77ac58fcc768f522913f64d7c Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Thu, 13 Nov 2025 14:54:35 +0100 Subject: [PATCH 14/19] pkg/util/diff add more test coverage for differ options --- pkg/util/diff_test.go | 98 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 93 insertions(+), 5 deletions(-) diff --git a/pkg/util/diff_test.go b/pkg/util/diff_test.go index b2cf40b13..982293575 100644 --- a/pkg/util/diff_test.go +++ b/pkg/util/diff_test.go @@ -22,19 +22,16 @@ import ( ) var _ = Describe("Unit test Diff", func() { - var differ *differ - - BeforeEach(func() { - differ = newDiffer() - }) type testInput struct { a unstructured.Unstructured b unstructured.Unstructured + diffOpts []diffopts wantChanged bool want string } DescribeTable("basic operations", func(tt testInput) { + differ := newDiffer(tt.diffOpts...) diff, err := differ.Diff(&tt.a, &tt.b) Expect(err).ToNot(HaveOccurred()) Expect(diff.HasChanges()).To(Equal(tt.wantChanged)) @@ -50,6 +47,20 @@ var _ = Describe("Unit test Diff", func() { wantChanged: false, want: "", }), + Entry("no diff on matching objects", testInput{ + a: unstructured.Unstructured{Object: map[string]any{ + "a": 1, + "b": 2, + "c": map[string]any{}, + }}, + b: unstructured.Unstructured{Object: map[string]any{ + "a": 1, + "b": 2, + "c": map[string]any{}, + }}, + wantChanged: false, + want: "", + }), Entry("diff when adding a field", testInput{ a: unstructured.Unstructured{Object: map[string]any{ "a": 1, @@ -152,5 +163,82 @@ var _ = Describe("Unit test Diff", func() { wantChanged: true, want: ".[b]: [1 2 3] != ", }), + Entry("no diff on matching objects with ignore fields", testInput{ + a: unstructured.Unstructured{Object: map[string]any{ + "someKey": "someValue", + "changed": 1, + "removed": 2, + "nil": map[string]any{}, + }}, + b: unstructured.Unstructured{Object: map[string]any{ + "someKey": "someValue", + "changed": 2, + "nil": nil, + }}, + diffOpts: []diffopts{ + WithIgnoreField("changed"), + WithIgnoreField("removed"), + WithIgnoreField("nil"), + }, + wantChanged: false, + want: "", + }), + Entry("no diff on matching objects with ignore fields that does not exist", testInput{ + a: unstructured.Unstructured{Object: map[string]any{ + "someKey": "someValue", + }}, + b: unstructured.Unstructured{Object: map[string]any{ + "someKey": "someValue", + }}, + diffOpts: []diffopts{ + WithIgnoreField("doesnotexist"), + }, + wantChanged: false, + want: "", + }), + Entry("diff on not matching objects with ignore fields that do not exist or are properly ignored", testInput{ + a: unstructured.Unstructured{Object: map[string]any{ + "someKey": "someValue", + "shouldbeignored": 1, + "someChangedKey": "a", + }}, + b: unstructured.Unstructured{Object: map[string]any{ + "someKey": "someValue", + "shouldbeignored": 2, + "someChangedKey": "b", + }}, + diffOpts: []diffopts{ + WithIgnoreField("doesnotexist"), + WithIgnoreField("shouldbeignored"), + }, + wantChanged: true, + want: ".[someChangedKey]: a != b", + }), + Entry("no diff on matching objects with modifyFunc", testInput{ + a: unstructured.Unstructured{Object: map[string]any{ + "someKey": "someValue", + "changed": 1, + "removed": 2, + "nil": map[string]any{}, + }}, + b: unstructured.Unstructured{Object: map[string]any{ + "someKey": "someValue", + "changed": 2, + "nil": nil, + }}, + diffOpts: []diffopts{ + func(d *differ) { + d.modifyFuncs["test"] = func(obj map[string]interface{}) error { + obj["new"] = "new" + obj["changed"] = 3 + obj["removed"] = 3 + obj["nil"] = nil + return nil + } + }, + }, + wantChanged: false, + want: "", + }), ) }) From dd3dead478e0daeb99aabbcf0ae0eee65ffd1e09 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Thu, 13 Nov 2025 15:21:49 +0100 Subject: [PATCH 15/19] fixup --- pkg/util/diff_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/util/diff_test.go b/pkg/util/diff_test.go index 982293575..e43e792a8 100644 --- a/pkg/util/diff_test.go +++ b/pkg/util/diff_test.go @@ -228,11 +228,12 @@ var _ = Describe("Unit test Diff", func() { }}, diffOpts: []diffopts{ func(d *differ) { - d.modifyFuncs["test"] = func(obj map[string]interface{}) error { + d.modifyFuncs["test"] = func(obj map[string]interface{}) error { //nolint:unparam obj["new"] = "new" obj["changed"] = 3 obj["removed"] = 3 obj["nil"] = nil + return nil } }, From 32808488af24b51990d59af2fe05bf85beade1f3 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Fri, 14 Nov 2025 16:23:08 +0100 Subject: [PATCH 16/19] capo: make conversion deterministic --- pkg/conversion/mapi2capi/openstack.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/conversion/mapi2capi/openstack.go b/pkg/conversion/mapi2capi/openstack.go index b01ca2c27..819d60b0b 100644 --- a/pkg/conversion/mapi2capi/openstack.go +++ b/pkg/conversion/mapi2capi/openstack.go @@ -17,6 +17,7 @@ package mapi2capi import ( "fmt" + "sort" "strings" configv1 "github.com/openshift/api/config/v1" @@ -789,6 +790,10 @@ func convertMAPOServerMetadataToCAPO(mapoServerMetadata map[string]string) []ope capoServerMetadata = append(capoServerMetadata, openstackv1.ServerMetadata{Key: k, Value: v}) } + sort.SliceStable(capoServerMetadata, func(i, j int) bool { + return capoServerMetadata[i].Key < capoServerMetadata[j].Key + }) + return capoServerMetadata } From 53d400770ffb13cd44d7d50bcbd443fce90ebb2f Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Tue, 18 Nov 2025 13:59:10 +0100 Subject: [PATCH 17/19] diff: ignore kind and apiVersion when diffing --- pkg/util/diff.go | 9 +++++++++ pkg/util/diff_test.go | 11 +++++++++++ 2 files changed, 20 insertions(+) diff --git a/pkg/util/diff.go b/pkg/util/diff.go index 83e3b0ba0..f8d2c63fb 100644 --- a/pkg/util/diff.go +++ b/pkg/util/diff.go @@ -18,6 +18,7 @@ package util import ( "errors" "fmt" + "reflect" "sort" "strings" @@ -129,6 +130,10 @@ func (d *differ) Diff(a, b client.Object) (DiffResult, error) { return nil, errObjectsToCompareCannotBeNil } + if reflect.TypeOf(a) != reflect.TypeOf(b) { + return nil, fmt.Errorf("objects to diff are not of the same type: %T != %T", a, b) + } + // 1. Convert the objects to unstructured. unstructuredA, err := runtime.DefaultUnstructuredConverter.ToUnstructured(a) if err != nil { @@ -196,6 +201,10 @@ func (d *differ) Diff(a, b client.Object) (DiffResult, error) { // NewDefaultDiffer creates a new default differ with the default options. func NewDefaultDiffer(opts ...diffopts) *differ { return newDiffer(append(opts, + // Always ignore kind and apiVersion as they may not be always set. Instead the differ checks if the input objects have the same type. + WithIgnoreField("kind"), + WithIgnoreField("apiVersion"), + // Options for handling of metadata fields. // Special handling for Cluster API's conversion-data label. diff --git a/pkg/util/diff_test.go b/pkg/util/diff_test.go index e43e792a8..b88605ca1 100644 --- a/pkg/util/diff_test.go +++ b/pkg/util/diff_test.go @@ -18,11 +18,22 @@ package util import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + mapiv1beta1 "github.com/openshift/api/machine/v1beta1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) var _ = Describe("Unit test Diff", func() { + Describe("error cases", func() { + It("should return an error if the objects are not of the same type", func() { + a := &mapiv1beta1.MachineSet{} + b := &mapiv1beta1.Machine{} + _, err := NewDefaultDiffer().Diff(a, b) + Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError(ContainSubstring("objects to diff are not of the same type"))) + }) + }) + type testInput struct { a unstructured.Unstructured b unstructured.Unstructured From d2ce231ca214c0e1c692817de855ae3e0ecc0e87 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Wed, 19 Nov 2025 11:10:06 +0100 Subject: [PATCH 18/19] fix lint --- pkg/util/diff.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/util/diff.go b/pkg/util/diff.go index f8d2c63fb..9bd17360a 100644 --- a/pkg/util/diff.go +++ b/pkg/util/diff.go @@ -33,6 +33,7 @@ import ( var ( errObjectsToCompareCannotBeNil = errors.New("objects to diff cannot be nil") + errObjectsToCompareNotSameType = errors.New("objects to diff are not of the same type") errProviderSpecNotFound = errors.New("providerSpec not found") ) @@ -131,7 +132,7 @@ func (d *differ) Diff(a, b client.Object) (DiffResult, error) { } if reflect.TypeOf(a) != reflect.TypeOf(b) { - return nil, fmt.Errorf("objects to diff are not of the same type: %T != %T", a, b) + return nil, fmt.Errorf("%w: %T != %T", errObjectsToCompareNotSameType, a, b) } // 1. Convert the objects to unstructured. From d87410bd5b2a0ed84f3b571c5caea45f97ae9900 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Fri, 21 Nov 2025 15:11:05 +0100 Subject: [PATCH 19/19] differ: add option to ignore condition types, ignore Paused for CAPI and Synchronized for MAPI --- .../machinesync/machine_sync_controller.go | 7 +++- .../machine_sync_mapi2capi_infrastructure.go | 6 ++- pkg/util/diff.go | 42 +++++++++++++++++++ 3 files changed, 53 insertions(+), 2 deletions(-) diff --git a/pkg/controllers/machinesync/machine_sync_controller.go b/pkg/controllers/machinesync/machine_sync_controller.go index e33186440..032ad3534 100644 --- a/pkg/controllers/machinesync/machine_sync_controller.go +++ b/pkg/controllers/machinesync/machine_sync_controller.go @@ -1293,7 +1293,10 @@ func (r *MachineSyncReconciler) ensureSyncFinalizer(ctx context.Context, mapiMac // compareCAPIMachines compares CAPI machines a and b, and returns a list of differences, or none if there are none. func compareCAPIMachines(capiMachine1, capiMachine2 *clusterv1.Machine) (util.DiffResult, error) { - diff, err := util.NewDefaultDiffer().Diff(capiMachine1, capiMachine2) + diff, err := util.NewDefaultDiffer( + // The paused condition is always handled by the corresponding CAPI controller. + util.WithIgnoreConditionType(clusterv1.PausedV1Beta2Condition), + ).Diff(capiMachine1, capiMachine2) if err != nil { return nil, fmt.Errorf("failed to compare Cluster API machines: %w", err) } @@ -1310,6 +1313,8 @@ func compareMAPIMachines(platform configv1.PlatformType, a, b *mapiv1beta1.Machi util.WithIgnoreField("status", "lastOperation"), util.WithIgnoreField("status", "authoritativeAPI"), util.WithIgnoreField("status", "synchronizedGeneration"), + // The synchronized condition is always handled by the migration controller separately. + util.WithIgnoreConditionType(string(controllers.SynchronizedCondition)), ).Diff(a, b) if err != nil { diff --git a/pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go b/pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go index 5655b48ad..948c03738 100644 --- a/pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go +++ b/pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go @@ -29,6 +29,7 @@ import ( awsv1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2" ibmpowervsv1 "sigs.k8s.io/cluster-api-provider-ibmcloud/api/v1beta2" openstackv1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -292,7 +293,10 @@ func compareCAPIInfraMachines(platform configv1.PlatformType, infraMachine1, inf return nil, fmt.Errorf("%w: %s", errPlatformNotSupported, platform) } - diff, err := util.NewDefaultDiffer().Diff(obj1, obj2) + diff, err := util.NewDefaultDiffer( + // The paused condition is always handled by the corresponding CAPI controller. + util.WithIgnoreConditionType(clusterv1.PausedV1Beta2Condition), + ).Diff(obj1, obj2) if err != nil { return nil, fmt.Errorf("failed to compare Cluster API infrastructure machines: %w", err) } diff --git a/pkg/util/diff.go b/pkg/util/diff.go index 9bd17360a..de050f5e5 100644 --- a/pkg/util/diff.go +++ b/pkg/util/diff.go @@ -272,6 +272,48 @@ func WithIgnoreConditionsLastTransitionTime() diffopts { } } +// WithIgnoreConditionType conditionType stringc onfigures the differ to ignore the condition of the given type when executing Diff. +func WithIgnoreConditionType(conditionType string) diffopts { + return func(d *differ) { + d.modifyFuncs[fmt.Sprintf("RemoveCondition[%s]", conditionType)] = func(a map[string]interface{}) error { + conditionPaths := [][]string{ + {"status", "conditions"}, + {"status", "v1beta2", "conditions"}, + {"status", "deprecated", "v1beta1", "conditions"}, + } + + for _, conditionPath := range conditionPaths { + conditions, found, err := unstructured.NestedSlice(a, conditionPath...) + if !found || err != nil { + continue + } + + newConditions := []interface{}{} + + for _, condition := range conditions { + conditionMap, ok := condition.(map[string]interface{}) + if !ok { + continue + } + + // Skip condition of the given type. + if conditionMap["type"] == conditionType { + continue + } + + newConditions = append(newConditions, condition) + } + + if err := unstructured.SetNestedField(a, newConditions, conditionPath...); err != nil { + return fmt.Errorf("failed to set nested field %s: %w", strings.Join(conditionPath, "."), err) + } + } + + return nil + } + } +} + // WithProviderSpec configures the differ to separately diff .spec.providerSpec. func WithProviderSpec(platform configv1.PlatformType, path []string, marshalProviderSpec func(platform configv1.PlatformType, rawExtension *runtime.RawExtension) (any, error)) diffopts { return func(d *differ) {