rpmostreed-transaction-types: fix override reset#5558
Conversation
There was a problem hiding this comment.
Code Review
This pull request fixes a regression where rpm-ostree override reset --all would silently fail on container-based systems. The fix introduces an override_changed flag to correctly track override resets and prevent an early-return optimization from skipping the deployment. The changes are logical and are accompanied by a new test case that covers the regression.
I have two main points of feedback:
- A potential incompleteness in the fix, where resetting individual package overrides might still be affected by the original bug.
- A minor maintainability concern in the new test script regarding hardcoded values.
Overall, this is a good fix for the reported issue.
| case $VERSION_ID in | ||
| 42) | ||
| koji_kernel_url="https://koji.fedoraproject.org/koji/buildinfo?buildID=2693809" | ||
| kernel_release=6.14.1-300.fc42.x86_64 | ||
| ;; | ||
| 43) | ||
| koji_kernel_url="https://koji.fedoraproject.org/koji/buildinfo?buildID=2837146" | ||
| kernel_release=6.17.1-300.fc43.x86_64 | ||
| ;; | ||
| *) | ||
| echo "Unsupported Fedora version: $VERSION_ID" | ||
| exit 1 | ||
| ;; | ||
| esac |
There was a problem hiding this comment.
This case statement hardcodes kernel versions and Koji build URLs for specific Fedora releases. This creates a future maintenance burden, as it will require updates for each new Fedora release to prevent the test from failing. To improve maintainability, consider making this more dynamic or adding a prominent comment warning developers that this needs to be updated for new Fedora versions.
There was a problem hiding this comment.
True, but we have this on other parts of the code, maybe next PR when I attempt to fix our CI here.
3c44eda to
8636227
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively resolves a bug where rpm-ostree override reset --all would fail silently on container-based systems. The introduction of the override_changed flag correctly tracks override modifications and prevents the premature early return, ensuring a new deployment is staged as expected. The C++ changes are minimal, targeted, and correct. Furthermore, the accompanying test case in idempotent-layering is a valuable addition, as it now covers this specific regression scenario with kernel overrides and resets. While the fix is solid, I've identified a couple of areas in the test script that could be made more robust. Overall, this is a well-executed fix with good test coverage.
@dustymabe Did we miss something for the CI with the recent changes in f-c-c? |
hmm. I don't think so. let me look |
CI here is using FCOS I did a replay dropping the |
8636227 to
45119f1
Compare
PR coreos#5510 introduced an early return optimization for container-based deployments to handle idempotent package layering. However, this early return checked only `remove_changed` (package removals) and not override changes, causing `rpm-ostree override reset --all` to silently return without staging a new deployment on container-based systems. The issue occurs because: 1. `skip_base_check` is TRUE for container-based ostree hosts 2. `no_overrides` (the -a flag) sets `changed = TRUE` but not `remove_changed` 3. The early return condition `!remove_changed` was satisfied, skipping deployment This fix adds an `override_changed` tracking variable that is set when overrides are reset, and includes it in the early return condition. Now when `rpm-ostree override reset --all` is called, the deployment is properly staged. Also extends the idempotent-layering test to cover this scenario: - Performs a kernel override on a container-based deployment - Tests that `override reset --all` properly stages a new deployment - Verifies the pending deployment has no overrides This would have caught the regression introduced in PR coreos#5510. Fixes: coreos#5510 Assisted-by: Claude (OpenCode) Signed-off-by: Joseph Marrero Corchado <jmarrero@redhat.com>
45119f1 to
665aa98
Compare
|
idempotent-layering kola test passes for me with the added tests. I still have to check the c9s rhcos test but this looks promising. I just need to rebuild rpm-ostree on c9s-toolbox and setup a COSA build with RHCOS. |
|
PR #5510 introduced an early return optimization for container-based deployments to handle idempotent package layering. However, this early return checked only
remove_changed(package removals) and not override changes, causingrpm-ostree override reset --allto silently return without staging a new deployment on container-based systems.The issue occurs because:
skip_base_checkis TRUE for container-based ostree hostsno_overrides(the -a flag) setschanged = TRUEbut notremove_changed!remove_changedwas satisfied, skipping deploymentThis fix adds an
override_changedtracking variable that is set when overrides are reset, and includes it in the early return condition. Now whenrpm-ostree override reset --allis called, the deployment is properly staged.Also extends the idempotent-layering test to cover this scenario:
override reset --allproperly stages a new deploymentThis would have caught the regression introduced in PR #5510.
Fixes: #5510
Assisted-by: Claude (OpenCode)