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

Standardize on OCI images in test-suite #2733

Merged
merged 13 commits into from Oct 31, 2023

Conversation

dmnks
Copy link
Contributor

@dmnks dmnks commented Oct 24, 2023

This replaces per-distro backends (currently only mktree.fedora) with a universal OCI-based backend. It's based on the original mktree.podman but enhanced to work for native development, too. Related refactorings are included.

See the commit messages for details.

Fixes: #2643

@dmnks dmnks marked this pull request as draft October 24, 2023 12:42
@dmnks dmnks mentioned this pull request Oct 24, 2023
8 tasks
@dmnks dmnks marked this pull request as ready for review October 24, 2023 12:48
@dmnks
Copy link
Contributor Author

dmnks commented Oct 24, 2023

OK, fixed a few typos and thinkos in the commit messages 😄

@pmatilai
Copy link
Member

Hmm. I'm getting a LOT of failures when running this as "make check" locally on F38, before and after doing an 'rm -rf *' of the entire build tree:

Subject: [rpm 4.19.90] rpmtests: 5 116 117 149 165 297 298 401 403 404 407 408 413 431 434 435 436 437 438 439 440 442 443 444 445 446 447 448 449 450 451 452 477 478 479 480 481 482 483 484 485 486 487 488 489 490 491 492 493 failed

Stuff like:

+++ /srv/rpmtests.dir/at-groups/5/stderr 2023-10-26 07:20:24.427086905 +0000
@@ -0,0 +1,4 @@
+ls: cannot access 'librpm.so..': No such file or directory
+ls: cannot access 'librpmbuild.so..': No such file or directory
+ls: cannot access 'librpmio.so..': No such file or directory
+ls: cannot access 'librpmsign.so..': No such file or directory

+++ /srv/rpmtests.dir/at-groups/116/stderr 2023-10-26 07:20:26.007099877 +0000
@@ -0,0 +1,3 @@
+bwrap: execvp /usr/lib/rpm/rpmuncompress: No such file or directory
+tar: This does not look like a tar archive
+tar: Exiting with failure status due to previous errors

Hmm okay, that happens when you build with -DCMAKE_INSTALL_PREFIX=/usr - I build that way because it allows me to use the devel version as if it was the system rpm. So something here clashes with the idea of placing rpm where the system version was.

Another random observation: mktree.docker seems to be suffering from a minor schizophrenia, because it mostly talks about Podman inside 😅 Maybe it should be mktree.oci instead?

@dmnks
Copy link
Contributor Author

dmnks commented Oct 26, 2023

Hmm. I'm getting a LOT of failures when running this as "make check" locally on F38, before and after doing an 'rm -rf *' of the entire build tree:

Ugh, I haven't tested the non-default (/usr/local) prefix... This is why PRs are actually valuable, it seems like 😅 Of course it has to work with any prefix, that was the requirement from the start (and seems like the current Fedora backend does work). I'll have a look, thanks!

Another random observation: mktree.docker seems to be suffering from a minor schizophrenia, because it mostly talks about Podman inside 😅 Maybe it should be mktree.oci instead?

The symlink really is meant as just a silly "shortcut" for something like (in the CI):

USE_DOCKER=1
./mktree.podman ...

But I agree, mktree.oci was also on my mind, it's perhaps more "correct" as it says "make an OCI-based test tree" (much like e.g. the mkfs.* binaries). Let me change it then 😄

@dmnks
Copy link
Contributor Author

dmnks commented Oct 26, 2023

BTW, we could just use Podman in the CI now, too. The Ubuntu version deployed there is recent enough to have Podman in a recent-ish version, too, and is even installed by default.

The only "advantage" (or rather a side effect) of Docker is that it must run with root privileges, in which case we get that one test covered which is normally disabled in unprivileged containers because it needs to create devices (or something like that, don't remember exactly). But we could do the same with Podman, I believe, just running it as root in that VM.

@dmnks
Copy link
Contributor Author

dmnks commented Oct 26, 2023

Hmm, so I just tried a build with -DCMAKE_INSTALL_PREFIX=/usr and it worked fine 😕

@dmnks
Copy link
Contributor Author

dmnks commented Oct 26, 2023

Could you please try a completely new build in a separate build dir? Just to rule out any local build configuration.

@dmnks
Copy link
Contributor Author

dmnks commented Oct 26, 2023

Oh, you mentioned you recreated the entire build tree (not just tests/), nevermind then... mystery deepens 😆

@pmatilai
Copy link
Member

Hmm, I'll retest after lunch. I also have -DRPM_VENDOR=redhat in my build script, and I probably took out that too 🤔

@pmatilai
Copy link
Member

I guess you need to do that 'rm -rf *' of the build tree for the prefix to really take effect - that way I can reliably reproduce the failures.

Looking at the build log makes it kinda obvious what is going on. With a -DCMAKE_INSTALL_PREFIX=/usr build, the build inside the image ends up installing to the default prefix:

Install the project...
-- Install configuration: ""
-- Installing: //usr/local/share/man/man1/gendiff.1
-- Installing: //usr/local/share/man/man8/rpm2cpio.8
-- Installing: //usr/local/share/man/man8/rpm.8
-- Installing: //usr/local/share/man/man8/rpmbuild.8

...but then the build itself is expecting to be in /usr prefix due to the "outside" configuration, and at that point its no wonder it fails.

@dmnks
Copy link
Contributor Author

dmnks commented Oct 26, 2023

I actually did precisely that, removed the whole build dir and created it from scratch, and yet, it works 😆 But, one important difference perhaps is that I did not configure anything besides the prefix, i.e:

$ mkdir _build
$ cd _build/
$ cmake -DCMAKE_INSTALL_PREFIX=/usr ..
$ make check

Did you use some other cmake options too?

@dmnks
Copy link
Contributor Author

dmnks commented Oct 26, 2023

OK, tried with a couple more config options, namely:

export CFLAGS="-Og -g"

cmake \
	-DCMAKE_INSTALL_PREFIX=/usr \
	-DENABLE_WERROR=ON \
	-DENABLE_BDB_RO=ON \
	-DWITH_FSVERITY=ON \
	-DWITH_IMAEVM=ON \
	-DWITH_FAPOLICYD=ON \
	-DRPM_VENDOR=redhat \
	$*

... and it worked for me, too. Strange, strange...

@pmatilai
Copy link
Member

Okay with offline debugging with @dmnks , the difference came from me not having buildah installed, and cmake reporting

-- Using mktree backend: podman (native: OFF)

After installing buildah that changes to ON, and the test-suite passes. So the apparent configuration leakage is an issue only in the non-native mode.

@dmnks
Copy link
Contributor Author

dmnks commented Oct 26, 2023

Indeed, sorry for not pointing that (new requirement) out in the beginning, somehow that didn't even cross my mind 😄

I can now reproduce this in non-native mode with a different prefix. Something is leaking into the image even if it shouldn't. Will fix.

Meanwhile, I'm thinking if we shouldn't just use Podman to apply the RPM layer on top of the base layer, instead of Buildah. Podman allows that too, just needs one extra step or so.

@pmatilai
Copy link
Member

In general, less build dependencies is better but only if skipping buildah is truly trivial. I don't want us to sink yet more time into that.

@dmnks
Copy link
Contributor Author

dmnks commented Oct 26, 2023

Just FTR, culprit found, it's because the rpmtests.sh wrapper runs rpmtests from the local build directory if it exists, even in non-native mode. Working on a fix now.

Don't symlink rpmtests itself, we can run it directly from the
installation directory in the test tree.  This eliminates some extra
cd-ing.

No functional change.
This invokes an interactive shell in a dummy test where all the usual
test commands work, and is meant as a replacement for what mktree.fedora
currently does for its shell/atshell commands.  This feature basically
emulates a single test so it really belongs to the rpmtests wrapper, not
mktree.

For consistency, also do the permission fixup for rpmtests.dir (just
like in mktree.fedora), this will be needed once we drop mktree.fedora.
Log mode is mostly useful in the CI environment so don't enable it by
default.  This is another step towards the unified OCI backend.

No functional change.
This is another step towards the unified OCI backend, no functional
change yet.
Right now, we build and tag a new image on each mktree.podman run.  This
causes an unnecessary buildup of images over time that needs to be
manually cleaned with podman-image-prune(1).  Instead, tag the base
layer and then only rebuild the RPM layer.
Rename the existing Dockerfile and add a symlink for the original
filename.  This will allow for supporting other, non-Fedora, Linux
distros in the future and is a prerequisite for the next commits.
Instead of building RPM from scratch as part of the image, allow for
reusing the existing build artifacts on the host to produce the final
image.

This ensures we test the actual local build, not just the sources, and
speeds up the whole process, allowing it to be used for local
development as well.

No immediate effect, we'll hook this up to cmake in the next commit.
Instead of manually bootstrapping our own base "image" using a host
specific script, just use the official, prebuilt OCI images with
Podman/Docker.  This has several advantages:

- Standard, ubiquitous OCI images (easy support for other distros)
- No manual setup of DNF, RPM macros, user namespaces and whatnot
- Single recipe (Dockerfile) for both the local and CI purposes
- Outsourced image caching (Podman/Docker storage)
- Faster (just downloads the prebuilt image)
- Less dependencies on the host

Now that we've prepared mktree.podman for local use, just switch to it
in cmake and drop the Fedora backend.  Update the docs and comments
accordingly, too, those should explain the details.

Fixes: rpm-software-management#2643
Commit 09e4720 moved the snapshot
function to mktree.common so that we could later use it in non-cmake
mode (outside of the build directory) too, to containerize rpmtests
itself.  In the end, however, we've decided to just use Podman/Docker
for the outer container as it simplifies things.

Therefore, the snapshot function is no longer needed outside of the
test-suite, it's an internal implementation detail at this point which
doesn't have anything to do with mktree, so move it back to atlocal
where it really belongs.

As a bonus, the rpmtests wrapper can now be simplified a bit more, too.

No functional change.
These are bashism, not really needed.
Turns out the docker command in CI is set up such that it doesn't need
an explicit sudo, so remove it.  This will also allow us to easily pass
environment variables to mktree in the next commit (as otherwise we'd
have to use the ugly -E, --preserve-env option).

No functional change.
This name is a better fit as it puts more emphasis on the format of the
resulting tree, not as much on the actual container engine used (which
can be swapped, and currently is in the CI where we use docker instead
of podman).

Also drop the awkward .docker symlink now and instead pass the container
engine through the MKTREE_ENGINE environment variable.
@dmnks
Copy link
Contributor Author

dmnks commented Oct 27, 2023

Fixed version pushed:

  • Don't build local test files if we're in non-native mode (this fixes the above issue)
  • Drop the Buildah dependency (was trivial in the end)
  • Rename to mktree.oci
  • Remove the confusing mktree.docker symlink, use an env var instead
  • Minor fixups that I've found along the way

@pmatilai
Copy link
Member

Works for me 👍

@pmatilai pmatilai merged commit 05c3b37 into rpm-software-management:master Oct 31, 2023
1 check passed
@dmnks dmnks added the test Testsuite-related label Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Testsuite-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFE: Standardize on OCI images for test-suite, even locally
2 participants