Running tests using external binary #27570
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
/test e2e-gcp-ovn |
openshift-ci
bot
added
vendor-update
soltysh
force-pushed
the
test_poc
branch
3 times, most recently
from
0f89bfc to
b25220d
Compare
soltysh
changed the title
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
soltysh
force-pushed
the
test_poc
branch
3 times, most recently
from
3a7f490 to
387b0a7
Compare
soltysh
force-pushed
the
test_poc
branch
2 times, most recently
from
d7e5552 to
b141fdf
Compare
|
/test e2e-gcp-ovn |
soltysh
force-pushed
the
test_poc
branch
4 times, most recently
from
58c2ed4 to
b3d41c3
Compare
|
/retest |
|
/retest |
| isKubeNamespace := upgradeFilter.MatchString(baseName) || // 1. | ||
| (isGoModulePath(ginkgo.CurrentSpecReport().FileName(), "k8s.io/kubernetes", "test/e2e") && !skipTestNamespaceCustomization()) // 2. | ||
| return e2e.CreateTestingNS(ctx, baseName, c, labels, isKubeNamespace) | ||
| } |
There was a problem hiding this comment.
This looks way simpler than the createTestingNS function (which is great), but it seems to behave differently. We don't need the old behavior anymore?
There was a problem hiding this comment.
The behavior is roughly similar, the e2e.CreateTestingNS is in o/k because we also need explicitly set the namespaces as privileged for all k8s tests. For origin tests we've identified two cases described in this file:
- when we run the embedded k8s tests (the path check)
- when we run the upgrade tests (those will be always embedded, based on my recent findings).
I'm planning to explicitly call it out these modifications for namespaces in openshift/enhancements#1291 to ensure this is more widely known that it is today.
| if err != nil { | ||
| errs = append(errs, err) | ||
| } | ||
| tests = append(tests, tc) |
There was a problem hiding this comment.
I know this logic was copied from the previous revision, but is it correct? Should it append tc even on error?
There was a problem hiding this comment.
Yes, I tried failing hard (during gingkgo v2 bump) this and it broke CSI tests which are generated on the fly. That's why we report errors, but still continue with adding tests. In the worst case we'll run the test w/o annotation, which is better than not.
| // Don't build the tree multiple times, it results in multiple initing of tests | ||
| if !ginkgo.GetSuite().InPhaseBuildTree() { | ||
| ginkgo.GetSuite().BuildTree() | ||
| } |
| fmt.Fprintf(opt.Out, "Falling back to built-in suite, failed reading external test suites: %v\n", err) | ||
| } | ||
| } else { | ||
| fmt.Fprintf(opt.Out, "Using built-in tests only due to OPENSHIFT_SKIP_EXTERNAL_TESTS being set\n") |
There was a problem hiding this comment.
Do we want to allow jobs to opt-out? When could this be useful?
There was a problem hiding this comment.
We don't have that embedded anywhere in CI, but I can think of a few cases like microshift, local dev or for vendors to certificate their OCP. I think I've covered that in the enhancement, if not I'll add it ther.
Lastly, even though we currently don't use it in CI, there are some possible places where this could in the future.
| var tests []*testCase | ||
|
|
||
| // TODO: add support for binaries from other images | ||
| testBinary, err := extractBinaryFromReleaseImage("hyperkube", "/usr/bin/k8s-tests") |
There was a problem hiding this comment.
How can I use my custom k8s-tests binary?
|
|
||
| // externalTestsForSuite reads tests from external binary, currently only | ||
| // k8s-tests is supported | ||
| func externalTestsForSuite(ctx context.Context) ([]*testCase, error) { |
There was a problem hiding this comment.
Nit: this adds a few seconds to the runtime
There was a problem hiding this comment.
Correct, up to 10s from what I was testing. But that's also the reason why we have that envvar to opt-out of it.
| } | ||
|
|
||
| if len(os.Getenv("OPENSHIFT_SKIP_EXTERNAL_TESTS")) == 0 { | ||
| fmt.Fprintf(opt.Out, "Attempting to pull tests from external binary...\n") |
There was a problem hiding this comment.
When I run the CSI test suite, this message gets printed correctly. But when I run a single test from the CSI test suite (i.e., run-test option) I don't see this message at all.
There was a problem hiding this comment.
Correct, this only applies to openshift-test run, ie. when running the whole suite.
|
/payload 4.14 ci informing |
|
@soltysh: trigger 4 job(s) of type informing for the ci release of OCP 4.14
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/ec0f88e0-ff24-11ed-8863-e0dd5781a139-0 trigger 55 job(s) of type informing for the nightly release of OCP 4.14
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/ec0f88e0-ff24-11ed-8863-e0dd5781a139-1 |
|
/retest |
Currently we support two binaries: 1. openshift-tests one, which is the current way how to run tests 2. k8s-tests, which allows running k8s native tests All the variables required to run tests are injected through environment variables.
Internal connectivity test relied on an exception for setting the namespace privileged. This removes the exception, explicitly setting up the namespace as required in the test.
|
|
||
| // extractBinaryFromReleaseImage is responsible for resolving the tag from | ||
| // release image and extracting binary, returns path to the binary or error | ||
| func extractBinaryFromReleaseImage(tag, binary string) (string, error) { |
There was a problem hiding this comment.
Instead of pulling the hypershift image and extracting the binary from it, would it be possible for ART to make the binary available in the tests image as well?
There was a problem hiding this comment.
Also, it might be useful to have an environment variable to override the binary.
There was a problem hiding this comment.
IIRC we discussed several options, and this was the simplest and quickest option to start the work. Also the simplest from a flow point of view. We can always revisit this bit in the future.
|
/retest-required |
|
@soltysh: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/label jira/valid-bug |
openshift-ci
bot
added
the
jira/valid-bug
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neisw, soltysh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
6 participants
It has to go in pair with openshift/kubernetes#1485
Still TODO:
Write-up in openshift/enhancements#1291