Skip to content

Commit

Permalink
daemon: Automatically reload sysroot before txn
Browse files Browse the repository at this point in the history
In this PR: #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
  • Loading branch information
cgwalters authored and rh-atomic-bot committed Mar 23, 2018
1 parent 9ddf65a commit 60aa9a4
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 10 deletions.
35 changes: 29 additions & 6 deletions src/daemon/rpmostreed-sysroot.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -691,10 +696,10 @@ rpmostreed_sysroot_class_init (RpmostreedSysrootClass *klass)
gdbus_interface_skeleton_class->g_authorize_method = sysroot_authorize_method;
}

gboolean
rpmostreed_sysroot_reload (RpmostreedSysroot *self,
gboolean *out_changed,
GError **error)
static gboolean
sysroot_reload (RpmostreedSysroot *self,
gboolean *out_changed,
GError **error)
{
gboolean ret = FALSE;
gboolean did_change;
Expand All @@ -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,
Expand All @@ -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;
}

Expand Down Expand Up @@ -796,13 +807,25 @@ 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,
OstreeSysroot **out_sysroot,
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)
Expand Down
1 change: 0 additions & 1 deletion src/daemon/rpmostreed-sysroot.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/daemon/rpmostreed-transaction.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
2 changes: 0 additions & 2 deletions tests/vmcheck/test-basic.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down

0 comments on commit 60aa9a4

Please sign in to comment.