Implement directory volume for virtlet with 9pfs

[miaoyq]

This PR implement directory volume for virtlet with 9pfs.
With this, virtlet can use the normal volume created by k8s. (Like emptyDir, hostPath ……)

Signed-off-by: Yanqiang Miao [email protected]

---

This change is 

[CLAassistant]

All committers have signed the CLA.

[ivan4th]

Thank you for the PR! It's indeed an important contribution. There are a few quirks, though.
First of all, I tried to modify ubuntu example to mount some host path into the VM:

--- a/examples/ubuntu-vm.yaml
+++ b/examples/ubuntu-vm.yaml
@@ -27,3 +27,10 @@ spec:
     # tty and stdin required for `kubectl attach -t` to work
     tty: true
     stdin: true
+    volumeMounts:
+    - name: host-logs
+      mountPath: /hostlogs
+  volumes:
+  - name: host-logs
+    hostPath:
+      path: /var/log

After the VM booted, /virtletShared was mounted, but /hostlogs wasn't.
In cloud-init userdata, I saw this:

mounts:
- - virtletShared
  - /virtletShared
  - 9p
  - trans=virtio
- - virtletShared
- /hostlogs

which doesn't seem to be correct and indeed produces these fstab entries

virtletShared   /virtletShared  9p      trans=virtio,comment=cloudconfig        0       2
virtletShared   /hostlogs       auto    defaults,nofail,x-systemd.requires=cloud-init.service,comment=cloudconfig       0       2

The second one doesn't do the expected mount.

Another thing, is it important to have just one /virtletShared 9p mount in the VM or can we use multiple mounts? I'd use multiple mounts. Also, IMO we don't need extra FileSystemList object. Instead, VMVolume interface can be changed to have

Setup() (*libvirtxml.DomainDisk, error)

instead of current

Setup() (*libvirtxml.DomainDisk, *libvirtxml.DomainFilesystem, error)

Then I'd add filesystemVolume that implements VMVolume and does proper Setup() and Teardown() for filesystem volumes, and then filesystemSource (based on https://github.com/Mirantis/virtlet/blob/master/pkg/libvirttools/flexvolume_volumesource.go) that scans kubelet mount dirs. filesystemVolume.Setup() will return nil as its first return value
and the filesystem entry as its second one. It can be based upon current filesystemItem struct
with some parts from filesystemList.
After that, it should be possible to simplify cloudinit code that handles filesystem mounts.
It would be nice to have test coverage for both filesystemVolume / filesystemVolumeSource, at least as a test case in virtualization_test.go, and for corresponding cloud-init user data generation code.
Please correct me if I missed something while making these suggestions.
Besides, I'll post some comments for the code itself. Some of them may be irrelevant though if the code is updated according to the above scheme.

Also, we were experiencing some CI problems after recent changes in kubeadm-dind-cluster,
please rebase your PR on master to make e2e tests pass.

[ivan4th]

Reviewed 3 of 28 files at r1.
Reviewable status: 0 of 2 LGTMs obtained

---

pkg/libvirttools/cloudinit.go, line 491 at r1 (raw file):

			}
			if !haveFsMounts {
				fsMountScript := fmt.Sprintf("if ! mountpoint '%s'; then mkdir -p '%s' && mount -t 9p -o trans=virtio %s '%s'; fi",

There are quoting issues in similar code that handles mounts in Virtlet, but I think with this extensive host path mounting it's important for us to do proper quoting here, e.g. using https://github.com/kballard/go-shellquote which is already in glide.lock

---

pkg/libvirttools/cloudinit.go, line 541 at r1 (raw file):

func generateFsBasedVolumeMounts(mount types.VMMount) ([]interface{}, string, error) {
	sourcePath := path.Join(mountPointPathfor9pfs, path.Base(mount.HostPath))

This may cause conflicts easily as multiple host paths can have same path.Base.

---

pkg/libvirttools/filesystemlist.go, line 32 at r1 (raw file):

}

func generateMountTag(hostPath string) string {

This function appears not to be used anywhere

---

pkg/libvirttools/filesystemlist.go, line 119 at r1 (raw file):

	}
	if err == nil {
		err = syscall.Mount(fsItem.mount.HostPath, fsItem.volumeMountPoint, "bind", syscall.MS_BIND|syscall.MS_REC, "")

This adds Linux dependency to the code, breaking tests and syntax checking on mac. It probably needs to be moved to a separate file. Probably mounter interface from pkg/flexvolume can be reused, see here https://github.com/Mirantis/virtlet/tree/master/pkg/flexvolume In this case it'll be need to be moved to pkg/utils

[miaoyq]

The second one doesn't do the expected mount.

@ivan4th Thanks for test and review.
I tested this with VirtletCloudInitUserDataScript: "@virtlet-mount-script@" only. :-P, will fix this.
Cloud you have a try with VirtletCloudInitUserDataScript: "@virtlet-mount-script@"?

Another thing, is it important to have just one /virtletShared 9p mount in the VM or can we use multiple mounts? I'd use multiple mounts.

Sorry, I'm not clear what multiple mounts means, multiple volumes, or multiple technologies(e.g. 9pfs or others)?
In this PR, virtletShared is just a data transfer channel with 9p,

/var/lib/virtlet/volumes/[POD-ID] ----9p---> /virtletShared

When we want to mount a hostpath to VM,
In host side:
virtlet will mount the hostpath to /var/lib/virtlet/volumes/[POD-ID]/[VolumeName]

In VM side,
/virtletShared/[VolumeName] will be seen. Then, cloud-init will mount /virtletShared/[VolumeName] to containerPath

Also, IMO we don't need extra FileSystemList object. Instead, VMVolume interface can be changed to have

Setup() (*libvirtxml.DomainDisk, error)

instead of current

Setup() (*libvirtxml.DomainDisk,  *libvirtxml.DomainFilesystem, error)

Did you mean Setup() (*libvirtxml.DomainDisk, *libvirtxml.DomainFilesystem, error) instead of Setup() (*libvirtxml.DomainDisk, error)?

[miaoyq]

Then I'd add filesystemVolume that implements VMVolume and does proper Setup() and Teardown() for filesystem volumes, and then filesystemSource (based on https://github.com/Mirantis/virtlet/blob/master/pkg/libvirttools/flexvolume_volumesource.go) that scans kubelet mount dirs.

@ivan4th I will have a try in this way.

[ivan4th]

Sorry, I'm not clear what multiple mounts means, multiple volumes, or multiple technologies(e.g. 9pfs or others)?

I meant multiple volumes. My suggestion is to have multiple filesystems attached via 9p instead of just single virtletShared folder. Or am I missing some problems with such approach?

Did you mean Setup() (*libvirtxml.DomainDisk, *libvirtxml.DomainFilesystem, error) instead of Setup() (*libvirtxml.DomainDisk, error)?

Yes, that's what I mean, sorry for the typo.

[miaoyq]

I meant multiple volumes. My suggestion is to have multiple filesystems attached via 9p instead of just single virtletShared folder. Or am I missing some problems with such approach?

@ivan4th This pr have supported multiple volumes, see my comment above #739 (comment) :-)

virtlet will mount all volume that is dinfined by k8s mount to /var/lib/virtlet/volumes/[POD-ID]/[VolumeName] in host.

Because virtlet have mount /var/lib/virtlet/volumes/[POD-ID](host path) to /virtletShared (vm path) via 9pfs before. So Vm can see all contents of /var/lib/virtlet/volumes/[POD-ID]

In VM side, cloud-init will mount /virtletShared/[VolumeName] to containerPath.

[ivan4th]

Well, I do understand that the PR supports multiple volumes as it is. Problem is, bindmounting the volumes under proper dirs using cloud-init doesn't appear work well and is generally tricky (it may depend on the order of mounts in the particular cloud-init impl, whether it supports bindmounting at all, etc.) What I was suggesting is instead of mounting /var/lib/virtlet/volumes/[POD-ID] under /virtletShared using 9pfs we should have multiple 9pfs mounts, mounting each /var/lib/virtlet/volumes/[POD-ID]/[VolumeName] under corresponding directory inside the VM without extra bindmounting steps.

[ivan4th]

In VM side, cloud-init will mount /virtletShared/[VolumeName] to containerPath.

This is what appears to be somewhat tricky and fragile, so I'm suggesting multiple 9pfs filesystems instead (one 9pfs per mount) instead of just one /virtletShared for all the mounts.

[miaoyq]

This is what appears to be somewhat tricky and fragile, so I'm suggesting multiple 9pfs filesystems instead (one 9pfs per mount) instead of just one /virtletShared for all the mounts.

@ivan4th Ooh, Got it.
Will this reduce data transmission efficiency?

[ivan4th]

Frankly I doubt it'll have a serious performance impact, although I can't find any direct comparisons of one 9pfs mount vs multiple ones performance-wise. Actually, first we need to make this feature work correctly, and then we can add single-mount mode as an option later if we'll be seeing some noticeable performance degradation when using multiple dirs.

[miaoyq]

@ivan4th I have refactored the codes based on your opinion, could you please take another look?

There are quoting issues in similar code that handles mounts in Virtlet, but I think with this extensive host path mounting it's important for us to do proper quoting here, e.g. using https://github.com/kballard/go-shellquote which is already in glide.lock

I'm not clear about this, clould you please give a example, Thx :-)

[jellonek]

As you are touching this part of code, you can replace all '%s' with %s then enclose all passed strings with shellquote.Join(stringVariable) as in https://github.com/Mirantis/virtlet/blob/master/pkg/config/fields.go#L315 - in both places, moved https://github.com/Mirantis/virtlet/pull/739/files#diff-369a31e4190533405038d63a652ea7a4R527 and newly introduced https://github.com/Mirantis/virtlet/pull/739/files#diff-369a31e4190533405038d63a652ea7a4R534.

[ivan4th]

Concerning the shell quotes, we can do it ourselves later, that's not really a problem. Fixing chown is more important. Thanks!

Reviewed 1 of 28 files at r1.
Reviewable status: 1 change requests, 0 of 2 approvals obtained (waiting on @miaoyq)

---

pkg/libvirttools/fileownership.go, line 62 at r2 (raw file):

	}
	if err := chown(filePath, emulatorUser.uid, emulatorUser.gid); err != nil {
		return fmt.Errorf("can't set the owner of tapmanager socket: %v", err)

Please update this message, it can be quite misleading (although the problem was before your PR... But still let's fix it as this func is touced

---

pkg/libvirttools/fileownership.go, line 71 at r2 (raw file):

	return filepath.Walk(path, func(name string, info os.FileInfo, err error) error {
		if err == nil {
			err = os.Chown(name, uid, gid)

I'd suggest printing a warning when a file can't be chowned. Right now, you can't mount a directory that has broken symlink in it b/c chown will fail on that symlink.

---

pkg/libvirttools/filesystem_volumesource.go, line 2 at r2 (raw file):

/*
Copyright 2018 ZTE corporation

Could you please change this to standard Mirantis copyright? Of course, ZTE will still be free to use the code as we're using permissive Apache license yet our policies require that contributions have Mirantis copyright in the source code files. Sorry for the inconvenience...

---

pkg/libvirttools/filesystemvolume.go, line 2 at r2 (raw file):

/*
Copyright 2018 ZTE corporation

Same as above, could you change this copyright notice?

---

pkg/libvirttools/filesystemvolume.go, line 46 at r2 (raw file):

	err := os.MkdirAll(v.volumeMountPoint, 0777)
	if err == nil {
		err = ChownForEmulator(v.volumeMountPoint, true)

Why recursive chown here? The dir must be empty IIUC

---

pkg/libvirttools/filesystemvolume.go, line 52 at r2 (raw file):

	}
	if err == nil {
		err = ChownForEmulator(v.volumeMountPoint, true)

Frankly speaking, this behavior is not always necessary and sometimes dangerous. What if the user wants to mount /etc from the host? With this code, the whole /etc directory tree will be chown'ed. I'm afraid we need to add a new annotation for virtlet pods, e.g. VirtletChown9pfsMounts which, when set to "true", will enable chown, but the user must know what (s)he is doing. See here for an example on how to add an annotation and corresponding VMConfig field: https://github.com/Mirantis/virtlet/pull/739/files

Sorry for not noticing this behavior during my first review.

---

pkg/libvirttools/filesystemvolume.go, line 73 at r2 (raw file):

	}
	if err == nil {
		err = os.RemoveAll(v.volumeMountPoint)

I think this is unsafe. What if umount fails but e.g. due to some bug introduced later the error is swallowed? This may lead to data loss. I'd suggest using os.Remove() to remove the dir that will be empty after successful umount.

---

pkg/libvirttools/root_volumesource_test.go, line 132 at r2 (raw file):

			})

			_, err := rootVol.Setup()

This line is broken right now. From test results:

# github.com/Mirantis/virtlet/pkg/libvirttools
pkg/libvirttools/root_volumesource_test.go:132:11: assignment mismatch: 2 variables but 3 values
FAIL	github.com/Mirantis/virtlet/pkg/libvirttools [build failed]

[miaoyq]

@ivan4th Thanks for your review again. I have addressed all your comments, PTAL

[ivan4th]

CI fails with something failing in flexvolume code. Trying to debug ...

[miaoyq]

@ivan4th
Is the following code useful in integration test? I found it make the integration test faild.

virtlet/tests/integration/container_create_test.go

Lines 171 to 175 in ae5cdb4

	mounts := []*kubeapi.Mount{
	{
	HostPath: "/var/lib/virtlet",
	},
	}

If it is just a filler, that can be defined as follow:

 mounts := []*kubeapi.Mount{}

[ivan4th]

Reviewed 3 of 37 files at r2, 33 of 33 files at r4.
Reviewable status: 1 change requests, 0 of 2 approvals obtained (waiting on @miaoyq)

[ivan4th]

Thanks! I'm inclined to merge this PR as-is now, with further changes done as followups. The current e2e breakage is due to nginx docker image problems, there's PR pending that fixes it and I created a test tmp branch to make sure this PR passes e2e with these fixes applied (it does). So the question is, are you willing to contribute unit and e2e tests for this functionality (as followup PR(s)) or should we do it ourselves? If you want to work on it, I'll give some hints on how to do this in the manner consistent with other unit/e2e tests. Thanks again!

[miaoyq]

So the question is, are you willing to contribute unit and e2e tests for this functionality (as followup PR(s)) or should we do it ourselves? If you want to work on it, I'll give some hints on how to do this in the manner consistent with other unit/e2e tests.

@ivan4th OK, I will work on this later, :-)

[pigmej]

Merging because as Ivan said failing tests are not related to this PR, and they pass in tmp branch.

Reviewed 3 of 37 files at r2, 33 of 33 files at r4.
Reviewable status: 1 of 2 approvals obtained (waiting on @miaoyq)

[pigmej]

Reviewable status: 1 of 2 approvals obtained (waiting on @miaoyq)