Skip to content

OCPCLOUD-3172: machinesetsync: refactor to a generalized differ which can work independent of types #382

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
+945 −566
Open

OCPCLOUD-3172: machinesetsync: refactor to a generalized differ which can work independent of types #382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
af19afc
pkg/util implement unit tests for CAPIMachineSetStatusEqual
chrischdi Oct 30, 2025
70bcc0e
pkg/util/sync: first iteration for new differ approach
chrischdi Oct 10, 2025
2881446
pkg/util/sync: second iteration prettify the output string
chrischdi Oct 30, 2025
88631d5
use differ everywhere
chrischdi Oct 30, 2025
97c0192
always compare client.Object
chrischdi Oct 31, 2025
2e368e6
fix MAPI MachineSet providerSpec diff
chrischdi Oct 31, 2025
cd6b956
Use proper naming for Cluster API and Machine API
chrischdi Oct 31, 2025
2ce10c4
self-review fixes
chrischdi Oct 31, 2025
d97f73f
rewrite tests in ginkgo style
chrischdi Oct 31, 2025
d00d34c
Review: coderabbit
chrischdi Oct 31, 2025
bc64cb8
Review CodeRabbit
chrischdi Nov 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
171 changes: 44 additions & 127 deletions pkg/controllers/machinesetsync/machineset_sync_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"errors"
"fmt"
"slices"
"sort"

"github.com/go-logr/logr"
configv1 "github.com/openshift/api/config/v1"
Expand All @@ -40,7 +39,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"
Expand Down Expand Up @@ -775,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
}
Expand Down Expand Up @@ -820,7 +818,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 Cluster API machine sets: %w", err)
}

// Make a deep copy of the converted CAPI machine set to avoid modifying the original.
updatedOrCreatedCAPIMachineSet := convertedCAPIMachineSet.DeepCopy()
Expand Down Expand Up @@ -873,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
}

Expand All @@ -899,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
}

Expand Down Expand Up @@ -950,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
}

Expand All @@ -977,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
}

Expand Down Expand Up @@ -1051,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)
}
Expand Down Expand Up @@ -1347,9 +1348,9 @@ 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) {
func compareCAPIInfraMachineTemplates(platform configv1.PlatformType, infraMachineTemplate1, infraMachineTemplate2 client.Object) (util.DiffResult, error) {
var obj1, obj2 client.Object

switch platform {
case configv1.AWSPlatformType:
typedInfraMachineTemplate1, ok := infraMachineTemplate1.(*awsv1.AWSMachineTemplate)
Expand All @@ -1362,19 +1363,8 @@ 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
obj1 = typedInfraMachineTemplate1
obj2 = typedinfraMachineTemplate2
case configv1.OpenStackPlatformType:
typedInfraMachineTemplate1, ok := infraMachineTemplate1.(*openstackv1.OpenStackMachineTemplate)
if !ok {
Expand All @@ -1386,17 +1376,8 @@ 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
obj1 = typedInfraMachineTemplate1
obj2 = typedinfraMachineTemplate2
case configv1.PowerVSPlatformType:
typedInfraMachineTemplate1, ok := infraMachineTemplate1.(*ibmpowervsv1.IBMPowerVSMachineTemplate)
if !ok {
Expand All @@ -1408,92 +1389,44 @@ 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
}

if diffObjectMeta := deep.Equal(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
obj1 = typedInfraMachineTemplate1
obj2 = typedinfraMachineTemplate2
default:
return nil, fmt.Errorf("%w: %s", errPlatformNotSupported, platform)
}
}

// 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 {
diff := make(map[string]any)

if diffSpec := deep.Equal(capiMachineSet1.Spec, capiMachineSet2.Spec); len(diffSpec) > 0 {
diff[".spec"] = diffSpec
diff, err := util.NewDefaultDiffer().Diff(obj1, obj2)
if err != nil {
return nil, fmt.Errorf("failed to compare Cluster API infrastructure machine templates: %w", err)
}

if diffObjectMeta := util.ObjectMetaEqual(capiMachineSet1.ObjectMeta, capiMachineSet2.ObjectMeta); len(diffObjectMeta) > 0 {
diff[".metadata"] = diffObjectMeta
}
return diff, nil
}

if diffStatus := util.CAPIMachineSetStatusEqual(capiMachineSet1.Status, capiMachineSet2.Status); len(diffStatus) > 0 {
diff[".status"] = diffStatus
// 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 {
return nil, fmt.Errorf("failed to compare Cluster API machinesets: %w", err)
}

return diff
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", "spec", "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
})

if diffProviderSpec := deep.Equal(ps1, ps2); len(diffProviderSpec) > 0 {
diff[".providerSpec"] = diffProviderSpec
}

// 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
}

if diffMetadata := util.ObjectMetaEqual(aCopy.ObjectMeta, bCopy.ObjectMeta); len(diffMetadata) > 0 {
diff[".metadata"] = diffMetadata
}

if diffStatus := util.MAPIMachineSetStatusEqual(a.Status, b.Status); len(diffStatus) > 0 {
diff[".status"] = diffStatus
return nil, fmt.Errorf("failed to compare Machine API machinesets: %w", err)
}

return diff, nil
Expand Down Expand Up @@ -1551,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 {
Expand Down
9 changes: 4 additions & 5 deletions pkg/controllers/machinesetsync/machineset_sync_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
})
})
})
Expand Down
Loading