CNTRLPLANE-2883: Migrate ARM64 NodePool creation test to v2 - #8926
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@georgelipceanu: This pull request references CNTRLPLANE-2883 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 "5.0.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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds an end-to-end Ginkgo test for ARM64 NodePool creation. The test runs for supported AWS and Azure multi-architecture HostedClusters, configures platform-specific sizing, validates the NodePool specification, and verifies one ready node with the It also parses Azure Sequence Diagram(s)sequenceDiagram
participant E2ETest
participant HostedCluster
participant NodePool
participant ReadyNode
E2ETest->>HostedCluster: Read release and platform details
E2ETest->>NodePool: Create one-replica ARM64 NodePool
E2ETest->>NodePool: Verify ARM64 spec and platform sizing
E2ETest->>ReadyNode: Wait for ready node
ReadyNode-->>E2ETest: Return node with kubernetes.io/arch=arm64
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (10 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/testing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/e2e/v2/tests/nodepool_arm64_create_test.go (1)
48-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove platform skip logic into
BeforeEach.Per the v2 e2e AGENTS.md pattern: Use BeforeEach with Skip() when a test applies only to specific platforms, using the format
if hostedCluster == nil || hostedCluster.Spec.Platform.Type != hyperv1.AWSPlatform { Skip(...) }. Here the AWS/Azure and multi-arch checks are embedded directly in theItbody instead of aBeforeEach.♻️ Suggested refactor
var testCtx *internal.TestContext BeforeEach(func() { testCtx = internal.GetTestContext() Expect(testCtx).NotTo(BeNil(), "test context should be set up in BeforeSuite") + + hc := testCtx.GetHostedCluster() + if hc.Spec.Platform.Type != hyperv1.AWSPlatform && hc.Spec.Platform.Type != hyperv1.AzurePlatform { + Skip("ARM64 NodePool test only supported on AWS and Azure platforms") + } + if hc.Status.PayloadArch != hyperv1.Multi { + Skip("ARM64 NodePool test requires a multi-arch release image") + } })As per coding guidelines, "Use
BeforeEachwithSkip()for platform-specific tests, and include a clear skip message explaining the platform restriction."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/v2/tests/nodepool_arm64_create_test.go` around lines 48 - 60, Move the platform and multi-arch gating out of the It body in the nodepool ARM64 test and into a BeforeEach using Skip(), following the v2 e2e pattern. Update the setup around getTestCtx(), ValidateHostedClusterClient(), and GetHostedCluster() so the HostedCluster is checked before the test runs, then skip early when the platform is not AWS/Azure or when hc.Status.PayloadArch is not hyperv1.Multi. Keep the skip messages clear and specific, and leave the It block focused only on the actual ARM64 NodePool assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/v2/tests/nodepool_arm64_create_test.go`:
- Around line 74-106: The ARM64 NodePool test in nodepool_arm64_create_test
should verify the AWS path after creation the same way it already verifies
Azure. Add an assertion in the createdNP check for the AWS branch that
createdNP.Spec.Platform.AWS.InstanceType matches the configured m6g.large value,
alongside the existing Azure VMSize assertion, using the createdNP object and
the NodePool platform type switch.
- Around line 68-70: The node pool test is using a raw architecture string
instead of the shared constant, which can drift from the expected value. Update
the NodePool setup in the arm64 create test to use hyperv1.ArchitectureARM64 in
the pool.Spec.Arch assignment, keeping it consistent with the existing assertion
that already checks against that constant. This change should be made in the
test helper block that builds the NodePool for the arm64 case.
---
Nitpick comments:
In `@test/e2e/v2/tests/nodepool_arm64_create_test.go`:
- Around line 48-60: Move the platform and multi-arch gating out of the It body
in the nodepool ARM64 test and into a BeforeEach using Skip(), following the v2
e2e pattern. Update the setup around getTestCtx(),
ValidateHostedClusterClient(), and GetHostedCluster() so the HostedCluster is
checked before the test runs, then skip early when the platform is not AWS/Azure
or when hc.Status.PayloadArch is not hyperv1.Multi. Keep the skip messages clear
and specific, and leave the It block focused only on the actual ARM64 NodePool
assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ab42037f-e9cd-4242-bb7d-949470229b73
📒 Files selected for processing (1)
test/e2e/v2/tests/nodepool_arm64_create_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8926 +/- ##
=======================================
Coverage 46.13% 46.13%
=======================================
Files 783 783
Lines 98377 98377
=======================================
Hits 45382 45382
Misses 49919 49919
Partials 3076 3076 see 1 file with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
a327767 to
2096a76
Compare
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll three Prow jobs ( Root CauseThe root cause is CI infrastructure capacity exhaustion on the All three jobs were submitted simultaneously at 09:54:22 UTC. Their pods were created at 09:57:02 UTC in the
Of the 136 nodes in the cluster at that time:
This left 0 eligible nodes, and preemption could not help (131 nodes where preemption wouldn't help, 2 with no viable preemption victims, 3 with insufficient memory even after preemption). After ~30 minutes in This is a transient infrastructure condition — the cluster was overloaded at the time these jobs were submitted. Retrying the jobs when capacity frees up will resolve the issue. Recommendations
Evidence
|
|
/retest images |
|
/test images testing these since they simply failed to find a node to run on, causing them to |
|
/test security |
|
/test verify-deps |
csrwng
left a comment
csrwng
left a comment
There was a problem hiding this comment.
Thanks for migrating this test to v2! One suggestion on making the coverage stronger.
2096a76 to
e4f6c86
Compare
csrwng
added
the
approved
|
/lgtm |
|
Scheduling tests matching the |
e4f6c86 to
87b50c6
Compare
|
@georgelipceanu Could you explain what has changed? Or post this in a separate commit. Thanks |
@mgencur I was meant to add changes to |
|
/lgtm |
georgelipceanu
left a comment
georgelipceanu
left a comment
There was a problem hiding this comment.
/verified by @georgelipceanu
Tested on AKS cluster with 5.0 HC using quay.io/openshift-release-dev/ocp-release-nightly@sha256:106e3771d9d3f4b862fc92278c461e7c307b81223627d242b134a293167f4868 multi image:
- Nodepools created successfully
- Joined successfully
openshift-ci-robot
added
the
verified
|
@georgelipceanu: This PR has been marked as verified by 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
added
the
needs-rebase
Test validates ARM64 NodePool creation succeeds, becomes ready and verifies it has arm64 label. Also update Azure v2 e2e lifecycle to read EXTRA_ARGS environment variable, enabling CI to pass --arch=arm64 for multi-arch clusters. Signed-off-by: George Lipceanu <glipcean@redhat.com>
2fd6c33 to
59f35e3
Compare
openshift-ci-robot
removed
the
verified
openshift-ci
Bot
removed
lgtm
|
/verified by @georgelipceanu same reason here, just needed rebase from merge conflict |
openshift-ci-robot
added
the
verified
|
@georgelipceanu: This PR has been marked as verified by 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. |
|
/lgtm |
|
Scheduling tests matching the |
|
/hold Revision 59f35e3 was retested 3 times: holding |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/test e2e-v2-azure-self-managed |
|
@georgelipceanu: 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. |
5 participants
What this PR does / why we need it:
This PR migrates the v1 E2E test that validates ARM64 NodePool creation succeeds with multi-arch to v2, on AWS and Azure platforms.
The test:
m6g.largeinstance typeStandard_D4ps_v5VM size witharo_422-armmarketplace imagekubernetes.io/arch=arm64labelThis PR also adds
EXTRA_ARGSfor the Azure v2 e2e cluster creation, allowing for CI to pass through--arch=arm64for multi-arch cluster creation.Which issue(s) this PR fixes:
Fixes #CNTRLPLANE-2883
Special notes for your reviewer:
Azure marketplace image selection:
The test explicitly uses thearo_422-armmarketplace SKU for Azure. Older SKUs (419, 420, 421) would get stuck in the GRUB boot menu when I was testing them. Thearo_422-armSKU successfully auto-boots and completes, which is why it is used in this test.The text above was an issue in my own setup, please ignore.
Checklist:
Summary by CodeRabbit
Tests
New Features