Merge lp:~jtv/maas/resolve-mac-from-leases into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Try harder to resolve a node's power IP address. Look not just at the ARP cache but the given IP address (if any) and parsed DHCP leases as well. This should enable cold boots of AMT nodes on MAAS-managed networks.

This should fix the problem where we can't power up nodes whose power addresses are not in the ARP cache — provided that the MAC got a DHCP lease from the cluster controller.

Jeroen

[Gavin Panella (allenap)]

Fwiw, I think the code is good.

However, I worry that it's going to be looking at a snapshot of leases that might be several minutes old, so we might still want a more immediate solution, e.g. nmap.

Also, we only ever ought to have one lease for a MAC address. This code is complicated because it has to consider multiple MAC addresses, but I'm not sure that's necessary or desirable. IMO, it should try the ARP cache then fall back to checking leases.

(This is somewhat related to something that has come up in review for lp:~allenap/maas/dhcp-leases-parsing. The new parser discards all but the last lease for each IP address then discards that if it has expired. The existing parser does the same operations but in reverse order: it discards all expired leases before choosing the most recent of those remaining. I'm still not 100% sure what the right behaviour should be, but I'm leaning towards the former.)

[Jeroen T. Vermeulen (jtv)]

Thanks for having a spontaneous look. This is still experimental: I really just wanted to see whether it would solve the problem. I'm trying that today.

As for the multiple mappings, I'm trying to cover for scenarios like a NIC getting different DHCP leases for its firmware boot and its OS boot. We don't support multiple MACs for one IP address, but multiple IPs mapping to one MAC seems quite plausible just because it takes an effort to prevent.

=== modified file 'src/provisioningserver/dhcp/leases.py'
--- src/provisioningserver/dhcp/leases.py	2013-10-07 09:12:40 +0000
+++ src/provisioningserver/dhcp/leases.py	2014-05-01 12:53:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Send lease updates to the server.
@@ -29,6 +29,7 @@
 
 __metaclass__ = type
 __all__ = [
+    'resolve_MAC',
     'upload_leases',
     'update_leases',
     ]
@@ -54,7 +55,11 @@
     get_recorded_nodegroup_uuid,
     )
 from provisioningserver.cluster_config import get_maas_url
-from provisioningserver.dhcp.leases_parser import parse_leases
+from provisioningserver.dhcp.leases_parser import (
+    normalise_MAC,
+    parse_leases,
+    )
+from provisioningserver.utils import find_ip_via_arp
 
 
 logger = getLogger(__name__)
@@ -117,7 +122,6 @@
     # but read them both at once here to reduce the scope for trouble.
     previous_leases = cache.cache.get(LEASES_CACHE_KEY)
     previous_leases_time = cache.cache.get(LEASES_TIME_CACHE_KEY)
-
     if get_leases_timestamp() == previous_leases_time:
         return None
     parse_result = parse_leases_file()
@@ -209,3 +213,28 @@
     if updated_lease_info is not None:
         timestamp, leases = updated_lease_info
         process_leases(timestamp, leases)
+
+
+def resolve_MAC(mac):
+    """Look up IP addresses associated with the given MAC address.
+
+    This uses the cached leases from the last upload, but also looks at the
+    system's ARP cache.
+
+    :param mac: A string representing a MAC address, in colon-separated format.
+    :return: A set of matching IP addresses, as strings.
+    """
+    mac = normalise_MAC(mac)
+    leases = cache.cache.get(LEASES_CACHE_KEY)
+    if leases is None:
+        ips = set()
+    else:
+        ips = {
+            lease_ip
+            for lease_ip, lease_mac in leases.items()
+            if lease_mac == mac
+            }
+    arp_ip = find_ip_via_arp(mac)
+    if arp_ip is not None:
+        ips.add(arp_ip)
+    return ips

=== modified file 'src/provisioningserver/dhcp/leases_parser.py'
--- src/provisioningserver/dhcp/leases_parser.py	2013-10-15 11:35:10 +0000
+++ src/provisioningserver/dhcp/leases_parser.py	2014-05-01 12:53:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Parser for ISC dhcpd leases file.
@@ -19,6 +19,7 @@
 
 __metaclass__ = type
 __all__ = [
+    'normalise_MAC',
     'parse_leases',
     ]
 
@@ -199,6 +200,16 @@
     return leases
 
 
+def normalise_MAC(mac):
+    """Represent a MAC address in a normalised way so it can be compared."""
+    return mac.lower()
+
+
+def normalise_leases(leases):
+    """Normalise IP/host mapping."""
+    return {ip: normalise_MAC(mac) for ip, mac in leases.items()}
+
+
 def parse_leases(leases_contents):
     """Parse contents of a leases file.
 
@@ -207,4 +218,5 @@
         address that it is associated with.
     """
     entries = lease_parser.searchString(leases_contents)
-    return combine_entries(entries)
+    leases = combine_entries(entries)
+    return normalise_leases(leases)

=== modified file 'src/provisioningserver/dhcp/tests/test_leases.py'
--- src/provisioningserver/dhcp/tests/test_leases.py	2013-10-07 09:12:40 +0000
+++ src/provisioningserver/dhcp/tests/test_leases.py	2014-05-01 12:53:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the report_leases task."""
@@ -40,12 +40,14 @@
     parse_leases_file,
     process_leases,
     record_lease_state,
+    resolve_MAC,
     send_leases,
     update_leases,
     upload_leases,
     )
 from provisioningserver.omshell import Omshell
 from provisioningserver.testing.testcase import PservTestCase
+from provisioningserver.testing.worker_cache import WorkerCacheFixture
 
 
 class TestHelpers(PservTestCase):
@@ -346,3 +348,61 @@
         leases = factory.make_random_leases()
         send_leases(leases)
         self.assertEqual([], MAASClient.post.calls)
+
+
+class TestResolveMAC(PservTestCase):
+
+    def patch_arp_lookup(self, mac_to_ip):
+        """Patch ARP lookup to map the given MAC addresses to the given IPs.
+
+        Lookup on any un-mapped addresses will return `None`.
+        """
+        self.patch(
+            leases_module, 'find_ip_via_arp', lambda mac: mac_to_ip.get(mac))
+
+    def patch_cached_leases(self, leases):
+        """Patch the worker cache to contain the given leases."""
+        self.useFixture(WorkerCacheFixture())
+        cache.cache.set(LEASES_CACHE_KEY, leases)
+
+    def test__returns_empty_if_nothing_cached(self):
+        self.patch_arp_lookup({})
+        self.patch_cached_leases(None)
+        self.assertEqual(set(), resolve_MAC(factory.getRandomMACAddress()))
+
+    def test__returns_IP_from_lease(self):
+        mac = factory.getRandomMACAddress()
+        ip = factory.getRandomIPAddress()
+        self.patch_arp_lookup({})
+        self.patch_cached_leases({ip: mac})
+        self.assertEqual({ip}, resolve_MAC(mac))
+
+    def test__ignores_nonmatching_leases(self):
+        self.patch_arp_lookup({})
+        self.patch_cached_leases({
+            factory.getRandomIPAddress(): factory.getRandomMACAddress(),
+            })
+        self.assertEqual(set(), resolve_MAC(factory.getRandomMACAddress()))
+
+    def test__filters_for_all_matching_leases(self):
+        self.patch_arp_lookup({})
+        mac = factory.getRandomMACAddress()
+        ips = {factory.getRandomIPAddress() for _ in range(3)}
+        self.patch_cached_leases({ip: mac for ip in ips})
+        self.assertEqual(ips, resolve_MAC(mac))
+
+    def test__adds_ARP_lookup_if_found(self):
+        mac = factory.getRandomMACAddress()
+        ip = factory.getRandomIPAddress()
+        self.patch_arp_lookup({mac: ip})
+        self.patch_cached_leases({})
+        self.assertEqual({ip}, resolve_MAC(mac))
+
+    def test__ignores_case(self):
+        mac = factory.getRandomMACAddress()
+        lease_ip = factory.getRandomIPAddress()
+        arp_ip = factory.getRandomIPAddress()
+        self.patch_arp_lookup({mac: arp_ip})
+        self.patch_cached_leases({lease_ip: mac})
+        self.assertEqual({lease_ip, arp_ip}, resolve_MAC(mac.upper()))
+        self.assertEqual({lease_ip, arp_ip}, resolve_MAC(mac.lower()))

=== modified file 'src/provisioningserver/dhcp/tests/test_leases_parser.py'
--- src/provisioningserver/dhcp/tests/test_leases_parser.py	2013-10-07 09:12:40 +0000
+++ src/provisioningserver/dhcp/tests/test_leases_parser.py	2014-05-01 12:53:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012, 2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the DHCP leases parser."""

=== modified file 'src/provisioningserver/tasks.py'
--- src/provisioningserver/tasks.py	2014-04-22 15:10:32 +0000
+++ src/provisioningserver/tasks.py	2014-05-01 12:53:29 +0000
@@ -49,7 +49,10 @@
     config,
     detect,
     )
-from provisioningserver.dhcp.leases import upload_leases
+from provisioningserver.dhcp.leases import (
+    resolve_MAC,
+    upload_leases,
+    )
 from provisioningserver.dns.config import (
     DNSConfig,
     execute_rndc_command,
@@ -158,20 +161,35 @@
     assert power_change in ('on', 'off'), (
         "Unknown power change keyword: %s" % power_change)
     kwargs['power_change'] = power_change
+
+    # Figure out the IP address(es) where we should send our power command.
     if 'mac_address' in kwargs:
-        kwargs['ip_address'] = find_ip_via_arp(kwargs['mac_address'])
-    kwargs.setdefault('ip_address', None)
-    try:
-        pa = PowerAction(power_type)
-        pa.execute(**kwargs)
-    except PowerActionFail:
-        # TODO: signal to webapp that it failed
-
-        # Re-raise, so the job is marked as failed.  Only currently
-        # useful for tests.
-        raise
-
-    # TODO: signal to webapp that it worked.
+        ips = resolve_MAC(kwargs['mac_address'])
+    else:
+        ips = set()
+    if 'ip_address' in kwargs:
+        ips.add(kwargs['ip_address'])
+    if len(ips) == 0:
+        # No address set.  We should run the action at least once though, in
+        # case it doesn't require an address.
+        ips.add(None)
+
+    # Send our power command to each IP address we have for the node.
+    failures = {}
+    for ip in ips:
+        kwargs['ip_address'] = ip
+        try:
+            pa = PowerAction(power_type)
+            pa.execute(**kwargs)
+        except PowerActionFail as e:
+            failures[ip] = e
+
+    if len(failures) == len(ips):
+        # All attempts failed.  Propagate an exception so that the job is
+        # marked as failed.
+        raise PowerActionFail(pa, "Power action failed: %s" % failures)
+
+    # XXX 2014-05-01 jtv: Notify the webapp of success or failure.
 
 
 @task

=== modified file 'src/provisioningserver/tests/test_tasks.py'
--- src/provisioningserver/tests/test_tasks.py	2014-04-22 15:10:32 +0000
+++ src/provisioningserver/tests/test_tasks.py	2014-05-01 12:53:29 +0000
@@ -41,6 +41,7 @@
     MockCalledOnceWith,
     )
 from mock import (
+    call,
     Mock,
     sentinel,
     )
@@ -173,35 +174,67 @@
         # Patch out PowerAction; we're just trying to demonstrate that
         # it's invoked with an IP address even if one is not supplied.
         PowerAction = self.patch(tasks, "PowerAction")
-        # find_ip_via_arp() is tested elsewhere; we just want to know
+        # resolve_MAC() is tested elsewhere; we just want to know
         # that it has been used.
-        self.patch(tasks, "find_ip_via_arp").return_value = sentinel.ip_address
+        self.patch(tasks, "resolve_MAC").return_value = {sentinel.ip_address}
+        mac = factory.getRandomMACAddress()
         # PowerAction.execute() is passed an ip_address argument in
         # addition to the mac_address argument when the latter is
         # supplied.
-        tasks.issue_power_action(
-            sentinel.power_type, "on", mac_address=sentinel.mac_address)
+        tasks.issue_power_action(sentinel.power_type, "on", mac_address=mac)
+        self.assertThat(tasks.resolve_MAC, MockCalledOnceWith(mac))
         self.assertThat(PowerAction, MockCalledOnceWith(sentinel.power_type))
-        self.assertThat(PowerAction.return_value.execute, MockCalledOnceWith(
-            power_change="on", mac_address=sentinel.mac_address,
-            ip_address=sentinel.ip_address))
+        self.assertEqual(
+            [
+                call(
+                    power_change='on', mac_address=mac,
+                    ip_address=sentinel.ip_address),
+            ],
+            PowerAction.return_value.execute.mock_calls)
 
-    def test_ip_address_is_looked_up_when_already_supplied(self):
+    def test_ip_address_is_looked_up_even_when_already_supplied(self):
         # Patch out PowerAction; we're just trying to demonstrate that
         # it's invoked with an IP address even if one is not supplied.
         PowerAction = self.patch(tasks, "PowerAction")
-        # find_ip_via_arp() is tested elsewhere; we just want to know
+        # resolve_MAC() is tested elsewhere; we just want to know
         # that it has been used.
-        self.patch(tasks, "find_ip_via_arp").return_value = sentinel.ip_address
+        self.patch(tasks, "resolve_MAC").return_value = {
+            sentinel.resolved_ip_address}
+        mac = factory.getRandomMACAddress()
         # The ip_address argument passed to PowerAction.execute() is
-        # looked-up via find_ip_via_arp() even if an ip_address is
-        # passed into issue_power_action().
+        # looked-up via resolve_MAC() even if an ip_address is
+        # passed into issue_power_action().  The power action is issued to both
+        # IP addresses.
         tasks.issue_power_action(
-            sentinel.power_type, "on", mac_address=sentinel.mac_address,
-            ip_address=sentinel.another_ip_address)
-        self.assertThat(PowerAction.return_value.execute, MockCalledOnceWith(
-            power_change="on", mac_address=sentinel.mac_address,
-            ip_address=sentinel.ip_address))
+            sentinel.power_type, "on", mac_address=mac,
+            ip_address=sentinel.given_ip_address)
+        self.assertItemsEqual(
+            [
+                call(
+                    power_change='on', mac_address=mac,
+                    ip_address=sentinel.given_ip_address),
+                call(
+                    power_change='on', mac_address=mac,
+                    ip_address=sentinel.resolved_ip_address),
+            ],
+            PowerAction.return_value.execute.mock_calls)
+
+    def test_ip_address_is_None_if_no_address_can_be_found(self):
+        # Patch out PowerAction; we're just trying to demonstrate that
+        # it's invoked with an IP address even if one is not supplied.
+        PowerAction = self.patch(tasks, "PowerAction")
+        # resolve_MAC() is tested elsewhere; we just want to know
+        # that it has been used.
+        self.patch(tasks, "resolve_MAC").return_value = set()
+        mac = factory.getRandomMACAddress()
+        # PowerAction.execute() is passed an ip_address argument in
+        # addition to the mac_address argument when the latter is
+        # supplied.
+        tasks.issue_power_action(sentinel.power_type, "on", mac_address=mac)
+        self.assertThat(PowerAction, MockCalledOnceWith(sentinel.power_type))
+        self.assertEqual(
+            [call(power_change='on', mac_address=mac, ip_address=None)],
+            PowerAction.return_value.execute.mock_calls)
 
 
 class TestDHCPTasks(PservTestCase):