launchpad-reviewers team mailing list archive

[Colin Watson <mp+450808@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/txpkgupload:fix-systemd-socket-activation into txpkgupload:master.

Commit message:
Fix systemd socket activation

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/txpkgupload/+git/txpkgupload/+merge/450808

We noticed that the application didn't start up in the new charmed deployment on qastaging.  I found that this was because socket activation wasn't set up quite right.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/txpkgupload:fix-systemd-socket-activation into txpkgupload:master.

diff --git a/charm/txpkgupload/charmcraft.yaml b/charm/txpkgupload/charmcraft.yaml
index 98f3499..6462c4e 100644
--- a/charm/txpkgupload/charmcraft.yaml
+++ b/charm/txpkgupload/charmcraft.yaml
@@ -35,7 +35,7 @@ parts:
     after:
       - ols-layers
     source: https://git.launchpad.net/launchpad-layers
-    source-commit: "300b0d9fd332c055395fe209512335cea03c7af3"
+    source-commit: "06169f9fa1548bd6abf9be39edf8b81c411595be"
     source-submodules: []
     source-type: git
     plugin: dump
diff --git a/charm/txpkgupload/templates/txpkgupload.service.j2 b/charm/txpkgupload/templates/txpkgupload.service.j2
index efd5b4e..ef44de4 100644
--- a/charm/txpkgupload/templates/txpkgupload.service.j2
+++ b/charm/txpkgupload/templates/txpkgupload.service.j2
@@ -1,6 +1,7 @@
 [Unit]
 Description=Launchpad FTP/SFTP package upload service
 After=network.target
+Requires=txpkgupload.socket
 ConditionPathExists=!{{ code_dir }}/maintenance.txt
 
 [Service]
diff --git a/charm/txpkgupload/templates/txpkgupload.socket.j2 b/charm/txpkgupload/templates/txpkgupload.socket.j2
index 2bdfdf9..435a710 100644
--- a/charm/txpkgupload/templates/txpkgupload.socket.j2
+++ b/charm/txpkgupload/templates/txpkgupload.socket.j2
@@ -1,9 +1,11 @@
 [Unit]
-Description=Twisted package uploader service
+Description=Twisted package uploader socket
 
 [Socket]
-ListenStream={{ sftp_port }}
+{# The order here must match the index= parameters in
+   txpkgupload_config.yaml.j2. -#}
 ListenStream={{ ftp_port }}
+ListenStream={{ sftp_port }}
 
 [Install]
 WantedBy=sockets.target
diff --git a/charm/txpkgupload/templates/txpkgupload_config.yaml.j2 b/charm/txpkgupload/templates/txpkgupload_config.yaml.j2
index 78b1edb..ea1368c 100644
--- a/charm/txpkgupload/templates/txpkgupload_config.yaml.j2
+++ b/charm/txpkgupload/templates/txpkgupload_config.yaml.j2
@@ -1,11 +1,11 @@
 ftp:
-  port: {{ ftp_port }}
+  port: "systemd:domain=INET6:index=0"
 
 sftp:
   authentication_endpoint: "{{ sftp_authentication_endpoint }}"
   host_key_private: "{{ sftp_host_key_private_path }}"
   host_key_public: "{{ sftp_host_key_public_path }}"
-  port: "tcp6:{{ sftp_port }}"
+  port: "systemd:domain=INET6:index=1"
 
 oops:
   directory: "{{ oopses_dir }}"
diff --git a/src/txpkgupload/plugin.py b/src/txpkgupload/plugin.py
index 9cf30ec..b78923c 100644
--- a/src/txpkgupload/plugin.py
+++ b/src/txpkgupload/plugin.py
@@ -72,8 +72,10 @@ class ConfigFtp(Schema):
 
     if_key_missing = None
 
-    # The port to run the FTP server on.
-    port = Int(if_missing=2121)
+    # The port to run the FTP server on.  This may be a plain integer (in
+    # which case it is prefixed with "tcp6:"), or it may be expressed in
+    # Twisted's "strports" mini-language.
+    port = String(if_missing="2121")
 
 
 class ConfigSftp(Schema):
diff --git a/src/txpkgupload/tests/test_plugin.py b/src/txpkgupload/tests/test_plugin.py
index 183de40..bb5ee29 100644
--- a/src/txpkgupload/tests/test_plugin.py
+++ b/src/txpkgupload/tests/test_plugin.py
@@ -108,7 +108,7 @@ class TestConfig(TestCase):
             "debug": False,
             "fsroot": None,
             "ftp": {
-                "port": 2121,
+                "port": "2121",
                 },
             "idle_timeout": 3600,
             "oops": {
@@ -148,17 +148,20 @@ class TestConfig(TestCase):
             "etc", "txpkgupload.yaml")
         Config.load(filename)
 
+    def test_load_ftp_integer(self):
+        observed = Config.parse("ftp:\n  port: 21")
+        self.assertEqual("21", observed["ftp"]["port"])
+
+    def test_load_ftp_strports(self):
+        observed = Config.parse("ftp:\n  port: tcp6:21")
+        self.assertEqual("tcp6:21", observed["ftp"]["port"])
+
     def check_exception(self, config, message):
         # Check that a UsageError is raised when parsing options.
         self.assertThat(
             partial(Config.to_python, config),
             Raises(MatchesException(Invalid, message)))
 
-    def test_option_ftp_port_integer(self):
-        self.check_exception(
-            {"ftp": {"port": "bob"}},
-            "ftp: port: Please enter an integer value")
-
     def test_option_idle_timeout_integer(self):
         self.check_exception(
             {"idle_timeout": "bob"},
diff --git a/src/txpkgupload/twistedftp.py b/src/txpkgupload/twistedftp.py
index 37deffb..995fa08 100644
--- a/src/txpkgupload/twistedftp.py
+++ b/src/txpkgupload/twistedftp.py
@@ -376,6 +376,9 @@ class FTPServiceFactory(service.Service):
 
     @staticmethod
     def makeFTPService(port, root, temp_dir, idle_timeout):
-        strport = "tcp6:%s" % port
+        if port.isdigit():
+            strport = "tcp6:%s" % port
+        else:
+            strport = port
         factory = FTPServiceFactory(port, root, temp_dir, idle_timeout)
         return strports.service(strport, factory.ftpfactory)