Add IncludePatterns and ExcludePatterns options for Copy #2082

aaronlehmann · 2021-04-22T01:27:05Z

Allow include and exclude patterns to be specified for the "copy" op,
similarly to "local".

tonistiigi · 2021-04-22T06:20:08Z

What about cache? Looks to me that atm cache would still be invalidated even if files that match the pattern didn't change.

aaronlehmann · 2021-04-22T16:16:52Z

What about cache? Looks to me that atm cache would still be invalidated even if files that match the pattern didn't change.

Good point. I see two issues with cache right now.

I'm not sure this change in its current state is invalidating cache if the include pattern or exclude pattern changes. Do I need to add something to (*fileOp).CacheMap to ensure that happens?
It sounds like you're saying we should have an optimization to avoid invalidating cache if there was a change to the source that doesn't actually afffect the end result after considering the include/exclude filters. I'm not sure how to approach this. Is there any existing infrastructure that can decide whether to invalidate the cache by applying these filters and seeing if anything changed? If not, where should it be added?

tonistiigi · 2021-04-22T16:21:41Z

Cache checksum is covered by the result of this func https://github.com/moby/buildkit/blob/master/solver/llbsolver/ops/file.go#L150 . I don't think the current contenthash.Checksum and contenthash.ChecksumWildcard are enough to cover the pattern cases so likely another function like this needs to be added to the contenthash pkg.

tonistiigi · 2021-04-22T16:23:38Z

Also, add a capability for this https://github.com/moby/buildkit/blob/master/solver/pb/caps.go and check in the client https://github.com/moby/buildkit/blob/master/client/llb/fileop.go#L660

aaronlehmann · 2021-04-22T18:10:17Z

Cache checksum is covered by the result of this func https://github.com/moby/buildkit/blob/master/solver/llbsolver/ops/file.go#L150 . I don't think the current contenthash.Checksum and contenthash.ChecksumWildcard are enough to cover the pattern cases so likely another function like this needs to be added to the contenthash pkg.

Here's my rough understanding so far of what needs to be done. I wanted to run it by you before I start doing the work:

Add IncludePatterns and ExcludePatterns to llbsolver.Selector so that NewContentHashFunc can pass them into contenthash.Checksum and contenthash.ChecksumWithWildcard.
Rewrite addSelector and dedupeSelectors not to use llbsolver.Selector as a map key, because that won't be possible once llbsolver.Selector has slice fields.
The tree walks in cache/contenthash/checksum.go need to match the include/exclude pattern when considering which files/dirs to include in the checksum. I think this should be done in (*cacheContext).wildcards and (*cacheContext).checksum (maybe it isn't necessary in wildcards, since it can be handled one level deeper in checksum?). It's better to do this at checksum time than at scan time (scanPath), so we can avoid extra FS walks, right?

tonistiigi · 2021-04-22T18:47:30Z

Rewrite addSelector and dedupeSelectors not to use llbsolver.Selector as a map key, because that won't be possible once llbsolver.Selector has slice fields.

dedupeSelectors can also detect if one path uses filter and another doesn't then filtered one does not matter

It's better to do this at checksum time than at scan time (scanPath), so we can avoid extra FS walks, right?

I think wildcards() is similar to what you want to achieve. So either new variant of that function or modification. Btw, are wildcards and filters allowed to be used together? For the scanning, I don't think we need to be super optimized in here initially. Iiuc then currently for wildcards it also just scans everything

buildkit/cache/contenthash/checksum.go

Lines 464 to 472 in 8e71ac4

    
           scan, err := cc.needsScan(root, "") 
        
           if err != nil { 
        
           	return nil, err 
        
           } 
        
           if scan { 
        
           	if err := cc.scanPath(ctx, m, ""); err != nil { 
        
           		return nil, err 
        
           	} 
        
           }

aaronlehmann · 2021-04-22T18:58:33Z

Btw, are wildcards and filters allowed to be used together?

Yes, they can be used together. I think modifying wildcards and using it when there are include or exclude filters might be a good option. Basically I think we'd replace the path.Match check with a function that checks path.Match if wildcards are allowed, and also checks include/exclude filters. For simplicity, we'd probably skip the

		if cr.Type == CacheRecordTypeDir {
			iter = root.Seek(append(k, 0, 0xff))
		}

if there are any exclude filters.

For the scanning, I don't think we need to be super optimized in here initially. Iiuc then currently for wildcards it also just scans everything

What are the conditions where needsScan returns true?

Are you saying it's reasonable to always scan the FS when there are include or exclude patterns? That would potentially be simpler, because it could reuse fsutils.Walker instead of having yet another implementation of include/exclude logic.

aaronlehmann · 2021-04-22T23:44:18Z

@tonistiigi: I've turned wildcards into something more generic that handles the include/exclude patterns as well. PTAL.

tonistiigi · 2021-04-26T22:57:03Z

What are the conditions where needsScan returns true?

When files are first checked from a snapshot that was not created by transferring local source. After that, specific path is scanned but other paths around it on same snapshot may be not.

Are you saying it's reasonable to always scan the FS when there are include or exclude patterns?

Yes

CI does not seem to be running because of vendoring issues.

Did you cover the case where parent paths of the included patters need to contain the checksum for the metadata of the dir itself but not its contents? Is pattern digest the checksum of the top path that matched pattern or all files (eg subdirs inside it) matched individually and merged later with some logic?

aaronlehmann · 2021-04-26T23:44:12Z

Did you cover the case where parent paths of the included patters need to contain the checksum for the metadata of the dir itself but not its contents?

Are you talking about the situation where we copy / with IncludePatterns = /foo/bar, and the metadata on /foo changes, so we need to invalidate the cache because we may need to copy the directory entry for /foo and want to preserve the metadata in its current state?

If so, I don't think I handled this. It should be an easy fix to include that parent directory metadata in the cache key.

Is pattern digest the checksum of the top path that matched pattern or all files (eg subdirs inside it) matched individually and merged later with some logic?

I think it's closer to the latter. Take a look at the logic in includedPaths. It iterates over the files/dirs in the prefix tree, and collects digests for files and dirs which match the include pattern. If a dir matches the include pattern and there are no exclusions, it uses the dir's content checksum directly and skips files within it. Checksum combines the digests returned by includedPaths to form the final checksum.

aaronlehmann · 2021-04-26T23:49:26Z

CI does not seem to be running because of vendoring issues.

Looks like a missing git add on the vendor dir - fixed.

aaronlehmann · 2021-04-27T01:17:46Z

Did you cover the case where parent paths of the included patters need to contain the checksum for the metadata of the dir itself but not its contents?

Are you talking about the situation where we copy / with IncludePatterns = /foo/bar, and the metadata on /foo changes, so we need to invalidate the cache because we may need to copy the directory entry for /foo and want to preserve the metadata in its current state?

If so, I don't think I handled this. It should be an easy fix to include that parent directory metadata in the cache key.

Added this (see latest commit)

hinshun · 2021-04-29T18:40:57Z

client/client_test.go

+
+	for _, name := range []string{"myfile", "sub/bar"} {
+		_, err = os.Stat(filepath.Join(destDir, name))
+		require.Equal(t, true, errors.Is(err, os.ErrNotExist))


I think there's a native function for this:
https://github.com/stretchr/testify/blob/6990a05d54c2287be8787a411d77815643347339/require/require.go#L313-L315

aaronlehmann · 2021-05-04T22:00:30Z

@tonistiigi: No rush, but this is ready for another pass.

thaJeztah · 2021-05-10T11:23:17Z

Could this be used to implement moby/moby#37333 / moby/moby#15771 ?

aaronlehmann · 2021-05-10T15:05:41Z

Could this be used to implement moby/moby#37333 / moby/moby#15771 ?

Yes, I believe it could.

tonistiigi

Is it true that with this I can do copy with excluding all go files with **/*.go exclude pattern but there is no way I can do copy only all go files?

tonistiigi · 2021-05-13T01:33:25Z

cache/contenthash/checksum.go


 	txn := cc.tree.Txn()
 	root = txn.Root()
 	var updated bool

+	lastIncludedDir := ""
 	iter := root.Seek([]byte{})


!wildcard case should seek to p?

tonistiigi · 2021-05-13T01:37:31Z

cache/contenthash/checksum.go

+			}
+		}
+		if !matched {
+			return false, partial, nil


false, false, nil

tonistiigi · 2021-05-13T01:46:33Z

cache/contenthash/checksum.go

+				dirSlash := fn + "/"
+				for _, pat := range excludePatternMatcher.Patterns() {
+					patStr := pat.String() + "/"
+					if strings.HasPrefix(patStr, dirSlash) {


Isn't this pattern regexp. How can prefix be checked. I think at least missing Exclusion check but can't find docs for that as well.

As curently implemented this is best-effort, and if we have a pattern here rather than a prefix we might end up including some paths in the checksum that would be excluded by the filters. This would never cause false cache hits but might lead to false negatives.

The alternative would be to never use a dir's content hash when there is at least one exclude pattern, and instead walk all included dirs and only incorporate non-excluded files in the final checksum. Basically this code would be replaced with:

if cr.Type == CacheRecordTypeDir { lastIncludedDir = fn if excludePatternMatcher != nil { // don't use the optimization where we add a whole dir's contents // to the checksum when there is an exclude pattern, because some // files in the dir may be excluded continue treeWalk } }

tonistiigi · 2021-05-13T01:48:39Z

cache/contenthash/checksum.go

 	for {
 		k, _, ok := iter.Next()
 		if !ok {
 			break
 		}
+		fn := string(convertKeyToPath(k))
+		dirHeader := false
 		if len(k) > 0 && k[len(k)-1] == byte(0) {


I don't understand where the non-null-byte dir is excluded from checksum for the partial case.

The continue a few lines below does this. It causes us to skip the call to checksum when partialMatch is true, but the path does not end in a null byte.

tonistiigi · 2021-05-13T01:51:29Z

cache/contenthash/checksum_test.go

+	cc, err := newCacheContext(ref.Metadata(), nil)
+	require.NoError(t, err)
+
+	dgst, err := cc.Checksum(context.TODO(), ref, "foo", ChecksumOpts{IncludePatterns: []string{"foo"}}, nil)


Is this a valid case? I thought only dirs can have subpatterns.

What do you think the behavior should be in this case? Should we exclude /foo because the specified path is not a dir? Return an error?

Same as copying a directory where IncludePatterns are set but none match.

If I understand correctly, you think it should copy no files in this scenario?

I don't think that matches the behavior implemented in fsutil. (*copier).copy only checks IncludePatterns on recursive calls, so calling Copy with /foo would copy /foo regardless of IncludePatterns. We would need a PR to fsutils to keep the behavior in sync.

So dir with no matching subdirs will make an empty dir? Thanks seems ok.

Not sure about the file case. What do you think is the best? It is always wrong to use it like this and a sign that the user did something wrong. Do you see cases where it might seem wrong for the user if they get an error for this case?

It seems reasonable to me to make it an error to try and do a Copy op with IncludePatterns or ExcludePatterns when the source is a file. If we do that, we don't need any special handling at the checksum level, since it wouldn't be a valid scenario.

I could add a Stat in docopy if you think that makes sense. Or we could add this check at the fsutil level.

tonistiigi · 2021-05-13T01:51:47Z

cache/contenthash/checksum_test.go

+	require.Equal(t, dgstFileData0, dgst)
+
+	dgstFoo, err := cc.Checksum(context.TODO(), ref, "", ChecksumOpts{IncludePatterns: []string{"foo"}}, nil)
+	require.NoError(t, err)


no dgstFileData0 check.

I don't think this will match dgstFileData0 because the record for / is included in the checksum (in case other files are added in the future which match the pattern).

If source ends with / (same for /) but then the dir checksum should not apply, correct? As the dir itself is not copied.

tonistiigi · 2021-05-13T01:55:53Z

cache/contenthash/checksum_test.go

+
+	dgstD0Star2, err := cc.Checksum(context.TODO(), ref, "", ChecksumOpts{IncludePatterns: []string{"d0/*"}}, nil)
+	require.NoError(t, err)
+	require.NotEqual(t, dgstD0Star, dgstD0Star2)


do/abc should be equal to dgstD0

not sure if this covers the non-nil-byte case described above or a case with intermediate dir is needed.

When we have IncludePatterns = []string{"d0"}, we include the content hash of the dir d0 in the final checksum.

When we have IncludePatterns = []string{"d0/*"}, we only include the metadata has for dir d0 in the final checksum (plus the files or dirs inside d0 which match the pattern).

So I believe these currently end up different, but we could make them match by removing the optimization that includes the "non-null byte" digest in the final checksum when a dir matches an include pattern exactly.

d0 vs d0/* differences should be ok. But we should check the case where path and includepatterns are the same, but one adds an extra file under path that doesn't match includepatters. That should still always give the same checksum.

tonistiigi · 2021-05-13T02:00:13Z

client/client_test.go

+		fstest.CreateFile("sub/foo", []byte("foo0"), 0600),
+		fstest.CreateFile("sub/bar", []byte("bar0"), 0600),
+	)
+	require.NoError(t, err)


should add some checks for cache stability. Showing that adding files not matching copy does not invalidate the cache.

I like the idea, but how can this client test check whether the cache was invalidated or not?

This can be done with Exec that adds a file with random contents and then check that after a repeated run the extra file's contents either changes or does not change.

tonistiigi · 2021-05-13T02:01:12Z

solver/pb/caps.go

-	CapFileRmWildcard apicaps.CapID = "file.rm.wildcard"
+	CapFileBase                apicaps.CapID = "file.base"
+	CapFileRmWildcard          apicaps.CapID = "file.rm.wildcard"
+	CapFileCopyIncludePatterns apicaps.CapID = "file.copy.includepatterns"


Not very important but could be one cap as added together.

aaronlehmann · 2021-05-13T14:27:38Z

Is it true that with this I can do copy with excluding all go files with **/*.go exclude pattern but there is no way I can do copy only all go files?

I don't think it supports doublestar - for that we need tonistiigi/fsutil#79

tonistiigi · 2021-05-13T22:42:07Z

I don't think it supports doublestar - for that we need

ExcludePatterns does support ** though afaics https://github.com/moby/moby/blob/master/pkg/fileutils/fileutils.go#L170 . Hence the weird inconsitency.

aaronlehmann · 2021-05-13T22:43:49Z

I don't think it supports doublestar - for that we need

ExcludePatterns does support ** though afaics https://github.com/moby/moby/blob/master/pkg/fileutils/fileutils.go#L170 . Hence the weird inconsitency.

I think you're right then. Long term, we should add ** support for include patterns as well, which would resolve the inconsistency.

aaronlehmann · 2021-05-19T15:31:17Z

@tonistiigi: Do you know why the vendor check is failing? Also, is there anything you think still needs to be addressed here?

aaronlehmann · 2021-05-24T23:49:31Z

cache/contenthash/checksum_test.go

 	require.NoError(t, err)
-	require.Equal(t, digest.FromBytes(append([]byte("d0"), []byte(dgstDirD0)...)), dgst)
+	require.Equal(t, dgstDirD0FileByFile, dgst)


This checksum changed because Checksum used to return the content hash for the dir that matched the pattern, but we now go file-by-file in case any files are matched by exclude patterns (or exclusions from an include pattern).

We could reintroduce the old behavior, conditional on not having any include/exclude patterns, if you think that makes sense.

Allow include and exclude patterns to be specified for the "copy" op, similarly to "local". Depends on tonistiigi/fsutil#101 Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

Consider IncludePatterns and ExcludePattern when calculating content hashes. Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

…rator Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

… ExcludePatterns Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

aaronlehmann · 2021-06-01T15:59:56Z

@tonistiigi @hinshun: Any feedback on the latest version of this PR?

hinshun

Happy to see logic converging for IncludePaths and ExcludePaths. This PR looks good to me. cc @tonistiigi

tonistiigi

As a follow-up, would be nice if we could have a page of docs that describes the algorithm for checksums based on file content, with examples for the different cases (file, dir, wildcard, includepath).

jasondamour · 2021-09-23T23:58:33Z

@aaronlehmann I just found this MR, it is the exact functionality I needed. Thank you!

Is this available in a stable version of docker? I'm not too familiar with how moby and docker-ce relate and release.

aaronlehmann · 2021-09-24T00:02:35Z

What I added here is low-level buildkit functionality, that I'm using in a buildkit-based project. I don't think it's available through Docker, unfortunately.

SInnget · 2021-09-27T21:11:28Z

@aaronlehmann Just found this MR too. Thank you!

I check the release note that claimed this pr is included, also the 1.3-labs came after the 0.9.0. I am curious whether whether this functionality already available in current buildkit, say, 1.3-labs?

Since I failed to find document on how to copy with exclude in dockerfile. Thank you in advance!

aaronlehmann force-pushed the copy-include-exclude branch from f293670 to 5ea2a92 Compare April 22, 2021 23:42

aaronlehmann force-pushed the copy-include-exclude branch from 5ea2a92 to cf80609 Compare April 26, 2021 23:49

aaronlehmann force-pushed the copy-include-exclude branch 2 times, most recently from 8947fc4 to bb9c8eb Compare April 26, 2021 23:57

aaronlehmann force-pushed the copy-include-exclude branch 2 times, most recently from e118757 to 57ddd68 Compare April 28, 2021 22:40

aaronlehmann mentioned this pull request Apr 28, 2021

Add includePatterns and excludePatterns support for copy openllb/hlb#239

Merged

hinshun reviewed Apr 29, 2021

View reviewed changes

aaronlehmann marked this pull request as ready for review May 3, 2021 15:47

tonistiigi reviewed May 13, 2021

View reviewed changes

tonistiigi requested a review from hinshun May 13, 2021 02:04

aaronlehmann commented May 24, 2021

View reviewed changes

aaronlehmann force-pushed the copy-include-exclude branch 2 times, most recently from 5ccb4c7 to 1d4a783 Compare May 25, 2021 15:02

aaronlehmann added 15 commits May 26, 2021 13:48

Add IncludePatterns and ExcludePatterns options for Copy

7660af2

Allow include and exclude patterns to be specified for the "copy" op, similarly to "local". Depends on tonistiigi/fsutil#101 Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

Add capabilities for copy include/exclude patterns

b545c28

Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

Cache awareness of IncludePatterns/ExcludePatterns

626744c

Consider IncludePatterns and ExcludePattern when calculating content hashes. Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

Include parent directories of include patterns in cache checksum

7a35420

Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

Update vendored fsutil fork

32e52f1

Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

Use require.ErrorIs

8e6fc83

Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

Revendor master version of fsutil

743bc1b

Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

Seek to path if !wildcard

b94fc7f

Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

return false, false, nil

607dc1f

Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

Combine include/exclude caps into a single cap

b27fc61

Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

Exclude source dir metadata from checksum if source path ends in sepa…

fc20beb

…rator Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

Add test case for added file which doesn't match include pattern

5f90a18

Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

Add test that non-included file does not invalidate cache

cb536b2

Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

Revendor updated fsutils fork; use same logic for IncludePatterns and…

7c13af3

… ExcludePatterns Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

Back to master version of fsutil

6f5ea71

Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

aaronlehmann force-pushed the copy-include-exclude branch from 1d4a783 to 6f5ea71 Compare May 26, 2021 20:48

hinshun approved these changes Jun 1, 2021

View reviewed changes

tonistiigi approved these changes Jun 1, 2021

View reviewed changes

tonistiigi merged commit 8df5671 into moby:master Jun 1, 2021

aaronlehmann deleted the copy-include-exclude branch September 24, 2021 00:01

srstsavage mentioned this pull request Nov 22, 2021

COPY with excluded files is not possible moby/moby#15771

Open

thaJeztah mentioned this pull request Aug 10, 2023

add an additional --exclude option to COPY instruction for the files or folders to be excluded while copying them #2853

Open

tonistiigi mentioned this pull request Jan 8, 2024

Feature request: On Dockerfiles, allow filepaths to be excluded from ADD and COPY commands #4439

Open

Add IncludePatterns and ExcludePatterns options for Copy #2082

Add IncludePatterns and ExcludePatterns options for Copy #2082

Conversation

aaronlehmann commented Apr 22, 2021

tonistiigi commented Apr 22, 2021

aaronlehmann commented Apr 22, 2021

tonistiigi commented Apr 22, 2021

tonistiigi commented Apr 22, 2021

aaronlehmann commented Apr 22, 2021

tonistiigi commented Apr 22, 2021

aaronlehmann commented Apr 22, 2021

aaronlehmann commented Apr 22, 2021

tonistiigi commented Apr 26, 2021

aaronlehmann commented Apr 26, 2021

aaronlehmann commented Apr 26, 2021

aaronlehmann commented Apr 27, 2021

Choose a reason for hiding this comment

aaronlehmann commented May 4, 2021

thaJeztah commented May 10, 2021

aaronlehmann commented May 10, 2021

tonistiigi left a 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

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

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

aaronlehmann commented May 13, 2021

tonistiigi commented May 13, 2021

aaronlehmann commented May 13, 2021

aaronlehmann commented May 19, 2021

Choose a reason for hiding this comment

aaronlehmann commented Jun 1, 2021

hinshun left a comment

Choose a reason for hiding this comment

tonistiigi left a comment

Choose a reason for hiding this comment

jasondamour commented Sep 23, 2021

aaronlehmann commented Sep 24, 2021

SInnget commented Sep 27, 2021