Enable to use stargz snapshotter without spawning plugin process#1666
Conversation
tonistiigi
left a comment
tonistiigi
left a comment
There was a problem hiding this comment.
We should run integration tests with this configuration. If it is too much to do it in every PR we can configure it so it at least runs on master build (maybe also if stargz files change or stargz is mentioned in the commit message).
| @@ -422,3 +486,163 @@ gotest.tools/v3/internal/difflib | |||
| gotest.tools/v3/internal/format | |||
| gotest.tools/v3/internal/source | |||
| gotest.tools/v3/poll | |||
| # k8s.io/api v0.19.0 | |||
There was a problem hiding this comment.
This is too much. What needs it and how can we get rid of it?
There was a problem hiding this comment.
This is for CRI compatibility (secret-based registry auth: doc). I think we can get rid of this in buildkit. I'll work on this.
Signed-off-by: ktock <ktokunaga.mail@gmail.com>
ktock
force-pushed
the
sgz-oci
branch
2 times, most recently
from
1c4dd68 to
90ca005
Compare
| # musl is needed to directly use the registry binary that is built on alpine | ||
| ENV BUILDKIT_INTEGRATION_CONTAINERD_EXTRA="containerd-1.3=/opt/containerd-alt/bin" | ||
| ENV BUILDKIT_INTEGRATION_CONTAINERD_STARGZ=1 | ||
| ENV BUILDKIT_INTEGRATION_STARGZ_SNAPSHOTTER=1 |
There was a problem hiding this comment.
BUILDKIT_INTEGRATION_SNAPSHOTTER=stargz ?
There was a problem hiding this comment.
Should we support multiple snapshotters? (e.g. BUILDKIT_INTEGRATION_SNAPSHOTTER="stargz snapshotter-b snapshotter-c")
There was a problem hiding this comment.
Updated the test to make the snapshotter configurable through BUILDKIT_INTEGRATION_SNAPSHOTTER. For keeping the scope of this PR narrower, the test currently supports specifying single snapshotter through that envvar. But it shouldn't be difficult to extend this for multiple snapshotters if needed in the future.
Signed-off-by: ktock <ktokunaga.mail@gmail.com>
|
@tonistiigi |
|
@tonistiigi PTAL? |
|
@AkihiroSuda merge if ready |
| # github.com/google/go-cmp v0.4.0 | ||
| github.com/google/go-cmp/cmp | ||
| github.com/google/go-cmp/cmp/internal/diff | ||
| github.com/google/go-cmp/cmp/internal/flags | ||
| github.com/google/go-cmp/cmp/internal/function | ||
| github.com/google/go-cmp/cmp/internal/value | ||
| # github.com/google/go-containerregistry v0.0.0-20200425101607-48f605c3b60a |
There was a problem hiding this comment.
I assume from this dependency that stargz makes its own registry connections (i guess expected) that are not compatible with what buildkit does through containerd libraries and I guess missing all the extra config+token logic? Not ideal
There was a problem hiding this comment.
Thanks for the feedback. Yes, the snapshotter makes its own registry connections and the logic isn't using containerd lib. I agree with that the logic should have compatibility with buildkit.
There was a problem hiding this comment.
opened containerd/stargz-snapshotter#149 as a reminder
3 participants
Fixes: #1663
This commit embeds stargz snapshotter Go package to OCI worker.
In go.mod, this commit adds the following changes for making the build succeed:
replace()directive forgoogle.golang.org/genproto. This is becausego mod tidytries to update this to the version that is incompatible withgithub.com/golang/protobuf.github.com/golang/protobufis already specified inreplace()asv1.3.5corresponding to containerd. This commit fixed this by addinggoogle.golang.org/genprototoreplace()asv0.0.0-20200224152610-e50cd9704f63which also corresponds to containerd(v1.4.1-0.20200827124858-efa0e809135e).github.com/uber/jaeger-client-goto the latest (full diff: jaegertracing/jaeger-client-go@v2.11.2...v2.25.0). This is becausego mod tidyupdatesgithub.com/apache/thriftto the version that is incompatible with v2.11.2 (older version) ofgithub.com/uber/jaeger-client-go.cc @AkihiroSuda @tonistiigi