Skip to content
This repository has been archived by the owner on Aug 25, 2021. It is now read-only.

30ignition: remove initramfs networking #78

Merged
merged 1 commit into from
May 1, 2019
Merged

30ignition: remove initramfs networking #78

merged 1 commit into from
May 1, 2019

Conversation

yuqi-zhang
Copy link

Add coreos-remove-initramfs-network.service to run after ignition
has finished using initramfs networking, so NetworkManager properly
brings up ignition-configured networking in the real root. Otherwise
the initramfs network persists into the real root.

Signed-off-by: Yu Qi Zhang [email protected]

@yuqi-zhang yuqi-zhang mentioned this pull request Apr 30, 2019
Closed
jlebon
jlebon reviewed Apr 30, 2019
View reviewed changes
dracut/30ignition/ignition-generator Outdated
@@ -39,6 +39,7 @@ if $(cmdline_bool 'ignition.firstboot' 0); then
add_requires ignition-disks.service
add_requires ignition-files.service
add_requires ignition-ask-var-mount.service
add_requires coreos-remove-initramfs-network.service
Copy link
Member

Choose a reason for hiding this comment

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

How about s/remove/teardown/ ?

Copy link
Author

Choose a reason for hiding this comment

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

will do

dracut/30ignition/module-setup.sh Outdated

# clean up networking so when we switch to the real root,
# NetworkManager will set up the correct ignition-configured networking
# inst_hook cleanup 10 "$moddir/networking-cleanup"
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for keeping this?

Copy link
Author

Choose a reason for hiding this comment

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

Nope, will remove, thanks for catching that!

dracut/30ignition/coreos-remove-initramfs-network.sh Outdated
interface=$(basename "$f")
ip link set $interface down
ip addr flush dev $interface
rm -f -- /tmp/net.$interface.did-setup
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Author

Choose a reason for hiding this comment

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

Hmm the file does exist in the initramfs, but it does not seem to cause errors if not removed. I just followed dracut's ifdown

Copy link
Member

Choose a reason for hiding this comment

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

Hmm OK, I see it now. Let's maybe add a comment at the top of the script that this is mimicking dracut's ifdown()?

dracut/30ignition/coreos-remove-initramfs-network.service Outdated
[Service]
Type=oneshot
ExecStart=/usr/sbin/coreos-remove-initramfs-network
RemainAfterExit=yes
Copy link
Member

Choose a reason for hiding this comment

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

So one thing that would be cool here is to follow the pattern coreos-mount-var.service & ignition-complete.target use, e.g.:

[Unit]
...

# Make sure ExecStop= runs before we switch root
Conflicts=initrd-switch-root.target umount.target
Before=initrd-switch-root.target

# Make sure if ExecStop= fails, the boot fails
OnFailure=emergency.target
OnFailureJobMode=isolate

[Service]
Type=oneshot
RemainAfterExit=yes
ExecStop=...

That way we tear down the network just before switching root.

jlebon
jlebon reviewed Apr 30, 2019
View reviewed changes
dracut/30ignition/coreos-remove-initramfs-network.sh Outdated
# ex: ts=8 sw=4 sts=4 et filetype=sh

# Clean up the interfaces set up in the initramfs
for f in /sys/class/net/*; do
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this should probably be strengthened to handle the case where there are no network interfaces at all.

Copy link
Author

Choose a reason for hiding this comment

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

I believe for this particular use case, if there is no network interfaces at all, NM will correctly bring up interfaces for the real root? Or is there some specific scenarios you are thinking of.

I meant for this PR to handle only network interfaces right now, since it is the first step (and unblocks static IP generation). I agree that the functionality needs to be strengthened a lot more (maybe in followup PRs?)

Copy link
Member

Choose a reason for hiding this comment

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

Or is there some specific scenarios you are thinking of.

If a directory is empty, then for f in /my/dir/*; do will still iterate once with that string verbatim as f. Though hmm, I guess we can always count on at least lo to be there.

@jlebon
Copy link
Member

jlebon commented Apr 30, 2019

This looks fine to me for spec2x.

Higher-level, I think it'd be great if we only actually brought up networking if we needed it (i.e. if Ignition needs to fetch a config over the network). Then this new service would be conditional on that as well. Anyway, we can definitely leave that for later!

jlebon
jlebon reviewed Apr 30, 2019
View reviewed changes
dracut/30ignition/coreos-remove-initramfs-network.sh Outdated
@@ -0,0 +1,11 @@
#!/bin/bash
# -*- mode: shell-script; indent-tabs-mode: nil; sh-basic-offset: 4; -*-
# ex: ts=8 sw=4 sts=4 et filetype=sh
Copy link
Member

Choose a reason for hiding this comment

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

This is missing the standard set -euo pipefail.

@yuqi-zhang
30ignition: remove initramfs networking interfaces
55131c7 
Add coreos-remove-initramfs-network.service to run after ignition
has finished using initramfs networking, so NetworkManager properly
brings up ignition-configured networking in the real root. Otherwise
the initramfs network persists into the real root.

Signed-off-by: Yu Qi Zhang <[email protected]>
@yuqi-zhang
Copy link
Author

Added fixups and comments, also confirmed that dhcp still works with a base ignition conf, as well as if the service fails, we drop into emergency shell so the dependency checks out. Also did some investigation into dhclient but AFAICT its not running and will not try e.g. re-leasing

jlebon
jlebon approved these changes May 1, 2019
View reviewed changes
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Thanks, this LGTM! Will leave it open for a bit for others to review.

@jlebon jlebon merged commit a3a7522 into coreos:spec2x May 1, 2019
@dustymabe dustymabe added the spec2x Applies to the ignition spec2x branch label May 8, 2019
This was referenced Oct 17, 2019
Closed
Closed
dustymabe added a commit to dustymabe/ignition-dracut that referenced this pull request Mar 19, 2020
@dustymabe
add coreos-teardown-initramfs.service
b1ddef5 
This is a forward port of coreos-teardown-initramfs-network.service
from the spec2x branch [1] (used for RHEL CoreOS). When moving to NM
in the initrd [2] we decided that we also needed a mechanism to take down
the networking between the initramfs and the real root. While we would
like to use NetworkManager's logic to do this operation in the future
it's currently not easily achieved because NetworkManager is not running
persistently in the initramfs [3].

[1] coreos#78
[2] coreos/fedora-coreos-tracker#394
[3] https://bugzilla.redhat.com/show_bug.cgi?id=1814038
@dustymabe dustymabe mentioned this pull request Mar 19, 2020
Merged
dustymabe added a commit to dustymabe/ignition-dracut that referenced this pull request Mar 19, 2020
@dustymabe
add coreos-teardown-initramfs-network.service
1eabe00 
This is a forward port of coreos-teardown-initramfs-network.service
from the spec2x branch [1] (used for RHEL CoreOS). When moving to NM
in the initrd [2] we decided that we also needed a mechanism to take down
the networking between the initramfs and the real root. While we would
like to use NetworkManager's logic to do this operation in the future
it's currently not easily achieved because NetworkManager is not running
persistently in the initramfs [3].

[1] coreos#78
[2] coreos/fedora-coreos-tracker#394
[3] https://bugzilla.redhat.com/show_bug.cgi?id=1814038
dustymabe added a commit to dustymabe/ignition-dracut that referenced this pull request Mar 24, 2020
@dustymabe
add coreos-teardown-initramfs-network.service
f4fbdfd 
This is a forward port of coreos-teardown-initramfs-network.service
from the spec2x branch [1] (used for RHEL CoreOS). When moving to NM
in the initrd [2] we decided that we also needed a mechanism to take down
the networking between the initramfs and the real root. While we would
like to use NetworkManager's logic to do this operation in the future
it's currently not easily achieved because NetworkManager is not running
persistently in the initramfs [3].

[1] coreos#78
[2] coreos/fedora-coreos-tracker#394
[3] https://bugzilla.redhat.com/show_bug.cgi?id=1814038
dustymabe added a commit to dustymabe/ignition-dracut that referenced this pull request Mar 24, 2020
@dustymabe
add coreos-teardown-initramfs-network.service
8bdbbbe 
This is a forward port of coreos-teardown-initramfs-network.service
from the spec2x branch [1] (used for RHEL CoreOS). When moving to NM
in the initrd [2] we decided that we also needed a mechanism to take down
the networking between the initramfs and the real root. While we would
like to use NetworkManager's logic to do this operation in the future
it's currently not easily achieved because NetworkManager is not running
persistently in the initramfs [3].

[1] coreos#78
[2] coreos/fedora-coreos-tracker#394
[3] https://bugzilla.redhat.com/show_bug.cgi?id=1814038
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jlebon jlebon jlebon approved these changes

Assignees
No one assigned
Labels
spec2x Applies to the ignition spec2x branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yuqi-zhang @jlebon @dustymabe