net: allow dhcp6 configuration from generate_fallback_configuration()

[ani-sinha]

This will make sure on Azure we can use both dhcp4 and dhcp6 when IMDS is not used. This is useful in situations where only ipv6 network is available and there is no dhcp4 running.

Fixes GH-4439

[TheRealFalcon]

LGTM!

[TheRealFalcon]

@ani-sinha , after looking a little closer, I don't think that this change is safe to ship in 23.4. In particular, on non-netplan based systems, cloud-init will render the following NetworkManager config:

root@me:~/a/net# cat /tmp/etc/NetworkManager/system-connections/cloud-init-eth0.nmconnection 
# Generated by cloud-init. Changes will be lost.

[connection]
id=cloud-init eth0
uuid=1dd9a779-d327-56e1-8454-c65e2556c12c
autoconnect-priority=120
type=ethernet
interface-name=eth0

[user]
org.freedesktop.NetworkManager.origin=cloud-init

[ethernet]

[ipv4]
method=auto
may-fail=false

[ipv6]
method=auto
may-fail=false

Note that may-fail=false for both ipv4 AND ipv6. According to the docs:

If TRUE, allow overall network configuration to proceed even if the configuration specified by this property times out

This means that DHCP must succeed for both ipv4 and ipv6 in order for network configuration to proceed, which isn't what we want.

Based on this info, I'm going to revert this commit for now, and we can figure out a safer way to proceed for the following release. Sorry for the additional churn on this.

[ani-sinha]

This means that DHCP must succeed for both ipv4 and ipv6 in order for network configuration to proceed, which isn't what we want.

If you look above, our QE tested this on a system where there is only ipv4 and seemed it was not an issue. I will draw attention of our QE on your comment.

[ani-sinha]

@ani-sinha , after looking a little closer, I don't think that this change is safe to ship in 23.4. In particular, on non-netplan based systems, cloud-init will render the following NetworkManager config:

root@me:~/a/net# cat /tmp/etc/NetworkManager/system-connections/cloud-init-eth0.nmconnection 
# Generated by cloud-init. Changes will be lost.

[connection]
id=cloud-init eth0
uuid=1dd9a779-d327-56e1-8454-c65e2556c12c
autoconnect-priority=120
type=ethernet
interface-name=eth0

[user]
org.freedesktop.NetworkManager.origin=cloud-init

[ethernet]

[ipv4]
method=auto
may-fail=false

[ipv6]
method=auto
may-fail=false

Note that may-fail=false for both ipv4 AND ipv6. According to the docs:

If TRUE, allow overall network configuration to proceed even if the configuration specified by this property times out

This means that DHCP must succeed for both ipv4 and ipv6 in order for network configuration to proceed, which isn't what we want.

Based on this info, I'm going to revert this commit for now, and we can figure out a safer way to proceed for the following release. Sorry for the additional churn on this.

I think you are wrong @TheRealFalcon . Please see from the doc

Note that at least one IP configuration must succeed or overall network configuration will still fail.
For example, in IPv6-only networks, setting this property to TRUE on the NMSettingIP4Config
allows the overall network configuration to succeed if IPv4 configuration fails but IPv6 configuration
completes successfully.

So your comment

This means that DHCP must succeed for both ipv4 and ipv6 in order for network configuration to proceed, which isn't what we want.

seems incorrect.

[holmanb]

@ani-sinha see my comments in IRC. The docs you quote support @TheRealFalcon's reason for reverting the commit. If you want this to be un-reverted, please explain why you think that your implementation is correct.

[ani-sinha]

@ani-sinha see my comments in IRC. The docs you quote support @TheRealFalcon's reason for reverting the commit. If you want this to be un-reverted, please explain why you think that your implementation is correct.

OK i think I was misreading the doc. Will check with QE as well. Can we please re-open #4439 so that we can fix this in a different way?

[xiachen-rh]

@ani-sinha Hi Ani, I tested sysconfig renderer and NM renderer, the problem only happens when using NM renderer with ipv4 only network, and the connection file would become inactive in a while when dhcp6 fails as the docs description. and I also found that ipv6.may-fail=false is not totally caused by this patch, the default settings of may-fail in cloudinit/net/network_manager.py is false, is there any reason to set it false? Is it possible to remove that line and make it rendering same configs as other renderers to the same networking subsystem?

[ani-sinha]

@ani-sinha Hi Ani, I tested sysconfig renderer and NM renderer, the problem only happens when using NM renderer with ipv4 only network, and the connection file would become inactive in a while when dhcp6 fails as the docs description. and I also found that ipv6.may-fail=false is not totally caused by this patch, the default settings of may-fail in cloudinit/net/network_manager.py is false, is there any reason to set it false? Could we change it to True? self._set_default(family, "may-fail", "false") --> self._set_default(family, "may-fail", "true")

No that would be wrong. may-fail is False only when we are explicitly set the subnet-type and the subnet-type is known. It seems correct. We need to find a different solution to this issue.

[xiachen-rh]

@ani-sinha Hi Ani, I tested sysconfig renderer and NM renderer, the problem only happens when using NM renderer with ipv4 only network, and the connection file would become inactive in a while when dhcp6 fails as the docs description. and I also found that ipv6.may-fail=false is not totally caused by this patch, the default settings of may-fail in cloudinit/net/network_manager.py is false, is there any reason to set it false? Could we change it to True? self._set_default(family, "may-fail", "false") --> self._set_default(family, "may-fail", "true")

No that would be wrong. may-fail is False only when we are explicitly set the subnet-type and the subnet-type is known. It seems correct. We need to find a different solution to this issue.

When I disable cloud-init and let NM to configure the network, NM does not configure may-fail to false, and cloud-init sysconfig renderer does not set may-fail, so my question is , why cloud-init NM renderer have to set may-fail to false?

[ani-sinha]

@ani-sinha Hi Ani, I tested sysconfig renderer and NM renderer, the problem only happens when using NM renderer with ipv4 only network, and the connection file would become inactive in a while when dhcp6 fails as the docs description. and I also found that ipv6.may-fail=false is not totally caused by this patch, the default settings of may-fail in cloudinit/net/network_manager.py is false, is there any reason to set it false? Could we change it to True? self._set_default(family, "may-fail", "false") --> self._set_default(family, "may-fail", "true")

No that would be wrong. may-fail is False only when we are explicitly set the subnet-type and the subnet-type is known. It seems correct. We need to find a different solution to this issue.

When I disable cloud-init and let NM to configure the network, NM does not configure may-fail to false, and cloud-init sysconfig renderer does not set may-fail, so my question is , why cloud-init NM renderer have to set may-fail to false?

Lets discuss this offline and if required with our NM folks internally. That said I may have an alternative idea and we can try that internally also. There is no need to discuss this here elaborately.

[xiachen-rh]

@ani-sinha Hi Ani, I tested sysconfig renderer and NM renderer, the problem only happens when using NM renderer with ipv4 only network, and the connection file would become inactive in a while when dhcp6 fails as the docs description. and I also found that ipv6.may-fail=false is not totally caused by this patch, the default settings of may-fail in cloudinit/net/network_manager.py is false, is there any reason to set it false? Could we change it to True? self._set_default(family, "may-fail", "false") --> self._set_default(family, "may-fail", "true")

No that would be wrong. may-fail is False only when we are explicitly set the subnet-type and the subnet-type is known. It seems correct. We need to find a different solution to this issue.

When I disable cloud-init and let NM to configure the network, NM does not configure may-fail to false, and cloud-init sysconfig renderer does not set may-fail, so my question is , why cloud-init NM renderer have to set may-fail to false?

Lets discuss this offline and if required with our NM folks internally. That said I may have an alternative idea and we can try that internally also. There is no need to discuss this here elaborately.

OK, no problem

[ani-sinha]

@holmanb @TheRealFalcon I looked at cloud-init closely. Currently there seems to be no way to set the value for "may-fail" . The closest I came was "dhcp4-override" and "dhcp6-override" https://cloudinit.readthedocs.io/en/latest/reference/network-config-format-v2.html#network-config-v2 .
They do not accept "may-fail" today. Do you think we should add a new "may-fail" setting to them? All we want is a way to pass down the value of this setting to NM renderer so that it can be consumed by us and we can set the property correctly in the NM keyfile.

[TheRealFalcon]

@ani-sinha I'm wondering if it's necessary at all. Would the following work the ipv4/ipv6 sections?

[ipv4]
method=auto

[ipv6]
method=auto

[ani-sinha]

@ani-sinha I'm wondering if it's necessary at all. Would the following work the ipv4/ipv6 sections?

[ipv4]
method=auto

[ipv6]
method=auto

How would you translate this to a config in generate_fallback_config()?

[TheRealFalcon]

Sorry, I mean that given a config like this:

network:
    version: 2
    ethernets:
        eth0:
            dhcp4: true
            dhcp6: true

it would render NetworkManager like this:

[connection]
id=cloud-init eth0
uuid=1dd9a779-d327-56e1-8454-c65e2556c12c
autoconnect-priority=120
type=ethernet
interface-name=eth0

[user]
org.freedesktop.NetworkManager.origin=cloud-init

[ethernet]

[ipv4]
method=auto

[ipv6]
method=auto

[ani-sinha]

Sorry, I mean that given a config like this:

network:
    version: 2
    ethernets:
        eth0:
            dhcp4: true
            dhcp6: true

How is this different from the patch I proposed? Could you show me the diff on top of the patch that was reverted?

it would render NetworkManager like this:

Have you tested this with the NM renderer in cloud-init?

[connection]
id=cloud-init eth0
uuid=1dd9a779-d327-56e1-8454-c65e2556c12c
autoconnect-priority=120
type=ethernet
interface-name=eth0

[user]
org.freedesktop.NetworkManager.origin=cloud-init

[ethernet]

[ipv4]
method=auto

[ipv6]
method=auto

[TheRealFalcon]

@ani-sinha , sorry, your questions make me think that we may be talking past one another or that I haven't explained something well enough. Let me back up and recap just to make sure that we're both on the same page.

Before this PR (and also now that it has been reverted), the fallback config looked something like this:

network:
    version: 2
    ethernets:
        ens4:
            dhcp4: true
            match:
                macaddress: 42:01:0a:a8:00:2d
            set-name: ens4

This PR changed the config to look something like:

network:
    version: 2
    ethernets:
        ens4:
            dhcp4: true
            dhcp6: true
            match:
                macaddress: 42:01:0a:a8:00:2d
            set-name: ens4

However, that caused issues that forced us to revert, because when that fallback config gets rendered to NetworkManger, it becomes:

# Generated by cloud-init. Changes will be lost.

[connection]
id=cloud-init ens4
uuid=5e54b594-0544-581f-8201-eecd17ab3a95
autoconnect-priority=120
type=ethernet

[user]
org.freedesktop.NetworkManager.origin=cloud-init

[ethernet]
mac-address=42:01:0A:A8:00:2D

[ipv4]
method=auto
may-fail=false

[ipv6]
method=auto
may-fail=false

In particular, we found those may-fail lines to be problematic.

What I am suggesting is that we could change the cloud-init code to stop rendering the may-fail lines. So given the same fallback config that you have requested:

network:
    version: 2
    ethernets:
        ens4:
            dhcp4: true
            dhcp6: true
            match:
                macaddress: 42:01:0a:a8:00:2d
            set-name: ens4

I would like to change cloud-init's code to instead output something like this:

# Generated by cloud-init. Changes will be lost.

[connection]
id=cloud-init ens4
uuid=5e54b594-0544-581f-8201-eecd17ab3a95
autoconnect-priority=120
type=ethernet

[user]
org.freedesktop.NetworkManager.origin=cloud-init

[ethernet]
mac-address=42:01:0A:A8:00:2D

[ipv4]
method=auto

[ipv6]
method=auto

If we were to make that change in cloud-init, would such a configuration that doesn't include the may-fail lines work for your purposes? If so, then we could make that change, and then bring this PR back once we know that we are no longer generating the may-fail lines.

[ani-sinha]

@ani-sinha , sorry, your questions make me think that we may be talking past one another or that I haven't explained something well enough. Let me back up and recap just to make sure that we're both on the same page.

Before this PR (and also now that it has been reverted), the fallback config looked something like this:

network:
    version: 2
    ethernets:
        ens4:
            dhcp4: true
            match:
                macaddress: 42:01:0a:a8:00:2d
            set-name: ens4

This PR changed the config to look something like:

network:
    version: 2
    ethernets:
        ens4:
            dhcp4: true
            dhcp6: true
            match:
                macaddress: 42:01:0a:a8:00:2d
            set-name: ens4

However, that caused issues that forced us to revert, because when that fallback config gets rendered to NetworkManger, it becomes:

# Generated by cloud-init. Changes will be lost.

[connection]
id=cloud-init ens4
uuid=5e54b594-0544-581f-8201-eecd17ab3a95
autoconnect-priority=120
type=ethernet

[user]
org.freedesktop.NetworkManager.origin=cloud-init

[ethernet]
mac-address=42:01:0A:A8:00:2D

[ipv4]
method=auto
may-fail=false

[ipv6]
method=auto
may-fail=false

In particular, we found those may-fail lines to be problematic.

What I am suggesting is that we could change the cloud-init code to stop rendering the may-fail lines. So given the same fallback config that you have requested:

network:
    version: 2
    ethernets:
        ens4:
            dhcp4: true
            dhcp6: true
            match:
                macaddress: 42:01:0a:a8:00:2d
            set-name: ens4

I would like to change cloud-init's code to instead output something like this:

# Generated by cloud-init. Changes will be lost.

[connection]
id=cloud-init ens4
uuid=5e54b594-0544-581f-8201-eecd17ab3a95
autoconnect-priority=120
type=ethernet

[user]
org.freedesktop.NetworkManager.origin=cloud-init

[ethernet]
mac-address=42:01:0A:A8:00:2D

[ipv4]
method=auto

[ipv6]
method=auto

If we were to make that change in cloud-init, would such a configuration that doesn't include the may-fail lines work for your purposes? If so, then we could make that change, and then bring this PR back once we know we no longer generating the may-fail lines.

Right and for this change to happen, the cloud-init/Network Manager renderer needs to be told. Today the may-fail lines get added automatically and is set to False. It is because when we add dhcp4: true , NM renderer expects that dhcp4 to work and if it does not, it is regarded as a failure. We need to tell NM renderer that failing is OK in our scenario. That is why I was proposing passing dhcp4-override options.
The other approach is change the NM renderer assumption - that is, even when dhcp4: true is passed, NM renderer can assume that the dhcp can fail and will set may-fail to True. I am strongly against that approach. The semantic change will potentially break existing deployments. We do not want to do that.

[TheRealFalcon]

The other approach is change the NM renderer assumption - that is, even when dhcp4: true is passed, NM renderer can assume that the dhcp can fail and will set may-fail to True. I am strongly against that approach. The semantic change will potentially break existing deployments. We do not want to do that.

This is what I was asking about.

How would this break existing deployments? Is it something where the default could be changed upstream but patched in the currently supported downstreams?

[ani-sinha]

The other approach is change the NM renderer assumption - that is, even when dhcp4: true is passed, NM renderer can assume that the dhcp can fail and will set may-fail to True. I am strongly against that approach. The semantic change will potentially break existing deployments. We do not want to do that.

This is what I was asking about.

How would this break existing deployments? Is it something where the default could be changed upstream but patched in the currently supported downstreams?

We like to minimize downstream only changes and hence prefer not to maintain downstream only patches except where its only absolutely necessary (for example certain build specific patches and spec file changes). Besides changing the default downstream does not help because we need the fix for this issue downstream also. If the fix depends on the default being a certain way, that needs to be present downstream also. Lastly a solution to a problem based on default being a certain value is IMHO a terrible idea.

[TheRealFalcon]

Lastly a solution to a problem based on default being a certain value is IMHO a terrible idea

I'm not sure what you mean by default.

The reason I am suggesting these changes as a possible avenue to explore is because we have also defined our Network v2 format to be a subset of netplan, so arbitrarily adding keys or changing the dhcp[4,6]-overrides semantics isn't really viable either.

Additionally, we'd like the same v2 config to result in the same (or close to same) configuration across the various distributions, but currently on any netplan enabled system using NetworkManager, given the proposed fallback config, I instead get:

[connection]
id=netplan-eth0
type=ethernet
interface-name=eth0

[ethernet]
wake-on-lan=0

[ipv4]
method=auto

[ipv6]
method=auto
ip6-privacy=0

Unfortunately this was overlooked when the NetworkManager renderer was added, but we really shouldn't be rendering different configs to the same networking subsystem.

[ani-sinha]

Lastly a solution to a problem based on default being a certain value is IMHO a terrible idea

I'm not sure what you mean by default.

By default I mean what happens when we have dhcp6:true in the config option. By default, NM renderer would set may-fail: False unless we come up with a mechanism to override it.

The reason I am suggesting these changes as a possible avenue to explore is because we have also defined our Network v2 format to be a subset of netplan, so arbitrarily adding keys or changing the dhcp[4,6]-overrides semantics isn't really viable either.

Additionally, we'd like the same v2 config to result in the same (or close to same) configuration across the various distributions, but currently on any netplan enabled system using NetworkManager, given the proposed fallback config, I instead get:

[connection]
id=netplan-eth0
type=ethernet
interface-name=eth0

[ethernet]
wake-on-lan=0

[ipv4]
method=auto

[ipv6]
method=auto
ip6-privacy=0

I do not understand how NM renderer could be generating this configuration.

Unfortunately this was overlooked when the NetworkManager renderer was added,

Why was this not caught with integration tests and unit tests?

but we really shouldn't be rendering different configs to the same networking subsystem.

This could be an independent issue and I prefer to look into this separately outside of this issue.

[ani-sinha]

Lastly a solution to a problem based on default being a certain value is IMHO a terrible idea

I'm not sure what you mean by default.

By default I mean what happens when we have dhcp6:true in the config option. By default, NM renderer would set may-fail: False unless we come up with a mechanism to override it.

The reason I am suggesting these changes as a possible avenue to explore is because we have also defined our Network v2 format to be a subset of netplan, so arbitrarily adding keys or changing the dhcp[4,6]-overrides semantics isn't really viable either.

Ok so then please give us a mechanism to pass "may-fail" option to network renderers. Thats all I ask. I do not care how you do it. dhcp6-overrides was one suggestion that I thought could be possible. If required, I can raise another ticket specifically asking for this feature.

[TheRealFalcon]

I do not understand how NM renderer could be generating this configuration.

The NM renderer didn't, netplan did. On netplan enabled systems, v2 is passed through to netplan rather than cloud-init rendering anything.

Why was this not caught with integration tests and unit tests?

As I said before, it was overlooked. You can't test something you haven't thought of, and existing tests for NM didn't exist because it was a new renderer.

This could be an independent issue and I prefer to look into this separately outside of this issue.

I agree. I'm not trying to solve that here, but I think any solution proposed here needs to keep that in mind.

Ok so then please give us a mechanism to pass "may-fail" option to network renderers. Thats all I ask. I do not care how you do it. dhcp6-overrides was one suggestion that I thought could be possible. If required, I can raise another ticket specifically asking for this feature.

A separate ticket linking back here is a good idea.

[ani-sinha]

I do not understand how NM renderer could be generating this configuration.

The NM renderer didn't, netplan did. On netplan enabled systems, v2 is passed through to netplan rather than cloud-init rendering anything.

Why was this not caught with integration tests and unit tests?

As I said before, it was overlooked. You can't test something you haven't thought of, and existing tests for NM didn't exist because it was a new renderer.

This could be an independent issue and I prefer to look into this separately outside of this issue.

I agree. I'm not trying to solve that here, but I think any solution proposed here needs to keep that in mind.

Ok so then please give us a mechanism to pass "may-fail" option to network renderers. Thats all I ask. I do not care how you do it. dhcp6-overrides was one suggestion that I thought could be possible. If required, I can raise another ticket specifically asking for this feature.

A separate ticket linking back here is a good idea.

Cool. Done #4621 .

[xiachen-rh]

I agree with you that NM renderer is a new renderer and it is not fully tested, so the exists doesn't mean it's reasonable.

We found that the NM renderer worked for the basic scenarios but it did not treat all cases well.
The NM renderer works well if cloud-init configures network based on meta-data and network connections are well,
however, the problem would happen
1.when user's configuration contains "dhcp4":True and "dhcp6":True (the example that James metioned)
OR
2.when "dhcp4":True and "dhcp6":True in generate_fallback_config()
but one of ipv4/ipv6 network connection fails, the *.nmconnection file, which contains may-fail=false for both ipv4 and ipv6, cannot be activated. That's not the expected behavior.

so let's find a better way to enhance it.

Why was this not caught with integration tests and unit tests?

As I said before, it was overlooked. You can't test something you haven't thought of, and existing tests for NM didn't exist because it was a new renderer.

[holmanb]

The other approach is change the NM renderer assumption - that is, even when dhcp4: true is passed, NM renderer can assume that the dhcp can fail and will set may-fail to True. I am strongly against that approach. The semantic change will potentially break existing deployments. We do not want to do that.

@ani-sinha Why do you think this needs to be a separate configuration option? Is it not sufficient to set may-fail=true if and only if both dhcp4 and dhcp6 are set? This would not break existing deployments, and if I understand correctly, this is the desired state for dhcp4+dhcp6, plus it would allow dhcp6-only to also be may-fail=false. Why expose a new key when we can correctly assume the desired behavior?

[ani-sinha]

The other approach is change the NM renderer assumption - that is, even when dhcp4: true is passed, NM renderer can assume that the dhcp can fail and will set may-fail to True. I am strongly against that approach. The semantic change will potentially break existing deployments. We do not want to do that.

@ani-sinha Why do you think this needs to be a separate configuration option? Is it not sufficient to set may-fail=true if and only if both dhcp4 and dhcp6 are set? This would not break existing deployments, and if I understand correctly, this is the desired state for dhcp4+dhcp6, plus it would allow dhcp6-only to also be may-fail=false. Why expose a new key when we can correctly assume the desired behavior?

@holmanb yes this is a very good idea and we can cook up a patch quickly. Let me revert once I have something.

[ani-sinha]

The other approach is change the NM renderer assumption - that is, even when dhcp4: true is passed, NM renderer can assume that the dhcp can fail and will set may-fail to True. I am strongly against that approach. The semantic change will potentially break existing deployments. We do not want to do that.

@ani-sinha Why do you think this needs to be a separate configuration option? Is it not sufficient to set may-fail=true if and only if both dhcp4 and dhcp6 are set? This would not break existing deployments, and if I understand correctly, this is the desired state for dhcp4+dhcp6, plus it would allow dhcp6-only to also be may-fail=false. Why expose a new key when we can correctly assume the desired behavior?

@holmanb yes this is not a bad idea and we can cook up a patch quickly. Let me revert once I have something.

@holmanb Please let me know what you think of #4622 .