Use absolute path for rootfs in OCI config.json #22256
Conversation
| @@ -274,18 +267,6 @@ func (clnt *client) setExited(containerID string) error { | |||
| ExitCode: exitCode, | |||
| }}) | |||
|
|
|||
| // Unmount and delete the bundle folder | |||
There was a problem hiding this comment.
You are missing the removal of unmount in 'clean()`. Maybe we would need to keep this unmount code here in some form though in case there was a ungraceful shutdown between the restarts. This method is only called on restore and could handle that case.
There was a problem hiding this comment.
Thanks! Reverted it somehow before pushing, fixed.
mlaventure
force-pushed
the
use-abs-rootfs-path
branch
from
8e4d960 to
68d0aa1
Compare
This avoid an extra bind mount within /var/run/docker/libcontainerd This should resolve situations where a container having the host /var/run bound prevents other containers from being cleanly removed (e.g. moby#21969). Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>
mlaventure
force-pushed
the
use-abs-rootfs-path
branch
from
68d0aa1 to
3135874
Compare
|
after discussion with @tonistiigi I put back the umount for the old rootfs bind mount to account for installations that will get upgraded without going through a clean daemon shutdown 😢 |
|
LGTM |
tonistiigi
added
status/2-code-review
process/cherry-pick
and removed
status/0-triage
labels
|
LGTM, but this means we aren't compliant with the OCI spec unless the spec gets updated. |
|
|
|
@mlaventure can you open up a PR in OCI for this? I think I've seen some other people ask for the path even be outside of the bundle dir so allowing for it to be absolute would align with what those folks want. |
|
@duglin Far more concerned with fixing a real bug than adhering 100% to a pre-1.0 spec. |
|
@mlaventure sorry - I missed that there's already an issue for it, perhaps you could submit a PR to close that issue? Should be easy. |
|
@cpuguy83 the problem is that docker advertises that other OCI compliant runtimes can be plugged in - if we put an absolute path in there instead of a relative one then some other runc variant might not work. |
|
@mlaventure thanks! |
|
Removed my objection. thanks @mlaventure for the quick turn around. |
"TestRestartContainerwithRestartPolicy" contains some codes that could be flaky, it's supposed to be fixed in moby#22256. This commit removes unnecessary code, make the test case cleaner. Signed-off-by: Zhang Wei <zhangwei555@huawei.com>
This avoid an extra bind mount within
/var/run/docker/libcontainerdThis should resolve situations where a container having the host
/var/runbound prevents other containers from being cleanly removed(e.g. #21969).
Signed-off-by: Kenfe-Mickael Laventure mickael.laventure@gmail.com
This is technically making docker nonOCIcompliant, but then againruncallows it currently :).There's an open issue for fixing the spec at opencontainers/runtime-spec/issues/389 with an associated PR for fixing the spec at opencontainers/runtime-spec/pull/394