Enable checkpoint/restore of containers with TTY by rst0git · Pull Request #38405 · moby/moby

rst0git · 2018-12-20T18:04:24Z

CRIU supports checkpoint and restore of tty devices since version 2.12
which was released on 8th of March 2017. Support for this functionality
was implemented with opencontainers/runc@1c43d09 (checkpoint: add
support for containers with terminals) and containerd/containerd@60daa41
(Allow to checkpoint and restore a container with console).

Therefore, we can enable the support in moby/docker.

Signed-off-by: Radostin Stoyanov rstoyanov1@gmail.com

- How to verify it

$ docker run -dit --name apache-app -p 8080:80 httpd:2.4
$ curl http://localhost:8080
<html><body><h1>It works!</h1></body></html>
$ docker checkpoint create apache-app c1
$ docker start --checkpoint c1 apache-app
$ curl http://localhost:8080
<html><body><h1>It works!</h1></body></html>

rst0git · 2018-12-20T18:07:49Z

@avagin @kolyshkin

avagin · 2018-12-20T18:53:26Z

LGTM

It would be good to add a test for this.

kolyshkin · 2018-12-20T23:28:22Z

CI failure is flaky test (TestServiceWithDefaultAddressPoolInit, #37836), so can be ignored.

It would be awesome to have some integration tests for C/R to be implemented (https://github.com/moby/moby/tree/master/integration/container is a good starting point to look at if you are up to it @rst0git), but can be done as a followup since alas currently we don't have any.

@rst0git did you make sure that containerd and runc vendored in moby/moby are new enough to support tty c/r?

codecov · 2018-12-21T00:58:43Z

Codecov Report

Merging #38405 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #38405      +/-   ##
==========================================
+ Coverage   36.54%   36.55%   +0.01%     
==========================================
  Files         608      608              
  Lines       45040    45038       -2     
==========================================
+ Hits        16458    16462       +4     
+ Misses      26300    26293       -7     
- Partials     2282     2283       +1

rst0git · 2018-12-21T03:09:42Z

Thanks for the feedback! The integration tests for C/R is a great idea and I will look into that.
The containerd and runc versions vendored in moby/moby are up to date and support tty c/r. I also tested this functionality manually with the development environment (i.e. make BIND_DIR=. shell). Although, for this to work there it requires the patches from #38378.

thaJeztah · 2018-12-24T16:47:45Z

Support for this functionality was implemented with opencontainers/runc@1c43d09 (checkpoint: add support for containers with terminals) and containerd/containerd@60daa41

double-checking; both are in the versions that are currently used in this repository? https://github.com/moby/moby/blob/master/hack/dockerfile/install/runc.installer#L7
and https://github.com/moby/moby/blob/master/hack/dockerfile/install/containerd.installer#L7

avagin · 2018-12-29T10:44:26Z

Thanks for the feedback! The integration tests for C/R is a great idea and I will look into that.

avagin@fbcefa3

rst0git · 2018-12-29T19:21:26Z

@avagin Thank you implementing the integration test, I have added it to the PR.

fntlnz · 2018-12-29T20:09:27Z

@rst0git thanks for adding the integration test, there's an error during checkpoint at line 93, doesn't seem to be related to the TTY but I think we need two test cases, one with and one without TTY

CRIU supports checkpoint and restore of tty devices since version 2.12 which was released on 8th of March 2017. Support for this functionality was implemented with opencontainers/runc@1c43d09 (checkpoint: add support for containers with terminals) and containerd/containerd@60daa41 (Allow to checkpoint and restore a container with console). Therefore, we can enable the support in moby/docker. Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com>

rst0git · 2018-12-30T07:58:02Z

@rst0git thanks for adding the integration test, there's an error during checkpoint at line 93, doesn't seem to be related to the TTY but I think we need two test cases, one with and one without TTY

@fntlnz Thanks for the feedback. This integration test is now in #38452
We could add a test case for TTY after it is merged.

olljanat · 2019-01-09T05:52:35Z

Derek add label: rebuild/z

gj · 2019-02-19T22:48:04Z

Hey 👋 anything still blocking this PR from making it in? Happy to help move it along if so!

cpuguy83 · 2019-02-19T22:55:05Z

Looks like it just needs an integration test.

kolyshkin · 2019-02-20T19:42:11Z

Looks like it just needs an integration test.

@cpuguy83 the problem is, there are 0 tests for C/R at the moment. This is the subject of #38452 -- once it's there, it will be easy to add a case for TTY

gj · 2019-03-07T06:33:50Z

Now that #38452 is in, I've gone ahead and copied @avagin's excellent work to create an integration test for a container with TTY:

https://github.com/gj/moby/commit/ba7fe405355c55ebbe6e1c2db1c834b4cba92cb8

ETA: Updated with feedback from @avagin and @kolyshkin: https://github.com/gj/moby/commit/1054ee5d38678cca7687fb2fd9cd3fc87d01c018, https://github.com/gj/moby/commit/ea5974634b70b9969cee2f407a1ad9f5570a0532, https://github.com/gj/moby/commit/9065cc83a0fad7966107a6a9a1839199a22f1287

rst0git · 2019-03-16T09:39:05Z

@gj Would you like to open a PR with your changes?

gj · 2019-03-18T16:17:18Z

@rst0git sorry for not being more responsive on this. I've been trying to figure out how to attach IO streams to the test container restored from a checkpoint in the same style as this test (helpfully pointed out by @avagin as a good example to follow).

kolyshkin · 2019-03-21T21:12:35Z

I guess we don't lose anything if we merge this one without a test case, which can be done as a followup.

@gj I suggest you open the PR with your changes (doesn't matter if it's ready for prime time or not) and mark it with [WIP] for now -- this way it will be much easier to track and review. Please cc me, @avagin and @rst0git on it so we can help.

gj · 2019-03-21T21:56:56Z

Will do that tonight @kolyshkin; thanks!

GordonTheTurtle added the status/0-triage label Dec 20, 2018

derek bot added the rebuild/experimental label Dec 20, 2018

GordonTheTurtle removed the rebuild/experimental label Dec 20, 2018

thaJeztah added status/2-code-review kind/experimental and removed status/0-triage labels Dec 24, 2018

rst0git force-pushed the enable-cr-tty branch from 0453d9a to e4ecc54 Compare December 29, 2018 19:18

rst0git requested a review from vdemeester as a code owner December 29, 2018 19:18

rst0git force-pushed the enable-cr-tty branch from e4ecc54 to 64b3b13 Compare December 30, 2018 07:53

fntlnz added rebuild/experimental labels Dec 31, 2018

GordonTheTurtle removed the rebuild/experimental label Dec 31, 2018

fntlnz added rebuild/windowsRS5-process and removed rebuild/windowsRS5-process labels Jan 1, 2019

derek bot added the rebuild/* label Jan 1, 2019

GordonTheTurtle removed the rebuild/* label Jan 1, 2019

derek bot removed rebuild/windowsRS5-process labels Jan 1, 2019

GordonTheTurtle assigned aaronlehmann Jan 4, 2019

derek bot added the rebuild/z label Jan 9, 2019

GordonTheTurtle removed the rebuild/z label Jan 9, 2019

derek bot added the rebuild/z label Jan 9, 2019

GordonTheTurtle removed the rebuild/z label Jan 9, 2019

kolyshkin merged commit 6680a5c into moby:master Mar 21, 2019

thaJeztah added the impact/changelog label Apr 4, 2019

RELOAD22 mentioned this pull request Feb 16, 2020

Don't have tty to inherit session from, aborting when migrate docker to different node checkpoint-restore/criu#952

Closed

Conversation

rst0git commented Dec 20, 2018

Uh oh!

rst0git commented Dec 20, 2018

Uh oh!

avagin commented Dec 20, 2018

Uh oh!

kolyshkin commented Dec 20, 2018

Uh oh!

codecov bot commented Dec 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rst0git commented Dec 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Dec 24, 2018

Uh oh!

avagin commented Dec 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rst0git commented Dec 29, 2018

Uh oh!

fntlnz commented Dec 29, 2018

Uh oh!

rst0git commented Dec 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olljanat commented Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gj commented Feb 19, 2019

Uh oh!

cpuguy83 commented Feb 19, 2019

Uh oh!

kolyshkin commented Feb 20, 2019

Uh oh!

gj commented Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rst0git commented Mar 16, 2019

Uh oh!

gj commented Mar 18, 2019

Uh oh!

kolyshkin commented Mar 21, 2019

Uh oh!

gj commented Mar 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

codecov bot commented Dec 21, 2018 •

edited

Loading

rst0git commented Dec 21, 2018 •

edited

Loading

avagin commented Dec 29, 2018 •

edited

Loading

rst0git commented Dec 30, 2018 •

edited

Loading

olljanat commented Jan 9, 2019 •

edited

Loading

gj commented Mar 7, 2019 •

edited

Loading