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
coreos · Sep 12, 2017 · 4ad3ea5 · 4ad3ea5
1 parent 077d7c1
commit 4ad3ea5
Show file tree

Hide file tree

Showing 5 changed files with 46 additions and 23 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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"