Upgrade agent #1368

cgwalters · 2018-05-15T21:54:11Z

May 15 21:37:19 localhost.localdomain rpm-ostree[4340]: client(id:cli dbus:1.95 unit:session-1.scope uid:0) vanished; remaining=0

The high level goal is to render in a better way what caused an update: coreos#247 (comment) This gets us for Cockpit: `Initiated txn DownloadUpdateRpmDiff for client(dbus:1.28 unit:session-6.scope uid:0): /org/projectatomic/rpmostree1/fedora_atomic` which isn't as good as I'd hoped; I was thinking we'd get `cockpit.service` but actually Cockpit does invocations as a real login for good reason. We get a similar result from the CLI.

This makes the logs a bit more useful, but the ultimate goal here is to write the originating client `id` to the cached update data, so users know that e.g. `gnome-software` triggered it.

cgwalters · 2018-05-16T12:57:54Z

Then e.g.

From 4447b4a47998184486f7b44774f08c2dcfe725d2 Mon Sep 17 00:00:00 2001
From: Colin Walters <walters@verbum.org>
Date: Wed, 16 May 2018 12:56:35 +0000
Subject: [PATCH] rpmostree: Add a client ID

See: https://github.com/projectatomic/rpm-ostree/pull/1368
This will help users understand what software initiated updates.
---
 plugins/rpm-ostree/gs-plugin-rpm-ostree.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/plugins/rpm-ostree/gs-plugin-rpm-ostree.c b/plugins/rpm-ostree/gs-plugin-rpm-ostree.c
index 2e8bda94..5049e558 100644
--- a/plugins/rpm-ostree/gs-plugin-rpm-ostree.c
+++ b/plugins/rpm-ostree/gs-plugin-rpm-ostree.c
@@ -30,6 +30,11 @@
 
 #include "gs-rpmostree-generated.h"
 
+/* This shows up in the `rpm-ostree status` as the software that
+ * initiated the update.
+ */
+#define GS_RPMOSTREE_CLIENT_ID PACKAGE_NAME
+
 struct GsPluginData {
 	GsRPMOSTreeOS		*os_proxy;
 	GsRPMOSTreeSysroot	*sysroot_proxy;
@@ -85,6 +90,7 @@ gboolean
 gs_plugin_setup (GsPlugin *plugin, GCancellable *cancellable, GError **error)
 {
 	GsPluginData *priv = gs_plugin_get_data (plugin);
+	g_autoptr(GVariantBuilder) options_builder = NULL;
 
 	/* Create a proxy for sysroot */
 	if (priv->sysroot_proxy == NULL) {
@@ -127,9 +133,12 @@ gs_plugin_setup (GsPlugin *plugin, GCancellable *cancellable, GError **error)
 		}
 	}
 
+	options_builder = g_variant_builder_new (G_VARIANT_TYPE ("a{sv}"));
+	g_variant_builder_add (options_builder, "{sv}", "id",
+			       g_variant_new_string (GS_RPMOSTREE_CLIENT_ID));
 	/* Register as a client so that the rpm-ostree daemon doesn't exit */
 	if (!gs_rpmostree_sysroot_call_register_client_sync (priv->sysroot_proxy,
-	                                                     g_variant_new ("a{sv}", NULL),
+	                                                     g_variant_new ("a{sv}", g_variant_builder_end (options_builder)),
 	                                                     cancellable,
 	                                                     error)) {
 		gs_utils_error_convert_gio (error);
-- 
2.17.0

jlebon

Nice idea, just a few nits otherwise LGTM!

jlebon · 2018-05-16T14:28:02Z

src/daemon/rpmostreed-daemon.c

@@ -96,26 +100,39 @@ G_DEFINE_TYPE_WITH_CODE (RpmostreedDaemon, rpmostreed_daemon, G_TYPE_OBJECT,
 struct RpmOstreeClient {
  char *address;
  guint name_watch_id;
+  /* In Rust this'd be Option<uid_t> etc. */


jlebon · 2018-05-16T14:41:01Z

src/daemon/rpmostreed-daemon.c

+  if (get_client_pid (self, address, &client->pid))
+    {
+      client->pid_valid = TRUE;
+      if (sd_pid_get_user_unit (client->pid, &client->sd_unit) == 0)


So in your example, this returned session-1.scope for the cli. Would it (or the following call) return e.g. rpm-ostreed-automatic.service for the automatic updates timer?

Yep with this it looks like:

May 16 16:18:56 localhost.localdomain rpm-ostree[3390]: client(id:cli dbus:1.47 unit:rpm-ostreed-automatic.service uid:0) vanished; remaining=0

And now checked in a test ⬇️

jlebon · 2018-05-16T14:43:38Z

src/daemon/org.projectatomic.rpmostree1.xml

@@ -32,7 +32,7 @@
         to either invoke methods on the daemon, or monitor status.
         If no clients are registered, the daemon may exit.

-         No options are currently defined.
+         'name' (type 's') - Package/component name (e.g. `cockpit`, `gnome-software`)


s/name/id/?

Yeah, I noticed that in self-review, had already pushed a fixup.

Ahh right, sigh. I tend to do reviews by looking at each commit one at a time, and I missed the fixup! WDYT if in general we just squash fixups into the commits as long as the PR hasn't been reviewed? Fixups are really helpful for follow-up reviews, but demand extra processing for initial reviews.

jlebon · 2018-05-16T17:49:11Z

tests/vmcheck/test-autoupdate-check.sh

 vm_rpmostree status -v > out.txt
 assert_not_file_has_content out.txt "Available update"
+# And check the unit name https://github.com/projectatomic/rpm-ostree/pull/1368
+vm_cmd journalctl -u rpm-ostreed --after-cursor > journal.txt


Don't we want to pass $cursor here?

Argh, I ran test-autoupdate-stage locally. Fixed ⬇️

jlebon · 2018-05-16T18:11:38Z

@rh-atomic-bot r+ 3eabc96

rh-atomic-bot · 2018-05-16T18:11:43Z

⌛ Testing commit 3eabc96 with merge 99901ac...

This makes the logs a bit more useful, but the ultimate goal here is to write the originating client `id` to the cached update data, so users know that e.g. `gnome-software` triggered it. Closes: #1368 Approved by: jlebon

rh-atomic-bot · 2018-05-16T18:56:06Z

☀️ Test successful - status-atomicjenkins
Approved by: jlebon
Pushing 99901ac to master...

cgwalters · 2018-05-16T20:47:15Z

https://gitlab.gnome.org/GNOME/gnome-software/merge_requests/44

See: coreos/rpm-ostree#1368 This will help users understand what software initiated updates.

cgwalters added 3 commits May 15, 2018 17:01

daemon: Add an 'id' param to RegisterClient, log it

abac65f

This makes the logs a bit more useful, but the ultimate goal here is to write the originating client `id` to the cached update data, so users know that e.g. `gnome-software` triggered it.

fixup! daemon: Add an 'id' param to RegisterClient, log it

912bccf

jlebon reviewed May 16, 2018

View reviewed changes

fixup! daemon: Add an 'id' param to RegisterClient, log it

58cd150

jlebon reviewed May 16, 2018

View reviewed changes

fixup! daemon: Add an 'id' param to RegisterClient, log it

3eabc96

jlebon added the reviewed/approved label May 16, 2018

rh-atomic-bot closed this in 7a7d7ad May 16, 2018

cgwalters mentioned this pull request May 16, 2018

Improve partition semantics coreos/ignition#544

Merged

gnomesysadmins pushed a commit to GNOME/gnome-software that referenced this pull request May 17, 2018

rpmostree: Add a client ID

4447b4a

See: coreos/rpm-ostree#1368 This will help users understand what software initiated updates.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade agent #1368

Upgrade agent #1368

cgwalters commented May 15, 2018

cgwalters commented May 16, 2018

jlebon left a comment

jlebon May 16, 2018

jlebon May 16, 2018

cgwalters May 16, 2018

jlebon May 16, 2018

cgwalters May 16, 2018 •

edited

Loading

jlebon May 16, 2018

jlebon May 16, 2018

cgwalters May 16, 2018

jlebon commented May 16, 2018

rh-atomic-bot commented May 16, 2018

rh-atomic-bot commented May 16, 2018

cgwalters commented May 16, 2018 •

edited

Loading

Upgrade agent #1368

Upgrade agent #1368

Conversation

cgwalters commented May 15, 2018

cgwalters commented May 16, 2018

jlebon left a comment

Choose a reason for hiding this comment

jlebon May 16, 2018

Choose a reason for hiding this comment

jlebon May 16, 2018

Choose a reason for hiding this comment

cgwalters May 16, 2018

Choose a reason for hiding this comment

jlebon May 16, 2018

Choose a reason for hiding this comment

cgwalters May 16, 2018 • edited Loading

Choose a reason for hiding this comment

jlebon May 16, 2018

Choose a reason for hiding this comment

jlebon May 16, 2018

Choose a reason for hiding this comment

cgwalters May 16, 2018

Choose a reason for hiding this comment

jlebon commented May 16, 2018

rh-atomic-bot commented May 16, 2018

rh-atomic-bot commented May 16, 2018

cgwalters commented May 16, 2018 • edited Loading

cgwalters May 16, 2018 •

edited

Loading

cgwalters commented May 16, 2018 •

edited

Loading