stage1: implement docker volume semantics #2315
Conversation
130433e to
a181792
Compare
73959c8 to
8e999c0
Compare
8e999c0 to
04a0ea0
Compare
|
review request |
| @@ -167,6 +168,11 @@ func CopyTree(src, dest string, uidRange *uid.UidRange) error { | |||
| return nil | |||
| } | |||
|
|
|||
| fis, err := ioutil.ReadDir(dest) | |||
There was a problem hiding this comment.
Do we really want this change? If we call CopyTree to copy some tree into a directory with some files there, then they will be deleted. Maybe we should rather have a separate function that deletes the old directory and then calls CopyTree?
There was a problem hiding this comment.
AFAICT, I only remove it if it's empty
There was a problem hiding this comment.
I guess I could call syscall.Rmdir
There was a problem hiding this comment.
Meh, I misread the if condition. And you can probably call os.Remove.
There was a problem hiding this comment.
Right, I just read the documentation and it should remove empty dirs.
There was a problem hiding this comment.
But anyway, what's the point of the change?
There was a problem hiding this comment.
If the destination is an empty directory, CopyTree fails and I want to be able to call it whether the destination exists (and it's empty) or not.
There was a problem hiding this comment.
That sounds pretty arbitrary. Does CopyTree work, when destination exists and is not empty? I don't know what to expect from this function - it is not documented.
There was a problem hiding this comment.
Nope, it will try to Mkdir the target directory, which already exists, and fail. That's why I'm removing it if it exists and it's empty.
And yes, we must document the function.
There was a problem hiding this comment.
Alright, then I think it is the caller of CopyTree who should be responsible for removing the directory, even if it is empty. It might be surprising later that suddenly a directory has different permissions/mode/whatever and that happened because CopyTree just removed it and then recreated with different permissions.
Docker volumes are initialized with the files in the image if they exist, unless a host directory is mounted there (see https://docs.docker.com/engine/userguide/containers/dockervolumes/#data-volumes). However, when we convert docker images with docker2aci, we convert docker volumes to mount points, and if the user doesn't specify a host path for mounting, we mount an empty volume, effectively masking the files in the image. To fix this, we create a special case for the case when application uses an image converted from docker and we mount an implicit empty volume. In that case, we copy the files from the image to the volume, so docker images work correctly. Note that we won't do this if the user explicitly specifies an empty volume or if they mount a host directory.
To test Docker volume semantics, we simulate the inspect aci is a Docker-converted image. The other tests don't care about this.
04a0ea0 to
4ebff8a
Compare
|
Updated |
|
LFAD. |
|
Awesome! Thank you guys, we can test some previous failing images now @sjpotter |
| // GenerateMounts maps MountPoint paths to volumes, returning a list of Mounts. | ||
| func GenerateMounts(ra *schema.RuntimeApp, volumes map[types.ACName]types.Volume) []schema.Mount { | ||
| func convertedFromDocker(ra *schema.RuntimeApp) bool { | ||
| ann := ra.Annotations |
There was a problem hiding this comment.
@iaguis Bad news, not working for rktnetes still, as it's using run --pod-manifest... When running from the run --pod-manifest, we don't really have the annotations in the runtime app. So probably we should check the image's annotation instead?
There was a problem hiding this comment.
Right, checking the image's annotation sounds better, since it is a property of the image.
Docker volumes are initialized with the files in the image if they
exist, unless a host directory is mounted there (see
https://docs.docker.com/engine/userguide/containers/dockervolumes/#data-volumes).
However, when we convert docker images with docker2aci, we convert
docker volumes to mount points, and if the user doesn't specify a host
path for mounting, we mount an empty volume, effectively masking the
files in the image.
To fix this, we create a special case for the case when application uses
an image converted from docker and we mount an implicit empty volume.
In that case, we copy the files from the image to the volume, so docker
images work correctly.
Note that we won't do this if the user explicitly specifies an empty
volume or if they mount a host directory.
TODO:
What if the user runs a pod with several docker images with the same mount point, then the files that end up in the volume are not defined. Maybe we should make implicit empty volumes unique.Prepended the app name which is unique.Tests