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

Add missing dependency crun to the podman spec #17587

Closed
wants to merge 1 commit into from

Conversation

dcermak
Copy link
Contributor

@dcermak dcermak commented Feb 21, 2023

quadlet uses crun as the runtime, but crun is not required in any way by podman

Does this PR introduce a user-facing change?

None

quadlet uses crun as the runtime, but crun is not required in any way by podman

Signed-off-by: Dan Čermák <[email protected]>
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

@lsm5
Copy link
Member

lsm5 commented Feb 21, 2023

Requires: oci-runtime
Recommends: crun
Requires: (crun if fedora-release-identity-server)

We've had that setup to allow people to use runc and remove crun if they wish. But if quadlet can only use crun and not runc, then I can change it to a hard dep on crun. I'd also like to know how I should update runc dep then.

EDIT: aforementioned crun removal only works on fedora workstation, not on fedora server.

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.

While I agree the dependency should be updated, I'd prefer not to do it in podman.spec.rpkg. It's best handled in containers-common-extra fedora package. That reminds me, I'll need to remove some other dependencies from here.

@vrothberg
Copy link
Member

We should probably also explore why Quadlet has a hard dep on crun.

@alexlarsson @ygalblum FYI

@dcermak
Copy link
Contributor Author

dcermak commented Feb 21, 2023

While I agree the dependency should be updated, I'd prefer not to do it in podman.spec.rpkg. It's best handled in containers-common-extra fedora package

Any particular reason for that? Burying direct dependencies in dependent spec files can make it very easy to forget to update them if dependencies are removed or added.

@dcermak
Copy link
Contributor Author

dcermak commented Feb 21, 2023

We should probably also explore why Quadlet has a hard dep on crun.

It's hardcoded here:

// We use crun as the runtime and delegated groups to it
service.Add(ServiceGroup, "Delegate", "yes")
podman.add(
"--runtime", "crun",
"--cgroups=split")

@lsm5
Copy link
Member

lsm5 commented Feb 21, 2023

While I agree the dependency should be updated, I'd prefer not to do it in podman.spec.rpkg. It's best handled in containers-common-extra fedora package

Any particular reason for that? Burying direct dependencies in dependent spec files can make it very easy to forget to update them if dependencies are removed or added.

There are many dependencies common to both buildah and podman, and also some to skopeo.

The podman.spec.rpkg is not the official fedora spec. It generates builds on rhcontainerbot/podman-next . So, it's a lot cheaper to list common dependencies for multiple packages hosted in multiple repos in 1 single common dependency package which is containers-common.

@ygalblum
Copy link
Contributor

@alexlarsson added this hard coded setting in .container file. I've just noticed that I did not set it for .kube files. So, should decide what's the right approach here:

  1. Force crun on both types
  2. Allow the user to set the value. If so, what should be the default value? Should Quadlet even set it?

@dcermak
Copy link
Contributor Author

dcermak commented Feb 21, 2023

While I agree the dependency should be updated, I'd prefer not to do it in podman.spec.rpkg. It's best handled in containers-common-extra fedora package

Any particular reason for that? Burying direct dependencies in dependent spec files can make it very easy to forget to update them if dependencies are removed or added.

There are many dependencies common to both buildah and podman, and also some to skopeo.

quadlet is a podman only feature, so the requirement on crun would be only for podman and not necessarily for buildah and skopeo.

@vrothberg
Copy link
Member

Allow the user to set the value. If so, what should be the default value? Should Quadlet even set it?

I think that's the right thing to do. Quadlet enforcing crun would otherwise ignore the settings in containers.conf.

@vrothberg
Copy link
Member

We should probably also explore why Quadlet has a hard dep on crun.

It's hardcoded here:

// We use crun as the runtime and delegated groups to it
service.Add(ServiceGroup, "Delegate", "yes")
podman.add(
"--runtime", "crun",
"--cgroups=split")

@giuseppe would that work with runc as well?

@lsm5
Copy link
Member

lsm5 commented Feb 21, 2023

While I agree the dependency should be updated, I'd prefer not to do it in podman.spec.rpkg. It's best handled in containers-common-extra fedora package

Any particular reason for that? Burying direct dependencies in dependent spec files can make it very easy to forget to update them if dependencies are removed or added.

There are many dependencies common to both buildah and podman, and also some to skopeo.

quadlet is a podman only feature, so the requirement on crun would be only for podman and not necessarily for buildah and skopeo.

@vrothberg does buildah never need crun ?

@vrothberg
Copy link
Member

@vrothberg does buildah never need crun ?

Any runtime would do the job for Buildah. There is no hard dep on crun.

@lsm5
Copy link
Member

lsm5 commented Feb 21, 2023

Any runtime would do the job for Buildah. There is no hard dep on crun.

Ack. Going by the kinda parallel conversation on this PR, if quadlet can use runc soon, then we can keep the dependency list as-is inside containers-common-extra. Please correct me if I'm wrong.

@dcermak btw, in containers-common fedora package, the containers-common-extra is only meant for podman and buildah. Skopeo depends only on the reduced dependencies in the main containers-common package. Sorry for not being clear earlier.

@lsm5
Copy link
Member

lsm5 commented Feb 21, 2023

Any runtime would do the job for Buildah. There is no hard dep on crun.

Ack. Going by the kinda parallel conversation on this PR, if quadlet can use runc soon, then we can keep the dependency list as-is inside containers-common-extra. Please correct me if I'm wrong.

if that's gonna take longer than expected, I'm fine with merging this PR. HTH.

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.

Given that quadlet is a crun-only feature for now, I'm cool with this. This can be reverted whenever we have runc support.

/lgtm
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 21, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 21, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 21, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dcermak, lsm5

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 21, 2023
@giuseppe
Copy link
Member

@giuseppe would that work with runc as well?

yes that works with runc too

@vrothberg
Copy link
Member

@giuseppe would that work with runc as well?

yes that works with runc too

Thanks! In that case, I suggest to remove these lines from Quadlet. Then, we don´t need any change to the spec.

@alexlarsson
Copy link
Contributor

crun is a better match for what quadlet does imho, but yeah, really there isn't I think any particular reason to hardcode it.

I don't think we really should allow per-quadlet-file options for runtime though. It seems like this would be more of a global option for all quadlet services? Or maybe just use whatever the systemwide default is for all podman runs.

@alexlarsson
Copy link
Contributor

@giuseppe And log=passthrough work with runc too?

@edsantiago
Copy link
Member

Please add:

diff --git a/test/system/252-quadlet.bats b/test/system/252-quadlet.bats
index 03b6497d7..b0cdc4cc5 100644
--- a/test/system/252-quadlet.bats
+++ b/test/system/252-quadlet.bats
@@ -16,6 +16,11 @@ function start_time() {
 function setup() {
     skip_if_remote "quadlet tests are meaningless over remote"
 
+    runtime=$(podman_runtime)
+    if [[ "$runtime" != "crun" ]]; then
+        skip "runtime is $runtime; quadlet only works with crun"
+    fi
+
     test -x "$QUADLET" || die "Cannot run quadlet tests without executable \$QUADLET ($QUADLET)"
 
     start_time

@giuseppe
Copy link
Member

@giuseppe And log=passthrough work with runc too?

I've not tried but it should work there as well, the handling is in conmon

@TomSweeneyRedHat
Copy link
Member

Once @edsantiago 's concern is addressed, LGTM

@ygalblum
Copy link
Contributor

Following the comments from @alexlarsson and @giuseppe about the ability to use other runtimes, I think we should not merge this change and instead remove the hardcoded setting in Quadlet. I'll open a PR for if

@dcermak
Copy link
Contributor Author

dcermak commented Feb 22, 2023

Following the comments from @alexlarsson and @giuseppe about the ability to use other runtimes, I think we should not merge this change and instead remove the hardcoded setting in Quadlet. I'll open a PR for if

Agreed, sounds like the much better solution.

@dcermak dcermak closed this Feb 22, 2023
@dcermak dcermak deleted the add-crun-dep branch February 22, 2023 07:42
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 8, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants