Merge ~mpontillo/maas:fix-juju-interation--bug-1789495--part1 into maas:master

Proposed by Mike Pontillo on 2018-08-30
Status: Merged
Approved by: Mike Pontillo on 2018-08-30
Approved revision: bd80ce2519c29308176ad6675884b2356c4d27fc
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~mpontillo/maas:fix-juju-interation--bug-1789495--part1
Merge into: maas:master
Diff against target: 129 lines (+61/-2)
4 files modified
src/maasserver/forms/pods.py (+2/-1)
src/maasserver/forms/tests/test_pods.py (+5/-1)
src/provisioningserver/utils/network.py (+19/-0)
src/provisioningserver/utils/tests/test_network.py (+35/-0)
Reviewer Review Type Date Requested Status
Blake Rouse (community) 2018-08-30 Approve on 2018-08-30
MAAS Lander Needs Fixing on 2018-08-30
Review via email: mp+354021@code.launchpad.net

Commit message

Calculate a better interface name based on the specified labels when composing a machine.

This is required because Juju passes in numeric interface labels, such as '0'. MAAS uses these labels (which are strings) to create correspondingly-named interfaces on the composed machine.

The algorithm used to calculate the ifname from the label was adapted from the Juju bridge name algorithm (which is used when a bridge name ends up being more than 15 characters).

Partially fixes bug #1789495.

To post a comment you must log in.
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b fix-juju-interation--bug-1789495--part1 lp:~mpontillo/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/3811/console
COMMIT: c48d24a767b324f182f1cf98bf88dd7dd04f9884

review: Needs Fixing
bd80ce2... by Mike Pontillo on 2018-08-30

Fix accidentally removed return statement.

MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b fix-juju-interation--bug-1789495--part1 lp:~mpontillo/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/3813/console
COMMIT: bd80ce2519c29308176ad6675884b2356c4d27fc

review: Needs Fixing
Blake Rouse (blake-rouse) wrote :

Looks good.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/src/maasserver/forms/pods.py b/src/maasserver/forms/pods.py
2index 01220f2..c64a690 100644
3--- a/src/maasserver/forms/pods.py
4+++ b/src/maasserver/forms/pods.py
5@@ -75,6 +75,7 @@ from provisioningserver.enum import (
6 MACVLAN_MODE,
7 MACVLAN_MODE_CHOICES,
8 )
9+from provisioningserver.utils.network import get_ifname_for_label
10 from provisioningserver.utils.twisted import asynchronous
11 from twisted.python.threadable import isInIOThread
12
13@@ -471,7 +472,7 @@ class ComposeMachineForm(forms.Form):
14 has_bootable_vlan = True
15 rmi = self.get_requested_machine_interface_by_interface(interface)
16 # Set the requested interface name and IP addresses.
17- rmi.ifname = label
18+ rmi.ifname = get_ifname_for_label(label)
19 rmi.requested_ips = result.allocated_ips.get(label, [])
20 requested_machine_interfaces.append(rmi)
21 if not has_bootable_vlan:
22diff --git a/src/maasserver/forms/tests/test_pods.py b/src/maasserver/forms/tests/test_pods.py
23index e44ca01..c28755a 100644
24--- a/src/maasserver/forms/tests/test_pods.py
25+++ b/src/maasserver/forms/tests/test_pods.py
26@@ -956,7 +956,8 @@ class TestComposeMachineForm(MAASTransactionServerTestCase):
27 pod = make_pod_with_hints()
28 pod.ip_address = pod_host.boot_interface.ip_addresses.first()
29 pod.save()
30- interfaces = "eth0:subnet=%s" % (
31+ # Test with a numeric label, since that's what Juju will pass in.
32+ interfaces = "0:subnet=%s" % (
33 pod_host.boot_interface.vlan.subnet_set.first().cidr)
34 form = ComposeMachineForm(data={
35 'interfaces': interfaces,
36@@ -970,6 +971,9 @@ class TestComposeMachineForm(MAASTransactionServerTestCase):
37 MatchesAll(
38 IsInstance(RequestedMachineInterface),
39 MatchesStructure(
40+ # Make sure the numeric label gets converted
41+ # to a sane interface name.
42+ ifname=Equals("eth0"),
43 attach_name=Equals(pod_host.boot_interface.name),
44 attach_type=Equals(InterfaceAttachType.MACVLAN),
45 attach_options=Equals(MACVLAN_MODE.BRIDGE)))
46diff --git a/src/provisioningserver/utils/network.py b/src/provisioningserver/utils/network.py
47index 354412f..458d1ac 100644
48--- a/src/provisioningserver/utils/network.py
49+++ b/src/provisioningserver/utils/network.py
50@@ -41,6 +41,7 @@ from typing import (
51 Optional,
52 TypeVar,
53 )
54+from zlib import crc32
55
56 from netaddr import (
57 EUI,
58@@ -1512,3 +1513,21 @@ def convert_host_to_uri_str(host):
59 return str(ip)
60 else:
61 return '[%s]' % str(ip)
62+
63+
64+def get_ifname_for_label(label: str) -> str:
65+ """Given a generic interface label, return a suitable interface name.
66+
67+ If the label is numeric, appends 'eth' to the beginning.
68+
69+ If the name is more than 15 characters, shortens it using a hash algorithm.
70+ """
71+ if label.isnumeric():
72+ label = 'eth%d' % int(label)
73+ # Need to measure the length of the interface name in bytes, not
74+ # characters, since that is how it's represented in the kernel.
75+ label = label.encode('utf-8')
76+ if len(label) > 15:
77+ ifname_hash = (b"%05x" % (crc32(label) & 0xffffffff))[-5:]
78+ label = b"eth-%s-%s" % (ifname_hash, label[len(label) - 5:])
79+ return label.decode('utf-8')
80diff --git a/src/provisioningserver/utils/tests/test_network.py b/src/provisioningserver/utils/tests/test_network.py
81index e106541..de972e6 100644
82--- a/src/provisioningserver/utils/tests/test_network.py
83+++ b/src/provisioningserver/utils/tests/test_network.py
84@@ -62,6 +62,7 @@ from provisioningserver.utils.network import (
85 get_all_interfaces_definition,
86 get_default_monitored_interfaces,
87 get_eui_organization,
88+ get_ifname_for_label,
89 get_ifname_ifdata_for_destination,
90 get_interface_children,
91 get_mac_organization,
92@@ -2389,3 +2390,37 @@ class TestConvertHostToUriStr(MAASTestCase):
93 def test__ipv6(self):
94 ipv6 = factory.make_ipv6_address()
95 self.assertEquals('[%s]' % ipv6, convert_host_to_uri_str(ipv6))
96+
97+
98+class TestGetIfnameForLabel(MAASTestCase):
99+
100+ scenarios = [
101+ ('numeric', {
102+ 'input': '0',
103+ 'expected': 'eth0'
104+ }),
105+ ('numeric_zero_pad', {
106+ 'input': '0000000000000000000000000000000000000000000000000000200',
107+ 'expected': 'eth200'
108+ }),
109+ ('normal_interface_name', {
110+ 'input': 'eth0',
111+ 'expected': 'eth0'
112+ }),
113+ ('long_but_not_too_long_name', {
114+ 'input': 'eth012345678901',
115+ 'expected': 'eth012345678901'
116+ }),
117+ ('name_a_little_too_long', {
118+ 'input': 'eth0123456789012',
119+ 'expected': 'eth-ff5c2-89012'
120+ }),
121+ ('crazy_long_numeric_name', {
122+ 'input': '1000000000000000000000000000000000000000000000000000001',
123+ 'expected': 'eth-8d3de-00001'
124+ }),
125+ ]
126+
127+ def test__scenarios(self):
128+ self.assertThat(
129+ get_ifname_for_label(self.input), Equals(self.expected))

Subscribers

People subscribed via source and target branches