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

fix(network-manager): support nm-initrd-generator under NetworkManager #2123

Closed
wants to merge 1 commit into from

Conversation

LaszloGombos
Copy link
Collaborator

@LaszloGombos LaszloGombos commented Dec 12, 2022

Upstream NetworkManager explicitly supports Debian. This PR proposes the same for the network-manager dracut module.

Upstream NetworkManager explicitly supports prefix for libexecdir here .

In network-manager/module-setup.sh nm-initrd-generator is already marked optional to accomodate different locations for nm-initrd-generator in different setups as the path is not stable between Linux installations and versions.

The file in Debian 12 is located in /usr/lib/NetworkManager/nm-initrd-generator .

The file location has been in stable Debian for about 4 years now (since Debian 10).

This issue impacts over 50 Linux distributions and blocks testing NetwrokManager for Debian on the upstream CI.

From FHS 3.0

Some previous versions of this document did not support /usr/libexec, despite it being standard practice in a number of environments. To accomodate this restriction, it became common practice to use /usr/lib instead. Either practice is now acceptable, but each application must choose one way or the other to organize itself.

CC @Mrfai @bdrung

@LaszloGombos LaszloGombos added the debian Issue tracker for the Debian distribution label Dec 12, 2022
@github-actions github-actions bot added modules Issue tracker for all modules network Issues related to the network module network-manager Issues related to the network-manager module labels Dec 12, 2022
@LaszloGombos LaszloGombos added this to the dracut-058 milestone Dec 12, 2022
@Mrfai
Copy link
Contributor

Mrfai commented Dec 12, 2022

This look very similar to the patch I use for the Debian version of dracut:
https://salsa.debian.org/debian/dracut/-/blob/master/debian/patches/nm-path

I will update my patch and decide if I still need it. I like the use of find_binary

IMO my patch has a better regex for inst_multiple

Comment on lines -33 to +34
inst_multiple -o /usr/{lib,libexec}/nm-initrd-generator
inst_multiple -o /usr/{lib,libexec}/nm-daemon-helper
inst_multiple -o /usr/{lib,libexec}{,/NetworkManager}/nm-initrd-generator
inst_multiple -o /usr/{lib,libexec}{,/NetworkManager}/nm-daemon-helper
Copy link
Member

Choose a reason for hiding this comment

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

@thaller, @lkundrak: are you folks aware that some put these binaries in /usr/libexec/NetworkManager? Have you considered making that the upstream default rather than dropping them directly in /usr/libexec?

@Conan-Kudo
Copy link
Member

File locations for debian (and debian based distro's) - https://packages.debian.org/sid/amd64/network-manager/filelist

I fired off an email to ask about reconciling the paths with upstream NetworkManager since Debian adopted FHS 3.0 some time ago and allows /usr/libexec.

@LaszloGombos
Copy link
Collaborator Author

You're adding different checks for the same thing (NetworkManager) using different PRs... it's quite confusing.

@aafeijoo-suse asked to combine this PR with #2081 .

Closing this PR in favor of #2081 .

@LaszloGombos
Copy link
Collaborator Author

Since #2081 is stalled, reopening this PR.

@johannbg
Copy link
Collaborator

Different distro's place nm-initrd-generator in different locations. In network-manager/module-setup.sh nm-initrd-generator is already marked optional to accomodate different locations for nm-initrd-generator.

Hmm that's probably a historic/snug in bug as in we support standards and the effort of the people creating those standards and we do so by adhering to those standards not by support working around those standards ( but are open for a strong technical argument for deviating from the standards if such thing exist which is a bug in the standard then ) so workarounds for downstream that deviate from the standards is something we dont support and downstream has to carry the support for it, themselves and in the progress all the effort in trying to maintain that support that deviation in otherwords we should not have to call find to find a binary in which locations should already be standardized upon.
Us having to do so is either a bug upstream, in the standard or religions deviation downstream ( which we dont support ).

That said like @aafeijoo-suse I'm getting a bit confused about all this work it's like you are constantly trying to refactor our code to fit your test case for debian?

@LaszloGombos
Copy link
Collaborator Author

LaszloGombos commented Dec 21, 2022

Closing with the conclusion that there is no interest to find a solution for current Debian and debian based distros upstream.

Debian and debian based distros should continue to carry this patch instead.

This of course means that dracut will not be able to test network-manager module for Debian container here upstream as it would all fail and have to instead invest time and effort to skip network-manager tests just for the Debian container.

I will try to find those standards for the file system location for nm-initrd-generator that are mentioned as a decisive factor for this PR yet not referenced. If somebody could help reference those standards for them, please drop a link on this PR.

@LaszloGombos LaszloGombos removed this from the dracut-058 milestone Dec 21, 2022
@LaszloGombos LaszloGombos deleted the nm_we branch January 11, 2023 14:40
@LaszloGombos LaszloGombos restored the nm_we branch March 2, 2023 23:42
@LaszloGombos LaszloGombos reopened this Mar 2, 2023
@LaszloGombos LaszloGombos changed the title fix(network): test for NetworkManager binary fix(network-manager): support nm-initrd-generator under NetworkManager Mar 2, 2023
@LaszloGombos LaszloGombos removed the network Issues related to the network module label Mar 2, 2023
@LaszloGombos
Copy link
Collaborator Author

Reopening based on recent policy changes and recent review feedbacks where it was suggested that the project should consider distro specific changes even if those changes are intentionally different than the corresponding upstream repo's for the purpose of networking related modules in dracut.

This PR is now only limited to find nm-initrd-generator on debian (and debian based distro's).

@LaszloGombos LaszloGombos added the bug Our bugs label Mar 2, 2023
@LaszloGombos
Copy link
Collaborator Author

I wrote down a few of my thoughts on how to review distribution specific changes on the wiki. In the past there was very little consistency around the reviews of distribution specific changes.

https://github.com/dracutdevs/dracut/wiki/Dracut-development#reviews

@LaszloGombos LaszloGombos added this to the dracut-060 milestone Apr 1, 2023
@LaszloGombos LaszloGombos removed the request for review from danimo May 8, 2023 01:55
Copy link
Contributor

@bdrung bdrung left a comment

Choose a reason for hiding this comment

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

All four paths /usr/{lib,libexec}{,/NetworkManager} are FHS compliant and this change will support the path in Debian/Ubuntu. So +1 for this change from my side.

@stale
Copy link

stale bot commented Oct 15, 2023

This issue is being marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. If this is still an issue in the latest release of Dracut and you would like to keep it open please comment on this issue within the next 7 days. Thank you for your contributions.

@stale stale bot added the stale communication is stuck label Oct 15, 2023
@bdrung
Copy link
Contributor

bdrung commented Oct 16, 2023

This PR is still valid.

@stale stale bot removed the stale communication is stuck label Oct 16, 2023
@LaszloGombos LaszloGombos added this to the dracut-061 milestone Nov 1, 2023
@aafeijoo-suse aafeijoo-suse removed this from the dracut-061 milestone Nov 18, 2023
Copy link
Contributor

@pvalena pvalena left a comment

Choose a reason for hiding this comment

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

LGTM.

@LaszloGombos LaszloGombos added the bug Our bugs label Nov 29, 2023
@Conan-Kudo
Copy link
Member

FYI, Debian 13 "Trixie" moves it to /usr/libexec

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Our bugs debian Issue tracker for the Debian distribution modules Issue tracker for all modules network-manager Issues related to the network-manager module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants