Merge lp:~allenap/maas/case-compare-mac--bug-1558635 into lp:~maas-committers/maas/trunk

Proposed by Gavin Panella
Status: Merged
Approved by: Gavin Panella
Approved revision: no longer in the source branch.
Merged at revision: 5061
Proposed branch: lp:~allenap/maas/case-compare-mac--bug-1558635
Merge into: lp:~maas-committers/maas/trunk
Diff against target: 138 lines (+58/-8)
3 files modified
src/maasserver/testing/factory.py (+19/-3)
src/maasserver/websockets/handlers/device.py (+11/-5)
src/maasserver/websockets/handlers/tests/test_device.py (+28/-0)
To merge this branch: bzr merge lp:~allenap/maas/case-compare-mac--bug-1558635
Reviewer Review Type Date Requested Status
Lee Trager (community) Approve
Review via email: mp+296173@code.launchpad.net

Commit message

When adding devices via the Web UI, cope with MAC addresses of different forms and cases.

To post a comment you must log in.
Revision history for this message
Lee Trager (ltrager) wrote :

LGTM!

review: Approve
Revision history for this message
MAAS Lander (maas-lander) wrote :
Download full text (1.3 MiB)

The attempt to merge lp:~allenap/maas/case-compare-mac--bug-1558635 into lp:maas failed. Below is the output from the failed tests.

Get:1 http://security.ubuntu.com/ubuntu xenial-security InRelease [94.5 kB]
Hit:2 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial InRelease
Get:3 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates InRelease [94.5 kB]
Hit:4 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-backports InRelease
Get:5 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates/main Sources [55.6 kB]
Get:6 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates/universe Sources [21.1 kB]
Get:7 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates/main amd64 Packages [169 kB]
Get:8 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates/main Translation-en [65.6 kB]
Get:9 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates/universe amd64 Packages [78.6 kB]
Get:10 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates/universe Translation-en [36.7 kB]
Fetched 616 kB in 0s (1,305 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
    --no-install-recommends install apache2 archdetect-deb authbind bash bind9 bind9utils build-essential bzr bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libpq-dev make nodejs-legacy npm postgresql pxelinux python3-all python3-apt python3-bson python3-convoy python3-crochet python3-cssselect python3-curtin python3-dev python3-distro-info python3-django python3-django-nose python3-django-piston3 python3-dnspython python3-docutils python3-formencode python3-hivex python3-httplib2 python3-jinja2 python3-jsonschema python3-lxml python3-netaddr python3-netifaces python3-novaclient python3-oauth python3-oauthlib python3-openssl python3-paramiko python3-petname python3-pexpect python3-psycopg2 python3-pyinotify python3-pyparsing python3-pyvmomi python3-requests python3-seamicroclient python3-setuptools python3-simplestreams python3-sphinx python3-tempita python3-twisted python3-txtftp python3-tz python3-yaml python3-zope.interface python-bson python-crochet python-django python-django-piston python-djorm-ext-pgarray python-formencode python-lxml python-netaddr python-netifaces python-pocket-lint python-psycopg2 python-simplejson python-tempita python-twisted python-yaml socat syslinux-common tgt ubuntu-cloudimage-keyring wget xvfb
Reading package lists...
Building dependency tree...
Reading state information...
apache2 is already the newest version (2.4.18-2ubuntu3).
archdetect-deb is already the newest version (1.117ubuntu2).
authbind is already the newest version (2.1.1+nmu1).
bash is already the newest version (4.3-14ubuntu1).
build-essential is already the newest version (12.1ubuntu2).
bzr is already the newest version (2.7.0-2ubuntu1).
curl is already the newest version (7.47.0-1ubuntu2).
debhelper is already the newest version ...

Revision history for this message
Gavin Panella (allenap) wrote :

I think this hit a 1 in 10000 error here where the PID randomly chosen
by the object factory collided with an existing PID. I've updated the
factory function to use successive PIDs from a cycling generator instead
of random integers.

ERROR: maasserver.triggers.tests.test_system_listener.TestCoreRegionRackRPCConnectionInsertListener.test_rebalance_doesnt_happen_when_less_than_half_conn
----------------------------------------------------------------------
testtools.testresult.real._StringException: Traceback (most recent call last):
  File "/tmp/tarmac/branch.mpRa89/src/maastesting/runtest.py", line 121, in _run_user
    result = function(*args, **kwargs)
  File "/home/ubuntu/.buildout/eggs/testtools-2.1.0-py3.5.egg/testtools/testcase.py", line 714, in _run_test_method
    return self._get_test_method()()
  File "/usr/lib/python3/dist-packages/crochet/_eventloop.py", line 461, in wrapper
    return eventual_result.wait(timeout)
  File "/usr/lib/python3/dist-packages/crochet/_eventloop.py", line 231, in wait
    result.raiseException()
  File "/usr/lib/python3/dist-packages/twisted/python/failure.py", line 368, in raiseException
    raise self.value.with_traceback(self.tb)
  File "/usr/lib/python3/dist-packages/twisted/internet/defer.py", line 1126, in _inlineCallbacks
    result = result.throwExceptionIntoGenerator(g)
  File "/usr/lib/python3/dist-packages/twisted/python/failure.py", line 389, in throwExceptionIntoGenerator
    return g.throw(self.type, self.value, self.tb)
  File "/tmp/tarmac/branch.mpRa89/src/maasserver/triggers/tests/test_system_listener.py", line 349, in test_rebalance_doesnt_happen_when_less_than_half_conn
    "region": region,
django.core.exceptions.ValidationError: {'__all__': ['Region controller process with this Region and Pid already exists.']}

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/testing/factory.py'
2--- src/maasserver/testing/factory.py 2016-05-12 19:07:37 +0000
3+++ src/maasserver/testing/factory.py 2016-06-02 14:44:32 +0000
4@@ -11,6 +11,10 @@
5 from datetime import timedelta
6 import hashlib
7 from io import BytesIO
8+from itertools import (
9+ chain,
10+ repeat,
11+)
12 import logging
13 import random
14 import time
15@@ -127,9 +131,12 @@
16 # src/maasserver/tests/data/test_rsa{0, 1, 2, 3, 4}.pub
17 MAX_PUBLIC_KEYS = 5
18
19-
20+# Used to save a node without worrying about a valid transition.
21 ALL_NODE_STATES = list(map_enum(NODE_STATUS).values())
22
23+# Maximum PID available on this machine.
24+with open("/proc/sys/kernel/pid_max") as fd:
25+ PID_MAX = int(fd.read())
26
27 # Use `undefined` instead of `None` for default factory arguments when `None`
28 # is a reasonable value for the argument.
29@@ -281,17 +288,26 @@
30 region.save()
31 return region
32
33+ # PIDs for use with make_RegionControllerProcess. Note that the simpler
34+ # cycle(range(...)) is not used because it gradually consumes all memory.
35+ _rcp_pids = chain.from_iterable(repeat(range(PID_MAX)))
36+
37 def make_RegionControllerProcess(
38 self, region=None, pid=None, updated=None):
39 if region is None:
40 region = self.make_RegionController()
41 if pid is None:
42- pid = random.randint(1, 10000)
43+ pid = next(self._rcp_pids)
44 process = RegionControllerProcess(
45 region=region, pid=pid, updated=updated)
46 process.save()
47 return process
48
49+ # Ports for use with make_RegionControllerProcessEndpoint. Note that the
50+ # simpler cycle(range(...)) is not used because it gradually consumes all
51+ # memory.
52+ _rcpe_ports = chain.from_iterable(repeat(range(2**16)))
53+
54 def make_RegionControllerProcessEndpoint(
55 self, process=None, address=None, port=None):
56 if process is None:
57@@ -299,7 +315,7 @@
58 if address is None:
59 address = self.make_ip_address()
60 if port is None:
61- port = random.randint(1, 10000)
62+ port = next(self._rcpe_ports)
63 endpoint = RegionControllerProcessEndpoint(
64 process=process, address=address, port=port)
65 endpoint.save()
66
67=== modified file 'src/maasserver/websockets/handlers/device.py'
68--- src/maasserver/websockets/handlers/device.py 2016-04-22 21:02:20 +0000
69+++ src/maasserver/websockets/handlers/device.py 2016-06-02 14:44:32 +0000
70@@ -29,6 +29,7 @@
71 node_prefetch,
72 NodeHandler,
73 )
74+from netaddr import EUI
75 from provisioningserver.logger import get_maas_logger
76
77
78@@ -50,11 +51,16 @@
79
80
81 def get_Interface_from_list(interfaces, mac):
82- """Return the `Interface` object based on the mac value."""
83- for obj in interfaces:
84- if obj.mac_address == mac:
85- return obj
86- return None
87+ """Return the `Interface` object with the given MAC address."""
88+ # Compare using EUI instances so that we're not concerned with a MAC's
89+ # canonical form, i.e. colons versus hyphens, uppercase versus lowercase.
90+ mac = EUI(mac)
91+ for interface in interfaces:
92+ ifmac = interface.mac_address
93+ if ifmac is not None and EUI(ifmac.raw) == mac:
94+ return interface
95+ else:
96+ return None
97
98
99 class DeviceHandler(NodeHandler):
100
101=== modified file 'src/maasserver/websockets/handlers/tests/test_device.py'
102--- src/maasserver/websockets/handlers/tests/test_device.py 2016-04-29 21:38:44 +0000
103+++ src/maasserver/websockets/handlers/tests/test_device.py 2016-06-02 14:44:32 +0000
104@@ -434,6 +434,34 @@
105 StaticIPAddress.objects.filter(ip=external_ip_address).count(),
106 Equals(1), "External StaticIPAddress was not created.")
107
108+ def test_create_copes_with_mac_addresses_of_different_case(self):
109+ user = factory.make_User()
110+ handler = DeviceHandler(user, {})
111+ mac = factory.make_mac_address()
112+ created_device = handler.create({
113+ "hostname": factory.make_name("hostname"),
114+ "primary_mac": mac.lower(), # Lowercase.
115+ "interfaces": [{
116+ "mac": mac.upper(), # Uppercase.
117+ "ip_assignment": DEVICE_IP_ASSIGNMENT.DYNAMIC,
118+ }],
119+ })
120+ self.assertThat(created_device["primary_mac"], Equals(mac))
121+
122+ def test_create_copes_with_mac_addresses_of_different_forms(self):
123+ user = factory.make_User()
124+ handler = DeviceHandler(user, {})
125+ mac = factory.make_mac_address(delimiter=":")
126+ created_device = handler.create({
127+ "hostname": factory.make_name("hostname"),
128+ "primary_mac": mac, # Colons.
129+ "interfaces": [{
130+ "mac": mac.replace(":", "-"), # Hyphens.
131+ "ip_assignment": DEVICE_IP_ASSIGNMENT.DYNAMIC,
132+ }],
133+ })
134+ self.assertThat(created_device["primary_mac"], Equals(mac))
135+
136 def test_missing_action_raises_error(self):
137 user = factory.make_User()
138 device = self.make_device_with_ip_address(owner=user)