NetworkManager implements activator in upstream service files

[holmanb]

Bug report

A couple of years ago, we synced the downstream centos-stream customizations into our upstream service files. This included a networkmanager restart implemented in the service files, which is quite peculiar and shouldn't be required.

I don't have the downstream history of that code, but I suspect that this is was added because the ifupdown activator implementation of "bring up each interface" is not sufficient.

The high level concept of activators and renderers in cloud-init is as follows:

1) renderer - if config is received before network service starts, just render the configuration to the filesystem and let the system network daemon bring up the system pre-configured whenever it starts 2) activator - if config is received after network service starts, render the configuration and then "activate the network"

Both codepaths are necessary because sometimes the configuration is received before the network daemon is running - sometimes the configuration is received after the daemon is running.

The old activator concept for "activate the network" under ifupdown would literally just iterate over interfaces and bring them up. Today with the systemd and netplan renderers, we just run a command to restart or reload the appropriate service[1][2].

I think that the existence of the NetworkManager.service restart in cloud-final.service is evidence that cloud-init's network activation is broken - unfortunately without the history of the downstream patch, I'm unsure whether the restart was initially required when used with the ifconfig activator, or with the network-manager activator. Either way, I strongly suspect that what we want to happen with NetworkManager is something like a systemctl reload-or-try-restart NetworkManager.service rather than the manual ifconfig or nmcli commands per-interface like we currently do.

[holmanb]

@major - Do you have context on the history of that change to the service file that you could share? A bug report or PR would help a lot (I tried looking but didn't find anything useful).

@major @lkundrak @Conan-Kudo - Do you see any issues with running systemctl reload-or-try-restart NetworkManager.service in place of the iterating over each interface and running nmcli on it?

I think that reloading the NetworkManager service probably makes more sense so that it can do things like add systemd .link files for devices which are not yet visible in userspace.

cc @ani-sinha for additional Red Hat input and awareness

[Conan-Kudo]

I am not sure why RHEL was doing that extra stuff when Fedora wasn't. Fedora has been using NetworkManager for cloud-init networking for over two years.

I don't see a particular problem with reload-or-try-restart for the service, but I think it might make sense to also give those template files another look-see, especially with how complex they've gotten lately.

[ani-sinha]

Its a ugly hack and is related to this change in sysconfig renderer:

        content = networkmanager_conf.NetworkManagerConf("")

        # If DNS server information is provided, configure
        # NetworkManager to not manage dns, so that /etc/resolv.conf
        # does not get clobbered.
        if network_state.dns_nameservers:
            content.set_section_keypair("main", "dns", "none") 

The reason why we need to restart network manager is because when the network manager starts earlier than cloud-init (therefore, when /etc/NetworkManager/conf.d/99-cloud-init.conf has not been created yet), both cloud-init and network manager tries to own and write to /etc/resolv.conf . So restarting network manager once cloud-init has fully started makes sure that NM reads the conf file that contains :

[main]
dns = none

and does not manage dns (and therefore does not try to manipulate /etc/resolv.conf). The above change is redundant when cloud-init is not managing the network because in that case cloud-init does not generate the config file and NM is free to manage DNS (or something like dhclient).

I have been aware of this for a while and thought of a number of ways to avoid this hack. However, any change we make risks breaking stuff given the complicated interactions with various components managing DNS. Therefore, I have resisted changing this. I always thought it was downstream only but seems I am wrong.

[holmanb]

The reason why we need to restart network manager is because when the network manager starts earlier than cloud-init (therefore, when /etc/NetworkManager/conf.d/99-cloud-init.conf has not been created yet), both cloud-init and network manager tries to own and write to /etc/resolv.conf . So restarting network manager once cloud-init has fully started makes sure that NM reads the conf file that contains :

Thanks for the context @ani-sinha.

Since cloud-init-local.service (where the renderer runs) is ordered Before=NetworkManager.service, the issue that you've described can only happen when this code path is called from cloud-init.service. During this stage, cloud-init first renders then activates the backend network manager. Therefore, I think that the proposal to make the activator run systemctl reload-or-try-restart NetworkManager.service should address this problem.

[ani-sinha]

The reason why we need to restart network manager is because when the network manager starts earlier than cloud-init (therefore, when /etc/NetworkManager/conf.d/99-cloud-init.conf has not been created yet), both cloud-init and network manager tries to own and write to /etc/resolv.conf . So restarting network manager once cloud-init has fully started makes sure that NM reads the conf file that contains :

Thanks for the context @ani-sinha.

Since cloud-init-local.service (where the renderer runs) is ordered Before=NetworkManager.service, the issue that you've described can only happen when this code path is called from cloud-init.service. During this stage, cloud-init first renders then activates the backend network manager. Therefore, I think that the proposal to make the activator run systemctl reload-or-try-restart NetworkManager.service should address this problem.

Yes I also suspect so and if someone cooks up a patch, we are happy to test it downstream. This should also remove the redundant NM restart when cloud-init is not managing network at all, unless I am mistaken. So this should avoid the problem of NM trying to own /etc/resolv.conf after it has been written to, by say dhclient. Downstream ticket: https://issues.redhat.com/browse/RHEL-7271

[ani-sinha]

@holmanb just to be clear - you are proposing systemctl reload-or-try-restart NetworkManager.service when Network Manager activator is used, correct? This would be what we want because simply doing nmcli conn load and nmcli conn up does not force Network Manager to reload its config file. cloud-init generates some specific NM config files to address certain situations. So nmcli conn load/up is not enough for cloud-init's purpose.

[holmanb]

@holmanb just to be clear - you are proposing systemctl reload-or-try-restart NetworkManager.service when Network Manager activator is used, correct?

Correct

Yes I also suspect so and if someone cooks up a patch, we are happy to test it downstream.

I'd be happy to. I'll ping you when I have one ready.

This should also remove the redundant NM restart when cloud-init is not managing network at all, unless I am mistaken.

Agreed

[holmanb]

@ani-sinha If you don't mind testing here is the branch or patch (based on main):

Branch: https://github.com/holmanb/cloud-init/tree/holmanb/networkmanager-activator Patch: 0001-fix-NetworkManager-Fix-network-activator.txt

[holmanb]

I think it might make sense to also give those template files another look-see, especially with how complex they've gotten lately.

@Conan-Kudo Agreed, I think it would be best to standardize and eliminate redundancy wherever possible. I'm planning to draft a proposal for review that should simplify our service files, but I plan to wait on that until this NetworkManager issues gets resolved.

[ani-sinha]

Thanks for the context @ani-sinha.

Red Hat BZ originally filed for this that introduced this change downstream only and then we sync'd it to upstream: https://bugzilla.redhat.com/show_bug.cgi?id=1748015

and this is the commit log :

    cloud-init service is set to start before NetworkManager service starts,
    but this does not avoid a race condition between them. NetworkManager
    starts before cloud-init can write `dns=none' to the file:
    /etc/NetworkManager/conf.d/99-cloud-init.conf. This way NetworkManager
    doesn't read the configuration and erases all resolv.conf values upon
    shutdown. On the next reboot neither cloud-init or NetworkManager will
    write anything to resolv.conf, leaving it blank.

    This patch introduces a NM reload (try-restart) at the end of cloud-init
    start up so it won't erase resolv.conf upon first shutdown.

[holmanb]

Thanks for the context @ani-sinha.

Red Hat BZ originally filed for this that introduced this change downstream only and then we sync'd it to upstream: https://bugzilla.redhat.com/show_bug.cgi?id=1748015

and this is the commit log :

    cloud-init service is set to start before NetworkManager service starts,
    but this does not avoid a race condition between them. NetworkManager
    starts before cloud-init can write `dns=none' to the file:
    /etc/NetworkManager/conf.d/99-cloud-init.conf. This way NetworkManager
    doesn't read the configuration and erases all resolv.conf values upon
    shutdown. On the next reboot neither cloud-init or NetworkManager will
    write anything to resolv.conf, leaving it blank.

    This patch introduces a NM reload (try-restart) at the end of cloud-init
    start up so it won't erase resolv.conf upon first shutdown.

+1 that helps a lot thanks!

[holmanb]

@ani-sinha If you don't mind testing here is the branch or patch (based on main):

Branch: https://github.com/holmanb/cloud-init/tree/holmanb/networkmanager-activator Patch: 0001-fix-NetworkManager-Fix-network-activator.txt

@ani-sinha ping! any progress?

[ani-sinha]

@ani-sinha If you don't mind testing here is the branch or patch (based on main): Branch: https://github.com/holmanb/cloud-init/tree/holmanb/networkmanager-activator Patch: 0001-fix-NetworkManager-Fix-network-activator.txt

@ani-sinha ping! any progress?

Pinged Amy - our QE but she was busy with something. She will update us here when she has completed testing.

[xiachen-rh]

Hi @holmanb and @ani-sinha , I tested this patch that Ani helped backport to downstream, no regression issue found, /etc/resolv.conf can keep same after reboot. As it was a race problem I created 100 VMs on OpenStack to test it. However I have a question about the patch, how could I know the code "bring_up_interfaces" is called? I asked this question because I just removed the code about "reload-or-try-restart" in systemd/cloud-final.service.tmpl, it also worked fine.

[ani-sinha]

Hi @holmanb and @ani-sinha ,

I tested this patch that Ani helped backport to downstream, no regression issue found, /etc/resolv.conf can keep same after reboot. As it was a race problem I created 100 VMs on OpenStack to test it.

However I have a question about the patch, how could I know the code "bring_up_interfaces" is called?

I asked this question because I just removed the code about "reload-or-try-restart" in systemd/cloud-final.service.tmpl, it also worked fine.

There should be a debug log indicating the command "reload-or-try-restart" was run.

[holmanb]

However I have a question about the patch, how could I know the code "bring_up_interfaces" is called? ... There should be a debug log indicating the command "reload-or-try-restart" was run.

Agreed - the logs should tell us that it happened.

As it was a race problem I created 100 VMs on OpenStack to test it. ... I asked this question because I just removed the code about "reload-or-try-restart" in systemd/cloud-final.service.tmpl, it also worked fine.

@xiachen-rh Sometimes to create a reliable reproducer for testing races I'll modify the systemd ordering to force the race. You might try adding a new service which starts after networkmanager and blocks cloud-init.service (where the activator runs) for some period of time.

[TheRealFalcon]

However I have a question about the patch, how could I know the code "bring_up_interfaces" is called?

This will only get called if no datasource was found during cloud-init's init-local phase. This is not a common use case and not relevant on any of the major clouds.

I think the main way to trigger this is to use a NoCloud datasource with an http seed that somehow alters the existing network configuration.

[ani-sinha]

However I have a question about the patch, how could I know the code "bring_up_interfaces" is called?

This will only get called if no datasource was found during cloud-init's init-local phase. This is not a common use case and not relevant on any of the major clouds.

I think the main way to trigger this is to use a NoCloud datasource with an http seed that somehow alters the existing network configuration.

OK so looking at the code, I see the following call chain: main_init() -> (stage 6) init.apply_network_config(bring_up = bring_up_interfaces) -> distro.apply_network_config(bring_up) -> activator.bring_up_all_interfaces()

Now bring_up_interfaces is set to true only when _should_bring_up_interfaces() returns True which is when its not cloud-init local stage (cloud-init is not run with --local) or disable_network_activation is not set in cloud-init config. In any case, if no attempt is made to bring up the interface, we should see the log LOG.debug("Not bringing up newly configured network interfaces")

Another question I have is this. Previously, before this change, the NetworkManager activator was running bring_up_interface() (its own implementation) because in NetworkActivator, we had:

   @classmethod
    def bring_up_interfaces(cls, device_names: Iterable[str]) -> bool:
        """Bring up specified list of interfaces.                                                                                                                                  

        Return True is successful, otherwise return False                                                                                                                          
        """
        return all(cls.bring_up_interface(device) for device in device_names)

    @classmethod
    def bring_up_all_interfaces(cls, network_state: NetworkState) -> bool:
        """Bring up all interfaces.                                                                                                                                                

        Return True is successful, otherwise return False                                                                                                                          
        """
        return cls.bring_up_interfaces(
            [i["name"] for i in network_state.iter_interfaces()]
        )

Now, with this change, we are not running cls.bring_up_interface() since we are overriding cls.bring_up_interfaces() in Network Manager activator with our change. It would seem this would break things @holmanb

[ani-sinha]

Since cloud-init-local.service (where the renderer runs) is ordered Before=NetworkManager.service, the issue that you've described can only happen when this code path is called from cloud-init.service. During this stage, cloud-init first renders then activates the backend network manager.

@holmanb I am confused in this part. Are you saying that renderer.render_network_state() is only called during --local stage? I could not verify that either way looking at the code.

[holmanb]

Now, with this change, we are not running cls.bring_up_interface() since we are overriding cls.bring_up_interfaces() in Network Manager activator with our change.

Correct. Previously activators would iterate through each interface and bring each one up a la ifup eth0. It looks like when the NetworkManager activator was implemented, this copied the same approach, albeit with nmcli instead.

It would seem this would break things @holmanb

How so? If NetworkManager brings up configured interfaces when restarted, I wouldn't expect breakage.

The reason why we need to restart network manager is because when the network manager starts earlier than cloud-init (therefore, when /etc/NetworkManager/conf.d/99-cloud-init.conf has not been created yet), both cloud-init and network manager tries to own and write to /etc/resolv.conf . So restarting network manager once cloud-init has fully started makes sure that NM reads the conf file that contains :

Since cloud-init-local.service (where the renderer runs) is ordered Before=NetworkManager.service, the issue that you've described can only happen when this code path is called from cloud-init.service. During this stage, cloud-init first renders then activates the backend network manager.

@holmanb I am confused in this part. Are you saying that renderer.render_network_state() is only called during --local stage?

I wasn't being clear - renderer.render_network_state() gets called from both stages, however the activator only runs during the Network stage (cloud-init init).

The point I was making was that if the network configuration is available during Local stage[1], then the network configuration will be rendered before NetworkManager ever starts which should prevent this issue. Restarting NetworkManager should only be required when DNS configuration isn't received during Local stage but is received later during Network stage

[1] which as @TheRealFalcon pointed out, is true on most clouds / datasources

[ani-sinha]

If NetworkManager brings up configured interfaces when restarted, I wouldn't expect breakage.

Our NM team confirmed that this isn't strictly true. Network Manager only brings up those interfaces where connection.autoconnect = true is set (default is True so if its not explicitly set to False the interface will be brought up). There are also some other details depending on the NM version. So I think it would be safer if we call cls.bring_up_interface() for now for NM activator. So essentially add this to your existing function

+        return _alter_interface(
+            ["systemctl", "reload-or-try-restart", "NetworkManager.service"],
+            "all",
+        ) and all(cls.bring_up_interface(device) for device in device_names)

Two things with the above argument. I am not sure if we should also bring up those interfaces that are explicitly marked as autoconnect = False . If we do not want to do that, then existing change is fine. Secondly, I see another issue with the above. The nmcli command from bring_up_interface() will likely fail if the service has not fully restarted. @holmanb

[ani-sinha]

@xiachen-rh Please update us if you have made any progress or have new inputs with your testing. We can close this then.