test(e2e+machine): add in machine tests #1212

charliedmcb · 2025-10-09T21:09:42Z

Fixes #

Description
Built off this underlaying framework:
#1200

These tests cover:

Assignment of post-provisioning networking labels that were previously handled by Karpenter
Karpenter handling of DriftAction + kubelet causing DriftAction
PUT machine during PUT MC

How was this change tested?

Tested with this PR:

DO-NOT-MERGE: Staging testing ground for E2E work, on top of Machine API POC branch #1204
NOTE: didn't get fully successful runs, and do expect the kubelet test to fail

Does this change impact docs?

Yes, PR includes docs updates
Yes, issue opened: #
No

Release Note

NONE

comtalyst · 2025-10-30T01:07:31Z

test/suites/machines/machines_test.go

+				bK8sVersion := lo.Must(semver.Parse(*b.KubernetesVersion))
+				return aK8sVersion.GT(bK8sVersion)
+			}).KubernetesVersion
+			upgradedMC := env.ExpectSuccessfulUpgradeOfManagedCluster(kubernetesUpgradeVersion)


According to the implementation, this might unintentionally wait for PUT to be done, while we want PUT to be ongoing?

agree - I don't see how cluster is still in upgrading at the end of this test given the above.

Yea, good catch.

I think I had a local fix for this changing the func to be async, and you could optionally wait for polling after, so here is could avoid that, and in the regular k8s test it could use that. Must have not gotten pushed, and lost in the codespace now. Sorry about that.

Should be updated.

matthchr · 2025-10-30T22:49:51Z

.github/workflows/e2e.yaml

          git_ref: ${{ inputs.git_ref }}
          location: ${{ inputs.location }}
          provisionmode: ${{ inputs.provisionmode }}
+          identity_type: ${{ inputs.suite == 'Machines' && 'UserAssigned' || 'SystemAssigned' }}


Why do we need machine identity to be UserAssigned? Can we leave a comment here or in create-cluster/action.yaml explaining?

This is required for handling kubelet update/upgrade.

You can't do the user assigned kubelet update test without the cluster being in a user assigned identity state.

matthchr · 2025-10-30T22:50:50Z

test/pkg/environment/azure/expectations.go

 	})
 }

+func (env *Environment) ExpectGetManagedCluster() containerservice.ManagedCluster {


Suggested change

func (env *Environment) ExpectGetManagedCluster() containerservice.ManagedCluster {

func (env *Environment) ExpectGetManagedCluster() *containerservice.ManagedCluster {

matthchr · 2025-10-30T22:51:26Z

test/pkg/environment/azure/expectations.go

+func (env *Environment) WarnIfClusterNotInExpectedProvisioningState(expectedProvisioningState string) containerservice.ManagedCluster {
+	GinkgoHelper()
+	managedCluster := env.ExpectGetManagedCluster()
+	Expect(managedCluster.Properties.ProvisioningState).ToNot(BeNil())


minor: assumes Properties is not nil

matthchr · 2025-10-30T22:51:58Z

test/pkg/environment/azure/expectations.go

+
+// WarnIfClusterNotInExpectedProvisioningState checks if the clusters provisioning state is equal to the
+// given expected provisioning state.
+func (env *Environment) WarnIfClusterNotInExpectedProvisioningState(expectedProvisioningState string) containerservice.ManagedCluster {


Suggested change

func (env *Environment) WarnIfClusterNotInExpectedProvisioningState(expectedProvisioningState string) containerservice.ManagedCluster {

func (env *Environment) WarnIfClusterNotInExpectedProvisioningState(expectedProvisioningState string) *containerservice.ManagedCluster {

matthchr · 2025-10-30T22:53:07Z

.github/workflows/e2e-matrix.yaml

      fail-fast: false
      matrix:
-        suite: [ACR, BYOK, Chaos, Consolidation, Drift, GPU, InPlaceUpdate, Integration, KubernetesUpgrade, NodeClaim, Scheduling, Spot, Subnet, Utilization]
+        suite: [ACR, BYOK, Chaos, Consolidation, Drift, GPU, InPlaceUpdate, Integration, KubernetesUpgrade, NodeClaim, Scheduling, Spot, Subnet, Utilization, Machines]


minor: Alphabetical?

TBH might be a good idea at this point to just break this guy and put 1 suite per line.
This is YAML after all we don't need to mash everything into a single line AFAIK.

matthchr · 2025-10-30T23:01:06Z

test/suites/machines/machines_test.go

+		Expect(node.Labels).To(HaveKeyWithValue("kubernetes.azure.com/azure-cni-overlay", "true"))
+		Expect(node.Labels).To(HaveKeyWithValue("kubernetes.azure.com/podnetwork-type", consts.NetworkPluginModeOverlay))
+
+		// Note: these labels we only check their keys since, the values are dynamic


Suggested change

// Note: these labels we only check their keys since, the values are dynamic

// Note: these labels we only check their keys since the values are dynamic

matthchr · 2025-10-30T23:02:13Z

test/suites/machines/machines_test.go

+	})
+
+	// NOTE: ClusterTests modify the actual cluster itself, which means that preforming tests after a cluster test
+	// might not have a clean environment, and might produce unexpected results. Ordering of cluster tests is important


minor: Should we add a note here that this is safe in CI because each E2E runs on its own cluster?

So, its safe so long as the suite itself is setup correctly to handle the ordering. However, there is leakage between test cases within the suite

matthchr · 2025-10-30T23:04:25Z

test/suites/machines/machines_test.go

+			// updatedKubeletIdentityResourceID := env.GetKubeletIdentityResourceID(env.Context)
+
+			// TODO: check if we want to have this possibly logged
+			// Expect(updatedKubeletIdentityResourceID).To(Equal(lo.FromPtr(newIdentity.ID)), "Expected updatedKubeletIdentityResourceID to match new kubelet resource id")


Do we not want this assert? Not sure I follow why not

This was a light concern on leaking the indentity info. If there is no concern, than yes this asset should be there

matthchr · 2025-10-30T23:04:41Z

test/suites/machines/machines_test.go

+			}).WithTimeout(3 * time.Minute).Should(Succeed())
+
+			By("expecting nodes to drift")
+			env.EventuallyExpectDriftedWithTimeout(15*time.Minute, nodeClaims...)


15m seems a long time?

especially since this is just detection of drift, not actual drift? Does this really take 15m? Shouldn't it take more like 30s?

yea, I think this came from the other drift tests. Agreed can be shortened; though, I feel like it'd be worth looking at the drift logic, or some metrics to make sure its now being shortened too far.

matthchr · 2025-10-30T23:06:33Z

test/suites/machines/machines_test.go

+				bK8sVersion := lo.Must(semver.Parse(*b.KubernetesVersion))
+				return aK8sVersion.GT(bK8sVersion)
+			}).KubernetesVersion
+			upgradedMC := env.ExpectSuccessfulUpgradeOfManagedCluster(kubernetesUpgradeVersion)


agree - I don't see how cluster is still in upgrading at the end of this test given the above.

add machines tests

fcc5e9c

charliedmcb changed the base branch from main to charliedmcb/addBYOMachinePoolE2EBaseStructure October 9, 2025 21:10

comtalyst reviewed Oct 30, 2025

View reviewed changes

matthchr reviewed Oct 30, 2025

View reviewed changes

	func (env *Environment) ExpectGetManagedCluster() containerservice.ManagedCluster {
	func (env Environment) ExpectGetManagedCluster() containerservice.ManagedCluster {

	func (env *Environment) WarnIfClusterNotInExpectedProvisioningState(expectedProvisioningState string) containerservice.ManagedCluster {
	func (env Environment) WarnIfClusterNotInExpectedProvisioningState(expectedProvisioningState string) containerservice.ManagedCluster {

	// Note: these labels we only check their keys since, the values are dynamic
	// Note: these labels we only check their keys since the values are dynamic

test(e2e+machine): add in machine tests #1212

Are you sure you want to change the base?

test(e2e+machine): add in machine tests #1212

Uh oh!

Conversation

charliedmcb commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

charliedmcb commented Oct 9, 2025 •

edited

Loading