OCPCLOUD-3017: e2e testing automation: Implement Status Syncing for MAPI/CAPI migration #387
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@sunzhaohua2: This pull request references OCPCLOUD-3017 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. |
📝 WalkthroughWalkthroughThis PR adds a comprehensive end-to-end test suite for validating status conversion between Machine API (MAPI) and Cluster API (CAPI) resources, including new helper functions and GinkgoHelper() invocations for proper test scoping across multiple migration test files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
Comment |
openshift-ci
bot
added
do-not-merge/hold
|
[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 |
|
@sunzhaohua2: This pull request references OCPCLOUD-3017 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. |
|
@sunzhaohua2: This pull request references OCPCLOUD-3017 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. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@sunzhaohua2: This pull request references OCPCLOUD-3017 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. |
![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
🧹 Nitpick comments (1)
e2e/machine_migration_helpers.go (1)
346-354: UsingExpectinside a transform function defeatsEventuallyretry semantics.When
getAWSProviderStatusFromMachineis used as a transform function withinEventually, anyExpectfailure will cause an immediate test failure instead of allowingEventuallyto retry. IfProviderStatus.Rawis nil or YAML unmarshalling fails on an early attempt (before status is populated), the test fails prematurely.Consider returning an error-indicating value that the matcher can handle, or restructure to allow retries.
🔎 Proposed fix
// getAWSProviderStatusFromMachine extracts and unmarshals the AWSMachineProviderStatus from a MAPI Machine. func getAWSProviderStatusFromMachine(mapiMachine *mapiv1beta1.Machine) *mapiv1beta1.AWSMachineProviderStatus { - Expect(mapiMachine.Status.ProviderStatus.Raw).ToNot(BeNil()) + if mapiMachine.Status.ProviderStatus == nil || mapiMachine.Status.ProviderStatus.Raw == nil { + return nil + } providerStatus := &mapiv1beta1.AWSMachineProviderStatus{} - Expect(yaml.Unmarshal(mapiMachine.Status.ProviderStatus.Raw, providerStatus)).To(Succeed()) + if err := yaml.Unmarshal(mapiMachine.Status.ProviderStatus.Raw, providerStatus); err != nil { + return nil + } return providerStatus }Then update the matcher usage to handle nil:
verifyMAPIMachineProviderStatus(mapiMachine, SatisfyAll( Not(BeNil()), HaveField("InstanceState", HaveValue(Equal(...))), ), )
📜 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 (2)
e2e/machine_migration_helpers.goe2e/status_conversion_test.go
🔇 Additional comments (12)
e2e/machine_migration_helpers.go (1)
22-22: LGTM!Import addition is appropriate for the new YAML unmarshalling functionality.
e2e/status_conversion_test.go (11)
1-21: LGTM!Imports are well-organized and appropriate for the comprehensive status conversion testing.
23-32: LGTM!Proper platform and feature gate guards ensure tests only run in appropriate environments.
34-57: LGTM!Well-structured test setup with proper cleanup via
DeferCleanup.
66-84: Consider potential stale reference issue.Line 75 compares against
mapiMachineSet.Status.ReadyReplicaswhich is captured when theEventuallyon line 68-71 completes. However, sincekomega.Object(mapiMachineSet)updates the object in place, this should work correctly. Just noting this pattern for awareness.
138-150: Good documentation.The comment clearly explains why only the Paused condition is verified for paused MachineSets.
461-491: Good practice: Pending tests with bug references.Using
PItwith explicit bug URLs (OCPBUGS-63340) is the right approach for known issues that are being tracked.
557-568: Address comparison assumes ordering.The index-based address comparison (
cm.Status.Addresses[i]vsmapiMachine.Status.Addresses[i]) will fail if addresses are returned in different orders between MAPI and CAPI. If ordering isn't guaranteed by the conversion logic, consider usingContainElementsor sorting before comparison.This is acceptable if the conversion preserves order, but worth verifying.
647-667: LGTM!Good coverage of providerStatus conversion including instanceState verification.
765-791: Pending test appropriately marked with bug reference.The
PItwith OCPBUGS-70136 reference correctly marks this as pending until the underlying bug is fixed.
777-790: Note: This test uses the helper function with theExpectissue.When this pending test is enabled, be aware that
verifyMAPIMachineProviderStatususesgetAWSProviderStatusFromMachinewhich hasExpectassertions inside a transform function. Applying the fix suggested inmachine_migration_helpers.gowill be needed.
805-816: Good use ofPItfor pending tests with bug tracking.Multiple tests appropriately marked as pending with OCPBUGS-63183 reference for the same-name scenario.
Also applies to: 832-842, 844-869
|
/test e2e-aws-capi-techpreview |
|
@sunzhaohua2: This pull request references OCPCLOUD-3017 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: 1
🧹 Nitpick comments (2)
e2e/status_conversion_test.go (2)
557-570: Consider order-independent address comparison.The current comparison assumes addresses are in the same order in both MAPI and CAPI. If address ordering is not guaranteed to be preserved during conversion, this could cause flaky test failures.
🔎 Suggested order-independent comparison
Eventually(komega.Object(capiMachine), capiframework.WaitMedium, capiframework.RetryMedium).Should( WithTransform(func(cm *clusterv1.Machine) bool { if len(cm.Status.Addresses) != len(mapiMachine.Status.Addresses) { return false } - for i, mapiAddr := range mapiMachine.Status.Addresses { - capiAddr := cm.Status.Addresses[i] - if string(capiAddr.Type) != string(mapiAddr.Type) || capiAddr.Address != mapiAddr.Address { - return false + for _, mapiAddr := range mapiMachine.Status.Addresses { + found := false + for _, capiAddr := range cm.Status.Addresses { + if string(capiAddr.Type) == string(mapiAddr.Type) && capiAddr.Address == mapiAddr.Address { + found = true + break + } + } + if !found { + return false } } return true }, BeTrue()),
736-749: Consider order-independent address comparison here as well.Same concern as the MAPI→CAPI address comparison - the index-based comparison assumes preserved ordering.
📜 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 (2)
e2e/machine_migration_helpers.goe2e/status_conversion_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/machine_migration_helpers.go
🧰 Additional context used
🧬 Code graph analysis (1)
e2e/status_conversion_test.go (5)
e2e/framework/util.go (1)
IsMachineAPIMigrationEnabled(41-76)pkg/conversion/mapi2capi/interface.go (2)
MachineSet(29-31)Machine(24-26)e2e/framework/framework.go (7)
WaitLong(25-25)RetryLong(19-19)WaitMedium(24-24)RetryMedium(18-18)WaitShort(23-23)RetryShort(17-17)CAPINamespace(14-14)e2e/framework/machine.go (2)
GetMachine(75-86)GetAWSMachine(61-72)e2e/framework/machineset.go (3)
WaitForMachineSet(137-173)ScaleCAPIMachineSet(224-235)GetNewestMachineFromMachineSet(204-221)
🔇 Additional comments (8)
e2e/status_conversion_test.go (8)
1-21: LGTM!Imports are well-organized and appropriate for the e2e test suite. The use of dot imports for Ginkgo/Gomega is a common pattern in Kubernetes e2e tests.
23-32: LGTM!Good use of skip conditions to ensure tests only run on AWS platforms with the MachineAPIMigration feature enabled. The
Ordereddecorator ensures deterministic test execution.
66-84: LGTM!The pattern of waiting for MAPI status to stabilize and then verifying CAPI synchronization is correct. The
komega.Object()matcher refreshes the object on each poll, ensuring fresh values are used.
205-230: LGTM!The error status conversion test correctly handles the case where either
ErrorReasonorErrorMessage(or both) may be set. The conditional assertions ensure proper matching without failing if only one field is populated.
461-490: LGTM on pending tests with bug tracking.Good practice to mark tests as pending (
PIt) with inline bug references (OCPBUGS-63340). This maintains test coverage visibility while avoiding false negatives until the underlying issues are resolved.
794-803: LGTM!The cross-authority scenario (CAPI Machine created first, then MAPI with ClusterAPI authority) is properly set up. Individual test cases appropriately wait for the required status fields to be populated.
34-246: Well-structured test suite for MAPI→CAPI MachineSet conversions.The test organization is clean with:
- Proper resource lifecycle management (BeforeAll setup, DeferCleanup teardown)
- Clear separation between normal operation and error condition tests
- Comprehensive coverage of status fields, conditions, and selector conversion
304-309: Access toDeprecated.V1Beta1should use defensive patterns or explicit nil checks.Direct access to
capiMachineSet.Status.Deprecated.V1Beta1.FullyLabeledReplicasat lines 306 and 397 may panic if the pointer is not initialized. UseHaveValue()matcher (as line 81 does forReadyReplicas) or add a nil guard in test setup to ensure the field is initialized before the test assertions.
| By("Verifying AWSMachine status.instanceState matches MAPI Machine providerStatus.instanceState") | ||
| var mapiProviderStatus mapiv1beta1.AWSMachineProviderStatus | ||
| err = yaml.Unmarshal(mapiMachine.Status.ProviderStatus.Raw, &mapiProviderStatus) | ||
| Expect(err).ToNot(HaveOccurred(), "failed to unmarshal MAPI Machine providerStatus") | ||
|
|
||
| Eventually(komega.Object(awsMachine), capiframework.WaitMedium, capiframework.RetryMedium).Should( | ||
| HaveField("Status.InstanceState", HaveValue(Equal(awsv1.InstanceState(ptr.Deref(mapiProviderStatus.InstanceState, ""))))), | ||
| "Should have AWSMachine status.instanceState match MAPI providerStatus.instanceState", | ||
| ) |
There was a problem hiding this comment.
Add nil check for ProviderStatus before accessing .Raw.
Accessing mapiMachine.Status.ProviderStatus.Raw without verifying ProviderStatus is non-nil could cause a panic if the MAPI Machine lacks provider status.
🔎 Proposed fix
By("Verifying AWSMachine status.instanceState matches MAPI Machine providerStatus.instanceState")
var mapiProviderStatus mapiv1beta1.AWSMachineProviderStatus
+Expect(mapiMachine.Status.ProviderStatus).NotTo(BeNil(), "MAPI Machine should have providerStatus")
err = yaml.Unmarshal(mapiMachine.Status.ProviderStatus.Raw, &mapiProviderStatus)
Expect(err).ToNot(HaveOccurred(), "failed to unmarshal MAPI Machine providerStatus")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| By("Verifying AWSMachine status.instanceState matches MAPI Machine providerStatus.instanceState") | |
| var mapiProviderStatus mapiv1beta1.AWSMachineProviderStatus | |
| err = yaml.Unmarshal(mapiMachine.Status.ProviderStatus.Raw, &mapiProviderStatus) | |
| Expect(err).ToNot(HaveOccurred(), "failed to unmarshal MAPI Machine providerStatus") | |
| Eventually(komega.Object(awsMachine), capiframework.WaitMedium, capiframework.RetryMedium).Should( | |
| HaveField("Status.InstanceState", HaveValue(Equal(awsv1.InstanceState(ptr.Deref(mapiProviderStatus.InstanceState, ""))))), | |
| "Should have AWSMachine status.instanceState match MAPI providerStatus.instanceState", | |
| ) | |
| By("Verifying AWSMachine status.instanceState matches MAPI Machine providerStatus.instanceState") | |
| var mapiProviderStatus mapiv1beta1.AWSMachineProviderStatus | |
| Expect(mapiMachine.Status.ProviderStatus).NotTo(BeNil(), "MAPI Machine should have providerStatus") | |
| err = yaml.Unmarshal(mapiMachine.Status.ProviderStatus.Raw, &mapiProviderStatus) | |
| Expect(err).ToNot(HaveOccurred(), "failed to unmarshal MAPI Machine providerStatus") | |
| Eventually(komega.Object(awsMachine), capiframework.WaitMedium, capiframework.RetryMedium).Should( | |
| HaveField("Status.InstanceState", HaveValue(Equal(awsv1.InstanceState(ptr.Deref(mapiProviderStatus.InstanceState, ""))))), | |
| "Should have AWSMachine status.instanceState match MAPI providerStatus.instanceState", | |
| ) |
🤖 Prompt for AI Agents
e2e/status_conversion_test.go around lines 658-666: add a nil-check for
mapiMachine.Status.ProviderStatus before accessing .Raw to avoid a panic;
specifically, insert an assertion like
Expect(mapiMachine.Status.ProviderStatus).ToNot(BeNil(), "expected MAPI Machine
providerStatus to be set") immediately before the yaml.Unmarshal call (or guard
with an if that fails the test with a clear message) and only call
yaml.Unmarshal when ProviderStatus is non-nil so the test fails cleanly instead
of panicking.
There was a problem hiding this comment.
We should never get here, but the check does not hurt :)
cc @sunzhaohua2
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
/test unit |
|
@sunzhaohua2: This pull request references OCPCLOUD-3017 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 (1)
Makefile (1)
60-60: Timeout increase appears reasonable given the new test coverage.The 50% increase in e2e test timeout (120m → 180m) aligns with the comprehensive status conversion test suite being added. However, consider verifying that the full runtime approaches this limit, as unnecessarily long timeouts can delay failure feedback in CI.
Optional: If the new tests are the primary driver of increased runtime, consider whether any tests could be parallelized or optimized to reduce overall execution time.
📜 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)
Makefile
|
/test e2e-aws-capi-techpreview |
|
@sunzhaohua2 is it ready for review? |
yes |
| }) | ||
|
|
||
| It("should convert status.availableReplicas from MAPI to CAPI status and status.Deprecated.V1Beta1", func() { | ||
| By("Verifying CAPI MachineSet status.availableReplicas is synchronized") |
There was a problem hiding this comment.
I'm curious about this (thinking out loud) - we're waiting for the field to be populated, and once it is we are stating it's synchronised. This works because we start with no CAPI machine set, right, so population == sync?
Should we be looking at SynchronizedGeneration too? Given that the By( says we're waiting for synchronisation (and that's a field in status)? We likely only need to do it once, in the BeforeAll? and it may speed up the tests a little?
e2e/status_conversion_test.go
Outdated
| }) | ||
|
|
||
| // bug https://issues.redhat.com/browse/OCPBUGS-63183 | ||
| It("should convert CAPI Machine phase to MAPI Machine phase", func() { |
There was a problem hiding this comment.
Should this be a PIt() ? Or has the bug been resolved?
There was a problem hiding this comment.
:) I removed the comment, we hit the bug only when the capi machine exists
Makefile
Outdated
| .PHONY: e2e | ||
| e2e: ## Run e2e tests against active kubeconfig | ||
| ./hack/test.sh "./e2e/..." 120m | ||
| ./hack/test.sh "./e2e/..." 180m |
There was a problem hiding this comment.
:'(
Should we consider splitting the e2es / sharing early?
There was a problem hiding this comment.
@theobarberbany I raised a pr here to add shard_count openshift/release#73182
theobarberbany
left a comment
theobarberbany
left a comment
There was a problem hiding this comment.
A few questions / nits, otherwise LGTM!
| } | ||
|
|
||
| // verifyMAPIMachineProviderStatus verifies that a MAPI Machine's providerStatus matches the given Gomega matcher. | ||
| func verifyMAPIMachineProviderStatus(mapiMachine *mapiv1beta1.Machine, matcher types.GomegaMatcher) { |
There was a problem hiding this comment.
I haven't looked at the rest of the file, but we should probably start these functions with GinkgoHelper() See https://onsi.github.io/ginkgo/#mental-model-how-ginkgo-handles-failure, which explains how Ginkgo uses helpers when handling failed assertions.
There was a problem hiding this comment.
Thank you @nrb for helping review, I have added.
| } | ||
|
|
||
| // getAWSProviderStatusFromMachine extracts and unmarshals the AWSMachineProviderStatus from a MAPI Machine. | ||
| func getAWSProviderStatusFromMachine(mapiMachine *mapiv1beta1.Machine) *mapiv1beta1.AWSMachineProviderStatus { |
There was a problem hiding this comment.
Same comment for GinkgoHelper()
e2e/status_conversion_test.go
Outdated
|
|
||
| By("Verifying MAPI Machine has matching addresses from CAPI") | ||
| Eventually(komega.Object(mapiMachine), capiframework.WaitMedium, capiframework.RetryMedium).Should( | ||
| WithTransform(func(mm *mapiv1beta1.Machine) bool { |
There was a problem hiding this comment.
Does the order matter here? If not, I think you could use something like
Eventually(komega.Object(capiMachine.Status.Addresses), capiframework.WaitShort, capiframework.RetryShort).Should(ConsistOf(mapiMachine.Status.Addresses))
This uses the ConsistOf matcher to ensure that each slice contains the same elements, ignoring order.
|
@sunzhaohua2: This pull request references OCPCLOUD-3017 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)
e2e/machineset_migration_helpers.go (1)
356-382: Consider adding GinkgoHelper() to cleanup function for consistency.While
cleanupMachineSetTestResourcesdoesn't make assertions, it uses Ginkgo'sBy()DSL. For consistency with other helper functions in this file that use the Ginkgo DSL, consider addingGinkgoHelper()at the start of this function.📝 Proposed addition
func cleanupMachineSetTestResources(ctx context.Context, cl client.Client, capiMachineSets []*clusterv1.MachineSet, awsMachineTemplates []*awsv1.AWSMachineTemplate, mapiMachineSets []*mapiv1beta1.MachineSet) { + GinkgoHelper() for _, ms := range capiMachineSets {e2e/machine_migration_helpers.go (1)
289-306: Consider adding GinkgoHelper() to cleanup function for consistency.While
cleanupMachineResourcesdoesn't make assertions, it uses Ginkgo'sBy()DSL. For consistency with other helper functions in this file that use the Ginkgo DSL, consider addingGinkgoHelper()at the start of this function.📝 Proposed addition
func cleanupMachineResources(ctx context.Context, cl client.Client, capiMachines []*clusterv1.Machine, mapiMachines []*mapiv1beta1.Machine) { + GinkgoHelper() for _, m := range capiMachines {
📜 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)
e2e/machine_migration_helpers.goe2e/machineset_migration_helpers.goe2e/status_conversion_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/status_conversion_test.go
🔇 Additional comments (4)
e2e/machineset_migration_helpers.go (1)
26-26: LGTM: GinkgoHelper() additions improve test failure reporting.The systematic addition of
GinkgoHelper()calls to these helper functions aligns with Ginkgo v2 best practices and ensures that test failures are reported at the calling site rather than inside the helper, making debugging easier.Also applies to: 57-57, 89-89, 99-99, 109-109, 176-176, 197-197, 230-230, 239-239, 250-250, 268-268, 292-292, 307-307, 324-324, 344-344
e2e/machine_migration_helpers.go (3)
22-22: LGTM: YAML import added for providerStatus unmarshaling.The addition of the
sigs.k8s.io/yamlimport supports the newgetAWSProviderStatusFromMachinehelper function.
26-26: LGTM: GinkgoHelper() additions improve test failure reporting.The systematic addition of
GinkgoHelper()calls to these helper functions aligns with Ginkgo v2 best practices and ensures that test failures are reported at the calling site rather than inside the helper, making debugging easier.Also applies to: 101-101, 146-146, 169-169, 178-178, 211-211, 224-224, 309-309, 316-316
347-354: LGTM: New helper function follows established patterns.The
verifyMAPIMachineProviderStatusfunction correctly usesGinkgoHelper(),Eventuallywithkomega.Object, and a transformer pattern consistent with similar helpers in the codebase (e.g.,verifyMAPIMachineSetProviderSpecin machineset_migration_helpers.go).
|
@sunzhaohua2: The following test 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. |
|
/test okd-scos-images |
|
@nrb @theobarberbany PTAL |
6 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.