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

Test/scripts: add scripts for convenient download of image build CI cache (HMS-5356) #1155

Open
wants to merge 4 commits into from
Open

Test/scripts: add scripts for convenient download of image build CI cache (HMS-5356) #1155

wants to merge 4 commits into from
+305 −14

Conversation

thozza
Copy link
Member

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

Add scripts for downloading image build CI cache to test osbuild behavior when rebuilding manifests in the osbuild repository (so-called manifest tests).

As I worked on using these scripts in the osbuild repo for manifest tests, I realized that there is a need to be able to split the test cases (corresponding to specific manifest) into batches to allow parallelization. For this reason, it is desirable to determine the current CI cache to download (which includes generating relevant manifests to determine their checksum), but don't download any image artifacts. The reason is that these are huge, and if traffic is not an issue, it still takes time to download files, which would not be used at all. Image artifacts are downloaded once test cases for the given batch are determined.

Instead of adding an overly complex script to handle both scenarios described above, I decided to split them and add two scripts (naming ideas are welcomed 😇):

  • The dl-image-build-cache script will determine the image build cache S3 paths based on the current branch of the images repository and download the image build cache files. Determining the S3 paths involves generating manifests for the relevant distro/arch/image_type/config combinations to determine manifest checksums. The script, by default, downloads only image build metadata (*.json files, but mainly info.json) and the manifest. It can optionally download also image artifacts. While the reworked manifest tests in osbuild do not use this functionality, I kept it there in the hope that it may be helpful for manual downloads.
  • The dl-one-image-build-cache script reads a single image build metadata (info.json) and downloads all corresponding files.

So the idea of the manifest tests (as implemented in osbuild/osbuild#1983) is that it will:

  1. Run dl-image-build-cache with specific arguments without downloading all image artifacts.
  2. Filter the test cases to a subset based on the test chunk configuration.
  3. Run dl-one-image-build-cache to download image artifacts for the selected test cases.
  4. Run manifest tests for the selected test cases.

/jira-epic COMPOSER-2318

JIRA: HMS-5356

@thozza thozza marked this pull request as draft January 20, 2025 13:17
@schutzbot schutzbot changed the title Test/scripts: add scripts for convenient download of image build CI cache Test/scripts: add scripts for convenient download of image build CI cache (HMS-5356) Jan 20, 2025
@thozza thozza mentioned this pull request Jan 20, 2025
Draft
@thozza thozza force-pushed the download-build-cache-script branch 4 times, most recently from dbdff22 to 559cd98 Compare January 22, 2025 07:55
@thozza thozza marked this pull request as ready for review January 22, 2025 07:56
@thozza
Copy link
Member Author

thozza commented Jan 22, 2025

I'm opening this PR for review, because the cache downloading part for osbuild manifest-tests is IMO sufficient, e.g. see https://gitlab.com/redhat/services/products/image-builder/ci/osbuild/-/jobs/8911649202

@schuellerf
Copy link
Contributor

two minor questions:

  • Is there a special reason why this isn't one script with a command line argument to select behavior?
    (just seems much duplicated code)
  • why don't they have a .py ending (I know it's technically not needed :-| )

@thozza thozza force-pushed the download-build-cache-script branch from 559cd98 to 7576acc Compare January 22, 2025 09:34
@thozza
Copy link
Member Author

thozza commented Jan 22, 2025

two minor questions:

First of all, thanks for having a look at the code.

  • Is there a special reason why this isn't one script with a command line argument to select behavior?
    (just seems much duplicated code)

Each script does fundamentally a different thing before downloading anything from the S3 cache, as described in the PR description.

Personally, I disagree with the statement that there is a lot of duplicate code. The shared parts are in the imgtestlib.py. Merging the two scripts and adding a command to select the behavior, where each takes a different set of arguments, would not be for free, so the lines saved are IMHO questionable. In addition, it would complicate the script. I prefer simpler and single-purpose scripts to ones that do everything.

  • why don't they have a .py ending (I know it's technically not needed :-| )

The reason is consistency with other Python scripts in the directory.

thozza added 4 commits January 22, 2025 11:11
@thozza
Test/imgtestlib: add get_common_ci_runner_distro()
2123104 
Add a helper function to get the distro of the common CI runner.

Signed-off-by: Tomáš Hozza <[email protected]>
@thozza
Test/imgtestlib: add dl_build_cache()
d362cee 
Add a new `dl_build_cache()` function, for convenient download of all or
just a specific files from the image build S3 cache. The function is
based on `dl_build_info()`, which now just calls `dl_build_cache()`.

Signed-off-by: Tomáš Hozza <[email protected]>
@thozza
Test/scripts: add dl-image-build-cache tool
e6c7778 
Add a script for downloading the current image build cache files from
the S3 store. This script will be used for manifest builds in the
osbuild repository, replacing manifest-db.

Signed-off-by: Tomáš Hozza <[email protected]>
@thozza
Test/scripts: add dl-one-image-build-cache tool
5646558 
Add a script for downloading a specific image build cache based on a
provided build info file.

This is useful for downloading specific image artifacts from S3 after
one already downloaded the image build metadata and do it without
regenerating manifests.

Signed-off-by: Tomáš Hozza <[email protected]>
@thozza thozza force-pushed the download-build-cache-script branch from 7576acc to 5646558 Compare January 22, 2025 10:11
@achilleas-k
Copy link
Member

  • why don't they have a .py ending (I know it's technically not needed :-| )

I prefer executable scripts with hashbangs not having extensions. I find it's cleaner for directly executable files to not need to advertise the language they're written in.
./test/scripts/thingie: is it a shell script? python? ruby? perl? A compiled C*/go/rust binary? Doesn't matter (as long as the interpreter is installed for scripted languages).

I don't know if this is a controversial topic (quick searching around suggests it might be), but that was my original thinking when writing these.

achilleas-k
achilleas-k approved these changes Jan 22, 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.

Good stuff. TY. LGTM!

@achilleas-k achilleas-k added this pull request to the merge queue Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@mvo5 mvo5 Awaiting requested review from mvo5

@schuellerf schuellerf Awaiting requested review from schuellerf

@supakeen supakeen Awaiting requested review from supakeen

Assignees
No one assigned
Labels
🌟 best practice
Projects
None yet
Milestone
No milestone
Development

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