Dump and restore containers with external terminals#1355
Merged
mrunalp merged 9 commits intoopencontainers:masterfrom May 18, 2017
Merged
Dump and restore containers with external terminals#1355mrunalp merged 9 commits intoopencontainers:masterfrom
mrunalp merged 9 commits intoopencontainers:masterfrom
Conversation
avagin
force-pushed
the
cr-console
branch
8 times, most recently
from
cb19ff6 to
ee0df0e
Compare
Member
|
This looks good. However, can you add a |
Member
|
Actually, since you're using |
Contributor
Author
|
@cyphar I added --console-socket. Thanks! |
dqminh
reviewed
Mar 3, 2017
Dockerfile
Outdated
| # install criu | ||
| ENV CRIU_VERSION 1.7 | ||
| ENV CRIU_VERSION 2.11.1 | ||
| COPY tests/hacks/0001-criu-allow-to-ignore-ipv6-if-CRIU_NOIPV6-is-set.patch /tmp/ |
Contributor
There was a problem hiding this comment.
i feel a little bit sad about this 😢
Can we make the janky machines load ipv6 modules prior to running all tests ? Or maybe we can just disable janky now that we have travis.
Contributor
Author
There was a problem hiding this comment.
I don't like this hack too. Do you know who supports janky machines?
Member
There was a problem hiding this comment.
They're managed by Docker Inc's infra team.
libcontainer/container_linux.go
Outdated
| return err | ||
| } | ||
| return nil | ||
| break |
Contributor
There was a problem hiding this comment.
i dont think we need this break here as it will break in the end of the block if we let it continue, just like how break in DUMP is removed ?
Contributor
Author
There was a problem hiding this comment.
I don't get this commet. What do you meen?
case t == criurpc.CriuReqType_RESTORE:
case t == criurpc.CriuReqType_DUMP:
break
case t == criurpc.CriuReqType_PRE_DUMP:
Member
There was a problem hiding this comment.
@avagin I think he's referring to the break at the end of the for loop.
Contributor
Author
There was a problem hiding this comment.
@cyphar No, it is about this break. I just found that it isn't required here in Go. But can we save it here, because I and probably others who read a lot of code in C will see a bug here.
Member
There was a problem hiding this comment.
But the current code is also "wrong" (if you're trying to make it more friendly to C programmers) because C programmers will assume that CriuReqType_RESTORE will fallthrough to _DUMP. Please just use idiomatic Go to avoid confusion like this for Go programmers.
Contributor
Author
There was a problem hiding this comment.
@cyphar I haven't thought that it is a problem to have one extra "break" here. Fixed.
| // If we got the message CriuReqType_PRE_DUMP it means | ||
| // CRIU was successful and we need to forcefully stop CRIU | ||
| logrus.Debugf("PRE_DUMP finished. Send close signal to CRIU service") | ||
| criuClient.Close() |
Contributor
There was a problem hiding this comment.
my guess is that criuClient is only a file so Close doesnt quite do what it should ?
criuClientCon can be casted to an unix socket, so maybe call Close, or even CloseWrite which is really SHUT_WR like we do below should do the correct thing
Contributor
Author
There was a problem hiding this comment.
I know what syscall.Shutdown() does. It work fast and reliable. I called it thouthds of times and it always works as expected;)
Contributor
Author
There was a problem hiding this comment.
@dqminh I found that net.FileConn() creates a new file descriptors and it was a reason why criuClientCon.Close() didn't close a socket. The current version of patches doesn't use the raw shutdown() syscall.
| // The current runc pre-dump approach, however, is | ||
| // start criu in PRE_DUMP once for a single pre-dump | ||
| // and not the whole series of pre-dump, pre-dump, ...m, dump | ||
| if !st.Success() && *req.Type != criurpc.CriuReqType_PRE_DUMP { |
Contributor
There was a problem hiding this comment.
i think we should still check for DUMP here right, as Predump is only an option and we can still specify DUMP ?
Contributor
Author
There was a problem hiding this comment.
we don't check exit code only for pre-dump, because it is not zero.
Closed
cyphar
reviewed
Mar 5, 2017
libcontainer/container_linux.go
Outdated
| fds, err := syscall.ParseUnixRights(&scm[0]) | ||
|
|
||
| process.consoleChan = make(chan *os.File, 1) | ||
| process.consoleChan <- os.NewFile(uintptr(fds[0]), "console") |
Member
Contributor
Author
There was a problem hiding this comment.
@cyphar I rebased my changes on #1356 and everything works as expected, so there is no problem with #1356
https://github.com/avagin/runc/tree/cr-console-after-1356
cyphar
reviewed
Mar 6, 2017
| CT_ACT_RESTORE | ||
| ) | ||
|
|
||
| func startContainer(context *cli.Context, spec *specs.Spec, action CtAct, criuOpts *libcontainer.CriuOpts) (int, error) { |
Member
There was a problem hiding this comment.
I don't really like passing criuOpts to startContainer. I do like the action argument though.
Contributor
Author
There was a problem hiding this comment.
@cyphar do you have any ideas how to pass criuOpts to container.Restore?
avagin
force-pushed
the
cr-console
branch
4 times, most recently
from
5b1076c to
a0cbd80
Compare
Contributor
Author
|
Contributor
Author
Member
avagin
force-pushed
the
cr-console
branch
4 times, most recently
from
89b73a7 to
7360326
Compare
Contributor
|
@avagin i tried the patch but couldnt make it to work dump.logDetails |
Contributor
Author
|
@dqminh I can not reproduce this issue and it looks strange. In CRIU this error is reported if the OrphanPtsMaster isn't set: but now runc always sets it https://github.com/avagin/runc/blob/cr-console/libcontainer/container_linux.go#L694 Could you check that you get runc from my repo (the cr-console branch)? Thank you for the feedback. |
Contributor
Author
|
@dqminh maybe you can send me a container config, I will try to reproduce on my host. |
|
271/278 passed on RHEL - Failed. |
|
270/278 passed on RHEL - Failed. |
hqhq
reviewed
Apr 20, 2017
| defer master.Close() | ||
|
|
||
| // While we can access console.master, using the API is a good idea. | ||
| if err := utils.SendFd(process.ConsoleSocket, master); err != nil { |
Contributor
There was a problem hiding this comment.
So it's still a two-steps-socket-translation? What #1356 does was try to avoid this, can the master be sent directly to process.consoleSocket in the container?
Contributor
Author
There was a problem hiding this comment.
We get this master form CRIU and CRIU doesn't have access to process.consoleSocket
Contributor
There was a problem hiding this comment.
But we can make it, right?
You can do something like this in criuSwrk: https://github.com/opencontainers/runc/blob/v1.0.0-rc3/libcontainer/container_linux.go#L373-L378
In criu, you can do similar as: https://github.com/opencontainers/runc/blob/v1.0.0-rc3/libcontainer/factory_linux.go#L251-L258
Then you'll have access to process.consoleSocket in criu.
Contributor
Author
There was a problem hiding this comment.
Unfortunately it isn't so easy. When a file descriptors are restored, we have to be sure that all restored file descriptors are not intersect with criu service descriptors. So the number of service descriptors are limited. It is one of reasons why we can't pass extra file descriptors to criu restore.
Another reason is that now we have very generic interface to handle external resources and it allows to handle any number of external terminals. It is impossible to pass a separate unix socket for each of them.
I understand your point, but I afraid there is no way to make it more optimal.
hqhq
reviewed
Apr 20, 2017
| @@ -1,3 +1,5 @@ | |||
| syntax = "proto2"; | |||
Contributor
Author
There was a problem hiding this comment.
No, we can't, because criu doesn't support proto3. It is a part of CRIU API: https://github.com/xemul/criu/blob/master/images/rpc.proto
Contributor
There was a problem hiding this comment.
Is that because CRIU uses protobuf-c? Just curious.
Contributor
Author
There was a problem hiding this comment.
Yes, it is. I tried to change proto2 to proto3 and get this error:
[avagin@laptop criu]$ make
PBCC images/rpc.pb-c.c
rpc.proto:1:10: Unrecognized syntax identifier "proto3". This parser only recognizes "proto2".
hqhq
reviewed
Apr 20, 2017
libcontainer/container_linux.go
Outdated
| break | ||
| } | ||
|
|
||
| syscall.Shutdown(fds[0], syscall.SHUT_WR) |
Contributor
There was a problem hiding this comment.
We are moving from syscall to sys/unix in runc, maybe you can use sys/unix here?
Contributor
Author
There was a problem hiding this comment.
Ok, will fix. Thanks
|
269/278 passed on RHEL - Failed. |
Contributor
Author
Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
Currently startContainer() is used to create and to run a container. In the next patch it will be used to restore a container. Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
CRIU was extended to report about orphaned master pty-s via RPC. Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
A freezer cgroup allows to dump processes faster. If a user wants to checkpoint a container and its storage, he has to pause a container, but in this case we need to pass a path to its freezer cgroup to "criu dump". Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
We have two test cases with and without pre-dump. Terminals and pre-dump features are orthogonal, so we can modify one of these test cases. Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
Member
|
LGTM |
Member
Contributor
|
LGTM |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CRIU (2.12) was extended to report orthaned master pty-s and it can be used to restore a runc container console.
Another good thing is that startContainer() is used to restore a container and this allows us to remove a lot of code from restore.go.
Cc: @cyphar
Fixes #1202