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

RPM: include criu dependencies #1256

Merged
merged 1 commit into from
Aug 4, 2023
Merged

RPM: include criu dependencies #1256

merged 1 commit into from
Aug 4, 2023

Conversation

lsm5
Copy link
Member

@lsm5 lsm5 commented Aug 4, 2023

These dependencies were already included in the official Fedora package but were not included in rpm/crun.spec currently used by copr and soon to be used for the official Fedora package.

Fixes: #1255

These dependencies were already included in the official Fedora package
but were not included in rpm/crun.spec currently used by copr and soon
to be used for the official Fedora package.

Fixes: containers#1255

Signed-off-by: Lokesh Mandvekar <[email protected]>
@lsm5
Copy link
Member Author

lsm5 commented Aug 4, 2023

@flouthoc @giuseppe PTAL

/cc @Luap99 @martinpitt

@lsm5
Copy link
Member Author

lsm5 commented Aug 4, 2023

@martinpitt we can block this until you can verify containers/podman#19500 is green for checkpointing.

@martinpitt
Copy link
Contributor

@lsm5 I don't understand -- why do you want to block on this PR? It seems obviously correct to sync the upstream spec with what's in Fedora, as otherwise you have a regression on the next release? (That said, it seems that the Fedora gating tests -- which do seem to test checkpointing -- don't run upstream?)

I would think that containers/podman#19500 can't become green until this lands and gets built in your COPR, can it?

@martinpitt
Copy link
Contributor

Interesting, it did become green. Hmm, that feels like your COPR receives both builds from PRs as well as builds from main branches -- isn't that horribly confusing and prone to conflicts and errors?

Anyway, thanks for fixing!

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

@giuseppe giuseppe merged commit 83c6f54 into containers:main Aug 4, 2023
@lsm5
Copy link
Member Author

lsm5 commented Aug 4, 2023

Interesting, it did become green. Hmm, that feels like your COPR receives both builds from PRs as well as builds from main branches -- isn't that horribly confusing and prone to conflicts and errors?

@martinpitt builds from PRs go to rhcontainerbot/packit-builds . builds from main go to rhcontainerbot/podman-next. The tests turned green before this PR merged when i ran /packit retest-failed on your PR.

Anyway, thanks for fixing!

@lsm5 I don't understand -- why do you want to block on this PR? It seems obviously correct to sync the upstream spec with what's in Fedora, as otherwise you have a regression on the next release? (That said, it seems that the Fedora gating tests -- which do seem to test checkpointing -- don't run upstream?)

I would think that containers/podman#19500 can't become green until this lands and gets built in your COPR, can it?

The tmt tests pulled the packages built from rhcontainerbot/packit-builds so that's why it turned green before this was merged. https://artifacts.dev.testing-farm.io/6696d5fb-6032-4023-bad8-ddc1e560e153/guest-setup-1edb0254-9163-4bc7-9aa6-6bfc01692d76/artifact-installation-1edb0254-9163-4bc7-9aa6-6bfc01692d76/1-Download-copr-repository.txt

@lsm5 lsm5 deleted the main-criu branch August 4, 2023 14:19
@lsm5
Copy link
Member Author

lsm5 commented Aug 4, 2023

@martinpitt builds from PRs go to rhcontainerbot/packit-builds . builds from main go to rhcontainerbot/podman-next. The tests turned green before this PR merged when i ran /packit retest-failed on your PR.

Might be too late to change the copr names if that's still confusing. But let me know if I can update the description or something to make that difference clearer. We can take this conversation to matrix or something if necessary.

@martinpitt
Copy link
Contributor

Aah, makes sense, thanks @lsm5 for the explanation! I suppose that works because TF goes out of its way to install the precise NVR from a PR, out of this "pool" of (potentially) lots of different parallel PR builds.

@martinpitt
Copy link
Contributor

Something in the back of my head kept chewing on this.. A PR against podman should really not see this crun build from a PR, only the latest build from what landed on crun's main branch. That it did see it shows that a podman PR will run its tests against whichever crun PR happened to be opened/pushed last. In other words, sending a broken crun PR breaks all subsequent podman PRs. This is unexpected, unintuitive, unreproducible, and thus hard to debug or even realize. I strongly recommend you to reconfigure your COPRs to stop doing that.

In other words, use packit's ephemeral coprs to build code from PRs, and configure it to build commits to main into your permanent COPR, for proper isolation of concurrent PRs.

The proper way to do validate crun PRs is to run podman's tests in the crun PRs, in pretty much the same way as I proposed with running cockpit-podman's tests in podman PRs. (Just one level down in the dependency tree).

(Happy to have a gmeet about this if you want to explore this more)

@lsm5
Copy link
Member Author

lsm5 commented Aug 7, 2023

Something in the back of my head kept chewing on this.. A PR against podman should really not see this crun build from a PR, only the latest build from what landed on crun's main branch. That it did see it shows that a podman PR will run its tests against whichever crun PR happened to be opened/pushed last. In other words, sending a broken crun PR breaks all subsequent podman PRs. This is unexpected, unintuitive, unreproducible, and thus hard to debug or even realize. I strongly recommend you to reconfigure your COPRs to stop doing that.

In other words, use packit's ephemeral coprs to build code from PRs, and configure it to build commits to main into your permanent COPR, for proper isolation of concurrent PRs.

@martinpitt good point. I think that should be doable. For now, apart from container-selinux, we are not running TMT tests anywhere apart from your cockpit tests PR for podman. And AFAICT, the copr change needs to be made in the PR itself (?) . Do you wanna try adding the copr change to your PR ?

@martinpitt
Copy link
Contributor

And AFAICT, the copr change needs to be made in the PR itself (?) . Do you wanna try adding the copr change to your PR ?

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?

@martinpitt
Copy link
Contributor

@lsm5 : I sent PR #1260 for that.

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.

podman from current rhcontainerbot/packit-builds breaks checkpointing
3 participants