Rework S3 CI cache prefix (HMS-5274)

test/scripts/build-image

@@ -46,8 +46,7 @@ def main():
 
     osbuild_ver, _ = testlib.runcmd(["osbuild", "--version"])
 
-    osrelease = testlib.read_osrelease()
-    distro_version = osrelease["ID"] + "-" + osrelease["VERSION_ID"]
+    distro_version = testlib.get_host_distro()
     osbuild_commit = testlib.get_osbuild_commit(distro_version)
     if osbuild_commit is None:
         osbuild_commit = "RELEASE"

test/scripts/generate-ostree-build-config

@@ -223,8 +223,8 @@ def setup_dependencies(manifests, config_map, distro, arch):
             ic_image_type = image_config["image-type"]
             dep_build_name = testlib.gen_build_name(ic_distro, ic_arch, dep_image_type, dep_config_name)
             manifest_id = manifests[dep_build_name + ".json"]["id"]
-            container_s3_prefix = testlib.gen_build_info_s3(distro, arch, manifest_id)
-            container_s3_path = f"{container_s3_prefix}container/container.tar"
+            container_s3_prefix = testlib.gen_build_info_s3_dir_path(distro, arch, manifest_id)
+            container_s3_path = os.path.join(container_s3_prefix, "container", "container.tar")
 
             # start each container once on an incremental port
             port = container_ports.get(container_s3_path)

test/scripts/imgtestlib.py

@@ -108,17 +108,11 @@ def list_images(distros=None, arches=None, images=None):
     return json.loads(out)
 
 
-def dl_build_info(destination, distro=None, arch=None):
+def dl_build_info(destination, distro=None, arch=None, osbuild_ref=None, runner_distro=None):
     """
     Downloads all the configs from the s3 bucket.
     """
-    s3url = f"{S3_BUCKET}/{S3_PREFIX}"
-    if distro and arch:
-        # only take them into account if both are defined
-        s3url = f"{s3url}/{distro}/{arch}"
-
-    s3url += "/"
-
+    s3url = gen_build_info_s3_dir_path(distro, arch, osbuild_ref=osbuild_ref, runner_distro=runner_distro)
     print(f"⬇️ Downloading configs from {s3url}")
     # only download info.json (exclude everything, then include) files, otherwise we get manifests and whole images
     job = sp.run(["aws", "s3", "sync",
@@ -153,31 +147,50 @@ def gen_build_name(distro, arch, image_type, config_name):
     return f"{_u(distro)}-{_u(arch)}-{_u(image_type)}-{_u(config_name)}"
 
 
-def gen_build_info_dir_path(root, osbuild_ver, manifest_id):
+def gen_build_info_dir_path_prefix(distro=None, arch=None, manifest_id=None, osbuild_ref=None, runner_distro=None):
     """
-    Generates the path to the directory that contains the build info.
-    This is a simple os.path.join() of the components, but ensures that paths are consistent.
-    """
-    return os.path.join(root, osbuild_ver, manifest_id, "")
+    Generates the relative path prefix for the location where build info and artifacts will be stored for a specific
+    build. This is a simple concatenation of the components, but ensures that paths are consistent. The caller is
+    responsible for prepending the location root to the generated path.
 
+    If no 'osbuild_ref' is specified, the value returned by get_osbuild_commit() for the 'runner_distro' will be used.
+    if no 'runner_distro' is specified, the value returned by get_host_distro() will be used.
 
-def gen_build_info_path(root, osbuild_ver, manifest_id):
-    """
-    Generates the path to the info.json.
-    This is a simple os.path.join() of the components, but ensures that paths are consistent.
+    A fully specified path is returned if all of the 'distro', 'arch' and 'manifest_id' parameters are specified,
+    otherwise a partial path is returned. Partial path may be useful for working with a superset of build infos.
+    For a more specific path to be generated when specifying any of the optional parameters, the caller must specify
+    all of the previous parameters. For example, if 'arch' is specified, 'distro' must also be specified for 'arch' to
+    be included in the path.
+
+    The returned path always has a trailing separator at the end to signal that it is a directory.
     """
-    return os.path.join(gen_build_info_dir_path(root, osbuild_ver, manifest_id), "info.json")
+    if runner_distro is None:
+        runner_distro = get_host_distro()
+    if osbuild_ref is None:
+        osbuild_ref = get_osbuild_commit(runner_distro)
+
+    path = os.path.join(f"osbuild-ref-{osbuild_ref}", f"runner-{runner_distro}")
+    for p in (distro, arch, f"manifest-id-{manifest_id}" if manifest_id else None):
+        if p is None:
+            return path + "/"
+        path = os.path.join(path, p)
+    return path + "/"
 
 
-def gen_build_info_s3(distro, arch, manifest_id):
+def gen_build_info_s3_dir_path(distro=None, arch=None, manifest_id=None, osbuild_ref=None, runner_distro=None):
     """
-    Generates the s3 URL for the location where build info and artifacts will be stored for a specific build
-    configuration.
-    This is a simple concatenation of the components, but ensures that paths are consistent.
+    Generates the s3 URL for the location where build info and artifacts will be stored for a specific
+    one or more builds, depending on the parameters specified.
+
+    A fully specified path is returned if all parameters are specified, otherwise a partial path is returned.
+    This function basically just prepends the S3_BUCKET and S3_PREFIX to the path generated by
+    gen_build_info_dir_path_prefix().
     """
-    osbuild_ver = get_osbuild_nevra()
-    build_info_prefix = f"{S3_BUCKET}/images/builds/{distro}/{arch}"
-    return gen_build_info_dir_path(build_info_prefix, osbuild_ver, manifest_id)
+    return os.path.join(
+        S3_BUCKET,
+        S3_PREFIX,
+        gen_build_info_dir_path_prefix(distro, arch, manifest_id, osbuild_ref, runner_distro),
+    )
 
 
 def check_config_names():
@@ -313,18 +326,17 @@ def filter_builds(manifests, distro=None, arch=None, skip_ostree_pull=True):
     Returns a list of build requests for the manifests that have no matching config in the test build cache.
     """
     print(f"⚙️ Filtering {len(manifests)} build configurations")
-    dl_path = os.path.join(TEST_CACHE_ROOT, "s3configs", f"builds/{distro}/{arch}/")
+    dl_root_path = os.path.join(TEST_CACHE_ROOT, "s3configs", "builds")
+    dl_path = os.path.join(dl_root_path, gen_build_info_dir_path_prefix(distro, arch))
     os.makedirs(dl_path, exist_ok=True)
     build_requests = []
 
-    out, dl_ok = dl_build_info(dl_path, distro=distro, arch=arch)
+    out, dl_ok = dl_build_info(dl_path, distro, arch)
     # continue even if the dl failed; will build all configs
     if dl_ok:
         # print output which includes list of downloaded files for CI job log
         print(out)
 
-    osbuild_ver = get_osbuild_nevra()
-
     errors: list[str] = []
     for manifest_fname, data in manifests.items():
         manifest_id = data["id"]
@@ -346,7 +358,10 @@ def filter_builds(manifests, distro=None, arch=None, skip_ostree_pull=True):
         build_request["manifest-checksum"] = manifest_id
 
         # check if the hash_fname exists in the synced directory
-        build_info_dir = gen_build_info_dir_path(dl_path, osbuild_ver, manifest_id)
+        build_info_dir = os.path.join(
+            dl_root_path,
+            gen_build_info_dir_path_prefix(distro, arch, manifest_id)
+        )
 
         if check_for_build(manifest_fname, build_info_dir, errors):
             build_requests.append(build_request)
@@ -364,8 +379,8 @@ def clargs():
     default_arch = os.uname().machine
     parser = argparse.ArgumentParser()
     parser.add_argument("config", type=str, help="path to write config")
-    parser.add_argument("--distro", type=str, default=None,
-                        help="distro to generate configs for (omit to generate for all distros)")
+    parser.add_argument("--distro", type=str, required=True,
+                        help="distro to generate configs for")
     parser.add_argument("--arch", type=str, default=default_arch,
                         help="architecture to generate configs for (defaults to host architecture)")
 
@@ -394,6 +409,15 @@ def read_osrelease():
     return osrelease
 
 
+def get_host_distro():
+    """
+    Get the host distro version based on data in the os-release file.
+    The format is <distro>-<version> (e.g. fedora-41).
+    """
+    osrelease = read_osrelease()
+    return f"{osrelease['ID']}-{osrelease['VERSION_ID']}"
+
+
 def get_osbuild_commit(distro_version):
     """
     Get the osbuild commit defined in the Schutzfile for the host distro.
@@ -416,14 +440,6 @@ def get_bib_ref():
     return data.get("common", {}).get("bootc-image-builder", {}).get("ref", None)
 
 
-def get_osbuild_nevra():
-    """
-    Returned the installed osbuild version. Exits with an error if osbuild is not installed.
-    """
-    out, _ = runcmd(["rpm", "-q", "--qf", "%{nevra}", "osbuild"])
-    return out.decode().strip()
-
-
 def rng_seed_env():
     """
     Read the rng seed from the Schutzfile and return it as a map to use as an environment variable with the appropriate

test/scripts/setup-osbuild-repo

@@ -50,9 +50,7 @@ def write_repo(commit, distro_version):
 
 
 def main():
-    osrelease = testlib.read_osrelease()
-
-    distro_version = osrelease["ID"] + "-" + osrelease["VERSION_ID"]
+    distro_version = testlib.get_host_distro()
     commit_id = testlib.get_osbuild_commit(distro_version)
     if not commit_id:
         print(f"Error: {distro_version} does not have the osbuild commit ID defined in the Schutzfile")

test/scripts/test_imgtestlib.py

@@ -1,6 +1,7 @@
 import os
 import subprocess as sp
 import tempfile
+from unittest.mock import patch
 
 import pytest
 
@@ -37,15 +38,172 @@ def test_read_seed():
     assert "OSBUILD_TESTING_RNG_SEED" in seed_env
 
 
-def test_path_generators():
-    testlib.get_osbuild_nevra = lambda: "osbuild-104-1.fc41.noarch"
-
-    assert testlib.gen_build_info_dir_path("inforoot", testlib.get_osbuild_nevra(), "abc123") == \
-        "inforoot/osbuild-104-1.fc41.noarch/abc123/"
-    assert testlib.gen_build_info_path("inforoot", testlib.get_osbuild_nevra(), "abc123") == \
-        "inforoot/osbuild-104-1.fc41.noarch/abc123/info.json"
-    assert testlib.gen_build_info_s3("fedora-41", "aarch64", "abc123") == \
-        testlib.S3_BUCKET + "/images/builds/fedora-41/aarch64/osbuild-104-1.fc41.noarch/abc123/"
+@pytest.mark.parametrize("kwargs,expected", (
+    (
+        {
+            "osbuild_ref": "abc123",
+            "runner_distro": "fedora-41",
+        },
+        "osbuild-ref-abc123/runner-fedora-41/"
+    ),
+    (
+        {
+            "osbuild_ref": "abc123",
+            "runner_distro": "fedora-41",
+            "distro": "fedora-41",
+        },
+        "osbuild-ref-abc123/runner-fedora-41/fedora-41/"
+    ),
+    (
+        {
+            "osbuild_ref": "abc123",
+            "runner_distro": "fedora-41",
+            "distro": "fedora-41",
+            "arch": "x86_64",
+        },
+        "osbuild-ref-abc123/runner-fedora-41/fedora-41/x86_64/"
+    ),
+    (
+        {
+            "osbuild_ref": "abc123",
+            "runner_distro": "fedora-41",
+            "distro": "fedora-41",
+            "arch": "x86_64",
+            "manifest_id": "abc123123",
+        },
+        "osbuild-ref-abc123/runner-fedora-41/fedora-41/x86_64/manifest-id-abc123123/"
+    ),
+    # Optional arg 'distro' not specified, thus following optional args 'arch' and 'manifest_id' are ignored
+    (
+        {
+            "osbuild_ref": "abc123",
+            "runner_distro": "fedora-41",
+            "arch": "x86_64",
+            "manifest_id": "abc123123"
+        },
+        "osbuild-ref-abc123/runner-fedora-41/"
+    ),
+    # Optional arg 'arch' not specified, thus following optional arg 'manifest_id' is ignored
+    (
+        {
+            "osbuild_ref": "abc123",
+            "runner_distro": "fedora-41",
+            "distro": "fedora-41",
+            "manifest_id": "abc123123"
+        },
+        "osbuild-ref-abc123/runner-fedora-41/fedora-41/"
+    ),
+    # default osbuild_ref
+    (
+        {
+            "runner_distro": "fedora-41",
+        },
+        "osbuild-ref-abcdef123456/runner-fedora-41/"
+    ),
+    # default runner_distro
+    (
+        {
+            "osbuild_ref": "abc123",
+        },
+        "osbuild-ref-abc123/runner-fedora-999/"
+    ),
+    # default osbuild_ref and runner_distro
+    (
+        {},
+        "osbuild-ref-abcdef123456/runner-fedora-999/"
+    ),
+))
+def test_gen_build_info_dir_path_prefix(kwargs, expected):
+    with patch("imgtestlib.get_host_distro", return_value="fedora-999"), \
+         patch("imgtestlib.get_osbuild_commit", return_value="abcdef123456"):
+        assert testlib.gen_build_info_dir_path_prefix(**kwargs) == expected
+
+
+@pytest.mark.parametrize("kwargs,expected", (
+    (
+        {
+            "osbuild_ref": "abcdef123456",
+            "runner_distro": "fedora-41",
+            "distro": "fedora-41",
+            "arch": "aarch64",
+            "manifest_id": "abc123"
+        },
+        testlib.S3_BUCKET + "/" + testlib.S3_PREFIX + \
+            "/osbuild-ref-abcdef123456/runner-fedora-41/fedora-41/aarch64/manifest-id-abc123/",
+    ),
+    (
+        {
+            "osbuild_ref": "abcdef123456",
+            "runner_distro": "fedora-41",
+            "distro": "fedora-41",
+            "arch": "aarch64",
+        },
+        testlib.S3_BUCKET + "/" + testlib.S3_PREFIX + \
+            "/osbuild-ref-abcdef123456/runner-fedora-41/fedora-41/aarch64/",
+    ),
+    (
+        {
+            "osbuild_ref": "abcdef123456",
+            "runner_distro": "fedora-41",
+            "distro": "fedora-41",
+        },
+        testlib.S3_BUCKET + "/" + testlib.S3_PREFIX + \
+            "/osbuild-ref-abcdef123456/runner-fedora-41/fedora-41/",
+    ),
+    (
+        {
+            "osbuild_ref": "abcdef123456",
+            "runner_distro": "fedora-41",
+        },
+        testlib.S3_BUCKET + "/" + testlib.S3_PREFIX + \
+            "/osbuild-ref-abcdef123456/runner-fedora-41/",
+    ),
+    # Optional arg 'distro' not specified, thus following optional args 'arch' and 'manifest_id' are ignored
+    (
+        {
+            "osbuild_ref": "abcdef123456",
+            "runner_distro": "fedora-41",
+            "arch": "aarch64",
+            "manifest_id": "abc123"
+        },
+        testlib.S3_BUCKET + "/" + testlib.S3_PREFIX + \
+            "/osbuild-ref-abcdef123456/runner-fedora-41/",
+    ),
+    # Optional arg 'arch' not specified, thus following optional arg 'manifest_id' is ignored
+    (
+        {
+            "osbuild_ref": "abcdef123456",
+            "runner_distro": "fedora-41",
+            "distro": "fedora-41",
+            "manifest_id": "abc123"
+        },
+        testlib.S3_BUCKET + "/" + testlib.S3_PREFIX + \
+            "/osbuild-ref-abcdef123456/runner-fedora-41/fedora-41/",
+    ),
+    # default osbuild_ref
+    (
+        {
+            "runner_distro": "fedora-41",
+        },
+        testlib.S3_BUCKET + "/" + testlib.S3_PREFIX + "/osbuild-ref-abcdef123456/runner-fedora-41/"
+    ),
+    # default runner_distro
+    (
+        {
+            "osbuild_ref": "abc123",
+        },
+        testlib.S3_BUCKET + "/" + testlib.S3_PREFIX + "/osbuild-ref-abc123/runner-fedora-999/"
+    ),
+    # default osbuild_ref and runner_distro
+    (
+        {},
+        testlib.S3_BUCKET + "/" + testlib.S3_PREFIX + "/osbuild-ref-abcdef123456/runner-fedora-999/"
+    ),
+))
+def test_gen_build_info_s3_dir_path(kwargs, expected):
+    with patch("imgtestlib.get_host_distro", return_value="fedora-999"), \
+         patch("imgtestlib.get_osbuild_commit", return_value="abcdef123456"):
+        assert testlib.gen_build_info_s3_dir_path(**kwargs) == expected
 
 
 test_container = "registry.gitlab.com/redhat/services/products/image-builder/ci/osbuild-composer/manifest-list-test"

test/scripts/upload-results

@@ -38,7 +38,7 @@ def main():
         build_info["pr"] = pr_number.removeprefix("PR-")
         testlib.write_build_info(build_dir, build_info)
 
-    s3url = testlib.gen_build_info_s3(distro, arch, manifest_id)
+    s3url = testlib.gen_build_info_s3_dir_path(distro, arch, manifest_id)
 
     print(f"⬆️ Uploading {build_dir} to {s3url}")
     testlib.runcmd_nc(["aws", "s3", "cp", "--no-progress", "--acl=private", "--recursive", build_dir+"/", s3url])