pkg/tarsum: fix unit test for Go 1.11+

[kolyshkin]

Since go-1.11beta1 archive/tar, tar headers with Typeflag == TypeRegA (numeric value 0, which is the default unless explicitly initialized) are modified to have Typeflag set to either tar.TypeReg (character value '0', not numeric 0) or tar.TypeDir (character value '5') [1]. This results in different Typeflag value in the resulting header, leading to a different Checksum, and causing the following test case errors:

12:09:14 --- FAIL: TestTarSums (0.05s)
12:09:14 tarsum_test.go:393: expecting
[tarsum+sha256:8bf12d7e67c51ee2e8306cba569398b1b9f419969521a12ffb9d8875e8836738],
but got
[tarsum+sha256:75258b2c5dcd9adfe24ce71eeca5fc5019c7e669912f15703ede92b1a60cb11f]
... (etc.)

All the other code explicitly sets the Typeflag field, but this test case does not, causing the incompatibility with Go 1.11. Therefore the fix is to set TypeReg value explicitly, and change the expected checksums
in test cases.

Alternatively, we can vendor archive/tar again (for the 100th time), but given that the issue is limited to the particular test case it does not make sense.

This fixes the test for all Go versions.

[1] https://go-review.googlesource.com/c/go/+/85656

[kolyshkin]

This one was part of #37358

[kolyshkin]

ppc CI failure is #37408

[kolyshkin]

Related discussion: #37358 (review)

[thaJeztah]

This is the change that makes Go 1.10 switch to the same behavior as Go 1.11? With this change, do we still need the vendored archive package (on Go 1.10)?

[kolyshkin]

This is the change that makes Go 1.10 switch to the same behavior as Go 1.11?

Yes. It just fixes the test case by explicitly specifying the file type (rather than using an obsolete default value).

With this change, do we still need the vendored archive package (on Go 1.10)?

Yes we do (it is there to fix a different problem about static build).

[thaJeztah]

Moving this back to "design review", so that we can complete the discussion on this change from a behavioural perspective 😅

[kolyshkin]

Let me reiterate it here. All the tarballs generated by docker have Typeflag set explicitly to proper values. It is this very test case that did not do it, which resulted in different checksums with go-1.11beta1 because its archive/tar started to set typeflag for the unset case. So, this issue is limited to this very test case (no build cache breakage etc as far as I can see)

[thaJeztah]

It is this very test case that did not do it, which resulted in different checksums with go-1.11beta1 because its archive/tar started to set typeflag for the unset case

I should learn how to read 😅 I see you referred to that in the linked issue, but we were also discussing the "cache breaking"

[thaJeztah]

SGTM

ping @tonistiigi @dmcgowan

[kolyshkin]

we were also discussing the "cache breaking"

I have briefly checked it, and it looks like

• every other piece of code in moby/moby do set the Typeflag explicitly,
• tarballs on dockerhub do have Typeflag set,

so there should be no cache breakage.

[dmcgowan]

every other piece of code in moby/moby do set the Typeflag explicitly,

Was this changed in 1.10? As in, did we already have a cache breakage

tarballs on dockerhub do have Typeflag set,

This is not related to build cache breakage

I don't think build cache breaking should be a blocker, I was just bringing it up so we are aware if there is breakage. After such a breakage we get users coming back complaining about a performance regression in build time and resource usage, when in reality it is just their builds being run without the cache hit they had with the previous version.

Regardless, this change LGTM

[kolyshkin]

every other piece of code in moby/moby do set the Typeflag explicitly,

Was this changed in 1.10? As in, did we already have a cache breakage

The change I'm talking about (essentially if h.Typeflag == 0 { h.Typeflag = '0' } occurred in archive/tar as of Go 1.11beta1 (see https://go-review.googlesource.com/c/go/+/85656).

This is not related to build cache breakage

I stand corrected. Could you explain what kind of changes in archive/tar can cause build cache breakage (or just link to an example from the past)?

[tonistiigi]

If all tars created by CLI for docker build already have Typeflag='0' then we should be ok and there is no build cache breakage. Could double check but that should be the case.

LGTM

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@7f91801). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #37498   +/-   ##
=========================================
  Coverage          ?   34.95%           
=========================================
  Files             ?      610           
  Lines             ?    44886           
  Branches          ?        0           
=========================================
  Hits              ?    15690           
  Misses            ?    27076           
  Partials          ?     2120