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
diff --git a/cloudinit/net/__init__.py b/cloudinit/net/__init__.py
index 5de5c6d..307da78 100644
--- a/cloudinit/net/__init__.py
+++ b/cloudinit/net/__init__.py
@@ -109,8 +109,22 @@ def is_bond(devname):
109 return os.path.exists(sys_dev_path(devname, "bonding"))109 return os.path.exists(sys_dev_path(devname, "bonding"))
110110
111111
112def has_master(devname):112def get_master(devname):
113 return os.path.exists(sys_dev_path(devname, path="master"))113 """Return the master path for devname, or None if no master"""
114 path = sys_dev_path(devname, path="master")
115 if os.path.exists(path):
116 return path
117 return None
118
119
120def master_is_bridge_or_bond(devname):
121 """Return a bool indicating if devname's master is a bridge or bond"""
122 master_path = get_master(devname)
123 if master_path is None:
124 return False
125 bonding_path = os.path.join(master_path, "bonding")
126 bridge_path = os.path.join(master_path, "bridge")
127 return (os.path.exists(bonding_path) or os.path.exists(bridge_path))
114128
115129
116def is_netfailover(devname, driver=None):130def is_netfailover(devname, driver=None):
@@ -158,7 +172,7 @@ def is_netfail_master(devname, driver=None):
158172
159 Return True if all of the above is True.173 Return True if all of the above is True.
160 """174 """
161 if has_master(devname):175 if get_master(devname) is not None:
162 return False176 return False
163177
164 if driver is None:178 if driver is None:
@@ -215,7 +229,7 @@ def is_netfail_standby(devname, driver=None):
215229
216 Return True if all of the above is True.230 Return True if all of the above is True.
217 """231 """
218 if not has_master(devname):232 if get_master(devname) is None:
219 return False233 return False
220234
221 if driver is None:235 if driver is None:
@@ -790,7 +804,7 @@ def get_interfaces():
790 continue804 continue
791 if is_bond(name):805 if is_bond(name):
792 continue806 continue
793 if has_master(name):807 if get_master(name) is not None and not master_is_bridge_or_bond(name):
794 continue808 continue
795 if is_netfailover(name):809 if is_netfailover(name):
796 continue810 continue
diff --git a/cloudinit/net/tests/test_init.py b/cloudinit/net/tests/test_init.py
index 999db98..6db93e2 100644
--- a/cloudinit/net/tests/test_init.py
+++ b/cloudinit/net/tests/test_init.py
@@ -157,11 +157,40 @@ class TestReadSysNet(CiTestCase):
157 ensure_file(os.path.join(self.sysdir, 'eth0', 'bonding'))157 ensure_file(os.path.join(self.sysdir, 'eth0', 'bonding'))
158 self.assertTrue(net.is_bond('eth0'))158 self.assertTrue(net.is_bond('eth0'))
159159
160 def test_has_master(self):160 def test_get_master(self):
161 """has_master is True when /sys/net/devname/master exists."""161 """get_master returns the path when /sys/net/devname/master exists."""
162 self.assertFalse(net.has_master('enP1s1'))162 self.assertIsNone(net.get_master('enP1s1'))
163 ensure_file(os.path.join(self.sysdir, 'enP1s1', 'master'))163 master_path = os.path.join(self.sysdir, 'enP1s1', 'master')
164 self.assertTrue(net.has_master('enP1s1'))164 ensure_file(master_path)
165 self.assertEqual(master_path, net.get_master('enP1s1'))
166
167 def test_master_is_bridge_or_bond(self):
168 bridge_mac = 'aa:bb:cc:aa:bb:cc'
169 bond_mac = 'cc:bb:aa:cc:bb:aa'
170
171 # No master => False
172 write_file(os.path.join(self.sysdir, 'eth1', 'address'), bridge_mac)
173 write_file(os.path.join(self.sysdir, 'eth2', 'address'), bond_mac)
174
175 self.assertFalse(net.master_is_bridge_or_bond('eth1'))
176 self.assertFalse(net.master_is_bridge_or_bond('eth2'))
177
178 # masters without bridge/bonding => False
179 write_file(os.path.join(self.sysdir, 'br0', 'address'), bridge_mac)
180 write_file(os.path.join(self.sysdir, 'bond0', 'address'), bond_mac)
181
182 os.symlink('../br0', os.path.join(self.sysdir, 'eth1', 'master'))
183 os.symlink('../bond0', os.path.join(self.sysdir, 'eth2', 'master'))
184
185 self.assertFalse(net.master_is_bridge_or_bond('eth1'))
186 self.assertFalse(net.master_is_bridge_or_bond('eth2'))
187
188 # masters with bridge/bonding => True
189 write_file(os.path.join(self.sysdir, 'br0', 'bridge'), '')
190 write_file(os.path.join(self.sysdir, 'bond0', 'bonding'), '')
191
192 self.assertTrue(net.master_is_bridge_or_bond('eth1'))
193 self.assertTrue(net.master_is_bridge_or_bond('eth2'))
165194
166 def test_is_vlan(self):195 def test_is_vlan(self):
167 """is_vlan is True when /sys/net/devname/uevent has DEVTYPE=vlan."""196 """is_vlan is True when /sys/net/devname/uevent has DEVTYPE=vlan."""
@@ -461,6 +490,26 @@ class TestGetInterfaceMAC(CiTestCase):
461 expected = [('ens3', mac, None, None)]490 expected = [('ens3', mac, None, None)]
462 self.assertEqual(expected, net.get_interfaces())491 self.assertEqual(expected, net.get_interfaces())
463492
493 def test_get_interfaces_does_not_skip_phys_members_of_bridges_and_bonds(
494 self
495 ):
496 bridge_mac = 'aa:bb:cc:aa:bb:cc'
497 bond_mac = 'cc:bb:aa:cc:bb:aa'
498 write_file(os.path.join(self.sysdir, 'br0', 'address'), bridge_mac)
499 write_file(os.path.join(self.sysdir, 'br0', 'bridge'), '')
500
501 write_file(os.path.join(self.sysdir, 'bond0', 'address'), bond_mac)
502 write_file(os.path.join(self.sysdir, 'bond0', 'bonding'), '')
503
504 write_file(os.path.join(self.sysdir, 'eth1', 'address'), bridge_mac)
505 os.symlink('../br0', os.path.join(self.sysdir, 'eth1', 'master'))
506
507 write_file(os.path.join(self.sysdir, 'eth2', 'address'), bond_mac)
508 os.symlink('../bond0', os.path.join(self.sysdir, 'eth2', 'master'))
509
510 interface_names = [interface[0] for interface in net.get_interfaces()]
511 self.assertEqual(['eth1', 'eth2'], sorted(interface_names))
512
464513
465class TestInterfaceHasOwnMAC(CiTestCase):514class TestInterfaceHasOwnMAC(CiTestCase):
466515

Subscribers

People subscribed via source and target branches