Merge lp:~allenap/maas/tftp-offload into lp:~maas-committers/maas/trunk

[Gavin Panella]

Implement *experimental* TFTP offload support.

In my own time I've been writing a TFTP server in Rust. This small change to MAAS allows it to take up TFTP duties for MAAS. I will publish the server's code imminently. Though it's far from finished, the performance, predictably, already far outstrips MAAS's in-process TFTP server. This will be important when putting MAAS on more constrained hardware, and may improve the experience for all: that's what this experiment is intended to discover.

The change in the proposal is as self-contained as possible. The offload support won't even be imported unless it's explicitly requested, and it will be easy to remove this support at a later date. It is not documented in any publicly visible place so we're not making any promises about supporting this.

Once this change has landed and in the PPA I will publish some instructions for getting it working with the external offload process.

[Mike Pontillo (mpontillo)]

This looks nice, and very valuable since we know the Python TFTP code causes memory leaks in MAAS long-term (on the order of months?). I'm tempted to just say land it, since it's effectively guarded from accidental use.

However, I'll say 'Needs Information', because I think this should be documented (in docs/) before it lands. (Preferably with a link to your rust-based reference implementation showing how to use it.)

My other worry is, given that this is an experimental project, it will be prone to bit rot unless we have CI tests for it. Should we commit to ensuring it is properly tested before we land it in trunk? (Maybe just keep it as a private branch until then.)

Also, it might be good if tftp_offload.py had comments with a reference to where the code is enabled in `plugin.py`, plus instructions on how to enable it.

A couple more comments below.

review: Needs Information

[Gavin Panella (allenap)]

Thanks Mike!

My expectation with this is that we'll either remove it before we release 2.2, or we'll rework it, add tests, and make it official. I think this is a worthwhile experiment but I do suspect the former (removal) will happen because this is a spike for which I want to get feedback but which is absolutely not production ready and is not intended to be. This is merely the easiest way to add support to MAAS for the experiment: the alternatives I considered were building packages myself in a PPA (too much work, too likely to break testers) or providing a patch to apply manually. I don't want to invest more time into documenting this than necessary, beyond providing instructions for how to get it working. Once this is cleared to land I will have the impetus to do that.

This is the server so far:
  https://github.com/allenap/allenap-tftp-offload
which builds on:
  https://github.com/allenap/allenap-libtftp

I'm not set on overriding tftp_port, though I think it's fairly harmless. Perhaps something like "does /etc/maas/experiment.tftp-offload exist" would be a reasonable alternative?

[Gavin Panella (allenap)]

I've written the instructions at https://github.com/allenap/allenap-tftp-offload.

[Mike Pontillo (mpontillo)]

Thanks. Looks good. See one comment below before landing, please.

review: Approve

[Gavin Panella (allenap)]

Thank you!

=== modified file 'src/provisioningserver/plugin.py'
--- src/provisioningserver/plugin.py	2016-10-19 17:06:30 +0000
+++ src/provisioningserver/plugin.py	2016-12-08 22:02:38 +0000
@@ -98,6 +98,24 @@
             resource_root=tftp_root, port=tftp_port,
             client_service=rpc_service)
         tftp_service.setName("tftp")
+
+        # *** EXPERIMENTAL ***
+        # https://code.launchpad.net/~allenap/maas/tftp-offload/+merge/312146
+        # If the TFTP port has been set to zero, use the experimental offload
+        # service. Otherwise stick to the normal in-process TFTP service.
+        if tftp_port == 0:
+            from provisioningserver.path import get_path
+            from provisioningserver.rackdservices import tftp_offload
+            from twisted.internet.endpoints import UNIXServerEndpoint
+            tftp_offload_socket = get_path("/var/lib/maas/tftp-offload.sock")
+            tftp_offload_endpoint = UNIXServerEndpoint(
+                reactor, tftp_offload_socket, wantPID=False)
+            tftp_offload_service = tftp_offload.TFTPOffloadService(
+                reactor, tftp_offload_endpoint, tftp_service.backend)
+            tftp_offload_service.setName("tftp-offload")
+            return tftp_offload_service
+        # *** /EXPERIMENTAL ***
+
         return tftp_service
 
     def _makeImageDownloadService(self, rpc_service, tftp_root):

=== added file 'src/provisioningserver/rackdservices/tftp_offload.py'
--- src/provisioningserver/rackdservices/tftp_offload.py	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/rackdservices/tftp_offload.py	2016-12-08 22:02:38 +0000
@@ -0,0 +1,207 @@
+# Copyright 2016 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""TFTP offload server.
+
+  ********************
+  **                **
+  **  EXPERIMENTAL  **
+  **                **
+  ********************
+
+"""
+
+__all__ = [
+    "TFTPOffloadService",
+]
+
+import os
+import shutil
+import tempfile
+
+from provisioningserver.logger import LegacyLogger
+from provisioningserver.utils.twisted import (
+    call,
+    callOut,
+)
+import tftp.backend
+from twisted.application.internet import StreamServerEndpointService
+from twisted.internet import (
+    interfaces,
+    protocol,
+)
+from twisted.internet.defer import (
+    inlineCallbacks,
+    maybeDeferred,
+)
+from twisted.python import context
+from twisted.python.filepath import FilePath
+from zope.interface import implementer
+
+
+log = LegacyLogger()
+
+
+class TFTPOffloadService(StreamServerEndpointService):
+    """Service for `TFTPOffloadProtocol` on a given endpoint."""
+
+    def __init__(self, reactor, endpoint, backend):
+        """
+        :param reactor: A Twisted reactor.
+        :param endpoint: A Twisted endpoint.
+        :param backend: An instance of `TFTPBackend`.
+        """
+        super(TFTPOffloadService, self).__init__(
+            endpoint, TFTPOffloadProtocolFactory(backend))
+
+
+@implementer(interfaces.IProtocolFactory)
+class TFTPOffloadProtocolFactory:
+    """Factory for `TFTPOffloadProtocol`.
+
+    This arranges a "hand-off" store (i.e. a temporary directory) that is used
+    by the protocol to give ephemeral files to the offload process, which is
+    then responsible for deleting them. This temporary directory will also be
+    removed during shutdown of this factory so belt-n-braces.
+    """
+
+    def __init__(self, backend):
+        super(TFTPOffloadProtocolFactory, self).__init__()
+        self.backend = backend
+
+    def buildProtocol(self, addr):
+        return TFTPOffloadProtocol(self.backend, self.store)
+
+    def doStart(self):
+        self.store = tempfile.mkdtemp(
+            prefix="maas.tftp.", suffix=".store")
+
+    def doStop(self):
+        store, self.store = self.store, None
+        shutil.rmtree(store)
+
+
+class TFTPOffloadProtocol(protocol.Protocol):
+    """A partially dynamic read-only TFTP **offload** protocol.
+
+    The protocol is simple. Where applicable and unless noted otherwise,
+    values are encoded as ASCII.
+
+    - On a new connection, write the following:
+
+      - The local IP address
+      - NULL (a single byte of value zero)
+      - The remote IP address
+      - NULL
+      - The requested file name
+      - NULL
+      - A literal dollar sign
+
+    - In response the following will be written:
+
+      - A hyphen to indicate success
+      - NULL
+      - Either a hyphen or "EPH" to indicate the type of response
+      - NULL
+      - The file name to serve (in file-system encoding)
+      - NULL
+      - A literal dollar sign
+
+      The offload process should then:
+
+      - Serve the file specified to its client
+      - Where "EPH" was specified, delete the file
+
+    - Or, in the case of failure:
+
+      - A decimal in ASCII denoting a TFTP error code
+      - NULL
+      - An error message encoded as UTF-8
+      - NULL
+      - A literal dollar sign
+
+      The offload process should then:
+
+      - Send an error packet to its client
+      - Terminate the transfer
+
+    """
+
+    def __init__(self, backend, store):
+        """
+        :param backend: An instance of `TFTPBackend`.
+        """
+        super(TFTPOffloadProtocol, self).__init__()
+        self.backend = backend
+        self.store = store
+
+    def connectionMade(self):
+        self.buf = b""
+
+    def dataReceived(self, data):
+        self.buf += data
+        parts = self.buf.split(b"\x00")
+        if len(parts) >= 4:
+            self.prepareResponse(*parts)
+
+    def prepareResponse(self, local, remote, file_name, over, *rest):
+        if over != b"$":
+            log.error(
+                "Message not properly terminated: local={local!r} "
+                "remote={remote!r} file_name={file_name!r} over={over!r} "
+                "rest={rest!r}", local=local, remote=remote,
+                file_name=file_name, over=over, rest=rest)
+            self.transport.loseConnection()
+        elif len(rest) != 0:
+            log.error(
+                "Message had trailing garbage: local={local!r} "
+                "remote={remote!r} file_name={file_name!r} over={over!r} "
+                "rest={rest!r}", local=local, remote=remote,
+                file_name=file_name, over=over, rest=rest)
+            self.transport.loseConnection()
+        else:
+            d = context.call(
+                {"local": (local.decode(), 0), "remote": (remote.decode(), 0)},
+                self.backend.get_reader, file_name)
+            d.addCallbacks(self.prepareWriteResponse, self.writeError)
+            d.addBoth(call, self.transport.loseConnection)
+            d.addErrback(log.err, "Failure in TFTP back-end.")
+
+    def prepareWriteResponse(self, reader):
+        if isinstance(reader, tftp.backend.FilesystemReader):
+            d = maybeDeferred(self.writeFileResponse, reader)
+        else:
+            d = maybeDeferred(self.writeStreamedResponse, reader)
+        return d.addBoth(callOut, reader.finish)
+
+    def writeFileResponse(self, reader):
+        return self.writeResponse(
+            reader.file_path, ephemeral=False)
+
+    def writeStreamedResponse(self, reader):
+        return self.copyReader(reader).addCallback(
+            self.writeResponse, ephemeral=True)
+
+    @inlineCallbacks
+    def copyReader(self, reader):
+        tempfd, tempname = tempfile.mkstemp(dir=self.store)
+        with os.fdopen(tempfd, "wb") as tempfd:
+            chunksize = 2 ** 16  # 64kiB
+            while True:
+                chunk = yield reader.read(chunksize)
+                tempfd.write(chunk)
+                if len(chunk) < chunksize:
+                    return FilePath(tempname)
+
+    def writeResponse(self, file_path, *, ephemeral):
+        self.transport.write(b"-\x00")  # Hyphen = okay.
+        self.transport.write(b"EPH\x00" if ephemeral else b"-\x00")
+        self.transport.write(file_path.asBytesMode().path)
+        self.transport.write(b"\x00$")  # Terminate. We're done.
+        self.transport.loseConnection()
+
+    def writeError(self, failure):
+        # Use TFTP error codes where possible.
+        self.transport.write(b"0\x00")  # 0 = See error message.
+        self.transport.write(failure.getErrorMessage().encode())
+        self.transport.write(b"\x00$")  # Terminate. We're done.

=== modified file 'src/provisioningserver/tests/test_plugin.py'
--- src/provisioningserver/tests/test_plugin.py	2016-10-19 21:08:54 +0000
+++ src/provisioningserver/tests/test_plugin.py	2016-12-08 22:02:38 +0000
@@ -44,13 +44,16 @@
     TFTPBackend,
     TFTPService,
 )
+from provisioningserver.rackdservices.tftp_offload import TFTPOffloadService
 from provisioningserver.testing.config import ClusterConfigurationFixture
 from testtools.matchers import (
     AfterPreprocessing,
+    Contains,
     Equals,
     IsInstance,
     MatchesAll,
     MatchesStructure,
+    Not,
 )
 from twisted.application.service import MultiService
 from twisted.python.filepath import FilePath
@@ -112,6 +115,22 @@
             logger.configure, MockCalledOnceWith(
                 options["verbosity"], logger.LoggingMode.TWISTD))
 
+    def test_makeService_with_EXPERIMENTAL_tftp_offload_service(self):
+        """
+        Only the site service is created when no options are given.
+        """
+        # Activate the offload service by setting port to 0.
+        self.useFixture(ClusterConfigurationFixture(tftp_port=0))
+
+        options = Options()
+        service_maker = ProvisioningServiceMaker("Harry", "Hill")
+        service = service_maker.makeService(options)
+        self.assertIsInstance(service, MultiService)
+        self.assertThat(service.namedServices, Not(Contains("tftp")))
+        self.assertThat(service.namedServices, Contains("tftp-offload"))
+        tftp_offload_service = service.getServiceNamed("tftp-offload")
+        self.assertThat(tftp_offload_service, IsInstance(TFTPOffloadService))
+
     def test_makeService_patches_tftp_service(self):
         mock_tftp_patch = (
             self.patch(plugin_module, 'add_patches_to_txtftp'))