agent: Add support for ephemeral volumes #236

harche · 2018-05-14T20:10:57Z

Ephemeral volumes needs to mounted inside VM
and not passed as 9pfs mounts.

Signed-off-by: Harshal Patil [email protected]

harche · 2018-05-14T20:11:28Z

Corresponding PR in runtime, kata-containers/runtime#307

egernst

Great idea - thanks for the PR, Harshal. Couple questions/comments.

@amshinde PTAL?

egernst · 2018-05-15T04:26:00Z

mount.go

-		absSource, err = filepath.EvalSymlinks(source)
-		if err != nil {
-			return grpcStatus.Errorf(codes.Internal, "Could not resolve symlink for source %v", source)
+		if fsType == typeTmpFs {


Since we have more than a couple cases now for the fsType now, I wonder if it'd warrant throwing this into a case now? WDYT?

Yes, switch case makes more sense here.

egernst · 2018-05-15T04:28:20Z

grpc.go

+		if len(splitSourceSlice) > 1 {
+			k8sStorageType := splitSourceSlice[len(splitSourceSlice)-2]
+			if k8sStorageType == "emphemeral" {
+				// Only mount tmpfs once on the host, and reuse the the mount destination


Just my own ignorance likely -- can you clarify "mount temps once on the host" ? The agent is running in the guest.

Maybe we shouldn't mount tmpfs in the host. And direct mount it in the guest vm

@egernst Sorry I will update that comment in the code to avoid the confusion. It should read, Only mount tmpfs once inside the VM, and reuse the the mount destination

Ephemeral Volumes live and die with the pod. They are created and mounted on the host by kublet and then binded mounted to every container in the pod that wants to use the volume. In case of kata, the pod is our VM Sandbox. So the volume has to be confined within the VM context. This tmpfs has to be created inside the VM, hence you see the code to create tmpfs in the agent. This way we increase the isolation provided by kata to the containers. The containers that want to share the data using ephemeral volumes won't have to leak their data to the host because the backing volume stays within VM memory.

@jshachm By the time control comes to kata, kublet has already provision tmpfs on the host. We can choose to ingore that and provision our tmpfs inside the guest as this patching achieving to do.

jodh-intel

Can you create some tests for this change (either unit tests here, or maybe raise an issue in https://github.com/kata-containers/tests for a different type of test)?

jodh-intel · 2018-05-15T07:08:11Z

grpc.go

+		splitSourceSlice := strings.Split(ociMnt.Source, "/")
+		if len(splitSourceSlice) > 1 {
+			k8sStorageType := splitSourceSlice[len(splitSourceSlice)-2]
+			if k8sStorageType == "emphemeral" {


Is this a reliable test? This appears to be looking for the word "emphemeral" in the ociMnt.Source path (and that "magic tag" might change at some point.

This block uses deep indentation. You could save one level by restructuring slightly:
if k8sStorageType != "emphemeral" { continue } ...

@jodh-intel Is it very reliable to check for k8sStorageType == "emphemeral" condition. The string emphemeral is introduced by this patch, https://github.com/kata-containers/runtime/pull/307/files#diff-8a56f5a1fa5971cc43e650ffa24f1e2bR376

That patch in runtime loops over the given OCI mounts to look for the mounts used by k8s for ephemeral storage. It then replaces those mount paths on the host with the mount paths in the guest by adding "/ephemeral" to the path. So it's very much in our control the existance of that string. Right now without this patch, an ephemeral volume is passed to guested via 9p.

e.g. A typical ephemeral volume path looks like, /var/lib/kubelet/pods/366c3a75-4869-11e8-b479-507b9ddd5ce4/volumes/kubernetes.io~empty-dir/cache-volume

But this resides on the host. We want that volume to be provisioned inside our VM. We can keep the same path or we can make it simpler since we have the total control on what happens inside the sandbox VM. So I replace that original source path with much simpler "/ephemeral". Inside the VM that above path becomes, /ephemeral/cache-volume

@jodh-intel I will add some unit test cases.

jodh-intel · 2018-05-15T07:08:37Z

grpc.go

+	for idx, ociMnt := range ociSpec.Mounts {
+		splitSourceSlice := strings.Split(ociMnt.Source, "/")
+		if len(splitSourceSlice) > 1 {
+			k8sStorageType := splitSourceSlice[len(splitSourceSlice)-2]


You could create a variable for len(splitSourceSlice) as you've scanned the string twice here.

bergwolf · 2018-05-16T06:35:36Z

Instead of checking k8sStorageType, we should add an ephemeral storage driver type and let ociSpec.Mounts just reference it. For example,

Let req.stroage have
Strorage{Driver:"ephemeral", fstype:"tmpfs", MountPoint: <sandbox-mountpoint-foo>}

And in ociSpec.Mounts, let ociSpec.Mounts[i].Mount have
Mount{Source:<sandbox-mountpoint-foo>, Destination:< ephemeral-volume-inside-of-container>, Type: "bind"}

The main change is in addStorages, we create the tmpfs mountpoint at <sandbox-mountpoint-foo>, with a new storage driver handler.

harche · 2018-05-16T08:36:05Z

@bergwolf Sounds good, I will give it a shot and update the PR.

sboeuf · 2018-05-17T22:51:07Z

@bergwolf I completely agree this should be the way to do it. We have designed Storage and Device for this purpose.
@harche please let us know when the code is ready :)

sboeuf · 2018-05-17T22:59:19Z

@harche just to clarify about ephemeral volumes. Do you expect anything to be present on the host at a location like /var/lib/kubelet/pods/366c3a75-4869-11e8-b479-507b9ddd5ce4/volumes/kubernetes.io~empty-dir/cache-volume ?

I ask because if this is basically an empty volume, then great, a simple volume backed by tmpfs on the guest will do the trick.
But if we expect some data to be there initially, then we would also have to share them, or copy them inside the guest as a first step.

Just want to make sure we're on the same page !

harche · 2018-05-18T11:37:32Z

@sboeuf There isn't any data initially. Ephemeral volumes are used only for sharing the data between the containers of the same pod. So the volume is empty to begin with unless a container from the respective pod writes something in it.

sboeuf · 2018-05-18T14:50:57Z

@harche okay thanks for clarification ;)

harche · 2018-05-22T04:26:37Z

@sboeuf @bergwolf @jodh-intel I have updated this PR as well as corresponding PR in runtime.

bergwolf · 2018-05-22T06:42:15Z

@harche Thanks for the change. It
LGTM!

caoruidong

LGTM. Once CI is green, it can be merged.

caoruidong · 2018-05-22T08:10:05Z

mount.go

+		_, err = commonStorageHandler(storage)
+		return "", err
+
+	} else {


You can delete this else to make CI happy.

codecov · 2018-05-22T08:25:23Z

Codecov Report

Merging #236 into master will increase coverage by 0.42%.
The diff coverage is 64.7%.

@@            Coverage Diff             @@
##           master     #236      +/-   ##
==========================================
+ Coverage   43.42%   43.84%   +0.42%     
==========================================
  Files          14       14              
  Lines        2091     2178      +87     
==========================================
+ Hits          908      955      +47     
- Misses       1070     1103      +33     
- Partials      113      120       +7

Impacted Files	Coverage Δ
device.go	`38.11% <ø> (ø)`	⬆️
mount.go	`64.65% <64.7%> (+2.03%)`	⬆️
network.go	`49.16% <0%> (+0.4%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7d934d...08674c0. Read the comment docs.

jodh-intel · 2018-05-22T14:03:46Z

Thanks @harche!

lgtm

sboeuf

One comment!
Looks good to me otherwise.

sboeuf · 2018-05-22T21:06:20Z

mount.go

+
+func ephemeralStorageHandler(storage pb.Storage, s *sandbox) (string, error) {
+	if _, err := os.Stat(storage.MountPoint); os.IsNotExist(err) {
+		os.MkdirAll(storage.MountPoint, os.ModePerm)


Please test the return of MkdirAll().

Good catch @sboeuf!

@harche You do not even need to check if the directory exists before calling MkdirAll. From the docs, "If path is already a directory, MkdirAll does nothing and returns nil."

@amshinde Well I think you need to check this because in case it does exist, you don't need to call into commonStorageHandler().

and why are we not calling then commonStorageHandler() then? It may happen that the directory exists, but we skip mounting altogether.

@amshinde the tmpfs mount point has to be mounted on the guest only once. Then rest of the containers will bind mount that directory.

e.g. If all the containers need to have an ephemeral volume in the pod they will bind mount, say /sharedMount directory on the VM. But before that this /sharedMount has to be mounted on the VM backed by tmpfs (but only once)

The above code is tested with a k8s yaml with 3 containers sharing the mount point.

apiVersion: v1 kind: Pod metadata: name: myapp-pod annotations: io.kubernetes.cri.untrusted-workload: "true" labels: app: myapp spec: containers: # Application Container - name: myapp-container volumeMounts: - mountPath: /cache name: cache-volume imagePullPolicy: Never image: app_image:latest command: ['sh', '-c', 'cat /cache/test && sleep 3600'] initContainers: # Copy Data container - name: data-container volumeMounts: - mountPath: /cache name: cache-volume imagePullPolicy: Never image: data_image:latest command: ['sh', '-c', 'cp /test /cache/'] # Append Data container - name: init-append-data volumeMounts: - mountPath: /cache name: cache-volume imagePullPolicy: Never image: test_image:latest command: ['sh', '-c', 'echo test >> /cache/test'] volumes: - name: cache-volume emptyDir: medium: "Memory" ``

@amshinde It's is intentionally skipped mounting if dir exists because then every single container end up pointing towards a seperate tmpfs volume and they don't end up sharing the volume. So only once (for the first time) you have to make sure the dir exists and then mount it on guest backed by tmpfs. The corresponding enteries in all container's OCI spec already point towards bind mounting that dir.

@harche Thanks for the explanation, my question was based on the assumption that you would be adding storage just once for the first container.

jodh-intel · 2018-05-23T15:42:38Z

Adding dnm label until the mkdir check is added.

amshinde · 2018-05-23T18:16:57Z

mount.go

-	if fsType != type9pFs {
-		var err error
-
+	var err error


nit: you dont need to declare err here, since you use a new instance in the inner scope.

yes. thanks.

amshinde · 2018-05-23T18:20:44Z

mount.go

+
+func ephemeralStorageHandler(storage pb.Storage, s *sandbox) (string, error) {
+	if _, err := os.Stat(storage.MountPoint); os.IsNotExist(err) {
+		os.MkdirAll(storage.MountPoint, os.ModePerm)


@harche You do not even need to check if the directory exists before calling MkdirAll. From the docs, "If path is already a directory, MkdirAll does nothing and returns nil."

amshinde · 2018-05-23T23:10:23Z

mount.go

+	var err error
+	switch fsType {
+	case typeTmpFs, type9pFs:
+		if err := createDestinationDir(destination); err != nil {


@harche Are you not already creating the mount destination directory in the "ephemeral" storage handler?

Yes, I will update the PR. Thanks.

amshinde

lgtm

amshinde · 2018-05-25T21:15:18Z

@harche CI is failing,can you take a look so that this can be merged.

sboeuf · 2018-05-25T22:00:42Z

@harche please fix the unit tests.

Ephemeral volumes needs to mounted inside VM and not passed as 9pfs mounts. Fixes : kata-containers#235 Signed-off-by: Harshal Patil <[email protected]>

harche · 2018-05-28T09:46:27Z

@sboeuf @amshinde done.

harche mentioned this pull request May 14, 2018

virtcontainers: Add support for ephemeral volumes kata-containers/runtime#307

Closed

harche force-pushed the tmpfs branch from 6a5e64d to 4d335ac Compare May 14, 2018 20:14

egernst reviewed May 15, 2018

View reviewed changes

jodh-intel reviewed May 15, 2018

View reviewed changes

harche force-pushed the tmpfs branch from 4d335ac to 3558bb2 Compare May 20, 2018 16:24

caoruidong approved these changes May 22, 2018

View reviewed changes

harche force-pushed the tmpfs branch from 3558bb2 to b0456e5 Compare May 22, 2018 08:25

sboeuf reviewed May 22, 2018

View reviewed changes

jodh-intel added the do-not-merge label May 23, 2018

amshinde reviewed May 23, 2018

View reviewed changes

harche force-pushed the tmpfs branch from b0456e5 to b6b646b Compare May 24, 2018 09:48

caoruidong removed the do-not-merge label May 24, 2018

amshinde approved these changes May 25, 2018

View reviewed changes

agent: Add support for ephemeral volumes

08674c0

Ephemeral volumes needs to mounted inside VM and not passed as 9pfs mounts. Fixes : kata-containers#235 Signed-off-by: Harshal Patil <[email protected]>

harche force-pushed the tmpfs branch from b6b646b to 08674c0 Compare May 28, 2018 08:13

sboeuf approved these changes May 29, 2018

View reviewed changes

sboeuf merged commit f6db83c into kata-containers:master May 29, 2018

bergwolf mentioned this pull request Jun 4, 2018

shm: Create shared /dev/shm kata-containers/runtime#357

Merged

jodh-intel mentioned this pull request Jun 22, 2018

Version the gRPC protocol #272

Closed

agent: Add support for ephemeral volumes #236

agent: Add support for ephemeral volumes #236

Conversation

harche commented May 14, 2018

harche commented May 14, 2018

egernst 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

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

bergwolf commented May 16, 2018 • edited Loading

harche commented May 16, 2018

sboeuf commented May 17, 2018

sboeuf commented May 17, 2018

harche commented May 18, 2018

sboeuf commented May 18, 2018

harche commented May 22, 2018

bergwolf commented May 22, 2018 • edited by amshinde Loading

caoruidong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 22, 2018 • edited Loading

Codecov Report

jodh-intel commented May 22, 2018 • edited by amshinde Loading

sboeuf 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

harche May 25, 2018 • edited Loading

Choose a reason for hiding this comment

harche May 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodh-intel commented May 23, 2018

Choose a reason for hiding this comment

harche May 24, 2018 • edited Loading

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

amshinde left a comment

Choose a reason for hiding this comment

amshinde commented May 25, 2018

sboeuf commented May 25, 2018

harche commented May 28, 2018

bergwolf commented May 16, 2018 •

edited

Loading

bergwolf commented May 22, 2018 •

edited by amshinde

Loading

codecov bot commented May 22, 2018 •

edited

Loading

jodh-intel commented May 22, 2018 •

edited by amshinde

Loading

harche May 25, 2018 •

edited

Loading

harche May 25, 2018 •

edited

Loading

harche May 24, 2018 •

edited

Loading