Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

daemon: Add new Reload D-Bus method #1311

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions src/app/rpmostree-dbus-helpers.c
Original file line number Diff line number Diff line change
Expand Up @@ -689,6 +689,16 @@ rpmostree_transaction_connect_active (RPMOSTreeSysroot *sysroot_proxy,
return FALSE;
}

static void
on_reload_done (GObject *src,
GAsyncResult *res,
gpointer user_data)
{
gboolean *donep = user_data;
*donep = TRUE;
(void) rpmostree_sysroot_call_reload_finish ((RPMOSTreeSysroot*)src, res, NULL);
}

/* Transactions need an explicit Start call so we can set up watches for signals
* beforehand and avoid losing information. We monitor the transaction,
* printing output it sends, and handle Ctrl-C, etc.
Expand Down Expand Up @@ -784,6 +794,16 @@ rpmostree_transaction_get_response_sync (RPMOSTreeSysroot *sysroot_proxy,
}
}

/* On success, call Reload() as a way to sync with the daemon. Do this in async mode so
* that gdbus handles signals for changed properties. */
if (success)
{
gboolean done = FALSE;
rpmostree_sysroot_call_reload (sysroot_proxy, NULL, on_reload_done, &done);
while (!done)
g_main_context_iteration (NULL, TRUE);
}

out:
if (sigintid)
g_source_remove (sigintid);
Expand Down
6 changes: 6 additions & 0 deletions src/daemon/org.projectatomic.rpmostree1.xml
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@
<arg type="a{sv}" name="options" direction="in"/>
</method>

<!-- Reload sysroot if changed. This can also be used as a way to sync with the daemon
to ensure e.g. D-Bus properties are updated before reading them. -->
<method name="Reload">
</method>

<!-- Like Reload, but also reload configuration files. -->
<method name="ReloadConfig">
</method>

Expand Down
64 changes: 56 additions & 8 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_ostree_configs_and_deployments (RpmostreedSysroot *self,
gboolean *out_changed,
GError **error);

/**
* SECTION: sysroot
* @title: RpmostreedSysroot
Expand Down Expand Up @@ -427,6 +432,29 @@ reset_config_properties (RpmostreedSysroot *self,
return TRUE;
}

/* reloads *only* deployments and os internals, *no* configuration files */
static gboolean
handle_reload (RPMOSTreeSysroot *object,
GDBusMethodInvocation *invocation)
{
RpmostreedSysroot *self = RPMOSTREED_SYSROOT (object);
g_autoptr(GError) local_error = NULL;

if (!sysroot_populate_deployments_unlocked (self, NULL, &local_error))
goto out;

/* always send an UPDATED signal to also force OS interfaces to reload */
g_signal_emit (self, signals[UPDATED], 0);

rpmostree_sysroot_complete_reload (object, invocation);

out:
if (local_error)
g_dbus_method_invocation_take_error (invocation, g_steal_pointer (&local_error));
return TRUE;
}

/* reloads *everything*: ostree configs, rpm-ostreed.conf, deployments, os internals */
static gboolean
handle_reload_config (RPMOSTreeSysroot *object,
GDBusMethodInvocation *invocation)
Expand All @@ -443,7 +471,7 @@ handle_reload_config (RPMOSTreeSysroot *object,
goto out;

gboolean sysroot_changed;
if (!rpmostreed_sysroot_reload (self, &sysroot_changed, error))
if (!sysroot_reload_ostree_configs_and_deployments (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 @@ -595,9 +623,10 @@ sysroot_authorize_method (GDBusInterfaceSkeleton *interface,
/* The daemon is on the session bus, running self tests */
authorized = TRUE;
}
else if (g_strcmp0 (method_name, "GetOS") == 0)
else if (g_strcmp0 (method_name, "GetOS") == 0 ||
g_strcmp0 (method_name, "Reload") == 0)
{
/* GetOS() is always allowed */
/* GetOS() and Reload() are always allowed */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things are weird now because this is identical to ReloadConfig right? The way I was thinking about this is we wouldn't reread the remotes in /etc, but we would do the "stat /sysroot for mtime" check.

The rationale I have for this is an admin might be in the middle of editing a config file in /etc, and having some other unprivileged software come in and read the half-written config file is exactly the problem we're avoiding by having an explicit reload command.

So internally we'd split reload_sysroot() from reload_sysroot_and_config()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reload only does sysroot_reload + signal right? ReloadConfig does that and rereads config files.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK right, I was slightly confused, it is doing ostree_repo_reload_config but...eh that's all right.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No actually that one will reload the remotes too, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh, right. It's confusing because we have three configs: repo config, remotes, and rpm-ostreed.conf. :) I renamed sysroot_reload to be more obvious about this and added some comments! ⬇️

authorized = TRUE;
}
else if (g_strcmp0 (method_name, "ReloadConfig") == 0)
Expand Down Expand Up @@ -691,10 +720,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_ostree_configs_and_deployments (RpmostreedSysroot *self,
gboolean *out_changed,
GError **error)
{
gboolean ret = FALSE;
gboolean did_change;
Expand All @@ -714,6 +743,12 @@ rpmostreed_sysroot_reload (RpmostreedSysroot *self,
return ret;
}

gboolean
rpmostreed_sysroot_reload (RpmostreedSysroot *self, GError **error)
{
return sysroot_reload_ostree_configs_and_deployments (self, NULL, error);
}

static void
on_deploy_changed (GFileMonitor *monitor,
GFile *file,
Expand All @@ -726,7 +761,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 All @@ -742,6 +777,7 @@ rpmostreed_sysroot_iface_init (RPMOSTreeSysrootIface *iface)
iface->handle_get_os = handle_get_os;
iface->handle_register_client = handle_register_client;
iface->handle_unregister_client = handle_unregister_client;
iface->handle_reload = handle_reload;
iface->handle_reload_config = handle_reload_config;
}

Expand Down Expand Up @@ -796,13 +832,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
3 changes: 3 additions & 0 deletions tests/vmcheck/test-autoupdate.sh
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,9 @@ assert_output
echo "ok check mode layered only with advisories"

# check we see the same output with --check/--preview
# clear out cache first to make sure they start from scratch
vm_rpmostree cleanup -m
vm_cmd systemctl stop rpm-ostreed
vm_rpmostree upgrade --check > out.txt
vm_rpmostree upgrade --preview > out-verbose.txt
assert_output
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