daemon: Exit on idle after txn #606

cgwalters · 2017-02-04T14:00:16Z

So...we have some bad memory leaks somewhere. We go from 14MB RSS after being
activated from atomic host status to 54MB after a dummy rpm-ostree install foo, 80MB for a successful install, and I've seen it as high as 140MB.

Exiting on idle is what PackageKit has done forever. One problem particularly
for both of us is that libdnf and all of the libraries underneath it are used
almost exclusively by command line clients that exit when done, so memory leaks
aren't particularly important to them.

This may break Cockpit though, since it won't be able to monitor for sysroot
changes. I considered instead running txns in a subprocess, which is possible
but harder.

cgwalters · 2017-02-04T14:00:43Z

This code all works, but marking as WIP.

cgwalters · 2017-02-12T19:52:45Z

@petervo From looking at the cockpit code, it seems like this would result in an error message? If so, how hard would it be to adjust the code to handle it?

petervo · 2017-02-12T23:30:51Z

@cgwalters way back when, the original WIP version of this had this in, but we took it out in review because of races. I think cockpit will handle this correctly and shouldn't cause an error. In any case I agree that's it's good to have so when it hits centos continuous, i'll make sure everything continues to work as expected.

cgwalters · 2017-02-13T16:25:31Z

I think I need to revisit https://lists.freedesktop.org/archives/dbus/2015-May/016671.html and at least try to adapt things here for that.

rh-atomic-bot · 2017-03-06T17:13:54Z

☔ The latest upstream changes (presumably f9944e6) made this pull request unmergeable. Please resolve the merge conflicts.

cgwalters · 2017-03-07T18:30:22Z

Ok, dependencies in #660

This should be good to go now. Removing WIP.

rh-atomic-bot · 2017-03-07T23:31:42Z

☔ The latest upstream changes (presumably 7cf3664) made this pull request unmergeable. Please resolve the merge conflicts.

cgwalters · 2017-03-08T01:47:06Z

OK, rebased again. Not totally sure whether to land this right now though...maybe we make it a config option?

jlebon

Hmm, wait we're missing a transition to APPSTATE_FLUSHING here, no?

diff --git a/src/app/rpmostree-builtin-start-daemon.c b/src/app/rpmostree-builtin-start-daemon.c
index 9bbcbd1..91dd817 100644
--- a/src/app/rpmostree-builtin-start-daemon.c
+++ b/src/app/rpmostree-builtin-start-daemon.c
@@ -382,6 +387,8 @@ rpmostree_builtin_start_daemon (int             argc,
        *  https://github.com/cgwalters/test-exit-on-idle
        */
       sd_notify (FALSE, "STOPPING=1");
+      state_transition (APPSTATE_FLUSHING);
       g_dbus_connection_call (bus,
                               "org.freedesktop.DBus", "/org/freedesktop/DBus",
                               "org.freedesktop.DBus", "ReleaseName",

Also, it doesn't seem like STOPPING=1 causes systemd to start up a new instance. E.g. if I do:

diff --git a/src/app/rpmostree-builtin-start-daemon.c b/src/app/rpmostree-builtin-start-daemon.c
index 9bbcbd1..81eccc7 100644
--- a/src/app/rpmostree-builtin-start-daemon.c
+++ b/src/app/rpmostree-builtin-start-daemon.c
@@ -372,6 +372,11 @@ rpmostree_builtin_start_daemon (int             argc,
   g_debug ("Entering main event loop");
   rpmostreed_daemon_run_until_idle_exit (rpm_ostree_daemon);

+ /* note that STOPPING=1 was already done in the idle exit */
+  sd_journal_print (LOG_INFO, "sleeping for race");
+  sleep (10);
+
   if (bus)
     {
       /* We first tell systemd we're stopping, so it knows to activate a new instance

And run rpm-ostree status during the race window, I get blocked until the daemon exits (which is expected), and then get:

error: Rejected send message, 1 matched rules; type="method_call", sender=":1.117" (uid=0 pid=22345 comm="rpm-ostree status ") interface="org.projectatomic.rpmostree1.Sysroot" member="RegisterClient" error name="(unset)" requested_reply="0" destination=":1.116" (uid=0 pid=22316 comm="/usr/bin/rpm-ostree start-daemon ")

OTOH, if I instead put the race window after ReleaseName, I still get blocked, but I then get served by the new instance, which I think is what we want to happen before as well, right?

jlebon · 2017-03-09T20:43:59Z

src/daemon/rpmostreed-daemon.c

+
+  sd_notifyf (0, "STOPPING=1\nSTATUS=Exiting due to idle");
+  self->running = FALSE;
+  g_main_context_wakeup (NULL);


Do we actually need this though? This is already running as part of the main context, so isn't it already woken up?

Yep, we don't need it here. However, if one is doing multithreading, the main loop does need to be woken up. So I always add this as part of the "manage mainloop termination condition" pattern. It's just a write() to an eventfd() underneath so it's cheap.

cgwalters · 2017-03-10T16:20:51Z

Hmm, wait we're missing a transition to APPSTATE_FLUSHING here, no?

Urgh, yes. Good catch. I'm going to need to squash together the idle exit/mainloop logic into one file really. I guess rpmostreed-daemon.c?

cgwalters · 2017-04-21T01:49:37Z

RE the dbus error...I think the problem is bus policy won't let messages through to us anymore since we don't own the name. To fix this we need to do #745

rh-atomic-bot · 2017-06-22T21:50:38Z

☔ The latest upstream changes (presumably 675066a) made this pull request unmergeable. Please resolve the merge conflicts.

cgwalters · 2017-08-16T19:24:11Z

Note to self: from looking at massif briefly, the vast majority of the memory usage goes into libsolv. I'm not sure I really want to debug that code. Better to make txns a subprocess at least (#897) or do this.

@jlebon

This closes a race condition for having the daemon idle exit. After the daemon has released its bus name, the dbus-daemon will no longer allow messages through that targeted its unique name. Since the intention of the `RegisterClient` method is to be the "knock on the door", fix this by directly sending a message to the well-known name. Second, we need to handle the case where the daemon exits without replying; @jlebon added a `sleep(10)` invocation after the daemon mainloop quit but before we `ReleaseName`, and I verified these two things combine to fix that case.

Now that we have the ability to both track clients and our active transaction, and the `RegisterClient` call acts "atomically", let's start doing exit-on-idle and return the RAM to the people.

cgwalters · 2017-09-28T00:21:20Z

Rebased 🏄 and fixed up! I figured out a way to close the race you found; see the first commit.

jlebon

Just some minor nits, otherwise looks good! I tried add a sleep in the same places and got appropriate behaviour in all of them.

jlebon · 2017-10-02T21:19:59Z

src/app/rpmostree-dbus-helpers.c

+        break;  /* Success! */
+      else
+        {
+          if (g_dbus_error_is_remote_error (local_error))


Nit: we can de-indent all of this by just dropping the else.

jlebon · 2017-10-02T21:31:11Z

src/app/rpmostree-builtin-start-daemon.c

+       * FLUSHING right now, let's just forcibly do so if it hasn't been done
+       * already.
+       */
+      if (appstate <= APPSTATE_FLUSHING)


This should technically be strictly <, right?

jlebon · 2017-10-02T21:50:13Z

src/daemon/rpmostreed-daemon.c

+
+  g_clear_pointer (&self->idle_exit_source, (GDestroyNotify)g_source_unref);
+
+  sd_notifyf (0, "STOPPING=1\nSTATUS=Exiting due to idle");


Let's just drop the STOPPING=1 here? We already do this once we exit the loop and it covers all other possible non-idle-exit scenarios as well.

cgwalters · 2017-10-03T16:05:13Z

Nice review, thanks! Fixups ⬆️

jlebon · 2017-10-03T17:56:18Z

Nice! @rh-atomic-bot r+ 2cda8d7

rh-atomic-bot · 2017-10-03T17:56:24Z

⚡ Test exempted: merge already tested.

Now that we have the ability to both track clients and our active transaction, and the `RegisterClient` call acts "atomically", let's start doing exit-on-idle and return the RAM to the people. Closes: #606 Approved by: jlebon

cgwalters added the WIP label Feb 4, 2017

cgwalters added WIP and removed WIP labels Feb 13, 2017

This was referenced Mar 6, 2017

Idle exit prep prep #659

Closed

Idle exit prep #660

Closed

cgwalters force-pushed the idle-exit branch from ea0bb53 to 0122cb4 Compare March 7, 2017 18:28

cgwalters removed the WIP label Mar 7, 2017

cgwalters force-pushed the idle-exit branch from 3a0db48 to 8849a4c Compare March 8, 2017 01:46

jlebon reviewed Mar 9, 2017

View reviewed changes

cgwalters added the reviewed/needs-rework label Mar 10, 2017

cgwalters mentioned this pull request Jul 27, 2017

rpm-ostree ignores CTRL-C while downloading rpm-md or rpms #897

Open

cgwalters force-pushed the idle-exit branch from 8849a4c to 338ec2a Compare September 27, 2017 21:28

cgwalters added 2 commits September 27, 2017 20:18

daemon: Exit on idle after ~10-15 seconds

bb0d5bb

Now that we have the ability to both track clients and our active transaction, and the `RegisterClient` call acts "atomically", let's start doing exit-on-idle and return the RAM to the people.

cgwalters force-pushed the idle-exit branch from 338ec2a to bb0d5bb Compare September 28, 2017 00:20

cgwalters removed the reviewed/needs-rework label Sep 28, 2017

jlebon reviewed Oct 3, 2017

View reviewed changes

cgwalters added 2 commits October 3, 2017 11:58

fixup! app: Call RegisterClient with well-known name, use retry loop

af3bc36

fixup! daemon: Exit on idle after ~10-15 seconds

1c83272

fixup! app: Call RegisterClient with well-known name, use retry loop

2cda8d7

rh-atomic-bot closed this in d31c9da Oct 3, 2017

jlebon added the reviewed/approved label Oct 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

daemon: Exit on idle after txn #606

daemon: Exit on idle after txn #606

cgwalters commented Feb 4, 2017

cgwalters commented Feb 4, 2017

cgwalters commented Feb 12, 2017

petervo commented Feb 12, 2017

cgwalters commented Feb 13, 2017

rh-atomic-bot commented Mar 6, 2017

cgwalters commented Mar 7, 2017

rh-atomic-bot commented Mar 7, 2017

cgwalters commented Mar 8, 2017

jlebon left a comment

jlebon Mar 9, 2017

cgwalters Mar 10, 2017

cgwalters commented Mar 10, 2017

cgwalters commented Apr 21, 2017

rh-atomic-bot commented Jun 22, 2017

cgwalters commented Aug 16, 2017

cgwalters commented Sep 28, 2017

jlebon left a comment

jlebon Oct 2, 2017

jlebon Oct 2, 2017

jlebon Oct 2, 2017

cgwalters commented Oct 3, 2017

jlebon commented Oct 3, 2017

rh-atomic-bot commented Oct 3, 2017


		g_clear_pointer (&self->idle_exit_source, (GDestroyNotify)g_source_unref);

		sd_notifyf (0, "STOPPING=1\nSTATUS=Exiting due to idle");

daemon: Exit on idle after txn #606

daemon: Exit on idle after txn #606

Conversation

cgwalters commented Feb 4, 2017

cgwalters commented Feb 4, 2017

cgwalters commented Feb 12, 2017

petervo commented Feb 12, 2017

cgwalters commented Feb 13, 2017

rh-atomic-bot commented Mar 6, 2017

cgwalters commented Mar 7, 2017

rh-atomic-bot commented Mar 7, 2017

cgwalters commented Mar 8, 2017

jlebon left a comment

Choose a reason for hiding this comment

jlebon Mar 9, 2017

Choose a reason for hiding this comment

cgwalters Mar 10, 2017

Choose a reason for hiding this comment

cgwalters commented Mar 10, 2017

cgwalters commented Apr 21, 2017

rh-atomic-bot commented Jun 22, 2017

cgwalters commented Aug 16, 2017

cgwalters commented Sep 28, 2017

jlebon left a comment

Choose a reason for hiding this comment

jlebon Oct 2, 2017

Choose a reason for hiding this comment

jlebon Oct 2, 2017

Choose a reason for hiding this comment

jlebon Oct 2, 2017

Choose a reason for hiding this comment

cgwalters commented Oct 3, 2017

jlebon commented Oct 3, 2017

rh-atomic-bot commented Oct 3, 2017