CNTRLPLANE-2883: Migrate ARM64 NodePool creation test to v2 #8926
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| //go:build e2ev2 | ||
|
|
||
| /* | ||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package tests | ||
|
|
||
| import ( | ||
| . "github.com/onsi/ginkgo/v2" | ||
| . "github.com/onsi/gomega" | ||
|
|
||
| hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" | ||
| e2eutil "github.com/openshift/hypershift/test/e2e/util" | ||
| "github.com/openshift/hypershift/test/e2e/v2/internal" | ||
|
|
||
| crclient "sigs.k8s.io/controller-runtime/pkg/client" | ||
| ) | ||
|
|
||
| // RegisterNodePoolArm64Tests registers ARM64 NodePool test cases. | ||
| func RegisterNodePoolArm64Tests(getTestCtx internal.TestContextGetter) { | ||
| NodePoolArm64CreateTest(getTestCtx) | ||
| } | ||
|
|
||
| var _ = Describe("[sig-hypershift][Jira:Hypershift][Feature:NodePoolArm64] NodePool ARM64", Label("nodepool-arm64"), func() { | ||
|
Contributor
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
Contributor
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
Member
Author
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. 👍
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.
Contributor
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 |
||
| var testCtx *internal.TestContext | ||
| BeforeEach(func() { | ||
| testCtx = internal.GetTestContext() | ||
| Expect(testCtx).NotTo(BeNil(), "test context should be set up in BeforeSuite") | ||
|
georgelipceanu marked this conversation as resolved.
|
||
| }) | ||
| RegisterNodePoolArm64Tests(func() *internal.TestContext { return testCtx }) | ||
| }) | ||
|
|
||
| // NodePoolArm64CreateTest creates an ARM64 NodePool, waits for the node to be ready, | ||
| // and validates that an actual ARM64 node comes up with the correct architecture label. | ||
| func NodePoolArm64CreateTest(getTestCtx internal.TestContextGetter) { | ||
| It("When creating an ARM64 NodePool, it should provision a node with ARM64 architecture", func() { | ||
| testCtx := getTestCtx() | ||
| hc, err := testCtx.GetHostedCluster() | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| 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") | ||
| } | ||
| ctx := testCtx.Context | ||
| hcClient, err := testCtx.GetHostedClusterClient(hc) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| defaultNP := getDefaultNodePool(ctx, testCtx.MgmtClient, hc) | ||
| Expect(defaultNP).NotTo(BeNil(), "default NodePool should exist") | ||
| var oneReplica int32 = 1 | ||
| np := buildTestNodePool(defaultNP, "arm64", func(pool *hyperv1.NodePool) { | ||
| pool.Spec.Replicas = &oneReplica | ||
| pool.Spec.Arch = hyperv1.ArchitectureARM64 | ||
| // Use the same release image as the HostedCluster | ||
| pool.Spec.Release.Image = hc.Spec.Release.Image | ||
| if pool.Spec.Platform.Type == hyperv1.AWSPlatform { | ||
| pool.Spec.Platform.AWS.InstanceType = "m6g.large" | ||
| } else if pool.Spec.Platform.Type == hyperv1.AzurePlatform { | ||
| pool.Spec.Platform.Azure.VMSize = "Standard_D2ps_v5" | ||
| // Let the controller select the marketplace image for this NodePool's architecture. | ||
| // Leaving AzureMarketplace nil signals the controller to auto-detect from the release payload. | ||
| pool.Spec.Platform.Azure.Image = hyperv1.AzureVMImage{Type: hyperv1.AzureMarketplace} | ||
| } | ||
| }) | ||
|
|
||
| err = testCtx.MgmtClient.Create(ctx, np) | ||
| Expect(err).NotTo(HaveOccurred(), "failed to create ARM64 NodePool %s", np.Name) | ||
| GinkgoWriter.Printf("Created ARM64 NodePool %s\n", np.Name) | ||
| DeferCleanup(func() { | ||
| cleanupNodePool(ctx, testCtx.MgmtClient, np) | ||
| }) | ||
| // Verify the NodePool was created with correct spec | ||
| createdNP := &hyperv1.NodePool{} | ||
| err = testCtx.MgmtClient.Get(ctx, crclient.ObjectKeyFromObject(np), createdNP) | ||
| Expect(err).NotTo(HaveOccurred(), "failed to get created ARM64 NodePool %s", np.Name) | ||
| Expect(createdNP.Spec.Arch).To(Equal(hyperv1.ArchitectureARM64), "NodePool should have ARM64 architecture") | ||
| Expect(createdNP.Spec.Replicas).NotTo(BeNil(), "NodePool should have replicas set") | ||
| Expect(*createdNP.Spec.Replicas).To(Equal(int32(1)), "NodePool should have 1 replica") | ||
| if createdNP.Spec.Platform.Type == hyperv1.AWSPlatform { | ||
| Expect(createdNP.Spec.Platform.AWS.InstanceType).To(Equal("m6g.large"), "NodePool should use ARM64-capable instance type") | ||
| } | ||
| if createdNP.Spec.Platform.Type == hyperv1.AzurePlatform { | ||
| Expect(createdNP.Spec.Platform.Azure.VMSize).To(Equal("Standard_D2ps_v5"), "NodePool should use ARM64-capable VM size") | ||
| } | ||
|
georgelipceanu marked this conversation as resolved.
|
||
| GinkgoWriter.Printf("Verified ARM64 NodePool %s created with correct architecture settings\n", np.Name) | ||
| // Wait for the ARM64 node to become ready | ||
| nodes := e2eutil.WaitForReadyNodesByNodePool(GinkgoTB(), ctx, hcClient, np, hc.Spec.Platform.Type) | ||
| Expect(nodes).To(HaveLen(1), "expected exactly 1 ARM64 node for NodePool %s", np.Name) | ||
| // Verify the node has the correct ARM64 architecture label | ||
| node := nodes[0] | ||
| Expect(node.Labels).NotTo(BeNil(), "node %s should have labels", node.Name) | ||
| arch, ok := node.Labels["kubernetes.io/arch"] | ||
| Expect(ok).To(BeTrue(), "node %s should have kubernetes.io/arch label", node.Name) | ||
| Expect(arch).To(Equal("arm64"), "node %s should have ARM64 architecture", node.Name) | ||
| GinkgoWriter.Printf("Verified node %s has ARM64 architecture label\n", node.Name) | ||
| }) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for bringing this to my attention. Is this in relation to the
ParseExtraArgs()function intest/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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK, the
--archflag isn't an Azure specific thing so I don't think that having a separateAZURE_CREATE_EXTRA_ARGSflag 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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