OCPCLOUD-2993: support network interface and no MAPI ebs volumesize conversion #431
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@huali9: This pull request references OCPCLOUD-2993 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. |
WalkthroughAdds a new helper function for creating MAPI MachineSets with customizable AWS provider spec updates and introduces an e2e test suite validating AWS special fields conversion from MAPI to CAPI AWSMachineTemplate, including networkInterfaceType and BlockDevices preservation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
[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 |
|
@huali9: This pull request references OCPCLOUD-2993 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/machineset_migration_mapi_authoritative_test.go (1)
493-510: Consider simplifying the Eventually assertion.The manual boolean checks in the
Eventuallyblock could be more concise and easier to maintain. Consider using Gomega matchers for better readability and error messages.Apply this diff to use Gomega matchers:
By("Verifying AWSMachine has correct blockDevice configuration") - Eventually(func() bool { - freshAWSMachine := capiframework.GetAWSMachine(cl, awsMachine.Name, capiframework.CAPINamespace) - if freshAWSMachine.Spec.RootVolume == nil { - return false - } - if freshAWSMachine.Spec.RootVolume.Encrypted == nil || !*freshAWSMachine.Spec.RootVolume.Encrypted { - return false - } - if freshAWSMachine.Spec.RootVolume.Type != awsv1.VolumeTypeGP3 { - return false - } - if freshAWSMachine.Spec.RootVolume.Size != int64(120) { - return false - } - return true - }, capiframework.WaitMedium, capiframework.RetryMedium).Should(BeTrue(), + Eventually(func() *awsv1.AWSMachine { + return capiframework.GetAWSMachine(cl, awsMachine.Name, capiframework.CAPINamespace) + }, capiframework.WaitMedium, capiframework.RetryMedium).Should( + SatisfyAll( + HaveField("Spec.RootVolume.Encrypted", Not(BeNil())), + HaveField("Spec.RootVolume.Encrypted", PointTo(BeTrue())), + HaveField("Spec.RootVolume.Type", Equal(awsv1.VolumeTypeGP3)), + HaveField("Spec.RootVolume.Size", BeNumerically("==", 120)), + ), "AWSMachine should have blockDevice configuration from MAPI MachineSet", )
📜 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/machineset_migration_helpers.go(1 hunks)e2e/machineset_migration_mapi_authoritative_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
e2e/machineset_migration_helpers.go (3)
pkg/conversion/mapi2capi/interface.go (1)
MachineSet(29-31)e2e/framework/machineset.go (1)
CreateMachineSet(49-94)e2e/framework/framework.go (3)
CAPINamespace(14-14)WaitShort(23-23)RetryShort(17-17)
🔇 Additional comments (3)
e2e/machineset_migration_helpers.go (1)
369-417: LGTM! Well-structured helper function.The implementation follows existing patterns and provides a clean extension point for customizing AWS provider specs during MAPI MachineSet creation. The marshal/unmarshal approach is consistent with other helpers in the file, and proper error handling and wait logic are in place.
e2e/machineset_migration_mapi_authoritative_test.go (2)
8-8: LGTM! Appropriate import for pointer helpers.The
ptrpackage import is correctly used throughout the test for creating pointer literals withptr.To().
399-411: Good test coverage for BlockDevices without VolumeSize.The test correctly validates the conversion of BlockDevices that lack an explicit
VolumeSize, setting up encryption, volume type, and intentionally omittingVolumeSizeto verify the defaulting behavior (expected default is 120 as verified on line 469). This aligns with the PR objective for OCP-80519.
| mapiMachineSet = createMAPIMachineSetWithProviderSpecUpdates(ctx, cl, 1, mapiMSSpecialFieldsName, | ||
| func(providerSpec *mapiv1beta1.AWSMachineProviderConfig) { | ||
| // Set networkInterfaceType to ENA | ||
| providerSpec.NetworkInterfaceType = "ENA" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the test file to see the context around the mentioned lines
cd /tmp && find . -name "machineset_migration_mapi_authoritative_test.go" 2>/dev/null | head -1Repository: openshift/cluster-capi-operator
Length of output: 57
🏁 Script executed:
# List files to understand repo structure
git ls-files "**/*machineset_migration*" 2>/dev/nullRepository: openshift/cluster-capi-operator
Length of output: 361
🏁 Script executed:
# Search for the test file
find . -type f -name "*machineset_migration_mapi_authoritative_test.go" 2>/dev/nullRepository: openshift/cluster-capi-operator
Length of output: 128
🏁 Script executed:
# Get NetworkInterfaceType related constants and conversion logic
rg -n "NetworkInterfaceType" --type=go -A 3 -B 1 | head -150Repository: openshift/cluster-capi-operator
Length of output: 8980
🏁 Script executed:
# Search for ENA, EFA, ENI definitions
rg -n "const.*NetworkInterfaceType" --type=go -A 5Repository: openshift/cluster-capi-operator
Length of output: 57
🏁 Script executed:
# Look for conversion or mapping logic between network interface types
rg -n "(ENA|EFA|ENI).*NetworkInterface|NetworkInterface.*(ENA|EFA|ENI)" --type=goRepository: openshift/cluster-capi-operator
Length of output: 2378
🏁 Script executed:
cat -n e2e/machineset_migration_mapi_authoritative_test.go | sed -n '460,495p'Repository: openshift/cluster-capi-operator
Length of output: 2258
🏁 Script executed:
# Also check if there are any constants defined for the network interface types
rg -n "AWSENANetworkInterfaceType|AWSEFANetworkInterfaceType" --type=go -B 2 -A 1Repository: openshift/cluster-capi-operator
Length of output: 2047
🏁 Script executed:
cat -n e2e/machineset_migration_mapi_authoritative_test.go | sed -n '385,400p'Repository: openshift/cluster-capi-operator
Length of output: 998
🏁 Script executed:
# Check if there's a PR or issue reference for OCP-81294
rg -n "OCP-81294|81294" --type=go --type=mdRepository: openshift/cluster-capi-operator
Length of output: 57
Fix assertion message on line 489 to match the expected value.
The test correctly sets NetworkInterfaceType to "ENA" in the MAPI MachineSet (line 396), which is intentionally converted to NetworkInterfaceTypeENI in CAPI per the conversion logic in pkg/conversion/mapi2capi/aws.go (lines 695-696). The assertions on lines 463 and 488 correctly expect NetworkInterfaceTypeENI. However, the assertion message on line 489 says "AWSMachine should have networkInterfaceType set to ENA" when the actual check is for NetworkInterfaceTypeENI. Update the message to say "set to ENI" to match the assertion.
🤖 Prompt for AI Agents
In e2e/machineset_migration_mapi_authoritative_test.go around lines 396–396 and
specifically fix the assertion message at line 489: the test sets
providerSpec.NetworkInterfaceType = "ENA" (line 396) which the conversion maps
to NetworkInterfaceTypeENI, and the assertion at line 488 correctly checks for
NetworkInterfaceTypeENI but the message on line 489 mistakenly says "set to
ENA"; update that assertion message to say "set to ENI" so the message matches
the expected value being asserted.
|
@huali9: all tests passed! 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. |
2 participants
auto OCP-80519 - [CAPI][Migration]MAPI to CAPI conversion should handle no MAPI ebs volumesize and OCP-81294 - [CAPI][Migration]MAPI to CAPI conversion should handle EFA network interface type
passed locally @sunzhaohua2 @miyadav @damdo PTAL, thanks!
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.