From f055334f5b7803b37a6e7d5a81dcc542c0bff33c 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, which is more informative than the "forbidden replacements" error we currently print out. Update submodule: libdnf --- libdnf | 2 +- src/libpriv/rpmostree-core.c | 41 +++++++++++++++++++++----- tests/vmcheck/test-layering-basic-1.sh | 25 ++++++++++++++-- 3 files changed, 58 insertions(+), 10 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..a881f72b2b 100644 --- a/src/libpriv/rpmostree-core.c +++ b/src/libpriv/rpmostree-core.c @@ -1565,12 +1565,14 @@ 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. +/* Before hy_goal_lock(), this function was the only way for us to make sure that libsolv + * wasn't trying to modify a base package it wasn't supposed to. Its secondary purpose was + * to collect the packages being replaced and removed into `self->pkgs_to_replace` and + * `self->pkgs_to_remove` respectively. + * + * Nowadays, that secondary purpose is now its primary purpose because we're already + * guaranteed the first part by hy_goal_lock(). Though we still leave all those original + * checks in place as a fail-safe in case libsolv messes up. */ static gboolean check_goal_solution (RpmOstreeContext *self, @@ -2004,6 +2006,7 @@ rpmostree_context_prepare (RpmOstreeContext *self, /* 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 +2020,13 @@ rpmostree_context_prepare (RpmOstreeContext *self, if (!add_pkg_from_cache (self, nevra, sha256, &pkg, cancellable, error)) return FALSE; + const char *name = dnf_package_get_name (pkg); + g_assert (name); + g_ptr_array_add (replaced_nevras, (gpointer)nevra); + // this is a bit wasteful, but for locking purposes, we need the pkgname to match + // against the base, not the nevra which naturally will be different + g_ptr_array_add (replaced_pkgnames, g_strdup (name)); g_hash_table_insert (local_pkgs_to_install, (gpointer)nevra, g_steal_pointer (&pkg)); } @@ -2175,6 +2184,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)) @@ -2198,7 +2225,7 @@ rpmostree_context_prepare (RpmOstreeContext *self, !check_goal_solution (self, removed_pkgnames, replaced_nevras, 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