-
Notifications
You must be signed in to change notification settings - Fork 6
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
test/e2e: Run in parallel #76
Conversation
@maiqueb looking at the e2e test failing, it seems like prior (and also after) to migration the primary UDN virt-launcher pod is not getting the appropriate network-status (logs):
Can we attribute this issue to the OVNK race you talked about? OR is it a new issue we need to check? |
It could be. You'll need to check the logs of the ovnkube-control plane and ensure that for that pod it failed to find the primary UDN network to be sure. |
I don't see any pod fails, but are you sure we should find a "pod fail"? I mean, it did find a primary network, it was just the wrong one.. In any case, this is very helpful, I've also asked on the race bug to know more information.. |
So that's what you need to find: when it returns the active network for the namespace, which one did it find ? It should have found the primary UDN, not the default one. Can you print the relevant log snippet ? |
After some digging on OVNK, I realized the network is being reconciled. The reason is that the network name in the tests is the same. In order to separate the test and run them in parallel - I changed the network names to be unique. |
8e794c9
to
127686b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems OK to me, but I wonder if / how we can still indicate which tests we want to run.
Makefile
Outdated
export KUBECONFIG=${KUBECONFIG} && \ | ||
export PATH=$$(pwd)/.output/ovn-kubernetes/bin:$${PATH} && \ | ||
export REPORT_PATH=$$(pwd)/.output/ && \ | ||
cd test/e2e && \ | ||
go test -test.v --ginkgo.v --test.timeout=${E2E_TEST_TIMEOUT} ${E2E_TEST_ARGS} --ginkgo.junit-report=$${REPORT_PATH}/test-e2e.junit.xml | ||
$(GINKGO) -v --timeout=${E2E_TEST_TIMEOUT} --junit-report=$${REPORT_PATH}/test-e2e.junit.xml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how will the user specify a test subset to run ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I re-introduced the E2E_TEST_ARGS flag.
DONE
Change: re-introduced the E2E_TEST_ARGS flag. |
Moving to use ginkgo tool instead of go test. This is done in order to use the parallel ginkgo parameter that is not supported on "go test" tool. Signed-off-by: Ram Lavi <[email protected]>
The e2e tests currently randomize the namespaces, but the network name inside the NADs is the same, creating dependency between the tests. In order to runthe tests in parallel, a change that will be introduced in future commit - randomizing the network names as well. Signed-off-by: Ram Lavi <[email protected]>
Change: Rebase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RamLavi did you check this does the trick locally ?
No way it shouldn't afaict, but still, please paste an example of how to invoke it both in the appropriate commit msg and in the PR's description.
Thank you !
I run it on zeus33. good enough right?
The usage is the same: The way it works is the that when the parallel flag is set, it auto-finds how many threads it can open, and runs with it. On Zeus33 - it's 10+ threads, so the CI takes ~3m. On github CI, it's more like ~10m. |
I mean regarding the flag. If you want to filter some tests, do you need to use a different flag ? Can you check locally if you need to ? I have the slight recollection it isn't exactly the same command. BUT I might be wrong. @qinqon could you chime in here ? Thanks in advance. |
Using the general
but that's not a new env var. |
Sure, but for us to keep it's current behavior, the value in it must now be different. All good. Could you add this Then, lgtm / approve / merge :) |
In order to manage failed tests artifacts, the process# is added to the log file name. e2e run example: `E2E_TEST_ARGS='--focus="should keep ips after live migration"' make test-e2e` Signed-off-by: Ram Lavi <[email protected]>
Change: Updated commit desc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
Thank you.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: maiqueb 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:
Approvers can indicate their approval by writing |
/hold |
/hold cancel |
What this PR does / why we need it:
This PR is running the e2e tests in parallel, as tests should be independent of each other.
The amount of threads depends on the number of CPUs available in the machine running the CI.
Running it on the PR should increase CI time, and potentially expose bugs.
e2e run example:
E2E_TEST_ARGS='--focus="should keep ips after live migration"' make test-e2e
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 #
Special notes for your reviewer:
Release note: