Fix silent upgrade failure on container systems#5569
Conversation
There was a problem hiding this comment.
Code Review
This pull request fixes a silent failure during rpm-ostree upgrade on container-based systems that have no layered packages. The core fix in src/daemon/rpmostreed-transaction-types.cxx correctly checks for base image changes to prevent an erroneous early exit, which is the right approach. The changes in rust/src/sysroot_upgrade.rs are a nice UX improvement for progress reporting. The new test cases in tests/kolainst/destructive/idempotent-layering are comprehensive and provide excellent regression coverage for the fixed issue. I have one suggestion to improve the robustness and clarity of the test script.
|
You already have the progress fix in another PR, so let's drop it from this one since it's unrelated. I would then squash the fix and test commits (the test commit message is pretty verbose IMO, don't need to explain every change in the commit msg). |
|
Yeah opened the other PR because this is not passing the test yet. I will update this PR with just one commit once I get the fix to actually work locally. |
3050488 to
68437db
Compare
|
/gemini review |
68437db to
bb64952
Compare
There was a problem hiding this comment.
Code Review
This pull request addresses a silent upgrade failure by correcting the change detection logic in rpmostreed-transaction-types.cxx and allowing empty transactions to proceed in rpmostree-core.cxx. These changes ensure that upgrades, especially for container-based systems, are correctly staged even when no new packages are layered. The addition of extensive regression tests in tests/kolainst/destructive/idempotent-layering is a great way to verify the fix and prevent future regressions. The changes look solid. I have one suggestion to improve the robustness of the test script.
PR coreos#5510 introduced an early return for container-based deployments to handle idempotent package layering, but it failed to check whether the base image itself had changed. This caused `rpm-ostree upgrade` to silently return success without staging a new deployment. Fold `remove_changed` and `override_changed` into the `changed` variable so the early return only triggers when truly nothing changed. Also fix a related crash in `rpmostree_context_assemble()` when `--idempotent --apply-live` is used on a package already in the base image: the empty transaction path hit `g_assert_not_reached()` instead of returning through `rpmostree_context_assemble_end()`. Closes: coreos#5567 Fixes: 4d4196e ("rpmostreed-transaction-types: don't use base-db when idempotent") Assisted-by: Claude Opus 4.6 (OpenCode) Signed-off-by: Joseph Marrero Corchado <jmarrero@redhat.com>
bb64952 to
2429032
Compare
Fix silent upgrade failure on container systems
PR #5510 introduced an early return for container-based deployments
to handle idempotent package layering, but it failed to check whether
the base image itself had changed. This caused
rpm-ostree upgradeto silently return success without staging a new deployment.
Fold
remove_changedandoverride_changedinto thechangedvariable so the early return only triggers when truly nothing changed.
Also fix a related crash in
rpmostree_context_assemble()when--idempotent --apply-liveis used on a package already in the baseimage: the empty transaction path hit
g_assert_not_reached()insteadof returning through
rpmostree_context_assemble_end().Closes: #5567
Fixes: 4d4196e ("rpmostreed-transaction-types: don't use base-db when idempotent")
Assisted-by: Claude Opus 4.6 (OpenCode)
Signed-off-by: Joseph Marrero Corchado jmarrero@redhat.com