-
Notifications
You must be signed in to change notification settings - Fork 876
Cirrus: use test-integration target #2725
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
base: main
Are you sure you want to change the base?
Conversation
|
For the record: this is motivated by the discussion in #2701 . Note #2701 (comment) : right now it seems that we have to choose between test coverage and meaningful containerization. Also, from a quick comparison, this seems to add 2-3 minutes (~20 %) to test runtime. That’s looking at one test run (vs. 169091e ), it might well be a random flake — but if not, it’s a bit annoying. |
Signed-off-by: Lokesh Mandvekar <[email protected]>
cac2fda to
8c4af36
Compare
Signed-off-by: Lokesh Mandvekar <[email protected]>
|
RE: #2701 (comment) , I've added another commit to run the container with cap_sys_admin and now it works. Thanks. Given there are set of container tests that are skipped in So, unless the openshift setup is made convenient, maybe let's just continue with skopeo_cidev. I'll check if the openshift binary used in openshift tests can be replaced with the newer oc binary. It's inconvenient that they don't ship it via an rpm AFAIK. If this is good enough for merge, we should only merge whenever containers/automation_images#419 (or whatever PR that change is added to) is ready to merge. |
8ef62df to
f1ef1bf
Compare
|
LGTM |
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.
What is the principal goal of this PR?
RE: #2701 (comment) , I've added another commit to run the container with cap_sys_admin and now it works. Thanks.
Given there are set of container tests that are skipped in
-local, I prefer we testtest-integrationin CI until we get to rewriting tests without container.
If this is referring to #2701, they are skipped in -local without SKOPEO_CONTAINER_TESTS. That makes -local ~useless for local development, sure, but I don’t see that it makes that much difference for CI (and -local was already ~useless for local development due to the need for specialized registry / OpenShift binaries).
I think adding cap_add_sysadmin to the test-integration target breaks containerization (or, at least, from the users’ point of view risks breaking containerization) enough that I don’t think users should want to run it locally either…
Overall, both before and after this PR, there would be no good way for an external risk-conscious contributor to run tests locally.
So that leaves the CI situation: for that, this PR adds a layer of indirection to the runtime environment, and (probably) a few extra minutes to the runtime; is there a benefit to doing this?
Hypothetically, removing the need for cap_add_sysadmin from test-integration, even at the cost of somewhat decreasing the test coverage, might be a true improvement for external contributors. But then… in CI, we would want to exercise those code paths (maybe not skipping the tests if SKOPEO_CONTAINER_TESTS is set?).
So the two ways to run CI would not be exactly the same, either… Is that the goal of the PR?
I do think it would make some sense to make the test-integration code and the CI “local in VM” code much more similar — the way we copy out binaries out of a container image in one of these is surprising. Maybe we can build a container image and a VM with ~the same contents? I’m not sure how effort that would be.
But, right now, this PR makes the CI and out-of-CI code paths similar at the cost of making the out-of-CI path too risky to use. Doesn’t that, mostly, defeat the point?
I'll check if the openshift binary used in openshift tests can be replaced with the newer oc binary.
That’s extremely unlikely. For here, we want
- a very old version of OpenShift which still provides the older version of the signature API used by
atomic:; anything newer changed the HTTP API paths. (Theatomic:transport is therefore no longer useful in practice, and dropping theatomic:transport itself would be fine — it’s only that our signing tests are mostly built around that transport, and we need the test coverage of the other signing features, so we’d need to migrate tests first.) - … and a version of OpenShift that can run just as 2-3 processes within a single container, without having to set up a ~true VM cluster with networking and CRI-O on each node and all that. That was possible in those old versions (“all-in-one” binary), and IIRC that feature just isn’t there any longer.
No description provided.