WIP: Add Checkpoint and Restore support to libcontainer #479

crosbymichael · 2015-03-24T20:41:54Z

This is a work in progress PR so that we can move the discussion around this work into a PR to help move things along and get other people involved.

We are trying to get nsinit work as an easy way to test but the first class support should be for using it from a daemon such as docker.

Any more work can be done on the criu branch of this repo via pull requests.

mrunalp · 2015-03-24T20:47:54Z

configs/config.go

@@ -96,6 +96,9 @@ type Config struct {
 	// ReadonlyPaths specifies paths within the container's rootfs to remount as read-only
 	// so that these files prevent any writes.
 	ReadonlyPaths []string `json:"readonly_paths"`
+
+	// Container's standard descriptors (std{in,out,err}), needed for checkpoint and restore
+	StdFds [3]string `json:"ext_pipes,omitempty"`


Minor nit: Could the json property be called something else?

@mrunalp
Sure, any suggestions? The name ext_pipes was chosen because the standard file descriptors are actually external pipes but I am open to better suggestions.

@SaiedKazemi I was thinking that the name of the exported field and the json tag be similar like elsewhere in the code. If the name is StdFds, then maybe std_fds for the json tag.

Yes, +1. or rename StdFds to ExtPipes :)

Sounds good. Will do.

LK4D4 · 2015-03-24T20:49:18Z

Supercool.
Gofmt failed.

jessfraz · 2015-03-24T20:50:18Z

so cool so cool so cool, can't contain myself

On Tue, Mar 24, 2015 at 1:49 PM, Alexander Morozov <[email protected]

wrote:

Supercool.
Gofmt failed.

—
Reply to this email directly or view it on GitHub
#479 (comment).

mrunalp · 2015-03-24T21:07:16Z

container_linux.go

+	if err := os.Mkdir(dir, 0655); err != nil && !os.IsExist(err) {
+		return err
+	}
+	args := []string{


I know we have hashed this before :) Should we check for the presence of a minimum version of criu that supports all the flags or maybe just document it, so distributions know what version they need?

Most of the flags have been there for many years and we haven't introduced new flags in a long time. But if someone is running an old version of CRIU that doesn't support a specific flag, CRIU will print an error message and exit -- no damage done. That said, we can certainly add code to do version check every time but I really think it'd be an overkill :)

@SaiedKazemi That is fair. I think if we just document a minimum version, that will be helpful :)

Sounds good.

+1 for validating compatibility.

vishh · 2015-03-24T22:47:36Z

👍

hqhq · 2015-03-26T03:13:59Z

Looking forward... 👍

vishh · 2015-03-27T18:01:05Z

container_linux.go

@@ -249,6 +257,124 @@ func (c *linuxContainer) NotifyOOM() (<-chan struct{}, error) {
 	return notifyOnOOM(c.cgroupManager.GetPaths())
 }

+// XXX debug support, remove when debugging done.


Can we get rid of all the debug code?

@vishh
Since we're still debugging, it makes a lot easier to add additional command line options when invoking CRIU through the environment. Otherwise, we need to edit and recompile source for every little change in the options. I will remove it soon.

crosbymichael · 2015-03-27T18:09:45Z

Just for an FYI, nsinit is just for testing, the use case we should optimize for is the daemon(docker) use cases. We should have the ability to make checkpoint/restore options configurable.

vishh · 2015-03-27T18:38:44Z

container_linux.go

+
+	// XXX This doesn't really belong here as our caller should have
+	//     already set up root (including devices) and mounted it.
+	/*


Delete dead code?

@vishh
Will do.

boucher · 2015-03-31T21:57:59Z

I'm interested in a slightly different use case than possible with the way this PR would work. Specifically, I want to be able to checkpoint a container without stopping it, and then later be able to either "roll back" the container to that point in time or alternatively spin up a new container from that checkpoint. Is there any interest in the idea of allowing you to specify an explicit path to checkpoint to and restore from (and, additionally, flags like --leave-running).

boucher · 2015-04-01T18:46:04Z

I'm trying to get this pull request running and having some trouble. What I've tried so far is to build libcontainer (with it's Dockerfile, and the addition of criu to that Dockerfile), and run the following test:

docker run -d --privileged -w /busybox dockercore/libcontainer nsinit exec -- sh -c 'i=0; while true; do echo $i; i=$(expr $i + 1); sleep 3; done'

followed by nsinit checkpoint from within the resulting container. That fails, with this output log:

https://gist.github.com/boucher/d2539bcaa1311c5d5f5c

Any suggestions? Or, is there a better way to test this?

(edit: I should note that I tried both master and the 1.5 stable branch of criu)

boucher · 2015-04-03T20:47:54Z

Just wanted to follow up in this thread that I was able to get this PR running outside of Docker, directly on a linux host, but haven't yet been able to get it working inside of Docker.

boucher · 2015-04-08T01:29:25Z

container_linux.go

+
+	// cmd.Wait() waits cmd.goroutines which are used for proxying file descriptors.
+	// Here we want to wait only the CRIU process.
+	st, err := cmd.Process.Wait()


I'm working on getting this branch up and running in Docker, but I'm a bit unsure about what to do here. Because we're not passing the -d flag to criu, this Wait will actually just wait forever -- that's fine in the sort of direct nsinit resume an interactive container case (which I had used to test this branch), but it doesn't work when dealing with the Docker daemon. Should this be a configurable flag we're passing in, or should the wait be handled at the nsinit level and not here?

Yes, here is a mistake. I've fixed it in #486

boucher · 2015-04-17T01:05:48Z

container_linux.go

+
+	switch {
+	case notify.GetScript() == "post-dump":
+		f, err := os.Create(filepath.Join(c.root, "checkpoint"))


Is the purpose of this file now just to be a flag that things are checkpointed? (Previously, this is where things were being stored if no directory was specified)

@boucher Yes, it's a flag. It's used in currentStatus() to determine the Checkpointed state.

boucher · 2015-04-17T06:16:26Z

I'm getting this error on some restore attempts when running this branch:

Error (cr-service.c:58): Failed unpacking request: Success
Error (cr-service.c:694): Can't recv request: Success
data too short after length-prefix of 1217

I added a log in criuSwrk to print all the opts right before actually sending them, and other than the actual directory paths I seem to have two identical set of options that work and don't work (though the underlying container isn't the same, but the error message seems to imply a communication failure not an actual restore failure). I'm running in two different VMs so I'm going to try and narrow down the differences some more to figure out what's going on but I figured I'd post this here to see if there were any ideas.

avagin · 2015-04-17T06:55:08Z

@boucher I think we use too small buffer in CRIU. Probably we need to allocate it dinamically. Could you try out this patch for criu?

diff --git a/include/cr-service-const.h b/include/cr-service-const.h
index 668882b..66a059f 100644
--- a/include/cr-service-const.h
+++ b/include/cr-service-const.h
@@ -1,7 +1,7 @@
 #ifndef __CR_SERVICE_CONST_H__
 #define __CR_SERVICE_CONST_H__

-#define CR_MAX_MSG_SIZE 1024
+#define CR_MAX_MSG_SIZE 16384
 #define CR_DEFAULT_SERVICE_ADDRESS "/var/run/criu_service.socket"

 #endif /* __CR_SERVICE_CONST_H__ */

avagin · 2015-04-17T19:35:41Z

The last issue is a criu bug: http://lists.openvz.org/pipermail/criu/2015-April/019928.html

avagin · 2015-04-17T19:39:32Z

@crosbymichael @SaiedKazemi @LK4D4 @mrunalp @vmarmol I think I've fixed all known issue. Maybe it's time to merge this branch?

SaiedKazemi · 2015-04-17T19:43:57Z

@avagin @crosbymichael @LK4D4 @vmarmol
Thanks Andrew. I agree it's time to merge into master.

crosbymichael · 2015-04-17T19:46:48Z

Ok, so I should be able to build this branch, and everything with work flawlessly the first time?

In a future we may want to have more external descriptors. Signed-off-by: Andrey Vagin <[email protected]>

Docker-DCO-1.1-Signed-off-by: Ross Boucher <[email protected]> (github: boucher)

Signed-off-by: Andrey Vagin <[email protected]>

…a const. Docker-DCO-1.1-Signed-off-by: Ross Boucher <[email protected]> (github: boucher)

Docker-DCO-1.1-Signed-off-by: Ross Boucher <[email protected]> (github: boucher)

container.Checkpoint() doesn't clear c.initProcess and it's used on restore. Signed-off-by: Andrew Vagin <[email protected]>

…equiring that one already exists. Docker-DCO-1.1-Signed-off-by: Ross Boucher <[email protected]> (github: boucher)

Docker-DCO-1.1-Signed-off-by: Ross Boucher <[email protected]> (github: boucher)

Adds iptables as a requirement of criu in the Dockerfile. Docker-DCO-1.1-Signed-off-by: Ross Boucher <[email protected]> (github: boucher)

The format of criu version is X.Y[.Z]. The current code can not parse X.Y, because scanf returns the "input does not match format" error. Signed-off-by: Andrey Vagin <[email protected]>

Signed-off-by: Michael Crosby <[email protected]>

Docker-DCO-1.1-Signed-off-by: Ross Boucher <[email protected]> (github: boucher)

sirupsen · 2015-05-21T15:53:27Z

This is really exciting work! Is there any update on the status of this PR?

README example for using checkpoint/restore.

crosbymichael · 2015-05-21T18:46:45Z

I'm getting the error:

nsinit checkpoint --image-path /tmp/checkpoint
criu failed: type DUMP errno 0

crosbymichael · 2015-05-21T18:47:03Z

criu is compiled via the v1.5.2 tag and running on ubuntu 15.04 (the new LTS)

boucher · 2015-05-21T19:09:44Z

Can you send the dump log?

On Thursday, May 21, 2015, Michael Crosby [email protected] wrote:

criu is compiled via the v1.5.2 tag

—
Reply to this email directly or view it on GitHub
#479 (comment).

Sent from Gmail Mobile

crosbymichael · 2015-05-21T19:19:27Z

@boucher i don't have the dump.log in the image dir? is it located somewhere else?

avagin · 2015-05-21T19:31:42Z

@crosbymichael Could you attache a strace log?
strace -fo strace.log -s 256 nsinit checkpoint --image-path /tmp/checkpoint

crosbymichael · 2015-05-21T19:34:36Z

@avagin https://gist.github.com/f8f2a349ecd9ad6f62cc

avagin · 2015-05-21T20:13:25Z

@crosbymichael this log doesn't contain "criu failed:...". dump.log is saved in /var/run/nsinit/nsinit/criu.work/

'''
write(1023, "(00.235986) Dumping finished successfully\n", 42) = 42
'''

crosbymichael · 2015-05-21T20:20:49Z

ok, it was a setuid issue with my binary

crosbymichael · 2015-05-21T20:28:56Z

LGTM

crosbymichael · 2015-05-21T20:32:33Z

thanks @avagin @boucher @SaiedKazemi

WIP: Add Checkpoint and Restore support to libcontainer

mrunalp reviewed Mar 24, 2015
View reviewed changes

vishh reviewed Mar 27, 2015
View reviewed changes

crosbymichael force-pushed the criu branch from 17e5c87 to 753c326 Compare April 3, 2015 17:52

thaJeztah mentioned this pull request Apr 6, 2015

Move containers between hosts rancher/rancher#438

Closed

boucher reviewed Apr 8, 2015
View reviewed changes

SaiedKazemi mentioned this pull request Apr 11, 2015

Add checkpoint/restore functionality to nsinit and libcontainer #204

Closed

GordonTheTurtle added dco/no and removed dco/yes labels Apr 16, 2015

boucher reviewed Apr 17, 2015
View reviewed changes

GordonTheTurtle added dco/no and removed dco/yes labels Apr 17, 2015

avagin and others added 11 commits May 20, 2015 15:19

Don't fix the size of the ExternalDescriptors array

8f0cad5

In a future we may want to have more external descriptors. Signed-off-by: Andrey Vagin <[email protected]>

Fix formatting issues, and missing test methods.

78a21b7

Docker-DCO-1.1-Signed-off-by: Ross Boucher <[email protected]> (github: boucher)

cr: handle all external descriptros in ct.Restore()

7d32656

Signed-off-by: Andrey Vagin <[email protected]>

Reformat some error handling code and declare descriptor filename as …

7c138eb

…a const. Docker-DCO-1.1-Signed-off-by: Ross Boucher <[email protected]> (github: boucher)

Require criu 1.5.2 rather than 1.5.1

420180a

Docker-DCO-1.1-Signed-off-by: Ross Boucher <[email protected]> (github: boucher)

container: don't use c.initProcess before it's set

a16d7f1

container.Checkpoint() doesn't clear c.initProcess and it's used on restore. Signed-off-by: Andrew Vagin <[email protected]>

Allow restore to actually exec/create the libcontainer, rather than r…

bfb093a

…equiring that one already exists. Docker-DCO-1.1-Signed-off-by: Ross Boucher <[email protected]> (github: boucher)

Add steps to install criu to the Dockerfile

b3ec1fc

Docker-DCO-1.1-Signed-off-by: Ross Boucher <[email protected]> (github: boucher)

First attempt at an integration test for checkpoint/restore.

206b5e6

Adds iptables as a requirement of criu in the Dockerfile. Docker-DCO-1.1-Signed-off-by: Ross Boucher <[email protected]> (github: boucher)

cr: fix parsing of criu version

5f6ea0e

The format of criu version is X.Y[.Z]. The current code can not parse X.Y, because scanf returns the "input does not match format" error. Signed-off-by: Andrey Vagin <[email protected]>

Remove logging for checkpoint code

e754214

Signed-off-by: Michael Crosby <[email protected]>

crosbymichael force-pushed the criu branch from ab35f71 to e754214 Compare May 20, 2015 22:49

GordonTheTurtle removed the dco/no label May 20, 2015

README example for using checkpoint/restore.

fee819c

Docker-DCO-1.1-Signed-off-by: Ross Boucher <[email protected]> (github: boucher)

Merge pull request #600 from boucher/criu-readme

703d9a1

README example for using checkpoint/restore.

crosbymichael added a commit that referenced this pull request May 21, 2015

Merge pull request #479 from docker/criu

35d1c0e

WIP: Add Checkpoint and Restore support to libcontainer

crosbymichael merged commit 35d1c0e into master May 21, 2015

crosbymichael deleted the criu branch May 21, 2015 20:32

wonderflow mentioned this pull request May 26, 2015

Update nsinit readme for C/R #605

Merged

WIP: Add Checkpoint and Restore support to libcontainer #479

WIP: Add Checkpoint and Restore support to libcontainer #479

Conversation

crosbymichael commented Mar 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LK4D4 commented Mar 24, 2015

jessfraz commented Mar 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishh commented Mar 24, 2015

hqhq commented Mar 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crosbymichael commented Mar 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boucher commented Mar 31, 2015

boucher commented Apr 1, 2015

boucher commented Apr 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boucher commented Apr 17, 2015

avagin commented Apr 17, 2015

avagin commented Apr 17, 2015

avagin commented Apr 17, 2015

SaiedKazemi commented Apr 17, 2015

crosbymichael commented Apr 17, 2015

sirupsen commented May 21, 2015

crosbymichael commented May 21, 2015

crosbymichael commented May 21, 2015

boucher commented May 21, 2015

crosbymichael commented May 21, 2015

avagin commented May 21, 2015

crosbymichael commented May 21, 2015

avagin commented May 21, 2015

crosbymichael commented May 21, 2015

crosbymichael commented May 21, 2015

crosbymichael commented May 21, 2015