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

test/browser: run tests inside the tasks container #1628

Merged

Conversation

allisonkarlitskaya
Copy link
Member

This simplifies the "outside" setup quite a bit and gives us the same tasks container that test runs on the Cockpit CI run under.

@allisonkarlitskaya
Copy link
Member Author

This is going to need a fairly extensive treatment of finding all of the places we make exceptions for the -ws container on ostree images and extending them to include exceptions for testing farm. Probably we want to add a new API for that...

@martinpitt
Copy link
Member

@allisonkarlitskaya As a data point: RHEL8 has systemd-nspawn (in the systemd-container package). However, nspawn can't directly run {oci,docker}-{archive,directory}. It is possible to let podman construct the file system into a flat tree and run that, but it's a lot of overhead especially for the large tasks container:

mkdir /var/tmp/nspawntest
podman export "$(podman create --name nspawntest localhost/test-busybox true)" | tar -x -C /var/tmp/nspawntest
systemd-nspawn -D /var/tmp/nspawntest/

(I didn't specify any of the network options for this)

@martinpitt
Copy link
Member

Treating the -ws container is already awkward, but we can get away with it because it can be killed and restarted for each test. We don't have that option for the tasks container. Given how brittle the clean up is already, I have a pretty strong feeling that we can't do robust podman tests and at the same time rely on that podman container -- so using docker or possibly nspawn or even just chroot feels more promising here.

@martinpitt martinpitt marked this pull request as draft March 18, 2024 09:59
@allisonkarlitskaya
Copy link
Member Author

Treating the -ws container is already awkward, but we can get away with it because it can be killed and restarted for each test. We don't have that option for the tasks container. Given how brittle the clean up is already, I have a pretty strong feeling that we can't do robust podman tests and at the same time rely on that podman container -- so using docker or possibly nspawn or even just chroot feels more promising here.

💯

Same basic justification as what I wrote in the commit message:

We have to deviate from the starter-kit template here: the tests have setup/teardown logic which wipes the podman slate completely clean. We can't possibly survive that, and trying to carve out an exemption would be difficult and would cut against the spirit of the deep clean that we're trying for. This isn't a problem for the cockpit-ws container, since it gets started after the tests start running.

We can avoid all that by just running the tasks container in docker.

@martinpitt
Copy link
Member

Ah right -- I had blindly assumed the "all tests red" was still due to the podman conflict, but it's "just" testCreateContainerValidation now

@jelly
Copy link
Member

jelly commented Mar 18, 2024

Ah right -- I had blindly assumed the "all tests red" was still due to the podman conflict, but it's "just" testCreateContainerValidation now

See #1624 I suspected I created a flake here.

@martinpitt
Copy link
Member

The "pin tasks container" commit already landed, and now the flake fix #1624 landed. So I rebased this.

@martinpitt
Copy link
Member

martinpitt commented Mar 18, 2024

hmm, "Failed to start docker.service: Unit docker.service not found." in c8s/c9s log

lol:

no package provides docker
[...]
Installed:
 podman-docker-3:4.6.1-5.module_el8+712+4cd1bd69.noarch   

I tried https://docs.docker.com/engine/install/centos/ , but it's not co-installable with podman unfortunately:

yum-config-manager --add-repo https://download.docker.com/linux/centos/docker-ce.repo
dnf install docker-ce
[..]
package containerd.io-1.6.28-3.2.el8.x86_64 from docker-ce-stable conflicts with runc provided by runc-1:1.1.4-1.module_el8.7.0+1196+721f4eb0.x86_64 from appstream

(and hundreds more)

@martinpitt
Copy link
Member

I updated the docker approach comment above a little bit, and that looks like a dead end. So I had another look at nspawn:

mkdir /var/tmp/tasks
podman export "$(podman create --name tasks-import ghcr.io/cockpit-project/tasks)" | tar -x -C /var/tmp/tasks
podman rm tasks-import
podman rmi ghcr.io/cockpit-project/tasks

# disable seccomp, see https://bugzilla.redhat.com/show_bug.cgi?id=2040247
SYSTEMD_SECCOMP=0 systemd-nspawn -D /var/tmp/tasks/ --ephemeral --user user

I'll add a fixup commit here to play around with this a little bit.

@martinpitt martinpitt force-pushed the tf-tasks branch 2 times, most recently from 35ddf5e to 20188b7 Compare March 18, 2024 15:12
@martinpitt
Copy link
Member

The rawhide errors are unrelated:

Error: pasta failed with exit code 1:
netlink: Unexpected sequence number (6 != 10)

At least that's now slightly different than the previous complete FUBAR in containers/podman#22052 , but we can continue to ignore this on our side. I'll report it to podman/pasta ASAP.

This simplifies the "outside" setup quite a bit and gives us the same
tasks container that test runs on the Cockpit CI run under.

We have to deviate from the starter-kit template here: the tests have
setup/teardown logic which wipes the podman slate completely clean.  We
can't possibly survive that, and trying to carve out an exemption would
be difficult and would cut against the spirit of the deep clean that
we're trying for.  This isn't a problem for the cockpit-ws container,
since it gets started after the tests start running.

We tried to avoid that by using docker, but it caused other issues.
systemd-nspawn seems to be a good approach here.

Co-authored-by: Martin Pitt <[email protected]>
Copy link
Member Author

@allisonkarlitskaya allisonkarlitskaya left a comment

Choose a reason for hiding this comment

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

I mean, I'm not thrilled about it, but it seems to work. I'm happy to land it in its current state.

@allisonkarlitskaya allisonkarlitskaya marked this pull request as ready for review March 18, 2024 17:26
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Ack, let's go with that then. It's at least closer to the new paradigm than keeping the old setup.

@allisonkarlitskaya allisonkarlitskaya merged commit 8b55b3e into cockpit-project:main Mar 18, 2024
29 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants