From 1c52fbed06665e6d824d27919e32161938877715 Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Mon, 11 Sep 2017 20:00:40 +0000 Subject: [PATCH] app: smarter deployment change detection Commands like `upgrade` and `deploy` need to know if a new deployment was actually laid down so that it may print a pkg diff if so. This is implemented by listening for changes to the DefaultDeployment D-Bus property. D-Bus emits a signal when the deployment variant changes value. However, in #595, with the introduction of `pending-*` related keys, the deployment variant no longer represents data solely tied to that specific deployment. In this case, because `deploy` operations currently set the ref to the resolved checksum, it can happen that deploying the same base commit when the current refspec *isn't* pointing to that base commit will result in the `pending-*` keys dropping out and a default deployment change notification going out. In this patch, we strengthen how we determine whether a new deployment was laid down by actually looking at the deployment id, rather than just assuming that a change to the property implies a new deployment. Closes: #981 --- src/app/rpmostree-builtin-deploy.c | 6 ++--- src/app/rpmostree-builtin-upgrade.c | 7 +++--- src/app/rpmostree-libbuiltin.c | 36 +++++++++++++++++++++++++---- src/app/rpmostree-libbuiltin.h | 2 +- tests/vmcheck/test-basic.sh | 18 +++++++++++++++ 5 files changed, 57 insertions(+), 12 deletions(-) diff --git a/src/app/rpmostree-builtin-deploy.c b/src/app/rpmostree-builtin-deploy.c index 403de88841..d42de9494f 100644 --- a/src/app/rpmostree-builtin-deploy.c +++ b/src/app/rpmostree-builtin-deploy.c @@ -52,7 +52,6 @@ rpmostree_builtin_deploy (int argc, g_autoptr(GOptionContext) context = NULL; glnx_unref_object RPMOSTreeOS *os_proxy = NULL; glnx_unref_object RPMOSTreeSysroot *sysroot_proxy = NULL; - g_autoptr(GVariant) new_default_deployment = NULL; g_autofree char *transaction_address = NULL; const char * const packages[] = { NULL }; const char *revision; @@ -93,6 +92,7 @@ rpmostree_builtin_deploy (int argc, cancellable, &os_proxy, error)) return EXIT_FAILURE; + gboolean default_deployment_changed = FALSE; if (opt_preview) { if (!rpmostree_os_call_download_deploy_rpm_diff_sync (os_proxy, @@ -105,7 +105,7 @@ rpmostree_builtin_deploy (int argc, } else { - rpmostree_monitor_default_deployment_change (os_proxy, &new_default_deployment); + rpmostree_monitor_default_deployment_change (os_proxy, &default_deployment_changed); g_autoptr(GVariant) options = rpmostree_get_options_variant (opt_reboot, @@ -174,7 +174,7 @@ rpmostree_builtin_deploy (int argc, } else if (!opt_reboot) { - if (new_default_deployment == NULL) + if (!default_deployment_changed) return RPM_OSTREE_EXIT_UNCHANGED; /* do diff without dbus: https://github.com/projectatomic/rpm-ostree/pull/116 */ diff --git a/src/app/rpmostree-builtin-upgrade.c b/src/app/rpmostree-builtin-upgrade.c index b4c28edcfb..32a481a409 100644 --- a/src/app/rpmostree-builtin-upgrade.c +++ b/src/app/rpmostree-builtin-upgrade.c @@ -60,7 +60,6 @@ rpmostree_builtin_upgrade (int argc, g_autoptr(GOptionContext) context = g_option_context_new (""); glnx_unref_object RPMOSTreeOS *os_proxy = NULL; glnx_unref_object RPMOSTreeSysroot *sysroot_proxy = NULL; - g_autoptr(GVariant) new_default_deployment = NULL; g_autofree char *transaction_address = NULL; _cleanup_peer_ GPid peer_pid = 0; const char *const *install_pkgs = NULL; @@ -107,6 +106,8 @@ rpmostree_builtin_upgrade (int argc, cancellable, &os_proxy, error)) return EXIT_FAILURE; + gboolean default_deployment_changed = FALSE; + if (opt_preview || opt_check) { if (!rpmostree_os_call_download_update_rpm_diff_sync (os_proxy, @@ -117,7 +118,7 @@ rpmostree_builtin_upgrade (int argc, } else { - rpmostree_monitor_default_deployment_change (os_proxy, &new_default_deployment); + rpmostree_monitor_default_deployment_change (os_proxy, &default_deployment_changed); g_autoptr(GVariant) options = rpmostree_get_options_variant (opt_reboot, @@ -184,7 +185,7 @@ rpmostree_builtin_upgrade (int argc, } else if (!opt_reboot) { - if (new_default_deployment == NULL) + if (!default_deployment_changed) { if (opt_upgrade_unchanged_exit_77) return RPM_OSTREE_EXIT_UNCHANGED; diff --git a/src/app/rpmostree-libbuiltin.c b/src/app/rpmostree-libbuiltin.c index e5c9f998f8..9c76c1d943 100644 --- a/src/app/rpmostree-libbuiltin.c +++ b/src/app/rpmostree-libbuiltin.c @@ -42,21 +42,47 @@ rpmostree_usage_error (GOptionContext *context, (void) glnx_throw (error, "usage error: %s", message); } +static char* +get_id_from_deployment_variant (GVariant *deployment) +{ + g_autoptr(GVariantDict) dict = g_variant_dict_new (deployment); + g_autofree char *id; + g_assert (g_variant_dict_lookup (dict, "id", "s", &id)); + return g_steal_pointer (&id); +} + +static +G_DEFINE_QUARK (rpmostree-original-id, rpmostree_original_id) +#define RPMOSTREE_ORIGINAL_ID rpmostree_original_id_quark() + static void default_deployment_change_cb (GObject *object, GParamSpec *pspec, - GVariant **value) + gboolean *changed) { - g_object_get (object, pspec->name, value, NULL); + GVariant *new_default_deployment; + g_object_get (object, pspec->name, &new_default_deployment, NULL); + g_autofree char *new_id = get_id_from_deployment_variant (new_default_deployment); + + const char *original_id = g_object_get_qdata (object, RPMOSTREE_ORIGINAL_ID); + if (!g_str_equal (original_id, new_id)) + *changed = TRUE; } void rpmostree_monitor_default_deployment_change (RPMOSTreeOS *os_proxy, - GVariant **deployment) + gboolean *changed) { - /* This will set the GVariant if the default deployment changes. */ + g_autoptr(GVariant) default_deployment = rpmostree_os_dup_default_deployment (os_proxy); + g_autofree char *original_id = get_id_from_deployment_variant (default_deployment); + + /* we use a quark here so original_id automatically gets freed with os_proxy (but also as + * an easy way to pass data to the cb without a struct and worrying about its lifetime) */ + g_object_set_qdata_full (G_OBJECT (os_proxy), RPMOSTREE_ORIGINAL_ID, + g_steal_pointer (&original_id), (GDestroyNotify)g_free); + g_signal_connect (os_proxy, "notify::default-deployment", - G_CALLBACK (default_deployment_change_cb), deployment); + G_CALLBACK (default_deployment_change_cb), changed); } /* Print the diff between the booted and pending deployments */ diff --git a/src/app/rpmostree-libbuiltin.h b/src/app/rpmostree-libbuiltin.h index 3c004c8cae..52a83d5b54 100644 --- a/src/app/rpmostree-libbuiltin.h +++ b/src/app/rpmostree-libbuiltin.h @@ -43,6 +43,6 @@ rpmostree_print_treepkg_diff_from_sysroot_path (const gchar *sysroot_path, void rpmostree_monitor_default_deployment_change (RPMOSTreeOS *os_proxy, - GVariant **deployment); + gboolean *changed); G_END_DECLS diff --git a/tests/vmcheck/test-basic.sh b/tests/vmcheck/test-basic.sh index bba7d4acd8..220c021346 100755 --- a/tests/vmcheck/test-basic.sh +++ b/tests/vmcheck/test-basic.sh @@ -118,3 +118,21 @@ fi assert_not_file_has_content err.txt "Writing rpmdb" assert_file_has_content err.txt "File exists" echo "ok detecting file name conflicts before writing rpmdb" + +# check that the way we detect deployment changes is not dependent on pending-* +# https://github.com/projectatomic/rpm-ostree/issues/981 +vm_rpmostree cleanup -rp +csum=$(vm_cmd ostree commit -b vmcheck --tree=ref=vmcheck) +# restart to make daemon see the pending cheksum +vm_cmd systemctl restart rpm-ostreed +vm_assert_status_jq '.deployments[0]["pending-base-checksum"]' +# hard reset to booted csum (simulates what deploy does to remote refspecs) +vm_cmd ostree reset vmcheck $(vm_get_booted_csum) +rc=0 +vm_rpmostree deploy $(vm_get_booted_csum) > out.txt || rc=$? +if [ $rc != 77 ]; then + assert_not_reached "trying to re-deploy same commit didn't exit 77" +fi +assert_file_has_content out.txt 'No change.' +vm_assert_status_jq '.deployments[0]["pending-base-checksum"]|not' +echo "ok changes to deployment variant don't affect deploy"