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

packit: Build PRs into default packit COPRs #1260

Merged
merged 1 commit into from
Aug 8, 2023
Merged

packit: Build PRs into default packit COPRs #1260

merged 1 commit into from
Aug 8, 2023

Conversation

martinpitt
Copy link
Contributor

@martinpitt martinpitt commented Aug 7, 2023

Building all PRs of all container projects into the same COPR does not properly isolate PRs from each other: E.g. a podman PR currently runs against whichever crun PR was opened/updated last; in other words, sending a broken crun PR will instantly break tests for all subsequent podman runs.

To avoid that, change the copr_build configuration to use the packit default COPRs, which are specific to the particular PR, and disappear after a few weeks. Depending projects like podman should only run against what landed in crun/main, i.e. the podman-next COPR.

Note that this does not preclude testing a podman PR against a crun PR: This can be explicitly requested [1]. But most PRs don't change the API and thus should default to isolation.

[1] https://packit.dev/posts/testing-farm-triggering


See the discussion in #1256 for the background, hello @lsm5 . I kept the same COPR targets as https://copr.fedorainfracloud.org/coprs/rhcontainerbot/podman-next/ . If you agree, I'm happy to send the same change to podman, container-selinux (other projects?), afterwards the packit-builds COPR can be removed. However, I'm not aware why you chose to do it that way in the first place? I.e. do you need the packit-builds COPR for anything else? Then I'd recommend to at least having one PR build COPR per project, not one for the whole org and cross-project testing.

Working PRs for all current packages in the packit-builds COPR. Once they are all ready, they need to land together. The green ones (ready for review) need review and work; the gray ones (draft) still fail, I'm looking into them:

@lsm5
Copy link
Member

lsm5 commented Aug 7, 2023

@martinpitt the main reason for having rhcontainerbot/packit-builds for trigger: pull_request was better control, simpler yaml (RFE: packit/packit-service#1903) , and so I could just go to https://copr.fedorainfracloud.org/coprs/rhcontainerbot/packit-builds/builds/ and get a quick recent builds status.

RE: transient copr, looks like the current setup for packit-builds doesn't quite consider it transient. Is that something that could be adjusted in copr settings? Alternatively, can we adjust the TMT jobs to pull all packages from podman-next except the build that's currently being tested?

Responding to comment from #1256:

The problem is that both crun and podman currently do builds for PRs in the same rhcontainer/packit-builds COPR. So Runs for podman will necessarily see some (arbitrary) crun PR builds as well. That's what I meant with "use packit's ephemeral coprs to build code from PRs". So this needs to happen in both/all containers projects, the packit-builds COPR can be removed, and only the podman-next COPR remains. Does that make sense?

So, what I'm trying to understand is if we change from rhcontainerbot/packit-builds to $PACKIT_EPHEMERAL_DEFAULT for all our trigger: pull_request, we're just switching from one copr to another. If using packit's default copr ensures we pull from rhcontainerbot/podman-next for testing, can't we ensure the same if we're using rhcontainerbot/packit-builds ? IOW, what makes packit's own default copr special in this regard ?

@martinpitt martinpitt marked this pull request as draft August 7, 2023 18:25
@martinpitt
Copy link
Contributor Author

Moving to draft, this still needs fixing to include the packit-next COPR. But I didn't want to spend a lot of work on this before discussing.

looks like the current setup for packit-builds doesn't quite consider it transient

How do you mean? Each PR gets its own COPR, https://copr.fedorainfracloud.org/coprs/packit/containers-crun-1260 in this case (note the PR number), and they get deleted after a month or so.

can we adjust the TMT jobs to pull all packages from podman-next except the build that's currently being tested?

Possibly -- the test could do some dnf incantation with the right amount of downgrade, --disablerepo etc. I don't know precedence for that.

we're just switching from one copr to another.

IOW, what makes packit's own default copr special in this regard ?

The "special" bit is just that they get auto-created per PR.

Not quite -- you switch from a single COPR for all PRs of all projects to one COPR per project PR. That's the whole point of it, to isolate PRs from one another.

Note that podman-next is already past the point of "collisions" -- main commits are serialized and vetted (hopefully) to not break other bits; while open PRs are parallel and can be arbitrarily broken.

@lsm5
Copy link
Member

lsm5 commented Aug 7, 2023

How do you mean? Each PR gets its own COPR, https://copr.fedorainfracloud.org/coprs/packit/containers-crun-1260 in this case (note the PR number), and they get deleted after a month or so.

It'd been quite a while since I used the default copr, so I'd forgotten it created a unique copr for every PR. My bad.

Possibly -- the test could do some dnf incantation with the right amount of downgrade, --disablerepo etc. I don't know precedence for that.

In cockpit project, do you do anything to get a quick view of packit failures or do you simply navigate to failures from the PR status? I'm kinda tempted to try some dnf stuff, but I do see it becoming a problem often.

So, all things considered, I'm fine with the ephemeral unique copr for trigger: pull request provided we can make all builds work. I don't quite remember off-hand but some packages have needed me to add some dependencies separately to make all build targets happy.

@martinpitt I welcome PRs from you if you got the time. If not, I can make the changes myself as and when I get the chance.

@martinpitt
Copy link
Contributor Author

In cockpit project, do you do anything to get a quick view of packit failures or do you simply navigate to failures from the PR status?

The latter -- we rarely feel the need for such a "quick view". Unless COPR has an infra problem (which is very rarely the case), our fedora-stable and centos builds are always green when landing a PR, i.e. build failures in a PR are the fault of the PR and get sorted out right there. Rawhide is different -- there things do "spontaneously" (from our POV) combust, and if that is outside of our reach, we just have to complain/file a bug/wait until it is resolved. But then that failure will happen in all PRs anyway, so it's very easy to find an instance.

That is a decision that your team has to make:

  • do you prefer to see rawhide problems early and ignore them for a while (which takes some mental training to stop ignoring failures once the problem got fixed)
  • or do you rather only do CI on stable Fedoras, and then possibly have to do a bigger amount of spec updates etc. to adjust them when Fedora branches off?

As you currently enable rawhide, I kept it enabled as well. If/once it gets too much on your nerve, it's easy to change the "fedora-all" to "fedora-stable".

I'm fine with the ephemeral unique copr provided we can make all builds work

Yes, makes sense. I suppose you mean "for all projects", so let's get working PRs for all projects that currently build into the packit-builds repo, and then land them together only once they work. So far this was just a proposal to gauge if you even want this.

Building all PRs of all container projects into the same COPR does not
properly isolate PRs from each other: E.g. a podman PR currently runs
against whichever crun PR was opened/updated last; in other words,
sending a broken crun PR will instantly break tests for all subsequent
podman runs.

To avoid that, change the copr_build configuration to use the packit
default COPRs, which are specific to the particular PR, and disappear
after a few weeks. Depending projects like podman should only run
against what landed in crun/main, i.e. the podman-next COPR.

Note that this does not preclude testing a podman PR against a crun PR:
This can be explicitly requested [1]. But most PRs don't change the API
and thus should default to isolation.

[1] https://packit.dev/posts/testing-farm-triggering

Signed-off-by: Martin Pitt <mpitt@redhat.com>
@martinpitt
Copy link
Contributor Author

martinpitt commented Aug 8, 2023

@lsm5 This works now. I sent some more corresponding PRs for other projects (copy pasta pretty much), and made a todo list in the description. Note that I'll be on PTO for two weeks starting tomorrow, but if you have any questions, feel free to ping @marusak and @jelly.

Copy link
Member

@lsm5 lsm5 left a comment

Choose a reason for hiding this comment

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

@martinpitt This actually works out better given we can now easily add rhel-N-$ARCH buildroots without fighting the copr settings.

LGTM.

@lsm5 lsm5 merged commit 4ee9bde into containers:main Aug 8, 2023
34 checks passed
@martinpitt martinpitt deleted the packit-temporary branch August 8, 2023 13:04
@martinpitt
Copy link
Contributor Author

@lsm5 : FTR, I have a half-done gvisor-tap-vsock on my disk (just in a meeting). I think I can do skopeo (the last one) too still today.

@lsm5
Copy link
Member

lsm5 commented Aug 8, 2023

@lsm5 : FTR, I have a half-done gvisor-tap-vsock on my disk (just in a meeting). I think I can do skopeo (the last one) too still today.

sgtm, no rush, thanks! Appreciate the quick turnaround. I'm gonna add rhel targets to these soon as we have rhel testing on our agenda as well.

@martinpitt
Copy link
Contributor Author

Cool, I didn't know that we could run RHEL testing in public upstream PRs. Do you need to send @thrix some special 🍺 💝 for that? 😁

@lsm5
Copy link
Member

lsm5 commented Aug 8, 2023

Cool, I didn't know that we could run RHEL testing in public upstream PRs.

containers/container-selinux#260

Do you need to send @thrix some special beer gift_heart for that? grin

i'll need to buy him a year's supply at least. I hope I can file it as a company expense 😆

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.

2 participants