pkg/archive.FileInfoHeader: fill file type bits

[AkihiroSuda]

• relates to archive/tar: mode header field contains incorrect bits golang/go#20150

Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp

- What I did

Fix Go 1.9 test failure #33892 (comment)

- How I did it

Go 1.9 (golang/go@66b5a2f) removed file type bits from
archive/tar.FileInfoHeader().

This commit ensures file type bits are filled even on Go 1.9 for
compatibility.

- How to verify it

Execute remotecontext tests on both Go 1.8 and Go 1.9beta2.

$ go test -c ./builder/remotecontext && sudo ./remotecontext.test -test.v
$ go1.9beta2 test -c ./builder/remotecontext && sudo ./remotecontext.test -test.v

go1.9beta2 can be installed by running go get golang.org/x/build/version/go1.9beta2.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

[vieux]

LGTM

[tonistiigi]

nit: can we make it even more obvious that this is a stdlib workaround by calling it fillGo18FileTypeBits

[tonistiigi]

Design LGTM

[AkihiroSuda]

done @tonistiigi

[tiborvass]

@AkihiroSuda @tonistiigi why were the tests failing on the 1.9 bump PR and why does this PR fix it?
IOW: I thought that Go1.9 would simply bust the cache from one docker release to another. But this clearly fixes something.

[tonistiigi]

@tiborvass Build cache hashes are computed from tar headers. go1.9 changes tar.FileInfoHeader so it returns a different header and therefore a different hash. This PR modifies our own archive.FileInfoHeader function (that calls tar.FileInfoHeader internally) to revert the go1.9 changes.

[thaJeztah]

@tonistiigi what I was wondering is if this only affects the build cache, or also the content-addressable store. If only the build-cache, I'm wondering if a one-time cache-bust would be acceptable (preventing us to having to keep carrying this patch)

[tiborvass]

@tonistiigi @AkihiroSuda but why don't we let the cache be busted? If that's what we decide to do, all we need is to update the expected hashes in tarsum_test.go in the go1.9 PR.

[thaJeztah]

Should this override be in a separate file, fenced by a build flag? (// go1.8)?

[tiborvass]

If it were to have a tag it would have to have go1.9 tag, but even then it's not needed since in go1.8 these bits are already set, so it would just set it again (it's OR'd)

[thaJeztah]

Ugh, meant to type 1.9 😅

[tonistiigi]

@tiborvass because we care for our users :)

I'm with @AkihiroSuda for avoiding the cache bust. Cache bust will probably be unavoidable with https://github.com/moby/buildkit so let's not make it a recurring theme. There are real issues with the cache hash(some listed in moby/buildkit#38) that we haven't fixed because we don't want to break it. Let's do it only one time.

[tiborvass]

I'm with you on trying our best to make it as seldom as possible. I didn't know about coming cache busts, so +1 for this PR. Let's not forget to remove the fix function once we are ready.

LGTM

[stevvooe]

@AkihiroSuda Was this reported upstream or are we just forever adding exceptions? Was this change even correct in Go 1.9?

[tonistiigi]

@stevvooe This is not an upstream issue, we are just keeping the compatibility of a method in our own library. The point of archive.FileInfoHeader() is to list all docker specific exceptions(name, special device handling etc). Like commented above, only use case for these mode bits is the current build cache implementation, so this is one of the things that can be removed when moving builder to a more modern architecture.

[Larsjep]

Sorry I didn't notice this before, but it is very unfortunate that these bits has been added for compatibility reasons.
The change in go-lang was actually made to solve this problem docker/docker-py#998.
I understand the arguments about not wanting to break the customers cache, but in this case I think it's needed, because the old cache is build on incorrect tar headers.
The problem with keeping these bits is that it makes it difficult for 3rd party clients (like docker-py) to create a build context that is compatible with the context used by "docker build", because standard tar libraries (like python/tar) doesn't allow setting these bits.

[tiborvass]

@Larsjep It is not exactly about not wanting to break users' cache: it is about limiting as much as possible the number of times we do it since it is very disruptive. Since we know we are going to break the cache again when we start depending more on buildkit, and that the issue you're referring to was always present, we think it is reasonable to not address the issue right away, and prefer to break the cache just once in the foreseeable future instead of twice. But the issue will be solved.

[Larsjep]

@tiborvass Sounds reasonable. We must just remember to remove this patch at that point.
Do you know when it's planned that buildkit should be integrated ?

[edmorley]

Do we need to open an issue to track reverting this / is there an issue open already for the mentioned buildkit changes? It's just docker/compose#5090 is blocked on these changes :-)

[thaJeztah]

ping @AkihiroSuda @tonistiigi ^^

[edmorley]

Do we need to open an issue to track reverting this / is there an issue open already for the mentioned buildkit changes? It's just docker/compose#5090 is blocked on these changes :-)

@tiborvass, @AkihiroSuda @tonistiigi - I don't suppose you saw the above? :-)

[tonistiigi]

@edmorley BuildKit already doesn't use tar headers for content hash so is not affected by this(and doesn't have this patch anywhere)

[edmorley]

@tonistiigi - ah to clarify the question - the change we want to make will break existing caches so needs to be coordinated with other such cache-breaking changes, such as the buildkit related work (see #33935 (comment)). My question was asking if an issue was already filed for that work, so we can ensure the tar file related changes happen at the same time. (It would be incredibly frustrating to find the tar file changes were forgotten and we had to wait yet again for the next cache-breaking event... :-))

[tonistiigi]

@edmorley When docker build is integrated with BuildKit the current code that does the cache checksum calculation or context extraction is removed and everything will be handled by BuildKit so there is no other change that needs to be made. There is no issue that I know of to revert this specific patch from archive package, but the builder will not be calling these archive package functions anymore.

[edmorley]

When docker build is integrated with BuildKit the current code that does the cache checksum calculation or context extraction is removed and everything will be handled by BuildKit so there is no other change that needs to be made.

Is there an issue/PR we can track where this is occurring? I'm just keen to keep momentum on this, so we can get the 3 years old docker/compose#883 closed out :-)

[AkihiroSuda]

containerd content store needs to be integrated first

cc @dmcgowan Do we have a github issue?

[dmcgowan]

@AkihiroSuda Not yet, expect something soon though

[tmo-trustpilot]

Any news on reverting the added bits? The long running issue with docker-compose and docker build sharing build caches (best explained here docker/compose#883 (comment)) is still a problem three years on...

[AkihiroSuda]

I think docker-compose should use BuildKit filesync API instead