OCPCLOUD-2998: implement synchronizedAPI #376
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-merge-robot
added
the
needs-rebase
📝 WalkthroughWalkthroughThis PR introduces comprehensive Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 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 ignored due to path filters (73)
📒 Files selected for processing (17)
🚧 Files skipped from review as they are similar to previous changes (5)
🧰 Additional context used🧬 Code graph analysis (7)e2e/machine_migration_helpers.go (1)
pkg/controllers/machinesetmigration/machineset_migration_controller_test.go (2)
pkg/controllers/machinesetsync/machineset_sync_controller.go (1)
e2e/machine_migration_mapi_authoritative_test.go (2)
pkg/controllers/machinemigration/machine_migration_controller.go (1)
pkg/controllers/synccommon/migratestatus.go (1)
pkg/controllers/machinesetmigration/machineset_migration_controller.go (1)
🔇 Additional comments (30)
✏️ Tip: You can disable this entire section by setting 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 |
RadekManak
force-pushed
the
synchronizedAPI
branch
from
57b06ba to
6bcba96
Compare
openshift-merge-robot
removed
the
needs-rebase
damdo
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@RadekManak: This pull request references OCPCLOUD-2998 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. DetailsIn 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. |
RadekManak
force-pushed
the
synchronizedAPI
branch
from
6bcba96 to
4af6d4e
Compare
|
@RadekManak: This pull request references OCPCLOUD-2998 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. DetailsIn 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-merge-robot
added
the
needs-rebase
RadekManak
force-pushed
the
synchronizedAPI
branch
from
4af6d4e to
f00ae34
Compare
openshift-merge-robot
removed
the
needs-rebase
RadekManak
force-pushed
the
synchronizedAPI
branch
from
f00ae34 to
7488790
Compare
|
@RadekManak: This pull request references OCPCLOUD-2998 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. DetailsIn 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. |
RadekManak
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@RadekManak: This pull request references OCPCLOUD-2998 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. DetailsIn 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
e2e/machine_migration_mapi_authoritative_test.go (1)
207-287: Excellent rollback test coverage!The new "Machine Migration Rollback Tests" comprehensively test the cancellation workflow:
- Initial state verification
- Rollback from Migrating state back to MachineAPI
- Successful migration after a previous rollback
- Cleanup verification
One structural note: This
Describeblock is nested inside the "Machine Migration Round Trip Tests"Describe(line 136). While Ginkgo supports this, it may be cleaner to place this as a siblingDescribeblock rather than a child, for better test organization. However, this is a minor style preference and doesn't affect test execution.Consider moving this
Describeblock to be a sibling of "Machine Migration Round Trip Tests" rather than nested inside it:- var _ = Describe("Machine Migration Round Trip Tests", Ordered, func() { - // ... existing round trip tests ... - - var _ = Describe("Machine Migration Rollback Tests", Ordered, func() { + var _ = Describe("Machine Migration Round Trip Tests", Ordered, func() { + // ... existing round trip tests ... + }) + + var _ = Describe("Machine Migration Rollback Tests", Ordered, func() {pkg/controllers/synccommon/migratestatus.go (1)
129-142: Consider handling unknown authority values explicitly.The function correctly maps
MachineAPIandClusterAPIto their synchronized counterparts and returns empty string forMigrating. However, the default case silently returns empty string for any unknown values.Consider whether logging a warning for unknown values would aid debugging, or if the empty string fallback is intentional for forward compatibility.
case mapiv1beta1.MachineAuthorityMigrating: return "" default: + // Unknown authority values return empty string. + // This provides forward compatibility if new authority values are added. return "" }pkg/controllers/machinemigration/machine_migration_controller.go (1)
230-277: Consider extracting shared initialization logic to reduce duplication.The
handleMigrationStatusInitializationfunction is identical to the one inMachineSetMigrationReconciler. While the duplication is understandable given the different resource types and apply configurations, consider whether a shared helper could be created insynccommonpackage using generics, similar to the existingApplyMigrationStatuspattern.This is a minor refactor suggestion for future maintainability - not blocking.
📜 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 ignored due to path filters (83)
e2e/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumvendor/github.com/openshift/api/config/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_insights.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_node.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_scheduling.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-Hypershift.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/console/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/console/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_awsprovider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_machine.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_machineset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machines-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machines-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machines-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machinesets-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machinesets-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machinesets-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/openapi/generated_openapi/zz_generated.openapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/awsplatformstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/azureplatformstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/baremetalplatformstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/custom.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/gatherconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/gathererconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/gatherers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/gcpplatformstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/gcpserviceendpoint.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/insightsdatagather.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/insightsdatagatherspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/nutanixplatformstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/openstackplatformstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/ovirtplatformstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/persistentvolumeclaimreference.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/persistentvolumeconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/storage.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/vsphereplatformstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/config_client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/generated_expansion.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/insightsdatagather.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/informers/externalversions/config/v1/insightsdatagather.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/informers/externalversions/config/v1/interface.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/informers/externalversions/generic.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/listers/config/v1/expansion_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/listers/config/v1/insightsdatagather.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1beta1/machinesetstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1beta1/machinestatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/ingresscontrollerspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/ingresscontrollertuningoptions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/cluster-api-actuator-pkg/testutils/resourcebuilder/machine/v1beta1/machine.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/cluster-api-actuator-pkg/testutils/resourcebuilder/machine/v1beta1/machineset.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (19)
e2e/go.mod(1 hunks)e2e/machine_migration_capi_authoritative_test.go(3 hunks)e2e/machine_migration_helpers.go(2 hunks)e2e/machine_migration_mapi_authoritative_test.go(4 hunks)e2e/machineset_migration_capi_authoritative_test.go(2 hunks)e2e/machineset_migration_helpers.go(1 hunks)e2e/machineset_migration_mapi_authoritative_test.go(3 hunks)go.mod(2 hunks)pkg/controllers/machinemigration/machine_migration_controller.go(4 hunks)pkg/controllers/machinemigration/machine_migration_controller_test.go(16 hunks)pkg/controllers/machinesetmigration/machineset_migration_controller.go(4 hunks)pkg/controllers/machinesetmigration/machineset_migration_controller_test.go(16 hunks)pkg/controllers/machinesetsync/machineset_sync_controller.go(1 hunks)pkg/controllers/machinesync/machine_sync_controller.go(1 hunks)pkg/controllers/synccommon/applyconfiguration.go(1 hunks)pkg/controllers/synccommon/migratestatus.go(2 hunks)pkg/controllers/synccommon/migratestatus_test.go(1 hunks)pkg/controllers/synccommon/suite_test.go(1 hunks)pkg/conversion/test/fuzz/fuzz.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
pkg/controllers/synccommon/migratestatus_test.go (1)
pkg/controllers/synccommon/migratestatus.go (1)
IsMigrationCancellationRequested(121-127)
pkg/controllers/machinesetmigration/machineset_migration_controller_test.go (2)
e2e/migration_common.go (1)
SynchronizedCondition(10-10)pkg/controllers/common_consts.go (1)
SynchronizedCondition(41-41)
e2e/machine_migration_mapi_authoritative_test.go (3)
pkg/conversion/mapi2capi/interface.go (1)
Machine(24-26)e2e/framework/machine.go (2)
GetMachine(75-86)DeleteMachines(89-119)e2e/framework/framework.go (1)
CAPINamespace(14-14)
pkg/controllers/machinemigration/machine_migration_controller.go (1)
pkg/controllers/synccommon/migratestatus.go (4)
IsMigrationCancellationRequested(121-127)AuthoritativeAPIToSynchronizedAPI(131-142)ApplyMigrationStatus(63-79)ApplyMigrationStatusAndResetSyncStatus(42-60)
e2e/machine_migration_helpers.go (1)
e2e/framework/framework.go (2)
WaitMedium(24-24)RetryMedium(18-18)
pkg/controllers/machinesetmigration/machineset_migration_controller.go (2)
pkg/controllers/synccommon/migratestatus.go (4)
IsMigrationCancellationRequested(121-127)ApplyMigrationStatus(63-79)AuthoritativeAPIToSynchronizedAPI(131-142)ApplyMigrationStatusAndResetSyncStatus(42-60)pkg/conversion/mapi2capi/interface.go (1)
MachineSet(29-31)
e2e/machineset_migration_helpers.go (2)
pkg/conversion/mapi2capi/interface.go (1)
MachineSet(29-31)e2e/framework/framework.go (2)
WaitMedium(24-24)RetryMedium(18-18)
🔇 Additional comments (58)
pkg/conversion/test/fuzz/fuzz.go (2)
736-736: LGTM! Correct handling of MAPI-only field in roundtrip testing.The change properly clears the
SynchronizedAPIfield during fuzzing to ensure roundtrip conversion tests pass, since this field has no CAPI equivalent and would be lost during MAPI→CAPI→MAPI conversion. The implementation follows the established pattern for other MAPI-only fields likeAuthoritativeAPIandSynchronizedGeneration.
783-783: LGTM! Consistent handling across MachineSet fuzzing.The change correctly clears the
SynchronizedAPIfield for MachineSet status, mirroring the implementation for Machine status (line 736). This ensures consistent behavior across both resource types during roundtrip conversion testing.e2e/go.mod (1)
19-19: LGTM! Dependency version aligned with root module.The openshift/api version is correctly aligned with the root go.mod (line 26), ensuring consistency across the e2e test module and main module.
pkg/controllers/synccommon/applyconfiguration.go (1)
44-44: LGTM! Interface extension follows established pattern.The new
WithSynchronizedAPImethod follows the same design pattern as the existing interface methods (WithConditions,WithSynchronizedGeneration,WithAuthoritativeAPI), maintaining consistency in the fluent API design.pkg/controllers/machinesync/machine_sync_controller.go (1)
1547-1547: LGTM! Proper status field preservation.The SynchronizedAPI field is correctly preserved from the existing machine status, following the same pattern as AuthoritativeAPI (line 1545) and SynchronizedGeneration (line 1546). This ensures the synchronization state is maintained during status updates.
pkg/controllers/synccommon/migratestatus_test.go (1)
26-74: LGTM! Comprehensive test coverage for migration cancellation logic.The test table covers all the key scenarios for detecting migration cancellation:
- Both directions of cancellation (ClusterAPI → MachineAPI and vice versa)
- In-progress migrations that should NOT trigger cancellation
- Pre-migration states that should NOT trigger cancellation
The test structure follows Ginkgo best practices with descriptive entry names and clear expectations.
pkg/controllers/synccommon/suite_test.go (1)
26-29: LGTM! Standard test suite setup.The test suite follows the standard Ginkgo/Gomega pattern for test registration, correctly setting up the fail handler and naming the suite "SyncCommon Suite".
e2e/machineset_migration_helpers.go (1)
221-228: LGTM! Helper function follows established patterns.The new
verifyMachineSetSynchronizedAPIhelper is well-designed:
- Mirrors the structure of
verifyMachineSetAuthoritative(lines 95-101)- Uses appropriate timeouts (WaitMedium, RetryMedium) consistent with similar assertions in the file
- Provides clear assertion messages for test failures
e2e/machineset_migration_capi_authoritative_test.go (2)
169-169: LGTM! Appropriate synchronization verification after authority switch.The addition of
verifyMachineSetSynchronizedAPIcorrectly validates that after switching the MachineSet authority to MachineAPI (line 165), the synchronization status reflectsMachineAPISynchronized. This aligns with the PR's objective of tracking the last successfully synchronized API.
208-208: LGTM! Complete test coverage for bidirectional migration.The verification at line 208 properly validates the synchronization state after switching back to ClusterAPI authority (line 204), ensuring
ClusterAPISynchronizedis set. Together with the verification at line 169, this provides complete coverage for both migration directions.go.mod (1)
5-11: Ensure temporary replace directives are tracked for removal.The TODO comment and temporary replace directives in go.mod (lines 5-11) reference changes that should be reverted when companion PRs are merged. Verify that removal of these replacements is tracked via linked GitHub issues, PR dependencies, or a dedicated tracking mechanism beyond the inline TODO comment, to ensure they don't persist longer than necessary. Consider updating the TODO with specific issue numbers or PR links if not already linked.
e2e/machine_migration_helpers.go (2)
163-171: LGTM! Well-structured helper function.The
verifyMachineMigratinghelper correctly usesSatisfyAllto verify both the Migrating state and the expected SynchronizedAPI value in a single assertion, following the established patterns in this file.
337-345: LGTM! Consistent implementation.The
verifyMachineSynchronizedAPIhelper follows the same pattern asverifyMachineAuthoritativeand other verification helpers in the file, usingEventuallywithkomega.Objectand appropriate timeouts.e2e/machine_migration_capi_authoritative_test.go (3)
201-201: LGTM! Appropriate verification placement.Adding
verifyMachineSynchronizedAPIafterverifyMachineSynchronizedGenerationprovides complete coverage of the synchronization state, ensuring both generation and API are correctly tracked.
213-213: LGTM!Correctly verifies
MachineAPISynchronizedafter switching authority to MachineAPI.
225-225: LGTM!Correctly verifies
ClusterAPISynchronizedafter switching back to ClusterAPI.pkg/controllers/machinesetsync/machineset_sync_controller.go (1)
1101-1104: LGTM! Correct status field preservation.Preserving
SynchronizedAPIalongsideSynchronizedGenerationandAuthoritativeAPIensures consistency during CAPI-to-MAPI synchronization, with these fields being managed separately viaapplySynchronizedConditionWithPatch.e2e/machineset_migration_mapi_authoritative_test.go (3)
169-170: LGTM! Comprehensive verification added.Adding both
verifyMachineSetAuthoritativeandverifyMachineSetSynchronizedAPIensures complete state verification after the authority switch.
209-210: LGTM!Correctly verifies MachineAPI authority and
MachineAPISynchronizedafter switching back.
338-339: LGTM!Consistent verification pattern for the update context after switching to ClusterAPI.
pkg/controllers/machinemigration/machine_migration_controller_test.go (5)
20-20: LGTM! Good addition for debugging.Adding
cmppackage enables clearer diff output in test failure messages, improving debugging experience.
220-226: Good improvement to test assertions.Using
cmp.Diffin the failure message provides detailed comparison output when the test fails, making it easier to identify unexpected changes to the machine object.
777-903: Comprehensive migration cancellation test coverage.The three cancellation contexts cover the essential scenarios:
- Cancelling back to MachineAPI
- Cancelling back to ClusterAPI
- Cancelling back to ClusterAPI with paused CAPI resources (verifying unpause behavior)
The tests correctly simulate stuck migration states and verify proper status transitions.
905-937: Good edge case coverage for status initialization.This test verifies the
handleMigrationStatusInitializationlogic whereSynchronizedAPIis empty butAuthoritativeAPIis set, ensuring backward compatibility and proper bootstrapping.
939-977: Important test for SynchronizedAPI preservation.This test verifies that when transitioning from a stable state (ClusterAPI) to Migrating, the
SynchronizedAPIis preserved to enable rollback detection. This is critical for the cancellation mechanism to work correctly.e2e/machine_migration_mapi_authoritative_test.go (3)
166-166: LGTM!Correctly verifies
MachineAPISynchronizedafter initial synchronization.
178-178: LGTM!Correctly verifies
ClusterAPISynchronizedafter switching to ClusterAPI.
190-190: LGTM!Correctly verifies
MachineAPISynchronizedafter switching back to MachineAPI.pkg/controllers/machinesetmigration/machineset_migration_controller_test.go (17)
200-211: LGTM - Test setup properly initializes SynchronizedAPI.The test correctly sets both
AuthoritativeAPIandSynchronizedAPItoMachineAPIin the status, matching the expected synchronized state for this scenario.
247-249: LGTM - SynchronizedAPI initialization added to migration request test.Correctly sets
SynchronizedAPItoMachineAPISynchronizedwhen starting a migration from MachineAPI to ClusterAPI.
288-290: LGTM - Proper SynchronizedAPI tracking during MachineAPI→ClusterAPI migration.Test correctly maintains
MachineAPISynchronizedas the synchronized state while inMigratingstatus.
323-330: LGTM - ClusterAPI→MachineAPI migration uses correct synchronized state.Test properly uses
WithSynchronizedAPIStatus(mapiv1beta1.ClusterAPISynchronized)and sets it on the status, reflecting that ClusterAPI was the last synchronized source.
374-391: LGTM - Synchronized state properly tracked during pausing phase.Test correctly sets
SynchronizedAPItoMachineAPISynchronizedwhen testing the pausing flow during MachineAPI→ClusterAPI migration.
430-438: LGTM - ClusterAPI→MachineAPI pause flow correctly sets synchronized state.Test properly uses
ClusterAPISynchronizedas the synchronized state when migrating from ClusterAPI to MachineAPI.
496-506: LGTM - Synchronized state properly set for generation mismatch test.Test correctly uses
MachineAPISynchronizedwhen testing the scenario wheresynchronizedGenerationdoesn't match.
537-547: LGTM - ClusterAPI generation mismatch test properly configured.Test correctly sets
ClusterAPISynchronizedfor the ClusterAPI→MachineAPI generation mismatch scenario.
579-597: LGTM - Complete migration prerequisites test properly configured.Test correctly sets
MachineAPISynchronizedfor the MachineAPI→ClusterAPI completion scenario with all prerequisites satisfied.
630-634: Verify assertion uses correct SynchronizedAPI value after migration completion.The test asserts
SynchronizedAPIequalsClusterAPISynchronizedafter migration completes from MachineAPI to ClusterAPI. This is correct since the new authority (ClusterAPI) should also be the new synchronized state.
660-678: LGTM - ClusterAPI→MachineAPI completion test properly configured.Test correctly sets
ClusterAPISynchronizedas the starting synchronized state for migration from ClusterAPI to MachineAPI.
702-707: LGTM - Correct assertion for ClusterAPI→MachineAPI migration completion.Test properly asserts
MachineAPISynchronizedas the final synchronized state after completing migration to MachineAPI.
711-742: LGTM - Migration cancellation back to MachineAPI test is well-structured.The test correctly simulates a stuck migration (status
Migrating, synchronized toMachineAPI) and verifies that whenspec.AuthoritativeAPImatches the synchronized state, the controller detects cancellation and transitions back.
745-776: LGTM - Migration cancellation back to ClusterAPI test is comprehensive.Test properly verifies rollback to ClusterAPI when
spec.AuthoritativeAPIequalsClusterAPIandSynchronizedAPIisClusterAPISynchronized.
778-820: LGTM - Migration cancellation with paused CAPI resource is well-tested.Test verifies that when cancelling back to ClusterAPI, the paused CAPI resource gets unpaused. This is important for ensuring the rollback target becomes operational.
823-853: LGTM - Empty SynchronizedAPI initialization test covers important edge case.Test verifies that when
AuthoritativeAPIis set butSynchronizedAPIis empty, the reconciler initializesSynchronizedAPIfromAuthoritativeAPI. This handles upgrade scenarios from older versions.
855-891: LGTM - Transition to Migrating preserves SynchronizedAPI correctly.Test verifies that when transitioning from a stable state (ClusterAPI) to
Migrating, theSynchronizedAPIis preserved asClusterAPISynchronized. This is essential for enabling future cancellation/rollback.pkg/controllers/synccommon/migratestatus.go (4)
33-60: LGTM - ApplyMigrationStatusAndResetSyncStatus correctly extended to include SynchronizedAPI.The function now atomically sets both
AuthoritativeAPIandSynchronizedAPIwhile resetting sync status. The call tostatusAC.WithSynchronizedAPI(synchronizedAPI)at line 57 ensures the synchronized state is properly included in the patch.
62-79: LGTM - ApplyMigrationStatus correctly extended to include SynchronizedAPI.The function now properly sets both
AuthoritativeAPIandSynchronizedAPIin a single patch operation, ensuring atomicity of the status update.
100-108: LGTM - Comments clarify field ownership management.The updated comments correctly explain the field ownership semantics and the validation rule requiring
synchronizedGenerationreset when changingauthoritativeAPI.
116-127: LGTM - IsMigrationCancellationRequested correctly detects rollback intent.The function properly detects when a user wants to cancel a migration by checking:
- Status is
Migrating- Spec's AuthoritativeAPI (converted to SynchronizedAPI) matches the current SynchronizedAPI
This correctly identifies when
spec.authoritativeAPIhas been reverted to the last synchronized state.pkg/controllers/machinesetmigration/machineset_migration_controller.go (5)
130-158: LGTM - Migration status initialization and cancellation handling are well-implemented.The code correctly:
- Delegates to
handleMigrationStatusInitializationfor empty status fields- Detects migration cancellation using
IsMigrationCancellationRequested- Ensures the rollback target is unpaused before applying the status patch
- Provides clear logging for cancellation scenarios
178-182: LGTM - Transition to Migrating correctly captures the source as SynchronizedAPI.Before entering the
Migratingstate, the currentAuthoritativeAPIis converted toSynchronizedAPIand both are patched atomically. This ensures the source of truth is preserved for potential rollback.
215-222: LGTM - Migration completion correctly updates both AuthoritativeAPI and SynchronizedAPI.The new synchronized state is derived from the target authority, and both fields are updated atomically while resetting the sync status. The enhanced logging provides useful debugging information.
228-275: Consider potential edge case in SynchronizedAPI inference logic.The
handleMigrationStatusInitializationfunction handles three scenarios well. However, in the third case (lines 251-271), whenSynchronizedAPIis empty butAuthoritativeAPIis set:The logic assumes we're in a forward migration and infers the source from the opposite of spec. This could be incorrect if the controller restarts mid-cancellation. However, given this is a transitional state for upgrades from older versions, the assumption is reasonable.
Consider adding a comment explaining this assumption:
if mapiMachineSet.Status.SynchronizedAPI == "" { // We are in a migration (Status.AuthoritativeAPI is Migrating) but we don't have SynchronizedAPI. // Assuming this is a standard forward migration (not a cancellation), the Spec tells us the Target. // Therefore, the Source (SynchronizedAPI) must be the opposite of the Spec. + // Note: This heuristic is for upgrade compatibility from older versions without SynchronizedAPI. + // In edge cases (e.g., controller restart mid-cancellation), this may not be perfectly accurate, + // but the worst case is a failed cancellation that requires re-initiating migration. targetAPI := mapiMachineSet.Spec.AuthoritativeAPI
423-431: LGTM - Wrapper functions improve code readability.The
applyMigrationStatusWithPatchandapplyMigrationStatusAndResetSyncStatusWithPatchwrappers encapsulate the generic function calls, making the reconciler code cleaner and more maintainable.pkg/controllers/machinemigration/machine_migration_controller.go (4)
130-157: LGTM - Migration status initialization and cancellation handling consistent with MachineSet controller.The implementation correctly mirrors the MachineSet controller logic:
- Delegates to
handleMigrationStatusInitialization- Detects cancellation with
IsMigrationCancellationRequested- Unpauses rollback target before patching
- Provides clear logging
177-181: LGTM - Transition to Migrating correctly captures synchronizedAPI.Identical pattern to MachineSet controller - captures current authority as synchronized state before transitioning to
Migrating.
217-224: LGTM - Migration completion correctly updates status fields.The implementation properly derives
newSynchronizedAPIfrom the target authority and updates both fields atomically with enhanced logging.
462-470: LGTM - Wrapper functions properly encapsulate status patching.The wrapper functions correctly use
MachineStatusApplyConfigurationtype parameter and call the sharedsynccommonfunctions.
RadekManak
force-pushed
the
synchronizedAPI
branch
from
7488790 to
1466b09
Compare
|
@RadekManak: This pull request references OCPCLOUD-2998 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.22.0" version, but no target version was set. DetailsIn 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
🤖 Fix all issues with AI agents
In @e2e/machine_migration_mapi_authoritative_test.go:
- Around line 208-215: There’s a duplicated/overlapping test block: the outer
Context("Machine Migration Rollback Tests", Ordered, func() { with vars
machineRollbackName, newMapiMachine, newCapiMachine is immediately followed by a
Describe("Machine Migration Rollback Tests", Ordered, func() { and a repeated
set of the same variable declarations; remove the structural duplication by
keeping only one block (either the Context or the Describe) and the single set
of declarations (machineRollbackName, newMapiMachine, newCapiMachine), or if
both are intended, properly close the Context before starting the Describe;
ensure only one declaration of those variables remains to fix the
compilation/runtime error.
In @pkg/controllers/machinemigration/machine_migration_controller_test.go:
- Around line 852-863: Replace the incorrect clusterv1beta1.PausedAnnotation
usages with clusterv1.PausedAnnotation in the machine_migration_controller_test
to match the v1beta2 CAPI Machine type; find occurrences of
clusterv1beta1.PausedAnnotation (used when building capiMachine and capaMachine
via capiMachineBuilder/capaMachineBuilder) and swap them to
clusterv1.PausedAnnotation so the test annotations match the controller and
other tests that use clusterv1.PausedAnnotation.
🧹 Nitpick comments (2)
pkg/controllers/synccommon/migratestatus_test.go (1)
26-74: Solid unit test coverage forIsMigrationCancellationRequested.The test table covers the key scenarios:
- ✓ Cancellation requests (spec reverted to match synchronized state while migrating)
- ✓ Normal migrations in progress (spec differs from synchronized state)
- ✓ Migration requests not yet acknowledged
Consider adding edge case entries for empty
SynchronizedAPIor whenspec.AuthoritativeAPIisMigrating(though schema validation prevents this).pkg/controllers/machinesetmigration/machineset_migration_controller.go (1)
149-153: Inconsistent method usage for status patching.Line 151 directly calls
synccommon.ApplyMigrationStatus[...]instead of using the wrapper methodr.applyMigrationStatusWithPatch. Other status patches in this file (e.g., lines 181, 236, 245, 266) use the wrapper methods.For consistency and maintainability, consider using the wrapper:
♻️ Suggested refactor
- if err := synccommon.ApplyMigrationStatus[*machinev1applyconfigs.MachineSetStatusApplyConfiguration](ctx, r.Client, controllerName, machinev1applyconfigs.MachineSet, mapiMachineSet, mapiMachineSet.Spec.AuthoritativeAPI, mapiMachineSet.Status.SynchronizedAPI); err != nil { + if err := r.applyMigrationStatusWithPatch(ctx, mapiMachineSet, mapiMachineSet.Spec.AuthoritativeAPI, mapiMachineSet.Status.SynchronizedAPI); err != 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 ignored due to path filters (17)
go.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumvendor/github.com/openshift/api/machine/v1beta1/types_machine.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_machineset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machines-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machines-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machines-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machinesets-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machinesets-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machinesets-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/openapi/generated_openapi/zz_generated.openapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1beta1/machinesetstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1beta1/machinestatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/cluster-api-actuator-pkg/testutils/resourcebuilder/machine/v1beta1/machine.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/cluster-api-actuator-pkg/testutils/resourcebuilder/machine/v1beta1/machineset.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (17)
e2e/machine_migration_capi_authoritative_test.goe2e/machine_migration_helpers.goe2e/machine_migration_mapi_authoritative_test.goe2e/machineset_migration_capi_authoritative_test.goe2e/machineset_migration_helpers.goe2e/machineset_migration_mapi_authoritative_test.gopkg/controllers/machinemigration/machine_migration_controller.gopkg/controllers/machinemigration/machine_migration_controller_test.gopkg/controllers/machinesetmigration/machineset_migration_controller.gopkg/controllers/machinesetmigration/machineset_migration_controller_test.gopkg/controllers/machinesetsync/machineset_sync_controller.gopkg/controllers/machinesync/machine_sync_controller.gopkg/controllers/synccommon/applyconfiguration.gopkg/controllers/synccommon/migratestatus.gopkg/controllers/synccommon/migratestatus_test.gopkg/controllers/synccommon/suite_test.gopkg/conversion/test/fuzz/fuzz.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/conversion/test/fuzz/fuzz.go
- e2e/machine_migration_capi_authoritative_test.go
- e2e/machine_migration_helpers.go
- pkg/controllers/machinesetsync/machineset_sync_controller.go
- pkg/controllers/machinesync/machine_sync_controller.go
🧰 Additional context used
🧬 Code graph analysis (6)
pkg/controllers/synccommon/migratestatus.go (2)
pkg/controllers/common_consts.go (1)
ReasonAuthoritativeAPIChanged(48-48)pkg/util/ssa.go (1)
ApplyConfigPatch(42-46)
pkg/controllers/synccommon/migratestatus_test.go (1)
pkg/controllers/synccommon/migratestatus.go (1)
IsMigrationCancellationRequested(121-127)
e2e/machineset_migration_helpers.go (2)
pkg/conversion/mapi2capi/interface.go (1)
MachineSet(29-31)e2e/framework/framework.go (2)
WaitMedium(24-24)RetryMedium(18-18)
pkg/controllers/machinesetmigration/machineset_migration_controller.go (2)
pkg/controllers/synccommon/migratestatus.go (4)
IsMigrationCancellationRequested(121-127)ApplyMigrationStatus(63-79)AuthoritativeAPIToSynchronizedAPI(131-142)ApplyMigrationStatusAndResetSyncStatus(42-60)pkg/conversion/mapi2capi/interface.go (1)
MachineSet(29-31)
pkg/controllers/machinesetmigration/machineset_migration_controller_test.go (2)
e2e/migration_common.go (1)
SynchronizedCondition(10-10)pkg/controllers/common_consts.go (1)
SynchronizedCondition(41-41)
e2e/machine_migration_mapi_authoritative_test.go (2)
e2e/framework/machine.go (2)
GetMachine(75-86)DeleteMachines(105-135)e2e/framework/framework.go (1)
CAPINamespace(14-14)
🔇 Additional comments (22)
pkg/controllers/synccommon/suite_test.go (1)
1-29: LGTM!Standard Ginkgo test suite scaffolding that enables the new
IsMigrationCancellationRequestedtests inmigratestatus_test.go.pkg/controllers/synccommon/applyconfiguration.go (1)
40-45: LGTM!The interface extension follows the existing pattern and enables the migration status helpers to set
SynchronizedAPIalongsideAuthoritativeAPIduring status patches.pkg/controllers/machinemigration/machine_migration_controller_test.go (2)
772-897: Comprehensive migration cancellation test coverage added.The new test contexts thoroughly cover:
- Cancellation back to MachineAPI from stuck migration
- Cancellation back to ClusterAPI from stuck migration
- Cancellation with paused CAPI resources (verifying unpause behavior)
The assertions correctly verify both
AuthoritativeAPIandSynchronizedAPIstatus fields after cancellation.
899-999: Status initialization edge cases well covered.The tests properly cover:
- Empty
SynchronizedAPIwith setAuthoritativeAPI(lines 899-931)- Empty
AuthoritativeAPIwith setSynchronizedAPI(lines 933-959)- Stable-to-Migrating transition preserving
SynchronizedAPI(lines 961-999)This aligns with the initialization logic in
handleMigrationStatusInitialization.pkg/controllers/synccommon/migratestatus.go (3)
116-127: Clean cancellation detection logic.The
IsMigrationCancellationRequestedfunction correctly identifies the cancellation scenario: whenstatus.AuthoritativeAPIisMigratingandspec.AuthoritativeAPImatches thestatus.SynchronizedAPI(via the conversion helper). This enables administrators to abort stuck migrations by reverting spec to the last known good state.
129-142: Mapping function handles all authority values correctly.The
AuthoritativeAPIToSynchronizedAPIhelper appropriately:
- Maps
MachineAPI→MachineAPISynchronized- Maps
ClusterAPI→ClusterAPISynchronized- Returns empty string for
Migratingand unknown values (safe default)
42-60: Updated status patching functions correctly propagateSynchronizedAPI.The renamed
ApplyMigrationStatusAndResetSyncStatusfunction now:
- Accepts the
synchronizedAPIparameter- Calls
WithSynchronizedAPIon the status apply configuration before patchingThis ensures atomic updates of both
AuthoritativeAPIandSynchronizedAPIduring migration state transitions.pkg/controllers/machinemigration/machine_migration_controller.go (4)
138-159: Migration cancellation flow is well-structured.The cancellation logic:
- Detects cancellation via
IsMigrationCancellationRequested- Ensures the rollback target is unpaused
- Applies status patch to transition back to synchronized state
- Logs success with both field values
This provides a clean escape hatch for stuck migrations.
464-472: Clean wrapper methods for status patching.The new
applyMigrationStatusWithPatchandapplyMigrationStatusAndResetSyncStatusWithPatchmethods properly encapsulate the generic function calls with controller-specific types, improving readability throughout the reconciler.
243-253: Implicit coupling based on string value assumptions.Line 248 casts
SynchronizedAPItoMachineAuthority:mapiv1beta1.MachineAuthority(mapiMachine.Status.SynchronizedAPI)This relies on
MachineAPISynchronizedandMachineAuthorityMachineAPIhaving identical underlying string values (and similarly for the ClusterAPI variants). While the forward conversion functionAuthoritativeAPIToSynchronizedAPIdocuments this mapping explicitly, the reverse conversion at line 248 uses a direct type cast without validation or a helper function. This creates implicit coupling to external API package constants that could silently break if their values diverge.Consider adding a reverse conversion helper (e.g.,
SynchronizedAPIToMachineAuthority) to make this dependency explicit and maintainable.
255-275: This concern is not valid—the scenario described cannot occur.The code handles an initialization edge case where
SynchronizedAPIis empty. However, wheneverstatus.AuthoritativeAPItransitions toMigrating, the controller explicitly setsSynchronizedAPIat the same time (line 186-189). All test cases confirm this invariant: whenstatus.AuthoritativeAPIisMigrating,status.SynchronizedAPIis always populated, never empty.Therefore, the third code block (which handles empty
SynchronizedAPI) cannot execute whenstatus.AuthoritativeAPIis alreadyMigrating. The inference logic is only used for recovery from inconsistent states and correctly reflects the actual last synchronized state.pkg/controllers/machinesetmigration/machineset_migration_controller.go (2)
229-276:handleMigrationStatusInitializationmirrors Machine controller logic.The initialization logic is identical to the Machine controller implementation, correctly handling:
- Both fields empty → initialize from spec
- Only
AuthoritativeAPIempty → derive fromSynchronizedAPI- Only
SynchronizedAPIempty → infer from migration directionThis duplication is acceptable given the different receiver types, though a shared implementation via generics could be considered in the future.
137-159: Migration cancellation flow correctly implemented for MachineSet.The cancellation handling follows the same pattern as the Machine controller, ensuring consistent behavior across both resource types.
e2e/machineset_migration_capi_authoritative_test.go (1)
169-169: LGTM! Synchronization state verification is correctly integrated.The additions of
verifyMachineSetSynchronizedAPIcalls after authority transitions properly validate that the synchronization tracking field reflects the expected state following MachineAPI ↔ ClusterAPI switches.Also applies to: 208-208
e2e/machineset_migration_helpers.go (1)
221-228: LGTM! Well-implemented verification helper.The new
verifyMachineSetSynchronizedAPIfunction follows the established pattern used by other verification helpers in this file, with appropriate Eventually assertions and descriptive logging.e2e/machineset_migration_mapi_authoritative_test.go (1)
169-170: LGTM! Consistent synchronization state verification.The additions properly verify both authoritative status and synchronization state after authority switches, ensuring comprehensive validation of migration state transitions.
Also applies to: 208-209, 339-340
e2e/machine_migration_mapi_authoritative_test.go (2)
166-166: LGTM! Synchronization state verification enhances round-trip test coverage.The additions of
verifyMachineSynchronizedAPIcalls throughout the round-trip test correctly validate that the synchronization state is tracked across MAPI → CAPI → MAPI transitions.Also applies to: 178-178, 190-190
217-291: Excellent rollback test coverage.The rollback test scenario comprehensively validates the migration cancellation workflow:
- Initiates migration from MAPI to ClusterAPI
- Cancels during Migrating state
- Verifies rollback to MachineAPI with preserved synchronization state
- Completes a subsequent successful migration to ClusterAPI
This provides valuable coverage for the administrator workflow described in the PR objectives.
pkg/controllers/machinesetmigration/machineset_migration_controller_test.go (4)
199-209: LGTM! Consistent SynchronizedAPI field integration across all test scenarios.The updates consistently set and verify
Status.SynchronizedAPIalongsideStatus.AuthoritativeAPIthroughout the test suite, ensuring proper tracking of the synchronized state in all migration scenarios.Also applies to: 246-246, 287-287, 322-329, 373-388, 429-436, 493-501, 534-542, 576-592, 627-627, 655-671, 697-697
704-814: Excellent migration cancellation test coverage.The new test contexts comprehensively validate migration cancellation scenarios:
- Cancellation back to MachineAPI from Migrating state
- Cancellation back to ClusterAPI from Migrating state
- Proper unpausing of target resources when cancelling
These tests ensure the cancellation detection logic (mentioned in PR objectives as
IsMigrationCancellationRequested()) works correctly in both directions.
816-874: Comprehensive status initialization test coverage.The initialization tests properly validate bootstrap logic for edge cases:
AuthoritativeAPIset butSynchronizedAPIemptySynchronizedAPIset butAuthoritativeAPIemptyThis coverage ensures the
handleMigrationStatusInitialization()function (referenced in PR description) handles partial state correctly.
876-912: Good coverage for SynchronizedAPI preservation during state transitions.The test validates that when transitioning from a stable state (ClusterAPI) to Migrating, the
SynchronizedAPIfield correctly preserves the source API, which is essential for the cancellation detection workflow.
pkg/controllers/machinemigration/machine_migration_controller_test.go
Outdated
Show resolved
Hide resolved
RadekManak
force-pushed
the
synchronizedAPI
branch
from
1466b09 to
f081583
Compare
|
@RadekManak: This pull request references OCPCLOUD-2998 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.22.0" version, but no target version was set. DetailsIn 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
🧹 Nitpick comments (2)
pkg/controllers/machinemigration/machine_migration_controller_test.go (1)
899-959: LGTM: Good coverage of status initialization scenarios.These tests handle important bootstrap cases where status fields are partially populated:
- Lines 899-931: Initialize
SynchronizedAPIwhen onlyAuthoritativeAPIis set- Lines 933-959: Derive
AuthoritativeAPIwhen onlySynchronizedAPIis setBoth scenarios correctly verify the controller fills in the missing field with the appropriate value.
💡 Optional: Consider verifying requeue behavior
The test descriptions mention "requeue" (line 922) but don't verify
result.Requeue. While this appears to be a consistent pattern in the file (e.g., line 183), explicitly asserting the requeue behavior could make the tests more precise:It("should initialize SynchronizedAPI from AuthoritativeAPI and requeue", func() { - _, err := reconciler.Reconcile(ctx, req) + result, err := reconciler.Reconcile(ctx, req) Expect(err).NotTo(HaveOccurred()) + Expect(result.Requeue).To(BeTrue(), "expected requeue after initialization") Eventually(k.Object(mapiMachine)).Should(SatisfyAll(However, if the existing pattern is intentional (e.g., the requeue is implicit or tested via
Eventually), this can be safely ignored.pkg/controllers/machinesetmigration/machineset_migration_controller_test.go (1)
816-874: Test coverage is good but could be extended.The tests verify initialization when one field is empty and can be derived from the other. However, consider adding test coverage for the more complex inference scenario in
handleMigrationStatusInitializationwhere:
spec.AuthoritativeAPI != status.AuthoritativeAPI(migration requested)status.AuthoritativeAPIis set butstatus.SynchronizedAPIis empty- The controller must infer the source API from the target
This would exercise the inference logic at lines 255-267 that I flagged in the controller review.
📝 Suggested additional test
Context("when migration is in progress but SynchronizedAPI is empty (inference scenario)", func() { BeforeEach(func() { By("Setting the MAPI machine set spec AuthoritativeAPI to ClusterAPI (migration target)") mapiMachineSet = mapiMachineSetBuilder. WithAuthoritativeAPI(mapiv1beta1.MachineAuthorityClusterAPI). Build() Eventually(k8sClient.Create(ctx, mapiMachineSet)).Should(Succeed()) By("Creating mirror CAPI machine set") capiMachineSet = capiMachineSetBuilder.Build() Eventually(k8sClient.Create(ctx, capiMachineSet)).Should(Succeed()) By("Setting AuthoritativeAPI to MachineAPI (source) but leaving SynchronizedAPI empty") Eventually(k.UpdateStatus(mapiMachineSet, func() { mapiMachineSet.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI // SynchronizedAPI intentionally left empty - should be inferred as opposite of target })).Should(Succeed()) req = reconcile.Request{NamespacedName: client.ObjectKeyFromObject(mapiMachineSet)} }) It("should infer SynchronizedAPI as the opposite of the migration target", func() { _, err := reconciler.Reconcile(ctx, req) Expect(err).NotTo(HaveOccurred()) Eventually(k.Object(mapiMachineSet)).Should(SatisfyAll( HaveField("Status.AuthoritativeAPI", Equal(mapiv1beta1.MachineAuthorityMachineAPI)), HaveField("Status.SynchronizedAPI", Equal(mapiv1beta1.MachineAPISynchronized)), )) }) })
📜 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 ignored due to path filters (17)
go.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumvendor/github.com/openshift/api/machine/v1beta1/types_machine.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_machineset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machines-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machines-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machines-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machinesets-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machinesets-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machinesets-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/openapi/generated_openapi/zz_generated.openapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1beta1/machinesetstatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machine/applyconfigurations/machine/v1beta1/machinestatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/cluster-api-actuator-pkg/testutils/resourcebuilder/machine/v1beta1/machine.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/cluster-api-actuator-pkg/testutils/resourcebuilder/machine/v1beta1/machineset.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (17)
e2e/machine_migration_capi_authoritative_test.goe2e/machine_migration_helpers.goe2e/machine_migration_mapi_authoritative_test.goe2e/machineset_migration_capi_authoritative_test.goe2e/machineset_migration_helpers.goe2e/machineset_migration_mapi_authoritative_test.gopkg/controllers/machinemigration/machine_migration_controller.gopkg/controllers/machinemigration/machine_migration_controller_test.gopkg/controllers/machinesetmigration/machineset_migration_controller.gopkg/controllers/machinesetmigration/machineset_migration_controller_test.gopkg/controllers/machinesetsync/machineset_sync_controller.gopkg/controllers/machinesync/machine_sync_controller.gopkg/controllers/synccommon/applyconfiguration.gopkg/controllers/synccommon/migratestatus.gopkg/controllers/synccommon/migratestatus_test.gopkg/controllers/synccommon/suite_test.gopkg/conversion/test/fuzz/fuzz.go
🚧 Files skipped from review as they are similar to previous changes (5)
- e2e/machine_migration_capi_authoritative_test.go
- e2e/machineset_migration_mapi_authoritative_test.go
- pkg/controllers/synccommon/suite_test.go
- pkg/controllers/machinesetsync/machineset_sync_controller.go
- pkg/controllers/synccommon/applyconfiguration.go
🧰 Additional context used
🧬 Code graph analysis (6)
e2e/machine_migration_helpers.go (1)
e2e/framework/framework.go (2)
WaitMedium(24-24)RetryMedium(18-18)
pkg/controllers/machinesetmigration/machineset_migration_controller_test.go (2)
e2e/migration_common.go (1)
SynchronizedCondition(10-10)pkg/controllers/common_consts.go (1)
SynchronizedCondition(41-41)
pkg/controllers/machinemigration/machine_migration_controller.go (1)
pkg/controllers/synccommon/migratestatus.go (4)
IsMigrationCancellationRequested(121-127)AuthoritativeAPIToSynchronizedAPI(131-142)ApplyMigrationStatus(63-79)ApplyMigrationStatusAndResetSyncStatus(42-60)
pkg/controllers/machinesetmigration/machineset_migration_controller.go (2)
pkg/controllers/synccommon/migratestatus.go (4)
IsMigrationCancellationRequested(121-127)ApplyMigrationStatus(63-79)AuthoritativeAPIToSynchronizedAPI(131-142)ApplyMigrationStatusAndResetSyncStatus(42-60)pkg/conversion/mapi2capi/interface.go (1)
MachineSet(29-31)
pkg/controllers/synccommon/migratestatus_test.go (1)
pkg/controllers/synccommon/migratestatus.go (1)
IsMigrationCancellationRequested(121-127)
e2e/machineset_migration_helpers.go (2)
pkg/conversion/mapi2capi/interface.go (1)
MachineSet(29-31)e2e/framework/framework.go (2)
WaitMedium(24-24)RetryMedium(18-18)
🔇 Additional comments (29)
pkg/controllers/machinesync/machine_sync_controller.go (1)
1515-1515: LGTM! SynchronizedAPI field properly preserved during synchronization.The added line correctly preserves the
SynchronizedAPIstatus field from the existing MAPI machine to the converted MAPI machine, following the same pattern as other preserved fields (AuthoritativeAPI,SynchronizedGeneration,LastOperation,ProviderStatus). This ensures the synchronization state is maintained during CAPI→MAPI machine status updates.pkg/controllers/machinemigration/machine_migration_controller_test.go (4)
20-20: LGTM: Helpful debugging aid.The
go-cmpimport is used at line 225 to provide detailed diff output when the resource-version assertion fails, which aids in debugging test failures.
201-770: LGTM: Comprehensive updates to existing migration tests.All existing migration scenarios have been systematically updated to handle the new
SynchronizedAPIstatus field:
- Test setup properly initializes
SynchronizedAPIalongsideAuthoritativeAPIusing the builder pattern- Assertions verify both fields in migration completion scenarios (lines 678, 765)
- The defensive use of
cmp.Diffat line 225 provides helpful debugging context when assertions fail- The pattern is consistent across all migration directions (MachineAPI ↔ ClusterAPI)
772-897: LGTM: Excellent coverage of migration cancellation scenarios.The new cancellation tests thoroughly exercise the rollback functionality:
- Lines 772-806: Cancel migration back to MachineAPI
- Lines 808-841: Cancel migration back to ClusterAPI
- Lines 843-896: Cancel to ClusterAPI with proper unpause of CAPI resources
Each scenario correctly:
- Simulates a stuck migration state (
status.AuthoritativeAPI: Migrating)- Sets
status.SynchronizedAPIto reflect the last good API- Reverts
spec.AuthoritativeAPIto trigger cancellation detection- Verifies the controller transitions back to the synchronized state
- Explicitly confirms no requeue is needed (lines 799, 834, 877)
The paused-resource scenario (lines 843-896) is particularly valuable, ensuring that CAPI resources are properly unpaused when rolling back.
961-999: LGTM: Important invariant verification for migration transitions.This test verifies a critical behavior: when transitioning from a stable state (ClusterAPI) to
Migrating, theSynchronizedAPIfield correctly preserves the last synchronized API rather than followingAuthoritativeAPItoMigrating.This preservation is essential for the cancellation detection logic, which relies on comparing
spec.AuthoritativeAPIwithstatus.SynchronizedAPIto detect rollback requests.pkg/conversion/test/fuzz/fuzz.go (1)
764-764: LGTM! Consistent handling of MAPI-only field in fuzz tests.The SynchronizedAPI field is correctly cleared during fuzzing since it has no CAPI equivalent, consistent with how other MAPI-only fields like AuthoritativeAPI are handled.
Also applies to: 811-811
e2e/machine_migration_helpers.go (2)
172-180: LGTM! Well-structured helper for migration state verification.The function correctly validates both the Migrating state and the expected SynchronizedAPI value in a single assertion, with clear error messaging.
347-354: LGTM! Clean and focused helper function.The function provides a reusable way to verify SynchronizedAPI status with proper timeout handling and descriptive messaging.
pkg/controllers/machinesetmigration/machineset_migration_controller.go (5)
138-159: LGTM! Migration cancellation logic is sound.The code correctly detects cancellation requests and ensures the rollback target is unpaused before transitioning. The call to
ensureUnpauseRequestedOnNewAuthoritativeResourcehandles cases where the migration was cancelled early, before all pausing completed.
179-183: LGTM! Correct synchronization state capture.The code properly captures the current authoritative API as the synchronized state before transitioning to Migrating, creating the necessary breadcrumb for potential rollback.
216-223: LGTM! Proper migration completion.The code correctly updates both
authoritativeAPIandsynchronizedAPIatomically to reflect the new stable state after successful migration, with appropriate sync status reset.
424-432: LGTM! Clean patch helper wrappers.The helper functions provide a clean interface for atomic status updates, properly delegating to the shared migration status utilities.
255-267: The proposed validation is ineffective and doesn't address the actual risk in the inference logic.The if/else structure (lines 258-262) guarantees that
synchronizedAPIwill always be set to one of two valid enum values (ClusterAPISynchronizedorMachineAPISynchronized), making the proposed validation redundant—it will never catch an error.The actual concern should be the input validation, not the output. If
spec.AuthoritativeAPI(targetAPI) isMachineAuthorityMigratingor some other unexpected value, the code silently defaults toMachineAPISynchronized, which may be incorrect. The inference logic only works safely when targetAPI is guaranteed to be eitherMachineAuthorityMachineAPIorMachineAuthorityClusterAPI.Consider validating that
targetAPImatches one of the two expected values before the inference:if targetAPI != mapiv1beta1.MachineAuthorityMachineAPI && targetAPI != mapiv1beta1.MachineAuthorityClusterAPI { return &ctrl.Result{}, fmt.Errorf("cannot infer synchronizedAPI from unexpected targetAPI: %s", targetAPI) }pkg/controllers/machinemigration/machine_migration_controller.go (2)
138-159: LGTM! Consistent migration controller implementation.The migration cancellation detection, synchronizedAPI capture, completion logic, and helper functions correctly mirror the MachineSet controller implementation with appropriate handling for Machine resources.
Also applies to: 179-183, 219-226, 464-472
255-267: Remove the proposed validation—it's redundant and adds no safety.The inference logic is intentional and correct. In a binary system with only two possible authority states (MachineAPI or ClusterAPI), knowing the target authority reveals the source. The if-else statement deterministically assigns one of the two valid
SynchronizedAPIconstants, so a runtime validation on the result can never fail. The function already guarantees valid values through its type constraints and logic.If there are concerns about the inference pattern itself (whether inverting target to infer source is always correct during migrations), that would require domain-specific review, but the proposed code does not address such concerns.
pkg/controllers/machinesetmigration/machineset_migration_controller_test.go (2)
704-814: LGTM! Comprehensive migration cancellation test coverage.The test contexts thoroughly exercise cancellation scenarios including:
- Cancellation back to both MachineAPI and ClusterAPI
- Verification that paused resources are properly unpaused during cancellation
- Proper state transitions during rollback
876-912: LGTM! Good coverage of Migrating state transition.The test verifies that when transitioning to Migrating state, the controller correctly preserves the
SynchronizedAPIas a snapshot of the source state, which is essential for potential migration cancellation.e2e/machineset_migration_capi_authoritative_test.go (2)
164-170: LGTM! Good addition of synchronization state verification.The
verifyMachineSetSynchronizedAPIcall appropriately verifies that the status reflectsMachineAPISynchronizedafter the authority switch to MachineAPI. This aligns well with the existing verification pattern for paused conditions and synchronized conditions.
203-209: LGTM! Consistent synchronization verification after authority switch.The verification correctly asserts that the MachineSet's
status.synchronizedAPIis set toClusterAPISynchronizedafter switching authority back to ClusterAPI, maintaining consistency with the earlier verification at Line 169.pkg/controllers/synccommon/migratestatus_test.go (1)
26-74: LGTM! Comprehensive test coverage for migration cancellation detection.The table-driven tests thoroughly cover all critical scenarios:
- Both directions of migration cancellation (MAPI→CAPI and CAPI→MAPI)
- In-progress migrations that should not be cancelled
- Pre-migration states
The test logic correctly validates the cancellation detection algorithm where cancellation is identified when
status.authoritativeAPIisMigratingand thespec.authoritativeAPImatchesstatus.synchronizedAPI.e2e/machineset_migration_helpers.go (2)
221-228: LGTM! Well-structured verification helper.The
verifyMachineSetSynchronizedAPIfunction follows the established pattern from other verification helpers likeverifyMachineSetAuthoritative(Lines 94-101), using appropriate timeouts (WaitMedium/RetryMedium) and clear assertion messages.
230-246: LGTM! Clean refactor using WithTransform pattern.The introduction of
getAWSProviderSpecFromMachineSetas an extraction helper and its use viaWithTransforminverifyMAPIMachineSetProviderSpec(Line 234) is an idiomatic Gomega pattern that improves testability and readability by separating extraction logic from assertion logic.e2e/machine_migration_mapi_authoritative_test.go (3)
27-27: LGTM! Non-functional test structure improvement.The change from
DescribetoContextblocks better organizes the test hierarchy and aligns with Ginkgo best practices for distinguishing between test suites (Describe) and test scenarios (Context).Also applies to: 65-65, 136-136
162-193: LGTM! Comprehensive synchronization state verification throughout the round trip.The additions of
verifyMachineSynchronizedAPIat Lines 166, 178, and 190 ensure that thestatus.synchronizedAPIfield correctly reflects the current synchronized state at each stage of the MAPI→CAPI→MAPI round trip. This provides valuable coverage for the synchronization tracking feature.
209-288: LGTM! Excellent test coverage for migration rollback scenario.This new test suite validates a critical user workflow described in the PR objectives: canceling a stuck migration by reverting
spec.authoritativeAPIto the last successfully synchronized state. The test comprehensively verifies:
- Entry into the
Migratingstate with propersynchronizedAPItracking (Line 245)- Cancellation detection and rollback when spec is reverted (Lines 247-262)
- Successful completion of a subsequent full migration after rollback (Lines 264-275)
- Proper cleanup of all resources (Lines 277-286)
The test flow matches the administrator workflow described in the PR objectives and provides strong validation of the cancellation mechanism.
pkg/controllers/synccommon/migratestatus.go (4)
42-60: LGTM! Clean extension to support atomic synchronizedAPI updates.The addition of the
synchronizedAPIparameter and the call toWithSynchronizedAPI(Line 57) enables atomic updates of bothauthoritativeAPIandsynchronizedAPIfields during migration state transitions, which is essential for consistent state management.
63-79: LGTM! Consistent pattern for synchronizedAPI propagation.The function signature extension and
WithSynchronizedAPIcall (Line 76) mirror the pattern inApplyMigrationStatusAndResetSyncStatus, maintaining consistency across migration status update paths.
116-127: LGTM! Correct cancellation detection logic.The
IsMigrationCancellationRequestedfunction correctly identifies when an administrator has revertedspec.authoritativeAPIto the last successfully synchronized state whilestatus.authoritativeAPIis stillMigrating. This implements the cancellation workflow described in the PR objectives.The logic is validated by the comprehensive test coverage in
migratestatus_test.go.
129-142: LGTM! Straightforward and correct authority-to-synchronization mapping.The
AuthoritativeAPIToSynchronizedAPIfunction provides a clean mapping fromMachineAuthoritytoSynchronizedAPIvalues. The explicit handling ofMachineAuthorityMigratingreturning an empty string (Line 138) is appropriate since "Migrating" is a transient state that doesn't represent a synchronized API.
|
/test unit |
|
/payload required |
|
@RadekManak: it appears that you have attempted to use some version of the payload command, but your comment was incorrectly formatted and cannot be acted upon. See the docs for usage info. |
|
/pipeline required |
|
Scheduling tests matching the |
|
/assign @mdbooth |
|
@RadekManak are the vendor and verify expected to fail for now? |
|
go: github.com/openshift/[email protected] (replaced by ../api): reading ../api/go.mod: open /go/src/github.com/openshift/api/go.mod: no such file or directory Yes, I switched to using a local project folders for development instead of pushing into my fork. I'll correct this when the API PR is merged. |
|
/testwith openshift/cluster-capi-operator/master/e2e-aws-capi-techpreview openshift/machine-api-operator#1442 |
|
@RadekManak, |
|
/testwith openshift/cluster-capi-operator/main/e2e-aws-capi-techpreview openshift/machine-api-operator#1442 |
mdbooth
left a comment
mdbooth
left a comment
There was a problem hiding this comment.
We just discussed this. Unfortunately I don't think this approach is correct.
The purpose of the SynchronizedAPI field is to eliminate the heuristic in isSynchronized which would fail if we were reverting an incomplete migration. Specifically, the SynchronizedAPI field defines deterministically exactly which object the value of SynchronizedGeneration refers to. Therefore SynchronizedAPI should be written by the sync controller whenever we write SynchronizedGeneration.
Additionally, we should ensure that each field is owned by a particular controller and is never touched by any other controller. Therefore the migration controller should not write to this value even to initialise it. It should just wait for the sync controller to initialise it if it is required.
With these changes in place, the additional code path in the migration controller guarded by IsMigrationCancellationRequested is not required. As long as we can unambiguously determine the synchronisation state in all circumstances, the existing logic is already correct. Therefore we should just need to update the logic in isSynchronized to use the SynchronizedAPI field and therefore no longer require the invalid heuristic.
RadekManak
force-pushed
the
synchronizedAPI
branch
from
f081583 to
d753c73
Compare
|
@RadekManak: This pull request references OCPCLOUD-2998 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.22.0" version, but no target version was set. DetailsIn 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. |
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@RadekManak: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/testwith openshift/cluster-capi-operator/main/e2e-aws-capi-techpreview openshift/machine-api-operator#1442 |
5 participants
Why:
Adds
.status.synchronizedAPIfield to Machine and MachineSet resources to enable reliable migration cancellation when migrations get stuck atstatus.authoritativeAPI: Migrating. Without this field, the system cannot determine which API was the migration source when users revertspec.authoritativeAPI, preventing proper rollback to the last known good state. Implements OCPCLOUD-2998.What:
.status.synchronizedAPIfield (values:""|MachineAPI|ClusterAPI) to track the last successfully synchronized APIhandleMigrationStatusInitialization()in migration controllers to bootstrap empty status fields with proper inference logicIsMigrationCancellationRequested()detection whenspec.authoritativeAPImatchesstatus.synchronizedAPIwhilestatus.authoritativeAPI == MigratingApplyMigrationStatus()helpers to atomically set bothauthoritativeAPIandsynchronizedAPIduring state transitionsHow can it be used:
Administrators can cancel stuck migrations by reverting
spec.authoritativeAPIback to the previously synchronized state:The migration controller detects this pattern and transitions back to the synchronized state without requiring manual intervention.
How did you test it:
Unit tests cover status initialization scenarios (both fields empty, only one empty, mid-migration inference), migration cancellation detection logic, and rollback flows.
Adds e2e tests to verify field behavior during migrations.
Notes for the reviewer:
Requires companion PR openshift/machine-api-operator#1442 for API definition vendoring. This PR description was generated with AI assistance.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.