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

ci: Run cockpit tests in PRs #1843

Merged
merged 1 commit into from
Oct 19, 2023
Merged

Conversation

martinpitt
Copy link
Contributor

@martinpitt martinpitt commented Aug 17, 2023

See https://cockpit-project.org/blog/tmt-cross-project-testing.html

Drop the install-only tests, as TF only runs the default "install check" test if there are no plans, but now we have one. That will cover the installation/upgrade as preparation step.


When done seriously, this is an ongoing commitment. In our recent years the Cockpit team has reported a couple dozen bug reports found by our tests, many of them were actual regressions. With this setup, we have a good chance of prevent landing such regressions.

This approach is fairly new at least for us in the Fedora world, so let's treat this as an experiment. We do need to talk about commitments and expectations, though. From our side, I propose:

  • We don't expect you to become experts in our tests. When they fail, it would be nice if you could have a quick look at the log and see if it's something obvious, especially an unexpected SELinux AVC in the log. But in general, we expect that someone from our team will investigate and discuss that with you. The PR where that happened provides a nice place to collect notes.

  • Whenever one of our tests fail, three of our team members will be notified by packit automatically, so that we can timely investigate them.

  • We don't expect you to block PRs on these tests, especially not if the testing farm has infrastructure problems . E.g. sometimes they to run into provisioning errors. Just ignore these then -- we see them too in our projects, and will usually prod #testing-farm then.

  • We would like you to at least give us a chance on that workday to look at a failed test (not infra failure, a "real" one) before you land a PR, so that this all makes sense. If something is urgent and you quick-land, then at least we still retain the benefit of having a trace in which PR tests started to fail, but then the damage is possibly already done. Note that this isn't a big new commitment from our side: we already spend many hours every week looking at regressions, and it takes a lot more effort to track them down weeks after they happened. So this will acually reduce both our and your time spent on hunting down regressions, because the context (PR) is fresh and small, as opposed to "whatever changed in Fedora in the last 4 weeks".

  • I set this up for Fedora latest and rawhide for the time being. If you think that rawhide has too much churn, of course feel free to drop that from .packit.yaml, or I do it here right away when you want to start slow. We have enabled rawhide in cockpit projects a while ago, and it does often fail due to unrelated reasons. Improving this is pretty much exactly why I start this initiative, and at least knowing about problems is still better IMHO than entirely ignoring rawhide, but it can get annoying.

Please let me know about any other question that you may have. I'm happy to discuss them here or in a gmeet for higher bandwidth.

Thanks!

@martinpitt
Copy link
Contributor Author

martinpitt commented Aug 17, 2023

You still need to actually enable packit on this project to make use of this. See #1820 (comment)

Thanks!

@martinpitt

This comment was marked as resolved.

@martinpitt
Copy link
Contributor Author

So, who would be able to enable packit for the selinux-policy project or the fedora-selinux org? @bachradsusi or @wrabcak? @zpytela told me that he's not permitted to do that.

This is semi-urgent, as #1820 was merged without actually enabling packit, so there's no testing of the rpm build any more.

@bachradsusi
Copy link
Member

It's Friday 8pm here. I'll take a look on Monday. Ok?

@martinpitt
Copy link
Contributor Author

Sure, thanks @bachradsusi ! (It's not that urgent of course, I just wouldn't like to stall it for another month)

@bachradsusi
Copy link
Member

/packit test

@bachradsusi
Copy link
Member

@martinpitt looks like it does things. Let me know if I missed a thing.

@martinpitt
Copy link
Contributor Author

Thanks @bachradsusi ! Yep, I'll wait for the results and then rebase this PR, as it's already a month old. If the RPM builds started to fail since #1820, I'll fix them separately first.

@martinpitt
Copy link
Contributor Author

The built COPR packages were uninstallable. Trying a rebase first, that also sorts out automatic notifications.

@packit-as-a-service
Copy link

Cockpit tests failed for commit 3bab283. @martinpitt, @jelly, @mvollmer please check.

@martinpitt martinpitt marked this pull request as draft September 25, 2023 11:46
@martinpitt
Copy link
Contributor Author

Hmm, this uninstallability a problem indeed.. Reproduces easily in podman run -it --rm registry.fedoraproject.org/fedora:38:

dnf -y update
dnf  install --allowerasing https://download.copr.fedorainfracloud.org/results/packit/fedora-selinux-selinux-policy-1843/fedora-38-x86_64/06440741-selinux-policy/selinux-policy-doc-v38.28-1.20230925105543192971.pr1843.6.g3bab2837d.fc38.noarch.rpm https://download.copr.fedorainfracloud.org/results/packit/fedora-selinux-selinux-policy-1843/fedora-38-x86_64/06440741-selinux-policy/selinux-policy-sandbox-v38.28-1.20230925105543192971.pr1843.6.g3bab2837d.fc38.noarch.rpm https://download.copr.fedorainfracloud.org/results/packit/fedora-selinux-selinux-policy-1843/fedora-38-x86_64/06440741-selinux-policy/selinux-policy-minimum-v38.28-1.20230925105543192971.pr1843.6.g3bab2837d.fc38.noarch.rpm https://download.copr.fedorainfracloud.org/results/packit/fedora-selinux-selinux-policy-1843/fedora-38-x86_64/06440741-selinux-policy/selinux-policy-devel-v38.28-1.20230925105543192971.pr1843.6.g3bab2837d.fc38.noarch.rpm https://download.copr.fedorainfracloud.org/results/packit/fedora-selinux-selinux-policy-1843/fedora-38-x86_64/06440741-selinux-policy/selinux-policy-mls-v38.28-1.20230925105543192971.pr1843.6.g3bab2837d.fc38.noarch.rpm https://download.copr.fedorainfracloud.org/results/packit/fedora-selinux-selinux-policy-1843/fedora-38-x86_64/06440741-selinux-policy/selinux-policy-v38.28-1.20230925105543192971.pr1843.6.g3bab2837d.fc38.noarch.rpm https://download.copr.fedorainfracloud.org/results/packit/fedora-selinux-selinux-policy-1843/fedora-38-x86_64/06440741-selinux-policy/selinux-policy-targeted-v38.28-1.20230925105543192971.pr1843.6.g3bab2837d.fc38.noarch.rpm

fails with

Error: 
 Problem: The operation would result in removing the following protected packages: sudo
(try to add '--skip-broken' to skip uninstallable packages)

which is not quite "systemd, systemd-udev" as on the TF (as the container doesn't have them installed), but same root cause. One can even run dnf install systemd systemd-udev first to make it identical.

In a Fedora 38 cloud VM, it's a bit more straightforward to investigate, as that already has policycoreutils etc. installed.

Error: 
 Problem 1: package selinux-policy-v38.28-1.20230925105543192971.pr1843.6.g3bab2837d.fc38.noarch from @commandline requires policycoreutils >= 3.4-1, but none of the providers can be installed
  - package policycoreutils-3.5-1.fc38.x86_64 from @System conflicts with selinux-policy-base < 3.13.1-138 provided by selinux-policy-v38.28-1.20230925105543192971.pr1843.6.g3bab2837d.fc38.noarch from @commandline
  - package policycoreutils-3.5-1.fc38.x86_64 from fedora conflicts with selinux-policy-base < 3.13.1-138 provided by selinux-policy-v38.28-1.20230925105543192971.pr1843.6.g3bab2837d.fc38.noarch from @commandline

(plus 6 more). So perhaps we need a newer policycoreutils in the game, that was rebuilt against selinux-policy from the 'rawhide' branch instead of from Fedora proper?

@bachradsusi @zpytela how did you handle/test this before? There was only the build-rpm workflow before, but that didn't do any installation/validation. How did you test/install branch/PR builds manually before? Thanks!

@bachradsusi
Copy link
Member

selinux-policy-v38.28-1.20230925103732538035.pr1843.7.g99ba0a36e.fc38.noarch seems to be wrong NVR . Fedora selinux-policy spec file uses:

Summary: SELinux policy configuration
Name: selinux-policy
Version: 38.7
Release: 1%{?dist}
$ rpmdev-vercmp v38.28-1.20230925105543192971.pr1843.6.g3bab2837d.fc38 38.27-1
v38.28-1.20230925105543192971.pr1843.6.g3bab2837d.fc38 < 38.27-1

@bachradsusi
Copy link
Member

Disclaimer: I haven't followed this packit/copr/ci changes so I know almost nothing about it

But

libselinux conflicts with selinux-policy-base < 3.13.1-138 due the past changes in SELinux module store and according to rpm, v38.28-1.20230925103732538035.pr1843.7.g99ba0a36e is lower than 3.13.1-138

$ rpmdev-vercmp v38.28-1.20230925105543192971.pr1843.6.g3bab2837d.fc38 3.13.1-138.1        
v38.28-1.20230925105543192971.pr1843.6.g3bab2837d.fc38 < 3.13.1-138.1

@bachradsusi
Copy link
Member

Packit used https://download.copr.fedorainfracloud.org/results/packit/fedora-selinux-selinux-policy-1843/fedora-38-x86_64/06440741-selinux-policy/selinux-policy.spec to build selinux-policy and there's already the version changed. Apparently, packit uses latest tag from this repo [v38.28](https://github.com/fedora-selinux/selinux-policy/releases/tag/v38.28) and overrides it in the spec file.

@bachradsusi
Copy link
Member

I guess you need to use https://packit.dev/docs/configuration#upstream_tag_template

upstream_tag_template
(string) Packit by default expects git tags to match versions (e.g. when doing the propose-downstream command) - if you are using a different tagging scheme, let's say v1.2.3 you can then set this parameter to v{version} and packit will fill in the version argument.

@martinpitt
Copy link
Contributor Author

Thanks @bachradsusi for spotting this! I'll see how to fix this.

See https://cockpit-project.org/blog/tmt-cross-project-testing.html

Drop the install-only tests, as TF only runs the default "install check"
test if there are no plans, but now we have one. That will cover the
installation/upgrade as preparation step.
@martinpitt
Copy link
Contributor Author

@zpytela , @WOnder93 This is now working and ready for reviewing and discussion. We've had this running on https://github.com/containers/podman/ and https://github.com/storaged-project/udisks for a while already, and sorted out the initial quirks. In particular, automatic notifications. Nevertheless, please go over the points in the description to make sure we agree on the expectations. Thank you!

@martinpitt
Copy link
Contributor Author

@zpytela @bachradsusi : Gentle monthly ping -- can we discuss this at some point? Just yesterday we found https://bugzilla.redhat.com/show_bug.cgi?id=2244759 , such bugs would better be found right away in a PR rather than after landing.

@bachradsusi
Copy link
Member

Looks good to me. But I'm not active maintainer of this repo. It needs to be merged by @zpytela

@thrix
Copy link

thrix commented Oct 19, 2023

@zpytela pretty please 👀

@zpytela
Copy link
Contributor

zpytela commented Oct 19, 2023

Thank you Martin, I am going to merge it now, sorry for the delay.

@zpytela zpytela merged commit ba41134 into fedora-selinux:rawhide Oct 19, 2023
7 checks passed
@martinpitt martinpitt deleted the revdeps branch October 19, 2023 20:43
@martinpitt
Copy link
Contributor Author

Thanks @zpytela ! No worries, I actually expected that you still had some questions to discuss, but we can also do that on the life subject when trouble occurs (and it certainly will, as this is still a fairly new thing). Thank you for jumping in, let's see how this will work out!

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