Merge lp:~allenap/maas/tftp-rebind-on-netif-change into lp:~maas-committers/maas/trunk

[Gavin Panella]

Rebind TFTP services to new interfaces when pserv received SIGHUP.

Previously the TFTP services would only be bound to those interfaces configured at start time.

[Julian Edwards (julian-edwards)]

Are we using threads anywhere in pserv (other than spawning one after the HUP)? Signals don't play nice with threads but it may be moot if Twisted is DTRT.

Anyway, lovely change.

review: Approve

[Gavin Panella (allenap)]

> Are we using threads anywhere in pserv (other than spawning one after the
> HUP)? Signals don't play nice with threads but it may be moot if Twisted is
> DTRT.

Nope, don't think we're using threads. However, I've been cautious;
reactor.callFromThread will DTRT whether the handler runs in the reactor
thread or outside.

>
> Anyway, lovely change.

Thanks for the review!

[Raphaël Badin (rvb)]

Looks good although I'm not familiar enough with twisted to understand 100% of it. At always when making important changes to a core piece of the infrastructure, I recommend building a package from this branch and testing it before landing that whole lot.

[0]

188 +class TestTFTPService(MAASTestCase):
189 +
190 + def test_tftp_service(self):

This test is a bit painful to digest: it checks a ton of stuff. Could you think of a way to split it into more focused tests?

review: Approve

[Gavin Panella (allenap)]

> Looks good although I'm not familiar enough with twisted to understand 100% of
> it.  At always when making important changes to a core piece of the
> infrastructure, I recommend building a package from this branch and testing it
> before landing that whole lot.

This is not something I'm in the habit of doing, but I ought to.

>
> [0]
>
> 188     +class TestTFTPService(MAASTestCase):
> 189     +
> 190     +    def test_tftp_service(self):
>
> This test is a bit painful to digest: it checks a ton of stuff.  Could you
> think of a way to split it into more focused tests?

Breaking it up would mean a lot of repetition, getting it to the right
state. The liberal use of matchers will, I hope, make any failures
easier to understand.

Thanks for the re-review!

[Julian Edwards (julian-edwards)]

On Thursday 13 Feb 2014 14:50:55 you wrote:
> > Looks good although I'm not familiar enough with twisted to understand
> > 100% of it. At always when making important changes to a core piece of
> > the infrastructure, I recommend building a package from this branch and
> > testing it before landing that whole lot.
>
> This is not something I'm in the habit of doing, but I ought to.

FTR it's a total piece of cake to do this.

Get the packaging branch and at its top level dir type:

bzr builddeb

And that's it :) The packages will be placed in ../build-area/

You may need to edit a couple of things:
1. Set the revno in the top debian/changelog version if it's not new enough
2. For speed's sake, edit debian/rules and find the reference to lp:maas in
the get-orig-source rule and change it to use your local trunk branch.

[Julian Edwards (julian-edwards)]

+ # TimerService ensures that a call is made to it's target

its*

=== modified file 'services/pserv/run'
--- services/pserv/run	2013-08-30 09:20:25 +0000
+++ services/pserv/run	2014-02-13 12:29:56 +0000
@@ -21,6 +21,7 @@
 # Obtain the development setting for CLUSTER_UUID.
 . etc/demo_maas_cluster.conf
 export CLUSTER_UUID
+export MAAS_URL
 
 exec $(command -v authbind && echo --deep) \
     "${script}" --nodaemon --pidfile="" maas-pserv --config-file "${config}"

=== modified file 'src/provisioningserver/plugin.py'
--- src/provisioningserver/plugin.py	2014-02-10 17:38:08 +0000
+++ src/provisioningserver/plugin.py	2014-02-13 12:29:56 +0000
@@ -21,10 +21,7 @@
     LogService,
     OOPSService,
     )
-from provisioningserver.tftp import TFTPBackend
-from provisioningserver.utils import get_all_interface_addresses
-from tftp.protocol import TFTP
-from twisted.application import internet
+from provisioningserver.tftp import TFTPService
 from twisted.application.internet import (
     TCPClient,
     TCPServer,
@@ -154,18 +151,9 @@
 
     def _makeTFTPService(self, tftp_config):
         """Create the dynamic TFTP service."""
-        backend = TFTPBackend(tftp_config["root"], tftp_config["generator"])
-        # Create a UDP server individually for each discovered network
-        # interface, so that we can detect the interface via which we have
-        # received a datagram.
-        tftp_services = MultiService()
-        tftp_services.setName("tftp")
-        for address in get_all_interface_addresses():
-            tftp_service = internet.UDPServer(
-                tftp_config["port"], TFTP(backend), interface=address)
-            tftp_service.setName(address)
-            tftp_service.setServiceParent(tftp_services)
-        return tftp_services
+        tftp_service = TFTPService(**tftp_config)
+        tftp_service.setName("tftp")
+        return tftp_service
 
     def _makeRPCService(self, rpc_config):
         rpc_service = ClusterClientService(reactor)

=== modified file 'src/provisioningserver/tests/test_plugin.py'
--- src/provisioningserver/tests/test_plugin.py	2014-01-31 16:14:52 +0000
+++ src/provisioningserver/tests/test_plugin.py	2014-02-13 12:29:56 +0000
@@ -19,29 +19,29 @@
 
 from maastesting.factory import factory
 from maastesting.testcase import MAASTestCase
-from provisioningserver import plugin
 from provisioningserver.plugin import (
     Options,
     ProvisioningRealm,
     ProvisioningServiceMaker,
     SingleUsernamePasswordChecker,
     )
-from provisioningserver.tftp import TFTPBackend
+from provisioningserver.tftp import (
+    TFTPBackend,
+    TFTPService,
+    )
 from testtools.deferredruntest import (
     assert_fails_with,
     AsynchronousDeferredRunTest,
     )
 from testtools.matchers import (
     AfterPreprocessing,
-    AllMatch,
     Equals,
     IsInstance,
     MatchesAll,
     MatchesException,
+    MatchesStructure,
     Raises,
     )
-from tftp.protocol import TFTP
-from twisted.application.internet import UDPServer
 from twisted.application.service import MultiService
 from twisted.cred.credentials import UsernamePassword
 from twisted.cred.error import UnauthorizedLogin
@@ -128,13 +128,6 @@
 
     def test_tftp_service(self):
         # A TFTP service is configured and added to the top-level service.
-        interfaces = [
-            factory.getRandomIPAddress(),
-            factory.getRandomIPAddress(),
-            ]
-        self.patch(
-            plugin, "get_all_interface_addresses",
-            lambda: interfaces)
         config = {
             "tftp": {
                 "generator": "http://candlemass/solitude",
@@ -146,14 +139,9 @@
         options["config-file"] = self.write_config(config)
         service_maker = ProvisioningServiceMaker("Harry", "Hill")
         service = service_maker.makeService(options)
-        tftp_services = service.getServiceNamed("tftp")
-        # The "tftp" service is a multi-service containing UDP servers for
-        # each interface defined by get_all_interface_addresses().
-        self.assertIsInstance(tftp_services, MultiService)
-        services = [
-            tftp_services.getServiceNamed(interface)
-            for interface in interfaces
-            ]
+        tftp_service = service.getServiceNamed("tftp")
+        self.assertIsInstance(tftp_service, TFTPService)
+
         expected_backend = MatchesAll(
             IsInstance(TFTPBackend),
             AfterPreprocessing(
@@ -162,27 +150,12 @@
             AfterPreprocessing(
                 lambda backend: backend.generator_url.geturl(),
                 Equals(config["tftp"]["generator"])))
-        expected_protocol = MatchesAll(
-            IsInstance(TFTP),
-            AfterPreprocessing(
-                lambda protocol: protocol.backend,
-                expected_backend))
-        expected_service = MatchesAll(
-            IsInstance(UDPServer),
-            AfterPreprocessing(
-                lambda service: len(service.args),
-                Equals(2)),
-            AfterPreprocessing(
-                lambda service: service.args[0],  # port
-                Equals(config["tftp"]["port"])),
-            AfterPreprocessing(
-                lambda service: service.args[1],  # protocol
-                expected_protocol))
-        self.assertThat(services, AllMatch(expected_service))
-        # Only the interface used for each service differs.
-        self.assertEqual(
-            [svc.kwargs for svc in services],
-            [{"interface": interface} for interface in interfaces])
+
+        self.assertThat(
+            tftp_service, MatchesStructure(
+                backend=expected_backend,
+                port=Equals(config["tftp"]["port"]),
+            ))
 
 
 class TestSingleUsernamePasswordChecker(MAASTestCase):

=== modified file 'src/provisioningserver/tests/test_tftp.py'
--- src/provisioningserver/tests/test_tftp.py	2013-10-15 11:35:10 +0000
+++ src/provisioningserver/tests/test_tftp.py	2014-02-13 12:29:56 +0000
@@ -31,9 +31,22 @@
 from provisioningserver.tftp import (
     BytesReader,
     TFTPBackend,
+    TFTPService,
     )
 from testtools.deferredruntest import AsynchronousDeferredRunTest
+from testtools.matchers import (
+    AfterPreprocessing,
+    AllMatch,
+    Equals,
+    IsInstance,
+    MatchesAll,
+    MatchesStructure,
+    )
 from tftp.backend import IReader
+from tftp.protocol import TFTP
+from twisted.application import internet
+from twisted.application.service import MultiService
+from twisted.internet import reactor
 from twisted.internet.defer import (
     inlineCallbacks,
     succeed,
@@ -288,3 +301,97 @@
         self.assertEqual(fake_render_result.encode("utf-8"), output)
         backend.render_pxe_config.assert_called_once_with(
             kernel_params=fake_kernel_params, **fake_params)
+
+
+class TestTFTPService(MAASTestCase):
+
+    def test_tftp_service(self):
+        # A TFTP service is configured and added to the top-level service.
+        interfaces = [
+            factory.getRandomIPAddress(),
+            factory.getRandomIPAddress(),
+            ]
+        self.patch(
+            tftp_module, "get_all_interface_addresses",
+            lambda: interfaces)
+        example_root = self.make_dir()
+        example_generator = "http://example.com/generator"
+        example_port = factory.getRandomPort()
+        tftp_service = TFTPService(
+            root=example_root, generator=example_generator,
+            port=example_port)
+        tftp_service.updateServers()
+        # The "tftp" service is a multi-service containing UDP servers for
+        # each interface defined by get_all_interface_addresses().
+        self.assertIsInstance(tftp_service, MultiService)
+        # There's also a TimerService that updates the servers every 45s.
+        self.assertThat(
+            tftp_service.refresher, MatchesStructure.byEquality(
+                step=45, parent=tftp_service, name="refresher",
+                call=(tftp_service.updateServers, (), {}),
+            ))
+        expected_backend = MatchesAll(
+            IsInstance(TFTPBackend),
+            AfterPreprocessing(
+                lambda backend: backend.base.path,
+                Equals(example_root)),
+            AfterPreprocessing(
+                lambda backend: backend.generator_url.geturl(),
+                Equals(example_generator)))
+        expected_protocol = MatchesAll(
+            IsInstance(TFTP),
+            AfterPreprocessing(
+                lambda protocol: protocol.backend,
+                expected_backend))
+        expected_server = MatchesAll(
+            IsInstance(internet.UDPServer),
+            AfterPreprocessing(
+                lambda service: len(service.args),
+                Equals(2)),
+            AfterPreprocessing(
+                lambda service: service.args[0],  # port
+                Equals(example_port)),
+            AfterPreprocessing(
+                lambda service: service.args[1],  # protocol
+                expected_protocol))
+        self.assertThat(
+            tftp_service.getServers(),
+            AllMatch(expected_server))
+        # Only the interface used for each service differs.
+        self.assertItemsEqual(
+            [svc.kwargs for svc in tftp_service.getServers()],
+            [{"interface": interface} for interface in interfaces])
+
+    def test_tftp_service_rebinds_on_HUP(self):
+        # Initial set of interfaces to bind to.
+        interfaces = {"1.1.1.1", "2.2.2.2"}
+        self.patch(
+            tftp_module, "get_all_interface_addresses",
+            lambda: interfaces)
+
+        tftp_service = TFTPService(
+            root=self.make_dir(), generator="http://mighty/wind",
+            port=factory.getRandomPort())
+        tftp_service.updateServers()
+
+        # The child services of tftp_services are named after the
+        # interface they bind to.
+        self.assertEqual(interfaces, {
+            server.name for server in tftp_service.getServers()
+        })
+
+        # Update the set of interfaces to bind to.
+        interfaces.add("3.3.3.3")
+        interfaces.remove("1.1.1.1")
+
+        # Ask the TFTP service to update its set of servers.
+        tftp_service.updateServers()
+
+        # We're in the reactor thread but we want to move the reactor
+        # forwards, hence we need to get all explicit about it.
+        reactor.runUntilCurrent()
+
+        # The interfaces now bound match the updated interfaces set.
+        self.assertEqual(interfaces, {
+            server.name for server in tftp_service.getServers()
+        })

=== modified file 'src/provisioningserver/tftp.py'
--- src/provisioningserver/tftp.py	2013-10-15 11:35:10 +0000
+++ src/provisioningserver/tftp.py	2014-02-13 12:29:56 +0000
@@ -14,6 +14,7 @@
 __metaclass__ = type
 __all__ = [
     "TFTPBackend",
+    "TFTPService",
     ]
 
 import httplib
@@ -31,12 +32,18 @@
 from provisioningserver.enum import ARP_HTYPE
 from provisioningserver.kernel_opts import KernelParameters
 from provisioningserver.pxe.config import render_pxe_config
-from provisioningserver.utils import deferred
+from provisioningserver.utils import (
+    deferred,
+    get_all_interface_addresses,
+    )
 from tftp.backend import (
     FilesystemSynchronousBackend,
     IReader,
     )
 from tftp.errors import FileNotFound
+from tftp.protocol import TFTP
+from twisted.application import internet
+from twisted.application.service import MultiService
 from twisted.python.context import get
 from twisted.web.client import getPage
 import twisted.web.error
@@ -223,3 +230,72 @@
             d = self.get_config_reader(params)
             d.addErrback(self.get_page_errback, file_name)
             return d
+
+
+class TFTPService(MultiService, object):
+    """An umbrella service representing a set of running TFTP servers.
+
+    Creates a UDP server individually for each discovered network
+    interface, so that we can detect the interface via which we have
+    received a datagram.
+
+    It then periodically updates the servers running in case there's a
+    change to the host machine's network configuration.
+
+    :ivar backend: The :class:`TFTPBackend` being used to service TFTP
+        requests.
+
+    :ivar port: The port on which each server is started.
+
+    :ivar refresher: A :class:`TimerService` that calls
+        ``updateServers`` periodically.
+
+    """
+
+    def __init__(self, root, port, generator):
+        """
+        :param root: The root directory for this TFTP server.
+        :param port: The port on which each server should be started.
+        :param generator: The URL to be queried for PXE configuration.
+        """
+        super(TFTPService, self).__init__()
+        self.backend, self.port = TFTPBackend(root, generator), port
+        # Establish a periodic call to self.updateServers() every 45
+        # seconds, so that this service eventually converges on truth.
+        # TimerService ensures that a call is made to it's target
+        # function immediately as it's started, so there's no need to
+        # call updateServers() from here.
+        self.refresher = internet.TimerService(45, self.updateServers)
+        self.refresher.setName("refresher")
+        self.refresher.setServiceParent(self)
+
+    def getServers(self):
+        """Return a set of all configured servers.
+
+        :rtype: :class:`set` of :class:`internet.UDPServer`
+        """
+        return {
+            service for service in self
+            if service is not self.refresher
+        }
+
+    def updateServers(self):
+        """Run a server on every interface.
+
+        For each configured network interface this will start a TFTP
+        server. If called later it will bring up servers on newly
+        configured interfaces and bring down servers on deconfigured
+        interfaces.
+        """
+        addrs_established = set(service.name for service in self.getServers())
+        addrs_desired = set(get_all_interface_addresses())
+
+        for address in addrs_desired - addrs_established:
+            tftp_service = internet.UDPServer(
+                self.port, TFTP(self.backend), interface=address)
+            tftp_service.setName(address)
+            tftp_service.setServiceParent(self)
+
+        for address in addrs_established - addrs_desired:
+            tftp_service = self.getServiceNamed(address)
+            tftp_service.disownServiceParent()