rpmostreed-transaction-types: don't use base-db when idempotent

jmarrero · jmarrero · commit 4d4196e2db6c · 2025-11-26T15:27:03.000-05:00
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 &lt;jmarrero@redhat.com&gt;
diff --git a/src/daemon/rpmostree-sysroot-upgrader.cxx b/src/daemon/rpmostree-sysroot-upgrader.cxx
@@ -111,10 +111,11 @@ struct RpmOstreeSysrootUpgrader
 
   gboolean layering_initialized; /* Whether layering_type is known */
   RpmOstreeSysrootUpgraderLayeringType layering_type;
-  gboolean layering_changed; /* Whether changes to layering should result in a new commit */
-  gboolean pkgs_imported;    /* Whether pkgs to be layered have been downloaded & imported */
-  char *base_revision;       /* Non-layered replicated commit */
-  char *final_revision;      /* Computed by layering; if NULL, only using base_revision */
+  gboolean layering_changed;        /* Whether changes to layering should result in a new commit */
+  gboolean allow_empty_transaction; /* Allow empty DNF transaction for idempotent layering */
+  gboolean pkgs_imported;           /* Whether pkgs to be layered have been downloaded & imported */
+  char *base_revision;              /* Non-layered replicated commit */
+  char *final_revision;             /* Computed by layering; if NULL, only using base_revision */
 
   char **kargs_strv; /* Kernel argument list to be written into deployment  */
 };
@@ -401,6 +402,13 @@ rpmostree_sysroot_upgrader_get_sack (RpmOstreeSysrootUpgrader *self, GError **er
   return self->rpmmd_sack;
 }
 
+void
+rpmostree_sysroot_upgrader_set_allow_empty_transaction (RpmOstreeSysrootUpgrader *self,
+                                                        gboolean allow)
+{
+  self->allow_empty_transaction = allow;
+}
+
 /*
  * Like ostree_sysroot_upgrader_pull(), but also handles the `baserefspec` we
  * use when doing layered packages.
@@ -998,10 +1006,24 @@ prep_local_assembly (RpmOstreeSysrootUpgrader *self, GCancellable *cancellable,
       self->layering_changed = strcmp (previous_state_sha512, new_state_sha512) != 0;
     }
   else
-    /* Otherwise, we're transitioning from not-layered to layered, so it
-       definitely changed */
-    self->layering_changed = TRUE;
-
+    {
+      /* Otherwise, we're transitioning from not-layered to layered, so it
+         definitely changed */
+      self->layering_changed = TRUE;
+      /* Special case for containers: if the DNF transaction is empty (all packages already in
+       * base), don't consider this a change even if transitioning from unlayered to layered. This
+       * happens with idempotent layering when packages are already in the container image. */
+      if (self->layering_type == RPMOSTREE_SYSROOT_UPGRADER_LAYERING_RPMMD_REPOS)
+        {
+          auto refspec = rpmostree_origin_get_refspec (self->computed_origin);
+          if (refspec.kind == rpmostreecxx::RefspecType::Container
+              && rpmostree_dnf_context_has_empty_transaction (
+                  rpmostree_context_get_dnf (self->ctx)))
+            {
+              self->layering_changed = FALSE;
+            }
+        }
+    }
   return TRUE;
 }
 
@@ -1021,6 +1043,7 @@ perform_local_assembly (RpmOstreeSysrootUpgrader *self, GCancellable *cancellabl
 
   rpmostree_context_set_devino_cache (self->ctx, self->devino_cache);
   rpmostree_context_set_tmprootfs_dfd (self->ctx, self->tmprootfs_dfd);
+  rpmostree_context_set_allow_empty_transaction (self->ctx, self->allow_empty_transaction);
 
   if (self->layering_type == RPMOSTREE_SYSROOT_UPGRADER_LAYERING_RPMMD_REPOS)
     {
diff --git a/src/daemon/rpmostree-sysroot-upgrader.h b/src/daemon/rpmostree-sysroot-upgrader.h
@@ -98,6 +98,9 @@ const char *rpmostree_sysroot_upgrader_get_base (RpmOstreeSysrootUpgrader *self)
 
 DnfSack *rpmostree_sysroot_upgrader_get_sack (RpmOstreeSysrootUpgrader *self, GError **error);
 
+void rpmostree_sysroot_upgrader_set_allow_empty_transaction (RpmOstreeSysrootUpgrader *self,
+                                                             gboolean allow);
+
 gboolean rpmostree_sysroot_upgrader_pull_base (RpmOstreeSysrootUpgrader *self,
                                                const char *dir_to_pull, OstreeRepoPullFlags flags,
                                                OstreeAsyncProgress *progress, gboolean *out_changed,
diff --git a/src/daemon/rpmostreed-transaction-types.cxx b/src/daemon/rpmostreed-transaction-types.cxx
@@ -1128,6 +1128,8 @@ deploy_transaction_execute (RpmostreedTransaction *transaction, GCancellable *ca
   if (upgrader == NULL)
     return FALSE;
 
+  rpmostree_sysroot_upgrader_set_allow_empty_transaction (upgrader, idempotent_layering);
+
   g_autoptr (RpmOstreeOrigin) origin = rpmostree_sysroot_upgrader_dup_origin (upgrader);
 
   /* Handle local repo remotes immediately; the idea is that the remote is "transient"
@@ -1191,6 +1193,7 @@ deploy_transaction_execute (RpmostreedTransaction *transaction, GCancellable *ca
     }
 
   gboolean changed = FALSE;
+  gboolean remove_changed = FALSE;
   if (no_initramfs
       && (rpmostree_origin_get_regenerate_initramfs (origin)
           || rpmostree_origin_has_initramfs_etc_files (origin)))
@@ -1214,7 +1217,10 @@ deploy_transaction_execute (RpmostreedTransaction *transaction, GCancellable *ca
   if (no_layering)
     {
       if (rpmostree_origin_remove_all_packages (origin))
-        changed = TRUE;
+        {
+          changed = TRUE;
+          remove_changed = TRUE;
+        }
     }
   else
     {
@@ -1223,43 +1229,60 @@ deploy_transaction_execute (RpmostreedTransaction *transaction, GCancellable *ca
        * create a new deployment; see https://github.com/projectatomic/rpm-ostree/issues/753 */
       if (!rpmostree_origin_remove_packages (origin,
                                              util::rust_stringvec_from_strv (uninstall_pkgs),
-                                             idempotent_layering, &changed, error))
+                                             idempotent_layering, &remove_changed, error))
         return FALSE;
+      if (remove_changed)
+        changed = TRUE;
     }
 
   /* lazily loaded cache that's used in a few conditional blocks */
   g_autoptr (RpmOstreeRefSack) base_rsack = NULL;
 
+  /* Check if we should skip the base-db check:
+   * When booted from a container image on an ostree host,
+   * we skip the base commit rpmdb check because container images don't have
+   * meaningful base-db separation like traditional ostree commits do. */
+  gboolean skip_base_check = FALSE;
+  CXX_TRY_VAR (host_type, rpmostreecxx::get_system_host_type (), error);
+  if (host_type == rpmostreecxx::SystemHostType::OstreeHost)
+    {
+      const auto refspec = rpmostree_origin_get_refspec (origin);
+      skip_base_check = (refspec.kind == rpmostreecxx::RefspecType::Container);
+    }
+
   if (install_pkgs)
     {
       /* we run a special check here; let's just not allow trying to install a pkg that will
        * right away become inactive because it's already installed */
 
-      if (!base_rsack)
+      if (!skip_base_check)
         {
-          const char *base = rpmostree_sysroot_upgrader_get_base (upgrader);
-          base_rsack = rpmostree_get_refsack_for_commit (repo, base, cancellable, error);
-          if (base_rsack == NULL)
-            return FALSE;
-        }
+          if (!base_rsack)
+            {
+              const char *base = rpmostree_sysroot_upgrader_get_base (upgrader);
+              base_rsack = rpmostree_get_refsack_for_commit (repo, base, cancellable, error);
+              if (base_rsack == NULL)
+                return FALSE;
+            }
 
-      for (char **it = install_pkgs; it && *it; it++)
-        {
-          const char *pkg = *it;
-          g_autoptr (GPtrArray) pkgs = rpmostree_get_matching_packages (base_rsack->sack, pkg);
-          if (pkgs->len > 0 && !allow_inactive)
+          for (char **it = install_pkgs; it && *it; it++)
             {
-              g_autoptr (GString) pkgnames = g_string_new ("");
-              for (guint i = 0; i < pkgs->len; i++)
+              const char *pkg = *it;
+              g_autoptr (GPtrArray) pkgs = rpmostree_get_matching_packages (base_rsack->sack, pkg);
+              if (pkgs->len > 0 && !allow_inactive)
                 {
-                  auto p = static_cast<DnfPackage *> (pkgs->pdata[i]);
-                  g_string_append_printf (pkgnames, " %s", dnf_package_get_nevra (p));
+                  g_autoptr (GString) pkgnames = g_string_new ("");
+                  for (guint i = 0; i < pkgs->len; i++)
+                    {
+                      auto p = static_cast<DnfPackage *> (pkgs->pdata[i]);
+                      g_string_append_printf (pkgnames, " %s", dnf_package_get_nevra (p));
+                    }
+                  return glnx_throw (error,
+                                     "\"%s\" is already provided by:%s. Use "
+                                     "--allow-inactive to explicitly "
+                                     "require it.",
+                                     pkg, pkgnames->str);
                 }
-              return glnx_throw (error,
-                                 "\"%s\" is already provided by:%s. Use "
-                                 "--allow-inactive to explicitly "
-                                 "require it.",
-                                 pkg, pkgnames->str);
             }
         }
 
@@ -1565,6 +1588,15 @@ deploy_transaction_execute (RpmostreedTransaction *transaction, GCancellable *ca
     return FALSE;
   changed = changed || layering_changed;
 
+  /* When using containers and nothing changed after prep_layering, return early.
+   * This happens when all requested packages are already present in the container image.
+   * For idempotent mode, this avoids the "No packages in transaction" error.
+   * However, if packages were removed from the origin, we need to deploy to update the origin
+   * even if no layering is needed. Similarly, if we're rebasing (refspec) or deploying a
+   * specific revision, we need to deploy. */
+  if (skip_base_check && !layering_changed && !remove_changed && !self->refspec && !self->revision)
+    return TRUE;
+
   if (dry_run)
     /* Note early return here; we printed the transaction already */
     return TRUE;
diff --git a/src/libpriv/rpmostree-core-private.h b/src/libpriv/rpmostree-core-private.h
@@ -40,6 +40,7 @@ struct _RpmOstreeContext
   std::optional<rust::Box<rpmostreecxx::Treefile>> treefile_owned;
   rpmostreecxx::Treefile *treefile_rs; /* For composes for now */
   gboolean empty;
+  gboolean allow_empty_transaction;
   gboolean disable_selinux;
   char *ref;
 
diff --git a/src/libpriv/rpmostree-core.cxx b/src/libpriv/rpmostree-core.cxx
@@ -381,6 +381,12 @@ rpmostree_context_set_is_empty (RpmOstreeContext *self)
   self->empty = TRUE;
 }
 
+void
+rpmostree_context_set_allow_empty_transaction (RpmOstreeContext *self, gboolean allow)
+{
+  self->allow_empty_transaction = allow;
+}
+
 void
 rpmostree_context_disable_selinux (RpmOstreeContext *self)
 {
@@ -784,7 +790,8 @@ rpmostree_context_setup (RpmOstreeContext *self, const char *install_root, const
        * we're only installing local RPMs. Missing deps will cause the regular
        * 'not found' error from libdnf. */
       auto pkgs = self->treefile_rs->get_packages ();
-      if (!pkgs.empty ())
+      auto local_pkgs = self->treefile_rs->get_local_packages ();
+      if (!pkgs.empty () && local_pkgs.empty ())
         return glnx_throw (error, "No enabled repositories");
     }
 
@@ -4229,8 +4236,11 @@ rpmostree_context_assemble (RpmOstreeContext *self, GCancellable *cancellable, G
   g_autoptr (GPtrArray) overrides_remove = dnf_goal_get_packages (
       dnf_context_get_goal (dnfctx), DNF_PACKAGE_INFO_REMOVE, DNF_PACKAGE_INFO_OBSOLETE, -1);
 
-  if (overlays->len == 0 && overrides_remove->len == 0 && overrides_replace->len == 0)
-    return glnx_throw (error, "No packages in transaction");
+  if (rpmostree_dnf_context_has_empty_transaction (dnfctx))
+    {
+      if (!self->allow_empty_transaction)
+        return glnx_throw (error, "No packages in transaction");
+    }
 
   /* Sort the packages as rpmtsOrder() only reorder to satisfy dependencies
    * but doesn't impose any ordering to packages with the same dependencies.
diff --git a/src/libpriv/rpmostree-core.h b/src/libpriv/rpmostree-core.h
@@ -109,6 +109,7 @@ void rpmostree_context_configure_from_deployment (RpmOstreeContext *self, Ostree
                                                   OstreeDeployment *cfg_deployment);
 
 void rpmostree_context_set_is_empty (RpmOstreeContext *self);
+void rpmostree_context_set_allow_empty_transaction (RpmOstreeContext *self, gboolean allow);
 void rpmostree_context_disable_selinux (RpmOstreeContext *self);
 const char *rpmostree_context_get_ref (RpmOstreeContext *self);
 
diff --git a/src/libpriv/rpmostree-rpm-util.cxx b/src/libpriv/rpmostree-rpm-util.cxx
@@ -1048,6 +1048,21 @@ rpmostree_print_transaction (DnfContext *dnfctx)
     rpmostree_output_message ("Empty transaction");
 }
 
+/* Helper function to check if a DNF transaction has any packages to install, remove, or update */
+gboolean
+rpmostree_dnf_context_has_empty_transaction (DnfContext *dnfctx)
+{
+  HyGoal goal = dnf_context_get_goal (dnfctx);
+
+  g_autoptr (GPtrArray) install_pkgs = dnf_goal_get_packages (goal, DNF_PACKAGE_INFO_INSTALL, -1);
+  g_autoptr (GPtrArray) remove_pkgs
+      = dnf_goal_get_packages (goal, DNF_PACKAGE_INFO_REMOVE, DNF_PACKAGE_INFO_OBSOLETE, -1);
+  g_autoptr (GPtrArray) update_pkgs
+      = dnf_goal_get_packages (goal, DNF_PACKAGE_INFO_UPDATE, DNF_PACKAGE_INFO_DOWNGRADE, -1);
+
+  return (install_pkgs->len == 0 && remove_pkgs->len == 0 && update_pkgs->len == 0);
+}
+
 G_DEFINE_AUTO_CLEANUP_FREE_FUNC (cap_t, cap_free, NULL)
 
 GVariant *
diff --git a/src/libpriv/rpmostree-rpm-util.h b/src/libpriv/rpmostree-rpm-util.h
@@ -117,6 +117,8 @@ gint rpmostree_pkg_array_compare (DnfPackage **p_pkg1, DnfPackage **p_pkg2);
 
 void rpmostree_print_transaction (DnfContext *context);
 
+gboolean rpmostree_dnf_context_has_empty_transaction (DnfContext *dnfctx);
+
 GVariant *rpmostree_fcap_to_ostree_xattr (const char *fcap, GError **error);
 GVariant *rpmostree_fcap_to_xattr_variant (const char *fcap, GError **error);
 
diff --git a/tests/kolainst/destructive/idempotent-layering b/tests/kolainst/destructive/idempotent-layering
