net/network_manager: do not set "may-fail" to False for both ipv4 and ipv6 #4622

ani-sinha · 2023-11-23T07:33:59Z

Please see discussions in #4474 .

The second patch unreverts the revert TheRealFalcon@21e6dfb .

The first change tries to the fix the issue due to which the above reversal was made.

   If "may-fail" is set to False in the Network Manager keyfile for both ipv4
    and ipv6, it essentially means both ipv4 and ipv6 network initialization must
    succeed for the overall network configuration to succeed. This means, for
    environments where only ipv4 or ipv6 is available but not both and we need to
    configure both ipv4 and ipv6 options, the overall network configuration will
    fail. This is not what we want. When both ipv4 and ipv6 are configured,
    it is enough for the overall configuration to succeed if any one succeeds.
    Therefore, set "may-fail" to True for both ipv4 and ipv6 if and only if both
    ipv4 and ipv6 are configured in the Network Manager keyfile and "may-fail" is
    set to False for both. If both ipv4 and ipv6 are configured in the keyfile
    and if for any of them "may-fail" is already set to True, then do nothing.
    All other cases remain same as before.

tests/unittests/test_net.py

holmanb · 2023-11-29T20:37:07Z

The second patch unreverts

Please note that since we don't want to introduce new commits that put cloud-init in a broken state, so this will be squash merged if/when ready.

ani-sinha · 2023-11-30T03:26:24Z

The second patch unreverts

Please note that since we don't want to introduce new commits that put cloud-init in a broken state, so this will be squash merged if/when ready.

That is precisely why I arranged the commits in such a way that first the fix is applied and then the revert of the revert of the original commit so that once we re-apply the original commit, at no stage cloud-init is broken.

holmanb · 2023-11-30T15:02:10Z

first the fix is applied and then the revert of the revert of the original commit so that once we re-apply the original commit, at no stage cloud-init is broken.

Okay I see now what you are trying to do. However, I still don't think we want git history to look the way that you've proposed this.

Reasons:

"Unrevert a reverted commit" -> This describes "what happened", not "what does this code do". This adds a layer of indirection in the commit message that we don't want.
"Unrevert a reverted commit" -> This claim isn't actually true. 560dede and 518047a are different.

I think I'm fine with keeping these commits separate if the commit message gets fixed, since "do the right thing for NetworkManager in all cases of dhcp4+dhcp6" is a fundamentally different thing than "change the default configuration to include dhcp6".

ani-sinha · 2023-11-30T16:12:54Z

first the fix is applied and then the revert of the revert of the original commit so that once we re-apply the original commit, at no stage cloud-init is broken.

Okay I see now what you are trying to do. However, I still don't think we want git history to look the way that you've proposed this.

Reasons:

"Unrevert a reverted commit" -> This describes "what happened", not "what does this code do". This adds a layer of indirection in the commit message that we don't want.

"Unrevert a reverted commit" -> This claim isn't actually true. 560dede and 518047a are different.

I think I'm fine with keeping these commits separate if the commit message gets fixed, since "do the right thing for NetworkManager in all cases of dhcp4+dhcp6" is a fundamentally different thing than "change the default configuration to include dhcp6".

Ok I have tried to update the commit message to something that is more appropriate.

TheRealFalcon

Thanks for the update. This works, but I requested some minor changes inline.

Here's a patch containing what I suggested:

diff --git a/cloudinit/net/network_manager.py b/cloudinit/net/network_manager.py
index 589ab7368..237b440c8 100644
--- a/cloudinit/net/network_manager.py
+++ b/cloudinit/net/network_manager.py
@@ -98,24 +98,16 @@ class NMConnection:
         if self._config_option_is_set(section, option):
             self.config[section][option] = value
 
-    def _set_mayfail_true_if_both_false_dhcp(self):
+    def _set_mayfail_true_if_both_dhcp(self):
         """
-        If for both ipv4 and ipv6, 'may-fail' is set to be False,
-        set it to True for both of them.
+        If for both ipv4 and ipv6, method is 'dhcp' or 'auto', set
+        'may-fail' to 'true'.
         """
         for family in ["ipv4", "ipv6"]:
-            if not self._config_option_is_set(family, "may-fail"):
-                # if either ipv4 or ipv6 sections are not set/configured,
-                # do not do anything.
-                return
-            if self._get_config_option(family, "may-fail") == "true":
-                # if for either ipv4 or ipv6, 'may-fail' is 'true', do
-                # not do anything.
-                return
-            if (
-                self._get_config_option(family, "method") != "auto"
-                and self._get_config_option(family, "method") != "dhcp"
-            ):
+            if self._get_config_option(family, "method") not in [
+                "dhcp",
+                "auto",
+            ]:
                 # if both v4 and v6 are not dhcp, do not do anything.
                 return
 
@@ -130,7 +122,6 @@ class NMConnection:
         Ensures there's appropriate [ipv4]/[ipv6] for given family
         appropriate for given configuration type
         """
-
         method_map = {
             "static": "manual",
             "static6": "manual",
@@ -170,14 +161,6 @@ class NMConnection:
         self.config[family]["method"] = method
         self._set_default(family, "may-fail", "false")
 
-        # we do not want to set may-fail to false for both ipv4 and ipv6 dhcp
-        # at the at the same time. This will make the network configuration
-        # work only when both ipv4 and ipv6 dhcp succeeds. This may not be
-        # what we want. If we have configured both ipv4 and ipv6 dhcp, any one
-        # succeeding should be enough. Therefore, if "may-fail" is set to
-        # False for both ipv4 and ipv6 dhcp, set them both to True.
-        self._set_mayfail_true_if_both_false_dhcp()
-
     def _add_numbered(self, section, key_prefix, value):
         """
         Adds a numbered property, such as address<n> or route<n>, ensuring
@@ -333,6 +316,14 @@ class NMConnection:
             if family == "ipv4" and "mtu" in subnet:
                 ipv4_mtu = subnet["mtu"]
 
+        # we do not want to set may-fail to false for both ipv4 and ipv6 dhcp
+        # at the at the same time. This will make the network configuration
+        # work only when both ipv4 and ipv6 dhcp succeeds. This may not be
+        # what we want. If we have configured both ipv4 and ipv6 dhcp, any one
+        # succeeding should be enough. Therefore, if "may-fail" is set to
+        # False for both ipv4 and ipv6 dhcp, set them both to True.
+        self._set_mayfail_true_if_both_dhcp()
+
         if ipv4_mtu is None:
             ipv4_mtu = device_mtu
         if not ipv4_mtu == device_mtu:

Let me know if that works for you.

cloudinit/net/network_manager.py

ani-sinha · 2023-12-02T08:05:35Z

Thanks for the update. This works, but I requested some minor changes inline.

Here's a patch containing what I suggested:

diff --git a/cloudinit/net/network_manager.py b/cloudinit/net/network_manager.py
index 589ab7368..237b440c8 100644
--- a/cloudinit/net/network_manager.py
+++ b/cloudinit/net/network_manager.py
@@ -98,24 +98,16 @@ class NMConnection:
         if self._config_option_is_set(section, option):
             self.config[section][option] = value
 
-    def _set_mayfail_true_if_both_false_dhcp(self):
+    def _set_mayfail_true_if_both_dhcp(self):
         """
-        If for both ipv4 and ipv6, 'may-fail' is set to be False,
-        set it to True for both of them.
+        If for both ipv4 and ipv6, method is 'dhcp' or 'auto', set
+        'may-fail' to 'true'.
         """
         for family in ["ipv4", "ipv6"]:
-            if not self._config_option_is_set(family, "may-fail"):
-                # if either ipv4 or ipv6 sections are not set/configured,
-                # do not do anything.
-                return
-            if self._get_config_option(family, "may-fail") == "true":
-                # if for either ipv4 or ipv6, 'may-fail' is 'true', do
-                # not do anything.
-                return
-            if (
-                self._get_config_option(family, "method") != "auto"
-                and self._get_config_option(family, "method") != "dhcp"
-            ):
+            if self._get_config_option(family, "method") not in [
+                "dhcp",
+                "auto",
+            ]:
                 # if both v4 and v6 are not dhcp, do not do anything.
                 return
 
@@ -130,7 +122,6 @@ class NMConnection:
         Ensures there's appropriate [ipv4]/[ipv6] for given family
         appropriate for given configuration type
         """
-
         method_map = {
             "static": "manual",
             "static6": "manual",
@@ -170,14 +161,6 @@ class NMConnection:
         self.config[family]["method"] = method
         self._set_default(family, "may-fail", "false")
 
-        # we do not want to set may-fail to false for both ipv4 and ipv6 dhcp
-        # at the at the same time. This will make the network configuration
-        # work only when both ipv4 and ipv6 dhcp succeeds. This may not be
-        # what we want. If we have configured both ipv4 and ipv6 dhcp, any one
-        # succeeding should be enough. Therefore, if "may-fail" is set to
-        # False for both ipv4 and ipv6 dhcp, set them both to True.
-        self._set_mayfail_true_if_both_false_dhcp()
-
     def _add_numbered(self, section, key_prefix, value):
         """
         Adds a numbered property, such as address<n> or route<n>, ensuring
@@ -333,6 +316,14 @@ class NMConnection:
             if family == "ipv4" and "mtu" in subnet:
                 ipv4_mtu = subnet["mtu"]
 
+        # we do not want to set may-fail to false for both ipv4 and ipv6 dhcp
+        # at the at the same time. This will make the network configuration
+        # work only when both ipv4 and ipv6 dhcp succeeds. This may not be
+        # what we want. If we have configured both ipv4 and ipv6 dhcp, any one
+        # succeeding should be enough. Therefore, if "may-fail" is set to
+        # False for both ipv4 and ipv6 dhcp, set them both to True.
+        self._set_mayfail_true_if_both_dhcp()
+
         if ipv4_mtu is None:
             ipv4_mtu = device_mtu
         if not ipv4_mtu == device_mtu:

Let me know if that works for you.

I have sent an updated rework. Kept most of the changes except that I do check for "may-fail".

… ipv6 dhcp If "may-fail" is set to False in the Network Manager keyfile for both ipv4 and ipv6 for dhcp configuration, it essentially means both ipv4 and ipv6 network initialization using dhcp must succeed for the overall network configuration to succeed. This means, for environments where only ipv4 or ipv6 is available but not both and we need to configure both ipv4 and ipv6 dhcp, the overall network configuration will fail. This is not what we want. When both ipv4 and ipv6 dhcp are configured, it is enough for the overall configuration to succeed if any one succeeds. Therefore, set "may-fail" to True for both ipv4 and ipv6 if and only if both ipv4 and ipv6 are configured as dhcp in the Network Manager keyfile and "may-fail" is set to False for both. If both ipv4 and ipv6 are configured in the keyfile and if for any of them "may-fail" is already set to True,then do nothing. All other cases remain same as before. Please see discussions in PR canonical#4474. Co-authored-by: James Falcon <james.falcon@canonical.com> Signed-off-by: Ani Sinha <anisinha@redhat.com>

ani-sinha · 2023-12-02T10:24:51Z

cloudinit/net/network_manager.py

+        set it to True for both of them.
+        """
+        for family in ["ipv4", "ipv6"]:
+            if self._get_config_option(family, "may-fail") != "false":


I agree that this check is currently not required. But I still want to have this explicitly spelled out.

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. This change is mostly a reversal of commit 29ed5f5 and therefore, re-application of the commit 518047a with some small changes. The issue that caused the reversal of 518047a is fixed by the earlier commit: cab0eaf ("net/network_manager: do not set "may-fail" to False for both ipv4 and ipv6 dhcp") Fixes canonicalGH-4439 Signed-off-by: Ani Sinha <anisinha@redhat.com>

TheRealFalcon

Thanks @ani-sinha , this looks good to me.

@holmanb , since you proposed the idea, do you mind reviewing as well?

holmanb

Looks good, @ani-sinha.

Thanks for your work on this.

ani-sinha mentioned this pull request Nov 23, 2023

net: allow dhcp6 configuration from generate_fallback_configuration() #4474

Merged

ani-sinha force-pushed the fallback-fix branch 2 times, most recently from d813fad to aeea3d1 Compare November 27, 2023 06:15

TheRealFalcon reviewed Nov 28, 2023

View reviewed changes

tests/unittests/test_net.py Outdated Show resolved Hide resolved

ani-sinha force-pushed the fallback-fix branch 2 times, most recently from d1dff82 to 560dede Compare November 29, 2023 06:25

ani-sinha force-pushed the fallback-fix branch from 560dede to dff7239 Compare November 30, 2023 16:11

TheRealFalcon self-assigned this Dec 1, 2023

TheRealFalcon requested changes Dec 1, 2023

View reviewed changes

cloudinit/net/network_manager.py Outdated Show resolved Hide resolved

cloudinit/net/network_manager.py Show resolved Hide resolved

ani-sinha force-pushed the fallback-fix branch from dff7239 to 0cd31f7 Compare December 2, 2023 08:04

ani-sinha force-pushed the fallback-fix branch 2 times, most recently from 623b578 to dbc156c Compare December 2, 2023 08:21

ani-sinha force-pushed the fallback-fix branch from dbc156c to aa8e7b7 Compare December 2, 2023 10:21

ani-sinha commented Dec 2, 2023

View reviewed changes

ani-sinha force-pushed the fallback-fix branch from aa8e7b7 to 0153924 Compare December 2, 2023 11:22

holmanb mentioned this pull request Dec 4, 2023

[enhancement]: Add ability to pass specific ipv4/ipv6 options to network renderers #4621

Open

TheRealFalcon approved these changes Dec 5, 2023

View reviewed changes

holmanb approved these changes Dec 6, 2023

View reviewed changes

holmanb merged commit 0264e96 into canonical:main Dec 6, 2023
26 checks passed

TheRealFalcon mentioned this pull request Oct 2, 2024

IPv6 not enabled by default #3682

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net/network_manager: do not set "may-fail" to False for both ipv4 and ipv6 #4622

net/network_manager: do not set "may-fail" to False for both ipv4 and ipv6 #4622

ani-sinha commented Nov 23, 2023 •

edited

Loading

holmanb commented Nov 29, 2023 •

edited

Loading

ani-sinha commented Nov 30, 2023

holmanb commented Nov 30, 2023

ani-sinha commented Nov 30, 2023

TheRealFalcon left a comment

ani-sinha commented Dec 2, 2023

ani-sinha Dec 2, 2023

TheRealFalcon left a comment

holmanb left a comment

net/network_manager: do not set "may-fail" to False for both ipv4 and ipv6 #4622

net/network_manager: do not set "may-fail" to False for both ipv4 and ipv6 #4622

Conversation

ani-sinha commented Nov 23, 2023 • edited Loading

holmanb commented Nov 29, 2023 • edited Loading

ani-sinha commented Nov 30, 2023

holmanb commented Nov 30, 2023

ani-sinha commented Nov 30, 2023

TheRealFalcon left a comment

Choose a reason for hiding this comment

ani-sinha commented Dec 2, 2023

ani-sinha Dec 2, 2023

Choose a reason for hiding this comment

TheRealFalcon left a comment

Choose a reason for hiding this comment

holmanb left a comment

Choose a reason for hiding this comment

ani-sinha commented Nov 23, 2023 •

edited

Loading

holmanb commented Nov 29, 2023 •

edited

Loading