Merge ~daniel-thewatkins/cloud-init/+git/cloud-init:lp1846535 into cloud-init:master

Proposed by Dan Watkins on 2019-10-04
Status: Merged
Approved by: Dan Watkins on 2019-10-04
Approved revision: 4e751e0f2ecb6c08a182d50619b1a7726f289302
Merge reported by: Server Team CI bot
Merged at revision: not available
Proposed branch: ~daniel-thewatkins/cloud-init/+git/cloud-init:lp1846535
Merge into: cloud-init:master
Diff against target: 132 lines (+73/-10)
2 files modified
cloudinit/net/__init__.py (+19/-5)
cloudinit/net/tests/test_init.py (+54/-5)
Reviewer Review Type Date Requested Status
Chad Smith 2019-10-04 Approve on 2019-10-04
Server Team CI bot continuous-integration Approve on 2019-10-04
Review via email: mp+373649@code.launchpad.net

Commit message

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

To post a comment you must log in.

PASSED: Continuous integration, rev:4e751e0f2ecb6c08a182d50619b1a7726f289302
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1192/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

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

review: Approve (continuous-integration)
Chad Smith (chad.smith) wrote :

This is good for me. tested this last night as well.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py
2index 5de5c6d..307da78 100644
3--- a/cloudinit/net/__init__.py
4+++ b/cloudinit/net/__init__.py
5@@ -109,8 +109,22 @@ def is_bond(devname):
6 return os.path.exists(sys_dev_path(devname, "bonding"))
7
8
9-def has_master(devname):
10- return os.path.exists(sys_dev_path(devname, path="master"))
11+def get_master(devname):
12+ """Return the master path for devname, or None if no master"""
13+ path = sys_dev_path(devname, path="master")
14+ if os.path.exists(path):
15+ return path
16+ return None
17+
18+
19+def master_is_bridge_or_bond(devname):
20+ """Return a bool indicating if devname's master is a bridge or bond"""
21+ master_path = get_master(devname)
22+ if master_path is None:
23+ return False
24+ bonding_path = os.path.join(master_path, "bonding")
25+ bridge_path = os.path.join(master_path, "bridge")
26+ return (os.path.exists(bonding_path) or os.path.exists(bridge_path))
27
28
29 def is_netfailover(devname, driver=None):
30@@ -158,7 +172,7 @@ def is_netfail_master(devname, driver=None):
31
32 Return True if all of the above is True.
33 """
34- if has_master(devname):
35+ if get_master(devname) is not None:
36 return False
37
38 if driver is None:
39@@ -215,7 +229,7 @@ def is_netfail_standby(devname, driver=None):
40
41 Return True if all of the above is True.
42 """
43- if not has_master(devname):
44+ if get_master(devname) is None:
45 return False
46
47 if driver is None:
48@@ -790,7 +804,7 @@ def get_interfaces():
49 continue
50 if is_bond(name):
51 continue
52- if has_master(name):
53+ if get_master(name) is not None and not master_is_bridge_or_bond(name):
54 continue
55 if is_netfailover(name):
56 continue
57diff --git a/cloudinit/net/tests/test_init.py b/cloudinit/net/tests/test_init.py
58index 999db98..6db93e2 100644
59--- a/cloudinit/net/tests/test_init.py
60+++ b/cloudinit/net/tests/test_init.py
61@@ -157,11 +157,40 @@ class TestReadSysNet(CiTestCase):
62 ensure_file(os.path.join(self.sysdir, 'eth0', 'bonding'))
63 self.assertTrue(net.is_bond('eth0'))
64
65- def test_has_master(self):
66- """has_master is True when /sys/net/devname/master exists."""
67- self.assertFalse(net.has_master('enP1s1'))
68- ensure_file(os.path.join(self.sysdir, 'enP1s1', 'master'))
69- self.assertTrue(net.has_master('enP1s1'))
70+ def test_get_master(self):
71+ """get_master returns the path when /sys/net/devname/master exists."""
72+ self.assertIsNone(net.get_master('enP1s1'))
73+ master_path = os.path.join(self.sysdir, 'enP1s1', 'master')
74+ ensure_file(master_path)
75+ self.assertEqual(master_path, net.get_master('enP1s1'))
76+
77+ def test_master_is_bridge_or_bond(self):
78+ bridge_mac = 'aa:bb:cc:aa:bb:cc'
79+ bond_mac = 'cc:bb:aa:cc:bb:aa'
80+
81+ # No master => False
82+ write_file(os.path.join(self.sysdir, 'eth1', 'address'), bridge_mac)
83+ write_file(os.path.join(self.sysdir, 'eth2', 'address'), bond_mac)
84+
85+ self.assertFalse(net.master_is_bridge_or_bond('eth1'))
86+ self.assertFalse(net.master_is_bridge_or_bond('eth2'))
87+
88+ # masters without bridge/bonding => False
89+ write_file(os.path.join(self.sysdir, 'br0', 'address'), bridge_mac)
90+ write_file(os.path.join(self.sysdir, 'bond0', 'address'), bond_mac)
91+
92+ os.symlink('../br0', os.path.join(self.sysdir, 'eth1', 'master'))
93+ os.symlink('../bond0', os.path.join(self.sysdir, 'eth2', 'master'))
94+
95+ self.assertFalse(net.master_is_bridge_or_bond('eth1'))
96+ self.assertFalse(net.master_is_bridge_or_bond('eth2'))
97+
98+ # masters with bridge/bonding => True
99+ write_file(os.path.join(self.sysdir, 'br0', 'bridge'), '')
100+ write_file(os.path.join(self.sysdir, 'bond0', 'bonding'), '')
101+
102+ self.assertTrue(net.master_is_bridge_or_bond('eth1'))
103+ self.assertTrue(net.master_is_bridge_or_bond('eth2'))
104
105 def test_is_vlan(self):
106 """is_vlan is True when /sys/net/devname/uevent has DEVTYPE=vlan."""
107@@ -461,6 +490,26 @@ class TestGetInterfaceMAC(CiTestCase):
108 expected = [('ens3', mac, None, None)]
109 self.assertEqual(expected, net.get_interfaces())
110
111+ def test_get_interfaces_does_not_skip_phys_members_of_bridges_and_bonds(
112+ self
113+ ):
114+ bridge_mac = 'aa:bb:cc:aa:bb:cc'
115+ bond_mac = 'cc:bb:aa:cc:bb:aa'
116+ write_file(os.path.join(self.sysdir, 'br0', 'address'), bridge_mac)
117+ write_file(os.path.join(self.sysdir, 'br0', 'bridge'), '')
118+
119+ write_file(os.path.join(self.sysdir, 'bond0', 'address'), bond_mac)
120+ write_file(os.path.join(self.sysdir, 'bond0', 'bonding'), '')
121+
122+ write_file(os.path.join(self.sysdir, 'eth1', 'address'), bridge_mac)
123+ os.symlink('../br0', os.path.join(self.sysdir, 'eth1', 'master'))
124+
125+ write_file(os.path.join(self.sysdir, 'eth2', 'address'), bond_mac)
126+ os.symlink('../bond0', os.path.join(self.sysdir, 'eth2', 'master'))
127+
128+ interface_names = [interface[0] for interface in net.get_interfaces()]
129+ self.assertEqual(['eth1', 'eth2'], sorted(interface_names))
130+
131
132 class TestInterfaceHasOwnMAC(CiTestCase):
133

Subscribers

People subscribed via source and target branches