deploy: Retain staged by default

For `rpm-ostree ex livefs` we have a use case of pushing a rollback
deployment.  There's no reason this should require deleting the staged
deployment (and doing so actually breaks livefs which tries to access
it as a data source).

I was initially very conservative here, but I think it ends up
being fairly easy to retain the staged deployment.  We need to handle
two cases:

First, when the staged is *intentionally* deleted; here, we just need
to unlink the `/run` file, and then everything will be sync'd up after
reloading.

Second, (as in the livefs case) where we're retaining it,
e.g. adding a deployment to the end.  What I realized here is that
we can have the code keep `new_deployments` as view without staged,
and then when we do the final reload we'll end up re-reading it from
disk anyways.

Closes: #1672
Approved by: jlebon

diff --git a/src/libostree/ostree-sysroot-deploy.c b/src/libostree/ostree-sysroot-deploy.c
index 42128e72eef126d3e9a2e11607d634aa467fa331..a4387ab264b3c4a423480baa6ced4620c90664f1 100644 (file)
--- a/src/libostree/ostree-sysroot-deploy.c
+++ b/src/libostree/ostree-sysroot-deploy.c
@@ -2182,39 +2182,56 @@ ostree_sysroot_write_deployments_with_options (OstreeSysroot     *self,
 {
   g_assert (self->loaded);
 
-  /* It dramatically simplifies a lot of the logic below if we
-   * drop the staged deployment from both the source deployment list,
-   * as well as the target list.  We don't want to write it to the bootloader
-   * now, which is mostly what this function is concerned with.
-   * In the future we though should probably adapt things to keep it.
+  /* Dealing with the staged deployment is quite tricky here. This function is
+   * primarily concerned with writing out "finalized" deployments which have
+   * bootloader entries. Originally, we simply dropped the staged deployment
+   * here unconditionally. Now, the high level strategy is to retain it, but
+   * *only* if it's the first item in the new deployment list - otherwise, it's
+   * silently dropped.
    */
-  gboolean removed_staged = FALSE;
-  if (self->staged_deployment)
+
+  g_autoptr(GPtrArray) new_deployments_copy = g_ptr_array_new ();
+  gboolean removed_staged = (self->staged_deployment != NULL);
+  if (new_deployments->len > 0)
     {
+      OstreeDeployment *first = new_deployments->pdata[0];
+      /* If the first deployment is the staged, we filter it out for now */
+      g_assert (first);
+      if (first == self->staged_deployment)
+        {
+          g_assert (ostree_deployment_is_staged (first));
+
+          /* In this case note staged was retained */
+          removed_staged = FALSE;
+        }
+
+      /* Create a copy without any staged deployments */
+      for (guint i = 0; i < new_deployments->len; i++)
+        {
+          OstreeDeployment *deployment = new_deployments->pdata[i];
+          if (!ostree_deployment_is_staged (deployment))
+            g_ptr_array_add (new_deployments_copy, deployment);
+        }
+      new_deployments = new_deployments_copy;
+    }
+
+  /* Take care of removing the staged deployment's on-disk state if we should */
+  if (removed_staged)
+    {
+      g_assert (self->staged_deployment);
+      g_assert (self->staged_deployment == self->deployments->pdata[0]);
+
       if (!glnx_unlinkat (AT_FDCWD, _OSTREE_SYSROOT_RUNSTATE_STAGED, 0, error))
         return FALSE;
 
       if (!_ostree_sysroot_rmrf_deployment (self, self->staged_deployment, cancellable, error))
         return FALSE;
 
-      g_assert (self->staged_deployment == self->deployments->pdata[0]);
+      /* Clear it out of the *current* deployments list to maintain invariants */
+      self->staged_deployment = NULL;
       g_ptr_array_remove_index (self->deployments, 0);
-      removed_staged = TRUE;
     }
-  /* First new deployment; we'll see if it's staged */
-  OstreeDeployment *first_new =
-    (new_deployments->len > 0 ? new_deployments->pdata[0] : NULL);
-  g_autoptr(GPtrArray) new_deployments_copy = NULL;
-  if (first_new && ostree_deployment_is_staged (first_new))
-    {
-      g_assert_cmpint (new_deployments->len, >, 0);
-      new_deployments_copy = g_ptr_array_sized_new (new_deployments->len - 1);
-      for (guint i = 1; i < new_deployments->len; i++)
-        g_ptr_array_add (new_deployments_copy, new_deployments->pdata[i]);
-    }
-  else
-    new_deployments_copy = g_ptr_array_ref (new_deployments);
-  new_deployments = new_deployments_copy;
+  const guint nonstaged_current_len = self->deployments->len - (self->staged_deployment ? 1 : 0);
 
   /* Assign a bootserial to each new deployment.
    */
@@ -2226,7 +2243,8 @@ ostree_sysroot_write_deployments_with_options (OstreeSysroot     *self,
    * subbootversion bootlinks.
    */
   gboolean requires_new_bootversion = FALSE;
-  if (new_deployments->len != self->deployments->len)
+
+  if (new_deployments->len != nonstaged_current_len)
     requires_new_bootversion = TRUE;
   else
     {

diff --git a/tests/installed/destructive/staged-deploy.yml b/tests/installed/destructive/staged-deploy.yml
index 8802fb1b650b8a51fe849ff57df2cf3a1549d378..bc53d06d13ecb87afbc0ed075a07a9c4eed2e51b 100644 (file)
--- a/tests/installed/destructive/staged-deploy.yml
+++ b/tests/installed/destructive/staged-deploy.yml
@@ -70,7 +70,7 @@
     grep -vqFe '(staged)' status.txt
     test '!' -f /run/ostree/staged-deployment
 
-- name: Staged should be overwritten by non-staged
+- name: Staged should be overwritten by non-staged as first
   shell: |
     set -xeuo pipefail
     ostree admin deploy --stage staged-deploy
@@ -84,5 +84,19 @@
   environment:
     commit: "{{ rpmostree_status['deployments'][0]['checksum'] }}"
 
+- name: Staged is retained when pushing rollback
+  shell: |
+    set -xeuo pipefail
+    ostree admin deploy --stage staged-deploy
+    test -f /run/ostree/staged-deployment
+    ostree admin deploy --retain-pending --not-as-default nonstaged-deploy
+    test -f /run/ostree/staged-deployment
+    ostree admin status > status.txt
+    grep -qFe '(staged)' status.txt
+    ostree admin undeploy 0
+    ostree admin undeploy 1
+  environment:
+    commit: "{{ rpmostree_status['deployments'][0]['checksum'] }}"
+
 - name: Cleanup refs
   shell: ostree refs --delete staged-deploy nonstaged-deploy