Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

virtcontainers: Do not pass /dev/shm as 9p mount #191

Merged
merged 1 commit into from
Apr 10, 2018

Conversation

amshinde
Copy link
Member

@amshinde amshinde commented Apr 5, 2018

All bind mounts are now passed to the guest with 9p.
We need to exclude /dev/shm, as this is passed as a bind mount
in the spec. We handle /dev/shm in the guest by allocating
memory for it on the guest side. Passing /dev/shm as a 9p mount
was causing it to be mounted twice.

Fixes #190

Signed-off-by: Archana Shinde [email protected]

@amshinde amshinde added the review label Apr 5, 2018
@amshinde
Copy link
Member Author

amshinde commented Apr 5, 2018

cc @grahamwhaley

@grahamwhaley
Copy link
Contributor

Unit tests failed I think:

--- FAIL: TestContainerAddResources (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x50 pc=0xc87264]

Having a look at the docker docs, I think we can treat /dev/shm as special enough to special case.
I will note - we don't support the --shm-size option already, and I've just noticed there is also a --ipc=non option that enables a 'private ipc namespace' - which, err, is sort of what we have by default with VMs??

@grahamwhaley grahamwhaley requested a review from sboeuf April 5, 2018 17:06
@jodh-intel
Copy link
Contributor

@amshinde, @sboeuf, @grahamwhaley - yep, that limitation is recorded in kata-containers/kata-containers#21.

@jodh-intel
Copy link
Contributor

A quick look suggests that --shm-size is not passed in config.json fwiw.

Copy link
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

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

With the feedback in the comments, as a specific quick fix for this double mount shm==9p issue,
lgtm

but we should discuss further if we need a longer term plan for shm support and equality with soft containers, and open Issues as appropriate.

@grahamwhaley
Copy link
Contributor

ouch, not passed in the config.json - I guess it might set the size on the host ... which will make it a little more awkward if we do ever support that setting (well, OK, we look up what is on the host and pass it through to the agent to set it I guess)

@amshinde
Copy link
Member Author

amshinde commented Apr 5, 2018

@grahamwhaley Yes, we need to inspect the mount on the host to find out the size and pass that to the guest. Had created an issue for this in CC long back.

@amshinde
Copy link
Member Author

amshinde commented Apr 5, 2018

@grahamwhaley Unit tests are failing related to CPU resources for sandbox. Not related to this change.
@devimc @chavafg Can you take a look. It might be worth restarting the CI run. @chavafg Can you restart the CI jobs and add my account as well, so that I can restart jobs.

@chavafg
Copy link
Contributor

chavafg commented Apr 5, 2018

@amshinde I think this is related also to: kata-containers/tests#203 which enabled unit tests on jenkins.

@amshinde
Copy link
Member Author

amshinde commented Apr 5, 2018

@chavafg Oh, so we were not running unit tests before that change. I see a similar failure on your other PR : http://kata-jenkins-ci.westus2.cloudapp.azure.com/job/kata-containers-runtime-ubuntu-16-04-PR/163/console

@devimc Do you have any input on this? Do you think we should disable those tests till you fix the issue?

@devimc
Copy link

devimc commented Apr 5, 2018

@amshinde #193 fixes unit tests

// We need to treat /dev/shm as a special case. This is passed as a bind mount in the spec,
// but it does not make sense to pass this as a 9p mount from the host side.
// This needs to be handled purely in the guest, by allocating memory for this inside the VM.
if m.Destination == "/dev/shm" {
Copy link
Member

Choose a reason for hiding this comment

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

Both runc spec and docker-runc spec creates an OCI spec listing /dev/shm as tmpfs. How did you get the bind mounted /dev/shm in OCI spec?

Copy link
Member

Choose a reason for hiding this comment

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

@bergwolf
It looks like docker will create a bind mount for "/dev/shm", I just checked config file from docker, it adds this item:

{"destination":"/dev/shm","type":"bind","source":"/var/lib/docker/containers/fc243bca3002a1264072d07086b77970ab9e6a8da9c5143b0af6c0fbb1f2367d/shm","options":["rbind","rprivate"]}

So this change makes sense in this case.

Copy link
Member

Choose a reason for hiding this comment

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

@WeiZhang555 Are there other files in /dev/ and /sys directory we want to ignore?

Copy link
Member

Choose a reason for hiding this comment

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

Default mounts are here:

    "mounts": [
        {
            "destination": "/proc",
            "options": [
                "nosuid",
                "noexec",
                "nodev"
            ],
            "source": "proc",
            "type": "proc"
        },
        {
            "destination": "/dev",
            "options": [
                "nosuid",
                "strictatime",
                "mode=755"
            ],
            "source": "tmpfs",
            "type": "tmpfs"
        },
        {
            "destination": "/dev/pts",
            "options": [
                "nosuid",
                "noexec",
                "newinstance",
                "ptmxmode=0666",
                "mode=0620",
                "gid=5"
            ],
            "source": "devpts",
            "type": "devpts"
        },
        {
            "destination": "/sys",
            "options": [
                "nosuid",
                "noexec",
                "nodev",
                "ro"
            ],
            "source": "sysfs",
            "type": "sysfs"
        },
        {
            "destination": "/sys/fs/cgroup",
            "options": [
                "ro",
                "nosuid",
                "noexec",
                "nodev"
            ],
            "source": "cgroup",
            "type": "cgroup"
        },
        {
            "destination": "/dev/mqueue",
            "options": [
                "nosuid",
                "noexec",
                "nodev"
            ],
            "source": "mqueue",
            "type": "mqueue"
        },
        {
            "destination": "/etc/resolv.conf",
            "options": [
                "rbind",
                "rprivate"
            ],
            "source": "/var/lib/docker/containers/fc243bca3002a1264072d07086b77970ab9e6a8da9c5143b0af6c0fbb1f2367d/resolv.conf",
            "type": "bind"
        },
        {
            "destination": "/etc/hostname",
            "options": [
                "rbind",
                "rprivate"
            ],
            "source": "/var/lib/docker/containers/fc243bca3002a1264072d07086b77970ab9e6a8da9c5143b0af6c0fbb1f2367d/hostname",
            "type": "bind"
        },
        {
            "destination": "/etc/hosts",
            "options": [
                "rbind",
                "rprivate"
            ],
            "source": "/var/lib/docker/containers/fc243bca3002a1264072d07086b77970ab9e6a8da9c5143b0af6c0fbb1f2367d/hosts",
            "type": "bind"
        },
        {
            "destination": "/dev/shm",
            "options": [
                "rbind",
                "rprivate"
            ],
            "source": "/var/lib/docker/containers/fc243bca3002a1264072d07086b77970ab9e6a8da9c5143b0af6c0fbb1f2367d/shm",
            "type": "bind"
        }
    ],

In this bind mount case, ignoring shm could be enough.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @WeiZhang555 !

@bergwolf
Copy link
Member

bergwolf commented Apr 8, 2018

lgtm!

Approved with PullApprove

@sboeuf
Copy link

sboeuf commented Apr 10, 2018

@amshinde please rebase on top of #193 so that we can get the CI passing here.

All bind mounts are now passed to the guest with 9p.
We need to exclude /dev/shm, as this is passed as a bind mount
in the spec. We handle /dev/shm in the guest by allocating
memory for it on the guest side. Passing /dev/shm as a 9p mount
was causing it to be mounted twice.

Fixes kata-containers#190

Signed-off-by: Archana Shinde <[email protected]>
@codecov
Copy link

codecov bot commented Apr 10, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@be151cb). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #191   +/-   ##
=========================================
  Coverage          ?   65.59%           
=========================================
  Files             ?       73           
  Lines             ?     7635           
  Branches          ?        0           
=========================================
  Hits              ?     5008           
  Misses            ?     2085           
  Partials          ?      542
Impacted Files Coverage Δ
virtcontainers/container.go 48.61% <0%> (ø)

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 be151cb...e96d3ef. Read the comment docs.

@amshinde
Copy link
Member Author

@sboeuf @devimc Can we merge this?

@sboeuf sboeuf merged commit f74f61e into kata-containers:master Apr 10, 2018
@sboeuf sboeuf removed the review label Apr 10, 2018
zklei pushed a commit to zklei/runtime that referenced this pull request Jun 13, 2019
The statement returning an error in case VmPath is empty is wrong
because an Id can be provided instead. This patch fixes this behavior
and generates an error only if VmPath and Id are both empty.

Fixes kata-containers#191

Signed-off-by: Sebastien Boeuf <[email protected]>
@amshinde amshinde deleted the handle-shm-mount branch July 11, 2019 22:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants