Merge lp:~ted/ubuntu-app-launch/cgrouped-applications into lp:ubuntu-app-launch/14.10

[Ted Gould]

CGroup test and clarification comments

Forgot to rebuild after adding these cleanup revisions, so CI Train didn't merge them.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:195
http://jenkins.qa.ubuntu.com/job/upstart-app-launch-ci/280/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-utopic-amd64-ci/19
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-utopic-armhf-ci/19
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-utopic-i386-ci/19

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/upstart-app-launch-ci/280/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:196
http://jenkins.qa.ubuntu.com/job/upstart-app-launch-ci/285/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-utopic-amd64-ci/24/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-utopic-armhf-ci/24
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-utopic-i386-ci/24

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/upstart-app-launch-ci/285/rebuild

review: Needs Fixing (continuous-integration)

[Charles Kerr (charlesk)]

According to the click-here-to-rebuild ubuntu-cui link, there've been two subsequent builds of this that were both successful, on 2014-08-20 and 2014-08-26, so I'm going to review this as if Jenkins is already happy :-)

Apologies for not inlining the comments...

In cgroup-reap-all.c, it makes more sense to call getpid() and getppid() once each before entering the while() loop, rather than calling them forever.

In cgroup-reap-all.c, it looks to me like maybe pids_from_cgroup() only returns NULL if something went wrong in the DBus call? If that reading is correct, maybe a g_critical() would be appropriate if NULL is returned?

In helpers-shared.c's cgroup_manager_connection()... couldn't we add g_setenv("DBUS_SESSION_BUS_ADDRESS", CGMANAGER_DBUS_PATH) to the test code instead of adding "if (g_getenv("UBUNTU_APP_LAUNCH_CG_MANAGER_SESSION_BUS"))" to the production code?

In helpers-shared.c's pids_from_cgroup()'s g_debug() call, need to use 'groupname ? groupname : ""'

In application-click.conf.in, I don't understand the cahnge to post-start.

In ubuntu-app-launch.c's ubuntu_app_launch_pid_in_app_id()... wow, that implementation gained a lot of scope :)

review: Approve

[Charles Kerr (charlesk)]

That should be "Comment only", rather than "Approve" ... meh.

[Ted Gould (ted)]

On Tue, 2014-08-26 at 19:40 +0000, Charles Kerr wrote:

> In cgroup-reap-all.c, it makes more sense to call getpid() and
> getppid() once each before entering the while() loop, rather than
> calling them forever.

Fixed, r197

> In cgroup-reap-all.c, it looks to me like maybe pids_from_cgroup()
> only returns NULL if something went wrong in the DBus call? If that
> reading is correct, maybe a g_critical() would be appropriate if NULL
> is returned?

I'm thinking no. In reality the reaper's pid should always be in the
list, but that's not explicitly defined behavior in Upstart, so if they
moves the post-stop job out of the cgroup it wouldn't be in the list.
(which would make sense to me actually, but eh, it is how it is)

> In helpers-shared.c's cgroup_manager_connection()... couldn't we add
> g_setenv("DBUS_SESSION_BUS_ADDRESS", CGMANAGER_DBUS_PATH) to the test
> code instead of adding "if
> (g_getenv("UBUNTU_APP_LAUNCH_CG_MANAGER_SESSION_BUS"))" to the
> production code?

Unfortunately we can't because the connection to the cgroup manager
isn't a bus connection, so it needs different auth. So we have to build
the connection slightly differently in the two cases.

> In helpers-shared.c's pids_from_cgroup()'s g_debug() call, need to use
> 'groupname ? groupname : ""'

g_debug() can handle null in that case and will print "(null)" for the
value.

> In application-click.conf.in, I don't understand the cahnge to
> post-start.

What we used to do was send kill to everyone in the pgroup for the main
process. It was a hack to handle the fact that we didn't have cgroup
support. We're now adding cgroup support and removing the hack. To the
reaper replaces the kill.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:196
http://jenkins.qa.ubuntu.com/job/upstart-app-launch-ci/289/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-utopic-amd64-ci/28
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-utopic-armhf-ci/28
    FAILURE: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-utopic-i386-ci/28/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/upstart-app-launch-ci/289/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:197
http://jenkins.qa.ubuntu.com/job/upstart-app-launch-ci/292/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-utopic-amd64-ci/30
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-utopic-armhf-ci/30
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-utopic-i386-ci/30

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/upstart-app-launch-ci/292/rebuild

review: Approve (continuous-integration)

[Charles Kerr (charlesk)]

Makes sense. Thanks for the explanation wrt application-click.conf.in

review: Approve

=== modified file 'cgroup-reap-all.c'
--- cgroup-reap-all.c	2014-08-08 03:58:38 +0000
+++ cgroup-reap-all.c	2014-08-26 20:00:00 +0000
@@ -31,6 +31,9 @@
 	GDBusConnection * cgmanager = cgroup_manager_connection();
 	g_return_val_if_fail(cgmanager != NULL, -1);
 
+	GPid selfpid = getpid();
+	GPid parentpid = getppid();
+
 	/* We're gonna try to kill things forever, literally. It's important
 	   enough that we can't consider failure an option. */
 	gboolean killed = TRUE;
@@ -46,7 +49,7 @@
 			/* We don't want to kill ourselves, or if we're being executed by
 			   a script, that script, either. We also don't want things in our
 			   process group which we forked at the opening */
-			if (pid != getpid() && pid != getppid() && getpgid(pid) != getpid()) {
+			if (pid != selfpid && pid != parentpid && getpgid(pid) != selfpid) {
 				g_debug("Killing pid: %d", pid);
 				kill(pid, SIGKILL);
 				killed = TRUE;

=== modified file 'tests/cgroup-reap-test.cc'
--- tests/cgroup-reap-test.cc	2014-08-08 03:10:26 +0000
+++ tests/cgroup-reap-test.cc	2014-08-26 20:00:00 +0000
@@ -47,6 +47,9 @@
 			g_setenv("UBUNTU_APP_LAUNCH_CG_MANAGER_NAME", "org.test.cgmock", TRUE);
 
 			DbusTestDbusMockObject * cgobject = dbus_test_dbus_mock_get_object(cgmock, "/org/linuxcontainers/cgmanager", "org.linuxcontainers.cgmanager0_0", NULL);
+			/* This Python code executes in dbusmock and checks to see if the sleeping
+			   process is running. If it is, it returns its PID in the list of PIDs, if
+			   not it doesn't return any PIDs. */
 			gchar * pythoncode = g_strdup_printf(
 				"if os.spawnlp(os.P_WAIT, 'ps', 'ps', '%d') == 0 :\n"
 				"  ret = [ %d ]\n"

=== modified file 'tests/manual'
--- tests/manual	2014-08-26 20:00:00 +0000
+++ tests/manual	2014-08-26 20:00:00 +0000
@@ -50,6 +50,17 @@
         <dd>Ensure that all applications start and stop correctly</dd>
 </dl>
 
+Test-case ubuntu-app-launch/cgroup-containment
+<dl>
+    <dt>Install the Terminal app if not already installed</dt>
+        <dd>It can be found in the Application Store if not currently installed</dd>
+    <dt>Launch the Terminal app</dt>
+        <dd>A new application should open with a terminal prompt where you can type commands</dd>
+    <dt>Ensure the Terminal is in a terminal specific cgroup</dt>
+        <dd>Type the command <tt>cat /proc/self/cgroup</tt></dd>
+        <dd>Under the "freeze" cgroup the end of the path should include the AppID of the terminal. An example would be (the version many change): <tt>4:freezer:/user.slice/user-32011.slice/session-c2.scope/upstart/application-click-com.ubuntu.terminal_terminal_0.5.106</tt></dd>
+</dl>
+
 Test-case ubuntu-app-launch/correct-job-type
 <dl>
     <dt>Launch the settings application: <tt>ubuntu-app-launch ubuntu-system-settings</tt></dt>