Merge ~cjwatson/launchpad:charm-ppa-publisher-archives-dir into launchpad:master

[Colin Watson]

charm: Set archives_dir for PPA publisher

The PPA publisher mostly doesn't use this, but it's still used to construct `lp.archivepublisher.config.Config.temproot`, and we need to make sure that that's on the same filesystem as the published archive, otherwise `os.rename` will fail.

[Ines Almeida (ines-almeida)]

By "mostly doesn't use this" does that mean that the directory itself won' be used? Would it make sense to provide an existing directory instead of creating a new empty one?

[Colin Watson (cjwatson)]

The place to look here is `lib/lp/archivepublisher/config.py`. It's unreasonably complex for historical reasons, but if you boil it down, it _mostly_ uses directories under `ppa_config.root` or `ppa_config.private_root` in the PPA case - all the key directories such as `distsroot` and `poolroot` end up under there. However, `temproot` ends up under a path derived from the `PublisherConfig` table.

Up to now, this mostly hasn't mattered. It's only temporary, as the name implies: files get dropped in there briefly until they're renamed into place in wherever they're actually supposed to live.

Until very recently, Ubuntu's `PublisherConfig.root_dir` in production was "/srv/launchpad.net/ubuntu-archive/", and hence `temproot` was "/srv/launchpad.net/ubuntu-archive/ubuntu-temp". This apparently existed on the PPA publisher and was on the same filesystem as `ppa_config.{root,private_root}`, so all was well.

However, because one of my early design decisions for our charms was to use "/srv/launchpad/" for everything rather than having things like "/srv/launchpad.net/", "/srv/staging.launchpad.net/", etc. depending on the application and environment, having "/srv/launchpad.net/" hardcoded in the database was problematic, especially for migration; we ran into this when deploying the qastaging ftpmaster. I changed Launchpad to automatically prepend `config.archivepublisher.archives_dir` to `PublisherConfig.root_dir`, set that to appropriate values in various places, and changed the database to drop the "/srv/launchpad.net/" prefix. But this caused the production outage that you can see in https://chat.canonical.com/canonical/pl/gfs13ina87rfzc9xrx1ncuf8zh, in particular:

  OSError: [Errno 18] Invalid cross-device link: '/var/tmp/archives/ubuntu-archive/ubuntu-temp/temp-download.3z8mk9bd' -> '/srv/launchpad.net/ppa-archive/ubuntu-elisp/ppa/ubuntu/pool/main/e/emacs-snapshot/emacs-snapshot_107161-dd818ed821f~ubuntu20.04.1.tar.gz'

I fixed the legacy PPA publisher deployment in https://code.launchpad.net/~cjwatson/lp-production-configs/ppa-archives-dir/+merge/450824, and I think we should do the same here. `temproot` needs to end up somewhere under `data_dir`, since we intend to mount that as a separate filesystem. The reason I went for creating a new subdirectory of `data_dir` was that I was a little concerned that if we're building the `temproot` path from a combination of `config.archivepublisher.archives_dir` and a database column then it might be possible to accidentally collide with something else; a subdirectory seemed like a simple way to avoid that problem without having to think about it too hard.

[Ines Almeida (ines-almeida)]

Thanks for the context and details!

review: Approve

diff --git a/charm/launchpad-ppa-publisher/reactive/launchpad-ppa-publisher.py b/charm/launchpad-ppa-publisher/reactive/launchpad-ppa-publisher.py
index e3c5b75..9118203 100644
--- a/charm/launchpad-ppa-publisher/reactive/launchpad-ppa-publisher.py
+++ b/charm/launchpad-ppa-publisher/reactive/launchpad-ppa-publisher.py
@@ -32,6 +32,12 @@ def get_data_dir():
     return os.path.join(base.base_dir(), "data")
 
 
+def archives_dir():
+    # Used by `lp.archivepublisher.config.getPubConfig` to construct
+    # `Config.temproot`, even for PPAs.
+    return os.path.join(get_data_dir(), "archives")
+
+
 def ppa_archive_root():
     return os.path.join(get_data_dir(), "ppa-archive")
 
@@ -53,6 +59,7 @@ def configure():
     ppa_keys_root = os.path.join(data_dir, "ppa-signing-keys")
     oval_data_root = os.path.join(data_dir, "oval-data")
 
+    config["archives_dir"] = archives_dir()
     config["ppa_archive_root"] = ppa_archive_root()
     config["ppa_archive_private_root"] = ppa_archive_private()
     config["ppa_signing_keys_root"] = ppa_keys_root
@@ -60,6 +67,9 @@ def configure():
 
     host.mkdir(data_dir, owner=base.user(), group=base.user(), perms=0o775)
     host.mkdir(
+        archives_dir(), owner=base.user(), group=base.user(), perms=0o775
+    )
+    host.mkdir(
         ppa_archive_root(), owner=base.user(), group=base.user(), perms=0o775
     )
     host.mkdir(
diff --git a/charm/launchpad-ppa-publisher/templates/launchpad-ppa-publisher-lazr.conf.j2 b/charm/launchpad-ppa-publisher/templates/launchpad-ppa-publisher-lazr.conf.j2
index 0a9e514..a0e08e8 100644
--- a/charm/launchpad-ppa-publisher/templates/launchpad-ppa-publisher-lazr.conf.j2
+++ b/charm/launchpad-ppa-publisher/templates/launchpad-ppa-publisher-lazr.conf.j2
@@ -12,6 +12,7 @@
 extends: ../launchpad-db-lazr.conf
 
 [archivepublisher]
+archives_dir: {{ archives_dir }}
 {{- opt("oval_data_rsync_endpoint", oval_data_rsync_endpoint) }}
 oval_data_root: {{ base_dir }}/oval-data