-
Notifications
You must be signed in to change notification settings - Fork 183
cmd-fetch: Derive konflux lockfiles from rpm-ostree #4298
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a valuable feature for generating Konflux-compatible lockfiles to enable hermetic builds. The approach of deriving the new lockfile from the existing rpm-ostree one and including a sanity check is solid. I've identified a couple of high-severity issues that could cause script failures, including a syntax error and a potential TypeError
. I've also included several medium-severity suggestions to improve code clarity, fix a typo in a user-facing message, and enhance maintainability by removing hardcoded values.
I pushed a second commit here that use @joelcapitao PTAL |
4818082
to
2ca3c27
Compare
2ca3c27
to
12937d1
Compare
This ends up being a whole more complex than I tought so turning it back to draft |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, looking better! I'm much more confident about this personally now that it's not doing a separate depsolve.
src/cosalib/cmdlib.py
Outdated
return json.loads(data) | ||
|
||
|
||
def get_locked_nevras(srcdir): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I don't think we need this. Why not use the $outfile
from the rpm-ostree step?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point ! I grafted my stuff into cmd-fetch without looking too much at the integration. Thanks for the suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
follow-up suggestion that I only came up with, with some changes I have not push yet : using dnf repoquery
we can get all the packages location from the x86 builder in one process, using --forcearch
so we can load all locks from the x86 main machine.
So I could move the --konflux
flag to be a separate thing that is called after the tests at the end of the pipeline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine either way. Seems mostly equivalent I think, no? cosa fetch
is also called on each architecture individually.
I see two minor advantages to just doing it part of the same cosa fetch
build:
- keeps the pipeline code simpler
- if repoquery flakes on infra, we fail early and not after all the expensive tests have run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to this discussion, I think we probably don't actually need a --konflux
flag and could just unconditionally do this if we detect there are Konflux locks. The flag is nice if we want to gain confidence first. Though now that this is much more mechanical, I'm personally OK just going straight to the unconditional logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* keeps the pipeline code simpler
Does it ? Because if each builder write it's own lockfile we have to add code to the pipeline that merges all of them back to the same file at the end anyway.
* if repoquery flakes on infra, we fail early and not after all the expensive tests have run
Yeah agreed. Looking at the pipeline code this can still be done on the x86 builder before the tests, as all the lockfiles are synced back (for change detection and other things). I'll come up with something and we can discuss it further
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I updated the script with your suggestion. See last commit for details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* keeps the pipeline code simpler
Does it ? Because if each builder write it's own lockfile we have to add code to the pipeline that merges all of them back to the same file at the end anyway.
Sorry, I missed that a key difference here is that Konflux lockfiles keep the different arches in the same lockfile. Is there an option to support separating them out? Otherwise, yeah I guess just doing the merge step separately SGTM.
9bd68a3
to
442cd35
Compare
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new --konflux
flag to generate a hermetic lockfile for Konflux builds. The implementation adds a new script konflux-rpm-lockfile
and a helper function in cosalib
. The overall approach is sound, but I've identified a critical bug in the new script where command-line arguments for dnf
are incorrectly accumulated within a loop, which will lead to incorrect package resolution. I've also found a logic issue in how override lockfiles are merged and suggested a few minor code quality improvements. Please address these points to ensure the new functionality is robust.
We are about to commit lockfiles to enable hermetic builds, we'll bump definitions with cosa within bump-lockfile Jenkins job [1]. So we don't want Renovate/MintMaker do the maintenance of this file. [1] coreos/coreos-assembler#4298
We are about to commit lockfiles to enable hermetic builds, we'll bump definitions with cosa within bump-lockfile Jenkins job [1]. So we don't want Renovate/MintMaker do the maintenance of this file. [1] coreos/coreos-assembler#4298
Let's use our lockfiles to get hermeto download a set of matching RPMs. This is done by simply getting the package URL with `dnf repoquery` using the full NEVRA from the rpm-ostree lockfile. This will allow hermetic builds in Konflux. I also added an override yaml file to allow merging packages to the lock this will allow injecting arbitrary package. This will help enabling workarounds such as coreos/fedora-coreos-config@fb167ed See also coreos/fedora-coreos-config#3644 for the inital implementation and discussion.
442cd35
to
d014342
Compare
Refactor `konflux-rpm-lockfile` to support parallel, per-arch builds. The script is now split into `generate` and `merge` subcommands. `cmd-fetch` now calls `generate` to create arch-specific lockfile (e.g., `x86_64.rpms.lock.yaml`). It also look for an existing lockfile and call generate in that case. The `merge` subcommand combines these files in one, while injecting overrides. The override mechanism allow developpers to add extra RPMs needed in the build, e.g. in `buildroot-prep` [1] The pipeline will call merge after running `generate` on each builder. [1] coreos/fedora-coreos-config@fb167ed See: coreos#4298 (comment)
Refactor `konflux-rpm-lockfile` to support parallel, per-arch builds. The script is now split into `generate` and `merge` subcommands. `cmd-fetch` now calls `generate` to create arch-specific lockfile (e.g., `x86_64.rpms.lock.yaml`). It also look for an existing lockfile and call generate in that case. The `merge` subcommand combines these files in one, while injecting overrides. The override mechanism allow developpers to add extra RPMs needed in the build, e.g. in `buildroot-prep` [1] The pipeline will call merge after running `generate` on each builder. [1] coreos/fedora-coreos-config@fb167ed See: coreos#4298 (comment)
23c2587
to
156db4a
Compare
Refactor `konflux-rpm-lockfile` to support parallel, per-arch builds. The script is now split into `generate` and `merge` subcommands. `cmd-fetch` now calls `generate` to create arch-specific lockfile (e.g., `x86_64.rpms.lock.yaml`). It also look for an existing lockfile and call generate in that case. The `merge` subcommand combines these files in one, while injecting overrides. The override mechanism allow developpers to add extra RPMs needed in the build, e.g. in `buildroot-prep` [1] The pipeline will call merge after running `generate` on each builder. [1] coreos/fedora-coreos-config@fb167ed See: coreos#4298 (comment)
156db4a
to
b961d04
Compare
Refactor `konflux-rpm-lockfile` to support parallel, per-arch builds. The script is now split into `generate` and `merge` subcommands. `cmd-fetch` now calls `generate` to create arch-specific lockfile (e.g., `x86_64.rpms.lock.yaml`). It also look for an existing lockfile and call generate in that case. The `merge` subcommand combines these files in one, while injecting overrides. The override mechanism allow developpers to add extra RPMs needed in the build, e.g. in `buildroot-prep` [1] The pipeline will call merge after running `generate` on each builder. [1] coreos/fedora-coreos-config@fb167ed See: coreos#4298 (comment)
b961d04
to
eb5bd01
Compare
--with-cosa-overrides Don't ignore cosa overrides in `overrides/rpm` | ||
--autolock=VERSION If no base lockfile used, create one from any arch build of `VERSION` | ||
--konflux Generate hermeto lockfile for Konflux derived from the rpm-ostree lockfiles. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could inform the user that if cosa fetch
finds rpms.lock.yaml
in config directory, then this flag is enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested it locally, LGTM 👍
# We want to ensure that hermeto creates repo definitions for every repository. | ||
# A round-robin assignment ensures each repo is mentioned at least once. | ||
# This is needed because rpm-ostree uses the full list of repos to | ||
# resolve packages and errors out if a repository is missing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. And I guess hermeto won't inject a repo if it's not mentioned in the lockfile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I guess it doesn't matter than in actuality the package URL isn't coming from that given repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes for both. The repoid in the lockfile is for the local repo creation : the rpm have been downloaded, and it will be available under that repo id, so the url is no longer relevant.
i = 0 | ||
for pkg in pkgs: | ||
packages.append({"url": pkg, "repoid": local_repos[i % repo_numbers]}) | ||
i += 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i = 0 | |
for pkg in pkgs: | |
packages.append({"url": pkg, "repoid": local_repos[i % repo_numbers]}) | |
i += 1 | |
for i, pkg in enumerate(pkgs): | |
packages.append({"url": pkg, "repoid": local_repos[i % repo_numbers]}) |
return int(subprocess.check_output(['kola', 'ncpu'])) | ||
|
||
|
||
def get_locked_nevras(srcdir, arch=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, not sure I understand why we still have this. There's no need for merge semantics here I don't think. And at that point, the caller can just load the base lockfile directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we still want overrides for pins don't we ?
If i understand your comment it means the lockfile already include those after cosa fetch --update-lockfile
runs ? In which case yeah, we don't we need this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If i understand your comment it means the lockfile already include those after
cosa fetch --update-lockfile
runs ? In which case yeah, we don't we need this
Correct, yes. --update-lockfile
initially did not do that in fact and ignored overrides, but then we had to change it because sometimes things are so broken that you can't even do a depsolve without the overrides.
I mean, ideally yes we do handle override lockfiles as well in a similar way. E.g. if someone today opens PR to fast-track something, they only update the overrides lockfile, not the base one. And things still work.
But now with this Konflux lockfile, it wouldn't pick up that change until the next bump-lockfile run.
We probably want to call this script from https://github.com/coreos/fedora-coreos-config/blob/testing-devel/ci/overrides.py here as well, which is the script that powers the fast-tracking GHA workflow.
Longer-term of course, the right fix is to have a single lockfile language that's understood by both Konflux and the actual build tools (i.e. rpm-software-management/dnf5#833 and the work going on in https://github.com/rpm-software-management/libpkgmanifest).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, yes.
--update-lockfile
initially did not do that in fact and ignored overrides, but then we had to change it because sometimes things are so broken that you can't even do a depsolve without the overrides.
Oh OK, yeah that make sense.
But now with this Konflux lockfile, it wouldn't pick up that change until the next bump-lockfile run.
Unless you query the url manually, add it to the override file and run merge
. But then it's a more complex process to add an override.
We probably want to call this script from coreos/fedora-coreos-config@
testing-devel
/ci/overrides.py here as well, which is the script that powers the fast-tracking GHA workflow.
I'll look into that
Longer-term of course, the right fix is to have a single lockfile language that's understood by both Konflux and the actual build tools
Agreed. I read a bunch of discussion about this but it looks like it's going to take a little while
print("Some packages from the lockfile could not be resolved. The rpm-ostree lockfile is probably out of date.") | ||
sys.exit(1) | ||
|
||
print(f"Done. Solved {len(pkg_urls)} packages.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
print(f"Done. Solved {len(pkg_urls)} packages.") | |
print(f"Done. Resolved location for {len(pkg_urls)} packages.") |
Let's use our lockfiles to get hermeto download a set of matching RPMs. This is done through injecting
includepkg
filters to the repofiles then feeding that torpm-lockfile-prototype
.The overview is:
we parse the manifest for a package list and enabled repos. For each package, we take the expected EVRA in the rpm-ostree lockfile and inject it in the
includepkg
filter in the repos files. Finally we feed that package list and the custom repo definition to rpm-ostree-prototype.The result is a hermeto/cachi2 lockfile we will commit to the repo.
This
--konflux
flag would then be added to the Jenkins bump-lockfiles job to update both lockfiles in the same commit.This will allow hermetic builds in Konflux.
I also added a sanity check at the end that iterate over both lockfiles and compare all versions string to make sure we have exact matches all around.
See also coreos/fedora-coreos-config#3644 for the inital implementation.