In addition to being wasteful, without this, concurrent calls to `Mount`
would have lost the previous `target` and so would never have been
removed.
Signed-off-by: Justin Chadwell <me@jedevc.com>
> WIP: still going on with investigation to determine
> exactly which process is accessing the file, will
> update on the issue #5807 thread for the records.
Fix the race condition with maximum 2 retries for now.
From several test runs, 1 retry seems to be
enough, even without backoff. Added a simple
linear backoff for each retry starting at 30 ms.
fixes#5807
Signed-off-by: Anthony Nandaa <profnandaa@gmail.com>
Convert usages of `github.com/docker/docker/pkg/idtools` to
`github.com/moby/sys/user` in order to break the dependency between
buildkit and docker.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
Trace blob is created 3 seconds after build completion.
If this happens after test has cleaned all history records
and before it checks for leaked blobs, test can report the
trace blob as leaked. In practice it would be cleaned up
next time containerd GC gets triggered.
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
appdefaults_unix.go, constants_unix.go:
/run is a thing only for Linux. Other Unixes use /var/run.
diffapply_linux.go and source_linux.go:
These files use Linux-only API.
Signed-off-by: Marat Radchenko <marat@slonopotamus.org>
Commit 4b85f11164 migrated the use of the
userns package to the github.com/moby/sys/user module.
After further discussion with maintainers, it was decided to move the
userns package to a separate module, as it has no direct relation with
"user" operations (other than having "user" in its name).
This patch migrates our code to use the new module.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
I noticed that we were importing the nydus errdefs package here, and
looking at [f044e0a946][1] (v0.12.0-rc1),
which introduced this import, this very likely was meant to be containerd's
errdefs package.
The only function consumed from the package is `errdefs.IsNotFound` which at
the time of the commit was not compatible with containerd's `errdefs.IsNotFound`
as it was [checking for the nydus error specifically][2].
Nydus-snapshotter v0.8.0 fixed this incompatibility by aliasing the error to
[containerd's `ErrNotFound`][3] and was updated through [483e87725e][4].
Ironically, the original commit [f044e0a946][1]
broke vendoring, because the Nydus errdefs was no longer vendored. This was
fixed in [75dd88efb8][4], but failed to notice
that the missing vendor was due to an incorrect import.
So it looks like things were broken _twice_ in the chain of events (once
because the wrong errdefs package did not match the expected error; once
because the errdefs package was missing), but all of them landed in v0.12.0-rc1,
so nothing broke in a release ':-)
This PR;
- fixes the wrong import
- adds a depguard rule to prevent accidental importing of this package
[1]: f044e0a946
[2]: f044e0a946/vendor/github.com/containerd/nydus-snapshotter/pkg/errdefs/errors.go (L22-L33)
[3]: f044e0a946/vendor/github.com/containerd/nydus-snapshotter/pkg/errdefs/errors.go (L22-L33)
[4]: 483e87725e
[5]: 75dd88efb8
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The userns package in libcontainer was integrated into the moby/sys/user
module at commit [3778ae603c706494fd1e2c2faf83b406e38d687d][1].
The userns package is used in many places, and currently either depends
on runc/libcontainer, or on containerd, both of which have a complex
dependency tree. This patch is part of a series of patches to unify the
implementations, and to migrate toward that implementation to simplify
the dependency tree.
[1]: 3778ae603c
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Update to containerd 1.7.18, which now migrated to the errdefs module. The
existing errdefs package is now an alias for the module, and should no longer
be used directly.
This patch:
- updates the containerd dependency: https://github.com/containerd/containerd/compare/v1.7.17...v1.7.18
- replaces uses of the old package in favor of the new module
- adds a linter check to prevent accidental re-introduction of the old package
- adds a linter check to enforce using an alias, to prevent accidental use
of the errdefs package in BuildKit or Moby.
- adds a linter check to prevent using the "log" package, which was also
migrated to a separate module.
There are still some uses of the old package in (indirect) dependencies,
which should go away over time.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
While submount paths were already validated there are some
cases where the parent mount may not be immutable while the
submount is created.
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
(cherry picked from commit 2529ec4121bcd8c35bcd96218083da175c2e5b77)
Running interactive container APIs was done by giving
the gateway implementation access to worker controller
directly, but it should be passed with a build job instead.
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
(cherry picked from commit 0971dffaab93d91e51af984b44c745b35b3c5b4d)
Whiteouts may share the same inode which cause the snapshotter
to attempt to create a hardlink between two whiteouts in the destination layer.
The problem is that the first whiteout is only created if it is part of the lower layer from the base snapshot.
Otherwise, the whiteout is not created in the destination layer because it is not needed.
If the first whiteout is not created in the destination, attempting to create a hardlink for the second whiteout will fail.
To avoid this issue, this fix disables hardlinks for whiteouts.
Signed-off-by: Grégoire Payen de La Garanderie <gregoire.payen.de.la.garanderie@intel.com>
Buildkit code is mostly generic enough to support FreeBSD, however
there are some quirks / infrastructural pieces that need to be
addressed for full support, to name some
- contenthash.NewFromStat attempts to set Devmajor / Devminor for
regular files, assuming that RDev is zero for regular
files. Unlike on Linux, it's not the case for FreeBSD.
- containerdexecutor.Run uses bind mounts for rootfs. Bind mounts
are not supported in FreeBSD and we should use nullfs instead
- There is no CI job to run tests on FreeBSD
- Some dependencies weren't ported
This change ports buildkit to FreeBSD
Signed-off-by: Artem Khramov <akhramov@pm.me>
Co-authored-by: Akihiro Suda <suda.kyoto@gmail.com>
Unless we're extracting an image to a layer, we should never return it
directly. We must always interact with it via a mount point, as Windows
layers hold a number of metadata files which should never be mutated
directly. When reading/manipulating the contents of a layer we should
always pass through a mount.
Allow the containerd mount.Mount() to properly mount the layer before we
interact with it.
Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
Migrate history objects to separate namespace to holding
reference to a blob does not interfer with the GC labels
held for same blobs by the containerd image store.
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Factorize code that check for an overlay mount type by using a function
instead.
This will allow supporting other overlay-based mount types more easily
in the future (for example fuse-overlayfs).
Signed-off-by: Alexis Murzeau <amubtdx@gmail.com>
Port https://github.com/moby/moby/blob/v23.0.1/daemon/oci_linux.go#L430-L460
> // Get the set of mount flags that are set on the mount that contains the given
> // path and are locked by CL_UNPRIVILEGED. This is necessary to ensure that
> // bind-mounting "with options" will not fail with user namespaces, due to
> // kernel restrictions that require user namespace mounts to preserve
> // CL_UNPRIVILEGED locked flags.
Fix issue 3098
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
Chown will result file capabilities getting reset, so it should be done
before setting xattrs to ensure they are retained.
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
This commit replaces `os.MkdirTemp` with `t.TempDir` in tests. The
directory created by `t.TempDir` is automatically removed when the test
and all its subtests complete.
Prior to this commit, temporary directory created using `os.MkdirTemp`
needs to be removed manually by calling `os.RemoveAll`, which is omitted
in some tests. The error handling boilerplate e.g.
defer func() {
if err := os.RemoveAll(dir); err != nil {
t.Fatal(err)
}
}
is also tedious, but `t.TempDir` handles this for us nicely.
Reference: https://pkg.go.dev/testing#T.TempDir
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
A temporary lease ctx was previously made during the snapshotter Merge
method but it was not passed to diffApply, which may have meant that
temporary View snapshots created there could be garbage collected by
containerd at any time.
It's possible this was encountered in CI, but I was unable to reproduce
the failure after 1000 attempts locally, so this commit is just a best
guess at the problem.
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
A previous commit fixed an issue where directories that overwrite
whiteout devices needed to be set to be opaque. While that was needed,
it wasn't enough because directories also need to be set opaque if they
are overwriting anything that isn't a directory, such as a file or a
symlink. This commit updates the code to handle that situation in
general.
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
The stargz snapshotter creates separate FUSE mounts for each lazy
snapshot and non-FUSE mounts for newly created local snapshots. This
means that hardlinking during diffApply fails. The code then falls back
to doing a copy, which works fine, but it also results in a ton of debug
logs about the failing hardlinks to be printed. This change just results
in those noisy log lines no longer appearing.
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Before this, there was a bug triggered under the following conditions:
1. An overlay snapshotter was being used, which caused the optimization
of preparing a new snapshot off of the base layers to be triggered
2. The base layers contained a directory that had contents
3. One subsequent layer deleted that directory w/out recreating it
4. A later layer recreated the directory
In this case, what happened was a whiteout device would be created as
part of 3 above but then in step 4 the whiteout device would be removed
and replaced with a plain directory. The problem is that such a
directory doesn't block out the files from step 2 and it doesn't know
about them because they are in a lowerdir (not the upperdir being
applied to).
The simplest fix, which this commit implements, is to just set the
directory created in step 4 as opaque, which enables the correct
behavior of blocking out files below it.
This was missed in test coverage before because tests for opaque
handling always combined 3+4 into one layer, whereas the bug requires
they be separate layers. A new integration test has been added to cover
this case.
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
This allows you to create refs that are single layers representing the
diff between any two arbitrary refs. The primary use case for this is
to allows users to extract the changes created by ops like Exec and
rebase them elsewhere through MergeOp. However, there is no restriction
on the inputs to DiffOp and the resulting ref's layer is simply the
layer created by running the differ on the two inputs refs
(specifically, the same differ used during exports).
A Diff ref can be mounted by itself, in which case it is defined as the
result of applying the diff to Scratch. Most use cases though will use
Diff refs as the input to a MergeOp, in which case the diff is just
applied on top of the lower merge inputs, as was the case before.
In cases like Diff(A, A->B->C) (i.e. cases where the diff is between two
refs where the lower is an ancestor of upper), the diff will be defined
as the layers separating the two refs. In other cases, the diff is just
a single layer, not re-used from the inputs, representing the diff
between the two refs (which can be defined as the layer "Diff(A,B)" that
satisfies "Merge(A, Diff(A,B)) == B").
Note that there is technically a meaningful difference between the
"unmerge" behavior of extracting the layers separating diffs and the
"simple diff" of just running the differ on the two refs. Namely, in the
case where there are "intermediate deletes" (i.e. deletes that only
exist in layers between A and B but not between A and B by themselves),
then the simple diff and unmerge can create different results when
plugged into a MergeOp. This is due to the fact that intermediate
deletes will apply to the merge when using the unmerge behavior, but not
when using the simple diff. This is on top of the fact that the simple
diff inherently has a "flattening" behavior where multiple layers are
squashed into a single one.
So, in the case where lower is an ancestor of upper, we choose to follow
the unmerge behavior, but it's possible users may prefer the simple diff
behavior. As of right now, they won't be able to do so, but if needed we
can add the ability to choose which behavior is followed in the future.
This could be done through a flag provided to DiffOp or possibly by
adapting llb.Copy to support this type of behavior with the same
efficiency as DiffOp.
Signed-off-by: Erik Sipsma <erik@sipsma.dev>
