From 581291294fd8125a830cdcca248ce6dacd5efb65 Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Tue, 9 Jun 2020 12:23:07 -0400 Subject: [PATCH] core: Use SOLVER_LOCK for locking base packages For the Fedora CoreOS extensions work, when layering packages, we need to be able to tell libsolv to pick the packages which will go with the base packages. IOW, it needs to know that the base packages shouldn't be uninstalled. While investigating https://github.com/coreos/fedora-coreos-tracker/issues/525, I realized that libsolv does have a flag which allows us to express this: `SOLVER_LOCK`. This then allows libsolv to choose the right package for us (if found). And in the case where it can't find a matching package, libsolv itself will print exactly what the conflict is, allowing us to avoid sanity-checking the solution post-solve for "forbidden replacements". Update submodule: libdnf --- libdnf | 2 +- src/libpriv/rpmostree-core.c | 204 ++++++++----------------- tests/vmcheck/test-layering-basic-1.sh | 25 ++- 3 files changed, 91 insertions(+), 140 deletions(-) diff --git a/libdnf b/libdnf index 6043874b95..240fdd4f14 160000 --- a/libdnf +++ b/libdnf @@ -1 +1 @@ -Subproject commit 6043874b9542f8dcedd9f0be25e1044b1b4d6a66 +Subproject commit 240fdd4f14d9b552067ee713f858b5d19b624b99 diff --git a/src/libpriv/rpmostree-core.c b/src/libpriv/rpmostree-core.c index 27bc32f042..fc85f26252 100644 --- a/src/libpriv/rpmostree-core.c +++ b/src/libpriv/rpmostree-core.c @@ -1565,147 +1565,58 @@ gv_nevra_hash (gconstpointer v) return g_str_hash (g_variant_get_string (nevra, NULL)); } -/* We need to make sure that only the pkgs in the base allowed to be - * removed are removed. The issue is that marking a package for DNF_INSTALL - * could uninstall a base pkg if it's an update or obsoletes it. There doesn't - * seem to be a way to tell libsolv to not touch some pkgs in the base layer, - * so we just inspect its solution in retrospect. libdnf has the concept of - * protected packages, but it still allows updating protected packages. - */ +/* For every package which was replaced (i.e. upgraded or downgraded), collects into + * self->pkgs_to_replace the (old pkg, new pkg) tuples so we can inject it later on in the + * commit metadata (this then gets displaying in `rpm-ostree status` for example). We do + * this right after depsolving. */ static gboolean -check_goal_solution (RpmOstreeContext *self, - GPtrArray *removed_pkgnames, - GPtrArray *replaced_nevras, - GError **error) +collect_replaced_packages (RpmOstreeContext *self, + GPtrArray *replaced_pkgnames, + GError **error) { - HyGoal goal = dnf_context_get_goal (self->dnfctx); - - /* check that we're not removing anything we didn't expect */ - { g_autoptr(GPtrArray) forbidden = g_ptr_array_new_with_free_func (g_free); - - /* also collect info about those we're removing */ - g_assert (!self->pkgs_to_remove); - self->pkgs_to_remove = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, - (GDestroyNotify)g_variant_unref); - g_autoptr(GPtrArray) packages = dnf_goal_get_packages (goal, - DNF_PACKAGE_INFO_REMOVE, - DNF_PACKAGE_INFO_OBSOLETE, -1); - for (guint i = 0; i < packages->len; i++) - { - DnfPackage *pkg = packages->pdata[i]; - const char *name = dnf_package_get_name (pkg); - const char *nevra = dnf_package_get_nevra (pkg); - - /* did we expect this package to be removed? */ - if (rpmostree_str_ptrarray_contains (removed_pkgnames, name)) - g_hash_table_insert (self->pkgs_to_remove, g_strdup (name), - gv_nevra_from_pkg (pkg)); - else - g_ptr_array_add (forbidden, g_strdup (nevra)); - } - - if (forbidden->len > 0) - return throw_package_list (error, "Base packages would be removed", forbidden); - } - - /* check that all the pkgs we expect to remove are marked for removal */ - { g_autoptr(GPtrArray) forbidden = g_ptr_array_new (); - - for (guint i = 0; i < removed_pkgnames->len; i++) - { - const char *pkgname = removed_pkgnames->pdata[i]; - if (!g_hash_table_contains (self->pkgs_to_remove, pkgname)) - g_ptr_array_add (forbidden, (gpointer)pkgname); - } - - if (forbidden->len > 0) - return throw_package_list (error, "Base packages not marked to be removed", forbidden); - } + g_assert (self->dnfctx); + /* it's programmer error to call this function twice */ + g_assert (!self->pkgs_to_replace); - /* REINSTALLs should never happen since it doesn't make sense in the rpm-ostree flow, and - * we check very early whether a package is already in the rootfs or not, but let's check - * for it anyway so that we get a bug report in case it somehow happens. */ - { g_autoptr(GPtrArray) packages = - dnf_goal_get_packages (goal, DNF_PACKAGE_INFO_REINSTALL, -1); - g_assert_cmpint (packages->len, ==, 0); - } + HyGoal goal = dnf_context_get_goal (self->dnfctx); - /* Look at UPDATE and DOWNGRADE, and see whether they're doing what we expect */ - { g_autoptr(GHashTable) forbidden_replacements = - g_hash_table_new_full (NULL, NULL, (GDestroyNotify)g_object_unref, - (GDestroyNotify)g_object_unref); - - /* also collect info about those we're replacing */ - g_assert (!self->pkgs_to_replace); - self->pkgs_to_replace = g_hash_table_new_full (gv_nevra_hash, g_variant_equal, - (GDestroyNotify)g_variant_unref, - (GDestroyNotify)g_variant_unref); - g_autoptr(GPtrArray) packages = dnf_goal_get_packages (goal, - DNF_PACKAGE_INFO_UPDATE, - DNF_PACKAGE_INFO_DOWNGRADE, -1); - for (guint i = 0; i < packages->len; i++) - { - DnfPackage *pkg = packages->pdata[i]; - const char *nevra = dnf_package_get_nevra (pkg); - - /* just pick the first pkg */ - g_autoptr(GPtrArray) old = hy_goal_list_obsoleted_by_package (goal, pkg); - g_assert_cmpint (old->len, >, 0); - DnfPackage *old_pkg = old->pdata[0]; - - /* did we expect this nevra to replace a base pkg? */ - if (rpmostree_str_ptrarray_contains (replaced_nevras, nevra)) - g_hash_table_insert (self->pkgs_to_replace, gv_nevra_from_pkg (pkg), - gv_nevra_from_pkg (old_pkg)); - else - g_hash_table_insert (forbidden_replacements, g_object_ref (old_pkg), - g_object_ref (pkg)); - } + g_autoptr(GHashTable) replaced_pkgs = + g_hash_table_new_full (gv_nevra_hash, g_variant_equal, (GDestroyNotify)g_variant_unref, + (GDestroyNotify)g_variant_unref); + g_autoptr(GPtrArray) packages = dnf_goal_get_packages (goal, + DNF_PACKAGE_INFO_UPDATE, + DNF_PACKAGE_INFO_DOWNGRADE, -1); + g_autoptr(GPtrArray) forbidden = NULL; + for (guint i = 0; i < packages->len; i++) + { + DnfPackage *pkg = packages->pdata[i]; + const char *pkgname = dnf_package_get_name (pkg); + g_assert (pkgname); - if (g_hash_table_size (forbidden_replacements) > 0) - { - rpmostree_output_message ("Forbidden base package replacements:"); - GLNX_HASH_TABLE_FOREACH_KV (forbidden_replacements, DnfPackage*, old_pkg, - DnfPackage*, new_pkg) - { - const char *old_name = dnf_package_get_name (old_pkg); - const char *new_name = dnf_package_get_name (new_pkg); - const char *new_repo = dnf_package_get_reponame (new_pkg); - if (g_str_equal (old_name, new_name)) - rpmostree_output_message (" %s %s -> %s (%s)", old_name, - dnf_package_get_evr (old_pkg), - dnf_package_get_evr (new_pkg), new_repo); - else - rpmostree_output_message (" %s -> %s (%s)", - dnf_package_get_nevra (old_pkg), - dnf_package_get_nevra (new_pkg), new_repo); - } - rpmostree_output_message ("This likely means that some of your layered packages " - "have requirements on newer or older versions of some " - "base packages. Doing `rpm-ostree cleanup -m` and " - "`rpm-ostree upgrade` first may help. For " - "more details, see: " - "https://github.com/projectatomic/rpm-ostree/issues/415"); - - return glnx_throw (error, "Some base packages would be replaced"); - } - } + /* just pick the first pkg */ + g_autoptr(GPtrArray) old = hy_goal_list_obsoleted_by_package (goal, pkg); + g_assert_cmpint (old->len, >, 0); + DnfPackage *old_pkg = old->pdata[0]; - /* check that all the pkgs we expect to replace are marked for replacement */ - { g_autoptr(GPtrArray) forbidden = g_ptr_array_new_with_free_func (g_free); + /* just a final sanity-check here that it's a package we expected to be replaced */ + if (!rpmostree_str_ptrarray_contains (replaced_pkgnames, pkgname)) + { + if (!forbidden) + forbidden = g_ptr_array_new_with_free_func (g_free); + g_ptr_array_add (forbidden, g_strdup_printf ("%s -> %s", + dnf_package_get_nevra (old_pkg), + dnf_package_get_nevra (pkg))); + continue; + } - GLNX_HASH_TABLE_FOREACH_KV (self->pkgs_to_replace, GVariant*, new, GVariant*, old) - { - g_autoptr(GVariant) nevra_v = g_variant_get_child_value (new, 0); - const char *nevra = g_variant_get_string (nevra_v, NULL); - if (!rpmostree_str_ptrarray_contains (replaced_nevras, nevra)) - g_ptr_array_add (forbidden, g_strdup (nevra)); - } + g_hash_table_insert (replaced_pkgs, gv_nevra_from_pkg (pkg), + gv_nevra_from_pkg (old_pkg)); + } - if (forbidden->len > 0) - return throw_package_list (error, "Base packages not marked to be installed", forbidden); - } + if (forbidden && forbidden->len > 0) + return throw_package_list (error, "Unexpected upgrade/downgrade", forbidden); + self->pkgs_to_replace = g_steal_pointer (&replaced_pkgs); return TRUE; } @@ -2003,7 +1914,7 @@ rpmostree_context_prepare (RpmOstreeContext *self, g_hash_table_new_full (g_str_hash, g_str_equal, NULL, g_object_unref); /* Handle packages to replace; only add them to the sack for now */ - g_autoptr(GPtrArray) replaced_nevras = g_ptr_array_new (); + g_autoptr(GPtrArray) replaced_pkgnames = g_ptr_array_new_with_free_func (g_free); for (char **it = cached_replace_pkgs; it && *it; it++) { const char *nevra = *it; @@ -2017,7 +1928,7 @@ rpmostree_context_prepare (RpmOstreeContext *self, if (!add_pkg_from_cache (self, nevra, sha256, &pkg, cancellable, error)) return FALSE; - g_ptr_array_add (replaced_nevras, (gpointer)nevra); + g_ptr_array_add (replaced_pkgnames, g_strdup (dnf_package_get_name (pkg))); g_hash_table_insert (local_pkgs_to_install, (gpointer)nevra, g_steal_pointer (&pkg)); } @@ -2175,6 +2086,24 @@ rpmostree_context_prepare (RpmOstreeContext *self, if (missing_pkgs && missing_pkgs->len > 0) return throw_package_list (error, "Packages not found", missing_pkgs); + /* And lock all the base packages we don't expect to be replaced. */ + { hy_autoquery HyQuery query = hy_query_create (sack); + hy_query_filter (query, HY_PKG_REPONAME, HY_EQ, HY_SYSTEM_REPO_NAME); + g_autoptr(GPtrArray) pkgs = hy_query_run (query); + for (guint i = 0; i < pkgs->len; i++) + { + DnfPackage *pkg = pkgs->pdata[i]; + const char *pkgname = dnf_package_get_name (pkg); + g_assert (pkgname); + + if (rpmostree_str_ptrarray_contains (removed_pkgnames, pkgname) || + rpmostree_str_ptrarray_contains (replaced_pkgnames, pkgname)) + continue; + if (hy_goal_lock (goal, pkg, error) != 0) + return glnx_prefix_error (error, "while locking pkg '%s'", pkgname); + } + } + if (self->rojig_spec) { if (!setup_rojig_state (self, error)) @@ -2194,11 +2123,12 @@ rpmostree_context_prepare (RpmOstreeContext *self, rpmostree_output_task_begin (&task, "Resolving dependencies"); /* XXX: consider a --allow-uninstall switch? */ - if (!dnf_goal_depsolve (goal, actions, error) || - !check_goal_solution (self, removed_pkgnames, replaced_nevras, error)) + if (!dnf_goal_depsolve (goal, actions, error)) + return FALSE; + if (!collect_replaced_packages (self, replaced_pkgnames, error)) return FALSE; g_clear_pointer (&self->pkgs, (GDestroyNotify)g_ptr_array_unref); - self->pkgs = dnf_goal_get_packages (dnf_context_get_goal (dnfctx), + self->pkgs = dnf_goal_get_packages (goal, DNF_PACKAGE_INFO_INSTALL, DNF_PACKAGE_INFO_UPDATE, DNF_PACKAGE_INFO_DOWNGRADE, -1); diff --git a/tests/vmcheck/test-layering-basic-1.sh b/tests/vmcheck/test-layering-basic-1.sh index 9820b44fbd..1e10c96363 100755 --- a/tests/vmcheck/test-layering-basic-1.sh +++ b/tests/vmcheck/test-layering-basic-1.sh @@ -206,15 +206,36 @@ echo "ok no leftover rpmdb files" # upgrade to a layer with foo already builtin vm_ostree_commit_layered_as_base $booted_csum vmcheck vm_rpmostree upgrade + +# check that we can't layer a pkg which wants to change a base pkg vm_build_rpm bar conflicts foo if vm_rpmostree install bar &> err.txt; then assert_not_reached "successfully layered conflicting pkg bar?" fi -assert_file_has_content err.txt "Base packages would be removed" +assert_file_has_content err.txt "conflicting requests" +assert_file_has_content err.txt "foo-1.0-1.x86_64" +echo "ok can't layer pkg that would remove base pkg" + +vm_build_rpm foo version 2.0 +vm_build_rpm foo-ext version 2.0 requires "foo = 2.0-1" +if vm_rpmostree install foo-ext &> err.txt; then + assert_not_reached "successfully layered updated split pkg foo-ext?" +fi +assert_file_has_content err.txt "conflicting requests" assert_file_has_content err.txt "foo-1.0-1.x86_64" +assert_file_has_content err.txt "foo-2.0-1.x86_64" +echo "ok can't layer pkg that would upgrade base pkg" + +# check that we can select a repo split pkg which matches the base version +vm_build_rpm foo version 1.0 +vm_build_rpm foo-ext version 1.0 requires "foo = 1.0-1" +vm_rpmostree install foo-ext +vm_assert_status_jq \ + '.deployments[0]["packages"]|length == 1' \ + '.deployments[0]["packages"]|index("foo-ext") >= 0' +echo "ok can layer split pkg matching base version" vm_rpmostree cleanup -p vm_cmd ostree reset vmcheck $(vm_cmd ostree rev-parse "vmcheck^") -echo "ok can't layer pkg that would remove base pkg" # check that root is a shared mount # https://bugzilla.redhat.com/show_bug.cgi?id=1318547