allow tests to run back to back #372

KauzClay · 2022-02-04T23:57:26Z

Fixes #371

I would like to be able to run the e2e tests multiple times without cleaning everything up. Particularly, I don't want to have to recreate the vcsim.

From what I have seen, vcsim will remember the state of things. So if in the source test, we power off two vms, when we run again and try to power off those same vms, nothing will happen. No events will be sent, and the test will fail.

Same thing happens for the TestBindingGOVC. The tag and category will already exist, so those commands won't prompt any new events.

Proposed Changes

🧹 Update or clean up current behavior
added helper functions to create/cleanup vcsim and secret
use those helper functions in the tests
added a checkpoint configmap to the CreateSource function. This will make sure the source goes back in time for events in case the test events happen before the source is ready.
improved cleanup functions to also clean up completed job pods associated with listener and govc jobs

Pre-review Checklist

At least 80% unit test coverage (I believe this does not apply, since I am modifying a test)
E2E tests for any new behavior
Docs for any user-facing impact (I don't believe there is any user-facing impact)

Release Note

N/A no user-facing impact

codecov-commenter · 2022-02-05T00:01:28Z

Codecov Report

Merging #372 (9a7424b) into main (abcb2be) will increase coverage by 0.19%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #372      +/-   ##
==========================================
+ Coverage   83.04%   83.23%   +0.19%     
==========================================
  Files          27       27              
  Lines        1032     1032              
==========================================
+ Hits          857      859       +2     
+ Misses        147      146       -1     
+ Partials       28       27       -1

Impacted Files	Coverage Δ
pkg/vsphere/adapter.go	`64.12% <0.00%> (+1.52%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update abcb2be...9a7424b. Read the comment docs.

embano1 · 2022-02-05T08:02:22Z

QQ: shouldn't we actually do cleanup after test, eg delete deployments, instead of manually resetting state?

KauzClay · 2022-02-09T17:14:34Z

hi @embano1 , are you suggesting that the vcsim deployment be applied/deleted as part of the test too?

As far as I can tell, the vcsim deployment isn't applied as part of the test. You have to deploy it manually before running the e2e, and therefore, delete it manually too if you want to rerun

embano1 · 2022-02-09T19:22:51Z

hi @embano1 , are you suggesting that the vcsim deployment be applied/deleted as part of the test too?

Well, IMHO that would fix your problem and avoid sharing state between runs.

As far as I can tell, the vcsim deployment isn't applied as part of the test. You have to deploy it manually before running the e2e, and therefore, delete it manually too if you want to rerun

True, this repo still sets it up outside of the Go test framework. Two solutions:

add a step in the workflow to delete any state, e.g. vcsim
update the E2E tests to also manage the lifecycle of vcsim (which I typically do in other projetcts)

KauzClay · 2022-02-10T15:27:25Z

okay great, that sounds good then.

update the E2E tests to also manage the lifecycle of vcsim (which I typically do in other projetcts)

~~do you have an example of where you do this elsewhere?~~ Is this a good example?

* it seems like when running the test, the events are being sent to vcsim before the vsphere source is ready. This means the source misses the events, and the test fails * apparently, there is a default checkpoint window of 5 mins. However, that doesn't seem to be applying. This may need separate investigation * creating a checkpoint configmap ourselves allows us to guarantee the source will ask vcsim for events from the past N minutes

* when the tests run, there are two lefover pods for the listener job and the govc job. * this commit cleans those up, so upon finishing the test, all resources are cleaned up from the cluster

* now that e2e tests create and manage vcsim and secret, setup doesn't need to do it

embano1

While reviewing this PR I saw that RunJobListener() calls pkgtest.CleanupOnInterrupt twice in the function. IMHO this is a bug if you look at how pkgtest.CleanupOnInterrupt works internally. Can you PTAL?

And in general I think the way we use pkgtest.CleanupOnInterrupt across the tests might not achieve the goal since multiple goroutines are registered, blocking for a signal and then racing to call os.Exit(1)

test/e2e/util.go

embano1 · 2022-02-18T09:53:55Z

test/e2e/util.go

+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{{
+						Name:  vcsim,
+						Image: "vmware/vcsim:latest",


non-blocking: if we see docker.io 429s we can always fall back to using ko: to build vcsim since it's already a vendored dependency.

KauzClay · 2022-02-22T17:42:58Z

@embano1 I addressed the nits, hopefully to what you were expecting :)

Also, with regard to this comment...

While reviewing this PR I saw that RunJobListener() calls pkgtest.CleanupOnInterrupt twice in the function. IMHO this is a bug if you look at how pkgtest.CleanupOnInterrupt works internally. Can you PTAL?

And in general I think the way we use pkgtest.CleanupOnInterrupt across the tests might not achieve the goal since multiple goroutines are registered, blocking for a signal and then racing to call os.Exit(1)

The way I'm reading it, it seems like calling it multiple times is okay. It looks like cleanup.funcs = append(cleanup.funcs, cleanupFunc) is just adding them to the slice of functions.

Initially I got hung up on the cleanup.once.Do(waitForInterrupt) line, but I believe the once.Do() means that every call after that will skip over that line, so I think there is only 1 goroutine actually blocking on a call to os.Exit(1).

I tested it out here: https://gist.github.com/KauzClay/abf3fe664e6867bca90dc16c8a11f594
And this is what I saw:

❯ go run main.go      
doing stuff
sleeping, cancel me
^CI am cleanup function 3
I am cleanup function 2
I am cleanup function 1
exit status

embano1

LGTM, thx mate!

And regarding CleanupOnInterrupt: I don't know which code I looked at when I wrote this. Just checked src again and it definitely does not call exit more than once. All good 👍

gabo1208 · 2022-02-22T19:28:27Z

Let me know if this is ready to review al lgtm :)

* add helper for creating vcsim * create vcsim in source test * manually create checkpoint * it seems like when running the test, the events are being sent to vcsim before the vsphere source is ready. This means the source misses the events, and the test fails * apparently, there is a default checkpoint window of 5 mins. However, that doesn't seem to be applying. This may need separate investigation * creating a checkpoint configmap ourselves allows us to guarantee the source will ask vcsim for events from the past N minutes * remove dead code * create vcsim in all tests * clean up job pods * when the tests run, there are two lefover pods for the listener job and the govc job. * this commit cleans those up, so upon finishing the test, all resources are cleaned up from the cluster * run update-codegen.sh * update git workflow * now that e2e tests create and manage vcsim and secret, setup doesn't need to do it * refactor: address nits * refactor: make context a variable

KauzClay requested a review from a team as a code owner February 4, 2022 23:57

KauzClay added 3 commits February 11, 2022 13:39

add helper for creating vcsim

45596ba

create vcsim in source test

7a9d50f

KauzClay force-pushed the ck-run-e2e-test-b2b branch from d520e6a to a5461c7 Compare February 11, 2022 18:45

KauzClay added 5 commits February 11, 2022 13:48

remove dead code

fa749c1

create vcsim in all tests

aed8a92

clean up job pods

5a651ea

* when the tests run, there are two lefover pods for the listener job and the govc job. * this commit cleans those up, so upon finishing the test, all resources are cleaned up from the cluster

run update-codegen.sh

c166b11

update git workflow

9a7424b

* now that e2e tests create and manage vcsim and secret, setup doesn't need to do it

embano1 reviewed Feb 18, 2022

View reviewed changes

refactor: address nits

99dc5e1

refactor: make context a variable

5d77413

KauzClay force-pushed the ck-run-e2e-test-b2b branch from 4b5d52d to 5d77413 Compare February 22, 2022 17:48

embano1 approved these changes Feb 22, 2022

View reviewed changes

gabo1208 approved these changes Feb 28, 2022

View reviewed changes

gabo1208 merged commit 8c4047c into vmware-tanzu:main Feb 28, 2022

KauzClay mentioned this pull request Feb 28, 2022

cherry-pick: (#372) for v0.27.x #379

Closed

3 tasks

gabo1208 mentioned this pull request Feb 28, 2022

Release 1.0 #380

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow tests to run back to back #372

allow tests to run back to back #372

KauzClay commented Feb 4, 2022 •

edited

Loading

codecov-commenter commented Feb 5, 2022 •

edited

Loading

embano1 commented Feb 5, 2022

KauzClay commented Feb 9, 2022

embano1 commented Feb 9, 2022

KauzClay commented Feb 10, 2022 •

edited

Loading

embano1 left a comment

embano1 Feb 18, 2022

KauzClay commented Feb 22, 2022

embano1 left a comment

gabo1208 commented Feb 22, 2022

allow tests to run back to back #372

allow tests to run back to back #372

Conversation

KauzClay commented Feb 4, 2022 • edited Loading

Proposed Changes

Pre-review Checklist

codecov-commenter commented Feb 5, 2022 • edited Loading

Codecov Report

embano1 commented Feb 5, 2022

KauzClay commented Feb 9, 2022

embano1 commented Feb 9, 2022

KauzClay commented Feb 10, 2022 • edited Loading

embano1 left a comment

Choose a reason for hiding this comment

embano1 Feb 18, 2022

Choose a reason for hiding this comment

KauzClay commented Feb 22, 2022

embano1 left a comment

Choose a reason for hiding this comment

gabo1208 commented Feb 22, 2022

KauzClay commented Feb 4, 2022 •

edited

Loading

codecov-commenter commented Feb 5, 2022 •

edited

Loading

KauzClay commented Feb 10, 2022 •

edited

Loading