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

checkpoint: handle config.Devices and config.MaskPaths #1110

Merged
merged 1 commit into from
Jan 10, 2017

Conversation

avagin
Copy link
Contributor

@avagin avagin commented Oct 13, 2016

In user namespaces devices are bind-mounted from the host, so
we need to add them as external mounts for CRIU.

Reported-by: @boucher

c.addCriuRestoreMount(req, m)
}

if c.config.Namespaces.Contains(configs.NEWUSER) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really know enough about this to know the answer, but is it definitely the case that we only want to add mounts for c.config.Devices if there's a newuser namespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two cases when devices are bind-mounted from host:

  • if a container is executed in its own userns
  • if a container is executed from a non-root userns
// Create the device nodes in the container.
func createDevices(config *configs.Config) error {
        useBindMount := system.RunningInUserNS() || config.Namespaces.Contains(configs.NEWUSER)

Actually we can add this ext-mounts always and they will be ignored if devices should not be bind-mounted.

Currently I have a problem with MaskPaths. Because behaviour is different for files and directories, but we don't know who is who when a container isn't running.

// For files, maskPath bind mounts /dev/null over the top of the specified path.
// For directories, maskPath mounts read-only tmpfs over the top of the specified path.

@avagin avagin force-pushed the cpt-in-userns branch 2 times, most recently from f5f4609 to 83ed56a Compare October 17, 2016 19:34
@crosbymichael
Copy link
Member

Should these only be added when userns is enabled?

@boucher
Copy link
Contributor

boucher commented Oct 17, 2016

@crosbymichael heh, it was originally implemented that way and I asked the opposite question. I don't know the answer.

@crosbymichael
Copy link
Member

Well when userns is not enabled we don't create bind mounts for devices, we just mknod the device so I don't think it applies.

@avagin
Copy link
Contributor Author

avagin commented Oct 17, 2016

        useBindMount := system.RunningInUserNS() || config.Namespaces.Contains(configs.NEWUSER)

@crosbymichael We create bind-mounts if userns is enabled for a container and if a container is started from a non-root userns. I suggest to always set these options for CRIU, because if devices are not mounted, CRIU will do nothing with them.

In user namespaces devices are bind-mounted from the host, so
we need to add them as external mounts for CRIU.

Reported-by: Ross Boucher <[email protected]>
Signed-off-by: Andrei Vagin <[email protected]>
@crosbymichael
Copy link
Member

@avagin so even though you add all these mounts for the devices in criu it won't do anything with them if they were not originally mounted?

@avagin
Copy link
Contributor Author

avagin commented Nov 10, 2016

@crosbymichael No, it won't.

@crosbymichael
Copy link
Member

crosbymichael commented Jan 9, 2017

LGTM

Approved with PullApprove

1 similar comment
@hqhq
Copy link
Contributor

hqhq commented Jan 10, 2017

LGTM

Approved with PullApprove

@hqhq hqhq merged commit db99936 into opencontainers:master Jan 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants