Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable checkpoint/restore of containers with TTY #38405

Merged
merged 1 commit into from Mar 21, 2019

Conversation

Projects
None yet
10 participants
@rst0git
Copy link
Contributor

rst0git commented Dec 20, 2018

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

This comment has been minimized.

Copy link
Contributor Author

rst0git commented Dec 20, 2018

@avagin

This comment has been minimized.

Copy link
Contributor

avagin commented Dec 20, 2018

LGTM

It would be good to add a test for this.

@kolyshkin

This comment has been minimized.

Copy link
Contributor

kolyshkin commented Dec 20, 2018

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

This comment has been minimized.

Copy link

codecov bot commented Dec 21, 2018

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

This comment has been minimized.

Copy link
Contributor Author

rst0git commented Dec 21, 2018

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

This comment has been minimized.

Copy link
Member

thaJeztah commented Dec 24, 2018

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

This comment has been minimized.

Copy link
Contributor

avagin commented Dec 29, 2018

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

avagin@fbcefa3

@rst0git rst0git force-pushed the rst0git:enable-cr-tty branch from 0453d9a to e4ecc54 Dec 29, 2018

@rst0git rst0git requested a review from vdemeester as a code owner Dec 29, 2018

@rst0git

This comment has been minimized.

Copy link
Contributor Author

rst0git commented Dec 29, 2018

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

@fntlnz

This comment has been minimized.

Copy link
Member

fntlnz commented Dec 29, 2018

@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

Enable checkpoint/restore of containers with 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 rst0git force-pushed the rst0git:enable-cr-tty branch from e4ecc54 to 64b3b13 Dec 30, 2018

@rst0git

This comment has been minimized.

Copy link
Contributor Author

rst0git commented Dec 30, 2018

@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

This comment has been minimized.

Copy link
Contributor

olljanat commented Jan 9, 2019

Derek add label: rebuild/z

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

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

@gj

This comment has been minimized.

Copy link

gj commented Feb 19, 2019

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

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented Feb 19, 2019

Looks like it just needs an integration test.

@kolyshkin

This comment has been minimized.

Copy link
Contributor

kolyshkin commented Feb 20, 2019

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

@thaJeztah thaJeztah added this to backlog in maintainers-session Feb 21, 2019

@gj

This comment has been minimized.

Copy link

gj commented Mar 7, 2019

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:

gj@ba7fe40

ETA: Updated with feedback from @avagin and @kolyshkin: gj@1054ee5, gj@ea59746, gj@9065cc8

@rst0git

This comment has been minimized.

Copy link
Contributor Author

rst0git commented Mar 16, 2019

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

@gj

This comment has been minimized.

Copy link

gj commented Mar 18, 2019

@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

This comment has been minimized.

Copy link
Contributor

kolyshkin commented Mar 21, 2019

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.

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

9 checks passed

codecov/patch Coverage not affected when comparing 056840c...64b3b13
Details
codecov/project 36.55% (+0.01%) compared to 056840c
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 43478 has succeeded
Details
janky Jenkins build Docker-PRs 52298 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 12721 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 23448 has succeeded
Details
windowsRS5-process Jenkins build Docker-PRs-WoW-RS5-Process 929 has succeeded
Details
z Jenkins build Docker-PRs-s390x 12793 has succeeded
Details
@gj

This comment has been minimized.

Copy link

gj commented Mar 21, 2019

Will do that tonight @kolyshkin; thanks!

@thaJeztah thaJeztah removed this from backlog in maintainers-session Apr 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.