task: allow to specify namespaces which are restored externally by avagin · Pull Request #2425 · containerd/containerd

avagin · 2018-06-27T18:16:17Z

Currently, C/R in https://github.com/moby/moby is completely broken. This pr is required to fix it.

Here is an example of how entirely fix looks like:
avagin/docker-ce@911c89f

codecov-io · 2018-06-27T18:32:12Z

Codecov Report

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

@@            Coverage Diff            @@
##           master   #2425      +/-   ##
=========================================
+ Coverage   44.73%     45%   +0.27%     
=========================================
  Files          93      92       -1     
  Lines        9501    9412      -89     
=========================================
- Hits         4250    4236      -14     
+ Misses       4566    4493      -73     
+ Partials      685     683       -2

Flag	Coverage Δ
#linux	`49.23% <ø> (+0.28%)`	⬆️
#windows	`41.3% <ø> (+0.32%)`	⬆️

Impacted Files	Coverage Δ
remotes/docker/resolver.go	`61.11% <0%> (-0.22%)`	⬇️
archive/tar.go	`43.05% <0%> (-0.06%)`	⬇️
oci/spec_unix.go	`98.37% <0%> (-0.03%)`	⬇️
mount/temp_unix.go	`0% <0%> (ø)`	⬆️
sys/reaper.go	`0% <0%> (ø)`	⬆️
images/oci/exporter.go	`0% <0%> (ø)`	⬆️
services/server/config.go	`0% <0%> (ø)`	⬆️
sys/reaper_linux.go
services/server/server.go	`2.2% <0%> (+0.53%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 394784b...fc2fcf6. Read the comment docs.

avagin · 2018-06-27T21:01:40Z

Cc: @kolyshkin

kolyshkin · 2018-06-28T00:07:50Z

So, why are we using interface{} type for CheckpointTaskInfo.Options field? Wouldn't it be cleaner for it to be of runctypes.CheckpointOptions? I.e. have

// TaskInfo sets options for task creation type TaskInfo struct { // Checkpoint is the Descriptor for an existing checkpoint that can be used // to restore a task's runtime and memory state Checkpoint *types.Descriptor // RootFS is a list of mounts to use as the task's root filesystem RootFS []mount.Mount // Options hold runtime specific settings for task creation Options runctypes.CheckpointOptions }

At least this way we'll avoid doing tricks like the one in here, as well as typecasting it every time.

kolyshkin · 2018-06-28T00:17:35Z

Notes

this also requires some changes to moby/moby for restore to start working again
statically compiled containerd then segfaults on restore for the same reason as dockerd did (see https://github.com/kolyshkin/go-tar/blob/go-1.10/README.md; should be fixed by Go 1.11)
Either this repo or moby (or both?) would benefit from having tests for cpt/rst

kolyshkin · 2018-06-28T00:13:56Z

The word "externally" here means "external to criu". I would change it to say

... a namespace which properties are not to be restored by criu

Also maybe add that this corresponds to criu --empty-ns option.

It doesn't matter what tools are used for C/R

avagin · 2018-06-28T01:09:27Z

Either this repo or moby (or both?) would benefit from having tests for cpt/rst

Here we have a few tests:
https://github.com/containerd/containerd/blob/master/container_checkpoint_test.go

kolyshkin

LGTM

crosbymichael · 2018-07-03T21:06:05Z

can we keep this generic?

Yes, we can. @kolyshkin and I could not understand this trick. Could you explain its idea?

@crosbymichael from my perspective having interface{} here makes the rest of code unnecessary complicated (with extra casts and type checks). Can you elaborate on why you want this to be of interface{} type? I could (theoretically) anticipate a C/R engine other than CRIU but practically this is too far in the future I'd care for now.

https://github.com/google/gvisor (runsc) also has checkpoint/restore capability, and we are actively working on hooking it up to docker (i.e. fixing moby and stuff) as well. See moby/moby#37360.

So there already is another C/R engine.

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

kolyshkin · 2018-07-25T07:57:05Z

Is there anything needed for this PR to go through?

crosbymichael · 2018-07-25T14:26:04Z

LGTM

Ace-Tang · 2018-08-09T03:42:59Z

Hi , @avagin , from your example fix commit, you write that

checkpoint: don't restore a container network namespace
A network namespace should not be dumped and restored, it is created by
Docker even for restored containers.

Could you tell me why criu cannot dump and restore net ns, thanks.

kolyshkin · 2018-08-09T09:44:41Z

@Ace-Tang criu can dump/restore netns, but it should not in this very case.

Ace-Tang · 2018-08-09T12:34:06Z

Thanks for your reply, @kolyshkin

This was referenced Jun 27, 2018

Does criu work well with ubuntu 18.04? checkpoint-restore/criu#511

Closed

Can't restore containers moby/moby#35691

Closed

docker start checkpoint failed checkpoint-restore/criu#510

Closed

kolyshkin reviewed Jun 28, 2018

View reviewed changes

kolyshkin suggested changes Jun 28, 2018

View reviewed changes

avagin force-pushed the docker-fixes branch from 3e9f334 to 0a01d9e Compare June 28, 2018 19:03

kolyshkin approved these changes Jul 3, 2018

View reviewed changes

avagin mentioned this pull request Jul 3, 2018

Restoring containers from a custom checkpoint-dir is broken moby/moby#37344

Open

crosbymichael reviewed Jul 3, 2018

View reviewed changes

avagin mentioned this pull request Jul 9, 2018

Fix checkpoint's exiting semantics. moby/moby#37360

Merged

avagin force-pushed the docker-fixes branch 2 times, most recently from 69278c8 to bf848ff Compare July 20, 2018 20:18

task: WithExit() doesn't have to overwrite existing options

fc2fcf6

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

avagin force-pushed the docker-fixes branch from bf848ff to fc2fcf6 Compare July 20, 2018 20:19

crosbymichael merged commit 26e2dd6 into containerd:master Jul 25, 2018

Ace-Tang mentioned this pull request Aug 9, 2018

[question] why containerd can not dump and restore net ns checkpoint-restore/criu#538

Closed

Conversation

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

Uh oh!

Uh oh!

codecov-io commented Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

avagin commented Jun 27, 2018

Uh oh!

kolyshkin Jun 28, 2018

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Jun 28, 2018

Uh oh!

kolyshkin Jun 28, 2018

Choose a reason for hiding this comment

Uh oh!

avagin Jun 28, 2018

Choose a reason for hiding this comment

Uh oh!

avagin commented Jun 28, 2018

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

crosbymichael Jul 3, 2018

Choose a reason for hiding this comment

Uh oh!

avagin Jul 4, 2018

Choose a reason for hiding this comment

Uh oh!

kolyshkin Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hugelgupf Jul 10, 2018

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Jul 25, 2018

Uh oh!

crosbymichael commented Jul 25, 2018

Uh oh!

Ace-Tang commented Aug 9, 2018

Uh oh!

kolyshkin commented Aug 9, 2018

Uh oh!

Ace-Tang commented Aug 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

avagin commented Jun 27, 2018 •

edited

Loading

codecov-io commented Jun 27, 2018 •

edited

Loading

kolyshkin Jul 9, 2018 •

edited

Loading