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(45ifcfg): mark as deprecated and strictly opt-in #2529

Closed
wants to merge 1 commit into from

Conversation

jlebon
Copy link
Contributor

@jlebon jlebon commented Sep 29, 2023

The 45ifcfg module should be considered deprecated now since ifcfg files themselves are deprecated. They've been replaced by e.g. NetworkManager keyfiles or systemd-networkd files.

Currently, the 45ifcfg module checks if the /etc/sysconfig/network-scripts directory exists to know if to automatically get pulled in. However, on systems with NetworkManager, this directory always exists even if the functionality to read ifcfg files is disabled by default (NM kindly ships a README in there to ease migrating to keyfiles).

Since almost everyone should've already migrated to a more modern alternative, let's make this module purely opt-in now.

Checklist

  • I have tested it locally
  • I have reviewed and updated any documentation if relevant
  • I am providing new code and test(s) for it

@github-actions github-actions bot added modules Issue tracker for all modules ifcfg Issues related to the ifcfg module labels Sep 29, 2023
@jlebon
Copy link
Contributor Author

jlebon commented Sep 29, 2023

/cc @thom311 @bengal

@jlebon
Copy link
Contributor Author

jlebon commented Sep 29, 2023

An alternative is to tweak the conditional to check whether the directory is non-empty (skipping over the README) to know if ifcfg files are in use, but that creates a bootstrapping problem where you want ifcfg files generated on first boot to get propagated there in the first place. I also wanted to see if we could have a stronger stance on this and maybe eventually get rid of that module entirely.

jlebon added a commit to jlebon/fedora-coreos-config that referenced this pull request Sep 29, 2023
It's been a while since we've moved to only propagating NM keyfiles from
the initrd to the real root, so we no longer need this module.

It currently still gets pulled in due to the `/etc/sysconfig/network-
scripts` directory existing in the real root (which contains a README
by default).

There's a patch upstream to make it purely opt-in[[1]], but for now let's just
explicitly opt out ourselves.

(Note this has no impact on whether ifcfg in the real root is supported,
which it still is in RHCOS. It just stops uselessly generating ifcfg
files in the initrd.)

[1]: dracutdevs/dracut#2529
Copy link
Member

@aafeijoo-suse aafeijoo-suse left a comment

Choose a reason for hiding this comment

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

TEST-60-BONDBRIDGEVLANIFCFG using the network-legacy module in Fedora needs to be addressed: https://github.com/dracutdevs/dracut/actions/runs/6353041817/job/17256980641?pr=2529#step:4:1186

The ifcfg module is required and not automatically included. See an old run, e.g.: https://github.com/dracutdevs/dracut/actions/runs/6309637128/job/17129970526?pr=2527#step:4:101

Apart from that, I'm ok with deprecating RH-specific code, or removing this module if it's no longer needed.

@bengal
Copy link
Contributor

bengal commented Oct 2, 2023

Makes sense to me, ifcfg support is also being deprecated in NetworkManager and will be removed eventually.

HuijingHei pushed a commit to coreos/fedora-coreos-config that referenced this pull request Oct 13, 2023
It's been a while since we've moved to only propagating NM keyfiles from
the initrd to the real root, so we no longer need this module.

It currently still gets pulled in due to the `/etc/sysconfig/network-
scripts` directory existing in the real root (which contains a README
by default).

There's a patch upstream to make it purely opt-in[[1]], but for now let's just
explicitly opt out ourselves.

(Note this has no impact on whether ifcfg in the real root is supported,
which it still is in RHCOS. It just stops uselessly generating ifcfg
files in the initrd.)

[1]: dracutdevs/dracut#2529
@jlebon
Copy link
Contributor Author

jlebon commented Oct 26, 2023

TEST-60-BONDBRIDGEVLANIFCFG using the network-legacy module in Fedora needs to be addressed: dracutdevs/dracut/actions/runs/6353041817/job/17256980641?pr=2529#step:4:1186

The ifcfg module is required and not automatically included. See an old run, e.g.: dracutdevs/dracut/actions/runs/6309637128/job/17129970526?pr=2527#step:4:101

Hmm OK, I'm not sure if I'm following the testing code properly. Do I just need a -m network-legacy here?

@LaszloGombos
Copy link
Collaborator

LaszloGombos commented Oct 26, 2023

Hmm OK, I'm not sure if I'm following the testing code properly. Do I just need a -m network-legacy here?

@jlebon can you please try to add this and see if that resolves the regression with TEST-60. Thanks !

diff --git a/test/TEST-60-BONDBRIDGEVLAN/test.sh b/test/TEST-60-BONDBRIDGEVLAN/test.sh
index 5110ddb1..a10630dc 100755
--- a/test/TEST-60-BONDBRIDGEVLAN/test.sh
+++ b/test/TEST-60-BONDBRIDGEVLAN/test.sh
@@ -367,7 +367,7 @@ test_setup() {
     "$DRACUT" -l -i "$TESTDIR"/overlay / \
         --no-early-microcode \
         -o "plymouth" \
-        -a "debug ${USE_NETWORK}" \
+        -a "debug ${USE_NETWORK} ifcfg" \
         --no-hostonly-cmdline -N \
         -f "$TESTDIR"/initramfs.testing "$KVERSION" || return 1

The `45ifcfg` module should be considered deprecated now since
ifcfg files themselves are deprecated. They've been replaced by e.g.
NetworkManager keyfiles or systemd-networkd files.

Currently, the `45ifcfg` module checks if the `/etc/sysconfig/network-
scripts` directory exists to know if to automatically get pulled in.
However, on systems with NetworkManager, this directory always exists
even if the functionality to read ifcfg files is disabled by default (NM
kindly ships a README in there to ease migrating to keyfiles).

Since almost everyone should've already migrated to a more modern
alternative, let's make this module purely opt-in now.
@github-actions github-actions bot added the test Issues related to testing label Oct 27, 2023
@jlebon
Copy link
Contributor Author

jlebon commented Oct 27, 2023

Hmm OK, I'm not sure if I'm following the testing code properly. Do I just need a -m network-legacy here?

@jlebon can you please try to add this and see if that resolves the regression with TEST-60. Thanks !

Ahhh thanks. My mind got mixed up while looking at the test code and was thinking about network-legacy instead of ifcfg.

Looks like that fixes it, though there are other tests that failed. Seems like we'll need to tweak a few others.

@LaszloGombos
Copy link
Collaborator

Looks like that fixes it, though there are other tests that failed. Seems like we'll need to tweak a few others.

Remaining test failures are not regressions, they are existing failures. PR LGTM.

@LaszloGombos LaszloGombos added this to the dracut-061 milestone Oct 30, 2023
@aafeijoo-suse aafeijoo-suse removed this from the dracut-061 milestone Nov 18, 2023
aaradhak pushed a commit to aaradhak/fedora-coreos-config that referenced this pull request Mar 18, 2024
It's been a while since we've moved to only propagating NM keyfiles from
the initrd to the real root, so we no longer need this module.

It currently still gets pulled in due to the `/etc/sysconfig/network-
scripts` directory existing in the real root (which contains a README
by default).

There's a patch upstream to make it purely opt-in[[1]], but for now let's just
explicitly opt out ourselves.

(Note this has no impact on whether ifcfg in the real root is supported,
which it still is in RHCOS. It just stops uselessly generating ifcfg
files in the initrd.)

[1]: dracutdevs/dracut#2529
Copy link

stale bot commented Apr 22, 2024

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 Apr 22, 2024
@jlebon jlebon closed this May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ifcfg Issues related to the ifcfg module modules Issue tracker for all modules stale communication is stuck test Issues related to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants