Add image volume support. #241

Random-Liu · 2017-09-12T06:22:22Z

For image volume, currently we only:

Create host directory and mount into the image;
Change the ownership of the host directory to be the same with mount point inside the image.

What we are going to do in the future:

Consider whether we need to copy existing content in the image into the volume. Support ExposedPorts and Volumes in image config. #186.
Consider whether we should add the helper into containerd Add WithVolume helper in client. containerd#1482.
Consider how to properly manage these volumes in kubelet Ensure that host disk space used by Docker volume is accounted for kubernetes/kubernetes#52032.

@ijc
Signed-off-by: Lantao Liu lantaol@google.com

Random-Liu · 2017-09-12T06:23:31Z

I've verified this PR, ownership and rw of the image volume is set properly now:

# crictl --runtime-endpoint=/var/run/cri-containerd.sock exec -s 2545316f7d3ff883c80da6480833804d5bd0d463e2080e0777147e2fe4718f99 cat /proc/self/mountinfo | grep "data"
314 295 8:1 /var/lib/cri-containerd/sandboxes/dadf4bf511917af2a595d7d12356bdfabb620190c27f0e8ac67b65f7ae755d6c/hosts /etc/hosts ro,relatime - ext4 /dev/sda1 rw,data=ordered
315 295 8:1 /var/lib/cri-containerd/sandboxes/dadf4bf511917af2a595d7d12356bdfabb620190c27f0e8ac67b65f7ae755d6c/resolv.conf /etc/resolv.conf ro,relatime - ext4 /dev/sda1 rw,data=ordered
317 295 8:1 /var/lib/cri-containerd/containers/2545316f7d3ff883c80da6480833804d5bd0d463e2080e0777147e2fe4718f99/volumes/4281438d8117f1b6e33a10658d5e5290035c6a63dd68d6abe87bf97f3ec6b010 /data/configdb rw,relatime - ext4 /dev/sda1 rw,data=ordered
318 295 8:1 /var/lib/cri-containerd/containers/2545316f7d3ff883c80da6480833804d5bd0d463e2080e0777147e2fe4718f99/volumes/abe3b46973580e81795394a5067ad98717bf1d462b5eb42805e6c8b2558a09bf /data/db rw,relatime - ext4 /dev/sda1 rw,data=ordered
# crictl --runtime-endpoint=/var/run/cri-containerd.sock exec -s 2545316f7d3ff883c80da6480833804d5bd0d463e2080e0777147e2fe4718f99 ls -al /data
total 16
drwxr-xr-x 4 root    root    4096 Sep  8 08:09 .
drwxr-xr-x 1 root    root    4096 Sep 12 06:31 ..
drwxr-xr-x 2 mongodb mongodb 4096 Sep 12 06:31 configdb
drwxr-xr-x 2 mongodb mongodb 4096 Sep 12 06:31 db

ijc · 2017-09-12T09:33:56Z

Consider whether we need to copy existing content in the image into the volume. Support ExposedPorts and Volumes in image config. #186.

I think we do, I found the sock-shop demo was relying on it (I think to preseed the database within the volume).

ijc · 2017-09-12T09:37:07Z

pkg/server/container_create.go

+	// Generate container runtime spec.
+	mounts := c.generateContainerMounts(getSandboxRootDir(c.rootDir, sandboxID), config)
+
+	spec, err := c.generateContainerSpec(id, sandboxPid, config, sandboxConfig, image.Config, append(mounts, volumeMounts...))


I think you need to subtract from volumeMounts anything which is also expliclty in mounts, since an explicit bind mount is supposed to take precedence over the implicit ones.

BTW -- this would also mean that as soon as Kubelet starts to understand these things and provide them via mounts directly it will magically stop trying to do things here.

I think you need to subtract from volumeMounts anything which is also expliclty in mounts, since an explicit bind mount is supposed to take precedence over the implicit ones.

Done.

BTW -- this would also mean that as soon as Kubelet starts to understand these things and provide them via mounts directly it will magically stop trying to do things here.

We'll see how CRI will change in that case, and make change our side correspondingly. Added TODO for it.

Signed-off-by: Lantao Liu <lantaol@google.com>

Random-Liu · 2017-09-13T00:55:06Z

I think we do, I found the sock-shop demo was relying on it (I think to preseed the database within the volume).

I'll do this in another PR. For this PR, I'll only copy the ownership, and see how things going.

ijc

LGTM

ijc · 2017-09-13T11:13:24Z

Consider whether we need to copy existing content in the image into the volume. #186.

I think we do, I found the sock-shop demo was relying on it (I think to preseed the database within the volume).

I'll do this in another PR. For this PR, I'll only copy the ownership, and see how things going.

It seems like I was mistaken -- I just fired up sock-shop with this PR and everything looked to be working ok. I was unable to login with any of the preseeded accounts but I made a quick patch to add copy up and it didn't seem to make a difference to that functionality. In any case I was able to manually add a new account.

Random-Liu · 2017-09-14T21:01:16Z

Apply LGTM based on #241 (review).

Will merge after test passes.

ijc · 2017-09-15T09:39:47Z

-- FAIL: TestGeneralContainerSpec (0.00s)

	Error Trace:	container_create_test.go:207

	Error:		Received unexpected error failed to set OCI bind mounts \
[«redacted by ijc»]: \
failed to resolve symlink "": lstat host-path-1: no such file or directory
	
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=0x18 pc=0x2158fa8]

For the panic I think specCheck should start with require.NotNil(t, spec). Or else one or more assert.Foo should be require.Foo so the test stops at that point. Perhaps the assert.NoError(t, err) after calling c.generateContainerSpec (in several places) should instead be `require.NoError(t, err).

The failed lstat appears to be from resolveSymbolicLink which appears to have been added by this PR, not sure why it is suddenly showing up as a failure now, I guess these tests run on master+pr rather than just the pr and there was a clash with #242.

ijc · 2017-09-15T10:13:42Z

I fixed the tests carried this in #247.

ijc · 2017-09-15T16:43:11Z

It seems like I was mistaken -- I just fired up sock-shop with this PR and everything looked to be working ok. I was unable to login with any of the preseeded accounts but I made a quick patch to add copy up and it didn't seem to make a difference to that functionality. In any case I was able to manually add a new account.

Sorry to flip-flop on this, but I tested with docker as the engine and I could login, so the issue here was that my copy-up prototype was somehow incomplete or wrong. I'm looking into why now, but we can move ahead with this PR/#247 in the meantime I think.

Random-Liu · 2017-09-15T21:28:00Z

Sorry to flip-flop on this, but I tested with docker as the engine and I could login, so the issue here was that my copy-up prototype was somehow incomplete or wrong. I'm looking into why now, but we can move ahead with this PR/#247 in the meantime I think.

No wonder. :p

Random-Liu · 2017-09-15T21:28:59Z

@ijc I'll review and merge your PR instead.

Random-Liu · 2017-09-15T21:34:03Z

Close this one, because this has been included in #247.

Random-Liu assigned abhi Sep 12, 2017

k8s-ci-robot added size/L cncf-cla: yes labels Sep 12, 2017

Random-Liu force-pushed the volumes-support branch from f6c1061 to 2ec3ba2 Compare September 12, 2017 06:40

Random-Liu added this to the v1.0.0-alpha.0 milestone Sep 12, 2017

ijc reviewed Sep 12, 2017

View reviewed changes

Add image volume support.

3e2e913

Signed-off-by: Lantao Liu <lantaol@google.com>

Random-Liu force-pushed the volumes-support branch from 2ec3ba2 to 3e2e913 Compare September 13, 2017 00:54

ijc approved these changes Sep 13, 2017

View reviewed changes

Random-Liu added the lgtm label Sep 14, 2017

ijc mentioned this pull request Sep 15, 2017

Add image volume support. #247

Merged

Random-Liu closed this Sep 15, 2017

Random-Liu deleted the volumes-support branch September 15, 2017 21:34

Add image volume support. #241

Add image volume support. #241

Uh oh!

Conversation

Random-Liu commented Sep 12, 2017

Uh oh!

Random-Liu commented Sep 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ijc commented Sep 12, 2017

Uh oh!

ijc Sep 12, 2017

Choose a reason for hiding this comment

Uh oh!

ijc Sep 12, 2017

Choose a reason for hiding this comment

Uh oh!

Random-Liu Sep 13, 2017

Choose a reason for hiding this comment

Uh oh!

Random-Liu commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ijc left a comment

Choose a reason for hiding this comment

Uh oh!

ijc commented Sep 13, 2017

Uh oh!

Random-Liu commented Sep 14, 2017

Uh oh!

ijc commented Sep 15, 2017

Uh oh!

ijc commented Sep 15, 2017

Uh oh!

ijc commented Sep 15, 2017

Uh oh!

Random-Liu commented Sep 15, 2017

Uh oh!

Random-Liu commented Sep 15, 2017

Uh oh!

Random-Liu commented Sep 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Random-Liu commented Sep 12, 2017 •

edited

Loading

Random-Liu commented Sep 13, 2017 •

edited

Loading