launchpad-reviewers team mailing list archive

[Colin Watson <mp+450805@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad-layers:upload-queue-processor-fsroot-property into launchpad-layers:main.

Commit message:
upload-queue-processor: Turn fsroot into a property

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad-layers/+git/launchpad-layers/+merge/450805

The strategy of saving previous values in an instance attribute doesn't work, because charm hooks are run in separate processes and those values don't persist.  Turn `fsroot` into a property instead so that it's always retrieved from relation data when needed.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad-layers:upload-queue-processor-fsroot-property into launchpad-layers:main.

diff --git a/turnip-storage/lib/charms/turnip/storage.py b/turnip-storage/lib/charms/turnip/storage.py
index 6b3505d..6922525 100644
--- a/turnip-storage/lib/charms/turnip/storage.py
+++ b/turnip-storage/lib/charms/turnip/storage.py
@@ -1,7 +1,7 @@
 # Copyright 2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-
+import ipaddress
 import os
 import subprocess
 
@@ -26,6 +26,20 @@ def ensure_mounted():
         subprocess.check_call(["mountpoint", "-q", data_dir()])
 
 
+def make_mount_path(hostname, mountpoint):
+    """Turn a hostname (or IP address) and mount point into a mount path."""
+    literal_ipv6_address = False
+    try:
+        if ipaddress.ip_address(hostname).version == 6:
+            literal_ipv6_address = True
+    except ValueError:
+        pass
+    if literal_ipv6_address:
+        return f"[{hostname}]:{mountpoint}"
+    else:
+        return f"{hostname}:{mountpoint}"
+
+
 def mount_data(mount_info):
     # We use a systemd.mount(5) unit rather than a line in /etc/fstab partly
     # because it's easier to deal with a file we can completely overwrite,
@@ -35,6 +49,9 @@ def mount_data(mount_info):
     data_mount_conf = f"/lib/systemd/system/{data_mount}"
     context = dict(mount_info)
     context["data_dir"] = data_dir()
+    context["mount_path"] = make_mount_path(
+        mount_info["hostname"], mount_info["mountpoint"]
+    )
     templating.render("data.mount.j2", data_mount_conf, context, perms=0o644)
     host.service("unmask", data_mount)
     reload_systemd()
diff --git a/turnip-storage/templates/data.mount.j2 b/turnip-storage/templates/data.mount.j2
index b115460..77ad6eb 100644
--- a/turnip-storage/templates/data.mount.j2
+++ b/turnip-storage/templates/data.mount.j2
@@ -2,7 +2,7 @@
 Description=Turnip data file system
 
 [Mount]
-What={{ hostname }}:{{ mountpoint }}
+What={{ mount_path }}
 Where={{ data_dir }}
 Type={{ fstype }}
 Options={{ options }}
diff --git a/upload-queue-processor/requires.py b/upload-queue-processor/requires.py
index 3f3df36..34509e1 100644
--- a/upload-queue-processor/requires.py
+++ b/upload-queue-processor/requires.py
@@ -6,10 +6,6 @@ from charms.reactive import Endpoint, clear_flag, set_flag, when
 
 
 class UploadProcessorRequires(Endpoint):
-    def __init__(self, *args, **kwargs):
-        super().__init__(*args, **kwargs)
-        self.fsroot = None
-
     @when("endpoint.{endpoint_name}.joined")
     def handle_joined_unit(self):
         # We don't want to make it available before it's configured
@@ -31,11 +27,6 @@ class UploadProcessorRequires(Endpoint):
                 f"{len(self.relations)} instead."
             )
 
-        # Use `fsroot` values received from the queue processor
-        # If none were received, use the ones received previously
-        received_data = self.relations[0].units.received
-        self.fsroot = received_data.get("fsroot", self.fsroot)
-
         if self.fsroot:
             hookenv.log("Configuration from queue processor received")
             set_flag(self.expand_name("{endpoint_name}.configured"))
@@ -49,3 +40,11 @@ class UploadProcessorRequires(Endpoint):
         clear_flag(self.expand_name("{endpoint_name}.configured"))
         self.all_departed_units.clear()
         clear_flag(self.expand_name("departed"))
+
+    @property
+    def fsroot(self):
+        if self.relations:
+            received_data = self.relations[0].units.received
+            return received_data.get("fsroot")
+        else:
+            return None