cloud-init-dev team mailing list archive

[Dan Watkins <daniel.watkins@xxxxxxxxxxxxx>]

Dan Watkins has proposed merging ~daniel-thewatkins/cloud-init/+git/cloud-init:ubuntu/disco into cloud-init:ubuntu/disco.

Requested reviews:
  cloud-init Commiters (cloud-init-dev)
Related bugs:
  Bug #1846535 in cloud-init: "cloud-init 19.2.36 fails with python exception "Not all expected physical devices present ..."  during bionic image deployment from MAAS"
  https://bugs.launchpad.net/cloud-init/+bug/1846535

For more details, see:
https://code.launchpad.net/~daniel-thewatkins/cloud-init/+git/cloud-init/+merge/373657
-- 
Your team cloud-init Commiters is requested to review the proposed merge of ~daniel-thewatkins/cloud-init/+git/cloud-init:ubuntu/disco into cloud-init:ubuntu/disco.

diff --git a/debian/changelog b/debian/changelog
index e975bc2..befbd26 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
+cloud-init (19.2-36-g059d049c-0ubuntu2~19.04.1) disco; urgency=medium
+
+  * cherry-pick a7d8d032: get_interfaces: don't exclude bridge and bond
+    members (LP: #1846535)
+
+ -- Daniel Watkins <oddbloke@xxxxxxxxxx>  Fri, 04 Oct 2019 11:46:15 -0400
+
 cloud-init (19.2-36-g059d049c-0ubuntu1~19.04.1) disco; urgency=medium
 
   * New upstream snapshot. (LP: #1844334)
diff --git a/debian/patches/cpick-a7d8d032-get_interfaces-don-t-exclude-bridge-and-bond-members b/debian/patches/cpick-a7d8d032-get_interfaces-don-t-exclude-bridge-and-bond-members
new file mode 100644
index 0000000..4415fcb
--- /dev/null
+++ b/debian/patches/cpick-a7d8d032-get_interfaces-don-t-exclude-bridge-and-bond-members
@@ -0,0 +1,150 @@
+From a7d8d032a65e807007f1467a9220b7f161625668 Mon Sep 17 00:00:00 2001
+From: Daniel Watkins <oddbloke@xxxxxxxxxx>
+Date: Fri, 4 Oct 2019 15:34:41 +0000
+Subject: [PATCH] get_interfaces: don't exclude bridge and bond members
+
+The change that introduced this issue was handling interfaces that are
+bonded in the kernel, in a way that doesn't present as "a bond" to
+userspace in the normal way.  Both members of this "bond" will share a
+MAC address, so we filter one of them out to avoid incorrect MAC address
+collision warnings.
+
+Unfortunately, the matching condition was too broad, so that change also
+affected normal bonds and bridges.  This change specifically excludes
+bonds and bridges from that determination, to address that regression.
+
+LP: #1846535
+---
+ cloudinit/net/__init__.py        | 24 ++++++++++---
+ cloudinit/net/tests/test_init.py | 59 +++++++++++++++++++++++++++++---
+ 2 files changed, 73 insertions(+), 10 deletions(-)
+
+--- a/cloudinit/net/__init__.py
++++ b/cloudinit/net/__init__.py
+@@ -109,8 +109,22 @@ def is_bond(devname):
+     return os.path.exists(sys_dev_path(devname, "bonding"))
+ 
+ 
+-def has_master(devname):
+-    return os.path.exists(sys_dev_path(devname, path="master"))
++def get_master(devname):
++    """Return the master path for devname, or None if no master"""
++    path = sys_dev_path(devname, path="master")
++    if os.path.exists(path):
++        return path
++    return None
++
++
++def master_is_bridge_or_bond(devname):
++    """Return a bool indicating if devname's master is a bridge or bond"""
++    master_path = get_master(devname)
++    if master_path is None:
++        return False
++    bonding_path = os.path.join(master_path, "bonding")
++    bridge_path = os.path.join(master_path, "bridge")
++    return (os.path.exists(bonding_path) or os.path.exists(bridge_path))
+ 
+ 
+ def is_netfailover(devname, driver=None):
+@@ -158,7 +172,7 @@ def is_netfail_master(devname, driver=No
+ 
+         Return True if all of the above is True.
+     """
+-    if has_master(devname):
++    if get_master(devname) is not None:
+         return False
+ 
+     if driver is None:
+@@ -215,7 +229,7 @@ def is_netfail_standby(devname, driver=N
+ 
+         Return True if all of the above is True.
+     """
+-    if not has_master(devname):
++    if get_master(devname) is None:
+         return False
+ 
+     if driver is None:
+@@ -790,7 +804,7 @@ def get_interfaces():
+             continue
+         if is_bond(name):
+             continue
+-        if has_master(name):
++        if get_master(name) is not None and not master_is_bridge_or_bond(name):
+             continue
+         if is_netfailover(name):
+             continue
+--- a/cloudinit/net/tests/test_init.py
++++ b/cloudinit/net/tests/test_init.py
+@@ -157,11 +157,40 @@ class TestReadSysNet(CiTestCase):
+         ensure_file(os.path.join(self.sysdir, 'eth0', 'bonding'))
+         self.assertTrue(net.is_bond('eth0'))
+ 
+-    def test_has_master(self):
+-        """has_master is True when /sys/net/devname/master exists."""
+-        self.assertFalse(net.has_master('enP1s1'))
+-        ensure_file(os.path.join(self.sysdir, 'enP1s1', 'master'))
+-        self.assertTrue(net.has_master('enP1s1'))
++    def test_get_master(self):
++        """get_master returns the path when /sys/net/devname/master exists."""
++        self.assertIsNone(net.get_master('enP1s1'))
++        master_path = os.path.join(self.sysdir, 'enP1s1', 'master')
++        ensure_file(master_path)
++        self.assertEqual(master_path, net.get_master('enP1s1'))
++
++    def test_master_is_bridge_or_bond(self):
++        bridge_mac = 'aa:bb:cc:aa:bb:cc'
++        bond_mac = 'cc:bb:aa:cc:bb:aa'
++
++        # No master => False
++        write_file(os.path.join(self.sysdir, 'eth1', 'address'), bridge_mac)
++        write_file(os.path.join(self.sysdir, 'eth2', 'address'), bond_mac)
++
++        self.assertFalse(net.master_is_bridge_or_bond('eth1'))
++        self.assertFalse(net.master_is_bridge_or_bond('eth2'))
++
++        # masters without bridge/bonding => False
++        write_file(os.path.join(self.sysdir, 'br0', 'address'), bridge_mac)
++        write_file(os.path.join(self.sysdir, 'bond0', 'address'), bond_mac)
++
++        os.symlink('../br0', os.path.join(self.sysdir, 'eth1', 'master'))
++        os.symlink('../bond0', os.path.join(self.sysdir, 'eth2', 'master'))
++
++        self.assertFalse(net.master_is_bridge_or_bond('eth1'))
++        self.assertFalse(net.master_is_bridge_or_bond('eth2'))
++
++        # masters with bridge/bonding => True
++        write_file(os.path.join(self.sysdir, 'br0', 'bridge'), '')
++        write_file(os.path.join(self.sysdir, 'bond0', 'bonding'), '')
++
++        self.assertTrue(net.master_is_bridge_or_bond('eth1'))
++        self.assertTrue(net.master_is_bridge_or_bond('eth2'))
+ 
+     def test_is_vlan(self):
+         """is_vlan is True when /sys/net/devname/uevent has DEVTYPE=vlan."""
+@@ -461,6 +490,26 @@ class TestGetInterfaceMAC(CiTestCase):
+         expected = [('ens3', mac, None, None)]
+         self.assertEqual(expected, net.get_interfaces())
+ 
++    def test_get_interfaces_does_not_skip_phys_members_of_bridges_and_bonds(
++        self
++    ):
++        bridge_mac = 'aa:bb:cc:aa:bb:cc'
++        bond_mac = 'cc:bb:aa:cc:bb:aa'
++        write_file(os.path.join(self.sysdir, 'br0', 'address'), bridge_mac)
++        write_file(os.path.join(self.sysdir, 'br0', 'bridge'), '')
++
++        write_file(os.path.join(self.sysdir, 'bond0', 'address'), bond_mac)
++        write_file(os.path.join(self.sysdir, 'bond0', 'bonding'), '')
++
++        write_file(os.path.join(self.sysdir, 'eth1', 'address'), bridge_mac)
++        os.symlink('../br0', os.path.join(self.sysdir, 'eth1', 'master'))
++
++        write_file(os.path.join(self.sysdir, 'eth2', 'address'), bond_mac)
++        os.symlink('../bond0', os.path.join(self.sysdir, 'eth2', 'master'))
++
++        interface_names = [interface[0] for interface in net.get_interfaces()]
++        self.assertEqual(['eth1', 'eth2'], sorted(interface_names))
++
+ 
+ class TestInterfaceHasOwnMAC(CiTestCase):
+ 
diff --git a/debian/patches/series b/debian/patches/series
new file mode 100644
index 0000000..7922451
--- /dev/null
+++ b/debian/patches/series
@@ -0,0 +1 @@
+cpick-a7d8d032-get_interfaces-don-t-exclude-bridge-and-bond-members