Support volume annotations #40
Conversation
Random-Liu
force-pushed
the
support-volume-annotation
branch
from
a9e5c30 to
8462ffa
Compare
pkg/v1/utils/volumes.go
Outdated
| // volumeName gets volume name from volume annotation key, example: | ||
| // gvisor.dev/spec/mount/NAME/share | ||
| func volumeName(k string) string { | ||
| return strings.Split(strings.TrimPrefix(k, volumeKeyPrefix), "/")[0] |
pkg/v1/utils/volumes.go
Outdated
| } | ||
| } | ||
| } | ||
| done[volume] = struct{}{} |
There was a problem hiding this comment.
Why do you need to worry about duplicate volume annotations?
There was a problem hiding this comment.
Initially I didn't only handle the type key, so we need to skip other annotations once one is handled. But now we only handle the type key now, but still need to track whether the spec is updated or not, so I left done there.
I could change it to a single boolean if you prefer. :)
There was a problem hiding this comment.
Changed to an updated boolean.
pkg/v1/utils/volumes.go
Outdated
| // volumePath searches the volume path in the kubelet pod directory. | ||
| func volumePath(volume, uid string) (string, error) { | ||
| // TODO: Support subpath when gvisor supports pod volume bind mount. | ||
| volumeSearchPath := fmt.Sprintf("/var/lib/kubelet/pods/%s/volumes/*/%s", uid, volume) |
There was a problem hiding this comment.
You could make this a global variable or preferably a flag. Then you can change it to /tmp/... in the test.
There was a problem hiding this comment.
SG. I don't think we want to expose a flag for this, but a global variable seems better.
| } else { | ||
| // This is a container | ||
| for i := range s.Mounts { | ||
| if yes, _ := isVolumePath(volume, s.Mounts[i].Source); yes { |
There was a problem hiding this comment.
It's safer to match s.Mounts[i].Source, with the current annotation's source. Otherwise, it may upgrade to tmpfs a mount which doesn't have annotations associated with it (e.g. a mount with mismatching mount options).
There was a problem hiding this comment.
We don't have the source in shim for container, because there is no way to get pod UID right now.
There was a problem hiding this comment.
Wouldn't this incorrectly mark an EmptyDir mount with mismatching rw/ro options as gVisor internal tmpfs, instead of a shared volume mounted externally? The former would cause a distinct tmpfs to be mounted inside each containers.
There was a problem hiding this comment.
That's why I return an error for sandbox if an tmpfs annotation exists, but source is not successfully applied. That guaranteed that as long there is an tmpfs annotation, source should be applied on sandbox.
And the volume name is unique for each pod, so this should work. But of course, this is not the best way this should work, we need to find a way to pass down more information to get rid of this hack.
There was a problem hiding this comment.
Sounds good. Please leave a comment in the code explaining it and maybe a TODO to fix it later.
|
@fvoznika Addressed comments. Please take another look. |
Signed-off-by: Lantao Liu <[email protected]>
Signed-off-by: Lantao Liu <[email protected]>
Random-Liu
force-pushed
the
support-volume-annotation
branch
from
28eb582 to
2baa1ca
Compare
This is to support volume annotations introduced in google/gvisor#308.