Add support for backporting to RHEL z-streams#389
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces specialized handling for older z-stream RHEL versions across various agents. It refactors instruction prompts into constants and introduces new _ZSTREAM variants for backporting and build error fixing, which are conditionally selected based on the fix_version. The triage_agent now uses is_older_zstream and parse_rhel_version for more precise branch mapping and incorporates a ZStreamSearchTool. Common utility functions like is_cs_branch are integrated to dynamically select centpkg or rhpkg for package management commands. Additionally, the patch staging logic in backport_agent is improved to read patch references directly from the spec file, and aiohttp is replaced with run_tool for patch downloading, removing direct aiohttp and re dependencies from backport_agent and triage_agent respectively.
There was a problem hiding this comment.
Nicely done! I went through the code and haven't seen any serious problems.
Edit: Claude is wrong. I asked it to analyze again with opus 4.6:
My earlier review was wrong to flag this as a critical issue. It's the opposite — the old behavior was the bug.
--
Original comment: This is what Claude found locally:
1. _map_version_to_branch now returns a non-existent branch instead of falling back to CentOS Stream
This is the most significant behavioral change and the one most likely to cause real-world breakage.
Before (triage_agent.py, old lines 113-133):
- For z-stream versions, if the internal branch (rhel-9.7.0) did not exist, it fell back to CentOS Stream (c9s).
- If package was None, the z-stream block was skipped entirely, also falling back to c9s.
After (triage_agent.py:119-139):
- For z-stream (or older z-stream) versions, it always returns the internal branch name regardless of whether it exists.
- The branch existence check is now just a log message (line 133-134) — it logs a warning but still returns the branch.
- If package is None, it skips the existence check entirely and still returns the branch.
Why this could break: The returned branch name is used directly in:
- RedisQueues.get_backport_queue_for_branch(state.target_branch) (line 792) — this works fine, it just routes to c9s or c10s queue based on pattern matching.
- But the branch name also flows into the backport agent via state.dist_git_branch, where it's used in actual git operations like centpkg/rhpkg prep --release=<branch>. If the branch doesn't exist in the dist-git repo, those operations will fail.
The comment says "it will be created later if needed" (line 120), but I found no code in this commit that creates the branch. This is an intentional design change, but if the downstream branch-creation logic doesn't exist yet, backport jobs targeting
non-existent branches will fail at runtime.
Edit: all good, +1
agents/backport_agent.py
Outdated
| # sources inside it. Search for buildsubdir within any subdirectory. | ||
| for candidate in builddir.glob(f"*/{buildsubdir}"): | ||
| if candidate.is_dir(): | ||
| return candidate |
There was a problem hiding this comment.
A very similar implementation of this has been removed because of unreliability: 28abdbb
I think this should be much more reliable and work in all cases:
with Specfile(local_clone / f"{package}.spec", macros=[("_builddir", str(local_clone))]) as spec:
builddir = spec.expand("%{builddir}")
buildsubdir = spec.expand("%{buildsubdir}")
if "/" in buildsubdir:
# Sooner or later we'll run into a package where this will break. Sorry.
# More details: https://github.com/packit/jotnar/issues/217
buildsubdir = buildsubdir.split("/")[0]
return Path(builddir) / buildsubdirThere was a problem hiding this comment.
@nforro no, that doesnt work at all and fails to locate the actual builddir. i have another patch to address it when %{builddir} doesn't expand (specfile library can't resolve it since it's only set during rpmbuild execution) so we need to reconstruct it the way RPM does: %{_builddir}/%{NAME}-%{VERSION}-build. If that per-build directory exists on disk, use it; otherwise fall back to %{_builddir} for older RPM versions. i'm not sure its an improvement as it seems to be potentially flaky as well so if you can suggest other solution/s please do.
There was a problem hiding this comment.
Sorry, it works only with rpm >= 4.20.
nforro
left a comment
There was a problem hiding this comment.
I don't like the prompt duplication, especially in the backport agent it adds up and makes tweaking prompts/instructions a PITA. But fixing that is out of scope of this PR.
ec771fd to
8d7fd66
Compare
|
@nforro the latest push has that builddir logic patch (and related tests) i've mentioned in the review thread. i'm not really sure if its better or worse than what i had before as it relies on something that is baked into librpmbuild directly and its probably not a public api/interface but just how it currently works. this isnt my area of expertise (clearly) so if you have other suggestions we can try them. this i've tested on a real backport for rhel-9.0.0.z and it worked at least but so did the previous version. another small but important change in this one is we've experienced issues when patch names in the spec had macros in them so this goes on top of the previous related changes to improve them there. |
| # per-build directory under builddir and unpacks sources inside it. | ||
| builddir = spec.expand("%{builddir}") | ||
| if "%{builddir}" in builddir: | ||
| # %{builddir} is only set during rpmbuild execution (by %setup). |
There was a problem hiding this comment.
What are you basing this on? AFAICT it is set here, during preamble parsing, but only in rpm >= 4.20.
There was a problem hiding this comment.
there is a comment in /usr/lib/rpm/macros:
# The sub-directory (relative to %{builddir}) where sources are compiled.
# This macro is set after processing %setup, either explicitly from the
# value given to -n or the default name-version.
#
#%buildsubdir
There was a problem hiding this comment.
there is a comment in /usr/lib/rpm/macros:
# The sub-directory (relative to %{builddir}) where sources are compiled. # This macro is set after processing %setup, either explicitly from the # value given to -n or the default name-version. # #%buildsubdir
But that's about %buildsubdir, not %builddir.
| _builddir = spec.expand("%{_builddir}") | ||
| rpm_builddir = Path(_builddir) / f"{spec.expand('%{NAME}')}-{spec.expand('%{VERSION}')}-build" | ||
| builddir = str(rpm_builddir) if rpm_builddir.is_dir() else _builddir |
There was a problem hiding this comment.
This is not great either, but I think it's an improvement. But TBH I don't remember what exact issues we had with the globbing.
There was a problem hiding this comment.
i think i have a better solution. instead of fiddling with all of this we can just do this on the mcp gateway itself and always get the right value eg make prep_sources return the path and get rid of get_unpacked_sources entirely. WDYT?
There was a problem hiding this comment.
i think i have a better solution. instead of fiddling with all of this we can just do this on the mcp gateway itself and always get the right value eg make prep_sources return the path and get rid of get_unpacked_sources entirely. WDYT?
Makes sense. You still have to preserve the hack with multi-directory %buildsubdir though (for e.g. this).
This commit adds the ability to triage and backport for RHEL z-streams, specifically older z-streams. The main changes are:
Instructions are now split up into 2 different sets for z-stream and regular cases. This is perhaps best addressed long term with some sort of refactor into a template system and storing the instructions separate from the code. The instructions are mostly the same for now but we will be changing z-stream instructions further in the future.
There is now a distinction between centpkg and rhpkg and download and prep sources are invoked via MCP gateway instead of directly as rhpkg requires authentication even for basic functionality. This also required a change to how the package build directory is located as there are changes between MCP gateway rpm version and agents rpm version when preparing package sources, resulting in different directory structure.
The spec file is now the source of truth for patch files. Instead of tracking what has been added vs what is needed which can get inconsistent, as the agent might create or add random patch files in the process, we instead just track what we need from the spec file and ignore anything else.
Please note that I have only tested these changes on older RHEL z-stream backports and while I've tried not to disturb the existing code and workflow the best I can, there might be some hidden corner cases or inconsistencies related to the standard (non z-stream) process.