← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~jtv/maas/dhcp-updates-via-server into lp:maas

 

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/dhcp-updates-via-server into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/dhcp-updates-via-server/+merge/120367

This branch undoes a shortcut, as pre-imped with Julian.  When a worker notices a new DHCP lease, it calls update_leases on the central API, which triggers any required updates to DNS.  But somewhere in the process, we must also call OMAPI to replace the lease with a static one, so that it never expires while the node that got the lease is turned off.

The shortcut we took here was: instead of having the server call a task, which Rabbit will route back to the same worker, the worker can perform its OMAPI directly.  But there is a problem with this: the worker has no idea whether DHCP management is actually enabled!  If it isn't, then the worker has no business messing with OMAPI.

We have a mechanism that would deal with this problem very easily (the cache of shared variables that the server sends to the workers) but since we prefer not to use that mechanism, we had to do things differently.  In this branch I remove the shortcut: the update_leases API call (triggered from the worker) now sends a task back to that same worker to update the leases — or if DHCP management is disabled, it doesn't.

Most of the branch is removal of some bits that became unnecessary now.  In particular, the omapi key no longer needs to be in the shared-variables cache.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/dhcp-updates-via-server/+merge/120367
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/dhcp-updates-via-server into lp:maas.
=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-08-17 02:27:29 +0000
+++ src/maasserver/api.py	2012-08-20 11:24:36 +0000
@@ -928,8 +928,14 @@
     @api_exported('POST')
     def update_leases(self, request, name):
         leases = get_mandatory_param(request.data, 'leases')
+        new_leases = request.data.get('new_leases', None)
         nodegroup = get_nodegroup_for_worker(request, name)
-        DHCPLease.objects.update_leases(nodegroup, json.loads(leases))
+        leases = json.loads(leases)
+        DHCPLease.objects.update_leases(nodegroup, leases)
+        if new_leases is not None:
+            new_leases = json.loads(new_leases)
+            nodegroup.add_dhcp_host_maps(
+                {ip: mac for ip, mac in leases.items() if ip in new_leases})
         return HttpResponse("Leases updated.", status=httplib.OK)
 
 

=== modified file 'src/maasserver/models/nodegroup.py'
--- src/maasserver/models/nodegroup.py	2012-08-17 04:28:56 +0000
+++ src/maasserver/models/nodegroup.py	2012-08-20 11:24:36 +0000
@@ -28,7 +28,10 @@
     Token,
     )
 from provisioningserver.omshell import generate_omapi_key
-from provisioningserver.tasks import write_dhcp_config
+from provisioningserver.tasks import (
+    add_new_dhcp_host_map,
+    write_dhcp_config,
+    )
 
 
 class NodeGroupManager(Manager):
@@ -146,3 +149,10 @@
                 self.ip_range_low,
                 self.ip_range_high
                 ])
+
+    def add_dhcp_host_maps(self, new_leases):
+        if self.is_dhcp_enabled() and len(new_leases) > 0:
+            # The DHCP server is currently always local to the worker
+            # system, so use 127.0.0.1.
+            add_new_dhcp_host_map.delay(
+                new_leases, '127.0.0.1', self.dhcp_key)

=== modified file 'src/maasserver/refresh_worker.py'
--- src/maasserver/refresh_worker.py	2012-08-17 05:20:19 +0000
+++ src/maasserver/refresh_worker.py	2012-08-20 11:24:36 +0000
@@ -38,8 +38,5 @@
         'nodegroup_name': nodegroup.name,
     }
 
-    if nodegroup.dhcp_key is not None and len(nodegroup.dhcp_key) > 0:
-        items['omapi_key'] = nodegroup.dhcp_key
-
     # TODO: Route this to the right worker, once we have multiple.
     refresh_secrets.delay(**items)

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-08-17 02:27:29 +0000
+++ src/maasserver/tests/test_api.py	2012-08-20 11:24:36 +0000
@@ -93,6 +93,7 @@
     POWER_TYPE,
     POWER_TYPE_CHOICES,
     )
+from provisioningserver.tasks import add_new_dhcp_host_map
 from testtools.matchers import (
     Contains,
     Equals,
@@ -2440,6 +2441,16 @@
         get_worker_user(), token=nodegroup.api_token)
 
 
+def disable_dhcp_management(nodegroup):
+    """Turn off MAAS DHCP management on `nodegroup`."""
+    nodegroup.subnet_mask = None
+    nodegroup.broadcast_ip = None
+    nodegroup.router_ip = None
+    nodegroup.ip_range_low = None
+    nodegroup.ip_range_high = None
+    nodegroup.save()
+
+
 class TestNodeGroupAPI(APITestCase):
 
     def test_reverse_points_to_nodegroup(self):
@@ -2479,21 +2490,72 @@
 
     def test_update_leases_stores_leases(self):
         nodegroup = factory.make_node_group()
-        ip = factory.getRandomIPAddress()
-        mac = factory.getRandomMACAddress()
-        client = make_worker_client(nodegroup)
-        response = client.post(
-            reverse('nodegroup_handler', args=[nodegroup.name]),
-            {
-                'op': 'update_leases',
-                'leases': json.dumps({ip: mac}),
-            })
-        self.assertEqual(
-            (httplib.OK, "Leases updated."),
-            (response.status_code, response.content))
-        self.assertEqual([ip], [
-            lease.ip
-            for lease in DHCPLease.objects.filter(nodegroup=nodegroup)])
+        lease = factory.make_random_leases()
+        client = make_worker_client(nodegroup)
+        response = client.post(
+            reverse('nodegroup_handler', args=[nodegroup.name]),
+            {
+                'op': 'update_leases',
+                'leases': json.dumps(lease),
+            })
+        self.assertEqual(
+            (httplib.OK, "Leases updated."),
+            (response.status_code, response.content))
+        self.assertItemsEqual(lease.keys(), [
+            lease.ip
+            for lease in DHCPLease.objects.filter(nodegroup=nodegroup)])
+
+    def test_update_leases_stores_leases_even_if_not_managing_dhcp(self):
+        nodegroup = factory.make_node_group()
+        disable_dhcp_management(nodegroup)
+        lease = factory.make_random_leases()
+        client = make_worker_client(nodegroup)
+        response = client.post(
+            reverse('nodegroup_handler', args=[nodegroup.name]),
+            {
+                'op': 'update_leases',
+                'leases': json.dumps(lease),
+            })
+        self.assertEqual(
+            (httplib.OK, "Leases updated."),
+            (response.status_code, response.content))
+        self.assertItemsEqual(lease.keys(), [
+            lease.ip
+            for lease in DHCPLease.objects.filter(nodegroup=nodegroup)])
+
+    def test_update_leases_adds_new_leases_on_worker(self):
+        nodegroup = factory.make_node_group()
+        client = make_worker_client(nodegroup)
+        self.patch(add_new_dhcp_host_map, 'delay', FakeMethod())
+        new_leases = factory.make_random_leases()
+        response = client.post(
+            reverse('nodegroup_handler', args=[nodegroup.name]),
+            {
+                'op': 'update_leases',
+                'leases': json.dumps(new_leases),
+                'new_leases': json.dumps(new_leases.keys()),
+            })
+        self.assertEqual(
+            (httplib.OK, "Leases updated."),
+            (response.status_code, response.content))
+        [task_call] = add_new_dhcp_host_map.delay.extract_args()
+        self.assertEqual(new_leases, task_call[0])
+
+    def test_update_leases_does_not_add_old_leases(self):
+        nodegroup = factory.make_node_group()
+        client = make_worker_client(nodegroup)
+        self.patch(add_new_dhcp_host_map, 'delay', FakeMethod())
+        response = client.post(
+            reverse('nodegroup_handler', args=[nodegroup.name]),
+            {
+                'op': 'update_leases',
+                'leases': json.dumps(factory.make_random_leases()),
+                'new_leases': json.dumps([]),
+            })
+        self.assertEqual(
+            (httplib.OK, "Leases updated."),
+            (response.status_code, response.content))
+        self.assertEqual([], add_new_dhcp_host_map.delay.calls)
 
 
 class TestNodeGroupAPIAuth(APIv10TestMixin, TestCase):

=== modified file 'src/maasserver/tests/test_nodegroup.py'
--- src/maasserver/tests/test_nodegroup.py	2012-08-07 21:15:54 +0000
+++ src/maasserver/tests/test_nodegroup.py	2012-08-20 11:24:36 +0000
@@ -20,7 +20,10 @@
 from maastesting.fakemethod import FakeMethod
 from maastesting.matchers import ContainsAll
 from provisioningserver import tasks
-from provisioningserver.omshell import generate_omapi_key
+from provisioningserver.omshell import (
+    generate_omapi_key,
+    Omshell,
+    )
 from testtools.matchers import (
     FileContains,
     GreaterThan,
@@ -196,3 +199,28 @@
         nodegroup = factory.make_node_group()
         nodegroup.set_up_dhcp()
         self.assertEqual(1, recorder.call_count)
+
+    def test_add_dhcp_host_maps_calls_task(self):
+        self.patch(Omshell, 'create', FakeMethod())
+        nodegroup = factory.make_node_group()
+        leases = factory.make_random_leases()
+        nodegroup.add_dhcp_host_maps(leases)
+        self.assertEqual(
+            [(leases.keys()[0], leases.values()[0])],
+            Omshell.create.extract_args())
+
+    def test_add_dhcp_host_maps_adds_leases_if_managing_dhcp(self):
+        self.patch(tasks.add_new_dhcp_host_map, 'delay', FakeMethod())
+        nodegroup = factory.make_node_group()
+        leases = factory.make_random_leases()
+        nodegroup.add_dhcp_host_maps(leases)
+        self.assertEqual(
+            [(leases, '127.0.0.1', nodegroup.dhcp_key)],
+            tasks.add_new_dhcp_host_map.delay.extract_args())
+
+    def test_add_dhcp_host_maps_does_nothing_if_not_managing_dhcp(self):
+        self.patch(tasks.add_new_dhcp_host_map, 'delay', FakeMethod())
+        nodegroup = factory.make_node_group()
+        self.patch(nodegroup, 'is_dhcp_enabled', FakeMethod(result=False))
+        nodegroup.add_dhcp_host_maps(factory.make_random_leases())
+        self.assertEqual([], tasks.add_new_dhcp_host_map.delay.calls)

=== modified file 'src/maasserver/tests/test_refresh_worker.py'
--- src/maasserver/tests/test_refresh_worker.py	2012-08-17 04:28:56 +0000
+++ src/maasserver/tests/test_refresh_worker.py	2012-08-20 11:24:36 +0000
@@ -50,23 +50,6 @@
             [(creds_string, )],
             refresh_functions['api_credentials'].extract_args())
 
-    def test_refreshes_omapi_key(self):
-        refresh_functions = self.patch_refresh_functions()
-        dhcp_key = factory.getRandomString()
-        nodegroup = factory.make_node_group(dhcp_key=dhcp_key)
-        refresh_worker(nodegroup)
-        self.assertEqual(
-            [(dhcp_key, )],
-            refresh_functions['omapi_key'].extract_args())
-
-    def test_omits_omapi_key_if_not_set(self):
-        refresh_functions = self.patch_refresh_functions()
-        nodegroup = factory.make_node_group()
-        refresh_worker(nodegroup)
-        self.assertEqual(
-            [],
-            refresh_functions['omapi_key'].extract_args())
-
     def test_refreshes_nodegroup_name(self):
         refresh_functions = self.patch_refresh_functions()
         nodegroup = factory.make_node_group()

=== modified file 'src/provisioningserver/dhcp/leases.py'
--- src/provisioningserver/dhcp/leases.py	2012-08-17 11:45:32 +0000
+++ src/provisioningserver/dhcp/leases.py	2012-08-20 11:24:36 +0000
@@ -60,20 +60,6 @@
 LEASES_CACHE_KEY = 'recorded_leases'
 
 
-# Cache key for the key we use to authenticate to omshell.
-OMAPI_KEY_CACHE_KEY = 'omapi_key'
-
-
-def record_omapi_key(omapi_key):
-    """Record the OMAPI key as received from the server."""
-    cache.set(OMAPI_KEY_CACHE_KEY, omapi_key)
-
-
-def get_recorded_omapi_key():
-    """Return the current OMAPI key as received from the server."""
-    return cache.get(OMAPI_KEY_CACHE_KEY)
-
-
 def get_leases_timestamp():
     """Return the last modification timestamp of the DHCP leases file."""
     return stat(DHCP_LEASES_FILE).st_mtime
@@ -120,44 +106,6 @@
     cache.set(LEASES_CACHE_KEY, leases)
 
 
-def identify_new_leases(current_leases):
-    """Return a dict of those leases that weren't previously recorded.
-
-    :param current_leases: A dict mapping IP addresses to the respective
-        MAC addresses that own them.
-    """
-    # The recorded leases is shared between worker threads/processes.
-    # Read it just once to reduce the impact of concurrent changes.
-    previous_leases = cache.get(LEASES_CACHE_KEY)
-    if previous_leases is None:
-        return current_leases
-    else:
-        return {
-            ip: current_leases[ip]
-            for ip in set(current_leases).difference(previous_leases)}
-
-
-def register_new_leases(current_leases):
-    """Register new DHCP leases with the OMAPI.
-
-    :param current_leases: A dict mapping IP addresses to the respective
-        MAC addresses that own them.
-    """
-    # Avoid circular imports.
-    from provisioningserver.tasks import add_new_dhcp_host_map
-
-    # The recorded_omapi_key is shared between worker threads or
-    # processes, so read it just once, atomically.
-    omapi_key = get_recorded_omapi_key()
-    if omapi_key is None:
-        task_logger.info(
-            "Not registering new leases: "
-            "no OMAPI key received from server yet.")
-    else:
-        new_leases = identify_new_leases(current_leases)
-        add_new_dhcp_host_map(new_leases, 'localhost', omapi_key)
-
-
 def list_missing_items(knowledge):
     """Report items from dict `knowledge` that are still `None`."""
     return sorted(name for name, value in knowledge.items() if value is None)
@@ -188,11 +136,7 @@
 
 
 def process_leases(timestamp, leases):
-    """Register leases with the DHCP server, and send to the MAAS server."""
-    # Register new leases before recording them.  That way, any
-    # failure to register a lease will cause it to be retried at the
-    # next opportunity.
-    register_new_leases(leases)
+    """Send new leases to the MAAS server."""
     record_lease_state(timestamp, leases)
     send_leases(leases)
 

=== modified file 'src/provisioningserver/dhcp/tests/test_leases.py'
--- src/provisioningserver/dhcp/tests/test_leases.py	2012-08-20 07:28:41 +0000
+++ src/provisioningserver/dhcp/tests/test_leases.py	2012-08-20 11:24:36 +0000
@@ -33,31 +33,21 @@
 from provisioningserver.dhcp import leases as leases_module
 from provisioningserver.dhcp.leases import (
     check_lease_changes,
-    identify_new_leases,
     LEASES_CACHE_KEY,
     LEASES_TIME_CACHE_KEY,
-    OMAPI_KEY_CACHE_KEY,
     parse_leases_file,
     process_leases,
     record_lease_state,
-    record_omapi_key,
-    register_new_leases,
     send_leases,
     update_leases,
     upload_leases,
     )
 from provisioningserver.omshell import Omshell
 from provisioningserver.testing.testcase import PservTestCase
-from testtools.testcase import ExpectedException
 
 
 class TestHelpers(PservTestCase):
 
-    def test_record_omapi_key_records_key(self):
-        key = factory.getRandomString()
-        record_omapi_key(key)
-        self.assertEqual(key, cache.get(OMAPI_KEY_CACHE_KEY))
-
     def test_record_lease_state_records_time_and_leases(self):
         time = datetime.utcnow()
         leases = factory.make_random_leases()
@@ -118,16 +108,6 @@
         self.redirect_parser(leases_file)
         return leases_file
 
-    def set_omapi_key(self, key=None):
-        """Set a recorded omapi key for the duration of this test."""
-        if key is None:
-            key = factory.getRandomString()
-        cache.set(OMAPI_KEY_CACHE_KEY, key)
-
-    def clear_omapi_key(self):
-        """Clear the recorded omapi key."""
-        cache.set(OMAPI_KEY_CACHE_KEY, None)
-
     def set_nodegroup_name(self):
         """Set the recorded nodegroup name for the duration of this test."""
         name = factory.make_name('nodegroup')
@@ -164,7 +144,6 @@
         """Set the recorded items required by `update_leases`."""
         self.set_maas_url()
         self.set_api_credentials()
-        self.set_omapi_key()
         self.set_nodegroup_name()
 
     def set_lease_state(self, time=None, leases=None):
@@ -285,35 +264,6 @@
         self.fake_leases_file(new_leases)
         self.assertIsNone(check_lease_changes())
 
-    def test_process_leases_registers_new_leases(self):
-        self.set_lease_state()
-        self.set_items_needed_for_lease_update()
-        self.patch(Omshell, 'create', FakeMethod())
-        self.patch(leases_module, 'send_leases', FakeMethod())
-        ip = factory.getRandomIPAddress()
-        mac = factory.getRandomMACAddress()
-        process_leases(datetime.utcnow(), {ip: mac})
-        self.assertEqual([(ip, mac)], Omshell.create.extract_args())
-
-    def test_process_leases_retries_failed_registrations(self):
-
-        class OmshellFailure(Exception):
-            pass
-
-        self.set_lease_state()
-        self.set_items_needed_for_lease_update()
-        self.patch(leases_module, 'send_leases', FakeMethod())
-        self.patch(Omshell, 'create', FakeMethod(failure=OmshellFailure()))
-        ip = factory.getRandomIPAddress()
-        mac = factory.getRandomMACAddress()
-        with ExpectedException(OmshellFailure):
-            process_leases(datetime.utcnow(), {ip: mac})
-        # At this point {ip: mac} has not been successfully registered.
-        # But if we re-run process_leases later, it will retry.
-        self.patch(Omshell, 'create', FakeMethod())
-        process_leases(datetime.utcnow(), {ip: mac})
-        self.assertEqual([(ip, mac)], Omshell.create.extract_args())
-
     def test_upload_leases_processes_leases_unconditionally(self):
         leases = factory.make_random_leases()
         leases_file = self.fake_leases_file(leases)
@@ -343,52 +293,6 @@
             (get_write_time(leases_file), {params['ip']: params['mac']}),
             parse_leases_file())
 
-    def test_identify_new_leases_identifies_everything_first_time(self):
-        self.patch(leases_module, 'recorded_leases', None)
-        current_leases = factory.make_random_leases(2)
-        self.assertEqual(current_leases, identify_new_leases(current_leases))
-
-    def test_identify_new_leases_identifies_previously_unknown_leases(self):
-        self.patch(leases_module, 'recorded_leases', {})
-        current_leases = factory.make_random_leases()
-        self.assertEqual(current_leases, identify_new_leases(current_leases))
-
-    def test_identify_new_leases_leaves_out_previously_known_leases(self):
-        old_leases = factory.make_random_leases()
-        cache.set(LEASES_CACHE_KEY, old_leases)
-        new_leases = factory.make_random_leases()
-        current_leases = old_leases.copy()
-        current_leases.update(new_leases)
-        self.assertEqual(new_leases, identify_new_leases(current_leases))
-
-    def test_register_new_leases_registers_new_leases(self):
-        self.patch(Omshell, 'create', FakeMethod())
-        self.set_lease_state()
-        self.set_omapi_key()
-        ip = factory.getRandomIPAddress()
-        mac = factory.getRandomMACAddress()
-        register_new_leases({ip: mac})
-        [create_args] = Omshell.create.extract_args()
-        self.assertEqual((ip, mac), create_args)
-
-    def test_register_new_leases_ignores_known_leases(self):
-        self.patch(Omshell, 'create', FakeMethod())
-        self.set_omapi_key()
-        self.set_nodegroup_name()
-        old_leases = factory.make_random_leases()
-        self.set_lease_state(None, old_leases)
-        register_new_leases(old_leases)
-        self.assertEqual([], Omshell.create.calls)
-
-    def test_register_new_leases_does_nothing_without_omapi_key(self):
-        self.patch(Omshell, 'create', FakeMethod())
-        self.set_lease_state()
-        self.set_items_needed_for_lease_update()
-        self.clear_omapi_key()
-        new_leases = factory.make_random_leases()
-        register_new_leases(new_leases)
-        self.assertEqual([], Omshell.create.calls)
-
     def test_send_leases_posts_to_API(self):
         self.patch(Omshell, 'create', FakeMethod())
         self.set_items_needed_for_lease_update()

=== modified file 'src/provisioningserver/tasks.py'
--- src/provisioningserver/tasks.py	2012-08-17 05:20:19 +0000
+++ src/provisioningserver/tasks.py	2012-08-20 11:24:36 +0000
@@ -13,6 +13,7 @@
 __all__ = [
     'power_off',
     'power_on',
+    'refresh_secrets',
     'rndc_command',
     'setup_rndc_configuration',
     'restart_dhcp_server',
@@ -34,10 +35,7 @@
     record_maas_url,
     record_nodegroup_name,
     )
-from provisioningserver.dhcp import (
-    config,
-    leases,
-    )
+from provisioningserver.dhcp import config
 from provisioningserver.dns.config import (
     DNSConfig,
     execute_rndc_command,
@@ -54,7 +52,6 @@
 refresh_functions = {
     'api_credentials': record_api_credentials,
     'maas_url': record_maas_url,
-    'omapi_key': leases.record_omapi_key,
     'nodegroup_name': record_nodegroup_name,
 }
 
@@ -93,8 +90,6 @@
     :param api_credentials: A colon separated string containing this
         worker's credentials for accessing the MAAS API: consumer key,
         resource token, resource secret.
-    :param omapi_key: Shared key for working with the worker's
-        DHCP server.
     :param nodegroup_name: The name of the node group that this worker
         manages.
     """
@@ -253,6 +248,8 @@
 def add_new_dhcp_host_map(mappings, server_address, shared_key):
     """Add address mappings to the DHCP server.
 
+    Do not invoke this when DHCP is set to be managed manually.
+
     :param mappings: A dict of new IP addresses, and the MAC addresses they
         translate to.
     :param server_address: IP or hostname for the DHCP server
@@ -260,7 +257,6 @@
         control.
     """
     omshell = Omshell(server_address, shared_key)
-    refresh_secrets(omapi_key=shared_key)
     try:
         for ip_address, mac_address in mappings.items():
             omshell.create(ip_address, mac_address)
@@ -276,13 +272,14 @@
 def remove_dhcp_host_map(ip_address, server_address, omapi_key):
     """Remove an IP to MAC mapping in the DHCP server.
 
+    Do not invoke this when DHCP is set to be managed manually.
+
     :param ip_address: Dotted quad string
     :param server_address: IP or hostname for the DHCP server
     :param omapi_key: The HMAC-MD5 key that the DHCP server uses for access
         control.
     """
     omshell = Omshell(server_address, omapi_key)
-    refresh_secrets(omapi_key=omapi_key)
     try:
         omshell.remove(ip_address)
     except CalledProcessError:

=== modified file 'src/provisioningserver/tests/test_tasks.py'
--- src/provisioningserver/tests/test_tasks.py	2012-08-17 05:20:19 +0000
+++ src/provisioningserver/tests/test_tasks.py	2012-08-20 11:24:36 +0000
@@ -30,7 +30,6 @@
     tasks,
     )
 from provisioningserver.cache import cache
-from provisioningserver.dhcp.leases import get_recorded_omapi_key
 from provisioningserver.dns.config import (
     conf,
     DNSZoneConfig,
@@ -109,11 +108,6 @@
         refresh_secrets(nodegroup_name=nodegroup_name)
         self.assertEqual(nodegroup_name, cache.get('nodegroup_name'))
 
-    def test_updates_omapi_key(self):
-        key = factory.make_name('omapi-key')
-        refresh_secrets(omapi_key=key)
-        self.assertEqual(key, get_recorded_omapi_key())
-
 
 class TestPowerTasks(PservTestCase):
 
@@ -180,12 +174,6 @@
             CalledProcessError, add_new_dhcp_host_map.delay,
             {mac: ip}, server_address, key)
 
-    def test_add_new_dhcp_host_map_records_shared_key(self):
-        key = factory.getRandomString()
-        self.patch(Omshell, '_run', FakeMethod())
-        add_new_dhcp_host_map({}, factory.make_name('server'), key)
-        self.assertEqual(key, get_recorded_omapi_key())
-
     def test_remove_dhcp_host_map(self):
         # We don't want to actually run omshell in the task, so we stub
         # out the wrapper class's _run method and record what it would
@@ -210,13 +198,6 @@
             CalledProcessError, remove_dhcp_host_map.delay,
             ip, server_address, key)
 
-    def test_remove_dhcp_host_map_records_shared_key(self):
-        key = factory.getRandomString()
-        self.patch(Omshell, '_run', FakeMethod((0, "obj: <null>")))
-        remove_dhcp_host_map(
-            factory.getRandomIPAddress(), factory.make_name('server'), key)
-        self.assertEqual(key, get_recorded_omapi_key())
-
     def test_write_dhcp_config_writes_config(self):
         conf_file = self.make_file(contents=factory.getRandomString())
         self.patch(tasks, 'DHCP_CONFIG_FILE', conf_file)