OCPCLOUD-3172: machinesetsync: refactor to a generalized differ which can work independent of types #382
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@chrischdi: This pull request references OCPCLOUD-3172 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughReplaces ad-hoc map-based diffs with a structured pkg/util DiffResult across machine and machineset controllers; introduces a pluggable, platform-aware differ with providerSpec extraction and deterministic output; removes github.com/go-test/deep; updates many function signatures and adds unit tests for the new diffing behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Reconciler
participant Converter
participant Differ as "pkg/util differ"
participant API as "K8s API"
Reconciler->>Converter: build converted object (CAPI/MAPI)
Reconciler->>Differ: diffResult, err = Diff(existing, converted)
alt Diff error
Differ-->>Reconciler: error
Reconciler->>API: record/return error
else Diff computed
Differ-->>Reconciler: DiffResult (HasChanges?, HasSpec?, HasStatus?, HasProviderSpec?)
alt HasSpecChanges or HasMetadataChanges or HasProviderSpecChanges
Reconciler->>API: patch/update spec/metadata
end
alt HasStatusChanges
Reconciler->>API: patch/update status (with observedGeneration guards)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Focus review on:
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
/test unit |
There was a problem hiding this comment.
I think this is great and a clear improvement. I have some follow-on ideas, but I would personally gladly merge this and iterate on it in the unlikely event it ever became worth it.
I shared a thought inline based on something similar we did in ORC with field indexers.
Not at all important, but I also feel that Unstructured is an internal implementation detail: e.g. the Diff method could be implemented with reflection and the signature would not change.
pkg/util/sync.go
Outdated
| differ := UnstructuredDiffer[clusterv1.MachineSetStatus]{ | ||
| customDiff: []func(a clusterv1.MachineSetStatus, b clusterv1.MachineSetStatus) ([]string, error){ | ||
| func(a, b clusterv1.MachineSetStatus) ([]string, error) { | ||
| return compareCAPIMachineSetConditions(a.Conditions, b.Conditions), nil | ||
| }, | ||
| }, | ||
| ignoreFields: [][]string{{"conditions"}}, | ||
| } |
There was a problem hiding this comment.
differ := UnstructuredDiffer[clusterv1.MachineSetStatus]()
differ = WithCustomDiff(differ,
"conditions",
func(x *clusterv1.MachineSetStatus) []clusterv1.Condition {return x.conditions},
compareCAPIConditions)
func compareCAPIConditions(a, b []clusterv1.Conditions) []string {
...
}WithCustomDiff unfortunately can't be a method on UnstructuredDiffer as it has an additional type argument([]clusterv1.Condition).
Advantages of this approach:
- ignoreField is a required argument of CustomDiff, removing a footgun
- compareCAPIConditions is now reusable by all types with CAPI conditions
Disadvantages:
- The argument to pull a field out is a bit ugly
There was a problem hiding this comment.
ignoreField is a required argument of CustomDiff, removing a footgun
Are there cases when we want to ignore fields without custom diffs no? Maybe around deprecated fields?
There was a problem hiding this comment.
Yes there are (e.g. CAPI's conversion-data annotation).
However I'd combine it, but keeping the option to ignore paths completely:
differ := NewUnstructuredDiffer(
WithCustomDiff([]string{"conditions"}, func(a, b clusterv1.MachineSetStatus) ([]string, error) {
return compareCAPIV1Beta1Conditions(a.Conditions, b.Conditions), nil
}),
WithCustomDiff([]string{"v1beta2", "conditions"}, func(a, b clusterv1.MachineSetStatus) ([]string, error) {
return compareCAPIV1Beta2Conditions(
ptr.Deref(a.V1Beta2, clusterv1.MachineSetV1Beta2Status{}).Conditions,
ptr.Deref(b.V1Beta2, clusterv1.MachineSetV1Beta2Status{}).Conditions),
nil
}),
WithIgnoreField[clusterv1.MachineSetStatus]("conditions", "bar"),
)There was a problem hiding this comment.
Refactored, don't have the "WithCustomDiff" anymore.
I now have an option for the conditions part instead.
openshift-merge-robot
added
the
needs-rebase
pkg/util/diff.go
Outdated
| "k8s.io/apimachinery/pkg/runtime" | ||
| ) | ||
|
|
||
| type UnstructuredDiffer[T any] struct { |
There was a problem hiding this comment.
If this is unstructured, why is it also generic? Does it make this simpler than using runtime.Object for the a, b arguments?
There was a problem hiding this comment.
Renamed just to differ. Should work for any struct.
pkg/util/diff.go
Outdated
|
|
||
| diff := deep.Equal(unstructuredA, unstructuredB) | ||
| if len(diff) > 0 { | ||
| diffs["deep.Equal"] = diff |
There was a problem hiding this comment.
Why the deep.Equal key? Does this get exposed to end users? Is this a magic word?
There was a problem hiding this comment.
Not there anymore :-)
pkg/util/sync.go
Outdated
| // Maybe we can also have it this way: | ||
| // differ := NewUnstructuredDiffer( | ||
| // WithIgnoreField[clusterv1.MachineSetStatus]("conditions"), | ||
| // WithCustomDiff(func(a, b clusterv1.MachineSetStatus) ([]string, error) { | ||
| // return compareCAPIMachineSetConditions(a.Conditions, b.Conditions), nil | ||
| // }), | ||
| // ) |
pkg/util/sync.go
Outdated
| differ := UnstructuredDiffer[clusterv1.MachineSetStatus]{ | ||
| customDiff: []func(a clusterv1.MachineSetStatus, b clusterv1.MachineSetStatus) ([]string, error){ | ||
| func(a, b clusterv1.MachineSetStatus) ([]string, error) { | ||
| return compareCAPIMachineSetConditions(a.Conditions, b.Conditions), nil | ||
| }, | ||
| }, | ||
| ignoreFields: [][]string{{"conditions"}}, | ||
| } |
There was a problem hiding this comment.
ignoreField is a required argument of CustomDiff, removing a footgun
Are there cases when we want to ignore fields without custom diffs no? Maybe around deprecated fields?
chrischdi
force-pushed
the
pr-sync-refactor-how-to-diff
branch
from
ac815fd to
f494b0c
Compare
openshift-merge-robot
removed
the
needs-rebase
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@chrischdi: This pull request references OCPCLOUD-3172 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
| // 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() | ||
| } |
There was a problem hiding this comment.
I'm thinking about doing this even more generic, as this is the same across the codebase.
There was a problem hiding this comment.
More generic as in abstracting each of the diff fields?
There was a problem hiding this comment.
Nope, updated the PR. We now don't have to distinct and manually do the diff for metadata, spec, status.
We always compare the whole object and can have all results:
...Diff(clusterv1.Machine, clusterv1.Machine)
:-)
| status1 = typedInfraMachineTemplate1.Status | ||
| status2 = typedinfraMachineTemplate2.Status |
There was a problem hiding this comment.
Note: added status conversion here.
Let's see if we break something or not :-)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controllers/machinesetsync/machineset_sync_controller.go (1)
1346-1354: Return the correct OpenStack assertion error.In the OpenStack branch we still return
errAssertingCAPIIBMPowerVSMachineTemplateon a type mismatch. Callers that rely onerrors.Is(..., errAssertingCAPIOpenStackMachineTemplate)will now miss this case, breaking platform-specific handling. Please swap the constant to the OpenStack one for both checks in this branch.- typedInfraMachineTemplate1, ok := infraMachineTemplate1.(*openstackv1.OpenStackMachineTemplate) - if !ok { - return nil, errAssertingCAPIIBMPowerVSMachineTemplate - } + typedInfraMachineTemplate1, ok := infraMachineTemplate1.(*openstackv1.OpenStackMachineTemplate) + if !ok { + return nil, errAssertingCAPIOpenStackMachineTemplate + } typedinfraMachineTemplate2, ok := infraMachineTemplate2.(*openstackv1.OpenStackMachineTemplate) if !ok { return nil, errAssertingCAPIOpenStackMachineTemplate }
🧹 Nitpick comments (2)
pkg/util/sync_test.go (1)
332-341: Assert the string output even for unchanged diffs
Right now we skipgot.String()comparisons whentt.wantChangesis false, so a future regression whereChanged()returns false butString()leaks content would slide through. Consider moving theg.Expect(got.String()).To(Equal(tt.want))assertion outside the conditional (the happy-path cases already setwantto""), or duplicating it in theelse. That keeps the test guarding both facets of the contract.pkg/controllers/machinesync/machine_sync_controller.go (1)
1259-1277: Tighten the error message wording
These error strings still say “Cluster API Infrastructure machine …” even though this helper compares the top-level CAPI Machine. Tweaking the wording here (and in the similar spec/status branches) would make logs clearer when we do hit this path.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (7)
pkg/controllers/machinesetsync/machineset_sync_controller.go(6 hunks)pkg/controllers/machinesync/machine_sync_controller.go(3 hunks)pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go(5 hunks)pkg/util/diff.go(1 hunks)pkg/util/diff_test.go(1 hunks)pkg/util/sync.go(1 hunks)pkg/util/sync_test.go(1 hunks)
🔇 Additional comments (1)
pkg/util/diff_test.go (1)
78-115: Nice coverage for slice diffs
Exercising add/remove/change scenarios on list fields gives strong confidence that the new differ is handling positional data correctly. Thanks for including these cases.
| // 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() | ||
| } |
There was a problem hiding this comment.
More generic as in abstracting each of the diff fields?
|
|
||
| out := "." + strings.Join(d.diff, ", .") | ||
| out = strings.ReplaceAll(out, ".slice[", "[") | ||
| out = strings.ReplaceAll(out, "map[", "[") |
There was a problem hiding this comment.
Might be a nit, but could we distinguish maps from slices with {}? Might be too much work, since we'd have to find and replace the closing characters, but it could be helpful in interpreting output.
There was a problem hiding this comment.
This is pure cosmetics / output (I guess mostly for us when debugging).
I'm even okay with keeping the more verbose mode.
|
@chrischdi: This pull request references OCPCLOUD-3172 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@chrischdi: This pull request references OCPCLOUD-3172 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/util/diff.go (1)
126-134: Don't wrap the operands in&before converting to unstructured.
runtime.DefaultUnstructuredConverter.ToUnstructuredexpects the actual object. Passing&a/&bhands it*client.Object, so every diff on typed resources fails. While fixing that, please correct the first error message so it references “a”.- unstructuredA, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&a) - if err != nil { - return nil, fmt.Errorf("failed to convert b to unstructured: %w", err) + unstructuredA, err := runtime.DefaultUnstructuredConverter.ToUnstructured(a) + if err != nil { + return nil, fmt.Errorf("failed to convert a to unstructured: %w", err) } - unstructuredB, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&b) + unstructuredB, err := runtime.DefaultUnstructuredConverter.ToUnstructured(b)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (9)
pkg/controllers/machinesetsync/machineset_sync_controller.go(11 hunks)pkg/controllers/machinesetsync/machineset_sync_controller_test.go(1 hunks)pkg/controllers/machinesetsync/machineset_sync_controller_unit_test.go(1 hunks)pkg/controllers/machinesync/machine_sync_controller.go(6 hunks)pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go(7 hunks)pkg/conversion/mapi2capi/util.go(1 hunks)pkg/util/diff.go(1 hunks)pkg/util/diff_test.go(1 hunks)pkg/util/sync.go(0 hunks)
💤 Files with no reviewable changes (1)
- pkg/util/sync.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/util/diff_test.go
🔇 Additional comments (13)
pkg/conversion/mapi2capi/util.go (1)
44-49: Deterministic tag ordering looks great.Sorting the AWS tags up front eliminates noisy diffs when we convert back and forth between map and slice representations.
pkg/controllers/machinesetsync/machineset_sync_controller_test.go (1)
1177-1188: Thanks for exercising the new diff surface.Checking
HasProviderSpecChanges()andHasChanges()here makes sure the platform-aware comparer behaves correctly.pkg/controllers/machinesetsync/machineset_sync_controller_unit_test.go (1)
31-351: Great coverage for DiffResult behaviour.These table-driven cases hit all the tricky status paths—including the v1beta1/v1beta2 condition handling and LastTransitionTime suppression—so we can trust the refactored comparer.
pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go (1)
63-82: DiffResult integration reads clean.Branching on
HasSpecChanges,HasMetadataChanges, andHasStatusChangeskeeps the reconciliation flow explicit while reusing the shared differ.pkg/controllers/machinesync/machine_sync_controller.go (5)
649-674: LGTM! Signature and logic updated correctly.The function signature has been appropriately updated to accept
util.DiffResult, and the conditional logic correctly uses the newHas*Changes()methods to determine if updates are needed.
697-700: LGTM! Error handling added correctly.The comparison function now properly returns and propagates errors, improving robustness of the diff operation.
1254-1279: LGTM! Refactored comparison functions.Both
compareCAPIMachinesandcompareMAPIMachineshave been properly refactored to:
- Use the new
util.NewDefaultDiffer()interface- Return structured
util.DiffResultinstead of maps- Propagate errors from diff computation
- Support platform-aware provider spec comparison (MAPI machines)
The platform-aware diffing with
WithProviderSpecand field ignore options incompareMAPIMachinesis a solid improvement for handling provider-specific differences.
1282-1331: LGTM! Status update functions refactored correctly.Both
ensureCAPIMachineStatusUpdatedandensureMAPIMachineStatusUpdatedhave been consistently updated to acceptutil.DiffResultand use theHasStatusChanges()method for determining if status updates are needed.Also applies to: 1363-1408
1411-1434: LGTM! Comprehensive change detection for MAPI machines.The function correctly checks for metadata, spec, and provider spec changes, which is appropriate given that MAPI machines have provider-specific specifications that need to be tracked separately.
pkg/controllers/machinesetsync/machineset_sync_controller.go (4)
787-814: LGTM! Proper error handling for comparison.The comparison function call correctly handles errors and propagates them with appropriate wrapping.
843-866: LGTM! All ensure functions refactored correctly.All four
ensure*Updatedfunctions have been consistently updated to:
- Accept
util.DiffResultinstead of map-based diffs- Use the appropriate
Has*Changes()methods- Apply correct conditional logic with proper negation for early returns
Also applies to: 869-917, 920-944, 947-996
1021-1024: LGTM! Platform parameter added correctly.The comparison function correctly receives the platform parameter to enable platform-aware provider spec diffing.
1317-1399: LGTM! Comparison functions properly refactored.All three comparison functions have been successfully refactored to:
- Return
(util.DiffResult, error)instead of map-based diffs- Use
util.NewDefaultDiffer()with appropriate configuration options- Handle platform-specific differences where needed (provider specs)
- Properly wrap and propagate errors
The platform-aware diffing in
compareCAPIInfraMachineTemplatesandcompareMAPIMachineSetswith support for provider specs and field filtering is a significant improvement.
openshift-merge-robot
added
the
needs-rebase
|
@chrischdi: This pull request references OCPCLOUD-3172 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
chrischdi
force-pushed
the
pr-sync-refactor-how-to-diff
branch
from
a9987c6 to
dbbfcd3
Compare
openshift-merge-robot
removed
the
needs-rebase
|
@chrischdi: This pull request references OCPCLOUD-3172 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/controllers/machinesetsync/machineset_sync_controller.go (1)
742-745: Critical: Inverted condition prevents infrastructure template updates.The condition is inverted: when
HasChanges()returnstrue(changes exist), the code incorrectly logs "No changes detected" and returns early, preventing legitimate updates from being applied.Apply this fix:
- if capiInfraMachineTemplatesDiff.HasChanges() { + if !capiInfraMachineTemplatesDiff.HasChanges() { logger.Info("No changes detected for CAPI infra machine template") return nil }
🧹 Nitpick comments (2)
pkg/conversion/mapi2capi/util.go (1)
33-53: Export the sentinel error so callers can detect unsupported platforms.Because
ProviderSpecFromRawExtensionis exported, downstream callers will likely need to distinguish an unsupported platform from other parse failures. Keeping the sentinel unexported forces string matching; exporting it (e.g.,ErrUnsupportedPlatform) lets clients rely onerrors.Is. Please export the sentinel and update the reference accordingly.-var errUnsupportedPlatform = errors.New("unsupported platform") +var ErrUnsupportedPlatform = errors.New("unsupported platform") ... - return nil, fmt.Errorf("%w: %s", errUnsupportedPlatform, platform) + return nil, fmt.Errorf("%w: %s", ErrUnsupportedPlatform, platform)pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go (1)
248-301: LGTM: Platform-aware comparison successfully generalized.The refactored comparison function correctly handles platform-specific type assertions before delegating to the type-agnostic differ. The error handling is comprehensive, and the default case ensures unsupported platforms are caught.
Optional: Consider renaming
obj1andobj2totypedInfraMachine1andtypedInfraMachine2for consistency with the parameter names and to make the type assertion flow clearer.func compareCAPIInfraMachines(platform configv1.PlatformType, infraMachine1, infraMachine2 client.Object) (util.DiffResult, error) { - var obj1, obj2 client.Object + var typedInfraMachine1, typedInfraMachine2 client.Object switch platform { case configv1.AWSPlatformType: - typedInfraMachine1, ok := infraMachine1.(*awsv1.AWSMachine) + awsMachine1, ok := infraMachine1.(*awsv1.AWSMachine) if !ok { return nil, errAssertingCAPIAWSMachine } - typedinfraMachine2, ok := infraMachine2.(*awsv1.AWSMachine) + awsMachine2, ok := infraMachine2.(*awsv1.AWSMachine) if !ok { return nil, errAssertingCAPIAWSMachine } - obj1 = typedInfraMachine1 - obj2 = typedinfraMachine2 + typedInfraMachine1 = awsMachine1 + typedInfraMachine2 = awsMachine2 // ... similar for other platforms } - diff, err := util.NewDefaultDiffer().Diff(obj1, obj2) + diff, err := util.NewDefaultDiffer().Diff(typedInfraMachine1, typedInfraMachine2) if err != nil { return nil, fmt.Errorf("failed to compare Cluster API infrastructure machines: %w", err) } return diff, nil }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (9)
pkg/controllers/machinesetsync/machineset_sync_controller.go(11 hunks)pkg/controllers/machinesetsync/machineset_sync_controller_test.go(1 hunks)pkg/controllers/machinesetsync/machineset_sync_controller_unit_test.go(1 hunks)pkg/controllers/machinesync/machine_sync_controller.go(6 hunks)pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go(7 hunks)pkg/conversion/mapi2capi/util.go(1 hunks)pkg/util/diff.go(1 hunks)pkg/util/diff_test.go(1 hunks)pkg/util/sync.go(0 hunks)
💤 Files with no reviewable changes (1)
- pkg/util/sync.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/util/diff_test.go
- pkg/util/diff.go
🔇 Additional comments (20)
pkg/controllers/machinesetsync/machineset_sync_controller_unit_test.go (1)
30-351: Excellent test coverage for status diffing!The comprehensive test suite effectively validates the new DiffResult-based diff approach across multiple scenarios:
- Individual and combined field differences
- v1beta1 and v1beta2 conditions handling
- LastTransitionTime correctly ignored in comparisons
- nil vs non-nil status scenarios
The test structure is clear and the assertions properly validate both
HasChanges()and the diff string format.pkg/controllers/machinesetsync/machineset_sync_controller_test.go (1)
1175-1189: Test updates correctly reflect new diff API.The changes properly adapt to the platform-aware
compareMAPIMachineSetssignature and use the newDiffResultmethods (HasProviderSpecChanges(),HasChanges()) instead of map-based checks.pkg/controllers/machinesetsync/machineset_sync_controller.go (9)
787-790: Proper error handling for diff operation.The updated signature correctly propagates errors from
compareCAPIMachineSets, with appropriate error wrapping for context.
843-849: Clean integration with DiffResult API.The function signature and logic correctly use the new
DiffResultmethods (HasMetadataChanges(),HasSpecChanges(),HasProviderSpecChanges()) to determine when spec updates are needed.
869-875: Correct status update gating logic.The function properly uses
HasStatusChanges()and thespecUpdatedflag to determine when status updates are necessary, maintaining the synchronization semantics.
920-926: Consistent diff checking for MAPI spec updates.The implementation mirrors the CAPI version with appropriate use of
HasMetadataChanges(),HasSpecChanges(), andHasProviderSpecChanges()to detect when updates are needed.
947-953: Status update logic correctly adapted.The function properly integrates with the new
DiffResultAPI, usingHasStatusChanges()and thespecUpdatedflag to gate status updates appropriately.
1021-1024: Platform-aware diff comparison properly integrated.The call to
compareMAPIMachineSetscorrectly includes the platform parameter to enable platform-specific providerSpec handling, with appropriate error propagation.
1317-1370: Well-structured platform-aware infrastructure template comparison.The refactored function cleanly handles platform-specific types (AWS, OpenStack, PowerVS) and delegates to the new
util.NewDefaultDiffer()for consistent, generalized diffing with proper error handling.
1373-1380: Simplified and consistent CAPI MachineSet comparison.The refactored function correctly delegates to
util.NewDefaultDiffer()for consistent comparison logic, with proper error handling.
1383-1399: Comprehensive platform-aware MAPI MachineSet comparison.The refactored function properly configures the differ with:
- Platform-specific providerSpec handling via
WithProviderSpec- Appropriate status field exclusions (replicas, observedGeneration, etc.) that are managed separately by sync logic
- Clean error handling and propagation
pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go (3)
66-92: LGTM: Clean migration to DiffResult-based spec change detection.The refactored logic correctly uses
diff.HasSpecChanges()to detect when the infrastructure machine needs recreation due to immutable spec changes. The error handling and logging properly capture the diff details for debugging.
124-147: LGTM: Signature update aligns with the new differ approach.The function signature now accepts
util.DiffResult, and the logic correctly usesHasMetadataChanges()for early-exit optimization. This is a clean improvement over the previous map-based approach.
149-204: LGTM: Status update logic correctly uses DiffResult.The refactored signature and logic properly use
diff.HasStatusChanges()to determine when status updates are needed. The generation synchronization check (line 160) ensures consistency between MAPI and CAPI resources.pkg/controllers/machinesync/machine_sync_controller.go (6)
649-674: LGTM: Spec update logic cleanly migrated to DiffResult.The refactored function correctly uses
capiMachinesDiff.HasMetadataChanges()andcapiMachinesDiff.HasSpecChanges()to determine when updates are needed. The early-return optimization on line 653 is logically sound.
697-700: LGTM: Call site properly handles new comparison API.The error handling and return type match the updated
compareCAPIMachinessignature, with proper error wrapping.
751-754: LGTM: Platform parameter enables platform-aware diffing.The addition of the
r.Platformparameter is essential for the new platform-aware providerSpec comparison logic. This change aligns with the PR's refactoring objectives.
1254-1261: LGTM: Simplified and type-safe comparison for CAPI machines.The refactored implementation is clean and concise, delegating to the generalized differ. The error handling properly wraps errors for context. This is a significant improvement over the previous approach.
1264-1279: Excellent: Platform-aware MAPI comparison with providerSpec handling.This is a key improvement in the refactoring. The differ configuration correctly:
- Uses platform-specific providerSpec conversion via
WithProviderSpec(line 1266)- Ignores controller-managed status fields that should not affect sync decisions (lines 1268-1271)
- Enables deterministic, structured field-by-field comparison
The
mapi2capi.ProviderSpecFromRawExtensionconverter ensures platform-specific providerSpec schemas are handled correctly.
1411-1434: LGTM: MAPI spec update correctly includes providerSpec changes.The refactored logic properly checks for
HasProviderSpecChanges()in addition to metadata and spec changes (line 1415). This is essential for MAPI machines since their providerSpec field contains platform-specific configuration that requires special handling during comparison.
|
@chrischdi: This pull request references OCPCLOUD-3172 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pkg/util/diff.go (1)
130-138: Fix the unstructured conversion and error message.
runtime.DefaultUnstructuredConverter.ToUnstructuredexpects the concrete object, not a pointer to the interface wrapper. By passing&a/&b, you're handing it values of type*interface{}, which the converter cannot handle, so every diff involving typed structs will error out. Additionally, the first error message incorrectly says "convert b" when it should say "convert a".Apply this diff:
- 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) }pkg/controllers/machinesetsync/machineset_sync_controller.go (1)
742-745: Critical logic error: Inverted condition.The condition
if capiInfraMachineTemplatesDiff.HasChanges()is inverted. WhenHasChanges()returnstrue, it means there ARE changes detected, but the code logs "No changes detected" and returns early, skipping the update. This will prevent legitimate updates from being applied.Apply this fix:
- if capiInfraMachineTemplatesDiff.HasChanges() { + if !capiInfraMachineTemplatesDiff.HasChanges() { logger.Info("No changes detected for CAPI infra machine template") return nil }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
pkg/controllers/machinesetsync/machineset_sync_controller.go(11 hunks)pkg/conversion/mapi2capi/aws.go(2 hunks)pkg/conversion/mapi2capi/util.go(1 hunks)pkg/util/diff.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/conversion/mapi2capi/util.go
🔇 Additional comments (3)
pkg/conversion/mapi2capi/aws.go (1)
440-445: LGTM! Deterministic tag sorting.Sorting tags by name after unmarshalling ensures consistent ordering when converting between MAPI slice and CAPI map representations, reducing non-deterministic diffs. This pairs well with the new platform-aware differ framework.
pkg/controllers/machinesetsync/machineset_sync_controller.go (2)
787-790: LGTM! Improved error handling.Now properly capturing and propagating errors from
compareCAPIMachineSets, preventing silent comparison failures. The error wrapping provides clear context for debugging.
1383-1399: LGTM! Platform-aware MAPI diff implementation.The implementation correctly:
- Accepts platform for platform-specific providerSpec handling
- Uses
WithProviderSpecto extract and diff providerSpec separately (enablingHasProviderSpecChanges()checks)- Ignores status fields that are managed independently (observedGeneration, authoritativeAPI, synchronizedGeneration)
- Properly wraps and returns errors
This aligns well with the broader refactoring to generalized, platform-aware diffing.
|
@chrischdi: This pull request references OCPCLOUD-3172 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@chrischdi: This pull request references OCPCLOUD-3172 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
pkg/controllers/machinesetsync/machineset_sync_controller_unit_test.go(1 hunks)pkg/util/diff.go(1 hunks)pkg/util/diff_test.go(1 hunks)pkg/util/suite_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/util/diff.go
🔇 Additional comments (2)
pkg/util/diff_test.go (1)
24-156: LGTM!The test suite is comprehensive and well-structured. It covers empty objects, field operations (add/remove/change), nested changes, and list mutations. The test assertions validate both change detection and diff string formatting.
pkg/controllers/machinesetsync/machineset_sync_controller_unit_test.go (1)
30-330: LGTM!The test suite comprehensively covers MachineSet status comparison scenarios, including replica counts, v1beta1/v1beta2 conditions, and proper handling of LastTransitionTime changes (which should be ignored). The test structure is clean and assertions are appropriate.
|
@chrischdi: This pull request references OCPCLOUD-3172 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@chrischdi: This pull request references OCPCLOUD-3172 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@coderabbitai full-review |
|
@chrischdi: I'll perform a complete review of all the changes in this PR. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/util/diff.go (1)
140-159: Remove or populate the unused variable.The
additionalIgnoredPathsvariable declared on line 140 is never populated and serves no purpose. It's appended on line 156 but has no effect since it remains an empty slice throughout the function.Apply this diff to remove the dead code:
- 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. 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 %s on b: %w", funcName, err) } } // 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...) }Alternatively, if this variable is intended to be populated by the modify functions in the future, add a comment explaining its purpose.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
pkg/util/diff.go(1 hunks)
🔇 Additional comments (8)
pkg/util/diff.go (8)
1-43: LGTM!The interface design is clean and the error variable naming is correct.
45-84: LGTM!The Has*Changes methods correctly implement the DiffResult interface with appropriate guards.
86-111: LGTM!The String method provides deterministic, formatted output. The choice to simplify the output format (removing map/slice distinction) was already discussed in past reviews and is acceptable for debugging purposes.
113-139: LGTM!The object conversion is now correct, and error messages accurately reference the objects being converted. Previous issues have been properly addressed.
195-219: LGTM!The default differ configuration appropriately ignores standard Kubernetes metadata fields and condition timestamps. The special handling for Cluster API's conversion-data annotation is a good addition.
221-226: LGTM!The WithIgnoreField option provides a clean, flexible API for customizing ignored paths.
228-262: LGTM!The condition lastTransitionTime handling appropriately covers multiple API version paths. The use of "ignored" as a sentinel value is pragmatic for the diffing use case.
295-306: LGTM!The functional options pattern is correctly implemented with a clean, extensible design.
|
@chrischdi: This pull request references OCPCLOUD-3172 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@chrischdi: This pull request references OCPCLOUD-3172 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
chrischdi
force-pushed
the
pr-sync-refactor-how-to-diff
branch
from
7ac1306 to
115e11e
Compare
|
@chrischdi: This pull request references OCPCLOUD-3172 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controllers/machinesetsync/machineset_sync_controller.go (1)
1335-1343: Fix incorrect error constant for OpenStack case.Line 1337 returns
errAssertingCAPIIBMPowerVSMachineTemplatewhen asserting an OpenStackMachineTemplate fails, but should returnerrAssertingCAPIOpenStackMachineTemplate(which is defined at Line 81).Apply this diff:
case configv1.OpenStackPlatformType: typedInfraMachineTemplate1, ok := infraMachineTemplate1.(*openstackv1.OpenStackMachineTemplate) if !ok { - return nil, errAssertingCAPIIBMPowerVSMachineTemplate + return nil, errAssertingCAPIOpenStackMachineTemplate } typedinfraMachineTemplate2, ok := infraMachineTemplate2.(*openstackv1.OpenStackMachineTemplate) if !ok { return nil, errAssertingCAPIOpenStackMachineTemplate }
🧹 Nitpick comments (1)
pkg/util/diff.go (1)
143-143: Unused variableadditionalIgnoredPaths.The variable
additionalIgnoredPathsis declared but never populated, making it effectively dead code. The append at Line 159 will always append an empty slice.If this variable is reserved for future use, consider adding a comment explaining its purpose. Otherwise, remove it:
- 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. 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 %s on b: %w", funcName, err) } } // 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...) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
pkg/controllers/machinesetsync/machineset_sync_controller.go(11 hunks)pkg/util/diff.go(1 hunks)pkg/util/suite_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/util/suite_test.go
|
@chrischdi: This pull request references OCPCLOUD-3172 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
| clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" | ||
| ) | ||
|
|
||
| var _ = Describe("Unit tests for CAPIMachineSetStatusEqual", func() { |
There was a problem hiding this comment.
❤️ woop unit tests :D
|
@chrischdi: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
7 participants
Summary by CodeRabbit