coreos-teardown-initramfs-network: order ExecStop after other ignition units

[dustymabe]

We want to run the teardown after all other Ignition stages
have run because some platforms (like Packet) do remote status
reporting for each Ignition stage. Since we are tearing down
the networking using an ExecStop we need to make sure we run
the ExecStop after any other ignition*.service unit's ExecStop.
The only other one right now is ignition-mount that has an ExecStop
for doing an unmount. Since the ordering for ExecStop is the
opposite of ExecStart we need to use Before=ignition-mount.service.

Partial fix for coreos/fedora-coreos-tracker#440

This would most likely not be necessary if Ignition cached an
empty result: coreos/ignition#950

[bgilbert]

I actually implemented exactly this fix this weekend, for Packet status reporting, before settling on coreos/ignition#952 instead.

In principle I think this PR would break propagation of NM configs into the real root, in cases where Ignition has mounted a filesystem onto the destination path, because it'll cause Ignition to unmount that FS too early. (This wouldn't apply to the typical case where /etc/NetworkManager is in /sysroot, though.)

[bgilbert]

(To be clear, I'm arguing that we shouldn't take this approach; see coreos/fedora-coreos-tracker#440 (comment).)

[dustymabe]

In principle I think this PR would break propagation of NM configs into the real root, in cases where Ignition has mounted a filesystem onto the destination path, because it'll cause Ignition to unmount that FS too early. (This wouldn't apply to the typical case where /etc/NetworkManager is in /sysroot, though.)

Thanks for bringing this up. I'm not sure I fully understand so I'll ask some questions :)

Are there cases where /etc/ isn't in the root filesystem? Or are you talking about cases where the root filesystem is getting rewritten? Do we support that?

(To be clear, I'm arguing that we shouldn't take this approach; see coreos/fedora-coreos-tracker#440 (comment).)

+1. Though it sounds like you are still arguing for a change to be made here to make it not race. i.e. we'd want to flip from Before=ignition-mount.service to After= so that we guarantee the teardown happens before the unmount.

[bgilbert]

Discussed in IRC. I agree that /etc/NetworkManager not being in the root FS is an... obscure case. Objection withdrawn.

[dustymabe]

ok marking this as ready for review and doing some final testing

[jlebon]

I think we could generalize this to just "tear down networking on switchroot" instead of making it about Ignition.

Hmm, a more "technically correct" approach is to tie this service to whether networking was brought up. So e.g. it'd be a service that gets pulled in by network.target in the Ignition generator, and it would naturally get stopped via Conflicts=initrd-switch-root.target on switchroot. This also meshes better with the conditional networking work going on.

Anyway, not against getting this easy fix in and rewiring this in the future!

[dustymabe]

I think we could generalize this to just "tear down networking on switchroot" instead of making it about Ignition.

Yeah you're probably right. I'm not opposed to doing that in the future.

Hmm, a more "technically correct" approach is to tie this service to whether networking was brought up. So e.g. it'd be a service that gets pulled in by network.target in the Ignition generator, and it would naturally get stopped via Conflicts=initrd-switch-root.target on switchroot. This also meshes better with the conditional networking work going on.

Anyway, not against getting this easy fix in and rewiring this in the future!

+1 thanks for that. I think let's get this fix into the a new testing release to fix the regression (will merge after more testing) and then we can do a better fix for all of this in the next testing release that's coming out next week.