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 virtiofsd to rpm spec #23085

Closed
wants to merge 1 commit into from
Closed

Add virtiofsd to rpm spec #23085

wants to merge 1 commit into from

Conversation

baude
Copy link
Member

@baude baude commented Jun 24, 2024

With the merge of #22920, we now require virtiofsd on Linux for machine mounts.

Does this PR introduce a user-facing change?

Add virtiofsd to required package for Linux

With the merge of containers#22920, we now require virtiofsd on Linux for machine
mounts.

Signed-off-by: Brent Baude <bbaude@redhat.com>
Copy link
Contributor

openshift-ci bot commented Jun 24, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude

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 Jun 24, 2024
@baude baude added No New Tests Allow PR to proceed without adding regression tests and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note labels Jun 24, 2024
@@ -93,6 +93,7 @@ BuildRequires: systemd
BuildRequires: systemd-devel
Requires: catatonit
Requires: conmon >= 2:2.1.7-2
Requires: virtiofsd
Copy link
Contributor

@cgwalters cgwalters Jun 24, 2024

Choose a reason for hiding this comment

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

Hmm, I thought we talked about a new podman-machine subpackage where this requirement could live (and we could add qemu there, or even in the future libvirt say)?

It's definitely not required to just run containers right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, as long as we're touching the spec, we also must add a ln -sr /usr/libexec/virtiofs /usr/libexec/podman/ into it...which I think would be best done in that new "mostly metadata" subpackage.

Copy link
Member Author

Choose a reason for hiding this comment

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

im not going to wait for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm...but are you sure you want to pull in virtiofsd everywhere today? It seems inconsistent as it's only a requirement for podman machine, but we don't also depend on qemu.

Copy link
Member

Choose a reason for hiding this comment

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

IMO I don't think a requires on the podman package is right, it is not required for normal operation on linux, podman machine is some extra most users don't need which why I suggested creating a podman-machine package which holds the deps.

Copy link
Member

Choose a reason for hiding this comment

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

@baude subpackage for podman-machine which only fetches the additional deps will probably cause the least pushback from other projects. machine users will explicitly need to dnf install podman-machine then.

Copy link
Member

Choose a reason for hiding this comment

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

Set it up as recommends then or suggests. I don't think it is a big ideal if the package does not exists. But the podman machine commands will fail if it is not their by mistake. This means podman machine needs to be made smarter when qemu is not available.

Copy link
Member Author

Choose a reason for hiding this comment

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

thats my point. either do you subpackage now or merge this. we dont have any releases going out so this is for dev only. @lsm5 if you do the subpackage, refer to this PR so i know to close it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, because we look in $PATH for qemu, and after some debate due to the variations of virtiofsd's location on different OS/distros we ended up requiring a /usr/libexec/podman symlink...the status quo will just break podman machine users on these platforms unless they add that symlink into their packaging.

Copy link
Member

Choose a reason for hiding this comment

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

@baude i have a draft PR at #23095

@cgwalters got a link to the symlink change please?

@baude
Copy link
Member Author

baude commented Jun 25, 2024

/

@baude baude closed this Jun 25, 2024
@stale-locking-app stale-locking-app 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 24, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Sep 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. No New Tests Allow PR to proceed without adding regression tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants