Copy files from host to guest #1041

devimc · 2018-12-17T15:07:37Z

No description provided.

virtcontainers/capabilities.go

virtcontainers/kata_agent.go

sboeuf · 2018-12-17T16:29:11Z

virtcontainers/kata_agent.go

+	if !caps.is9pSupported() {
+		return nil
+	}
+
 	// Adding the shared volume.
 	// This volume contains all bind mounted container bundles.


Please extend the comment here, saying that this code path should only be executed when 9p is supported.

sboeuf · 2018-12-17T16:47:55Z

virtcontainers/kata_agent.go

@@ -1003,6 +1054,14 @@ func (k *kataAgent) createContainer(sandbox *Sandbox, c *Container) (p *Process,

 	k.handleShm(grpcSpec, sandbox)

+	caps := sandbox.hypervisor.capabilities()
+	if !caps.is9pSupported() {


I would do this check and the file copy from c.mountSharedDirMounts() instead. This means we centralize everything in the container.go implementation. But that also means that you need to define copyFile() as part of the agent interface, which makes sense to me as you're asking to copy a file to the guest through the agent support.

egernst · 2018-12-17T18:49:07Z

I am having mixed-thoughts regarding the capability flag for the hypervisor. I would like this to be .toml based in addition, perhaps. WDYT? @mcastelino @sboeuf @devimc @sameo

sboeuf · 2018-12-17T19:02:02Z

@egernst

I am having mixed-thoughts regarding the capability flag for the hypervisor. I would like this to be .toml based in addition, perhaps. WDYT? @mcastelino @sboeuf @devimc @sameo

Mid-term, definitely. If we think about real use cases, then we might need to cover the case where we want to use QEMU without 9pfs (even if 9p is supported), which means the internal decision should be based on hypervisor capabilities AND input from user (configuration.toml basically).

Long term, maybe we simply want to get rid of 9p, and we could call this capability filesystemSharingSupport, which would be covered by virtio-fs in case of QEMU, and which would be false in case of FC for instance.

mcastelino

It would be helpful at the top level to explain what we lose when we lose 9p.
Also what features we lose. The code that handles storage seem to indicate we lost volume support, which means configmaps for example. Is this true?

mcastelino · 2018-12-17T19:24:02Z

virtcontainers/capabilities.go

+	caps.flags |= hotplugUnsupported
+}
+
+func (caps *capabilities) is9pSupported() bool {


@egernst instead of calling this is9pSupported, should this not be something more generic. 9p is one way to share files between host and the VM?

Yeah we could call it filesystemSharingSupport. @egernst @devimc ?

+1 , @egernst ?

mcastelino · 2018-12-17T19:24:23Z

virtcontainers/capabilities.go

@@ -9,6 +9,8 @@ const (
 	blockDeviceSupport = 1 << iota
 	blockDeviceHotplugSupport
 	multiQueueSupport
+	hotplugUnsupported
+	plan9FSUnsupported


@egernst instead of calling this is9pSupported, should this not be something more generic. 9p is one way to share files between host and the VM?

mcastelino · 2018-12-17T19:39:00Z

virtcontainers/kata_agent.go

+	caps := sandbox.hypervisor.capabilities()
+	if !caps.is9pSupported() {
+		// 9p is not supported, files must be copied
+		if err := k.copyFiles(c.mounts, newMounts); err != nil {


@egernst does this mean even files coming in from tmpfs, secrets all of them get copied? What happens to volumes.

mcastelino · 2018-12-17T19:40:36Z

virtcontainers/kata_agent.go

+		// This is where at least some of the host config files
+		// (resolv.conf, etc...) and potentially all container
+		// rootfs will reside.
+		sharedVolume := &grpc.Storage{


@egernst what is the else case here. Do we lose support for volumes?

mcastelino · 2018-12-17T19:45:19Z

Long term, maybe we simply want to get rid of 9p, and we could call this capability filesystemSharingSupport, which would be covered by virtio-fs in case of QEMU, and which would be false in case of FC for instance.

Yes, this will decouple it from the specific implementation.

devimc · 2018-12-17T20:07:45Z

Depends-on kata-containers/agent#433

sboeuf · 2018-12-17T22:08:10Z

/test

sboeuf · 2018-12-18T18:20:09Z

@devimc kata-containers/agent#433 got merged! Could you please update this PR :)

jodh-intel · 2018-12-19T10:42:39Z

virtcontainers/container.go

+		// bind mount it in the shared directory.
+		caps := c.sandbox.hypervisor.capabilities()
+		if !caps.isFsSharingSupported() {
+			// filesystem sharing is not supported, files must be copied


How about converting this comment into a log call?

jodh-intel · 2018-12-19T10:44:21Z

virtcontainers/hyperstart_agent.go

+
+func (h *hyper) copyFile(src, dst string) error {
+	// hyperstart-agent does not support copyFile
+	return nil


Shouldn't this be...

Suggested change

return nil

return errors.New("hyperstart-agent does not support copyFile")

based on other implementations https://github.com/kata-containers/runtime/blob/master/virtcontainers/hyperstart_agent.go#L999-L1007

I don't think so

jodh-intel · 2018-12-19T10:45:06Z

virtcontainers/kata_agent.go

+
+	err := unix.Stat(src, &st)
+	if err != nil {
+		return fmt.Errorf("Could get file %s information", src)


Suggested change

return fmt.Errorf("Could get file %s information", src)

return fmt.Errorf("Could not get file %s information: %v", src, err)

good catch! thanks

jodh-intel · 2018-12-19T10:46:12Z

virtcontainers/kata_agent.go

+
+	b, err := ioutil.ReadFile(src)
+	if err != nil {
+		return fmt.Errorf("Could not read file %s", src)


Again, it would help to include the error so the user can see why the read failed.

yes, thanks 😄

Brings support to copy file from host to guest shortlog: 169d755 protocols/grpc: implement function to copy files ff87c26 virtio-mmio: Add support for virtio-mmio blk devices b9c5d5b libcontainer: use /run as root containers path 092f1a0 block: add support of block storage driver "nvdimm" Signed-off-by: Julio Montes <[email protected]>

devimc · 2018-12-19T15:26:58Z

/test

egernst

@devimc can you update commit msg/header for

vc: capabilities: add capability flags for 9p

I like the new name - let's update the msg for this.

Not all hypervisors support filesystem sharing. Add capability flags to track this. Since most hypervisor implementations in Kata *do* support this, the set semantices are reversed (ie, set the flag if you do not support the feature). Fixes: kata-containers#1022 Signed-off-by: Eric Ernst <[email protected]> Signed-off-by: Julio Montes <[email protected]>

Files are copied over gRPC and there is no limit in size of the files that can be copied. Small files are copied using just one gRPC call while big files are copied by parts. Signed-off-by: Julio Montes <[email protected]>

devimc · 2018-12-19T15:55:52Z

@egernst changes applied, thanks

devimc · 2018-12-19T15:56:01Z

/test

devimc · 2018-12-19T15:56:35Z

@jodh-intel changes applied, thanks

egernst

"Neither create shared directory nor add 9p device if hypervisor doesn't support
file sharing since files will be copied over gRPC using agent's copyFile
function."

Suggested reword:
"If the hypervisor does not support filesystem sharing (for example, 9p), files will be copied over gRPC using the copyFile request function"

If the hypervisor does not support filesystem sharing (for example, 9p), files will be copied over gRPC using the copyFile request function. Signed-off-by: Julio Montes <[email protected]>

Copy files to contaier's rootfs if hypervisor doesn't supports filesystem sharing, otherwise bind mount them in the shared directory. see kata-containers#1031 Signed-off-by: Julio Montes <[email protected]>

devimc · 2018-12-19T15:59:12Z

@egernst ok, sounds good

sameo

LGTM

sameo · 2018-12-19T15:59:45Z

virtcontainers/kata_agent.go

@@ -63,6 +65,7 @@ var (
 	shmDir               = "shm"
 	kataEphemeralDevType = "ephemeral"
 	ephemeralPath        = filepath.Join(kataGuestSandboxDir, kataEphemeralDevType)
+	grpcMaxDataSize      = int64(1024 * 1024)


Where is that value coming from?

Good question! @devimc - is this just a restriction added to avoid a DoS or something?

In other words: Is this related to any gRPC limitations?

yes, it's to avoid DoS attacks, I'd prefer to use small chunks, but I can change it if you want

according with this https://godoc.org/google.golang.org/grpc#MaxRecvMsgSize the default size 4MB , I can use it but if the value changes to 1000MB probably we'll face DoS attacks

jodh-intel

lgtm

jodh-intel · 2018-12-19T16:08:09Z

virtcontainers/kata_agent.go

@@ -63,6 +65,7 @@ var (
 	shmDir               = "shm"
 	kataEphemeralDevType = "ephemeral"
 	ephemeralPath        = filepath.Join(kataGuestSandboxDir, kataEphemeralDevType)
+	grpcMaxDataSize      = int64(1024 * 1024)


Good question! @devimc - is this just a restriction added to avoid a DoS or something?

devimc · 2018-12-19T16:25:53Z

/test

devimc force-pushed the topic/no9pCopyFiles branch from ea1982f to 96c3e34 Compare December 17, 2018 15:15

devimc mentioned this pull request Dec 17, 2018

hypervisor cleanup #1023

Closed

sboeuf suggested changes Dec 17, 2018

View reviewed changes

devimc force-pushed the topic/no9pCopyFiles branch from 96c3e34 to bd2de8c Compare December 17, 2018 18:05

mcastelino reviewed Dec 17, 2018

View reviewed changes

devimc force-pushed the topic/no9pCopyFiles branch from bd2de8c to aab7c26 Compare December 17, 2018 20:07

devimc added the do-not-merge label Dec 17, 2018

devimc force-pushed the topic/no9pCopyFiles branch from aab7c26 to cc72c85 Compare December 17, 2018 20:11

sboeuf approved these changes Dec 17, 2018

View reviewed changes

mcastelino mentioned this pull request Dec 17, 2018

Add support for firecracker in addition to QEMU as a supported hypervisor in Kata. #1031

Closed

11 tasks

devimc force-pushed the topic/no9pCopyFiles branch from cc72c85 to efe4298 Compare December 18, 2018 20:26

jodh-intel reviewed Dec 19, 2018

View reviewed changes

devimc force-pushed the topic/no9pCopyFiles branch 2 times, most recently from 996970e to d038fd8 Compare December 19, 2018 15:08

devimc force-pushed the topic/no9pCopyFiles branch 2 times, most recently from c39b991 to 483aab9 Compare December 19, 2018 15:20

egernst reviewed Dec 19, 2018

View reviewed changes

Eric Ernst and others added 2 commits December 19, 2018 09:54

virtcontainers: copy files form host to guest

6291762

Files are copied over gRPC and there is no limit in size of the files that can be copied. Small files are copied using just one gRPC call while big files are copied by parts. Signed-off-by: Julio Montes <[email protected]>

devimc force-pushed the topic/no9pCopyFiles branch from 483aab9 to 87509c5 Compare December 19, 2018 15:55

devimc removed the do-not-merge label Dec 19, 2018

egernst reviewed Dec 19, 2018

View reviewed changes

Julio Montes added 2 commits December 19, 2018 09:58

virtcontainers: Check file sharing support

bc31844

If the hypervisor does not support filesystem sharing (for example, 9p), files will be copied over gRPC using the copyFile request function. Signed-off-by: Julio Montes <[email protected]>

virtcontainers: copy or bind mount shared file

378d815

Copy files to contaier's rootfs if hypervisor doesn't supports filesystem sharing, otherwise bind mount them in the shared directory. see kata-containers#1031 Signed-off-by: Julio Montes <[email protected]>

devimc force-pushed the topic/no9pCopyFiles branch from 87509c5 to 378d815 Compare December 19, 2018 15:58

sameo approved these changes Dec 19, 2018

View reviewed changes

jodh-intel approved these changes Dec 19, 2018

View reviewed changes

egernst merged commit b51c57e into kata-containers:master Dec 19, 2018

mcastelino mentioned this pull request Jan 17, 2019

Copy Files: Allow all hypervisor implementations to support copyfiles #1147

Closed

devimc deleted the topic/no9pCopyFiles branch April 8, 2019 14:51

	return nil
	return errors.New("hyperstart-agent does not support copyFile")

	return fmt.Errorf("Could get file %s information", src)
	return fmt.Errorf("Could not get file %s information: %v", src, err)

Copy files from host to guest #1041

Copy files from host to guest #1041

Conversation

devimc commented Dec 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egernst commented Dec 17, 2018 • edited Loading

sboeuf commented Dec 17, 2018

mcastelino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcastelino commented Dec 17, 2018

devimc commented Dec 17, 2018

sboeuf commented Dec 17, 2018

sboeuf commented Dec 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devimc commented Dec 19, 2018

egernst left a comment

Choose a reason for hiding this comment

devimc commented Dec 19, 2018

devimc commented Dec 19, 2018

devimc commented Dec 19, 2018

egernst left a comment

Choose a reason for hiding this comment

devimc commented Dec 19, 2018

sameo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodh-intel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devimc commented Dec 19, 2018

egernst commented Dec 17, 2018 •

edited

Loading