Always attempt the fallback on hosts configured with the referrers
capability. Fallback for any non-404 error.
Signed-off-by: Derek McGowan <derek@mcg.dev>
Allow the mount manager to skip handling of custom types. Ensure that
custom types are still working with formatted mounts.
Signed-off-by: Derek McGowan <derek@mcg.dev>
Allow task manager to fetch info on runtimes at startup.
Use this info to configure whether the runtime allows formatted mounts.
This info could also be used in the future to enforce policy such as
requiring a pre-known set of runtimes or specific runtime properties.
Signed-off-by: Derek McGowan <derek@mcg.dev>
The autoclear may take a bit of time to clear out the file, check
multiple times for the file to get removed before returning an error.
Signed-off-by: Derek McGowan <derek@mcg.dev>
Avoid keeping file descriptor open to directory which is getting
removed. Update error handling and wrapping to provide more clarity
around failures.
Signed-off-by: Derek McGowan <derek@mcg.dev>
This commit makes all of the recommended changes to use the `testing`
package helper functions instead of doing the equivalent longhand
versions of the same thing.
This change was needed in order to properly detect errors, as the code
would previously skip running `tenv` stating that it had been deprecated
in favor of `usetesting`.
Signed-off-by: Enji Cooper <yaneurabeya@gmail.com>
If a Dockerfile is using a `VOLUME` directive and the directory exists
in the rootfs, like in this example:
FROM docker.io/library/alpine:latest
VOLUME [ "/run" ]
The alpine container image already contains a "/run" directory. This
will force the code in WithVolumes() to copy its content to the new
volume created for the VOLUME directive. This copies the content as well
as the ownership.
However, as we perform the mounts from the host POV without being inside
a userns, the idmap option will just shift the IDs in ways that will
screw up the ownerships when copied. We should only use the idmap option
when running the container inside a userns, so the ownerships are fine
(the userns will do a shift and the idmap another, to make it all seem
as if there was no UID/GID shift in the first place).
This PR does just that, remove the idmap option from mounts so we copy
the files without any ID transformations. It's simpler and easier to
reason about if we just don't mount with the idmap option here: all
files are copied just fine without ID transformations and ID
transformation is applied via the idmap option at mount time when
running the pod.
Also, note that `VOLUME` directives that refer to directories that don't
exist on the rootfs work fine (`VOLUME [ "/rata" ]` for example), as
there is no copy done in that case so the permissions weren't changed.
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
When a lot of pods are created all at the same time, the umount fails
with EBUSY sometimes. This causes some mounts to be leaked. A way to
repro this issue is here:
https://github.com/containerd/containerd/issues/12139#issuecomment-3165305904
While using lsof/fuser it was not possible to get the culprits on time
(the mount is busy for a few ms), @fuwei has found what is causing the
mount to be busy:
https://github.com/containerd/containerd/issues/12139#issuecomment-3184544534
When we fork in `GetUsernsFD()`, if a call to prepareIDMappedOverlay()
is ongoing and has an open fd to the path to idmap, then the unmount
callback will fail as the forked process still has an open fd to it.
Let's handle the idmap unmounts in the same way other temp mounts are
done, using the Unmount() helper, that retries the unmount.
This retry fixes it 100% of the times on my system and in the systems
of @halaney that reported the issue too.
This was originally fixed by #10721 by using a detached mount, but the
mount was switched to non-detached again in #10955 and we started to
leak mounts. As @fuwei mentioned, using a detached mount is quite
invisible for the admin and, therefore, a retry is a better alternative.
It seems these umounts are a great candidate for #11303, which will
manage the life-cycle of mounts and can handle these retries whenever
needed.
[1]: To do it, I just run as root "unshare -m", that creates a mntns
with private propagation, and then run the containerd daemon.
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
Before this patch, the cleanup was unconditionally trying an unmount,
which is not needed if the mount never succeeded in the first place.
This causes logs of failed stuff, that should never be there.
Also, one function was creating the tmpDir and the nested one removing
it (in the cleanup function), which complicates the reasoning about
not leaking resources.
This patch on one hand moves the creation of the tmpDir into the
function that will remove it later and renames the parameter. On the
other hand, it also separates the cleanup function in two functions:
cleanDir() and cleanMount(). Each one will clean the directory or the
mount, only if needed, and a new cleanup function that just calls
cleanDir() and cleanMount() is returned as the cleanup function handler.
Because we now create the tmp directory inside the function, we need to
adjust the test: the directory passed as param now will exist, but it
shoild be empty. Therefore, we change the os.Stat() to an os.Remove().
If it's not empty (the cleanup didn't work), the remove will fail.
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
doPrepareIDMappedOverlay() can return nil as the cleanup function. As
the function mandates for the callback to be called, even when errors
are returned, we end-up calling the nil function that of course causes:
panic: runtime error: invalid memory address or nil pointer dereference
The containerd daemon continues to run fine, though.
With the current structure of always calling the cleanup function, we
would either need to check which error it is, to call it only on
specific errors (ugly) or return a "stub function" that doesn't do
anything on those cases (so the call works). None of these options seem
very nice.
Let's just switch the logic to a pattern more common on go: only need to
cleanup if it returned fine. This makes it easier for callers to not do
the wrong thing.
For this change we need to do the cleanup when returning errors on the
function itself, so the user doesn't need to do it for us.
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
