Merge lp:~julian-edwards/maas/revert-2819 into lp:~maas-committers/maas/trunk

[Julian Edwards]

Revert r2819 (pserv-based dhcp probing service), which could never work as it was using API credentials that are not sent to pserv.

[Julian Edwards (julian-edwards)]

review: Approve (selfie)

=== modified file 'etc/celeryconfig_cluster.py'
--- etc/celeryconfig_cluster.py	2014-08-22 14:47:15 +0000
+++ etc/celeryconfig_cluster.py	2014-08-26 23:57:04 +0000
@@ -56,4 +56,12 @@
             'expires': int(REPORT_BOOT_IMAGES_SCHEDULE.total_seconds()),
         },
     },
+    'probe-dhcp-servers': {
+        'task': 'provisioningserver.tasks.periodic_probe_dhcp',
+        'schedule': PROBE_DHCP_SERVERS_SCHEDULE,
+        'options': {
+            'queue': CLUSTER_UUID,
+            'expires': int(PROBE_DHCP_SERVERS_SCHEDULE.total_seconds()),
+        },
+    },
 }

=== modified file 'src/provisioningserver/dhcp/detect.py'
--- src/provisioningserver/dhcp/detect.py	2014-08-26 10:19:58 +0000
+++ src/provisioningserver/dhcp/detect.py	2014-08-26 23:57:04 +0000
@@ -33,6 +33,11 @@
     MAASDispatcher,
     MAASOAuth,
     )
+from provisioningserver.auth import (
+    get_recorded_api_credentials,
+    get_recorded_nodegroup_uuid,
+    )
+from provisioningserver.cluster_config import get_maas_url
 from provisioningserver.logger import get_maas_logger
 
 
@@ -316,7 +321,7 @@
         client.post, api_path, 'report_foreign_dhcp', foreign_dhcp_ip=server)
 
 
-def periodic_probe_task(api_knowledge):
+def periodic_probe_task():
     """Probe for DHCP servers and set NodeGroupInterface.foriegn_dhcp.
 
     This should be run periodically so that the database has an up-to-date
@@ -325,12 +330,26 @@
     NOTE: This uses blocking I/O with sequential polling of interfaces, and
     hence doesn't scale well.  It's a future improvement to make
     to throw it in parallel threads or async I/O.
-
-    :param api_knowledge: A dict of the information needed to be able to
-        make requests to the region's REST API.
     """
+    # Items that the server must have sent us before we can do this.
+    knowledge = {
+        'maas_url': get_maas_url(),
+        'api_credentials': get_recorded_api_credentials(),
+        'nodegroup_uuid': get_recorded_nodegroup_uuid(),
+    }
+
+    if None in knowledge.values():
+        # The MAAS server hasn't sent us enough information for us to do
+        # this yet.  Leave it for another time.
+        maaslog.info(
+            "Not probing for rogue DHCP servers; not all required knowledge "
+            "received from server yet.  "
+            "Missing: %s" % ', '.join(sorted(
+                name for name, value in knowledge.items() if value is None)))
+        return
+
     # Determine all the active interfaces on this cluster (nodegroup).
-    interfaces = determine_cluster_interfaces(api_knowledge)
+    interfaces = determine_cluster_interfaces(knowledge)
     if interfaces is None:
         maaslog.info("No interfaces on cluster, not probing DHCP.")
         return
@@ -349,7 +368,6 @@
                 # Only send one, if it gets cleared out then the
                 # next detection pass will send a different one, if it
                 # still exists.
-                update_region_controller(
-                    api_knowledge, interface, servers.pop())
+                update_region_controller(knowledge, interface, servers.pop())
             else:
-                update_region_controller(api_knowledge, interface, None)
+                update_region_controller(knowledge, interface, None)

=== removed file 'src/provisioningserver/dhcp/dhcp_probe_service.py'
--- src/provisioningserver/dhcp/dhcp_probe_service.py	2014-08-26 15:47:52 +0000
+++ src/provisioningserver/dhcp/dhcp_probe_service.py	1970-01-01 00:00:00 +0000
@@ -1,85 +0,0 @@
-# Copyright 2014 Canonical Ltd.  This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""Periodic DHCP probing service."""
-
-from __future__ import (
-    absolute_import,
-    print_function,
-    unicode_literals,
-    )
-
-str = None
-
-__metaclass__ = type
-__all__ = [
-    "PeriodicDHCPProbeService",
-    ]
-
-
-from datetime import timedelta
-
-from provisioningserver.auth import get_recorded_api_credentials
-from provisioningserver.cluster_config import get_maas_url
-from provisioningserver.dhcp import detect
-from provisioningserver.logger.log import get_maas_logger
-from twisted.application.internet import TimerService
-from twisted.internet.defer import inlineCallbacks
-from twisted.internet.threads import deferToThread
-
-
-maaslog = get_maas_logger("dhcp.probe")
-
-
-class PeriodicDHCPProbeService(TimerService, object):
-    """Service to probe for DHCP servers on this cluster's network.
-
-    Built on top of Twisted's `TimerService`.
-
-    :param reactor: An `IReactor` instance.
-    :param cluster_uuid: This cluster's UUID.
-    """
-
-    check_interval = timedelta(minutes=1).total_seconds()
-
-    def __init__(self, reactor, cluster_uuid):
-        # Call self.check() every self.check_interval.
-        super(PeriodicDHCPProbeService, self).__init__(
-            self.check_interval, self.try_probe_dhcp)
-        self.clock = reactor
-        self.uuid = cluster_uuid
-
-    def _probe_dhcp(self, knowledge):
-        """Probe for DHCP servers."""
-        return deferToThread(detect.periodic_probe_task, knowledge)
-
-    @inlineCallbacks
-    def try_probe_dhcp(self):
-        # Items that the server must have sent us before we can do
-        # this.
-        knowledge = {
-            'api_credentials': get_recorded_api_credentials(),
-            'maas_url': get_maas_url(),
-            'nodegroup_uuid': self.uuid,
-        }
-
-        if None in knowledge.values():
-            # The MAAS server hasn't sent us enough information for
-            # us to do this yet.  Leave it for another time.
-            maaslog.info(
-                "Not probing for rogue DHCP servers; not all "
-                "required knowledge received from server yet.  "
-                "Missing: %s" % ', '.join(sorted(
-                    name for name, value in knowledge.items()
-                    if value is None)))
-            return
-        else:
-            maaslog.debug("Running periodic DHCP probe.")
-            try:
-                d = self._probe_dhcp(knowledge)
-                yield d
-                maaslog.debug("Finished periodic DHCP probe.")
-            except Exception as error:
-                maaslog.error(
-                    "Unable to probe for rogue DHCP servers: %s"
-                    % unicode(error))

=== modified file 'src/provisioningserver/dhcp/tests/test_detect.py'
--- src/provisioningserver/dhcp/tests/test_detect.py	2014-08-26 10:19:58 +0000
+++ src/provisioningserver/dhcp/tests/test_detect.py	2014-08-26 23:57:04 +0000
@@ -30,7 +30,10 @@
 from maastesting.testcase import MAASTestCase
 import mock
 from provisioningserver import cache
-from provisioningserver.auth import NODEGROUP_UUID_CACHE_KEY
+from provisioningserver.auth import (
+    NODEGROUP_UUID_CACHE_KEY,
+    record_api_credentials,
+    )
 from provisioningserver.dhcp.detect import (
     BOOTP_CLIENT_PORT,
     BOOTP_SERVER_PORT,
@@ -510,11 +513,17 @@
             "Failed talking to region controller, it returned:",
             self.maaslog.output)
 
+    def test_periodic_probe_task_exits_with_not_enough_knowledge(self):
+        mocked = self.patch(detect_module, 'determine_cluster_interfaces')
+        record_api_credentials(None)
+        periodic_probe_task()
+        self.assertFalse(mocked.called)
+
     def test_periodic_probe_task_exits_if_no_interfaces(self):
         mocked = self.patch(detect_module, 'probe_interface')
         self.patch(
             detect_module, 'determine_cluster_interfaces').return_value = None
-        periodic_probe_task(self.knowledge)
+        periodic_probe_task()
         self.assertFalse(mocked.called)
 
     def test_periodic_probe_task_updates_region_with_detected_server(self):
@@ -526,7 +535,7 @@
         self.patch(
             detect_module, 'probe_dhcp').return_value = {detected_server}
         mocked_update = self.patch(detect_module, 'update_region_controller')
-        periodic_probe_task(self.knowledge)
+        periodic_probe_task()
         calls = [
             mock.call(self.knowledge, 'eth0', detected_server),
             mock.call(self.knowledge, 'wlan0', detected_server),
@@ -541,7 +550,7 @@
         self.patch(
             detect_module, 'probe_dhcp').return_value = set()
         mocked_update = self.patch(detect_module, 'update_region_controller')
-        periodic_probe_task(self.knowledge)
+        periodic_probe_task()
         calls = [
             mock.call(self.knowledge, 'eth0', None),
             mock.call(self.knowledge, 'wlan0', None),

=== removed file 'src/provisioningserver/dhcp/tests/test_dhcp_probe_service.py'
--- src/provisioningserver/dhcp/tests/test_dhcp_probe_service.py	2014-08-26 15:45:21 +0000
+++ src/provisioningserver/dhcp/tests/test_dhcp_probe_service.py	1970-01-01 00:00:00 +0000
@@ -1,128 +0,0 @@
-# Copyright 2014 Canonical Ltd.  This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""Tests for periodic DHCP prober."""
-
-from __future__ import (
-    absolute_import,
-    print_function,
-    unicode_literals,
-    )
-
-str = None
-
-__metaclass__ = type
-__all__ = []
-
-import os
-
-from apiclient.testing.credentials import make_api_credentials
-from maastesting.factory import factory
-from maastesting.matchers import (
-    get_mock_calls,
-    HasLength,
-    MockCalledOnceWith,
-    MockNotCalled,
-    )
-from mock import ANY
-from provisioningserver import cache
-from provisioningserver.auth import (
-    NODEGROUP_UUID_CACHE_KEY,
-    record_api_credentials,
-    )
-from provisioningserver.dhcp import dhcp_probe_service
-from provisioningserver.dhcp.dhcp_probe_service import (
-    PeriodicDHCPProbeService,
-    )
-from provisioningserver.testing.testcase import PservTestCase
-from testtools.deferredruntest import AsynchronousDeferredRunTest
-from twisted.internet import defer
-from twisted.internet.task import Clock
-
-
-class TestDHCPProbeService(PservTestCase):
-
-    run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=5)
-
-    def setUp(self):
-        super(TestDHCPProbeService, self).setUp()
-        self.cluster_uuid = factory.make_UUID()
-        maas_url = 'http://%s.example.com/%s/' % (
-            factory.make_name('host'),
-            factory.make_string(),
-            )
-        api_credentials = make_api_credentials()
-
-        cache.cache.set(NODEGROUP_UUID_CACHE_KEY, self.cluster_uuid)
-        os.environ["MAAS_URL"] = maas_url
-        cache.cache.set('api_credentials', ':'.join(api_credentials))
-
-        self.knowledge = dict(
-            api_credentials=api_credentials,
-            maas_url=maas_url,
-            nodegroup_uuid=self.cluster_uuid)
-
-    def test_is_called_every_interval(self):
-        clock = Clock()
-        service = PeriodicDHCPProbeService(clock, self.cluster_uuid)
-
-        # Avoid actually probing
-        _probe_dhcp = self.patch(service, '_probe_dhcp')
-
-        # Until the service has started, periodic_probe_dhcp() won't
-        # be called.
-        self.assertThat(_probe_dhcp, MockNotCalled())
-
-        # The first call is issued at startup.
-        service.startService()
-        self.assertThat(_probe_dhcp, MockCalledOnceWith(ANY))
-
-        # Wind clock forward one second less than the desired interval.
-        clock.advance(service.check_interval - 1)
-
-        # No more periodic calls made.
-        self.assertEqual(1, len(get_mock_calls(_probe_dhcp)))
-
-        # Wind clock forward one second, past the interval.
-        clock.advance(1)
-
-        # Now there were two calls.
-        self.assertThat(get_mock_calls(_probe_dhcp), HasLength(2))
-
-    def test_download_is_initiated_in_new_thread(self):
-        clock = Clock()
-
-        # We could patch out 'periodic_probe_task' instead here but this
-        # is better because:
-        # 1. The former requires spinning the reactor again before being
-        #    able to test the result.
-        # 2. This way there's no thread to clean up after the test.
-        deferToThread = self.patch(dhcp_probe_service, 'deferToThread')
-        deferToThread.return_value = defer.succeed(None)
-        service = PeriodicDHCPProbeService(clock, self.cluster_uuid)
-        service.startService()
-        self.assertThat(
-            deferToThread, MockCalledOnceWith(
-                dhcp_probe_service.detect.periodic_probe_task,
-                self.knowledge))
-
-    def test_no_probe_if_api_credentials_not_set(self):
-        record_api_credentials(None)
-        clock = Clock()
-        service = PeriodicDHCPProbeService(clock, self.cluster_uuid)
-        _probe_dhcp = self.patch(service, '_probe_dhcp')
-        service.startService()
-        self.assertThat(_probe_dhcp, MockNotCalled())
-
-    def test_logs_errors(self):
-        clock = Clock()
-        maaslog = self.patch(dhcp_probe_service, 'maaslog')
-        service = PeriodicDHCPProbeService(clock, self.cluster_uuid)
-        error_message = factory.make_string()
-        self.patch(service, '_probe_dhcp').side_effect = Exception(
-            error_message)
-        service.startService()
-        self.assertThat(
-            maaslog.error, MockCalledOnceWith(
-                "Unable to probe for rogue DHCP servers: %s" %
-                error_message))

=== modified file 'src/provisioningserver/plugin.py'
--- src/provisioningserver/plugin.py	2014-08-26 17:23:16 +0000
+++ src/provisioningserver/plugin.py	2014-08-26 23:57:04 +0000
@@ -31,9 +31,6 @@
 import provisioningserver
 from provisioningserver.cluster_config import get_cluster_uuid
 from provisioningserver.config import Config
-from provisioningserver.dhcp.dhcp_probe_service import (
-    PeriodicDHCPProbeService,
-    )
 from provisioningserver.rpc.boot_images import PeriodicImageDownloadService
 from provisioningserver.rpc.clusterservice import ClusterClientService
 from provisioningserver.rpc.power import NodePowerMonitorService
@@ -242,12 +239,6 @@
         rpc_service.setName("rpc")
         return rpc_service
 
-    def _makePeriodicDHCPProbeService(self, rpc_service):
-        dhcp_probe_service = PeriodicDHCPProbeService(
-            reactor, get_cluster_uuid())
-        dhcp_probe_service.setName("dhcp_probe")
-        return dhcp_probe_service
-
     def makeService(self, options):
         """Construct a service."""
         services = provisioningserver.services
@@ -272,7 +263,4 @@
             rpc_service)
         image_download_service.setServiceParent(services)
 
-        dhcp_probe_service = self._makePeriodicDHCPProbeService(rpc_service)
-        dhcp_probe_service.setServiceParent(services)
-
         return services

=== modified file 'src/provisioningserver/tasks.py'
--- src/provisioningserver/tasks.py	2014-08-26 10:23:23 +0000
+++ src/provisioningserver/tasks.py	2014-08-26 23:57:04 +0000
@@ -43,7 +43,10 @@
     record_api_credentials,
     record_nodegroup_uuid,
     )
-from provisioningserver.dhcp import config
+from provisioningserver.dhcp import (
+    config,
+    detect,
+    )
 from provisioningserver.dhcp.control import (
     restart_dhcpv4,
     stop_dhcpv4,
@@ -445,6 +448,14 @@
         raise
 
 
+@task
+@log_task_events(level=logging.DEBUG)
+@log_exception_text
+def periodic_probe_dhcp():
+    """Probe for foreign DHCP servers."""
+    detect.periodic_probe_task()
+
+
 # =====================================================================
 # Boot images-related tasks
 # =====================================================================

=== modified file 'src/provisioningserver/tests/test_plugin.py'
--- src/provisioningserver/tests/test_plugin.py	2014-08-26 17:23:16 +0000
+++ src/provisioningserver/tests/test_plugin.py	2014-08-26 23:57:04 +0000
@@ -21,9 +21,6 @@
 from maastesting.testcase import MAASTestCase
 import provisioningserver
 from provisioningserver import plugin as plugin_module
-from provisioningserver.dhcp.dhcp_probe_service import (
-    PeriodicDHCPProbeService,
-    )
 from provisioningserver.plugin import (
     Options,
     ProvisioningRealm,
@@ -112,11 +109,9 @@
         service_maker = ProvisioningServiceMaker("Harry", "Hill")
         service = service_maker.makeService(options)
         self.assertIsInstance(service, MultiService)
-        expected_services = [
-            "dhcp_probe", "image_download", "log", "node_monitor",
-            "oops", "rpc", "tftp",
-            ]
-        self.assertItemsEqual(expected_services, service.namedServices)
+        self.assertSequenceEqual(
+            ["image_download", "log", "node_monitor", "oops", "rpc", "tftp"],
+            sorted(service.namedServices))
         self.assertEqual(
             len(service.namedServices), len(service.services),
             "Not all services are named.")
@@ -138,14 +133,6 @@
         node_monitor = service.getServiceNamed("node_monitor")
         self.assertIsInstance(node_monitor, NodePowerMonitorService)
 
-    def test_dhcp_probe_service(self):
-        options = Options()
-        options["config-file"] = self.write_config({})
-        service_maker = ProvisioningServiceMaker("Spike", "Milligan")
-        service = service_maker.makeService(options)
-        dhcp_probe = service.getServiceNamed("dhcp_probe")
-        self.assertIsInstance(dhcp_probe, PeriodicDHCPProbeService)
-
     def test_tftp_service(self):
         # A TFTP service is configured and added to the top-level service.
         config = {