llb: fileop implementation #809

tonistiigi · 2019-02-01T01:13:22Z

fixes #396 Add FileOp to LLB
fixes #781 WORKDIR issue
fixes #813 ADD --chown
fixes #786 ADD wildcard tar doesn't extract tarball to destination
fixes #735 ADDing files that may be missing
fixes #790 COPY --chown changes ownership of pre-existing directory
fixes #573 using wildcard does not use cache with multistage build

@AkihiroSuda @tiborvass @ijc

Signed-off-by: Tonis Tiigi [email protected]

solver/pb/ops.proto

ijc · 2019-02-04T15:28:02Z

client/llb/fileop_test.go

+func TestFileMkdir(t *testing.T) {
+	t.Parallel()
+
+	st := Image("foo").File(Mkdir("/foo", 0700))


.File doesn't seem like the right naming given it can actually do so much more. Having said that I'm not sure what would be better, FileOp perhaps, or even FsOp maybe?

Looking at a subsequent usage:

st := Image("foo").Dir("/etc").File(Mkdir("/foo", 0700).Mkdir("bar", 0600, WithParents(true)).Mkdir("bar/baz", 0701, WithParents(false)))

I'm left wondering if we coudn't arrange for this to be:

st := Image("foo").Dir("/etc").Mkdir("/foo", 0700).Mkdir("bar", 0600, WithParents(true)).Mkdir("bar/baz", 0701, WithParents(false)).Root()

i.e. dropping the .File() and instead chain at a higher level. That feels more in keeping with the existing llb client library stuff.

I suppose the desire for .File() is to chain multiple file ops into a single llb op?

With doing actions directly with the state we can't predict what actions run as part of a single op and what runs separately. As the caching is based on op, this isn't quite meaningless. We could add aliases of course state.Mkdir() -> state.File(Mkdir()) but I think it might be confusing and promote inefficient builds.

solver/pb/ops.proto

ijc · 2019-02-04T15:41:03Z

solver/pb/ops.proto

+message FileActionMkFile {
+	string path = 1;
+	int32 mode = 2;
+	bytes data = 3;


I think this is the first construct within the LLB which has the potential to create very large protobuf in one go in a possibly unconstrained way. Everything else can make huge graphs just through composition but is fairly limited by the complexity of the most complex graph you might sanely build.

That seems pretty unavoidable but has implications elsewhere (e.g. on maximum grpc size).

unavoidable...

Or maybe... could we use the session (along the lines of fssync) to somehow provide the data in bulk oob from the llb graph itself?

I think this can be done with docs or we can define limits. This is not an alternative for sources but should be only used for small one-off files. Eg. moby/moby#34423 is a good candidate.

solver/pb/ops.proto

ijc · 2019-02-04T15:46:39Z

solver/pb/ops.proto

+	int32 mode = 5;
+	bool followSymlink = 6;
+	bool dirCopyContents = 7;
+	bool attemptUnpack = 8;


what are the forward/backward compatibility implications here? Adding a new archive type that could be unpacked can't be done I think since it would change the semantics. s/attempt/must/g might be a simple way to avoid that?

This is actually a very bad feature and only here because of the dockerfile compatibility requirements. Normally, extraction is too high-level for LLB and should be done with an extra op. I think I'll rename it attemptUnpackDockerCompatibility to make it clear that new developments will not be accepted. If the behavior was mustUnpack then it would be very easy to prepend the input with exec and we wouldn't need it.

ijc · 2019-02-04T15:47:36Z

solver/pb/ops.proto

+	bool attemptUnpack = 8;
+	bool createDestPath = 9;
+	bool allowWildcard = 10;
+	bool allowEmptyWildcard = 11;


this is similar to bash's nullglob or something else?

filepath.Match

tonistiigi · 2019-02-11T23:20:22Z

Pushed initial solver implementation with unit tests. PTAL . A bit complex but wanted to make sure actions execute concurrently. Maybe it will become useful one day.

AkihiroSuda · 2019-03-02T06:38:50Z

@tonistiigi Do we want to merge this before v0.4?

AkihiroSuda · 2019-03-02T06:48:04Z

Doesn't it require adding new solver capability?

tonistiigi · 2019-03-02T07:40:00Z

@AkihiroSuda No, we can merge parts of it in v0.4 if needed but I don't expect to replace defaults for dockerfile for v0.4 . For cap I'll add one the same time. Atm it is considered in development(I don't want to add a new cap when a change is needed).

tonistiigi · 2019-03-11T06:44:55Z

This is complete now and ready for reviews. Dockerfile frontend has been switched to use new implementation by default and passes existing tests. Regression tests have been added for all the issues and matrix testing for non-fileop codepaths.

PR with fsutil changes that this PR vendors: tonistiigi/fsutil#60

PTAL @AkihiroSuda @tiborvass @ijc @thaJeztah

solver/pb/ops.proto

AkihiroSuda · 2019-03-11T06:54:38Z

util/testutil/integration/oci.go

@@ -60,7 +60,7 @@ func (s *oci) New(opt ...SandboxOpt) (Sandbox, func() error, error) {
 	}
 	logs := map[string]*bytes.Buffer{}
 	// Include use of --oci-worker-labels to trigger https://github.com/moby/buildkit/pull/603
-	buildkitdArgs := []string{"buildkitd", "--oci-worker=true", "--containerd-worker=false", "--oci-worker-labels=org.mobyproject.buildkit.worker.sandbox=true"}
+	buildkitdArgs := []string{"buildkitd", "--oci-worker=true", "--containerd-worker=false", "--oci-worker-gc=false", "--oci-worker-labels=org.mobyproject.buildkit.worker.sandbox=true"}


tonistiigi · 2019-03-11T07:00:24Z

frontend/dockerfile/dockerfile_test.go

-	dt, err = ioutil.ReadFile(filepath.Join(destDir, "subdest/dir1/dir2/foo"))
-	require.NoError(t, err)
-	require.Equal(t, "foo-contents", string(dt))
+	if isFileOp { // non-fileop implementation is historically buggy


This is a behavior change, but I verified that this is also the behavior in legacy docker build so the previous testcase was not correct.

frontend/dockerfile/dockerfile2llb/convert.go

solver/pb/ops.proto

thaJeztah · 2019-03-16T00:06:06Z

I saw you requested my review on this one; I did have a look, but, erm, I'd have to make myself more familiar with the BuildKit internals to give a fully meaningful review 😅 🤗 (I can go through the changes though if you want)

tonistiigi · 2019-03-16T00:13:00Z

@thaJeztah I pinged you cause it's fixing some of the issues you reported

Signed-off-by: Tonis Tiigi <[email protected]>

Signed-off-by: Tibor Vass <[email protected]>

Signed-off-by: Tonis Tiigi <[email protected]>

tiborvass

Tested with moby

tiborvass · 2019-03-16T02:12:10Z

Regarding ignorecache: I don't think we should make it configurable, rather fix the UX as Tonis said, to say "EXISTS" or "FOUND".

tonistiigi · 2019-03-16T04:17:42Z

There is no third state. Content was either created by exec or found in cache. Unless you want to just override all the status messages in cli but that seems pointless. It's a docs issue. If we don't want to change the docs then we just need to rerun all the time like the old behavior was (and is in this PR).

tonistiigi · 2019-03-18T01:02:42Z

Good to merge?

tonistiigi added the status/do-not-merge label Feb 1, 2019

tonistiigi mentioned this pull request Feb 1, 2019

Add FileOp to LLB #396

Closed

AkihiroSuda reviewed Feb 1, 2019

View reviewed changes

solver/pb/ops.proto Outdated Show resolved Hide resolved

ijc reviewed Feb 4, 2019

View reviewed changes

tonistiigi force-pushed the fileop branch 2 times, most recently from 0f4bb2c to b4848bb Compare February 11, 2019 23:18

tonistiigi force-pushed the fileop branch from b4848bb to cd68b54 Compare February 20, 2019 02:42

tonistiigi force-pushed the fileop branch from cd68b54 to ab9dffb Compare March 1, 2019 21:53

tonistiigi removed the status/do-not-merge label Mar 1, 2019

tonistiigi mentioned this pull request Mar 2, 2019

contenthash: add wildcard and nofollow support #850

Merged

AkihiroSuda added this to the v0.5.0 milestone Mar 2, 2019

tonistiigi force-pushed the fileop branch 7 times, most recently from 46a99b7 to 4187775 Compare March 11, 2019 06:11

tonistiigi mentioned this pull request Mar 11, 2019

copy: updates for buildkit fileop tonistiigi/fsutil#60

Merged

tonistiigi changed the title ~~llb: initial fileop implementation~~ llb: fileop implementation Mar 11, 2019

AkihiroSuda reviewed Mar 11, 2019

View reviewed changes

solver/pb/ops.proto Outdated Show resolved Hide resolved

AkihiroSuda reviewed Mar 11, 2019

View reviewed changes

tonistiigi commented Mar 11, 2019

View reviewed changes

AkihiroSuda reviewed Mar 11, 2019

View reviewed changes

frontend/dockerfile/dockerfile2llb/convert.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Mar 11, 2019

View reviewed changes

solver/pb/ops.proto Outdated Show resolved Hide resolved

tonistiigi and others added 16 commits March 15, 2019 17:49

vendor: add fsutil copy package

171feaa

Signed-off-by: Tonis Tiigi <[email protected]>

fileop: add chown support

7210bf6

Signed-off-by: Tonis Tiigi <[email protected]>

fileop: add dockerfile support

8a4674b

Signed-off-by: Tonis Tiigi <[email protected]>

fileop: updates with new fsutil copy pkg

0d17ac3

Signed-off-by: Tonis Tiigi <[email protected]>

vendor: revendor new packages

33955c9

Signed-off-by: Tonis Tiigi <[email protected]>

dockerfile: empty wildcard regression test

bb11c8c

Signed-off-by: Tonis Tiigi <[email protected]>

dockerfile: regression test for workdir creation

39ba2ed

Signed-off-by: Tonis Tiigi <[email protected]>

dockerfile: regression test for wildcard unpack

369f748

Signed-off-by: Tonis Tiigi <[email protected]>

dockerfile: regression test for existing dest dir perms

c4ef668

Signed-off-by: Tonis Tiigi <[email protected]>

dockerfile: add regression test for wildcard cache

c57e5b2

Signed-off-by: Tonis Tiigi <[email protected]>

dockerfile: make fileop default

637bec7

Signed-off-by: Tonis Tiigi <[email protected]>

dockerfile: add matrix testing for non-fileop versions

f38d971

Signed-off-by: Tonis Tiigi <[email protected]>

integration: disable gc

a16b47f

Signed-off-by: Tonis Tiigi <[email protected]>

fileop: review fixes

c6149da

Signed-off-by: Tonis Tiigi <[email protected]>

dockerfile: add more tests to fix fileop

6fd30e7

Signed-off-by: Tibor Vass <[email protected]>

dockerfile: improve symlinks copy tests

5effbff

Signed-off-by: Tonis Tiigi <[email protected]>

tonistiigi force-pushed the fileop branch from ca830e3 to 5effbff Compare March 16, 2019 00:50

tiborvass approved these changes Mar 16, 2019

View reviewed changes

AkihiroSuda merged commit a67ba78 into moby:master Mar 18, 2019

tonistiigi mentioned this pull request Mar 21, 2019

vendor: update buildkit to e5b647a1a moby/moby#38917

Merged

sj26 mentioned this pull request Jul 16, 2019

dockerfile: experimental ADD/COPY --chmod=... #1080

Closed

toolness mentioned this pull request May 18, 2020

Use Docker BuildKit to build images JustFixNYC/tenants2#1447

Closed

tonistiigi mentioned this pull request Sep 4, 2020

How to override copy-image from "docker build" CLI #1670

Closed

thaJeztah mentioned this pull request Nov 23, 2020

Is there a way to --strip-components with ADD #4136

Open

thaJeztah mentioned this pull request Mar 29, 2021

solve: remove duplicate filepath.Join #2048

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

llb: fileop implementation #809

llb: fileop implementation #809

tonistiigi commented Feb 1, 2019 •

edited

Loading

ijc Feb 4, 2019

ijc Feb 4, 2019

tonistiigi Feb 4, 2019

ijc Feb 4, 2019

ijc Feb 4, 2019

tonistiigi Feb 4, 2019

ijc Feb 4, 2019

tonistiigi Feb 4, 2019

ijc Feb 4, 2019

tonistiigi Feb 4, 2019

tonistiigi commented Feb 11, 2019

AkihiroSuda commented Mar 2, 2019

AkihiroSuda commented Mar 2, 2019

tonistiigi commented Mar 2, 2019

tonistiigi commented Mar 11, 2019

AkihiroSuda Mar 11, 2019

tonistiigi Mar 11, 2019

tonistiigi Mar 11, 2019

thaJeztah commented Mar 16, 2019

tonistiigi commented Mar 16, 2019

tiborvass left a comment

tiborvass commented Mar 16, 2019

tonistiigi commented Mar 16, 2019

tonistiigi commented Mar 18, 2019

llb: fileop implementation #809

llb: fileop implementation #809

Conversation

tonistiigi commented Feb 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonistiigi commented Feb 11, 2019

AkihiroSuda commented Mar 2, 2019

AkihiroSuda commented Mar 2, 2019

tonistiigi commented Mar 2, 2019

tonistiigi commented Mar 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Mar 16, 2019

tonistiigi commented Mar 16, 2019

tiborvass left a comment

Choose a reason for hiding this comment

tiborvass commented Mar 16, 2019

tonistiigi commented Mar 16, 2019

tonistiigi commented Mar 18, 2019

tonistiigi commented Feb 1, 2019 •

edited

Loading