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 coreos#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: coreos#981

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 */

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;

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 */

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

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"