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.

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.

• 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.

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

sure.

You can delete this else to make CI happy.

sure.

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.

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

yes. thanks.

@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."

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

Yes, I will update the PR. Thanks.