From 4ad3ea58c6d94c3731ae86c2b903cf6bfd1d68dc 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 Closes: #984 Approved by: cgwalters --- src/app/rpmostree-builtin-deploy.c | 7 +++---- src/app/rpmostree-builtin-upgrade.c | 7 +++---- src/app/rpmostree-libbuiltin.c | 29 ++++++++++++++++++----------- src/app/rpmostree-libbuiltin.h | 8 ++++---- tests/vmcheck/test-basic.sh | 18 ++++++++++++++++++ 5 files changed, 46 insertions(+), 23 deletions(-) diff --git a/src/app/rpmostree-builtin-deploy.c b/src/app/rpmostree-builtin-deploy.c index 403de88841..3e76b9d521 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,8 @@ rpmostree_builtin_deploy (int argc, cancellable, &os_proxy, error)) return EXIT_FAILURE; + g_autoptr(GVariant) previous_deployment = rpmostree_os_dup_default_deployment (os_proxy); + if (opt_preview) { if (!rpmostree_os_call_download_deploy_rpm_diff_sync (os_proxy, @@ -105,8 +106,6 @@ rpmostree_builtin_deploy (int argc, } else { - rpmostree_monitor_default_deployment_change (os_proxy, &new_default_deployment); - g_autoptr(GVariant) options = rpmostree_get_options_variant (opt_reboot, TRUE, /* allow-downgrade */ @@ -174,7 +173,7 @@ rpmostree_builtin_deploy (int argc, } else if (!opt_reboot) { - if (new_default_deployment == NULL) + if (!rpmostree_has_new_default_deployment (os_proxy, previous_deployment)) 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..50cb882fa0 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; + g_autoptr(GVariant) previous_deployment = rpmostree_os_dup_default_deployment (os_proxy); + if (opt_preview || opt_check) { if (!rpmostree_os_call_download_update_rpm_diff_sync (os_proxy, @@ -117,8 +118,6 @@ rpmostree_builtin_upgrade (int argc, } else { - rpmostree_monitor_default_deployment_change (os_proxy, &new_default_deployment); - g_autoptr(GVariant) options = rpmostree_get_options_variant (opt_reboot, opt_allow_downgrade, @@ -184,7 +183,7 @@ rpmostree_builtin_upgrade (int argc, } else if (!opt_reboot) { - if (new_default_deployment == NULL) + if (!rpmostree_has_new_default_deployment (os_proxy, previous_deployment)) { 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..edd7d69c2a 100644 --- a/src/app/rpmostree-libbuiltin.c +++ b/src/app/rpmostree-libbuiltin.c @@ -42,21 +42,28 @@ rpmostree_usage_error (GOptionContext *context, (void) glnx_throw (error, "usage error: %s", message); } -static void -default_deployment_change_cb (GObject *object, - GParamSpec *pspec, - GVariant **value) +static const char* +get_id_from_deployment_variant (GVariant *deployment) { - g_object_get (object, pspec->name, value, NULL); + g_autoptr(GVariantDict) dict = g_variant_dict_new (deployment); + const char *id; + g_assert (g_variant_dict_lookup (dict, "id", "&s", &id)); + return id; } -void -rpmostree_monitor_default_deployment_change (RPMOSTreeOS *os_proxy, - GVariant **deployment) +gboolean +rpmostree_has_new_default_deployment (RPMOSTreeOS *os_proxy, + GVariant *previous_deployment) { - /* This will set the GVariant if the default deployment changes. */ - g_signal_connect (os_proxy, "notify::default-deployment", - G_CALLBACK (default_deployment_change_cb), deployment); + g_autoptr(GVariant) new_deployment = rpmostree_os_dup_default_deployment (os_proxy); + + /* trivial case */ + if (g_variant_equal (previous_deployment, new_deployment)) + return FALSE; + + const char *previous_id = get_id_from_deployment_variant (previous_deployment); + const char *new_id = get_id_from_deployment_variant (new_deployment); + return !g_str_equal (previous_id, new_id); } /* 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..b96528aeac 100644 --- a/src/app/rpmostree-libbuiltin.h +++ b/src/app/rpmostree-libbuiltin.h @@ -31,6 +31,10 @@ rpmostree_usage_error (GOptionContext *context, const char *message, GError **error); +gboolean +rpmostree_has_new_default_deployment (RPMOSTreeOS *os_proxy, + GVariant *previous_deployment); + gboolean rpmostree_print_treepkg_diff (OstreeSysroot *sysroot, GCancellable *cancellable, @@ -41,8 +45,4 @@ rpmostree_print_treepkg_diff_from_sysroot_path (const gchar *sysroot_path, GCancellable *cancellable, GError **error); -void -rpmostree_monitor_default_deployment_change (RPMOSTreeOS *os_proxy, - GVariant **deployment); - G_END_DECLS diff --git a/tests/vmcheck/test-basic.sh b/tests/vmcheck/test-basic.sh index bba7d4acd8..23be115ff2 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 checksum +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"