Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rework S3 CI cache prefix (HMS-5274) #1130

Merged
merged 15 commits into from
Jan 9, 2025
Merged

Rework S3 CI cache prefix (HMS-5274) #1130

merged 15 commits into from
Jan 9, 2025

Conversation

thozza
Copy link
Member

@thozza thozza commented Jan 7, 2025 •
edited by schutzbot
Loading

To replace the manifest-db in osbuild CI with the images repo image build cache, we will need to be able to easily construct the S3 cache prefix corresponding to a specific commit checkout of this repository.

The current S3 prefix for the image build cache looks like this:

<s3_bucket>/images/builds/<img_distro>/<img_arch>/<osbuild_nevra>/<img_manifest_id>/

e.g.

<s3_bucket>/images/builds/centos-9/x86_64/osbuild-136-1.20190819git9071cd0.fc40.noarch/ede3be49f4a6748fb65948aea9eccdf1a6114c2d270a05178f92c3494b3854e1/

The most problematic part of constructing the correct prefix without running scripts on the CI runner is the osbuild NEVRA. The specific NEVRA is an RPM build of a particular osbuild commit for a specific distribution (the CI runner distro). The osbuild commit and the default CI runner distro can be determined from a local copy of the Schutzfile. However, getting the NEVRA requires querying a DNF repository for the CI runner distro.

After brainstorming with @achilleas-k, we agreed to rework the image build cache prefix by:

  • Replacing the osbuild NEVRA with a combination of osbuild commit ref and the CI runner distro. The reason is that this mimics the use of the current use of NEVRA, resulting in a full image build cache rebuild in case the osbuild version or the CI runner distro changes.
  • Moving the osbuild ref and CI runner distro to the beginning of the prefix. This will result in fewer files downloaded when checking if any image needs to be rebuilt. Previously, we would need to download cache for any old osbuild NEVRA if we wanted to download cache files for all distros or architectures.
  • Prefixing some values in the path prefix to fully specify their meaning (e.g. runner distro vs. image distro; osbuild commit SHA vs. manifest ID SHA).

The newly proposed S3 prefix for the image build cache looks like this:

<s3_bucket>/images/builds/osbuild-ref-<commit_ref>/runner-<runner_distro>/<img_distro>/<img_arch>/manifest-id-<img_manifest_id>/

e.g.

<s3_bucket>/images/builds/osbuild-ref-fcb93bde01d4a027c67b5747c8f976ceb4fc8d80/runner-fedora-41/rhel-9.6/x86_64/manifest-id-1ef93e5548a00c1ffe6a33d543da30c19924f5ff0ad1b06440030f63dd4ce4ee/

/jira-epic COMPOSER-2318

JIRA: HMS-5274

thozza added 14 commits January 7, 2025 13:40
@thozza
Test/imgtestlib/clargs(): make --distro option required
5328f3f 
While the arguments parser marked the --distro option as optional, in
reality it is always passed when calling the script. Moreover, some of
the underlying functions would behave weirdly if `distro` would be set
to `None` (e.g. create directories named "None").

Signed-off-by: Tomáš Hozza <[email protected]>
@thozza
Test/imgtestlib: move osbuild ver to the beginning of the S3 prefix
df96b12 
Move the osbuild nevra to the beginning of the image build cache S3
prefix. This is a preparation for not using osbuild NEVRA, but osbuild
commit ref and used CI runner distro. The goal is to make it easy to
determine the S3 prefix corresponding to checked out branch without the
need to setup YUM repo and install RPMs on the CI runner.

Signed-off-by: Tomáš Hozza <[email protected]>
@thozza
Test/imgtestlib: gen_build_info_s3() -> gen_build_info_s3_dir_path()
50dd327 
Rename the function to have "dir_path" in its name to signal that the
returned path has a trailing slash to comply with the S3 convention.

Signed-off-by: Tomáš Hozza <[email protected]>
@thozza
Test/imgtestlib: introduce get_host_distro()
90c8a48 
Add a helper function to get the host distro, instead of reading data
from the os-release and constructing it in every place.

Signed-off-by: Tomáš Hozza <[email protected]>
@thozza
Test/imgtestlib: add runner distro to the build cache S3 prefix
e2845d6 
Add the runner distro version to the image build cache S3 prefix. This
is a preparation for not using osbuild NEVRA, but osbuild commit ref
and used CI runner distro. The goal is to make it easy to determine
the S3 prefix corresponding to checked out branch without the need to
setup YUM repo and install RPMs on the CI runner.

Signed-off-by: Tomáš Hozza <[email protected]>
@thozza
Test/imgtestlib: replace osbuild ver by osbuild ref in the S3 prefix
e333bad 
Replace the osbuild nevra in the image build cache S3 prefix with
osbuild ref and prefix it with "osbuild-ref-" to make the meaning of
the SHA clear. The goal is to make it easy to determine the S3 prefix
corresponding to checked out branch without the need to setup YUM repo
and install RPMs on the CI runner.

Signed-off-by: Tomáš Hozza <[email protected]>
@thozza
Test/imgtestlib: delete get_osbuild_nevra()
a971984 
The function is no longer used by any code.

Signed-off-by: Tomáš Hozza <[email protected]>
@thozza
Test/imgtestlib: introduce gen_build_info_dir_path_prefix()
cc7e0be 
Let's unify the generation of build info dir path prefix into one
function, instead of generating it in various ways in multiple places.
This commit introduces a new function to generate the relative dir path
prefix, depending on provided arguments.

The existing code that generates the prefix on its own will be replaced
in the following commits.

Signed-off-by: Tomáš Hozza <[email protected]>
@thozza
Test/imgtestlib: make gen_build_info_s3_dir_path() more versatile
daae1a0 
Refactor gen_build_info_s3_dir_path() to call
gen_build_info_dir_path_prefix() to generate the S3 path, instead of
constructing it on its own. Amend the test coverage accordingly.

This will allow to use gen_build_info_s3_dir_path() in all places that
construct S3 paths.

Signed-off-by: Tomáš Hozza <[email protected]>
@thozza
Test/imgtestlib/dl_build_info: use gen_build_info_s3_dir_path()
b9f7f79 
Use gen_build_info_s3_dir_path() for generating S3 path, instead of
constructing it on its own.

Signed-off-by: Tomáš Hozza <[email protected]>
@thozza
Test/imgtestlib/filter_builds: use gen_build_info_dir_path_prefix()
647db4e 
Use gen_build_info_dir_path_prefix() for generating build info dir
paths, instead of constructing it on its own.

Signed-off-by: Tomáš Hozza <[email protected]>
@thozza
Test/imgtestlib: delete gen_build_info_path()
5e8a265 
The function is no longer used by any code, delete it.

Signed-off-by: Tomáš Hozza <[email protected]>
@thozza
Test/imgtestlib: delete gen_build_info_dir_path()
e11626e 
The function is no longer used by any code, delete it.

Signed-off-by: Tomáš Hozza <[email protected]>
@thozza
Test/imgtestlib/gen_build_info_dir_path_prefix: use "manifest-id-<ID>"
08f5bb5 
Prefix the manifest_id value in the generated path prefix with
"manifest-id-" to make it obvious what the value represents.

Signed-off-by: Tomáš Hozza <[email protected]>
@thozza thozza marked this pull request as draft January 7, 2025 17:06
@thozza
imgtestlib/gen_build_info_*: don't require osbuild_ref and runner_distro
549f3d2 
Modify `gen_build_info_dir_path_prefix()` and
`gen_build_info_s3_dir_path()` functions to not require osbuild_ref and
runner_distro to be passed explicitly as arguments. If not provided, the
runner_distro defaults to the host distro and the osbuild_ref defaults
to the pinned version for the runner_distro. This simplifies all the
places which call these functions.

Signed-off-by: Tomáš Hozza <[email protected]>
@thozza thozza marked this pull request as ready for review January 7, 2025 17:39
@thozza thozza requested a review from achilleas-k January 7, 2025 17:40
@schutzbot schutzbot changed the title Rework S3 CI cache prefix Rework S3 CI cache prefix (HMS-5274) Jan 7, 2025
@thozza thozza requested review from mvo5, schuellerf and supakeen January 8, 2025 05:02
achilleas-k
achilleas-k approved these changes Jan 8, 2025
View reviewed changes
Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice. All LGTM.

Since I was part of the planning, I wont merge yet and wait for others to review as well.

schuellerf
schuellerf approved these changes Jan 9, 2025
View reviewed changes
Copy link
Contributor

@schuellerf schuellerf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, although I'm not sure if the ref- is really needed as it's pretty obvious and just clutters the name? But works either way, I guess.

@thozza thozza added this pull request to the merge queue Jan 9, 2025
Merged via the queue into osbuild:main with commit f295ad7 Jan 9, 2025
20 checks passed
This was referenced Jan 9, 2025
Draft
Draft
Draft
This was referenced Jan 9, 2025
Open
Open
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schuellerf schuellerf schuellerf approved these changes

@achilleas-k achilleas-k achilleas-k approved these changes

@mvo5 mvo5 Awaiting requested review from mvo5

@supakeen supakeen Awaiting requested review from supakeen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thozza @achilleas-k @schuellerf