🌱 add MachineDeployment scale test #4647

jackfrancis · 2021-05-20T22:47:36Z

What this PR does / why we need it:

This PR adds a new E2E test to validate MachineDeployment scale scenarios.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

jackfrancis · 2021-05-20T22:50:25Z

/test pull-cluster-api-e2e-full-main

jackfrancis · 2021-05-20T23:30:08Z

/test pull-cluster-api-e2e-full-main

test/framework/machinedeployment_helpers.go

test/e2e/md_scale_test.go

enxebre · 2021-05-21T09:52:09Z

I'm not familiar with the e2e costs and duration, If there were concerns to keep those shrunk one could argue that this is implicitly tested by cluster upgrade and drain timeout. Otherwise this is good to have and lgtm.

jackfrancis · 2021-05-21T20:52:53Z

/retest

CecileRobertMichon · 2021-05-24T18:03:08Z

In general i'm +1 to this change since it will be better to have a clear signal on scale up and scale down than having to rely on the scale in the upgrade test.

lgtm besides nits above

jackfrancis · 2021-05-24T19:32:26Z

test/e2e/md_scale.go

+			WaitForMachineDeployments:    input.E2EConfig.GetIntervals(specName, "wait-worker-nodes"),
+		}, clusterResources)
+
+		Expect(clusterResources.MachineDeployments[0].Spec.Replicas).ToNot(BeNil())


@JoelSpeed @CecileRobertMichon I added these lines, I think this is the right place to deal with ensuring the hygiene of the replicas property. If that property is nil, we want to know about it here.

in fact after further investigation the strict nil checks against Replicas aren't required as long as we use the ginkgo Equal( method, as that method includes nil checks, and if nil returns a specific error about that.

test/e2e/node_drain_timeout.go

jackfrancis · 2021-05-24T19:33:20Z

test/framework/machinedeployment_helpers.go

@@ -70,6 +70,7 @@ func GetMachineDeploymentsByCluster(ctx context.Context, input GetMachineDeploym

 	deployments := make([]*clusterv1.MachineDeployment, len(deploymentList.Items))
 	for i := range deploymentList.Items {
+		Expect(deploymentList.Items[i].Spec.Replicas).ToNot(BeNil())


@JoelSpeed @CecileRobertMichon more Replicas non-nil hygiene

jackfrancis · 2021-05-24T20:28:59Z

/retest

CecileRobertMichon · 2021-05-24T20:36:18Z

/lgtm
/assign @fabriziopandini

test/e2e/md_scale.go

srm09 · 2021-05-26T17:43:55Z

test/e2e/md_scale.go

+			MachineDeployment:         clusterResources.MachineDeployments[0],
+			Replicas:                  3,
+			WaitForMachineDeployments: input.E2EConfig.GetIntervals(specName, "wait-worker-nodes"),
+		})


Would adding an assertion for the new number of replicas make sense here?

ScaleAndWaitMachineDeployment does that for us in this case. The earlier assertion is to get a validated baseline before going through the scaling flow(s).

Gotcha, I saw the implicit assertion now. During my earlier pass, I was looking for an explicit assertion in the ScaleAndWaitMD and hence the comment. Make sense, feel free to resolve.

srm09 · 2021-05-26T17:44:03Z

test/e2e/md_scale.go

+			MachineDeployment:         clusterResources.MachineDeployments[0],
+			Replicas:                  1,
+			WaitForMachineDeployments: input.E2EConfig.GetIntervals(specName, "wait-worker-nodes"),
+		})


Same as above

jackfrancis · 2021-05-26T18:03:08Z

/test pull-cluster-api-e2e-full-main

jackfrancis · 2021-05-26T18:22:42Z

/retest

jackfrancis · 2021-05-27T16:22:52Z

/test pull-cluster-api-e2e-full-main

jackfrancis · 2021-05-27T18:21:23Z

/retest

jackfrancis · 2021-05-27T19:33:02Z

/test pull-cluster-api-e2e-full-main

jackfrancis · 2021-05-27T20:38:55Z

/test pull-cluster-api-e2e-full-main

jackfrancis · 2021-05-27T21:08:38Z

/test pull-cluster-api-e2e-full-main

jackfrancis · 2021-05-27T23:03:34Z

@fabriziopandini this is ready for final review

fabriziopandini

@jackfrancis Thanks for adding a new test to the CAPI E2E test suite!
/lgtm

fabriziopandini · 2021-05-28T14:13:13Z

test/e2e/md_scale.go

+)
+
+// MachineDeploymentScaleSpecInput is the input for MachineDeploymentScaleSpec.
+type MachineDeploymentScaleSpecInput struct {


future work: expose the number of worker nodes we scale up to, so people reusing things Spec on providers might decide to have more (or less) workers according to their needs. As soon as we have HollowMachines in CAPD I plan to increase this for CAPI

CecileRobertMichon

/lgtm
/approve

k8s-ci-robot · 2021-05-28T16:40:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [CecileRobertMichon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 20, 2021

k8s-ci-robot requested review from JoelSpeed and vincepri May 20, 2021 22:47

jackfrancis changed the title ~~test: add MachineDeployment scale test~~ 🌱 add MachineDeployment scale test May 20, 2021

jackfrancis force-pushed the e2e-machinedeployment-scale branch from 624b212 to 021b34f Compare May 20, 2021 23:29

jackfrancis force-pushed the e2e-machinedeployment-scale branch from 021b34f to 91c44c1 Compare May 21, 2021 00:08

jackfrancis commented May 21, 2021

View reviewed changes

test/framework/machinedeployment_helpers.go Show resolved Hide resolved

enxebre reviewed May 21, 2021

View reviewed changes

test/e2e/md_scale_test.go Outdated Show resolved Hide resolved

jackfrancis force-pushed the e2e-machinedeployment-scale branch 2 times, most recently from 1699757 to cc262b6 Compare May 24, 2021 19:31

jackfrancis commented May 24, 2021

View reviewed changes

test/e2e/node_drain_timeout.go Outdated Show resolved Hide resolved

jackfrancis commented May 24, 2021

View reviewed changes

k8s-ci-robot assigned fabriziopandini and CecileRobertMichon May 24, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 24, 2021

srm09 reviewed May 26, 2021

View reviewed changes

jackfrancis force-pushed the e2e-machinedeployment-scale branch from cc262b6 to 35f72cd Compare May 26, 2021 17:59

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2021

jackfrancis force-pushed the e2e-machinedeployment-scale branch from 35f72cd to fb3e760 Compare May 27, 2021 16:22

test: add MachineDeployment scale test

dac8d41

jackfrancis force-pushed the e2e-machinedeployment-scale branch from fb3e760 to dac8d41 Compare May 27, 2021 17:32

fabriziopandini reviewed May 28, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 28, 2021

CecileRobertMichon reviewed May 28, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 28, 2021

k8s-ci-robot merged commit 8a62f7e into kubernetes-sigs:master May 28, 2021

k8s-ci-robot added this to the v0.4 milestone May 28, 2021

jackfrancis deleted the e2e-machinedeployment-scale branch May 28, 2021 17:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 add MachineDeployment scale test #4647

🌱 add MachineDeployment scale test #4647

jackfrancis commented May 20, 2021

jackfrancis commented May 20, 2021

jackfrancis commented May 20, 2021

enxebre commented May 21, 2021

jackfrancis commented May 21, 2021

CecileRobertMichon commented May 24, 2021

jackfrancis May 24, 2021

jackfrancis May 27, 2021

jackfrancis May 24, 2021

jackfrancis commented May 24, 2021

CecileRobertMichon commented May 24, 2021

srm09 May 26, 2021

jackfrancis May 26, 2021

srm09 May 26, 2021 •

edited

Loading

srm09 May 26, 2021

jackfrancis May 26, 2021

jackfrancis commented May 26, 2021

jackfrancis commented May 26, 2021

jackfrancis commented May 27, 2021

jackfrancis commented May 27, 2021

jackfrancis commented May 27, 2021

jackfrancis commented May 27, 2021

jackfrancis commented May 27, 2021

jackfrancis commented May 27, 2021

fabriziopandini left a comment

fabriziopandini May 28, 2021

jackfrancis May 28, 2021

CecileRobertMichon left a comment

k8s-ci-robot commented May 28, 2021

🌱 add MachineDeployment scale test #4647

🌱 add MachineDeployment scale test #4647

Conversation

jackfrancis commented May 20, 2021

jackfrancis commented May 20, 2021

jackfrancis commented May 20, 2021

enxebre commented May 21, 2021

jackfrancis commented May 21, 2021

CecileRobertMichon commented May 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfrancis commented May 24, 2021

CecileRobertMichon commented May 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srm09 May 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfrancis commented May 26, 2021

jackfrancis commented May 26, 2021

jackfrancis commented May 27, 2021

jackfrancis commented May 27, 2021

jackfrancis commented May 27, 2021

jackfrancis commented May 27, 2021

jackfrancis commented May 27, 2021

jackfrancis commented May 27, 2021

fabriziopandini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CecileRobertMichon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 28, 2021

srm09 May 26, 2021 •

edited

Loading