daemon: Automatically reload sysroot before txn

In this PR: https://github.com/projectatomic/rpm-ostree/pull/1309
I was hitting race conditions running `ostree admin pin` then
`rpm-ostree cleanup` as it was possible that the daemon hadn't handled
the inotify on the sysroot and reloaded the deployment state before
the txn request came in.

Close this race by doing an implicit `reload` before starting a txn.
This is a pretty efficient operation because for the sysroot we're
just doing a `stat()` and comparing mtime.

Implementation wise, change the external API to drop the "did change"
boolean as nothing outside of the `sysroot.c` file used it.

A followup to this would be changing the `status` CLI to call a
(new) DBus API like `RequestReload` that at least did the sysroot
reload if the daemon was otherwise idle or so?  And it'd be available
to unprivileged users.

Closes: #1311
Approved by: cgwalters

src/daemon/rpmostreed-sysroot.c

@@ -39,6 +39,11 @@
 #include <systemd/sd-login.h>
 #include <systemd/sd-journal.h>
 
+static gboolean
+sysroot_reload (RpmostreedSysroot *self,
+                gboolean *out_changed,
+                GError **error);
+
 /**
  * SECTION: sysroot
  * @title: RpmostreedSysroot
@@ -443,7 +448,7 @@ handle_reload_config (RPMOSTreeSysroot *object,
     goto out;
 
   gboolean sysroot_changed;
-  if (!rpmostreed_sysroot_reload (self, &sysroot_changed, error))
+  if (!sysroot_reload (self, &sysroot_changed, error))
     goto out;
 
   /* also send an UPDATED signal if configs changed to cause OS interfaces to reload; we do
@@ -691,10 +696,10 @@ rpmostreed_sysroot_class_init (RpmostreedSysrootClass *klass)
   gdbus_interface_skeleton_class->g_authorize_method = sysroot_authorize_method;
 }
 
-gboolean
+static gboolean
-rpmostreed_sysroot_reload (RpmostreedSysroot *self,
+sysroot_reload (RpmostreedSysroot *self,
-                           gboolean *out_changed,
+                gboolean *out_changed,
-                           GError **error)
+                GError **error)
 {
   gboolean ret = FALSE;
   gboolean did_change;
@@ -714,6 +719,12 @@ rpmostreed_sysroot_reload (RpmostreedSysroot *self,
   return ret;
 }
 
+gboolean
+rpmostreed_sysroot_reload (RpmostreedSysroot *self, GError **error)
+{
+  return sysroot_reload (self, NULL, error);
+}
+
 static void
 on_deploy_changed (GFileMonitor *monitor,
                    GFile *file,
@@ -726,7 +737,7 @@ on_deploy_changed (GFileMonitor *monitor,
 
   if (event_type == G_FILE_MONITOR_EVENT_ATTRIBUTE_CHANGED)
     {
-      if (!rpmostreed_sysroot_reload (self, NULL, &error))
+      if (!rpmostreed_sysroot_reload (self, &error))
         goto out;
     }
 
@@ -796,6 +807,10 @@ rpmostreed_sysroot_populate (RpmostreedSysroot *self,
   return TRUE;
 }
 
+/* Ensures the sysroot is up to date, and returns references to the underlying
+ * libostree sysroot object as well as the repo.  This function should
+ * be used at the start of both state querying and transactions.
+ */
 gboolean
 rpmostreed_sysroot_load_state (RpmostreedSysroot *self,
                                GCancellable *cancellable,
@@ -803,6 +818,14 @@ rpmostreed_sysroot_load_state (RpmostreedSysroot *self,
                                OstreeRepo **out_repo,
                                GError **error)
 {
+  /* Always do a reload check here to suppress race conditions such as
+   * doing: ostree admin pin && rpm-ostree cleanup
+   * Without this we're relying on the file monitoring picking things up.
+   * Note that the sysroot reload checks mtimes and hence is a cheap
+   * no-op if nothing has changed.
+   */
+  if (!rpmostreed_sysroot_reload (self, error))
+    return FALSE;
   if (out_sysroot)
     *out_sysroot = g_object_ref (rpmostreed_sysroot_get_root (self));
   if (out_repo)

src/daemon/rpmostreed-sysroot.h

@@ -36,7 +36,6 @@ gboolean            rpmostreed_sysroot_populate         (RpmostreedSysroot *self
                                                          GCancellable *cancellable,
                                                          GError **error);
 gboolean            rpmostreed_sysroot_reload           (RpmostreedSysroot *self,
-                                                         gboolean *out_changed,
                                                          GError **error);
 
 OstreeSysroot *     rpmostreed_sysroot_get_root         (RpmostreedSysroot *self);

src/daemon/rpmostreed-transaction.c

@@ -335,7 +335,7 @@ transaction_execute_done_cb (GObject *source_object,
   success = g_task_propagate_boolean (G_TASK (result), &local_error);
   if (success)
     {
-      if (!rpmostreed_sysroot_reload (rpmostreed_sysroot_get (), NULL, &local_error))
+      if (!rpmostreed_sysroot_reload (rpmostreed_sysroot_get (), &local_error))
         success = FALSE;
     }
 

tests/vmcheck/test-basic.sh

@@ -126,11 +126,9 @@ assert_not_file_has_content status.txt "Pinned: yes"
 echo "ok pinning"
 
 vm_cmd ostree admin pin 0
-vm_rpmostree reload  # Try to avoid reload races
 vm_rpmostree cleanup -p
 vm_assert_status_jq ".deployments|length == 2"
 vm_cmd ostree admin pin -u 0
-vm_rpmostree reload  # Try to avoid reload races
 vm_rpmostree cleanup -p
 vm_assert_status_jq ".deployments|length == 1"
 echo "ok pinned retained"