daemon/upgrade: Write out new cached update

Right now, cached updates generated during "check" policy runs are
completely decoupled from upgrade operations. This can lead
to the surprising situation where the "Available update" is *older* than
a freshly deployed pending tree with `rpm-ostree upgrade`.

We should just generate a cached update after upgrade operations. This
is also prep for staged deployments, where we'll want to do this as
well.

Note that we write out the cached update here even if automatic updates
are turned off since it's essentially free. I've been thinking about
always displaying that information after an `rpm-ostree upgrade` in
`status`. Though not sure if we should keep it in a separate "Available
update" section, or somehow morph it as part of the pending deployment
output.

Closes: #1344
Approved by: cgwalters

src/daemon/rpmostreed-deployment-utils.c

@@ -651,32 +651,41 @@ rpm_diff_variant_new (RpmDiff *diff)
 }
 
 static DnfPackage*
-find_newer_package (DnfSack    *sack,
-                    RpmOstreePackage *pkg)
+find_package (DnfSack          *sack,
+              gboolean          newer,
+              RpmOstreePackage *pkg)
 {
   hy_autoquery HyQuery query = hy_query_create (sack);
   hy_query_filter (query, HY_PKG_NAME, HY_EQ, rpm_ostree_package_get_name (pkg));
-  hy_query_filter (query, HY_PKG_EVR, HY_GT, rpm_ostree_package_get_evr (pkg));
-  hy_query_filter (query, HY_PKG_ARCH, HY_NEQ, "src");
-  hy_query_filter_latest (query, TRUE);
-  g_autoptr(GPtrArray) new_pkgs = hy_query_run (query);
-  if (new_pkgs->len == 0)
+  if (newer)
+    {
+      hy_query_filter (query, HY_PKG_EVR, HY_GT, rpm_ostree_package_get_evr (pkg));
+      hy_query_filter (query, HY_PKG_ARCH, HY_NEQ, "src");
+      hy_query_filter_latest (query, TRUE);
+    }
+  else
+    {
+      /* we want an exact match */
+      hy_query_filter (query, HY_PKG_NEVRA, HY_EQ, rpm_ostree_package_get_nevra (pkg));
+    }
+  g_autoptr(GPtrArray) pkgs = hy_query_run (query);
+  if (pkgs->len == 0)
     return NULL; /* canonicalize to NULL */
-  g_ptr_array_sort (new_pkgs, (GCompareFunc)rpmostree_pkg_array_compare);
-  return g_object_ref (new_pkgs->pdata[new_pkgs->len-1]);
+  g_ptr_array_sort (pkgs, (GCompareFunc)rpmostree_pkg_array_compare);
+  return g_object_ref (pkgs->pdata[pkgs->len-1]);
 }
 
 /* For all layered pkgs, check if there are newer versions in the rpmmd. Add diff to
  * @rpm_diff, and all new pkgs in @out_newer_packages (these are used later for advisories).
  * */
 static gboolean
-rpmmd_diff (OstreeRepo       *repo,
-            const char       *base_checksum,
-            const char       *layered_checksum,
-            DnfSack          *sack,
-            RpmDiff          *rpm_diff,
-            GPtrArray       **out_newer_packages,
-            GError          **error)
+rpmmd_diff_guess (OstreeRepo       *repo,
+                  const char       *base_checksum,
+                  const char       *layered_checksum,
+                  DnfSack          *sack,
+                  RpmDiff          *rpm_diff,
+                  GPtrArray       **out_newer_packages,
+                  GError          **error)
 {
   /* Note here that we *don't* actually use layered_pkgs; we want to look at all the RPMs
    * installed, whereas the layered pkgs (actually patterns) just represent top-level
@@ -710,7 +719,7 @@ rpmmd_diff (OstreeRepo       *repo,
   for (guint i = 0; i < all_layered_pkgs->len; i++)
     {
       RpmOstreePackage *pkg = all_layered_pkgs->pdata[i];
-      g_autoptr(DnfPackage) newer_pkg = find_newer_package (sack, pkg);
+      g_autoptr(DnfPackage) newer_pkg = find_package (sack, TRUE, pkg);
       if (!newer_pkg)
         continue;
 
@@ -726,6 +735,58 @@ rpmmd_diff (OstreeRepo       *repo,
   return TRUE;
 }
 
+/* If we have a staged deployment, then those are our new pkgs already. All we need to do is
+ * just find them in the rpmmd for advisory purposes. */
+static gboolean
+rpmmd_diff_exact (OstreeRepo       *repo,
+                  const char       *base_checksum,
+                  const char       *layered_checksum,
+                  DnfSack          *sack,
+                  RpmDiff          *rpm_diff,
+                  GPtrArray       **out_newer_packages,
+                  GError          **error)
+{
+  g_autoptr(GPtrArray) all_layered_pkgs = NULL;
+  RpmOstreeDbDiffExtFlags flags = RPM_OSTREE_DB_DIFF_EXT_ALLOW_NOENT;
+  if (!rpm_ostree_db_diff_ext (repo, base_checksum, layered_checksum, flags, NULL,
+                               &all_layered_pkgs, NULL, NULL, NULL, error))
+    return FALSE;
+
+  if (all_layered_pkgs == NULL || /* -> older layer before we injected pkglist metadata */
+      all_layered_pkgs->len == 0) /* -> no layered pkgs, e.g. override remove only */
+    {
+      *out_newer_packages = NULL;
+      return TRUE; /* note early return */
+    }
+
+  g_autoptr(GPtrArray) newer_packages =
+    g_ptr_array_new_with_free_func ((GDestroyNotify)g_object_unref);
+  for (guint i = 0; i < all_layered_pkgs->len; i++)
+    {
+      RpmOstreePackage *pkg = all_layered_pkgs->pdata[i];
+      g_autoptr(DnfPackage) dnfpkg = find_package (sack, FALSE, pkg);
+      if (!dnfpkg)
+        {
+         /* We *should* be able to always find the pkg since we're probably using the same
+          * rpmmd that was used to derive the layer in the first place. Handle gracefully if
+          * somehow we don't, but log to journal. */
+          sd_journal_print (LOG_WARNING, "Failed to find layered pkg %s in rpmmd.",
+                            rpm_ostree_package_get_nevra (pkg));
+          continue;
+        }
+
+      g_ptr_array_add (newer_packages, g_object_ref (dnfpkg));
+      rpm_diff_add_layered_diff (rpm_diff, pkg, dnfpkg);
+    }
+
+  /* canonicalize to NULL if there's nothing new */
+  if (newer_packages->len == 0)
+    g_clear_pointer (&newer_packages, (GDestroyNotify)g_ptr_array_unref);
+
+  *out_newer_packages = g_steal_pointer (&newer_packages);
+  return TRUE;
+}
+
 /* convert those now to make the D-Bus API nicer and easier for clients */
 static RpmOstreeAdvisorySeverity
 str2severity (const char *str)
@@ -870,17 +931,24 @@ rpm_ostree_pkgs_to_dnf (DnfSack   *sack,
 /* The variant returned by this function is backwards compatible with the one returned by
  * rpmostreed_commit_generate_cached_details_variant(). However, it also includes a base
  * tree db diff, layered pkgs diff, state, advisories, etc... Also, it will happily return
- * NULL if no updates are available. */
+ * %NULL if no updates are available.
+ *
+ * If @staged_deployment is %NULL, update details are based on latest downloaded ostree
+ * rpmmd metadata. If @staged_deployment is not %NULL, then the update describes the diff
+ * between @booted_deployment and @staged_deployment. */
 gboolean
-rpmostreed_update_generate_variant (OstreeDeployment  *deployment,
+rpmostreed_update_generate_variant (OstreeDeployment  *booted_deployment,
+                                    OstreeDeployment  *staged_deployment,
                                     OstreeRepo        *repo,
                                     DnfSack           *sack,
                                     GVariant         **out_update,
                                     GCancellable      *cancellable,
                                     GError           **error)
 {
+  GLNX_AUTO_PREFIX_ERROR ("Generating update variant", error);
+
   g_autoptr(RpmOstreeOrigin) origin =
-    rpmostree_origin_parse_deployment (deployment, error);
+    rpmostree_origin_parse_deployment (booted_deployment, error);
   if (!origin)
     return FALSE;
 
@@ -903,39 +971,62 @@ rpmostreed_update_generate_variant (OstreeDeployment  *deployment,
 
   /* let's start with the ostree side of things */
 
-  g_autofree char *new_checksum = NULL;
-  if (!ostree_repo_resolve_rev_ext (repo, refspec, TRUE, 0, &new_checksum, error))
-    return FALSE;
-
-  const char *current_checksum = ostree_deployment_get_csum (deployment);
+  const char *current_checksum = ostree_deployment_get_csum (booted_deployment);
   const char *current_base_checksum = current_checksum;
-  gboolean is_layered;
   g_autofree char *current_base_checksum_owned = NULL;
-  if (!rpmostree_deployment_get_layered_info (repo, deployment, &is_layered,
+  gboolean is_layered;
+  if (!rpmostree_deployment_get_layered_info (repo, booted_deployment, &is_layered,
                                               &current_base_checksum_owned, NULL, NULL, NULL,
                                               error))
     return FALSE;
   if (is_layered)
     current_base_checksum = current_base_checksum_owned;
 
+  gboolean is_new_layered;
+  const char *new_checksum = NULL;
+  const char *new_base_checksum = NULL;
+  g_autofree char *new_base_checksum_owned = NULL;
+  if (staged_deployment)
+    {
+      new_checksum = ostree_deployment_get_csum (staged_deployment);
+      if (!rpmostree_deployment_get_layered_info (repo, staged_deployment, &is_new_layered,
+                                                  &new_base_checksum_owned, NULL, NULL,
+                                                  NULL, error))
+        return FALSE;
+
+      if (is_new_layered)
+        new_base_checksum = new_base_checksum_owned;
+      else
+        new_base_checksum = new_checksum;
+    }
+  else
+    {
+      if (!ostree_repo_resolve_rev_ext (repo, refspec, TRUE, 0,
+                                        &new_base_checksum_owned, error))
+        return FALSE;
+      new_base_checksum = new_base_checksum_owned;
+      /* just assume that the hypothetical new deployment would also be layered if we are */
+      is_new_layered = is_layered;
+    }
+
   /* Graciously handle rev no longer in repo; e.g. mucking around with rebase/rollback; we
    * still want to do the rpm-md phase. In that case, just use the current csum. */
   gboolean is_new_checksum = FALSE;
-  if (!new_checksum)
-    new_checksum = g_strdup (current_base_checksum);
+  if (!new_base_checksum)
+    new_base_checksum = current_base_checksum;
   else
-    is_new_checksum = !g_str_equal (new_checksum, current_base_checksum);
+    is_new_checksum = !g_str_equal (new_base_checksum, current_base_checksum);
 
   g_autoptr(GVariant) commit = NULL;
-  if (!ostree_repo_load_commit (repo, new_checksum, &commit, NULL, error))
+  if (!ostree_repo_load_commit (repo, new_base_checksum, &commit, NULL, error))
     return FALSE;
 
   g_auto(GVariantDict) dict;
   g_variant_dict_init (&dict, NULL);
 
   /* first get all the traditional/backcompat stuff */
-  if (!add_all_commit_details_to_vardict (deployment, repo, refspec,
-                                          new_checksum, commit, &dict, error))
+  if (!add_all_commit_details_to_vardict (booted_deployment, repo, refspec,
+                                          new_base_checksum, commit, &dict, error))
     return FALSE;
 
   /* This may seem trivial, but it's important to keep the final variant as self-contained
@@ -956,7 +1047,7 @@ rpmostreed_update_generate_variant (OstreeDeployment  *deployment,
 
       /* Note we allow_noent here; we'll just skip over the rpm diff if there's no data */
       RpmOstreeDbDiffExtFlags flags = RPM_OSTREE_DB_DIFF_EXT_ALLOW_NOENT;
-      if (!rpm_ostree_db_diff_ext (repo, current_base_checksum, new_checksum, flags,
+      if (!rpm_ostree_db_diff_ext (repo, current_base_checksum, new_base_checksum, flags,
                                    &removed, &added, &modified_old, &ostree_modified_new,
                                    cancellable, error))
         return FALSE;
@@ -973,11 +1064,21 @@ rpmostreed_update_generate_variant (OstreeDeployment  *deployment,
 
   GHashTable *layered_pkgs = rpmostree_origin_get_packages (origin);
   /* check that it's actually layered (i.e. the requests are not all just dormant) */
-  if (sack && is_layered && g_hash_table_size (layered_pkgs) > 0)
+  if (sack && is_new_layered && g_hash_table_size (layered_pkgs) > 0)
     {
-      if (!rpmmd_diff (repo, current_base_checksum, current_checksum, sack, &rpm_diff,
-                       &rpmmd_modified_new, error))
-        return FALSE;
+      if (staged_deployment)
+        {
+          /* no need to guess, we *know* what the new layered pkgs are */
+          if (!rpmmd_diff_exact (repo, new_base_checksum, new_checksum, sack, &rpm_diff,
+                                 &rpmmd_modified_new, error))
+            return FALSE;
+        }
+      else
+        {
+          if (!rpmmd_diff_guess (repo, current_base_checksum, current_checksum, sack,
+                                 &rpm_diff, &rpmmd_modified_new, error))
+            return FALSE;
+        }
     }
 
   /* don't bother inserting if there's nothing new */
@@ -1012,6 +1113,8 @@ rpmostreed_update_generate_variant (OstreeDeployment  *deployment,
         g_variant_dict_insert (&dict, "advisories", "@a(suuasa{sv})", advisories);
     }
 
+  g_variant_dict_insert (&dict, "staged", "b", staged_deployment != NULL);
+
   /* but if there are no updates, then just ditch the whole thing and return NULL */
   if (is_new_checksum || rpmmd_modified_new)
     {

src/daemon/rpmostreed-deployment-utils.h

@@ -47,7 +47,8 @@ GVariant *      rpmostreed_commit_generate_cached_details_variant (OstreeDeploym
                                                                    const char       *checksum,
                                                                    GError          **error);
 
-gboolean        rpmostreed_update_generate_variant (OstreeDeployment  *deployment,
+gboolean        rpmostreed_update_generate_variant (OstreeDeployment  *booted_deployment,
+                                                    OstreeDeployment  *staged_deployment,
                                                     OstreeRepo        *repo,
                                                     DnfSack           *sack,
                                                     GVariant         **out_update,

src/daemon/rpmostreed-transaction-types.c

@@ -196,7 +196,8 @@ apply_revision_override (RpmostreedTransaction    *transaction,
  * https://github.com/projectatomic/rpm-ostree/pull/1268 */
 static gboolean
 generate_update_variant (OstreeRepo       *repo,
-                         OstreeDeployment *deployment,
+                         OstreeDeployment *booted_deployment,
+                         OstreeDeployment *staged_deployment,
                          DnfSack          *sack,
                          GCancellable     *cancellable,
                          GError          **error)
@@ -212,8 +213,8 @@ generate_update_variant (OstreeRepo       *repo,
     return FALSE;
 
   g_autoptr(GVariant) update = NULL;
-  if (!rpmostreed_update_generate_variant (deployment, repo, sack, &update,
-                                           cancellable, error))
+  if (!rpmostreed_update_generate_variant (booted_deployment, staged_deployment, repo, sack,
+                                           &update, cancellable, error))
     return FALSE;
 
   if (update != NULL)
@@ -342,7 +343,8 @@ package_diff_transaction_execute (RpmostreedTransaction *transaction,
        * that's all we updated here. This conflicts with auto-updates for now, though we
        * need better test coverage before uniting those two paths. */
       OstreeDeployment *booted_deployment = ostree_sysroot_get_booted_deployment (sysroot);
-      if (!generate_update_variant (repo, booted_deployment, NULL, cancellable, error))
+      if (!generate_update_variant (repo, booted_deployment, NULL, NULL,
+                                    cancellable, error))
         return FALSE;
     }
 
@@ -833,6 +835,7 @@ deploy_transaction_execute (RpmostreedTransaction *transaction,
     }
 
   /* https://github.com/projectatomic/rpm-ostree/issues/454 */
+  gboolean is_upgrade = FALSE;
   g_autoptr(GString) txn_title = g_string_new ("");
   if (is_install)
     g_string_append (txn_title, "install");
@@ -843,7 +846,10 @@ deploy_transaction_execute (RpmostreedTransaction *transaction,
   else if (self->revision)
     g_string_append (txn_title, "deploy");
   else
-    g_string_append (txn_title, "upgrade");
+    {
+      is_upgrade = TRUE; /* XXX: strengthen how we determine this */
+      g_string_append (txn_title, "upgrade");
+    }
 
   /* so users know we were probably fired by the automated timer when looking at status */
   if (cache_only)
@@ -1101,9 +1107,9 @@ deploy_transaction_execute (RpmostreedTransaction *transaction,
         return glnx_throw (error, "Refusing to download rpm-md for offline OS '%s'",
                            self->osname);
 
-      g_autoptr(DnfSack) sack = NULL;
 
       /* XXX: in rojig mode we'll want to do this unconditionally */
+      g_autoptr(DnfSack) sack = NULL;
       if (g_hash_table_size (rpmostree_origin_get_packages (origin)) > 0)
         {
           /* we always want to force a refetch of the metadata */
@@ -1112,7 +1118,8 @@ deploy_transaction_execute (RpmostreedTransaction *transaction,
             return FALSE;
         }
 
-      if (!generate_update_variant (repo, booted_deployment, sack, cancellable, error))
+      if (!generate_update_variant (repo, booted_deployment, NULL, sack,
+                                    cancellable, error))
         return FALSE;
 
       /* Note early return */
@@ -1153,7 +1160,9 @@ deploy_transaction_execute (RpmostreedTransaction *transaction,
           return TRUE;
         }
 
-      if (!rpmostree_sysroot_upgrader_deploy (upgrader, NULL, cancellable, error))
+      g_autoptr(OstreeDeployment) new_deployment = NULL;
+      if (!rpmostree_sysroot_upgrader_deploy (upgrader, &new_deployment,
+                                              cancellable, error))
         return FALSE;
 
       /* Are we rebasing?  May want to delete the previous ref */
@@ -1173,6 +1182,28 @@ deploy_transaction_execute (RpmostreedTransaction *transaction,
             }
         }
 
+      /* Always write out an update variant on vanilla upgrades since it's clearly the most
+       * up to date. If autoupdates "check" mode is enabled, the *next* run might yet
+       * overwrite it again because we always diff against the booted deployment. */
+      if (is_upgrade)
+        {
+          OstreeDeployment *booted_deployment =
+            ostree_sysroot_get_booted_deployment (sysroot);
+
+          g_autoptr(DnfSack) sack = NULL;
+          if (g_hash_table_size (rpmostree_origin_get_packages (origin)) > 0)
+            {
+              /* don't force a refresh; we want the same sack state used by the core */
+              if (!get_sack_for_booted (sysroot, repo, booted_deployment, FALSE, &sack,
+                                        cancellable, error))
+                return FALSE;
+            }
+
+          if (!generate_update_variant (repo, booted_deployment, new_deployment, sack,
+                                        cancellable, error))
+            return FALSE;
+        }
+
       if (self->flags & RPMOSTREE_TRANSACTION_DEPLOY_FLAG_REBOOT)
         rpmostreed_reboot (cancellable, error);
     }

tests/vmcheck/test-autoupdate.sh

@@ -231,6 +231,10 @@ assert_default_deployment_is_update() {
 }
 
 # now let's upgrade and check that it matches what we expect
+# (but start from scratch to check that vanilla `upgrade` also builds the cache)
+vm_rpmostree cleanup -m
+vm_cmd systemctl stop rpm-ostreed
 vm_rpmostree upgrade
+assert_output2
 assert_default_deployment_is_update
 echo "ok upgrade"