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

Proposed by Dan Watkins on 2019-10-04
Status: Merged
Merged at revision: 118bc10833af8ded1f26fb8f7af99d23dce0fb6d
Proposed branch: ~daniel-thewatkins/cloud-init/+git/cloud-init:ubuntu/disco
Merge into: cloud-init:ubuntu/disco
Diff against target: 178 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+373657@code.launchpad.net
To post a comment you must log in.
Chad Smith (chad.smith) wrote :

tox succeeds, package builds, same diff as mine with exception of metadata https://paste.ubuntu.com/p/SjJp2wzqb8/

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/373657/+edit-commit-message
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1193/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

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

review: Needs Fixing (continuous-integration)

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/373657/+edit-commit-message
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1197/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

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

review: Needs Fixing (continuous-integration)

Preview Diff

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

Subscribers

People subscribed via source and target branches