Update the versions of golang to test with

[rhatdan]

We should be testing with the latest golang versions.
Change golang 8,9 to 10 and 11 for testing.

Signed-off-by: Daniel J Walsh [email protected]

[TomSweeneyRedHat]

LGTM assuming happy tests

[giuseppe]

LGTM

[giuseppe]

could you rebase this PR?

[rhatdan]

@nalind @mtrmac @vbatts Any idea why storage is blowing up with newer golang?

[vbatts]

there were changes in the tar archive format. I think go1.11 broke cache.
see https://github.com/openSUSE/umoci/issues/269 and moby/moby#37498

[vrothberg]

there were changes in the tar archive format. I think go1.11 broke cache.
see openSUSE/umoci#269 and moby/moby#37498

I had a similar thought but the CI here seems to break on go 1.10 as well.

[mtrmac]

*shrug* let’s ask the code… #238 adds logging for the reasons of the unexpected {/file-6 C} entries.

It seems to be the changes semi-documented in https://golang.org/doc/go1.10#archive/tar , in particular https://go.googlesource.com/go/+/48db2c01b42d959f2d8fa0c24d853bdb6100cf8a/src/archive/tar/writer.go#83 : ModTime is now rounded to full seconds by default. At least the TestChrootUntarPath failure can be traced to this AFAICS.

Surprisingly(?) c/storage seems to already be ready for this — see https://github.com/containers/storage/blob/master/pkg/archive/archive_110.go which effectively disables the rounding logic already — but only if TarOptions.CopyPass is set (i.e. if TarUntar/CopyWithTar, which is not the case for TestChrootUntarPath.

The CopyPass condition was added in #180 — and I can’t quite tell what the intent was; #180 only makes sense when tested against Go 1.10, but AFAICS TestChrootUntarPath should have failed against Go 1.10, and it has existed at that time. Has the failure been only exposed due to some unrelated change? Or have we simply never noticed because 1.10 was not in CI?

It seems easy enough to just plunk hdr.Format = tar.FormatPAX everywhere, but it’s not immediately obvious to me that this is sufficiently interoperable. #180 goes out of its way to not use FormatPAX everywhere…

@nalind can you remember more about the context of #180 ?

[mtrmac]

Ah, sameFsTime/sameFsTimeSpec is already supposed to account for loss of time precision.

In 1.9.5, and apparently in 1.10 WIP branches ( https://go.googlesource.com/go/+/ead6255ce3bd39e56d66f39471ab1854fd4f67f2%5E%21/ ), the time was truncated down; since actual 1.10 ( https://go.googlesource.com/go/+/577aab0c595825ba113b63f4ef1460e8471c803e%5E%21/ ), time is rounded to the whole second, and sameFsTime{,Spec} don’t account for that.

Let’s reuse #238 to check this hypothesis…

---

NOTE: At a first glance it seems, at the very best, very unclear that unpredictably rounding times down/up is safe WRT change detection (remember all the make failures and corner cases before nanosecond time stamps). Has anyone checked that this is not a problem?

• From a quick check, NaiveDiffDriver.Diff (and related methods), through addChanges, seems to use sameFsTimeSpec - which seems both unnecessary (both source and destination are on a local filesystem) and risky (~2E-9 chance of not noticing a change). Am I missing something?
• Now that tar+untar can forward ModTime up to 0.5 seconds into the future, can this mean that a real change on the filesystem can happen to set ModTime to the same value, and remain unnoticed?

(Sure, maybe we can’t actually manage to create + need to diff a layer in 0.5 seconds right now, but we certainly want to make it as fast as possible…)

---

Meanwhile, the higher versions of Go have also uncovered two trivial real bugs: #239 .

[mtrmac]

Let’s reuse #238 to check this hypothesis…

OK, #238 confirms that all the outstanding failures (i.e. apart from #239) are caused by the timestamp rounding.

DO NOT JUST MERGE THE WORKAROUND IN #238. That only shows where the failure manifests; as detailed above, AFAICS in at least some important code paths it seems that this relaxed check should not be used at all. (Maybe the relaxed check should only exists in tests, even?)

DO NOT JUST MERGE THE WORKAROUND IN #238. I can’t tell that it is safe. I wash my hands of it.

DO NOT JUST MERGE THE WORKAROUND IN #238. If you ended up merging something similar, only ever do so based on your own knowledge or analysis of how the code works and why.

[rhatdan]

I think we need @nalind to chime in.

[rhatdan]

Version updated in a different PR.