Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add a test for deletion annotations after scale down #150

Merged

Conversation

elmiko
Copy link
Contributor

@elmiko elmiko commented May 5, 2020

This change adds a test to the autoscale "scale up and down" suite that
will check to see if there are any deletion annotations left on the
machine resources after a scale down. To accomplish this some logic has
been added to make the test wait until the scale down has completed so
that we can check the remaining machines.

This test is related to bugzilla case 1820410. reference:
https://bugzilla.redhat.com/show_bug.cgi?id=1820410

@elmiko
Copy link
Contributor Author

elmiko commented May 5, 2020

/hold

i think this test needs a little further examination, i have seen some test failures that i am having trouble tracking down.

@openshift-ci-robot 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 5, 2020
@elmiko elmiko force-pushed the test-for-deletion-annotation branch from cdb7eb0 to 0d4b7ae Compare May 6, 2020 14:05
@elmiko
Copy link
Contributor Author

elmiko commented May 6, 2020

/hold cancel

i've fixed this test to take into account the time involved with resolving the deletion taints from the remaining machines. we expect the annotations will be removed but it is difficult to guarantee /when/ they will be removed. this test will now wait for a short period of time to give the machines extra time to reflect the annotation changes.

@openshift-ci-robot 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 6, 2020
@elmiko elmiko force-pushed the test-for-deletion-annotation branch from 0d4b7ae to e3a6907 Compare May 6, 2020 15:20
@elmiko
Copy link
Contributor Author

elmiko commented May 6, 2020

adjusted the wait time for the annotations from short to medium. although this test passes locally, i am seeing error in the ci jobs. i have a feeling relaxing the timing here will help.

@elmiko
Copy link
Contributor Author

elmiko commented May 6, 2020

/retest

@elmiko elmiko force-pushed the test-for-deletion-annotation branch from e3a6907 to 6aaa7bb Compare May 6, 2020 17:18
@elmiko
Copy link
Contributor Author

elmiko commented May 6, 2020

just adding a little more info to the log statements

Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests look good, added some suggestions for improvements, LMK if you have any questions

pkg/autoscaler/autoscaler.go Outdated Show resolved Hide resolved
pkg/autoscaler/autoscaler.go Outdated Show resolved Hide resolved
pkg/autoscaler/autoscaler.go Outdated Show resolved Hide resolved
pkg/autoscaler/autoscaler.go Outdated Show resolved Hide resolved
pkg/autoscaler/autoscaler.go Outdated Show resolved Hide resolved
pkg/autoscaler/autoscaler.go Outdated Show resolved Hide resolved
pkg/autoscaler/autoscaler.go Outdated Show resolved Hide resolved
pkg/autoscaler/autoscaler.go Outdated Show resolved Hide resolved
@elmiko elmiko force-pushed the test-for-deletion-annotation branch from 6aaa7bb to 1eb287c Compare May 13, 2020 15:45
@elmiko
Copy link
Contributor Author

elmiko commented May 13, 2020

updated based on comments. as discussed offline, we will push on the By changes in favor of improving our log usage(in a different PR).

edit: related #154

@elmiko
Copy link
Contributor Author

elmiko commented May 13, 2020

/retest

@JoelSpeed
Copy link
Contributor

/lgtm

Needs a BZ if we want this to merge now

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 14, 2020
@elmiko
Copy link
Contributor Author

elmiko commented May 14, 2020

Needs a BZ if we want this to merge now

we do have a bz for this, but i was under the impression that we don't need them for this repo.

@elmiko
Copy link
Contributor Author

elmiko commented May 14, 2020

/retest

@JoelSpeed
Copy link
Contributor

Tide status seems to me to suggest otherwise 🤔

@elmiko
Copy link
Contributor Author

elmiko commented May 14, 2020

i had thought we didn't need them per our discussion offline, but looking at another recent pr i can see we do. i'll add it.

@elmiko elmiko changed the title add a test for deletion annotations after scale down Bug 1820410: add a test for deletion annotations after scale down May 14, 2020
@openshift-ci-robot
Copy link

@elmiko: This pull request references Bugzilla bug 1820410, which is invalid:

  • expected the bug to be in one of the following states: NEW, ASSIGNED, ON_DEV, POST, POST, but it is VERIFIED 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 1820410: add a test for deletion annotations after scale down

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 openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels May 14, 2020
@elmiko
Copy link
Contributor Author

elmiko commented Jun 15, 2020

added the cluster autoscaler event watcher into these tests. this is borrowed from the previous scale up/down test but it is added in a way to be automatic for tests in the same Context block.

i'm adding this because it has been invaluable in debugging issues with test failures.

@elmiko
Copy link
Contributor Author

elmiko commented Jun 16, 2020

/retest

Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@elmiko
Copy link
Contributor Author

elmiko commented Jun 16, 2020

/retest

@enxebre
Copy link
Member

enxebre commented Jun 17, 2020

/retest
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2020
@openshift-bot
Copy link

/retest

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

@elmiko
Copy link
Contributor Author

elmiko commented Jun 17, 2020

/retest

1 similar comment
@elmiko
Copy link
Contributor Author

elmiko commented Jun 17, 2020

/retest

@elmiko
Copy link
Contributor Author

elmiko commented Jun 17, 2020

/test e2e-azure-operator

@openshift-bot
Copy link

/retest

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

This change adds a new test to the autoscaler suite for detecting
annotations that are left on Machines after a scale down to minimum
size. To accomplish this, several larger changes were added to the
suite.

One of the larger changes is that `Context` blocks have been added
around the 2 major tests cases. These blocks represent the different
ClusterAutoscalers that are used with each testing group. The first test
group, namely the "scale up and down" suite, has been left untouched.
The second test group, with "scale to/from zero", has been changed to
make the ClusterAutoscaler management part of the Before/After clauses
for each test.

This test is related to bugzilla case 1820410. reference:
https://bugzilla.redhat.com/show_bug.cgi?id=1820410
@elmiko elmiko force-pushed the test-for-deletion-annotation branch from 9023238 to bc47cd7 Compare June 17, 2020 20:28
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2020
@elmiko
Copy link
Contributor Author

elmiko commented Jun 17, 2020

rebase to account for other changes.

@elmiko
Copy link
Contributor Author

elmiko commented Jun 17, 2020

/retest

1 similar comment
@elmiko
Copy link
Contributor Author

elmiko commented Jun 18, 2020

/retest

Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 18, 2020
@elmiko
Copy link
Contributor Author

elmiko commented Jun 18, 2020

/retest

@openshift-bot
Copy link

/retest

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

@elmiko
Copy link
Contributor Author

elmiko commented Jun 18, 2020

/retest

3 similar comments
@elmiko
Copy link
Contributor Author

elmiko commented Jun 18, 2020

/retest

@elmiko
Copy link
Contributor Author

elmiko commented Jun 18, 2020

/retest

@elmiko
Copy link
Contributor Author

elmiko commented Jun 19, 2020

/retest

@openshift-bot
Copy link

/retest

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

@elmiko
Copy link
Contributor Author

elmiko commented Jun 19, 2020

/retest

@openshift-merge-robot openshift-merge-robot merged commit c4f7e54 into openshift:master Jun 19, 2020
@elmiko elmiko deleted the test-for-deletion-annotation branch June 19, 2020 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants