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

[skip-ci] Packit/TMT: Run gating tests #1960

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

lsm5
Copy link
Member

@lsm5 lsm5 commented Apr 3, 2023

Commit 1
PR Labels: apply release label to release- branch PRs

This will be useful in the followup commit that enables TMT test jobs on PRs.

PRs on main branch should be tested with bleeding-edge dependencies from the podman-next COPR while PRs on release branches should be tested only with the official distro packages. Packit will run/skip the relevant set of tests based on this label.

Commit 2
Packit/TMT: Run system tests

This commit enables TMT jobs triggered by Packit to run system tests.

2 set of jobs dev and release have been added. dev jobs are meant to run on main PRs with additional package updates fetched from podman-next copr while release jobs are meant to run on release- branch PRs using only the dependencies present in the official distro.

Packit checks PR labels (see previous commit) to filter out the jobs that get run.

plans/main.fmf Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented May 4, 2023

A friendly reminder that this PR had no activity for 30 days.

@lsm5 lsm5 removed the stale-pr label May 4, 2023
@github-actions
Copy link

github-actions bot commented Jun 4, 2023

A friendly reminder that this PR had no activity for 30 days.

@lsm5 lsm5 removed the stale-pr label Jun 5, 2023
@github-actions
Copy link

github-actions bot commented Jul 6, 2023

A friendly reminder that this PR had no activity for 30 days.

@mtrmac
Copy link
Contributor

mtrmac commented Jul 7, 2023

@lsm5 are you still working on this?

(If so, this is not urgent from my POV, no need to push it to the front of your queue — I’d just like to close the PR if there is absolutely no chance this work will continue.)

@lsm5
Copy link
Member Author

lsm5 commented Jul 7, 2023

@mtrmac I do plan to work on this. But may take a while. I have bookmarked it for myself so I don't lose track. So please feel free to close it.

@mtrmac
Copy link
Contributor

mtrmac commented Jul 7, 2023

Thanks, if it is still relevant, let’s keep it open. It might help, or inspire, other contributors.

@github-actions github-actions bot removed the stale-pr label Jul 8, 2023
@github-actions
Copy link

github-actions bot commented Aug 7, 2023

A friendly reminder that this PR had no activity for 30 days.

@lsm5 lsm5 removed the stale-pr label Aug 7, 2023
@github-actions
Copy link

github-actions bot commented Sep 7, 2023

A friendly reminder that this PR had no activity for 30 days.

@lsm5 lsm5 removed the stale-pr label Oct 10, 2023
@lsm5 lsm5 force-pushed the packit-gating-tests branch from 7bfbd1c to 2e9afc9 Compare October 11, 2023 13:17
@lsm5
Copy link
Member Author

lsm5 commented Oct 11, 2023

@mtrmac @vrothberg @cevich RE: system tests in CI, is it critical to run them in a container or would we be ok with running test-system-local ? Asking mainly RE: packit + tmt tests.

@mtrmac
Copy link
Contributor

mtrmac commented Oct 11, 2023

@edsantiago is authoritative for system tests (question above).

@edsantiago
Copy link
Member

Forgive me please, I don't understand the question. It's been a while since I've looked at skopeo gating tests, but my recollection is that they run on a plain system, via dnf install skopeo-tests. What's the context for "in a container"?

@mtrmac
Copy link
Contributor

mtrmac commented Oct 11, 2023

@mtrmac @vrothberg @cevich RE: system tests in CI, is it critical to run them in a container or would we be ok with running test-system-local ?

IIRC the system tests run a registry server (container). So… running -local would be much simpler in that there would be no need for nested Podman; OTOH the tests would be much less isolated from the surrounding environment, which might be a difficulty.

I’m not immediately sure how much custom tooling exists in the CI container, and would need to be reproduced; https://github.com/containers/automation_images/blob/main/skopeo_cidev/setup.sh is relevant to that, but IIRC almost all of that exists for integration, not system, tests.

@thrix
Copy link

thrix commented Oct 11, 2023

/packit test

@thrix
Copy link

thrix commented Oct 11, 2023

So, unfortunately, the issue we are hitting is a bug in Packit:

packit/packit-service#2028

It was not considered a bug before, as people were not hitting it and we did not know it had this consequence.
They are working on a fix.

@lsm5
Copy link
Member Author

lsm5 commented Oct 11, 2023

So, unfortunately, the issue we are hitting is a bug in Packit:

packit/packit-service#2028

It was not considered a bug before, as people were not hitting it and we did not know it had this consequence. They are working on a fix.

Thanks @thrix . I'll disable the rhel tests for now.

@lsm5
Copy link
Member Author

lsm5 commented Oct 11, 2023

@mtrmac @vrothberg @cevich RE: system tests in CI, is it critical to run them in a container or would we be ok with running test-system-local ?

IIRC the system tests run a registry server (container). So… running -local would be much simpler in that there would be no need for nested Podman; OTOH the tests would be much less isolated from the surrounding environment, which might be a difficulty.

I’m not immediately sure how much custom tooling exists in the CI container, and would need to be reproduced; https://github.com/containers/automation_images/blob/main/skopeo_cidev/setup.sh is relevant to that, but IIRC almost all of that exists for integration, not system, tests.

ack, the test-system target seems to spawn some container too. Anyway, I'll try to mimic the cirrus setup in tmt. Thanks

@lsm5 lsm5 force-pushed the packit-gating-tests branch 3 times, most recently from ff43fea to 85e8158 Compare December 25, 2024 10:18
Copy link

Tests failed. @containers/packit-build please check.

@lsm5 lsm5 force-pushed the packit-gating-tests branch 3 times, most recently from b90c04c to cf73631 Compare December 25, 2024 14:15
@lsm5 lsm5 force-pushed the packit-gating-tests branch 10 times, most recently from 5fc00d7 to b0b9d2b Compare January 23, 2025 12:56
@lsm5
Copy link
Member Author

lsm5 commented Jan 23, 2025

@mtrmac RE: your earlier questions:

What is the longer-term plan? Is this going intended to be in addition to .cirrus.yml, or eventually replace it?

Plan is to eventually get rid of cirrus and depend only on TMT assuming we'll have all functionality sooner or later via TMT.

Update: I have pretty much redone the whole PR with scope limited to only triggering system tests as we'll need those for official Fedora and CentOS Stream gating too. So, I feel best to get those in first. Please also check out the updated PR description ^ for details on the 2 included commits.

You'll see a bunch of new jobs in CI. The ones with suffix dev will run on main branch. The ones with suffix release will show as Queued but are expected to not run on main. Reverse should happen on release-* branches once this change lands there.(Untested on release branches but I guess we'll find out when we cherrypick).

  • systemtest/tmt/* lists the actual test and the script.
  • plans/main.fmf configures the test environment based on the initiator (packit, fedora ci, human etc.). Any filtering of tests to run based on the environment will also be done here. For example, see the same on container-selinux

In this PR, I'm no longer changing the Makefile or scripts in hack/. So, that should make things a lot simpler.

I'll work on the integration / lint and ostree tests in followup PRs. I'll keep your other comments here in mind while I work on those later.

@lsm5 lsm5 marked this pull request as ready for review January 23, 2025 13:19
@lsm5
Copy link
Member Author

lsm5 commented Jan 23, 2025

Another advantage of TMT is convenient reverse dependency testing. See this c/common PR. That will run the tests maintained in skopeo on every PR in c/common.

@lsm5
Copy link
Member Author

lsm5 commented Jan 24, 2025

/packit test

lsm5 added 3 commits January 24, 2025 16:21
The default gobuild macro on CentOS Stream now accounts for `BUILDTAGS`,
so we don't need to redefine the macro in rpm spec.

The `libtrust_openssl` has been set in the spec for RHEL
environments.

Signed-off-by: Lokesh Mandvekar <[email protected]>
We're not running any tests in the check section.

Signed-off-by: Lokesh Mandvekar <[email protected]>
This will be useful in the followup commit that enables TMT test jobs on
PRs.

PRs on `main` branch should be tested with bleeding-edge dependencies
from the podman-next COPR while PRs on `release` branches should be
tested only with the official distro packages. Packit will run/skip the
relevant set of tests based on this label.

Signed-off-by: Lokesh Mandvekar <[email protected]>
@lsm5 lsm5 force-pushed the packit-gating-tests branch from b0b9d2b to 16655f4 Compare January 24, 2025 10:57
@lsm5
Copy link
Member Author

lsm5 commented Jan 28, 2025

/packit test

This commit enables TMT jobs triggered by Packit to run system tests.

2 set of jobs `dev` and `release` have been added. `dev` jobs are meant
to run on main PRs with additional package updates fetched from
podman-next copr while `release` jobs are meant to run on release-
branch PRs using only the dependencies present in the official distro.

Packit checks PR labels (see previous commit) to filter out
the jobs that get run.

Signed-off-by: Lokesh Mandvekar <[email protected]>
@lsm5 lsm5 force-pushed the packit-gating-tests branch from 16655f4 to 327e5b0 Compare January 30, 2025 13:30
Copy link
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Thanks!

I’m not sure I’m reading that correctly, but some logs seem to suggest that the TMT tests run in ~2 minutes vs. previous 13. If that represents wall-clock time, that would be a really impressive improvement.


You'll see a bunch of new jobs in CI. … The ones with suffix release will show as Queued but are expected to not run on main. Reverse should happen on release-* branches once this change lands there.

Is there any chance to correctly represent the status in GitHub (either as “skipped”, or not including the other jobs in at all)?

I expect that eventually we’ll want to make these jobs merge-blocking, and I don’t know how that would work with a “queued” never-finishing job. It’s also easier for newcomers/occasional contributors when the tool status matches the intuitive understanding.

@@ -0,0 +1,3 @@
# https://github.com/actions/labeler
release:
- base-branch: [^release-?(0|[1-9]\d*).(0|[1-9]\d*)$]
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking: Does the 0|… special case intend to exclude something specific? I’d expect something simpler like \d\+.

I don’t expect we would create a branch named release-0050.01, so I don’t think this matters that much, but I might be missing something important.

@@ -127,6 +125,10 @@ export BUILDTAGS="$BASEBUILDTAGS $(hack/btrfs_tag.sh) $(hack/btrfs_installed_tag
export BUILDTAGS="$BASEBUILDTAGS btrfs_noversion exclude_graphdriver_btrfs"
%endif

%if %{defined fips}
export BUILDTAGS="$BUILDTAGS libtrust_openssl"
%endif
Copy link
Contributor

Choose a reason for hiding this comment

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

The gobuild-related changes are happening separately in #2501, so can that be dropped here?

@mtrmac
Copy link
Contributor

mtrmac commented Jan 31, 2025

One more thing — not blocking this PR, just to possibly think about.

For historical reasons, c/image’s CI, via the test-skopeo Make rule, tests each c/image PR against the Skopeo unit+integration+system tests, by vendoring the proposed c/image PR into the Skopeo codebase, building Skopeo, and running tests. And these tests are important — they are ~the only tests of the c/image registry client, as well as important e2e tests of other c/image functionality like signing.

If we completely removed the current way of running system tests in favor of TMT testing (based on Skopeo RPMs?), we would need some way to continue to run the tests against c/image PRs. I have no idea whether that would be trivial, difficult, or how it might affect the packit/TMT test automation design.

Alternatively, the testing structure could be cleaned up — instead of c/image relying on Skopeo’s tests, we could move most of the tests into the c/image codebase, and rework them them not to rely on the Skopeo repo (and codebase?). That would, conceptually, be a better design — but, also, probably quite a lot of work and so far there was never a strong enough reason to spend time on this, so we have been relying on the test-skopeo mechanism.

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.

4 participants