Skip to content

Conversation

cgwalters
Copy link
Collaborator

@cgwalters cgwalters commented Sep 11, 2025

A key thing for me is that the Justfile should be a one-stop shop for development of the project. It can't have everything but it should answer the basic questions of "how do I build and test this project".

This aligns the recently added tmt-on-GHA flow a bit more closely with some of that. Biggest is to use the just build-integration-test-image as the canonical way to build a container image with our testing stuff in it; which uses our main Dockerfile

Other cleanups:

  • Change the scripts to accept data via argv[1] and not environment
  • Drop the hardcoded testing directory and use target/ as a generic build artifact dir

Closes: #1473

@bootc-bot bootc-bot bot requested a review from jeckersb September 11, 2025 18:05
@cgwalters cgwalters requested review from henrywang and removed request for jeckersb September 11, 2025 18:05
@cgwalters cgwalters force-pushed the testing-cleanups-p1 branch 2 times, most recently from 22ee1d8 to f195426 Compare September 12, 2025 11:27
cgwalters added a commit to cgwalters/bootc that referenced this pull request Sep 12, 2025
Over in bootc-dev#1607
I actually *just* deduplicated this code, but that isn't
ready to merge yet.
cgwalters added a commit to cgwalters/bootc that referenced this pull request Sep 12, 2025
Over in bootc-dev#1607
I actually *just* deduplicated this code, but that isn't
ready to merge yet.

Signed-off-by: Colin Walters <walters@verbum.org>
strategy:
matrix:
test_os: [fedora-41, fedora-42, fedora-43, centos-9]
test_runner: [ubuntu-latest, ubuntu-24.04-arm]
Copy link
Collaborator

@henrywang henrywang Sep 15, 2025

Choose a reason for hiding this comment

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

We do not run arm build test in github action runner? No nested virt support in github arm runner.

So only bootc build and bootc image build test can be run in github action in our case.

We can only keep x86_64 test running in github action, and run arm test on Packit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No nested virt support in github arm runner.

😢

We can only keep x86_64 test running in github action, and run arm test on Packit.

Hmmm...maybe yeah that's one way to dice the split.

I mean now, in reality, very very very little of the code is architecture sensitive and it's not clear to me we need to run all tests across both architectures across four operating system major versions...but trimming down that matrix is a whole other topic.

@cgwalters
Copy link
Collaborator Author

OK right, so moving the tmt stuff into a subdirectory just breaks packit. Argh! We're just really going to have to get that tmt+rsync bug fixed. I filed teemtee/tmt#4062

For now I'll go back to undoing the move and seeing if we can work around the tmt-rsync bug another way...

@cgwalters
Copy link
Collaborator Author

For now I'll go back to undoing the move and seeing if we can work around the tmt-rsync bug another way...

Hacked this by wrapping our tmt bits with a copy to a tempdir of the tests and running tmt from there.

@cgwalters
Copy link
Collaborator Author

Man, I lost way too much time to teemtee/testcloud#17

@cgwalters cgwalters force-pushed the testing-cleanups-p1 branch 9 times, most recently from 96bf82a to c4f092c Compare September 16, 2025 17:45
@cgwalters cgwalters force-pushed the testing-cleanups-p1 branch 4 times, most recently from a78b96d to 0cb60a4 Compare September 16, 2025 20:34
@cgwalters cgwalters marked this pull request as ready for review September 17, 2025 00:53
@bootc-bot bootc-bot bot requested a review from jeckersb September 17, 2025 00:54
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know it's bad, but we have to keep this script and tmt plan to work with gating test. Gating test does not have virt support TF runner.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, we can use system-reinstall-bootc for this part. We can land this PR first, then I'll send another PR to have gating test supported.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know it's bad, but we have to keep this script and tmt plan to work with gating test. Gating test does not have virt support TF runner.

Right. OK let's back up, in general our tests should work in all provisioning modes (Anaconda, to-existing-root, starting from an existing disk image (whether BIB or a plain to-disk). That's of course quite a matrix of things.

The biggest problem with the prior status quo was building bootc from source code in each test. That completely ruins iteration speed.

Now the more I think about this...at least for the Packit flow, it always injects a built RPM into the target environment right? So we should have always been able to use that instead of rebuilding.

Although digging in, I'm not seeing anything like that in our runs... Ah is it because we have skip_build: true in our packit config? It must be.

The annoying thing here AFAIK is teemtee/tmt#1018 - in order to run that workflow locally we'll need bespoke scripts to build an RPM from current sources and pass it to tmt the same way copr+testing-farm is doing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's also a related but different thread here in that I want to continue to support a container-native CI flow that isn't deeply tied into the plethora of Fedora-derivative specific CI/RPM tools...i.e. I want docker|podman build to continue to do something useful directly. But that shouldn't be too hard.

Copy link
Collaborator

Choose a reason for hiding this comment

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

container-native CI flow +1

Copy link
Collaborator

@henrywang henrywang Sep 17, 2025

Choose a reason for hiding this comment

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

The biggest problem with the prior status quo was building bootc from source code in each test. That completely ruins iteration speed.

In Packit/Gating test, yes, we have to do that. The Packit and gating environment can't deploy image mode VM for running test.

Testing Farm can deploy a image mode VM, so we can build a new image and switch to it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In Packit/Gating test, yes, we have to do that.

I don't think we ever needed to build from source in packit/gating, we just need to be able to start from a built RPM and turn the existing system into image mode via a local build...I may try a spike on that.

The Packit and gating environment can't deploy image mode VM for running test.

Surely they can be made to do so, right? Is this about missing compose bound images?

@cgwalters
Copy link
Collaborator Author

cgwalters commented Sep 17, 2025

OK so this now works, but in unifying the (3!) different container builds we had going on and now we're including LBIs in some of the install to-existing-root tests, that reveals what looks like a different bug where we're leaking the overlay mount for our containers-storage instance.

EDIT: filed as #1618

@cgwalters cgwalters force-pushed the testing-cleanups-p1 branch 2 times, most recently from f9844f7 to 9e1ea09 Compare September 17, 2025 18:07
A key thing for me is that the `Justfile` should be a one-stop
shop for development of the project. It can't have everything but
it should answer the basic questions of "how do I build and test
this project".

This aligns the recently added tmt-on-GHA flow a *bit* more closely
with some of that. Biggest is to use the `just build-integration-test-image` as the canonical
way to build a container image with our testing stuff in it;
which uses our main Dockerfile

Other cleanups:
- Change test script to move into tests/tmt/ as a workaround for
  teemtee/tmt#3037 (comment)
- Change the qemu logic to use SMBIOS credentials so we don't
  have to carry around both a disk image and a SSH key
- Change qemu to use `-snapshot` so we can reuse disks
- Change the scripts to accept data via argv[1] and not environment
- Drop the hardcoded testing directory and use `target/` as
  a generic build artifact dir

Signed-off-by: Colin Walters <walters@verbum.org>
This should reduce the flake rate.

Signed-off-by: Colin Walters <walters@verbum.org>
@cgwalters
Copy link
Collaborator Author

YES! This one finally passes.

@cgwalters
Copy link
Collaborator Author

Now in order to merge this, we need to change the merge gates to require different GHA contexts. I can do that once we agree to merge.

(What I actually want is to have the list of required CI contexts listed in the repo itself; at least Gemini suggests open coding this with a polling task which is kind of icky but maybe we can put something for this in https://github.com/bootc-dev/infra )

@cgwalters
Copy link
Collaborator Author

Who is taking the review/merge of this?

@jeckersb
Copy link
Collaborator

Who is taking the review/merge of this?

I'll take it

@jeckersb
Copy link
Collaborator

Initial thoughts are this looks great to me, I'm running through it again except this time trying the bcvk route.

Copy link
Collaborator

@jeckersb jeckersb left a comment

Choose a reason for hiding this comment

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

Well, the non-bcvk case works fine for me, I still need to do some untangling of things on my end so I won't block on that.

@cgwalters cgwalters disabled auto-merge September 19, 2025 18:54
@cgwalters cgwalters merged commit 88364c0 into bootc-dev:main Sep 19, 2025
28 checks passed
@cgwalters
Copy link
Collaborator Author

Initial thoughts are this looks great to me, I'm running through it again except this time trying the bcvk route.

You need bootc-dev/bcvk#6

@cgwalters
Copy link
Collaborator Author

Now in order to merge this, we need to change the merge gates to require different GHA contexts. I can do that once we agree to merge.

This should now be done, the merge gates are now switched to the GHA runs of tmt for c9s, c10s and fedora-42.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rework tmt to use pre-built container
3 participants