checkpoint: resolve symlink for external bind mount by liusdu · Pull Request #2902 · opencontainers/runc

liusdu · 2021-04-13T03:37:35Z

runc resolves symlink before doing bind mount. So
we should save original path while formatting CriuReq for
checkpoint.

Signed-off-by: Liu Hua weldonliu@tencent.com

kolyshkin · 2021-04-14T00:15:05Z

@liusdu thank you for the patch! Nice that you're using checkpoint/restore!

Can you add a test case (to e.g. tests/integration/checkpoint.bats?)

kolyshkin · 2021-04-15T20:04:43Z

@liusdu ^^^

liusdu · 2021-04-17T09:21:15Z

@liusdu ^^^

@kolyshkin Sorry for this late, checkpoint/restore of runc works very well for me.
The test cases is added, please take a look~~

liusdu · 2021-04-21T11:31:47Z

@kolyshkin

kolyshkin · 2021-04-21T20:06:27Z

+					options: ["bind"]
+				}]'
+	simple_cr
+


nit: extra empty line

kolyshkin · 2021-04-21T20:06:52Z


 func (c *linuxContainer) addCriuDumpMount(req *criurpc.CriuReq, m *configs.Mount) {
 	mountDest := strings.TrimPrefix(m.Destination, c.config.Rootfs)
+


nit: extra empty line

kolyshkin · 2021-04-21T20:07:13Z

LGTM except for a couple of nits. @adrianreber PTAL

adrianreber · 2021-04-21T20:32:39Z

@liusdu Can you give an example where you saw this error. It looks correct, but I am curious when this can happen.

liusdu · 2021-04-22T15:13:08Z

@adrianreber Look at following Dockerfile

FROM openjdk:8-jre
RUN wget  -qO-  'https://www.apache.org/dyn/closer.cgi?action=download&filename=flink/flink-1.12.2/flink-1.12.2-bin-scala_2.11.tgz' |tar -zvx -C /opt
RUN  ln -s  /opt/flink-1.12.2  /opt/flink

If we pass configuration as volume to container, we can meet this type of error.

docker run -d -v ${FLINK_CONF}:/opt/flink/conf ${flink_image}`

liusdu · 2021-04-22T15:14:06Z

@kolyshkin thanks for you review~

runc resolves symlink before doing bind mount. So we should save original path while formatting CriuReq for checkpoint. Signed-off-by: Liu Hua <weldonliu@tencent.com>

liusdu · 2021-04-29T01:51:30Z

ping @kolyshkin @adrianreber

kolyshkin

LGTM

kolyshkin · 2021-05-03T15:40:52Z

close/reopen to re-kick CI

haircommander · 2021-05-10T12:35:24Z

LGTM

cyphar · 2021-05-12T04:39:19Z

 func (c *linuxContainer) addCriuDumpMount(req *criurpc.CriuReq, m *configs.Mount) {
 	mountDest := strings.TrimPrefix(m.Destination, c.config.Rootfs)
+	if dest, err := securejoin.SecureJoin(c.config.Rootfs, mountDest); err == nil {
+		mountDest = dest[len(c.config.Rootfs):]


I do wonder -- what happens if the mount is moved from inside the container (with a simple rename) after the filesystem has been set up? Is this also something CRIU doesn't support / really doesn't like?

Yes @cyphar Since CRIU does not know how to restore bind mounts(master in host namespace), CRIU need this type of mount marked as external(via --external). runc formats --external parameter according to spec

So for situation you mentioned, CRIU does not work properly. btw, should we export --external for runc. Then users can mark external resource themselves?

I do wonder -- what happens if the mount is moved from inside the container (with a simple rename) after the filesystem has been set up? Is this also something CRIU doesn't support / really doesn't like?

I do not understand this question. But initially I would have said that CRIU can handle it. Now @liusdu says it is not possible. I probably misunderstood the question.

So for situation you mentioned, CRIU does not work properly. btw, should we export --external for runc. Then users can mark external resource themselves?

For Kubernetes, restoring containers in different Pods, I will need CRIU's --external to be available at the runc level.

Currently runc informs CRIU about all external mounts but it is not possible to change the location of the external mount between checkpoint and restore. For Kubernetes I will need to change the location of an external mount. The most obvious bind mount currently is the location of the secrets directory which has the Pod UID in its container external path and I need to tell CRIU/runc that that directory should be mounted from some other location.

ping @cyphar ..

liusdu · 2021-06-21T07:06:03Z

@haircommander can you approve this pr?

haircommander · 2021-06-21T13:36:36Z

It LGTM, but I'm not a maintainer so my approval wouldn't help unfortunately
@kolyshkin @mrunalp @AkihiroSuda PTAL

kolyshkin

LGTM

kolyshkin · 2021-06-21T21:14:27Z

We're in the middle of 1.0.0 GA release. Once it's done, I think we can merge this.

mrunalp

LGTM

kolyshkin · 2021-06-25T03:10:52Z

There's something wrong with CI :(

kolyshkin · 2021-06-25T03:15:52Z

Might be caused by #3029. @liusdu can you take a look? The failed CI can be seen from the Actions tab, https://github.com/opencontainers/runc/actions

liusdu force-pushed the checkpoint branch from 86f54eb to 213a805 Compare April 17, 2021 09:16

kolyshkin reviewed Apr 17, 2021

View reviewed changes

Comment thread tests/integration/checkpoint.bats Outdated

liusdu force-pushed the checkpoint branch from 213a805 to f5bdefe Compare April 17, 2021 23:17

kolyshkin reviewed Apr 21, 2021

View reviewed changes

Comment thread tests/integration/checkpoint.bats Outdated

options: ["bind"]

}]'

simple_cr

Copy link
Copy Markdown

Contributor

kolyshkin Apr 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra empty line

kolyshkin reviewed Apr 21, 2021

View reviewed changes

kolyshkin added the area/checkpoint-restore label Apr 21, 2021

liusdu force-pushed the checkpoint branch from f5bdefe to 4fa8536 Compare April 22, 2021 14:54

kolyshkin reviewed Apr 23, 2021

View reviewed changes

Comment thread tests/integration/checkpoint.bats

checkpoint: resolve symlink for external bind mount

da22625

runc resolves symlink before doing bind mount. So we should save original path while formatting CriuReq for checkpoint. Signed-off-by: Liu Hua <weldonliu@tencent.com>

liusdu force-pushed the checkpoint branch from 4fa8536 to da22625 Compare April 25, 2021 01:50

kolyshkin approved these changes Apr 30, 2021

View reviewed changes

kolyshkin closed this May 3, 2021

kolyshkin reopened this May 3, 2021

kolyshkin mentioned this pull request May 3, 2021

VERSION: release 1.0.0-rc94 #2927

Closed

7 tasks

liusdu closed this May 6, 2021

liusdu reopened this May 6, 2021

haircommander reviewed May 7, 2021

View reviewed changes

Comment thread tests/integration/checkpoint.bats

cyphar reviewed May 12, 2021

View reviewed changes

kolyshkin added this to the 1.1.0 milestone Jun 21, 2021

kolyshkin approved these changes Jun 21, 2021

View reviewed changes

kolyshkin requested review from AkihiroSuda, cyphar and mrunalp June 24, 2021 21:44

mrunalp approved these changes Jun 25, 2021

View reviewed changes

mrunalp merged commit 1079288 into opencontainers:master Jun 25, 2021

This was referenced Jun 25, 2021

CI is broken on master #3042

Closed

Revert "checkpoint: resolve symlink for external bind mount" #3043

Merged

liusdu mentioned this pull request Jun 28, 2021

checkpoint: resolve symlink for external bind mount (take II) #3047

Merged


		func (c linuxContainer) addCriuDumpMount(req criurpc.CriuReq, m *configs.Mount) {
		mountDest := strings.TrimPrefix(m.Destination, c.config.Rootfs)

Conversation

liusdu commented Apr 13, 2021

Uh oh!

kolyshkin commented Apr 14, 2021

Uh oh!

kolyshkin commented Apr 15, 2021

Uh oh!

liusdu commented Apr 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

liusdu commented Apr 21, 2021

Uh oh!

kolyshkin Apr 21, 2021

Choose a reason for hiding this comment

Uh oh!

kolyshkin Apr 21, 2021

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Apr 21, 2021

Uh oh!

adrianreber commented Apr 21, 2021

Uh oh!

liusdu commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liusdu commented Apr 22, 2021

Uh oh!

Uh oh!

liusdu commented Apr 29, 2021

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented May 3, 2021

Uh oh!

Uh oh!

haircommander commented May 10, 2021

Uh oh!

cyphar May 12, 2021

Choose a reason for hiding this comment

Uh oh!

liusdu May 13, 2021 • 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!

adrianreber May 13, 2021

Choose a reason for hiding this comment

Uh oh!

liusdu Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

liusdu commented Jun 21, 2021

Uh oh!

haircommander commented Jun 21, 2021

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Jun 21, 2021

Uh oh!

mrunalp left a comment

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Jun 25, 2021

Uh oh!

kolyshkin commented Jun 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

liusdu commented Apr 17, 2021 •

edited

Loading

liusdu commented Apr 22, 2021 •

edited

Loading

liusdu May 13, 2021 •

edited

Loading