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.