Repository: osbuild/osbuild
Pull request #1819 (2024-07-11 16:46:59)

sources(curl): use `--next` for each url in curl config
This is a clean version of https://github.com/osbuild/osbuild/pull/1812

Before this gets merged we should:
a) redo a speed test with new curl to ensure we have no regressions in the speed because of the next --nex
b) open the equivalent of https://github.com/osbuild/osbuild-composer/pull/4253 for this PR and ensure all composer tests are green (this is done in https://github.com/osbuild/osbuild-composer/pull/4281 as WIP)

bcl: Holding off pushing the big green button since I'm not sure if a speed test has been done. #4281 is passing though.
mvo5: I ran the speed test now in AWS in a micro instance with:
```
$ sudo bash -c "time python3 -m osbuild --libdir . --store $(mktemp -d) ./test/data/manifests/fedora-boot.json"
```

Regular main took for the download (because micro only two parallel curls are active):
```
$ git branch |grep '^*'
* (HEAD detached at upstream/main)
$ git describe
v125-6-gd1dbcc20
$ sudo bash -c "time python3 -m osbuild --libdir . --store $(mktemp -d) ./test/data/manifests/fedora-boot.json"
...
manifest ./test/data/manifests/fedora-boot.json finished successfully
build:    	32e87da44d9a519e89770723a33b7ecdd4ab85b872ae6ab8aaa94bdef9a275c7
os:       	3b4b32941a016e710b292b4735cde9172cccdc4e6edef442f7808b5a50a10311
image:    	ca2c3347b6a6b5c5c4032dd3175b3462e277262fa5a03dbda5a4fc3d43846fe5

real	1m56.591s
user	0m56.399s
sys	0m5.683s
```

Wen hacking it to 8 curls that are forked
```
$ git diff
diff --git a/sources/org.osbuild.curl b/sources/org.osbuild.curl
index 8071dcba..0c19b4c0 100755
--- a/sources/org.osbuild.curl
+++ b/sources/org.osbuild.curl
@@ -258,7 +258,7 @@ class CurlSource(sources.SourceService):
 
     content_type = "org.osbuild.files"
 
-    max_workers = 2 * (os.cpu_count() or 1)
+    max_workers = 8 * (os.cpu_count() or 1)
...
real	1m5.918s
user	0m58.312s
sys	0m5.419s
```

And for hacked with 32 forked ones:
```
$ git diff
diff --git a/sources/org.osbuild.curl b/sources/org.osbuild.curl
index 8071dcba..81b70c7c 100755
--- a/sources/org.osbuild.curl
+++ b/sources/org.osbuild.curl
@@ -258,7 +258,7 @@ class CurlSource(sources.SourceService):
 
     content_type = "org.osbuild.files"
 
-    max_workers = 2 * (os.cpu_count() or 1)
+    max_workers = 32 * (os.cpu_count() or 1)
 
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
....
real	1m9.256s
user	1m1.787s
sys	0m5.812s
```
(and even with hacked 128 forked curls I only get a real runtime of 1m11s)

And the new parallel branch.
```
$ git branch |grep '^*'
* use-curl-next-in-config
$ sudo bash -c "time python3 -m osbuild --libdir . --store $(mktemp -d) ./test/data/manifests/fedora-boot.json"
...
manifest ./test/data/manifests/fedora-boot.json finished successfully
build:    	32e87da44d9a519e89770723a33b7ecdd4ab85b872ae6ab8aaa94bdef9a275c7
os:       	3b4b32941a016e710b292b4735cde9172cccdc4e6edef442f7808b5a50a10311
image:    	ca2c3347b6a6b5c5c4032dd3175b3462e277262fa5a03dbda5a4fc3d43846fe5

real	0m9.065s
user	0m5.328s
sys	0m1.767s
```

So especially with really fast connections (like AWS) this PR really makes a difference.
mvo5: This will need https://github.com/osbuild/osbuild/pull/1868 and I will do a final test from osbuild-composer (like the one in https://github.com/osbuild/osbuild-composer/pull/4281) and then this can go finally in again I think :)

Patch:
diff --git a/devices/org.osbuild.lvm2.lv b/devices/org.osbuild.lvm2.lv
index c8b01432..ab4b447a 100755
--- a/devices/org.osbuild.lvm2.lv
+++ b/devices/org.osbuild.lvm2.lv
@@ -67,6 +67,15 @@ def get_parent_path(parent: str, options: Dict) -> str:
     return parent_path
 
 
+def escaped_lv_mapper_name(vg, lv: str) -> str:
+    """
+    Return the name of the mapper device under /dev/mapper/ for the given vg, lv
+    """
+    # see also lvm2:libdm/libdm-string.c:dm_build_dm_name() at
+    # https://github.com/lvmteam/lvm2/blob/v2_03_26/libdm/libdm-string.c#L325
+    return f'{vg.replace("-", "--")}-{lv.replace("-", "--")}'
+
+
 class LVService(devices.DeviceService):
 
     def __init__(self, args):
@@ -215,6 +224,11 @@ class LVService(devices.DeviceService):
 
         os.makedirs(os.path.join(devpath, vg), exist_ok=True)  # type: ignore
         self.ensure_device_node(fullpath, major, minor)
+        # also create the device under the mapper dir as this is what
+        # findmnt will need to get the uuid of the filesystem
+        os.makedirs(os.path.join(devpath, "mapper"), exist_ok=True)
+        mapper_path = os.path.join(devpath, "mapper", escaped_lv_mapper_name(vg, lv))
+        self.ensure_device_node(mapper_path, major, minor)
 
         data = {
             "path": devname,
diff --git a/sources/org.osbuild.curl b/sources/org.osbuild.curl
index 8071dcba..817d4f06 100755
--- a/sources/org.osbuild.curl
+++ b/sources/org.osbuild.curl
@@ -100,7 +100,9 @@ SCHEMA = """
 # We are not just using %{json} here because older curl (7.76) will
 # write {"http_connect":000} which python cannot parse so we write our
 # own json subset
-CURL_WRITE_OUT = r'\{\"url\": \"%{url}\"\, \"filename_effective\": \"%{filename_effective}\", \"exitcode\": %{exitcode}, \"errormsg\": \"%{errormsg}\" \}\n'
+CURL_WRITE_OUT_FMT = r'\{\"url\": \"%{url}\"\, \"filename_effective\": \"%{filename_effective}\", \"exitcode\": %{exitcode}, \"errormsg\": \"%{errormsg}\" \}\n'
+
+NR_RETRYS = 10
 
 
 def curl_has_parallel_downloads():
@@ -108,12 +110,6 @@ def curl_has_parallel_downloads():
     Return true if curl has all the support we needed for parallel downloading
     (this include --write-out "%{json}" too
     """
-    # Re-enable this once the failures in
-    # https://github.com/osbuild/osbuild-composer/pull/4247
-    # are understood
-    if os.getenv("OSBUILD_SOURCES_CURL_USE_PARALLEL") not in ["1", "yes", "true"]:
-        return False
-
     output = subprocess.check_output(["curl", "--version"], universal_newlines=True)
     first_line = output.split("\n", maxsplit=1)[0]
     m = re.match(r'^curl (\d+\.\d+\.\d+)', first_line)
@@ -139,31 +135,37 @@ def _quote_url(url: str) -> str:
 
 def gen_curl_download_config(config_path: pathlib.Path, chksum_desc_tuple: List[Tuple[str, Dict]], parallel=False):
     with open(config_path, "w", encoding="utf8") as fp:
-        # global options
-        if parallel:
-            fp.write("parallel\n")
-        fp.write(textwrap.dedent(f"""\
+        # Because we use --next which resets the parser state we need to set
+        # these options for each url.
+        per_url_opts = textwrap.dedent(f"""\
         user-agent = "osbuild (Linux.{platform.machine()}; https://osbuild.org/)"
         silent
         speed-limit = 1000
         connect-timeout = 30
         fail
         location
-        """))
+        """)
         if parallel:
-            fp.write(textwrap.dedent(f"""\
-            write-out = "{CURL_WRITE_OUT}"
-            """))
-
+            per_url_opts += textwrap.dedent(f"""\
+            write-out = "{CURL_WRITE_OUT_FMT}"
+            """)
         proxy = os.getenv("OSBUILD_SOURCES_CURL_PROXY")
         if proxy:
-            fp.write(f'proxy = "{proxy}"\n')
-        fp.write("\n")
-        # per url options
-        for checksum, desc in chksum_desc_tuple:
+            per_url_opts += f'proxy = "{proxy}"\n'
+        # start with the global option(s)
+        if parallel:
+            fp.write(textwrap.dedent("""\
+            # global options
+            parallel
+
+            """))
+        # then generate the per-url config
+        fp.write("# per-url options\n")
+        for i, (checksum, desc) in enumerate(chksum_desc_tuple):
             url = _quote_url(desc.get("url"))
             fp.write(f'url = "{url}"\n')
             fp.write(f'output = "{checksum}"\n')
+            fp.write(f'{per_url_opts}')
             secrets = desc.get("secrets")
             if secrets:
                 ssl_ca_cert = secrets.get('ssl_ca_cert')
@@ -180,7 +182,8 @@ def gen_curl_download_config(config_path: pathlib.Path, chksum_desc_tuple: List[
                 fp.write('insecure\n')
             else:
                 fp.write('no-insecure\n')
-            fp.write("\n")
+            if i + 1 < len(chksum_desc_tuple):
+                fp.write("next\n\n")
 
 
 def try_parse_curl_line(line):
@@ -220,6 +223,7 @@ def fetch_many_new_curl(tmpdir, targetdir, dl_pairs):
         curl_p = subprocess.Popen(curl_command, encoding="utf-8", cwd=tmpdir, stdout=subprocess.PIPE)
         # ensure that curl is killed even if an unexpected exit happens
         cm.callback(curl_p.kill)
+        errors = []
         while True:
             line = curl_p.stdout.readline()
             # empty line means eof/process finished
@@ -229,10 +233,13 @@ def fetch_many_new_curl(tmpdir, targetdir, dl_pairs):
             if not dl_details:
                 continue
             url = dl_details['url']
-            # ignore individual download errors, the overall exit status will
-            # reflect them and the caller can retry
+            # Keep track of individual errors as curl will only report
+            # the last download operation success/failure via the global
+            # exit code. There is "--fail-early" but the downside of that
+            # is that abort all in progress downloads too.
             if dl_details["exitcode"] != 0:
                 print(f"WARNING: failed to download {url}: {dl_details['errormsg']}", file=sys.stderr)
+                errors.append(f'{url}: error code {dl_details["exitcode"]}')
                 continue
             # the way downloads are setup the filename is the expected hash
             # so validate now and move into place
@@ -251,7 +258,10 @@ def fetch_many_new_curl(tmpdir, targetdir, dl_pairs):
             print(f"Downloaded {url}")
         # return overall download status (this will be an error if any
         # transfer failed)
-        return curl_p.wait()
+        curl_exit_code = curl_p.wait()
+        if not errors and curl_exit_code > 0:
+            errors.append("curl exited non-zero but reported no errors")
+        return errors
 
 
 class CurlSource(sources.SourceService):
@@ -310,13 +320,13 @@ class CurlSource(sources.SourceService):
             # some mirrors are sometimes broken. retry manually, because we could be
             # redirected to a different, working, one on retry.
             return_code = 0
-            for _ in range(10):
-                return_code = fetch_many_new_curl(tmpdir, self.cache, dl_pairs)
-                if return_code == 0:
+            for _ in range(NR_RETRYS):
+                errors = fetch_many_new_curl(tmpdir, self.cache, dl_pairs)
+                if not errors:
                     break
             else:
-                failed_urls = ",".join([itm[1]["url"] for itm in dl_pairs])
-                raise RuntimeError(f"curl: error downloading {failed_urls}: error code {return_code}")
+                details = ",".join(errors)
+                raise RuntimeError(f"curl: error downloading {details}")
 
         if len(dl_pairs) > 0:
             raise RuntimeError(f"curl: finished with return_code {return_code} but {dl_pairs} left to download")
@@ -334,7 +344,7 @@ class CurlSource(sources.SourceService):
             # some mirrors are sometimes broken. retry manually, because we could be
             # redirected to a different, working, one on retry.
             return_code = 0
-            for _ in range(10):
+            for _ in range(NR_RETRYS):
                 curl_config_path = f"{tmpdir}/curl-config.txt"
                 gen_curl_download_config(curl_config_path, [(checksum, desc)])
                 curl = subprocess.run(


Test patch:
diff --git a/devices/test/test_lv.py b/devices/test/test_lv.py
index 0e345d24..d460cf91 100644
--- a/devices/test/test_lv.py
+++ b/devices/test/test_lv.py
@@ -10,3 +10,8 @@ DEVICES_NAME = "org.osbuild.lvm2.lv"
 def test_lvm2_lv_get_parent_path(devices_module, parent, options, expected_parent_path):
     pp = devices_module.get_parent_path(parent, options)
     assert pp == expected_parent_path
+
+
+def test_lvm2_escaped_lv_mapper_name(devices_module):
+    expected = "1d2b2150--8de2--4b68--b387--f9bc709190e8-lvroot"
+    assert devices_module.escaped_lv_mapper_name("1d2b2150-8de2-4b68-b387-f9bc709190e8", "lvroot") == expected
diff --git a/sources/test/test_curl_source.py b/sources/test/test_curl_source.py
index 3a508e06..4527b9b5 100644
--- a/sources/test/test_curl_source.py
+++ b/sources/test/test_curl_source.py
@@ -1,6 +1,7 @@
 #!/usr/bin/python3
 
 import hashlib
+import pathlib
 import platform
 import re
 import shutil
@@ -10,7 +11,7 @@ from unittest.mock import patch
 import pytest
 
 import osbuild.testutil
-from osbuild.testutil.net import http_serve_directory
+from osbuild.testutil.net import http_serve_directory, https_serve_directory
 
 SOURCES_NAME = "org.osbuild.curl"
 
@@ -101,10 +102,11 @@ def test_curl_download_many_fail(curl_parallel):
     }
     with pytest.raises(RuntimeError) as exp:
         curl_parallel.fetch_all(TEST_SOURCES)
-    assert str(exp.value) == 'curl: error downloading http://localhost:9876/random-not-exists: error code 7'
+    assert str(exp.value).startswith("curl: error downloading http://localhost:9876/random-not-exists")
+    assert 'http://localhost:9876/random-not-exists: error code 7' in str(exp.value)
 
 
-def make_test_sources(fake_httpd_root, port, n_files):
+def make_test_sources(fake_httpd_root, port, n_files, start_n=0, cacert=""):
     """
     Create test sources for n_file. All files have the names
     0,1,2...
@@ -113,13 +115,20 @@ def make_test_sources(fake_httpd_root, port, n_files):
     Returns a sources dict that can be used as input for "fetch_all()" with
     the correct hash/urls.
     """
+    proto = "https" if cacert else "http"
+
     fake_httpd_root.mkdir(exist_ok=True)
     sources = {}
-    for i in range(n_files):
+    for i in range(start_n, start_n + n_files):
         name = f"{i}"
-        sources[f"sha256:{hashlib.sha256(name.encode()).hexdigest()}"] = {
-            "url": f"http://localhost:{port}/{name}",
+        key = f"sha256:{hashlib.sha256(name.encode()).hexdigest()}"
+        val = {
+            "url": f"{proto}://localhost:{port}/{name}",
         }
+        if cacert:
+            val["secrets"] = {}
+            val["secrets"]["ssl_ca_cert"] = cacert
+        sources[key] = val
         (fake_httpd_root / name).write_text(name, encoding="utf8")
 
     return sources
@@ -161,9 +170,7 @@ def test_curl_download_many_chksum_validate(tmp_path, curl_parallel):
 
 
 @pytest.mark.parametrize("curl_parallel", [True, False], indirect=["curl_parallel"])
-def test_curl_download_many_retries(tmp_path, monkeypatch, curl_parallel):
-    monkeypatch.setenv("OSBUILD_SOURCES_CURL_USE_PARALLEL", "1")
-
+def test_curl_download_many_retries(tmp_path, curl_parallel):
     fake_httpd_root = tmp_path / "fake-httpd-root"
 
     with http_serve_directory(fake_httpd_root) as httpd:
@@ -253,17 +260,16 @@ def test_curl_gen_download_config_old_curl(tmp_path, sources_module):
 
     assert config_path.exists()
     assert config_path.read_text(encoding="utf8") == textwrap.dedent(f"""\
+    # per-url options
+    url = "http://example.com/file/0"
+    output = "sha256:5feceb66ffc86f38d952786c6d696c79c2dbc239dd4e91b46729d73a27fb57e9"
     user-agent = "osbuild (Linux.{platform.machine()}; https://osbuild.org/)"
     silent
     speed-limit = 1000
     connect-timeout = 30
     fail
     location
-
-    url = "http://example.com/file/0"
-    output = "sha256:5feceb66ffc86f38d952786c6d696c79c2dbc239dd4e91b46729d73a27fb57e9"
     no-insecure
-
     """)
 
 
@@ -273,34 +279,59 @@ def test_curl_gen_download_config_parallel(tmp_path, sources_module):
 
     assert config_path.exists()
     assert config_path.read_text(encoding="utf8") == textwrap.dedent(f"""\
+    # global options
     parallel
+
+    # per-url options
+    url = "http://example.com/file/0"
+    output = "sha256:5feceb66ffc86f38d952786c6d696c79c2dbc239dd4e91b46729d73a27fb57e9"
     user-agent = "osbuild (Linux.{platform.machine()}; https://osbuild.org/)"
     silent
     speed-limit = 1000
     connect-timeout = 30
     fail
     location
-    write-out = "{sources_module.CURL_WRITE_OUT}"
-
-    url = "http://example.com/file/0"
-    output = "sha256:5feceb66ffc86f38d952786c6d696c79c2dbc239dd4e91b46729d73a27fb57e9"
+    write-out = "{sources_module.CURL_WRITE_OUT_FMT}"
     no-insecure
+    next
 
     url = "http://example.com/file/1"
     output = "sha256:6b86b273ff34fce19d6b804eff5a3f5747ada4eaa22f1d49c01e52ddb7875b4b"
+    user-agent = "osbuild (Linux.{platform.machine()}; https://osbuild.org/)"
+    silent
+    speed-limit = 1000
+    connect-timeout = 30
+    fail
+    location
+    write-out = "{sources_module.CURL_WRITE_OUT_FMT}"
     insecure
+    next
 
     url = "http://example.com/file/2"
     output = "sha256:d4735e3a265e16eee03f59718b9b5d03019c07d8b6c51f90da3a666eec13ab35"
+    user-agent = "osbuild (Linux.{platform.machine()}; https://osbuild.org/)"
+    silent
+    speed-limit = 1000
+    connect-timeout = 30
+    fail
+    location
+    write-out = "{sources_module.CURL_WRITE_OUT_FMT}"
     cacert = "some-ssl_ca_cert"
     no-insecure
+    next
 
     url = "http://example.com/file/3"
     output = "sha256:4e07408562bedb8b60ce05c1decfe3ad16b72230967de01f640b7e4729b49fce"
+    user-agent = "osbuild (Linux.{platform.machine()}; https://osbuild.org/)"
+    silent
+    speed-limit = 1000
+    connect-timeout = 30
+    fail
+    location
+    write-out = "{sources_module.CURL_WRITE_OUT_FMT}"
     cert = "some-ssl_client_cert"
     key = "some-ssl_client_key"
     no-insecure
-
     """)
 
 
@@ -322,14 +353,7 @@ Features: AsynchDNS IPv6 Largefile GSS-API Kerberos SPNEGO NTLM NTLM_WB SSL libz
 
 
 @patch("subprocess.check_output")
-def test_curl_has_parallel_download(mocked_check_output, monkeypatch, sources_module):
-    # by default, --parallel is disabled
-    mocked_check_output.return_value = NEW_CURL_OUTPUT
-    assert not sources_module.curl_has_parallel_downloads()
-
-    # unless this environemnt is set
-    monkeypatch.setenv("OSBUILD_SOURCES_CURL_USE_PARALLEL", "1")
-
+def test_curl_has_parallel_download(mocked_check_output, sources_module):
     mocked_check_output.return_value = NEW_CURL_OUTPUT
     assert sources_module.curl_has_parallel_downloads()
 
@@ -348,3 +372,32 @@ def test_curl_result_is_double_checked(tmp_path, curl_parallel):
         with pytest.raises(RuntimeError) as exp:
             curl_parallel.fetch_all(test_sources)
         assert re.match(r"curl: finished with return_code 0 but .* left to download", str(exp.value))
+
+
+@pytest.mark.parametrize("curl_parallel", [True, False], indirect=["curl_parallel"])
+def test_curl_download_many_mixed_certs(tmp_path, monkeypatch, sources_module, curl_parallel):
+    monkeypatch.setenv("OSBUILD_SOURCES_CURL_USE_PARALLEL", "1")
+    # ensure this does not accidentaly succeeds because we retry
+    monkeypatch.setattr(sources_module, "NR_RETRYS", 1)
+
+    fake_httpd_root = tmp_path / "fake-httpd-root"
+    git_root = pathlib.Path(__file__).parent.parent.parent
+    cert1_path = git_root / "test/data/certs/cert1.pem"
+    key1_path = git_root / "test/data/certs/key1.pem"
+    cert2_path = git_root / "test/data/certs/cert2.pem"
+    key2_path = git_root / "test/data/certs/key2.pem"
+
+    with https_serve_directory(fake_httpd_root, certfile=cert1_path, keyfile=key1_path) as httpds:
+        with https_serve_directory(fake_httpd_root, certfile=cert2_path, keyfile=key2_path) as httpds2:
+            test_sources = make_test_sources(
+                fake_httpd_root, httpds.server_port, 2, start_n=10, cacert=cert1_path)
+            test_sources.update(make_test_sources(
+                fake_httpd_root, httpds2.server_port, 2, cacert=cert2_path))
+            assert len(test_sources) == 4
+
+            curl_parallel.cache = tmp_path / "curl-download-dir"
+            curl_parallel.cache.mkdir()
+            curl_parallel.fetch_all(test_sources)
+
+            assert httpds.reqs.count == 2
+            assert httpds2.reqs.count == 2