Merge ~andrey-fedoseev/launchpad-buildd:backend-open into launchpad-buildd:master

[Andrey Fedoseev]

Use `Backend.open` to modify files in target environments

instead of using a local temporary file and `copy_out` / `copy_in` methods

A new method `open` is added to the base `Backend` class

The method access to the files in the target environment via a file-like object.

Under the hood it uses a temporary file on the host system and `copy_in` / `copy_out` methods to transfer the file to / from the target environment.

[Colin Watson (cjwatson)]

Nice simplification, thanks!

review: Approve

[Andrey Fedoseev (andrey-fedoseev)]

Colin,

Please see my comments below

[Colin Watson (cjwatson)]

All your replies make sense. Go ahead (except for adding the copyright/licensing comment to `lpbuildd/target/tests/test_backend.py`). Thanks!

diff --git a/debian/changelog b/debian/changelog
index 0ce11f2..fba13f2 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,9 +1,14 @@
 launchpad-buildd (224) UNRELEASED; urgency=medium
 
+  [ Colin Watson ]
   * Allow configuring builders to use a different ClamAV database URL.
   * Require the LXD snap to be installed, rather than depending on the lxd
     package (which no longer exists in jammy).
 
+  [ Andrey Fedoseev ]
+  * `open` method is added to the backends providing access to the files
+    in target environments via a file-like object.
+
  -- Colin Watson <[e-mail]>  Wed, 26 Oct 2022 08:55:39 +0200
 
 launchpad-buildd (223) focal; urgency=medium
diff --git a/lpbuildd/binarypackage.py b/lpbuildd/binarypackage.py
index c3fef09..3498e58 100644
--- a/lpbuildd/binarypackage.py
+++ b/lpbuildd/binarypackage.py
@@ -156,11 +156,11 @@ class BinaryPackageBuildManager(DebianBuildManager):
                self.archive_purpose))
         if self.build_debug_symbols:
             currently_building_contents += 'Build-Debug-Symbols: yes\n'
-        with tempfile.NamedTemporaryFile(mode='w+') as currently_building:
+        with self.backend.open(
+            '/CurrentlyBuilding', mode='w+'
+        ) as currently_building:
             currently_building.write(currently_building_contents)
-            currently_building.flush()
             os.fchmod(currently_building.fileno(), 0o644)
-            self.backend.copy_in(currently_building.name, '/CurrentlyBuilding')
 
         args = ["sbuild-package", self._buildid, self.arch_tag]
         args.append(self.suite)
diff --git a/lpbuildd/ci.py b/lpbuildd/ci.py
index 04aa5f8..601d8b4 100644
--- a/lpbuildd/ci.py
+++ b/lpbuildd/ci.py
@@ -6,7 +6,6 @@ from configparser import (
     NoSectionError,
     )
 import os
-import tempfile
 import yaml
 
 from twisted.internet import defer
@@ -168,14 +167,10 @@ class CIBuildManager(BuildManagerProxyMixin, DebianBuildManager):
                     ["--plugin-setting", f"{key}={value}"])
         if self.secrets is not None:
             text = yaml.dump(self.secrets)
-            with tempfile.NamedTemporaryFile(mode="w") as f:
+            with self.backend.open(
+                "/build/.launchpad-secrets.yaml", mode="w"
+            ) as f:
                 f.write(text)
-                f.flush()
-                path_to_secrets = f.name
-                self.backend.copy_in(
-                    source_path=path_to_secrets,
-                    target_path="/build/.launchpad-secrets.yaml"
-                )
             args.extend(
                 ["--secrets", "/build/.launchpad-secrets.yaml"])
         if self.scan_malware:
diff --git a/lpbuildd/target/apt.py b/lpbuildd/target/apt.py
index ebd4683..a411806 100644
--- a/lpbuildd/target/apt.py
+++ b/lpbuildd/target/apt.py
@@ -5,7 +5,6 @@ import logging
 import os
 import subprocess
 import sys
-import tempfile
 from textwrap import dedent
 import time
 
@@ -30,49 +29,44 @@ class OverrideSourcesList(Operation):
 
     def run(self):
         logger.info("Overriding sources.list in build-%s", self.args.build_id)
-        with tempfile.NamedTemporaryFile(mode="w+") as sources_list:
+        with self.backend.open(
+            "/etc/apt/sources.list", mode="w+"
+        ) as sources_list:
             for archive in self.args.archives:
                 print(archive, file=sources_list)
-            sources_list.flush()
             os.fchmod(sources_list.fileno(), 0o644)
-            self.backend.copy_in(sources_list.name, "/etc/apt/sources.list")
-        with tempfile.NamedTemporaryFile(mode="w+") as apt_retries_conf:
+        with self.backend.open(
+            "/etc/apt/apt.conf.d/99retries", mode="w+"
+        ) as apt_retries_conf:
             print('Acquire::Retries "3";', file=apt_retries_conf)
-            apt_retries_conf.flush()
             os.fchmod(apt_retries_conf.fileno(), 0o644)
-            self.backend.copy_in(
-                apt_retries_conf.name, "/etc/apt/apt.conf.d/99retries")
         # Versions of APT that support phased updates do this automatically
         # if running in a chroot, but builds may be running in a LXD
         # container instead.
-        with tempfile.NamedTemporaryFile(mode="w+") as apt_phasing_conf:
+        with self.backend.open(
+            "/etc/apt/apt.conf.d/99phasing", mode="w+"
+        ) as apt_phasing_conf:
             print('APT::Get::Always-Include-Phased-Updates "true";',
                   file=apt_phasing_conf)
-            apt_phasing_conf.flush()
             os.fchmod(apt_phasing_conf.fileno(), 0o644)
-            self.backend.copy_in(
-                apt_phasing_conf.name, "/etc/apt/apt.conf.d/99phasing")
         if self.args.apt_proxy_url is not None:
-            with tempfile.NamedTemporaryFile(mode="w+") as apt_proxy_conf:
+            with self.backend.open(
+                "/etc/apt/apt.conf.d/99proxy", mode="w+"
+            ) as apt_proxy_conf:
                 print(
                     f'Acquire::http::Proxy "{self.args.apt_proxy_url}";',
                     file=apt_proxy_conf)
-                apt_proxy_conf.flush()
                 os.fchmod(apt_proxy_conf.fileno(), 0o644)
-                self.backend.copy_in(
-                    apt_proxy_conf.name, "/etc/apt/apt.conf.d/99proxy")
         for pocket in ("proposed", "backports"):
-            with tempfile.NamedTemporaryFile(mode="w+") as preferences:
+            with self.backend.open(
+                f"/etc/apt/preferences.d/{pocket}.pref", mode="w+"
+            ) as preferences:
                 print(dedent(f"""\
                     Package: *
                     Pin: release a=*-{pocket}
                     Pin-Priority: 500
                     """), file=preferences, end="")
-                preferences.flush()
                 os.fchmod(preferences.fileno(), 0o644)
-                self.backend.copy_in(
-                    preferences.name,
-                    f"/etc/apt/preferences.d/{pocket}.pref")
         return 0
 
 
@@ -91,7 +85,9 @@ class AddTrustedKeys(Operation):
         gpg_cmd = [
             "gpg", "--ignore-time-conflict", "--no-options", "--no-keyring",
             ]
-        with tempfile.NamedTemporaryFile(mode="wb+") as keyring:
+        with self.backend.open(
+            "/etc/apt/trusted.gpg.d/launchpad-buildd.gpg", mode="wb+"
+        ) as keyring:
             subprocess.check_call(
                 gpg_cmd + ["--dearmor"], stdin=self.input_file, stdout=keyring)
             keyring.seek(0)
@@ -100,8 +96,6 @@ class AddTrustedKeys(Operation):
                 ["--show-keys", "--keyid-format", "long", "--fingerprint"],
                 stdin=keyring, stdout=self.show_keys_file)
             os.fchmod(keyring.fileno(), 0o644)
-            self.backend.copy_in(
-                keyring.name, "/etc/apt/trusted.gpg.d/launchpad-buildd.gpg")
         return 0
 
 
diff --git a/lpbuildd/target/backend.py b/lpbuildd/target/backend.py
index 69746bb..a920cf1 100644
--- a/lpbuildd/target/backend.py
+++ b/lpbuildd/target/backend.py
@@ -3,6 +3,10 @@
 
 import os.path
 import subprocess
+import tempfile
+from contextlib import contextmanager
+from pathlib import Path
+from shutil import rmtree
 
 
 class BackendException(Exception):
@@ -189,6 +193,27 @@ class Backend:
         """Remove the backend."""
         subprocess.check_call(["sudo", "rm", "-rf", self.build_path])
 
+    @contextmanager
+    def open(self, path: str, mode="r", **kwargs):
+        """
+        Provides access to the files in the target environment via a
+        file-like object.
+
+        The arguments are the same as those of the built-in `open` function.
+        """
+        tmp_dir = tempfile.mkdtemp()
+        tmp_path = os.path.join(tmp_dir, Path(path).name)
+        if self.path_exists(path):
+            self.copy_out(path, tmp_path)
+        tmp_file = open(tmp_path, mode=mode, **kwargs)
+        try:
+            yield tmp_file
+        finally:
+            tmp_file.close()
+            if mode not in ("r", "rb", "rt"):
+                self.copy_in(tmp_path, path)
+            rmtree(tmp_dir)
+
 
 def make_backend(name, build_id, series=None, arch=None):
     if name == "chroot":
diff --git a/lpbuildd/target/build_oci.py b/lpbuildd/target/build_oci.py
index 5241f9f..ecbda0d 100644
--- a/lpbuildd/target/build_oci.py
+++ b/lpbuildd/target/build_oci.py
@@ -3,7 +3,6 @@
 
 import logging
 import os.path
-import tempfile
 from textwrap import dedent
 
 from lpbuildd.target.backend import check_path_escape
@@ -56,10 +55,8 @@ class BuildOCI(BuilderProxyOperationMixin, VCSOperationMixin,
                 Environment="{setting.upper()}={self.args.proxy_url}"
                 """)
             file_path = f"/etc/systemd/system/docker.service.d/{setting}.conf"
-            with tempfile.NamedTemporaryFile(mode="w+") as systemd_file:
+            with self.backend.open(file_path, mode="w+") as systemd_file:
                 systemd_file.write(contents)
-                systemd_file.flush()
-                self.backend.copy_in(systemd_file.name, file_path)
 
     def install(self):
         logger.info("Running install phase...")
diff --git a/lpbuildd/target/build_snap.py b/lpbuildd/target/build_snap.py
index 30c5ecc..615c2d9 100644
--- a/lpbuildd/target/build_snap.py
+++ b/lpbuildd/target/build_snap.py
@@ -5,7 +5,6 @@ import argparse
 import json
 import logging
 import os.path
-import tempfile
 from textwrap import dedent
 from urllib.parse import urlparse
 
@@ -92,13 +91,12 @@ class BuildSnap(BuilderProxyOperationMixin, VCSOperationMixin,
             svn_servers += f"http-proxy-username = {proxy.username}\n"
         if proxy.password:
             svn_servers += f"http-proxy-password = {proxy.password}\n"
-        with tempfile.NamedTemporaryFile(mode="w+") as svn_servers_file:
+        self.backend.run(["mkdir", "-p", "/root/.subversion"])
+        with self.backend.open(
+            "/root/.subversion/servers", mode="w+"
+        ) as svn_servers_file:
             svn_servers_file.write(svn_servers)
-            svn_servers_file.flush()
             os.fchmod(svn_servers_file.fileno(), 0o644)
-            self.backend.run(["mkdir", "-p", "/root/.subversion"])
-            self.backend.copy_in(
-                svn_servers_file.name, "/root/.subversion/servers")
 
     def install(self):
         logger.info("Running install phase...")
diff --git a/lpbuildd/target/lxd.py b/lpbuildd/target/lxd.py
index d58d9ab..63f36d5 100644
--- a/lpbuildd/target/lxd.py
+++ b/lpbuildd/target/lxd.py
@@ -9,7 +9,6 @@ import re
 import stat
 import subprocess
 import tarfile
-import tempfile
 from textwrap import dedent
 import time
 
@@ -376,22 +375,16 @@ class LXD(Backend):
             ["hostname"], universal_newlines=True).rstrip("\n")
         fqdn = subprocess.check_output(
             ["hostname", "--fqdn"], universal_newlines=True).rstrip("\n")
-        with tempfile.NamedTemporaryFile(mode="w+") as hosts_file:
-            try:
-                self.copy_out("/etc/hosts", hosts_file.name)
-            except LXDException:
-                hosts_file.seek(0, os.SEEK_SET)
-                hosts_file.write(fallback_hosts)
+        with self.open("/etc/hosts", mode="a") as hosts_file:
             hosts_file.seek(0, os.SEEK_END)
+            if not hosts_file.tell():
+                # /etc/hosts is missing or empty
+                hosts_file.write(fallback_hosts)
             print(f"\n127.0.1.1\t{fqdn} {hostname}", file=hosts_file)
-            hosts_file.flush()
             os.fchmod(hosts_file.fileno(), 0o644)
-            self.copy_in(hosts_file.name, "/etc/hosts")
-        with tempfile.NamedTemporaryFile(mode="w+") as hostname_file:
+        with self.open("/etc/hostname", mode="w+") as hostname_file:
             print(hostname, file=hostname_file)
-            hostname_file.flush()
             os.fchmod(hostname_file.fileno(), 0o644)
-            self.copy_in(hostname_file.name, "/etc/hostname")
 
         resolv_conf = "/etc/resolv.conf"
 
@@ -403,23 +396,17 @@ class LXD(Backend):
 
         self.copy_in(resolv_conf, "/etc/resolv.conf")
 
-        with tempfile.NamedTemporaryFile(mode="w+") as policy_rc_d_file:
+        with self.open(
+            "/usr/local/sbin/policy-rc.d", mode="w+"
+        ) as policy_rc_d_file:
             policy_rc_d_file.write(policy_rc_d)
-            policy_rc_d_file.flush()
             os.fchmod(policy_rc_d_file.fileno(), 0o755)
-            self.copy_in(policy_rc_d_file.name, "/usr/local/sbin/policy-rc.d")
         # For targets that use Upstart, prevent the mounted-dev job from
         # creating devices.  Most of the devices it creates are unnecessary
         # in a container, and creating loop devices will race with our own
         # code to do so.
-        with tempfile.NamedTemporaryFile(mode="w+") as mounted_dev_file:
-            try:
-                self.copy_out(
-                    "/etc/init/mounted-dev.conf", mounted_dev_file.name)
-            except LXDException:
-                pass
-            else:
-                mounted_dev_file.seek(0, os.SEEK_SET)
+        if self.path_exists("/etc/init/mounted-dev.conf"):
+            with self.open("/etc/init/mounted-dev.conf") as mounted_dev_file:
                 script = ""
                 in_script = False
                 for line in mounted_dev_file:
@@ -431,15 +418,13 @@ class LXD(Backend):
                     elif line.strip() == "script":
                         script += line
                         in_script = True
-                if script:
-                    mounted_dev_file.seek(0, os.SEEK_SET)
-                    mounted_dev_file.truncate()
-                    mounted_dev_file.write(script)
-                    mounted_dev_file.flush()
-                    os.fchmod(mounted_dev_file.fileno(), 0o644)
-                    self.copy_in(
-                        mounted_dev_file.name,
-                        "/etc/init/mounted-dev.override")
+
+            if script:
+                with self.open(
+                    "/etc/init/mounted-dev.override", mode="w"
+                ) as mounted_dev_override_file:
+                    mounted_dev_override_file.write(script)
+                    os.fchmod(mounted_dev_override_file.fileno(), 0o644)
 
         # Start the container and wait for it to start.
         container.start(wait=True)
@@ -481,16 +466,14 @@ class LXD(Backend):
         # directory until the container has started.  We can get away with
         # this for the time being because snapd isn't in the buildd chroots.
         self.run(["mkdir", "-p", "/etc/systemd/system/snapd.service.d"])
-        with tempfile.NamedTemporaryFile(mode="w+") as no_cdn_file:
+        with self.open(
+            "/etc/systemd/system/snapd.service.d/no-cdn.conf", mode="w+"
+        ) as no_cdn_file:
             print(dedent("""\
                 [Service]
                 Environment=SNAPPY_STORE_NO_CDN=1
                 """), file=no_cdn_file, end="")
-            no_cdn_file.flush()
             os.fchmod(no_cdn_file.fileno(), 0o644)
-            self.copy_in(
-                no_cdn_file.name,
-                "/etc/systemd/system/snapd.service.d/no-cdn.conf")
 
         # Refreshing snaps from a timer unit during a build isn't
         # appropriate.  Mask this, but manually so that we don't depend on
diff --git a/lpbuildd/target/run_ci.py b/lpbuildd/target/run_ci.py
index 4ebf765..28dc2e7 100644
--- a/lpbuildd/target/run_ci.py
+++ b/lpbuildd/target/run_ci.py
@@ -3,7 +3,6 @@
 
 import logging
 import os
-import tempfile
 
 from lpbuildd.target.build_snap import SnapChannelsAction
 from lpbuildd.target.operation import Operation
@@ -77,16 +76,12 @@ class RunCIPrepare(BuilderProxyOperationMixin, VCSOperationMixin,
             # services, which is convenient since it allows us to ensure
             # that ClamAV's database is up to date before proceeding.
             if self.args.clamav_database_url:
-                freshclam_path = "/etc/clamav/freshclam.conf"
-                with tempfile.NamedTemporaryFile(mode="w+") as freshclam_file:
-                    self.backend.copy_out(freshclam_path, freshclam_file.name)
-                    freshclam_file.seek(0, os.SEEK_END)
-                    print(
-                        f"PrivateMirror {self.args.clamav_database_url}",
-                        file=freshclam_file,
+                with self.backend.open(
+                    "/etc/clamav/freshclam.conf", mode="a"
+                ) as freshclam_file:
+                    freshclam_file.write(
+                        f"PrivateMirror {self.args.clamav_database_url}\n"
                     )
-                    freshclam_file.flush()
-                    self.backend.copy_in(freshclam_file.name, freshclam_path)
             kwargs = {}
             env = self.build_proxy_environment(proxy_url=self.args.proxy_url)
             if env:
diff --git a/lpbuildd/target/tests/test_backend.py b/lpbuildd/target/tests/test_backend.py
new file mode 100644
index 0000000..ff617e3
--- /dev/null
+++ b/lpbuildd/target/tests/test_backend.py
@@ -0,0 +1,39 @@
+# Copyright 2022 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+from unittest.mock import patch, ANY
+
+from testtools import TestCase
+from fixtures import TempDir
+
+from lpbuildd.tests.fakebuilder import UncontainedBackend
+
+
+class TestBackend(TestCase):
+
+    def test_open(self):
+        backend = UncontainedBackend("1")
+        backend_root = self.useFixture(TempDir())
+        target_path = backend_root.join("test.txt")
+
+        with patch.object(
+            backend, "copy_in", wraps=backend.copy_in
+        ) as copy_in, patch.object(
+            backend, "copy_out", wraps=backend.copy_out
+        ) as copy_out:
+
+            with backend.open(target_path, "w") as f:
+                f.write("text")
+
+            copy_out.assert_not_called()
+            copy_in.assert_called_once_with(ANY, target_path)
+
+            self.assertTrue(backend.path_exists(target_path))
+
+            copy_in.reset_mock()
+            copy_out.reset_mock()
+
+            with backend.open(target_path, "r") as f:
+                self.assertEqual(f.read(), "text")
+
+            copy_in.assert_not_called()
+            copy_out.assert_called_once_with(target_path, ANY)
diff --git a/lpbuildd/target/tests/test_lxd.py b/lpbuildd/target/tests/test_lxd.py
index ecf3ddd..73f4d94 100644
--- a/lpbuildd/target/tests/test_lxd.py
+++ b/lpbuildd/target/tests/test_lxd.py
@@ -391,9 +391,10 @@ class TestLXD(TestCase):
             lambda wait=False: setattr(container, "status_code", LXD_RUNNING))
         files_api = container.api.files
         files_api._api_endpoint = f"/1.0/containers/lp-xenial-{arch}/files"
-        files_api.session.get.side_effect = FakeSessionGet({
+        existing_files = {
             "/etc/hosts": [b"127.0.0.1\tlocalhost\n"],
-            })
+        }
+        files_api.session.get.side_effect = FakeSessionGet(existing_files)
         processes_fixture = self.useFixture(FakeProcesses())
 
         def fake_sudo(args):
@@ -414,7 +415,13 @@ class TestLXD(TestCase):
         processes_fixture.add(lambda _: {}, name="lxc")
         processes_fixture.add(
             FakeHostname("example", "example.buildd"), name="hostname")
-        LXD("1", "xenial", arch).start()
+
+        with mock.patch.object(
+            LXD,
+            "path_exists",
+            side_effect=lambda path: path in existing_files
+        ):
+            LXD("1", "xenial", arch).start()
 
         self.assert_correct_profile()
 
@@ -515,9 +522,6 @@ class TestLXD(TestCase):
             params={"path": "/usr/local/sbin/policy-rc.d"},
             data=policy_rc_d.encode("UTF-8"),
             headers={"X-LXD-uid": "0", "X-LXD-gid": "0", "X-LXD-mode": "0755"})
-        files_api.session.get.assert_any_call(
-            f"/1.0/containers/lp-xenial-{arch}/files",
-            params={"path": "/etc/init/mounted-dev.conf"}, stream=True)
         self.assertNotIn(
             "/etc/init/mounted-dev.override",
             [kwargs["params"]["path"]
@@ -551,11 +555,10 @@ class TestLXD(TestCase):
         processes_fixture.add(lambda _: {}, name="lxc")
         processes_fixture.add(
             FakeHostname("example", "example.buildd"), name="hostname")
-        LXD("1", "xenial", "amd64").start()
 
-        files_api.session.get.assert_any_call(
-            "/1.0/containers/lp-xenial-amd64/files",
-            params={"path": "/etc/hosts"}, stream=True)
+        with mock.patch.object(LXD, "path_exists", return_value=False):
+            LXD("1", "xenial", "amd64").start()
+
         files_api.post.assert_any_call(
             params={"path": "/etc/hosts"},
             data=(
@@ -576,7 +579,7 @@ class TestLXD(TestCase):
             lambda wait=False: setattr(container, "status_code", LXD_RUNNING))
         files_api = container.api.files
         files_api._api_endpoint = "/1.0/containers/lp-trusty-amd64/files"
-        files_api.session.get.side_effect = FakeSessionGet({
+        existing_files = {
             "/etc/init/mounted-dev.conf": [dedent("""\
                 start on mounted MOUNTPOINT=/dev
                 script
@@ -584,11 +587,18 @@ class TestLXD(TestCase):
                     /sbin/MAKEDEV std fd ppp tun
                 end script
                 task
-                """).encode("UTF-8")]})
+                """).encode("UTF-8")]}
+        files_api.session.get.side_effect = FakeSessionGet(existing_files)
         processes_fixture = self.useFixture(FakeProcesses())
         processes_fixture.add(lambda _: {}, name="sudo")
         processes_fixture.add(lambda _: {}, name="lxc")
-        LXD("1", "trusty", "amd64").start()
+
+        with mock.patch.object(
+            LXD,
+            "path_exists",
+            side_effect=lambda path: path in existing_files
+        ):
+            LXD("1", "trusty", "amd64").start()
 
         files_api.session.get.assert_any_call(
             "/1.0/containers/lp-trusty-amd64/files",