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

mounts: Add check for system volumes#1418

Merged
amshinde merged 2 commits intokata-containers:masterfrom
amshinde:system-mounts
Apr 12, 2019
Merged

mounts: Add check for system volumes#1418
amshinde merged 2 commits intokata-containers:masterfrom
amshinde:system-mounts

Conversation

@amshinde
Copy link
Copy Markdown
Member

We handle system directories differently, if its a bind mount
we mount the guest system directory to the container mount and
skip the 9p share mount.
However, we should not do this for docker volumes which are directories
created by Docker.

This introduces a Docker specific check, but that is the only
information available to us at the OCI layer.
Fixes #1417 

Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

We handle system directories differently, if its a bind mount
we mount the guest system directory to the container mount and
skip the 9p share mount.
However, we should not do this for docker volumes which are directories
created by Docker.

This introduces a Docker specific check, but that is the only
information available to us at the OCI layer.

Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>
k8s host empty-dir is equivalent to docker volumes.
For this case, we should just use the host directory even
for system directories.

Move the isEphemeral function to virtcontainers to not
introduce cyclic dependency.

Fixes kata-containers#1417

Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>
@amshinde amshinde changed the title wip : mounts: Add check for system volumes mounts: Add check for system volumes Mar 28, 2019
@egernst
Copy link
Copy Markdown
Member

egernst commented Apr 3, 2019

@lifupan @bergwolf PTAL?

@lifupan
Copy link
Copy Markdown
Member

lifupan commented Apr 3, 2019

/retest

@devimc
Copy link
Copy Markdown

devimc commented Apr 3, 2019

fixes #1472 ?

Copy link
Copy Markdown
Member

@jcvenegas jcvenegas left a comment

Choose a reason for hiding this comment

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

Hi @amshinde weekly ping update :)

}

const (
dockerVolumePrefix = "/var/lib/docker/volumes"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be great to check where is defined this is docker code just to add as a comment reference in case it changes in the future.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

var/lib/docker is configurable at docker startup I may check only from volumes

func isEmptyDir(path string) bool {
splitSourceSlice := strings.Split(path, "/")
if len(splitSourceSlice) > 1 {
storageType := splitSourceSlice[len(splitSourceSlice)-2]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

magic number

}

func TestIsDockerVolume(t *testing.T) {
path := "/var/lib/docker/volumes/00da1347c7cf4f15db35f/_data"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

dockerVolumePrefix

@amshinde amshinde merged commit 228d151 into kata-containers:master Apr 12, 2019
@amshinde amshinde deleted the system-mounts branch July 11, 2019 22:27
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.

5 participants