app: smarter deployment change detection

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

src/daemon/rpmostree-sysroot-upgrader.c

@@ -373,21 +373,18 @@ rpmostree_sysroot_upgrader_pull_base (RpmOstreeSysrootUpgrader  *self,
                                       GCancellable           *cancellable,
                                       GError                **error)
 {
+  const char *refspec = rpmostree_origin_get_refspec (self->origin);
+
   g_autofree char *origin_remote = NULL;
   g_autofree char *origin_ref = NULL;
-  if (!ostree_parse_refspec (rpmostree_origin_get_refspec (self->origin),
-                             &origin_remote, &origin_ref, error))
+  if (!ostree_parse_refspec (refspec, &origin_remote, &origin_ref, error))
     return FALSE;
 
-  char *refs_to_fetch[] = { NULL, NULL };
-  if (rpmostree_origin_get_override_commit (self->origin) != NULL)
-    refs_to_fetch[0] = (char*)rpmostree_origin_get_override_commit (self->origin);
-  else
-    refs_to_fetch[0] = origin_ref;
-
   const gboolean allow_older =
     (self->flags & RPMOSTREE_SYSROOT_UPGRADER_FLAGS_ALLOW_OLDER) > 0;
 
+  const char *override_commit = rpmostree_origin_get_override_commit (self->origin);
+
   g_assert (self->origin_merge_deployment);
   if (origin_remote)
     {
@@ -403,7 +400,12 @@ rpmostree_sysroot_upgrader_pull_base (RpmOstreeSysrootUpgrader  *self,
         g_variant_builder_add (optbuilder, "{s@v}", "timestamp-check",
                                g_variant_new_variant (g_variant_new_boolean (TRUE)));
       g_variant_builder_add (optbuilder, "{s@v}", "refs",
-                             g_variant_new_variant (g_variant_new_strv ((const char *const*) refs_to_fetch, -1)));
+                             g_variant_new_variant (g_variant_new_strv (
+                                 (const char *const *)&origin_ref, 1)));
+      if (override_commit)
+        g_variant_builder_add (optbuilder, "{s@v}", "override-commit-ids",
+                               g_variant_new_variant (g_variant_new_strv (
+                                   (const char *const *)&override_commit, 1)));
 
       g_autoptr(GVariant) opts = g_variant_ref_sink (g_variant_builder_end (optbuilder));
       if (!ostree_repo_pull_with_options (self->repo, origin_remote, opts, progress,
@@ -414,48 +416,32 @@ rpmostree_sysroot_upgrader_pull_base (RpmOstreeSysrootUpgrader  *self,
         ostree_async_progress_finish (progress);
     }
 
-  g_autofree char *new_base_revision = NULL;
-  if (rpmostree_origin_get_override_commit (self->origin) != NULL)
-    {
-      if (!ostree_repo_set_ref_immediate (self->repo,
-                                          origin_remote,
-                                          origin_ref,
-                                          rpmostree_origin_get_override_commit (self->origin),
-                                          cancellable,
-                                          error))
-        return FALSE;
-
-      new_base_revision = g_strdup (rpmostree_origin_get_override_commit (self->origin));
-    }
+  g_autofree char *new_base_rev = NULL;
+  if (override_commit)
+    new_base_rev = g_strdup (override_commit);
   else
     {
-      if (!ostree_repo_resolve_rev (self->repo, rpmostree_origin_get_refspec (self->origin), FALSE,
-                                    &new_base_revision, error))
+      if (!ostree_repo_resolve_rev (self->repo, refspec, FALSE, &new_base_rev, error))
         return FALSE;
     }
 
-  {
-    gboolean changed = FALSE;
-
-    if (strcmp (new_base_revision, self->base_revision) != 0)
-      {
-        changed = TRUE;
-
-        if (!allow_older)
-          {
-            if (!ostree_sysroot_upgrader_check_timestamps (self->repo, self->base_revision,
-                                                           new_base_revision,
-                                                           error))
-              return FALSE;
-          }
-
-        g_free (self->base_revision);
-        self->base_revision = g_steal_pointer (&new_base_revision);
-      }
+  gboolean changed = !g_str_equal (new_base_rev, self->base_revision);
+  if (changed)
+    {
+      /* check timestamps here too in case the commit was already pulled (or the
+       * refspec is local) */
+      if (!allow_older)
+        {
+          if (!ostree_sysroot_upgrader_check_timestamps (self->repo, self->base_revision,
+                                                         new_base_rev, error))
+            return FALSE;
+        }
 
-    *out_changed = changed;
-  }
+      g_free (self->base_revision);
+      self->base_revision = g_steal_pointer (&new_base_rev);
+    }
 
+  *out_changed = changed;
   return TRUE;
 }
 

src/daemon/rpmostreed-transaction-types.c

@@ -814,7 +814,23 @@ deploy_transaction_execute (RpmostreedTransaction *transaction,
                                                  &base_changed, cancellable, error))
         return FALSE;
 
-      changed = changed || base_changed;
+      if (base_changed)
+        changed = TRUE;
+      else
+        {
+          /* If we're on a live deployment, then allow redeploying a clean version of the
+           * same base commit. This is useful if e.g. the pushed rollback was cleaned up. */
+
+          OstreeDeployment *deployment =
+            rpmostree_sysroot_upgrader_get_merge_deployment (upgrader);
+
+          gboolean is_live;
+          if (!rpmostree_syscore_deployment_is_live (sysroot, deployment, &is_live, error))
+            return FALSE;
+
+          if (is_live)
+            changed = TRUE;
+        }
     }
 
   /* let's figure out if those new overrides are valid and if so, canonicalize

tests/check/test-basic.sh

@@ -24,7 +24,7 @@ export RPMOSTREE_SUPPRESS_REQUIRES_ROOT_CHECK=yes
 
 ensure_dbus
 
-echo "1..24"
+echo "1..25"
 
 setup_os_repository "archive-z2" "syslinux"
 
@@ -157,6 +157,14 @@ rpm-ostree rebase --os=testos another-branch
 assert_status_jq '.deployments[0].origin == "another-branch"'
 echo "ok rebase from local branch to local branch"
 
+csum1=$($OSTREE commit -b another-branch --tree=ref=$branch)
+csum2=$($OSTREE commit -b another-branch --tree=ref=$branch)
+rpm-ostree deploy --os=testos $csum1
+if [ "$($OSTREE rev-parse another-branch)" != $csum2 ]; then
+    assert_not_reached "deploying from local branch changes branch"
+fi
+echo "ok local deploy doesn't affect branch"
+
 rpm-ostree rebase --skip-purge --os=testos testos:testos/buildmaster/x86_64-runtime
 assert_status_jq '.deployments[0].origin == "testos:testos/buildmaster/x86_64-runtime"'
 ostree --repo=${test_tmpdir}/testos-repo commit --add-metadata-string version=1.0.2 -b testos/stable/x86_64-runtime -s "Build" \

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"

tests/vmcheck/test-livefs.sh

@@ -110,6 +110,12 @@ vm_cmd cat /etc/test-livefs-with-etc.conf > test-livefs-with-etc.conf
 assert_file_has_content test-livefs-with-etc.conf "custom"
 echo "ok livefs preserved modified config"
 
+vm_rpmostree cleanup -p
+# make sure there's no layering going on somehow
+vm_assert_status_jq '.deployments[0]["base-checksum"]|not'
+vm_rpmostree deploy $(vm_get_booted_deployment_info checksum)
+echo "ok livefs redeploy booted commit"
+
 reset
 vm_rpmostree install /tmp/vmcheck/yumrepo/packages/x86_64/foo-1.0-1.x86_64.rpm
 vm_rpmostree ex livefs