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

mounts: Add check for system volumes #1418

Merged
merged 2 commits into from
Apr 12, 2019

Conversation

amshinde
Copy link
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
Member

egernst commented Apr 3, 2019

@lifupan @bergwolf PTAL?

@lifupan
Copy link
Member

lifupan commented Apr 3, 2019

/retest

@devimc
Copy link

devimc commented Apr 3, 2019

fixes #1472 ?

Copy link
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 :)

@@ -326,3 +326,69 @@ func bindUnmountAllRootfs(ctx context.Context, sharedDir string, sandbox *Sandbo
}
}
}

const (
dockerVolumePrefix = "/var/lib/docker/volumes"
Copy link
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
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
Member

Choose a reason for hiding this comment

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

magic number

@@ -282,3 +289,46 @@ func TestIsDeviceMapper(t *testing.T) {
t.Fatal()
}
}

func TestIsDockerVolume(t *testing.T) {
path := "/var/lib/docker/volumes/00da1347c7cf4f15db35f/_data"
Copy link
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