Code improvements #49

ingvagabund · 2019-03-13T13:46:05Z

simplifying the code
making the code more readable

enxebre · 2019-03-13T14:06:27Z

pkg/e2e/autoscaler/autoscaler.go

@@ -117,175 +121,151 @@ func labelMachineSetNodes(client runtimeclient.Client, ms *mapiv1beta1.MachineSe
 	})
 }

+// Build default CA resource to allow fast scaling up and down
+func clusterAutoscalerResource() *caov1alpha1.ClusterAutoscaler {


can we move this into utils?

I prefer to keep it in the same file as it is autoscaling specific. Unless it is needed in a different e2e/PACKAGE suite. The less we expose/export the better.

I think there's a lot to be said in having it scoped locally if it is only used locally, so +1 from me.

I was suggesting pkg/e2e/autoscaler/utils.go so no details are exposed on the ginkgo file, so it's smaller and just reads user stories. Not a blocker for me though

pkg/e2e/autoscaler/autoscaler.go

pkg/e2e/infra/infra.go

frobware

LGTM overall but there is a lot of changes here. I would like other folk on the team to take a pass through, then I'll come back for a second round. One nit is we should safely deref MachineSet.Spec.Replicas using pointer.Int32PtrDerefOr() throughout.

pkg/e2e/autoscaler/autoscaler.go

pkg/e2e/framework/common.go

pkg/e2e/autoscaler/autoscaler.go

pkg/e2e/infra/utils.go

pkg/e2e/infra/infra.go

pkg/e2e/infra/utils.go

If a List operation fails, there is no point of retrying as the operation will most likely fail for the second time again. There is a chance the List operation can fail due to infrastructure issues, e.g. network down, apiserver unreachable, etc. In those cases, entire test suite will most likely fail.

Delete all objects of a given kind that match labels. To simplify clean up code.

If any of CA/MA is not created right away, they either already exist (which is not expected) or the communication channel between apiserver and the client is broken. Both CA and MA CRDs are already deployed by CVO so the error caused by missing CRDs definitions is excluded.

ingvagabund · 2019-03-14T13:52:09Z

/retest

ingvagabund · 2019-03-14T14:24:50Z

/retest

ingvagabund · 2019-03-14T15:57:13Z

/test k8s-e2e

frobware · 2019-03-14T16:14:53Z

/lgtm

ingvagabund · 2019-03-14T16:20:32Z

/retest

spangenberg · 2019-03-14T17:41:31Z

/approve

spangenberg · 2019-03-14T17:41:52Z

/retest

openshift-bot · 2019-03-14T20:15:53Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-03-15T00:17:51Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-03-15T02:18:50Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-03-15T04:19:52Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-03-15T06:20:51Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-03-15T08:21:51Z

/retest

Please review the full test history for this PR and help us cut down flakes.

We use ginkgo/gomega as a framework to run our tests. There is no plan to spread ginkgo/gomega based language outside of the main execution. In the future, we might switch to a different framework, thus keeping the dependency on ginkgo/gomega as minimal as possible.

Given all such functions are already under utils.go, it's natural to put the remaining two there as well.

Using GetMachineSet allows to force the default machine API namespaces.

Otherwise possibly failing with: Operation cannot be fulfilled on machines.machine.openshift.io \"kubemark-actuator-testing-machineset-****\": the object has been modified; please apply your changes to the latest version and try again

…deployment

openshift-ci-robot · 2019-03-15T09:50:35Z

New changes are detected. LGTM label has been removed.

ingvagabund · 2019-03-15T09:52:02Z

This obviously is not right:

@@ -162,14 +165,16 @@ var _ = g.Describe("[Feature:Machines] Managed cluster should", func() {
 		o.Expect(err).NotTo(o.HaveOccurred())
 
 		g.By(fmt.Sprintf("waiting for cluster to grow %d nodes. Size should be %d", clusterGrowth, intermediateClusterSize))
-		waitForClusterSizeToBeHealthy(intermediateClusterSize)
+		err = waitForClusterSizeToBeHealthy(client, initialClusterSize)
+		o.Expect(err).NotTo(o.HaveOccurred())
 
 		g.By(fmt.Sprintf("scaling in %q machineSet to %d replicas", machineSet.Name, scaleIn))
 		err = scaleMachineSet(machineSet.Name, scaleIn)
 		o.Expect(err).NotTo(o.HaveOccurred())
 
 		g.By(fmt.Sprintf("waiting for cluster to decrease %d nodes. Final size should be %d nodes", clusterDecrease, finalClusterSize))
-		waitForClusterSizeToBeHealthy(finalClusterSize)
+		err = waitForClusterSizeToBeHealthy(client, initialClusterSize)
+		o.Expect(err).NotTo(o.HaveOccurred())

Just changed back to intermediateClusterSize and finalClusterSize.

spangenberg · 2019-03-15T10:08:30Z

/approve

openshift-ci-robot · 2019-03-15T10:08:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: spangenberg

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~~ [spangenberg]

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

openshift-ci-robot requested review from bison and vikaschoudhary16 March 13, 2019 13:46

openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 13, 2019

enxebre reviewed Mar 13, 2019

View reviewed changes

pkg/e2e/autoscaler/autoscaler.go Outdated Show resolved Hide resolved

enxebre reviewed Mar 13, 2019

View reviewed changes

pkg/e2e/autoscaler/autoscaler.go Outdated Show resolved Hide resolved

enxebre reviewed Mar 13, 2019

View reviewed changes

pkg/e2e/infra/infra.go Outdated Show resolved Hide resolved

frobware reviewed Mar 13, 2019

View reviewed changes

pkg/e2e/autoscaler/autoscaler.go Outdated Show resolved Hide resolved

pkg/e2e/autoscaler/autoscaler.go Show resolved Hide resolved

pkg/e2e/framework/common.go Outdated Show resolved Hide resolved

pkg/e2e/autoscaler/autoscaler.go Outdated Show resolved Hide resolved

frobware reviewed Mar 13, 2019

View reviewed changes

pkg/e2e/autoscaler/autoscaler.go Outdated Show resolved Hide resolved

frobware reviewed Mar 13, 2019

View reviewed changes

pkg/e2e/autoscaler/autoscaler.go Outdated Show resolved Hide resolved

enxebre reviewed Mar 13, 2019

View reviewed changes

pkg/e2e/infra/utils.go Outdated Show resolved Hide resolved

enxebre reviewed Mar 13, 2019

View reviewed changes

pkg/e2e/infra/utils.go Outdated Show resolved Hide resolved

enxebre reviewed Mar 13, 2019

View reviewed changes

pkg/e2e/infra/infra.go Outdated Show resolved Hide resolved

enxebre reviewed Mar 13, 2019

View reviewed changes

pkg/e2e/infra/utils.go Outdated Show resolved Hide resolved

ingvagabund added 16 commits March 14, 2019 12:36

Remove getMachineList in favor of getMachines

bae7e39

Makefile: build e2e

0059836

Remove getNodeList in favor of getNodes

d881f92

Refactor getClusterSize to get nodes by calling getNodes

5963bfe

Remove getMachineSetList in favor of getMachineSets

90efba3

Get machine directly without retry logic

14df399

Build CA and MA resources within a function

ff0cead

List machineset list directly without retry logic

c012327

Dereference a pointer only once

e4c63b7

Implement and apply DeleteObjectsByLabels

66f31b5

Delete all objects of a given kind that match labels. To simplify clean up code.

Fix error message

c2ef4e1

Create workload directly without any retry logic

4e07125

openshift-ci-robot assigned frobware Mar 14, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 14, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 14, 2019

ingvagabund added 6 commits March 15, 2019 10:48

Move util functions under utils.go

9d9e98c

Given all such functions are already under utils.go, it's natural to put the remaining two there as well.

Set the default machineset Replicas field in constant

7242ac6

Use GetMachineSet to retrieve a machineset by its name

95fed5d

Using GetMachineSet allows to force the default machine API namespaces.

Detect kubemark based on presence of machineapi-kubemark-controllers …

e734831

…deployment

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 15, 2019

ingvagabund added the lgtm Indicates that a PR is ready to be merged. label Mar 15, 2019

openshift-merge-robot merged commit 47c603f into openshift:master Mar 15, 2019

ingvagabund deleted the code-improvements branch March 15, 2019 11:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code improvements #49

Code improvements #49

ingvagabund commented Mar 13, 2019

enxebre Mar 13, 2019

ingvagabund Mar 13, 2019

frobware Mar 13, 2019

enxebre Mar 14, 2019

frobware left a comment

ingvagabund commented Mar 14, 2019

ingvagabund commented Mar 14, 2019

ingvagabund commented Mar 14, 2019

frobware commented Mar 14, 2019

ingvagabund commented Mar 14, 2019

spangenberg commented Mar 14, 2019

spangenberg commented Mar 14, 2019

openshift-bot commented Mar 14, 2019

openshift-bot commented Mar 15, 2019

openshift-bot commented Mar 15, 2019

openshift-bot commented Mar 15, 2019

openshift-bot commented Mar 15, 2019

openshift-bot commented Mar 15, 2019

openshift-ci-robot commented Mar 15, 2019

ingvagabund commented Mar 15, 2019

spangenberg commented Mar 15, 2019

openshift-ci-robot commented Mar 15, 2019

Code improvements #49

Code improvements #49

Conversation

ingvagabund commented Mar 13, 2019

enxebre Mar 13, 2019

Choose a reason for hiding this comment

ingvagabund Mar 13, 2019

Choose a reason for hiding this comment

frobware Mar 13, 2019

Choose a reason for hiding this comment

enxebre Mar 14, 2019

Choose a reason for hiding this comment

frobware left a comment

Choose a reason for hiding this comment

ingvagabund commented Mar 14, 2019

ingvagabund commented Mar 14, 2019

ingvagabund commented Mar 14, 2019

frobware commented Mar 14, 2019

ingvagabund commented Mar 14, 2019

spangenberg commented Mar 14, 2019

spangenberg commented Mar 14, 2019

openshift-bot commented Mar 14, 2019

openshift-bot commented Mar 15, 2019

openshift-bot commented Mar 15, 2019

openshift-bot commented Mar 15, 2019

openshift-bot commented Mar 15, 2019

openshift-bot commented Mar 15, 2019

openshift-ci-robot commented Mar 15, 2019

ingvagabund commented Mar 15, 2019

spangenberg commented Mar 15, 2019

openshift-ci-robot commented Mar 15, 2019