user-cr: Extra unshare() calls ?

Came across this while testing LXC.

1. Does ckpt_remount_proc() need to unshare() ? Or can we have the clone() that calls __ckpt_coordinator() clone with CLONE_NEWNS|CLONE_FS instead ?

The problem with the unshare() in ckpt_remount_proc() is that it creates an extra level in cgroup hierarchy (see below) after restart. So applications expecting the cgroup hierarchy before chckpoint will be surprised.

2. When --mount-pty (or --mntns) is specified, do we need to unshare() in the parent process ? Considering only the full-container restart for now (ignore self-restart and subtree restart), can we just specify (CLONE_NEWNS|CLONE_FS) at the time of creating the first restarted process ?

Here is an example (using LXC) that shows the problems I am running into Attached is a quick hack to point out the unshare() calls I am referring to.

If I create a simple container with LXC

	$ lxc-execute --name foo --rcfile lxc-macvlan.conf -- /bin/sleep 1000

It creates the following three processes:

	PID   PPID  CMD

        3350  3239  lxc-execute --name foo -- /bin/sleep 1000
        3353  3350  /usr/local/libexec/lxc-init -- /bin/sleep 1000
        3357  3353  /bin/sleep 1000

A new cgroup is created named 'foo' (which is basically a user-space rename of the pid of the lxc-init). This cgroup is in the root cgroup directory and has two tasks (lxc-init, sleep)

        $ cat /cgroup/foo/tasks
        3353
        3357

When I checkpoint and restart this container (using the equivalent of --pidns --pids --mount-pty options to /bin/restart). I get three processes:

        3434  3375  ./lxc_restart --name bar --statefile=/root/foo.ckpt
        3436  3434  /usr/local/libexec/lxc-init -- /bin/sleep 1000
        3437  3436  /bin/sleep 1000

But the directory in /cgroup referring to lxc-init is 3 levels deep:

	ls /cgroup/3434/3436/1
	cgroup.procs  freezer.state  notify_on_release  tasks

Here is the complete hierarchy created after the restart:

        $ ls -R /cgroup/3434
        /cgroup/3434:
        3436  cgroup.procs  freezer.state  notify_on_release  tasks

        /cgroup/3434/3436:
        1  cgroup.procs  freezer.state  notify_on_release  tasks

        /cgroup/3434/3436/1:
        cgroup.procs  freezer.state  notify_on_release  tasks

        $ cat /cgroup/3434/tasks
        3434

        $ cat /cgroup/3434/3436/tasks   # empty

        $ cat /cgroup/3434/3436/1/tasks
        3436
        3437

I think we get the directory /cgroup/3434 due to the following unshare()

		/* private mounts namespace ? */
		if (args->mntns && unshare(CLONE_NEWNS | CLONE_FS) < 0) {
			ckpt_perror("unshare");
			exit(1);
		}

And we get the "3436/1" directory due to the unshare() in ckpt_remount_proc().

Following hack seems to fix both the levels and the lxc_restart command correctly creates just the "/cgroup/3436" (which LXC renames to "/cgroup/bar" cgroup).

---
From: Sukadev Bhattiprolu <[email protected]>
Date: Mon, 8 Mar 2010 12:03:46 -0800
Subject: [PATCH 1/1] Minimize unshare() calls

---
 restart.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

index c82de21..6ac51e3 100644
@@ -459,10 +459,12 @@ int app_restart(struct app_restart_args *args)
 		exit(1);
 
 	/* private mounts namespace ? */
+#if 0
 	if (args->mntns && unshare(CLONE_NEWNS | CLONE_FS) < 0) {
 		ckpt_perror("unshare");
 		exit(1);
 	}
+#endif
 
 	/* chroot ? */
 	if (args->root && chroot(args->root) < 0) {

@@ -717,10 +719,12 @@ static int ckpt_probe_child(pid_t pid, char *str)
  */
 static int ckpt_remount_proc(struct ckpt_ctx *ctx)
 {
+#if 0
 	if (unshare(CLONE_NEWNS | CLONE_FS) < 0) {
 		ckpt_perror("unshare");
 		return -1;
 	}
+#endif
 	/* this is unlikely, but we don't want to fail */
 	if (umount2("/proc", MNT_DETACH) < 0) {
 		if (ckpt_cond_fail(ctx, CKPT_COND_MNTPROC)) {

@@ -778,6 +782,7 @@ static int ckpt_coordinator_pidns(struct ckpt_ctx *ctx)
 	int copy, ret;
 	genstack stk;
 	void *sp;
+	unsigned long flags = SIGCHLD;
 
 	ckpt_dbg("forking coordinator in new pidns\n");
 

@@ -802,7 +807,9 @@ static int ckpt_coordinator_pidns(struct ckpt_ctx *ctx)
 	copy = ctx->args->copy_status;
 	ctx->args->copy_status = 1;
 
-	coord_pid = clone(__ckpt_coordinator, sp, CLONE_NEWPID|SIGCHLD, ctx);
+	flags |= CLONE_NEWPID|CLONE_NEWNS|CLONE_FS;
+
+	coord_pid = clone(__ckpt_coordinator, sp, flags, ctx);
 	genstack_release(stk);
 	if (coord_pid < 0) {
 		ckpt_perror("clone coordinator");

1.6.6.1

And then move remounting of devpts into ckpt_remount_proc() called from __ckpt_coordinator() as well?

-serge

Serge E. Hallyn [serue-r/[email protected]] wrote:

| Quoting Sukadev Bhattiprolu ([email protected]):
| > 
| > Came across this while testing LXC.
| > 
| > 
| > 1. Does ckpt_remount_proc() need to unshare() ? Or can we have the
| >    clone() that calls __ckpt_coordinator() clone with CLONE_NEWNS|CLONE_FS
| >    instead ?
| > 
| >    The problem with the unshare() in ckpt_remount_proc() is that it
| >    creates an extra level in cgroup hierarchy (see below) after restart.
| >    So applications expecting the cgroup hierarchy before chckpoint will
| >    be surprised.
| > 
| > 2. When --mount-pty (or --mntns) is specified, do we need to unshare() 
| >    in the parent process ? Considering only the full-container restart
| >    for now (ignore self-restart and subtree restart), can we just
| >    specify (CLONE_NEWNS|CLONE_FS) at the time of creating the first
| >    restarted process ?
| 
| And then move remounting of devpts into ckpt_remount_proc() called
| from __ckpt_coordinator() as well?

Yes, I missed that in the hack.

Thanks,

Sukadev

Sukadev Bhattiprolu [[email protected]] wrote:

| Serge E. Hallyn [serue-r/[email protected]] wrote:
| | Quoting Sukadev Bhattiprolu ([email protected]):
| | > 
| | > Came across this while testing LXC.
| | > 
| | > 
| | > 1. Does ckpt_remount_proc() need to unshare() ? Or can we have the
| | >    clone() that calls __ckpt_coordinator() clone with CLONE_NEWNS|CLONE_FS
| | >    instead ?
| | > 
| | >    The problem with the unshare() in ckpt_remount_proc() is that it
| | >    creates an extra level in cgroup hierarchy (see below) after restart.
| | >    So applications expecting the cgroup hierarchy before chckpoint will
| | >    be surprised.
| | > 
| | > 2. When --mount-pty (or --mntns) is specified, do we need to unshare() 
| | >    in the parent process ? Considering only the full-container restart
| | >    for now (ignore self-restart and subtree restart), can we just
| | >    specify (CLONE_NEWNS|CLONE_FS) at the time of creating the first
| | >    restarted process ?
| | 
| | And then move remounting of devpts into ckpt_remount_proc() called
| | from __ckpt_coordinator() as well?
| 
| Yes, I missed that in the hack.

Here is a more involved fix, but not sure if there is a more efficient way to solve.

---
From 6813961bc1f357516685145279f9f8ce883c31a4 Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <[email protected]>
Date: Mon, 8 Mar 2010 12:03:46 -0800
Subject: [PATCH 1/1] Minimize unshare() calls

We currently have a few unshare() calls at different points in the code. While these don't affect the restart application itself, the excess calls create additional levels in the cgroup hierarchy, which can surprise the administrator (or other users of the hierarchy such as LXC.

Rather than several unshare() calls, can we instead specify the appropriate clone_flags while creating the coordinator/root process of the application tree ? When this root process is created it can remount /proc, remount devpts, chroot() etc if necessary.

Note that for "new-container with init" and "subtree restart", the first process is also the root of the application process tree.

In the case of "new-container without init", the coordinator process which acts as the container-init can do the setup.

In case of self-restart, the main process itself can do the unshare.

This patch has been very gently tested :-) but wanted to get more feedback on the direction and see if there is an easier way.

---
 restart.c |   78 ++++++++++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 62 insertions(+), 16 deletions(-)

index 63e1881..87a30dc 100644
@@ -127,6 +127,9 @@ struct task zero_task;
 #define TASK_SESSION	0x10	/* inherits creator's original sid */
 #define TASK_NEWPID	0x20	/* starts a new pid namespace */
 #define TASK_DEAD	0x40	/* dead task (dummy) */
+#define TASK_NEWROOT	0x80	/* task must chroot() */
+#define TASK_NEWPTS	0x100	/* remount devpts */
+#define TASK_NEWNS	0x200	/* unshare namespace/file-system */
 
 struct ckpt_ctx {
 	pid_t root_pid;

@@ -458,24 +461,24 @@ int app_restart(struct app_restart_args *args)
 	if (args->freezer && freezer_prepare(&ctx) < 0)
 		exit(1);
 
-	/* private mounts namespace ? */
-	if (args->mntns && unshare(CLONE_NEWNS | CLONE_FS) < 0) {
-		ckpt_perror("unshare");
-		exit(1);
-	}
+	/* self-restart ends here: */
+	if (args->self) {
+		/* private mounts namespace ? */
+		if (args->mntns && unshare(CLONE_NEWNS | CLONE_FS) < 0) {
+			ckpt_perror("unshare");
+			exit(1);
+		}
 
-	/* chroot ? */
-	if (args->root && chroot(args->root) < 0) {
-		ckpt_perror("chroot");
-		exit(1);
-	}
+		/* chroot ? */
+		if (args->root && chroot(args->root) < 0) {
+			ckpt_perror("chroot");
+			exit(1);
+		}
 
-	/* remount /dev/pts ? */
-	if (args->mnt_pty && ckpt_remount_devpts(&ctx) < 0)
-		exit(1);
+		/* remount /dev/pts ? */
+		if (args->mnt_pty && ckpt_remount_devpts(&ctx) < 0)
+			exit(1);
 
-	/* self-restart ends here: */
-	if (args->self) {
 		restart(getpid(), STDIN_FILENO, RESTART_TASKSELF, args->klogfd);
 		/* reach here if restart(2) failed ! */
 		ckpt_perror("restart");

@@ -520,10 +523,30 @@ int app_restart(struct app_restart_args *args)
 	if (ret < 0)
 		exit(1);
 
+	/*
+	 * Have the first child in the restarted process tree
+	 * setup devpts, root-dir and /proc if necessary, ...
+	 */
+	if (ctx.args->mnt_pty)
+		ctx.tasks_arr[0].flags |= TASK_NEWPTS;
+
+	if (ctx.args->mntns)
+		ctx.tasks_arr[0].flags |= TASK_NEWNS;
+
+	if (ctx.args->root)
+		ctx.tasks_arr[0].flags |= TASK_NEWROOT;
+
 	if (ctx.args->pidns && ctx.tasks_arr[0].pid != 1) {
 		ckpt_dbg("new pidns without init\n");
 		if (global_send_sigint == -1)
 			global_send_sigint = SIGINT;
+		/*
+		 * ...unless we have an explicit coordinator, in which case
+		 * the coordinator should set up the filesystems and
+		 * not the first process in the application process tree.
+		 */
+		ctx.tasks_arr[0].flags &= ~(TASK_NEWPTS | TASK_NEWROOT | \
+				TASK_NEWNS);
 		ret = ckpt_coordinator_pidns(&ctx);
 	} else if (ctx.args->pidns) {
 		ckpt_dbg("new pidns with init\n");

@@ -717,10 +740,12 @@ static int ckpt_probe_child(pid_t pid, char *str)
  */
 static int ckpt_remount_proc(struct ckpt_ctx *ctx)
 {
+#if 0
 	if (unshare(CLONE_NEWNS | CLONE_FS) < 0) {
 		ckpt_perror("unshare");
 		return -1;
 	}
+#endif
 	/* this is unlikely, but we don't want to fail */
 	if (umount2("/proc", MNT_DETACH) < 0) {
 		if (ckpt_cond_fail(ctx, CKPT_COND_MNTPROC)) {

@@ -743,9 +768,18 @@ static int __ckpt_coordinator(void *arg)
 {
 	struct ckpt_ctx *ctx = (struct ckpt_ctx *) arg;
 
+	/* chroot ? */
+	if (ctx->args->root && chroot(ctx->args->root) < 0) {
+		ckpt_perror("chroot");
+		exit(1);
+	}
+
 	if (ckpt_remount_proc(ctx) < 0)
 		return -1;
 
+	if (ctx->args->mnt_pty && ckpt_remount_devpts(ctx) < 0)
+		return -1;
+
 	if (!ctx->args->wait)
 		close(ctx->pipe_coord[0]);
 

@@ -778,6 +812,7 @@ static int ckpt_coordinator_pidns(struct ckpt_ctx *ctx)
 	int copy, ret;
 	genstack stk;
 	void *sp;
+	unsigned long flags = SIGCHLD;
 
 	ckpt_dbg("forking coordinator in new pidns\n");
 

@@ -802,7 +837,9 @@ static int ckpt_coordinator_pidns(struct ckpt_ctx *ctx)
 	copy = ctx->args->copy_status;
 	ctx->args->copy_status = 1;
 
-	coord_pid = clone(__ckpt_coordinator, sp, CLONE_NEWPID|SIGCHLD, ctx);
+	flags |= CLONE_NEWPID|CLONE_NEWNS;
+
+	coord_pid = clone(__ckpt_coordinator, sp, flags, ctx);
 	genstack_release(stk);
 	if (coord_pid < 0) {
 		ckpt_perror("clone coordinator");

@@ -1610,10 +1647,16 @@ int ckpt_fork_stub(void *data)
 	struct task *task = (struct task *) data;
 	struct ckpt_ctx *ctx = task->ctx;
 
+	if ((task->flags & TASK_NEWROOT) && chroot(ctx->args->root) < 0)
+		return -1;
+
 	/* tasks with new pid-ns need new /proc mount */
 	if ((task->flags & TASK_NEWPID) && ckpt_remount_proc(ctx) < 0)
 		return -1;
 
+	if ((task->flags & TASK_NEWPTS) && ckpt_remount_devpts(ctx) < 0)
+		return -1;
+
 	/*
 	 * In restart into a new pid namespace (--pidns), coordinator
 	 * is the container init, hence if it terminated permatutely

@@ -1691,6 +1734,9 @@ static pid_t ckpt_fork_child(struct ckpt_ctx *ctx, struct task *child)
 		pid = 0;
 	}
 #endif
+	if (child->flags & TASK_NEWNS) {
+		flags |= CLONE_NEWNS;
+	}
 
 	if (child->flags & (TASK_SIBLING | TASK_THREAD))
 		child->real_parent = getppid();

1.6.6.1

I don't know what you mean by more efficient - this looks good to me. (Apart from probably also pulling the self-restart stuff off into a helper)

-serge