Merge lp:~hopem/charm-helpers/allow-list-nics-return-all into lp:charm-helpers
- allow-list-nics-return-all
- Merge into devel
Proposed by
Edward Hope-Morley
Status: | Superseded |
---|---|
Proposed branch: | lp:~hopem/charm-helpers/allow-list-nics-return-all |
Merge into: | lp:charm-helpers |
Diff against target: |
343 lines (+132/-45) 6 files modified
charmhelpers/contrib/openstack/context.py (+17/-10) charmhelpers/contrib/openstack/neutron.py (+29/-16) charmhelpers/core/host.py (+45/-11) tests/contrib/openstack/test_neutron_utils.py (+3/-3) tests/contrib/openstack/test_os_contexts.py (+7/-2) tests/core/test_host.py (+31/-3) |
To merge this branch: | bzr merge lp:~hopem/charm-helpers/allow-list-nics-return-all |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Liam Young (community) | Needs Fixing | ||
Review via email:
|
This proposal has been superseded by a proposal from 2015-08-18.
Commit message
Description of the change
To post a comment you must log in.
- 432. By Edward Hope-Morley
-
[hopem,r=]
Add support to core.host.
list_nics( ) to allow for listing
all nics i.e. unfiltered. Ensure that neutron resolve_ports
only resolves physical interfaces. - 433. By Edward Hope-Morley
-
Add support for identiying bonded nics and resolving
their master. - 434. By Edward Hope-Morley
-
ensure master is bond
- 435. By Edward Hope-Morley
-
added multimac unit test
- 436. By Edward Hope-Morley
-
more unit test
- 437. By Edward Hope-Morley
-
*fix* py3 unit test error
- 438. By Edward Hope-Morley
-
fix unit test
Unmerged revisions
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'charmhelpers/contrib/openstack/context.py' |
2 | --- charmhelpers/contrib/openstack/context.py 2015-07-29 14:21:45 +0000 |
3 | +++ charmhelpers/contrib/openstack/context.py 2015-08-18 11:36:40 +0000 |
4 | @@ -50,6 +50,7 @@ |
5 | from charmhelpers.core.strutils import bool_from_string |
6 | |
7 | from charmhelpers.core.host import ( |
8 | + is_phy_iface, |
9 | list_nics, |
10 | get_nic_hwaddr, |
11 | mkdir, |
12 | @@ -923,7 +924,6 @@ |
13 | |
14 | |
15 | class NeutronPortContext(OSContextGenerator): |
16 | - NIC_PREFIXES = ['eth', 'bond'] |
17 | |
18 | def resolve_ports(self, ports): |
19 | """Resolve NICs not yet bound to bridge(s) |
20 | @@ -935,12 +935,14 @@ |
21 | |
22 | hwaddr_to_nic = {} |
23 | hwaddr_to_ip = {} |
24 | - for nic in list_nics(self.NIC_PREFIXES): |
25 | - hwaddr = get_nic_hwaddr(nic) |
26 | - hwaddr_to_nic[hwaddr] = nic |
27 | - addresses = get_ipv4_addr(nic, fatal=False) |
28 | - addresses += get_ipv6_addr(iface=nic, fatal=False) |
29 | - hwaddr_to_ip[hwaddr] = addresses |
30 | + for nic in list_nics(): |
31 | + # Ignore virtual interfaces |
32 | + if is_phy_iface(nic): |
33 | + hwaddr = get_nic_hwaddr(nic) |
34 | + hwaddr_to_nic[hwaddr] = nic |
35 | + addresses = get_ipv4_addr(nic, fatal=False) |
36 | + addresses += get_ipv6_addr(iface=nic, fatal=False) |
37 | + hwaddr_to_ip[hwaddr] = addresses |
38 | |
39 | resolved = [] |
40 | mac_regex = re.compile(r'([0-9A-F]{2}[:-]){5}([0-9A-F]{2})', re.I) |
41 | @@ -961,7 +963,8 @@ |
42 | # trust it to be the real external network). |
43 | resolved.append(entry) |
44 | |
45 | - return resolved |
46 | + # Ensure no duplicates |
47 | + return list(set(resolved)) |
48 | |
49 | |
50 | class OSConfigFlagContext(OSContextGenerator): |
51 | @@ -1280,15 +1283,19 @@ |
52 | def __call__(self): |
53 | ports = config('data-port') |
54 | if ports: |
55 | + # Map of {port/mac:bridge} |
56 | portmap = parse_data_port_mappings(ports) |
57 | - ports = portmap.values() |
58 | + ports = portmap.keys() |
59 | + # Resolve provided ports or mac addresses and filter out those |
60 | + # already attached to a bridge. |
61 | resolved = self.resolve_ports(ports) |
62 | + # FIXME: is this necessary? |
63 | normalized = {get_nic_hwaddr(port): port for port in resolved |
64 | if port not in ports} |
65 | normalized.update({port: port for port in resolved |
66 | if port in ports}) |
67 | if resolved: |
68 | - return {bridge: normalized[port] for bridge, port in |
69 | + return {bridge: normalized[port] for port, bridge in |
70 | six.iteritems(portmap) if port in normalized.keys()} |
71 | |
72 | return None |
73 | |
74 | === modified file 'charmhelpers/contrib/openstack/neutron.py' |
75 | --- charmhelpers/contrib/openstack/neutron.py 2015-06-10 11:17:45 +0000 |
76 | +++ charmhelpers/contrib/openstack/neutron.py 2015-08-18 11:36:40 +0000 |
77 | @@ -255,17 +255,30 @@ |
78 | return 'neutron' |
79 | |
80 | |
81 | -def parse_mappings(mappings): |
82 | +def parse_mappings(mappings, key_rvalue=False): |
83 | + """By default mappings are lvalue keyed. |
84 | + |
85 | + If key_rvalue is True, the mapping will be reversed to allow multiple |
86 | + configs for the same lvalue. |
87 | + """ |
88 | parsed = {} |
89 | if mappings: |
90 | mappings = mappings.split() |
91 | for m in mappings: |
92 | p = m.partition(':') |
93 | - key = p[0].strip() |
94 | - if p[1]: |
95 | - parsed[key] = p[2].strip() |
96 | + |
97 | + if key_rvalue: |
98 | + key_index = 2 |
99 | + val_index = 0 |
100 | + # if there is no rvalue skip to next |
101 | + if not p[1]: |
102 | + continue |
103 | else: |
104 | - parsed[key] = '' |
105 | + key_index = 0 |
106 | + val_index = 2 |
107 | + |
108 | + key = p[key_index].strip() |
109 | + parsed[key] = p[val_index].strip() |
110 | |
111 | return parsed |
112 | |
113 | @@ -283,25 +296,25 @@ |
114 | def parse_data_port_mappings(mappings, default_bridge='br-data'): |
115 | """Parse data port mappings. |
116 | |
117 | - Mappings must be a space-delimited list of bridge:port mappings. |
118 | + Mappings must be a space-delimited list of port:bridge mappings. |
119 | |
120 | - Returns dict of the form {bridge:port}. |
121 | + Returns dict of the form {port:bridge} where port may be an mac address or |
122 | + interface name. |
123 | """ |
124 | - _mappings = parse_mappings(mappings) |
125 | + |
126 | + # NOTE(dosaboy): we use rvalue for key to allow multiple values to be |
127 | + # proposed for <port> since it may be a mac address which will differ |
128 | + # across units this allowing first-known-good to be chosen. |
129 | + _mappings = parse_mappings(mappings, key_rvalue=True) |
130 | if not _mappings or list(_mappings.values()) == ['']: |
131 | if not mappings: |
132 | return {} |
133 | |
134 | # For backwards-compatibility we need to support port-only provided in |
135 | # config. |
136 | - _mappings = {default_bridge: mappings.split()[0]} |
137 | - |
138 | - bridges = _mappings.keys() |
139 | - ports = _mappings.values() |
140 | - if len(set(bridges)) != len(bridges): |
141 | - raise Exception("It is not allowed to have more than one port " |
142 | - "configured on the same bridge") |
143 | - |
144 | + _mappings = {mappings.split()[0]: default_bridge} |
145 | + |
146 | + ports = _mappings.keys() |
147 | if len(set(ports)) != len(ports): |
148 | raise Exception("It is not allowed to have the same port configured " |
149 | "on more than one bridge") |
150 | |
151 | === modified file 'charmhelpers/core/host.py' |
152 | --- charmhelpers/core/host.py 2015-08-17 11:15:38 +0000 |
153 | +++ charmhelpers/core/host.py 2015-08-18 11:36:40 +0000 |
154 | @@ -417,25 +417,59 @@ |
155 | return(''.join(random_chars)) |
156 | |
157 | |
158 | -def list_nics(nic_type): |
159 | +def is_phy_iface(interface): |
160 | + """Returns True if interface is not virtual, otherwise False.""" |
161 | + if interface: |
162 | + sys_net = '/sys/class/net' |
163 | + if os.path.isdir(sys_net): |
164 | + for iface in glob.glob(os.path.join(sys_net, '*')): |
165 | + if '/virtual/' in os.path.realpath(iface): |
166 | + continue |
167 | + |
168 | + if interface == os.path.basename(iface): |
169 | + return True |
170 | + |
171 | + return False |
172 | + |
173 | + |
174 | +def list_nics(nic_type=None): |
175 | '''Return a list of nics of given type(s)''' |
176 | if isinstance(nic_type, six.string_types): |
177 | int_types = [nic_type] |
178 | else: |
179 | int_types = nic_type |
180 | + |
181 | interfaces = [] |
182 | - for int_type in int_types: |
183 | - cmd = ['ip', 'addr', 'show', 'label', int_type + '*'] |
184 | + if nic_type: |
185 | + for int_type in int_types: |
186 | + cmd = ['ip', 'addr', 'show', 'label', int_type + '*'] |
187 | + ip_output = subprocess.check_output(cmd).decode('UTF-8') |
188 | + ip_output = ip_output.split('\n') |
189 | + ip_output = (line for line in ip_output if line) |
190 | + for line in ip_output: |
191 | + if line.split()[1].startswith(int_type): |
192 | + matched = re.search('.*: (' + int_type + |
193 | + r'[0-9]+\.[0-9]+)@.*', line) |
194 | + if matched: |
195 | + iface = matched.groups()[0] |
196 | + else: |
197 | + iface = line.split()[1].replace(":", "") |
198 | + |
199 | + if iface not in interfaces: |
200 | + interfaces.append(iface) |
201 | + else: |
202 | + cmd = ['ip', 'a'] |
203 | ip_output = subprocess.check_output(cmd).decode('UTF-8').split('\n') |
204 | - ip_output = (line for line in ip_output if line) |
205 | + ip_output = (line.strip() for line in ip_output if line) |
206 | + |
207 | + key = re.compile('^[0-9]+:\s+(.+):') |
208 | for line in ip_output: |
209 | - if line.split()[1].startswith(int_type): |
210 | - matched = re.search('.*: (' + int_type + r'[0-9]+\.[0-9]+)@.*', line) |
211 | - if matched: |
212 | - interface = matched.groups()[0] |
213 | - else: |
214 | - interface = line.split()[1].replace(":", "") |
215 | - interfaces.append(interface) |
216 | + matched = re.search(key, line) |
217 | + if matched: |
218 | + iface = matched.group(1) |
219 | + iface = iface.partition("@")[0] |
220 | + if iface not in interfaces: |
221 | + interfaces.append(iface) |
222 | |
223 | return interfaces |
224 | |
225 | |
226 | === modified file 'tests/contrib/openstack/test_neutron_utils.py' |
227 | --- tests/contrib/openstack/test_neutron_utils.py 2015-05-02 22:27:26 +0000 |
228 | +++ tests/contrib/openstack/test_neutron_utils.py 2015-08-18 11:36:40 +0000 |
229 | @@ -181,13 +181,13 @@ |
230 | ret = neutron.parse_data_port_mappings(None) |
231 | self.assertEqual(ret, {}) |
232 | ret = neutron.parse_data_port_mappings('br0:eth0') |
233 | - self.assertEqual(ret, {'br0': 'eth0'}) |
234 | + self.assertEqual(ret, {'eth0': 'br0'}) |
235 | # Back-compat test |
236 | ret = neutron.parse_data_port_mappings('eth0', default_bridge='br0') |
237 | - self.assertEqual(ret, {'br0': 'eth0'}) |
238 | + self.assertEqual(ret, {'eth0': 'br0'}) |
239 | # Multiple mappings |
240 | ret = neutron.parse_data_port_mappings('br0:eth0 br1:eth1') |
241 | - self.assertEqual(ret, {'br0': 'eth0', 'br1': 'eth1'}) |
242 | + self.assertEqual(ret, {'eth0': 'br0', 'eth1': 'br1'}) |
243 | |
244 | def test_parse_vlan_range_mappings(self): |
245 | ret = neutron.parse_vlan_range_mappings(None) |
246 | |
247 | === modified file 'tests/contrib/openstack/test_os_contexts.py' |
248 | --- tests/contrib/openstack/test_os_contexts.py 2015-07-29 14:21:45 +0000 |
249 | +++ tests/contrib/openstack/test_os_contexts.py 2015-08-18 11:36:40 +0000 |
250 | @@ -2425,6 +2425,8 @@ |
251 | self.assertEquals(context.ExternalPortContext()(), |
252 | {'ext_port': 'eth1010'}) |
253 | |
254 | + @patch('charmhelpers.contrib.openstack.context.is_phy_iface', |
255 | + lambda arg: True) |
256 | @patch('charmhelpers.contrib.openstack.context.get_nic_hwaddr') |
257 | @patch('charmhelpers.contrib.openstack.context.list_nics') |
258 | @patch('charmhelpers.contrib.openstack.context.get_ipv6_addr') |
259 | @@ -2452,6 +2454,8 @@ |
260 | |
261 | self.assertEquals(context.ExternalPortContext()(), {}) |
262 | |
263 | + @patch('charmhelpers.contrib.openstack.context.is_phy_iface', |
264 | + lambda arg: True) |
265 | @patch('charmhelpers.contrib.openstack.context.get_nic_hwaddr') |
266 | @patch('charmhelpers.contrib.openstack.context.list_nics') |
267 | @patch('charmhelpers.contrib.openstack.context.get_ipv6_addr') |
268 | @@ -2484,8 +2488,9 @@ |
269 | 'resolve_ports') |
270 | def test_data_port_eth(self, mock_resolve): |
271 | self.config.side_effect = fake_config({'data-port': |
272 | - 'phybr1:eth1010'}) |
273 | - mock_resolve.side_effect = lambda ports: ports |
274 | + 'phybr1:eth1010 ' |
275 | + 'phybr1:eth1011'}) |
276 | + mock_resolve.side_effect = lambda ports: ['eth1010'] |
277 | self.assertEquals(context.DataPortContext()(), |
278 | {'phybr1': 'eth1010'}) |
279 | |
280 | |
281 | === modified file 'tests/core/test_host.py' |
282 | --- tests/core/test_host.py 2015-08-17 11:15:38 +0000 |
283 | +++ tests/core/test_host.py 2015-08-18 11:36:40 +0000 |
284 | @@ -35,6 +35,11 @@ |
285 | link/ether e4:11:5b:ab:a7:3c brd ff:ff:ff:ff:ff:ff |
286 | """ |
287 | |
288 | +IP_LINE_ETH100 = b""" |
289 | +2: eth100: <BROADCAST,MULTICAST,SLAVE,UP,LOWER_UP> mtu 1500 qdisc mq master bond0 state UP qlen 1000 |
290 | + link/ether e4:11:5b:ab:a7:3d brd ff:ff:ff:ff:ff:ff |
291 | +""" |
292 | + |
293 | IP_LINE_ETH0_VLAN = b""" |
294 | 6: eth0.10@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default |
295 | link/ether 08:00:27:16:b9:5f brd ff:ff:ff:ff:ff:ff |
296 | @@ -47,7 +52,7 @@ |
297 | |
298 | IP_LINE_HWADDR = b"""2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP qlen 1000\ link/ether e4:11:5b:ab:a7:3c brd ff:ff:ff:ff:ff:ff""" |
299 | |
300 | -IP_LINES = IP_LINE_ETH0 + IP_LINE_ETH1 + IP_LINE_ETH0_VLAN |
301 | +IP_LINES = IP_LINE_ETH0 + IP_LINE_ETH1 + IP_LINE_ETH0_VLAN + IP_LINE_ETH100 |
302 | |
303 | IP_LINE_BONDS = b""" |
304 | 6: bond0.10@bond0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default |
305 | @@ -962,13 +967,36 @@ |
306 | pw2 = host.pwgen(10) |
307 | self.assertNotEqual(pw, pw2, 'Duplicated password') |
308 | |
309 | + @patch.object(host, 'glob') |
310 | + @patch('os.path.realpath') |
311 | + @patch('os.path.isdir') |
312 | + def test_is_phy_iface(self, mock_isdir, mock_realpath, mock_glob): |
313 | + mock_isdir.return_value = True |
314 | + mock_glob.glob.return_value = ['/sys/class/net/eth0', |
315 | + '/sys/class/net/veth0'] |
316 | + |
317 | + def fake_realpath(soft): |
318 | + if soft.endswith('/eth0'): |
319 | + hard = \ |
320 | + '/sys/devices/pci0000:00/0000:00:1c.4/0000:02:00.1/net/eth0' |
321 | + else: |
322 | + hard = '/sys/devices/virtual/net/veth0' |
323 | + |
324 | + return hard |
325 | + |
326 | + mock_realpath.side_effect = fake_realpath |
327 | + self.assertTrue(host.is_phy_iface('eth0')) |
328 | + self.assertFalse(host.is_phy_iface('veth0')) |
329 | + |
330 | @patch('subprocess.check_output') |
331 | def test_list_nics(self, check_output): |
332 | check_output.return_value = IP_LINES |
333 | + nics = host.list_nics() |
334 | + self.assertEqual(nics, ['eth0', 'eth1', 'eth0.10', 'eth100']) |
335 | nics = host.list_nics('eth') |
336 | - self.assertEqual(nics, ['eth0', 'eth1', 'eth0.10']) |
337 | + self.assertEqual(nics, ['eth0', 'eth1', 'eth0.10', 'eth100']) |
338 | nics = host.list_nics(['eth']) |
339 | - self.assertEqual(nics, ['eth0', 'eth1', 'eth0.10']) |
340 | + self.assertEqual(nics, ['eth0', 'eth1', 'eth0.10', 'eth100']) |
341 | |
342 | @patch('subprocess.check_output') |
343 | def test_list_nics_with_bonds(self, check_output): |
Please could you update the unit tests so that the macs of IP_LINE_ETH0 and IP_LINE_ETH0_VLAN match and add a test for list_nics(['eth'], include_ vlans=False) ?
As a nitpick I think it's cleaner parse the output of ip when using '-o' so that all entries are on one line but that's how it was before so I won't block on it.