CNTRLPLANE-2883: Migrate ARM64 NodePool creation test to v2 - #8926
CNTRLPLANE-2883: Migrate ARM64 NodePool creation test to v2#8926georgelipceanu wants to merge 1 commit into
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 45.73% 45.75% +0.01%
==========================================
Files 781 781
Lines 97828 97837 +9
==========================================
+ Hits 44738 44761 +23
+ Misses 50019 50007 -12
+ Partials 3071 3069 -2 see 2 files 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 |
87b50c6 to
8209f14
Compare
openshift-ci
Bot
added
the
area/platform/azure
8209f14 to
ede22f1
Compare
| func (a *AzurePlatformConfig) ClusterSpecs(releaseImage, n1Image string) []ClusterSpec { | ||
| // Parse EXTRA_ARGS from environment if provided | ||
| var extraArgs []string | ||
| if envArgs := os.Getenv("EXTRA_ARGS"); envArgs != "" { |
There was a problem hiding this comment.
Let's sync with this other PR which adds the same to AWS: https://github.com/openshift/hypershift/pull/9174/changes#r3682537387
There was a problem hiding this comment.
Thanks for bringing this to my attention. Is this in relation to the ParseExtraArgs() function in test/e2e/v2/lifecycle/platform.go? If so then we can wait for this to merge and leverage this in this PR, if that is a valid plan forward.
There was a problem hiding this comment.
I think this is fine to do since it's basically the wild west right now in terms of expanding the environment variable surface (and I'm also guilty of it now), but I do want to take a followup to revisit how we're defining and overriding configuration throughout the v2 framework, it's a mess and I think environment is the wrong tool for the job
There was a problem hiding this comment.
In my PR the arguments are totally generalized / not platform-coupled, if anything I would suggest maybe scoping yours as AZURE_CREATE_EXTRA_ARGS or something. My goal would be to find a way to remove this, mine, and all the rest in a different design....
There was a problem hiding this comment.
if anything I would suggest maybe scoping yours as AZURE_CREATE_EXTRA_ARGS or something.
AFAIK, the --arch flag isn't an Azure specific thing so I don't think that having a separate AZURE_CREATE_EXTRA_ARGS flag for Azure is necessary at the moment and may only create unnecessary complexities (unless that flag is Azure specific, but even then I imagine that is something that could be added to the non-platform coupled args anyway). So I don't mind holding this until your PR is merged, or until we revisit how we're adding individual configurations throughout the v2 framework. 👍
There was a problem hiding this comment.
@georgelipceanu @mgencur I think your approach is at more tightly scoped and cleaner, so I'll adopt your pattern in my PR and we can refactor it all in a followup design. Don't block on me
ede22f1 to
324256f
Compare
openshift-ci
Bot
added
the
needs-rebase
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: georgelipceanu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
324256f to
4f932e0
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/lgtm |
|
Scheduling tests matching the |
|
/test e2e-aws-4-22 |
|
/test e2e-aws-4-22 |
|
@georgelipceanu you need to rebase to pick up #9229, I'm actually not sure why your current branch isn't failing CI checks with compilation errors |
@ironcladlou I thought it was just CI flakes since the other tests were passing without failures while this was behind in commits. I do see that in the Codecov now though, so I'll get this rebased, apologies for the waste in resources. 😓 Other tests probably passed before since maybe that PR merged after they were run? Either way rebasing will make all tests rerun so it should be fine after that, thanks for pointing this out. 👍 |
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>
4f932e0 to
e72ba90
Compare
|
New changes are detected. LGTM label has been removed. |
| NodePoolArm64CreateTest(getTestCtx) | ||
| } | ||
|
|
||
| var _ = Describe("[sig-hypershift][Jira:Hypershift][Feature:NodePoolArm64] NodePool ARM64", Label("nodepool-arm64"), func() { |
There was a problem hiding this comment.
We've been talking about enforcing a standard where all new tests are labeled informing and must be promoted to blocking after they meet well defined stability criteria (e.g. following OCP, 20 consecutive green runs)... this hasn't been decided yet within the team but I think that's the general direction we're headed, we should talk about it before merging a new test
There was a problem hiding this comment.
We should also talk about what to do with any non-v2 test this is replacing (as applicable) on a case by case basis
There was a problem hiding this comment.
I'm still new to this new process of E2E and need to catch up and refresh myself on previous discussions on where things are headed, I appreciate reminder on the informing -> blocking criteria. I can hold this for now then and we can keep bringing this up in meetings and Slack threads until finalised. 👍
We should also talk about what to do with any non-v2 test this is replacing (as applicable) on a case by case basis
AFAIK, I don't think this test was ever blocking in any capacity, both pre-submit and periodic wise, and I don't know the significance of ARM64 NodePools in HCP today (I imagine that other tests in other areas that need to be migrate would take greater blocking priority over this). It could be something to discuss with one of the owners of the V1, or just with the whole team on how necessary this is to be get to blocking.
There was a problem hiding this comment.
Thanks, I still need to go decode the CI configs around the ARM tests and this also relates to the ARM Karpenter tests (which I want to port as a followup to #9292), I think there's also a special job for that as well that's not in the presubmits but I don't have it all mapped out yet. I'm still wanting to get up to speed with kubevirt as well, isn't the ARM support important there too?
I definitely want to help you get this in one way or another
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
@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