Merge ~daniel-thewatkins/cloud-init/+git/cloud-init:ubuntu/bionic into cloud-init:ubuntu/bionic

Proposed by Dan Watkins on 2019-10-04
Status: Merged
Merged at revision: 8c156cb59e6ead664215fb816795e59a91a9fb63
Proposed branch: ~daniel-thewatkins/cloud-init/+git/cloud-init:ubuntu/bionic
Merge into: cloud-init:ubuntu/bionic
Diff against target: 179 lines (+158/-0)
3 files modified
debian/changelog (+7/-0)
debian/patches/cpick-a7d8d032-get_interfaces-don-t-exclude-bridge-and-bond-members (+150/-0)
debian/patches/series (+1/-0)
Reviewer Review Type Date Requested Status
Server Team CI bot continuous-integration Needs Fixing on 2019-10-04
Chad Smith 2019-10-04 Approve on 2019-10-04
Review via email: mp+373655@code.launchpad.net
To post a comment you must log in.
Chad Smith (chad.smith) wrote :

tox run is green, changeset is equivalent to mine:
https://paste.ubuntu.com/p/fd8zmgKdZN/
+1

review: Approve
Chad Smith (chad.smith) :
review: Disapprove
Chad Smith (chad.smith) wrote :

false alarm I saw a version diff between your changelog and mine. But I had made a mistake in the suffix 19.2-36-g059d049c-0ubuntu1~18.04.1ubuntu1 instead of 19.2-36-g059d049c-0ubuntu1~18.04.1

review: Approve

No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want jenkins to rebuild you need to trigger it yourself):
https://code.launchpad.net/~daniel-thewatkins/cloud-init/+git/cloud-init/+merge/373655/+edit-commit-message
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1195/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1195//rebuild

review: Needs Fixing (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/debian/changelog b/debian/changelog
index 31760d7..10ac80b 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
1cloud-init (19.2-36-g059d049c-0ubuntu2~18.04.1) bionic; urgency=medium
2
3 * cherry-pick a7d8d032: get_interfaces: don't exclude bridge and bond
4 members (LP: #1846535)
5
6 -- Daniel Watkins <oddbloke@ubuntu.com> Fri, 04 Oct 2019 11:35:54 -0400
7
1cloud-init (19.2-36-g059d049c-0ubuntu1~18.04.1) bionic; urgency=medium8cloud-init (19.2-36-g059d049c-0ubuntu1~18.04.1) bionic; urgency=medium
29
3 * New upstream snapshot. (LP: #1844334)10 * 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
4new file mode 10064411new 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 @@
1From a7d8d032a65e807007f1467a9220b7f161625668 Mon Sep 17 00:00:00 2001
2From: Daniel Watkins <oddbloke@ubuntu.com>
3Date: Fri, 4 Oct 2019 15:34:41 +0000
4Subject: [PATCH] get_interfaces: don't exclude bridge and bond members
5
6The change that introduced this issue was handling interfaces that are
7bonded in the kernel, in a way that doesn't present as "a bond" to
8userspace in the normal way. Both members of this "bond" will share a
9MAC address, so we filter one of them out to avoid incorrect MAC address
10collision warnings.
11
12Unfortunately, the matching condition was too broad, so that change also
13affected normal bonds and bridges. This change specifically excludes
14bonds and bridges from that determination, to address that regression.
15
16LP: #1846535
17---
18 cloudinit/net/__init__.py | 24 ++++++++++---
19 cloudinit/net/tests/test_init.py | 59 +++++++++++++++++++++++++++++---
20 2 files changed, 73 insertions(+), 10 deletions(-)
21
22--- a/cloudinit/net/__init__.py
23+++ b/cloudinit/net/__init__.py
24@@ -109,8 +109,22 @@ def is_bond(devname):
25 return os.path.exists(sys_dev_path(devname, "bonding"))
26
27
28-def has_master(devname):
29- return os.path.exists(sys_dev_path(devname, path="master"))
30+def get_master(devname):
31+ """Return the master path for devname, or None if no master"""
32+ path = sys_dev_path(devname, path="master")
33+ if os.path.exists(path):
34+ return path
35+ return None
36+
37+
38+def master_is_bridge_or_bond(devname):
39+ """Return a bool indicating if devname's master is a bridge or bond"""
40+ master_path = get_master(devname)
41+ if master_path is None:
42+ return False
43+ bonding_path = os.path.join(master_path, "bonding")
44+ bridge_path = os.path.join(master_path, "bridge")
45+ return (os.path.exists(bonding_path) or os.path.exists(bridge_path))
46
47
48 def is_netfailover(devname, driver=None):
49@@ -158,7 +172,7 @@ def is_netfail_master(devname, driver=No
50
51 Return True if all of the above is True.
52 """
53- if has_master(devname):
54+ if get_master(devname) is not None:
55 return False
56
57 if driver is None:
58@@ -215,7 +229,7 @@ def is_netfail_standby(devname, driver=N
59
60 Return True if all of the above is True.
61 """
62- if not has_master(devname):
63+ if get_master(devname) is None:
64 return False
65
66 if driver is None:
67@@ -790,7 +804,7 @@ def get_interfaces():
68 continue
69 if is_bond(name):
70 continue
71- if has_master(name):
72+ if get_master(name) is not None and not master_is_bridge_or_bond(name):
73 continue
74 if is_netfailover(name):
75 continue
76--- a/cloudinit/net/tests/test_init.py
77+++ b/cloudinit/net/tests/test_init.py
78@@ -157,11 +157,40 @@ class TestReadSysNet(CiTestCase):
79 ensure_file(os.path.join(self.sysdir, 'eth0', 'bonding'))
80 self.assertTrue(net.is_bond('eth0'))
81
82- def test_has_master(self):
83- """has_master is True when /sys/net/devname/master exists."""
84- self.assertFalse(net.has_master('enP1s1'))
85- ensure_file(os.path.join(self.sysdir, 'enP1s1', 'master'))
86- self.assertTrue(net.has_master('enP1s1'))
87+ def test_get_master(self):
88+ """get_master returns the path when /sys/net/devname/master exists."""
89+ self.assertIsNone(net.get_master('enP1s1'))
90+ master_path = os.path.join(self.sysdir, 'enP1s1', 'master')
91+ ensure_file(master_path)
92+ self.assertEqual(master_path, net.get_master('enP1s1'))
93+
94+ def test_master_is_bridge_or_bond(self):
95+ bridge_mac = 'aa:bb:cc:aa:bb:cc'
96+ bond_mac = 'cc:bb:aa:cc:bb:aa'
97+
98+ # No master => False
99+ write_file(os.path.join(self.sysdir, 'eth1', 'address'), bridge_mac)
100+ write_file(os.path.join(self.sysdir, 'eth2', 'address'), bond_mac)
101+
102+ self.assertFalse(net.master_is_bridge_or_bond('eth1'))
103+ self.assertFalse(net.master_is_bridge_or_bond('eth2'))
104+
105+ # masters without bridge/bonding => False
106+ write_file(os.path.join(self.sysdir, 'br0', 'address'), bridge_mac)
107+ write_file(os.path.join(self.sysdir, 'bond0', 'address'), bond_mac)
108+
109+ os.symlink('../br0', os.path.join(self.sysdir, 'eth1', 'master'))
110+ os.symlink('../bond0', os.path.join(self.sysdir, 'eth2', 'master'))
111+
112+ self.assertFalse(net.master_is_bridge_or_bond('eth1'))
113+ self.assertFalse(net.master_is_bridge_or_bond('eth2'))
114+
115+ # masters with bridge/bonding => True
116+ write_file(os.path.join(self.sysdir, 'br0', 'bridge'), '')
117+ write_file(os.path.join(self.sysdir, 'bond0', 'bonding'), '')
118+
119+ self.assertTrue(net.master_is_bridge_or_bond('eth1'))
120+ self.assertTrue(net.master_is_bridge_or_bond('eth2'))
121
122 def test_is_vlan(self):
123 """is_vlan is True when /sys/net/devname/uevent has DEVTYPE=vlan."""
124@@ -461,6 +490,26 @@ class TestGetInterfaceMAC(CiTestCase):
125 expected = [('ens3', mac, None, None)]
126 self.assertEqual(expected, net.get_interfaces())
127
128+ def test_get_interfaces_does_not_skip_phys_members_of_bridges_and_bonds(
129+ self
130+ ):
131+ bridge_mac = 'aa:bb:cc:aa:bb:cc'
132+ bond_mac = 'cc:bb:aa:cc:bb:aa'
133+ write_file(os.path.join(self.sysdir, 'br0', 'address'), bridge_mac)
134+ write_file(os.path.join(self.sysdir, 'br0', 'bridge'), '')
135+
136+ write_file(os.path.join(self.sysdir, 'bond0', 'address'), bond_mac)
137+ write_file(os.path.join(self.sysdir, 'bond0', 'bonding'), '')
138+
139+ write_file(os.path.join(self.sysdir, 'eth1', 'address'), bridge_mac)
140+ os.symlink('../br0', os.path.join(self.sysdir, 'eth1', 'master'))
141+
142+ write_file(os.path.join(self.sysdir, 'eth2', 'address'), bond_mac)
143+ os.symlink('../bond0', os.path.join(self.sysdir, 'eth2', 'master'))
144+
145+ interface_names = [interface[0] for interface in net.get_interfaces()]
146+ self.assertEqual(['eth1', 'eth2'], sorted(interface_names))
147+
148
149 class TestInterfaceHasOwnMAC(CiTestCase):
150
diff --git a/debian/patches/series b/debian/patches/series
index 72f0fe9..6a93210 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -1,2 +1,3 @@
1openstack-no-network-config.patch1openstack-no-network-config.patch
2ubuntu-advantage-revert-tip.patch2ubuntu-advantage-revert-tip.patch
3cpick-a7d8d032-get_interfaces-don-t-exclude-bridge-and-bond-members

Subscribers

People subscribed via source and target branches