rpmostreed-transaction-types: don't use base-db when idempotent#5510
Merged
jmarrero merged 2 commits intocoreos:mainfrom Dec 1, 2025
Merged
rpmostreed-transaction-types: don't use base-db when idempotent#5510jmarrero merged 2 commits intocoreos:mainfrom
jmarrero merged 2 commits intocoreos:mainfrom
Conversation
|
Skipping CI for Draft Pull Request. |
jmarrero
force-pushed
the
idempotent-base
branch
from
b7c49bd to
bb3936e
Compare
Member
|
Offhand this seems a bit complicated to me; you mentioned in chat I think if we should be using the rpmdb for source of truth here and I think that would make sense; we already are doing that in other cases. It'd be really useful to have a test case for this of course, can we dig in in #5496 for what cases are broken? |
jmarrero
force-pushed
the
idempotent-base
branch
4 times, most recently
from
774d474 to
f77722d
Compare
Member
Author
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an issue with idempotent transactions for container-based systems by properly detecting when a transaction results in no changes. The logic added to rpmostree-sysroot-upgrader.cxx and rpmostreed-transaction-types.cxx correctly handles the special case where layered packages are already present in the base container image. The new test case is comprehensive and validates the fix. The changes are solid, but I have a few suggestions to improve code conciseness and script robustness.
jmarrero
force-pushed
the
idempotent-base
branch
from
f77722d to
7610686
Compare
jmarrero
changed the title
Member
Author
|
@cgwalters anything scary here beyond CI failures It looks like the failures are unrelated, gonna try to start looking at them now and see if I can fix them in #5516. |
cgwalters
approved these changes
Oct 24, 2025
Member
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
Overall looks sane to me! two optionals
jmarrero
force-pushed
the
idempotent-base
branch
from
7610686 to
0eea784
Compare
Member
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses idempotent package layering on container-based systems by skipping the base rpmdb check when a transaction is idempotent. The introduction of the rpmostree_dnf_context_has_empty_transaction helper function is a good refactoring that improves code clarity and reuse. The changes are well-contained, and the addition of a new test case ensures the fix is properly verified. Overall, this is a solid improvement.
jmarrero
force-pushed
the
idempotent-base
branch
from
0eea784 to
3b65b07
Compare
cgwalters
approved these changes
Oct 24, 2025
jmarrero
force-pushed
the
idempotent-base
branch
from
c33e6b7 to
dc3a279
Compare
|
@jmarrero: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
jmarrero
commented
Oct 27, 2025
jmarrero
force-pushed
the
idempotent-base
branch
2 times, most recently
from
31254d6 to
4c320c4
Compare
auto-merge was automatically disabled
November 26, 2025 19:00
Pull request was closed
jmarrero
force-pushed
the
idempotent-base
branch
from
4c320c4 to
f5b263a
Compare
When booted from a container image, skip the base commit rpmdb check during package installation transactions. Container images don't have meaningful base-db separation like traditional ostree commits do. This fixes idempotent layering on booted hosts when deployed via container, where packages already present in the base image would incorrectly fail. Now, if the DNF transaction is empty (all packages already in base), we properly exit cleanly. Also adds tests for idempotent layering scenarios with container images. Signed-off-by: Joseph Marrero Corchado <jmarrero@redhat.com>
jmarrero
force-pushed
the
idempotent-base
branch
from
e14cc1f to
e14b89f
Compare
Member
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a fix for idempotent layering on container-based systems. The changes are well-structured and correctly address the issue where rpm-ostree install --idempotent would fail if the package was already present in the base container image.
The core of the solution involves:
- Introducing an
allow_empty_transactionflag to prevent errors on empty DNF transactions for idempotent operations. - Skipping the base RPMDB check for container-based deployments, which is appropriate since they don't have the same base/layered separation as traditional ostree commits.
- Adjusting the
layering_changedlogic to correctly handle no-op layering on containers. - Adding a comprehensive test case that validates the new behavior in various idempotent scenarios.
The code is clear, and the changes are logically sound. The addition of the rpmostree_dnf_context_has_empty_transaction helper function is a good refactoring that improves code clarity. Overall, this is a solid improvement.
Member
Author
Locally the tests pass with F42, the tests failing on the Jenkins run are the same in other current PRs and I think it's just stuff we need to fix as they broke with the update to CoreOS F43. I think this is ready to be merged. |
Merged
jmarrero
added a commit
to jmarrero/rpm-ostree
that referenced
this pull request
Jan 28, 2026
…ystems 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>
jmarrero
added a commit
to jmarrero/rpm-ostree
that referenced
this pull request
Jan 28, 2026
…osts 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>
jmarrero
added a commit
to jmarrero/rpm-ostree
that referenced
this pull request
Jan 28, 2026
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>
This was referenced Jan 28, 2026
jmarrero
added a commit
to jmarrero/rpm-ostree
that referenced
this pull request
Jan 28, 2026
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>
jmarrero
added a commit
to jmarrero/rpm-ostree
that referenced
this pull request
Feb 4, 2026
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>
jmarrero
added a commit
to jmarrero/rpm-ostree
that referenced
this pull request
Feb 4, 2026
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>
jmarrero
added a commit
to jmarrero/rpm-ostree
that referenced
this pull request
Mar 9, 2026
… systems PR coreos#5510 introduced an early return optimization for container-based deployments to handle idempotent package layering. However, this early return did not check whether the base image had changed, causing `rpm-ostree upgrade` to silently return success without staging a new deployment when upgrading container-based systems without layered packages. The issue occurs because: 1. `skip_base_check` is TRUE for container-based ostree hosts 2. When pulling a new base image, `base_changed = TRUE` → `changed = TRUE` 3. But the early return condition only checked `origin_changed` (layering, remove, override changes) and `have_refspec_or_revision` 4. Since `changed` was not checked, the early return triggered even when a new base image was available This fix adds `!changed` to the early return condition, ensuring that if a new base image was pulled, the deployment proceeds normally. Closes: coreos#5567 Fixes: 4d4196e ("rpmostreed-transaction-types: don't use base-db when idempotent") Assisted-by: Claude (OpenCode) Signed-off-by: Joseph Marrero Corchado <jmarrero@redhat.com>
jmarrero
added a commit
to jmarrero/rpm-ostree
that referenced
this pull request
Mar 9, 2026
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()`. The test is updated to add disk space (`minDisk: 20`), container cleanup between phases, and corrected assertions for the upgrade regression scenarios. 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>
jmarrero
added a commit
to jmarrero/rpm-ostree
that referenced
this pull request
Mar 9, 2026
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 (OpenCode) Signed-off-by: Joseph Marrero Corchado <jmarrero@redhat.com>
jmarrero
added a commit
to jmarrero/rpm-ostree
that referenced
this pull request
Mar 9, 2026
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>
jmarrero
added a commit
to jmarrero/rpm-ostree
that referenced
this pull request
Mar 9, 2026
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When booted from a container image, skip the base commit rpmdb check
during package installation transactions. Container images don't have
meaningful base-db separation like traditional ostree commits do.
This fixes idempotent layering on booted hosts when deployed via
container, where packages already present in the base image would
incorrectly fail. Now, if the DNF transaction is empty (all packages
already in base), we properly exit cleanly.
Also adds tests for idempotent layering scenarios with container
images.
Assisted-by: Claude Code
Fixes #5496