Bug 1835851: Update DeleteNodesTwice test #151

JoelSpeed · 2020-05-07T09:06:35Z

The test was relying on the TargetSize to determine if the nodes had been deleted twice. Until #147 was merged, this value would have been the original value from when the nodegroup was constructed and did not reflect the true size of the MachineSet/Deployment underlying the nodegroup

I've updated the test to:

Check that the client sees the deletion timestamp added to Machines
Wait for the Nodegroup to have the correct number of replicas after the first iteration
Check against the API for the final check of the replica count so that we don't rely on our own implementation to pass the test

enxebre · 2020-05-07T10:15:21Z

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go

+			return err
+		}
+
+		// Ensure the update worked


curious if there's any particular reason you saw to add this extra check? seems fair to trust the error returned in 879.

This was actually to make sure the update had propagated and the client that is used by the nodegroup could read the deletiontimestamp, I'll update the comment, I'll also try seeing if this is as stable if I remove this.

I had used stress before to try and get it to fail but it didn't, I can retry that

enxebre · 2020-05-07T10:15:22Z

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go

-		// To make sure we don't run into any flakes in CI
-		// I've chosen to make this sleep duration 3s.
-		time.Sleep(3 * time.Second)
+		expectedSize := len(testConfig.machines) - len(testConfig.machines[7:])


can we take this chance to put the magic number 7 in a constant with a comment?

Yeah will do

JoelSpeed · 2020-05-07T11:35:10Z

@enxebre I reworked this a bit again to make it a bit more obvious and to use the same flow for fetching machines that the NodeGroups does. Just ran stress and seems happy, plus it fails if I remove the deletion timestamp check

stress ./clusterapi.test -test.run TestNodeGroupDeleteNodesTwice -test.count 5
48 runs so far, 0 failures
96 runs so far, 0 failures
156 runs so far, 0 failures
216 runs so far, 0 failures
276 runs so far, 0 failures
326 runs so far, 0 failures

elmiko

tested this out locally and it passes for me as well. thanks Joel!

/approve

enxebre · 2020-05-09T07:46:04Z

/hold
I think we should only merge this if we merge it upstream first. Otherwise we introduce go.mod skew and it means we need to revendor after rebasing every release. Re-vendor in this project is something we should currently avoid as it's painful.

JoelSpeed · 2020-05-11T10:06:51Z

I think we should only merge this if we merge it upstream first. Otherwise we introduce go.mod skew and it means we need to revendor after rebasing every release. Re-vendor in this project is something we should currently avoid as it's painful.

Agreed. I'll open a PR upstream and raise that discussion

JoelSpeed · 2020-05-11T13:36:51Z

Upstream PR: kubernetes#3125

openshift-ci-robot · 2020-05-14T10:16:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elmiko

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:

~~cluster-autoscaler/cloudprovider/clusterapi/OWNERS~~ [elmiko]

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

JoelSpeed · 2020-05-14T10:17:09Z

@enxebre This is actually not too bad using wait.PollImmediate and drops the dependency on Gomega, WDYT?

openshift-ci-robot · 2020-05-14T15:42:04Z

@JoelSpeed: This pull request references Bugzilla bug 1835851, which is invalid:

expected the bug to target the "4.5.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1835851: Update DeleteNodesTwice test

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 kubernetes/test-infra repository.

JoelSpeed · 2020-05-14T15:43:58Z

/bugzilla refresh

openshift-ci-robot · 2020-05-14T15:44:03Z

@JoelSpeed: This pull request references Bugzilla bug 1835851, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

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 kubernetes/test-infra repository.

JoelSpeed · 2020-05-14T15:47:00Z

/hold cancel

This is potentially blocking other bugs from being merged. Let's prioritise this and fixup the upstream later

enxebre · 2020-05-18T10:16:41Z

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_nodegroup_test.go

+			}
+			return targetSize == expectedSize, nil
+		}); err != nil {
+			t.Fatalf("unexpected error: %v", err)


nit: I think this error might go meaningless printed this way, the likes of "you got a time out but I'm not telling you where or why".

enxebre · 2020-05-18T10:16:54Z

/lgtm

… not TargetSize

enxebre · 2020-05-18T10:26:42Z

/lgtm

openshift-bot · 2020-05-18T11:28:14Z

/retest

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

openshift-ci-robot · 2020-05-18T12:19:06Z

@JoelSpeed: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-azure-operator	`3ec2062`	link	`/test e2e-azure-operator`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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 kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-bot · 2020-05-18T12:20:28Z

/retest

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

openshift-ci-robot · 2020-05-18T12:58:17Z

@JoelSpeed: All pull requests linked via external trackers have merged: openshift/kubernetes-autoscaler#151. Bugzilla bug 1835851 has been moved to the MODIFIED state.

In response to this:

Bug 1835851: Update DeleteNodesTwice test

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 kubernetes/test-infra repository.

openshift-ci-robot requested review from alexander-demicev and frobware May 7, 2020 09:07

enxebre reviewed May 7, 2020

View reviewed changes

elmiko approved these changes May 7, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 7, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 9, 2020

JoelSpeed force-pushed the update-delete-nodes-twice-test branch from 0979036 to 135b53f Compare May 11, 2020 13:31

JoelSpeed mentioned this pull request May 13, 2020

Skip DeleteNodesTwice test while waiting for fix #154

Closed

JoelSpeed changed the title ~~Update DeleteNodesTwice test~~ Bug 1835851: Update DeleteNodesTwice test May 14, 2020

openshift-ci-robot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label May 14, 2020

openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label May 14, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label May 14, 2020

openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label May 14, 2020

JoelSpeed force-pushed the update-delete-nodes-twice-test branch from e75294a to 7189c10 Compare May 14, 2020 15:45

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 14, 2020

enxebre reviewed May 18, 2020

View reviewed changes

openshift-ci-robot assigned enxebre May 18, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 18, 2020

UPSTREAM: 3125: openshift: Rewrite DeleteNodesTwice test to check API…

3ec2062

… not TargetSize

JoelSpeed force-pushed the update-delete-nodes-twice-test branch from 7189c10 to 3ec2062 Compare May 18, 2020 10:25

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 18, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 18, 2020

openshift-merge-robot merged commit 8e5c58a into openshift:master May 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1835851: Update DeleteNodesTwice test #151

Bug 1835851: Update DeleteNodesTwice test #151

JoelSpeed commented May 7, 2020

enxebre May 7, 2020

JoelSpeed May 7, 2020

enxebre May 7, 2020

JoelSpeed May 7, 2020

JoelSpeed commented May 7, 2020

elmiko left a comment

enxebre commented May 9, 2020 •

edited

Loading

JoelSpeed commented May 11, 2020

JoelSpeed commented May 11, 2020

openshift-ci-robot commented May 14, 2020

JoelSpeed commented May 14, 2020 •

edited

Loading

openshift-ci-robot commented May 14, 2020

JoelSpeed commented May 14, 2020

openshift-ci-robot commented May 14, 2020

JoelSpeed commented May 14, 2020

enxebre May 18, 2020

enxebre commented May 18, 2020

enxebre commented May 18, 2020

openshift-bot commented May 18, 2020

openshift-ci-robot commented May 18, 2020 •

edited

Loading

openshift-bot commented May 18, 2020

openshift-ci-robot commented May 18, 2020

Bug 1835851: Update DeleteNodesTwice test #151

Bug 1835851: Update DeleteNodesTwice test #151

Conversation

JoelSpeed commented May 7, 2020

enxebre May 7, 2020

Choose a reason for hiding this comment

JoelSpeed May 7, 2020

Choose a reason for hiding this comment

enxebre May 7, 2020

Choose a reason for hiding this comment

JoelSpeed May 7, 2020

Choose a reason for hiding this comment

JoelSpeed commented May 7, 2020

elmiko left a comment

Choose a reason for hiding this comment

enxebre commented May 9, 2020 • edited Loading

JoelSpeed commented May 11, 2020

JoelSpeed commented May 11, 2020

openshift-ci-robot commented May 14, 2020

JoelSpeed commented May 14, 2020 • edited Loading

openshift-ci-robot commented May 14, 2020

JoelSpeed commented May 14, 2020

openshift-ci-robot commented May 14, 2020

JoelSpeed commented May 14, 2020

enxebre May 18, 2020

Choose a reason for hiding this comment

enxebre commented May 18, 2020

enxebre commented May 18, 2020

openshift-bot commented May 18, 2020

openshift-ci-robot commented May 18, 2020 • edited Loading

openshift-bot commented May 18, 2020

openshift-ci-robot commented May 18, 2020

enxebre commented May 9, 2020 •

edited

Loading

JoelSpeed commented May 14, 2020 •

edited

Loading

openshift-ci-robot commented May 18, 2020 •

edited

Loading