Revendor Microsoft/go-winio for 8gB file fix when importing or committing image layers by TBBle · Pull Request #41430 · moby/moby

TBBle · 2020-09-09T13:14:43Z

- What I did

Revendored https://github.com/microsoft/go-winio to pull microsoft/go-winio#175, which replaces a bundled fork of archive/tar for Go 1.6 with use of vanilla archive/tar.

There's more context on this issue on microsoft/Windows-Containers#43.

Fixes: #40444

- How I did it

Implemented the test github.com/docker/docker/integration/build TestBuildWithHugeFile for Windows, to reproduce the issue.
Changed the revision in vendor.conf for github.com/Microsoft/go-winio to the master revision containing Replace archive/tar fork with use of archive/tar microsoft/go-winio#175 (Full diff: microsoft/go-winio@6c72808...5b44b70, thank you @cpuguy83 for the reminder)
Ran vndr -whitelist=^archive/tar github.com/Microsoft/go-winio (per the note at the end of vendor.conf)
Ran git checkout vendor/archive/tar to revert vndr's deletion of vendor/archive/tar.
Changed the one user of github.com/Microsoft/go-winio/archive/tar to use archive/tar instead.

- How to verify it

The existing unit test github.com/docker/docker/integration/build TestBuildWithHugeFile has been implemented for Windows Containers, and reproduces the issue.

For manual reproduction, see #40444 or the below procedure.

Given an 8gB file, e.g.,

$writer = [System.IO.File]::OpenWrite('8gfile')
$random = new-object Random
$blockSize = 1073741824
$bytes = new-object byte[] $blockSize
for ($i=0; $i -lt 8; $i++)
{
 $random.NextBytes($bytes)
 $writer.Write($bytes, 0, $blockSize)
}
$writer.Close()

And the following Dockerfile

FROM mcr.microsoft.com/windows/nanoserver:1909
COPY 8gfile 8gfile

docker build .

- Description for the changelog

Fix import or commit of Windows Container image layers containing files larger than 8gB by updating vendored go-winio library

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

TBBle · 2020-09-09T16:39:58Z

It should be right to go now, but the win-RS5 CI build has failed at the checkout stage, and I'm not seeing a way to reattempt it.

cpuguy83

Full diff microsoft/go-winio@6c72808...5b44b70

TBBle · 2020-09-09T17:02:00Z

I'm not clear how backporting candidacy is handled (I don't see a priority on #40444), but I think this could be a candidate for a 19.03 patch release, so we can resolve adamrehn/ue4-docker#44 sooner.

TBBle · 2020-09-09T17:38:13Z

Hmm. Test failed on Win-RS5.

[2020-09-09T17:32:42.803Z] === Failed
[2020-09-09T17:32:42.803Z] === FAIL: github.com/docker/docker/integration/build TestBuildWithHugeFile (600.28s)
[2020-09-09T17:32:42.803Z] panic: test timed out after 10m0s

Might need to increase the timeout, even a previous successful run took over 9 minutes for this test, since it's writing out an 8gB file inside the container. It's possible this could be made faster somehow, but I never worked out how during my repro efforts earlier this year.

TBBle · 2020-09-12T04:37:13Z

Okay, this is ready to go, again. ^_^

For some reason two of the integration-cli testsuites failed. They haven't failed before with these changes, and I noticed that they also seem a bit flaky on the master builds on other platforms, so I'm not sure if I can do anything about that.

Also, the CI pipeline appears to be stuck. One step has been stuck for almost two hours, despite hitting exit 1.

olljanat · 2020-09-19T12:18:31Z

@TBBle Windows CI have been on very bad shape on very long time here and I starting to be bit skeptic with these so now when #40599 is merged can I ask you to:

Force push version which only contains your updated test to here so we see that it really fails.
Wait that Windows CI: Enable more integration tests #38469 gets merged before push revendor commit back so we see that all of those tests still passes.

@marosset

This reproduces moby#40444, based on a suggestion from GitHub user @marosset Signed-off-by: Paul "TBBle" Hampson <[email protected]>

This pulls in the migration of go-winio/backuptar from the bundled fork of archive/tar from Go 1.6 to using Go's current archive/tar unmodified. This fixes the failure to import an OCI layer (tar stream) containing a file larger than 8gB. Fixes: moby#40444 Signed-off-by: Paul "TBBle" Hampson <[email protected]>

TBBle · 2020-09-19T15:12:38Z

A revert of the revendoring (ab8518ef4ba127a0344aeee3b792128b4bf79f3a), i.e. just the Windows enablement of TestBuildWithHugeFile demonstrates the test and pipeline failure on the Windows RS5 build.

TBBle · 2020-09-19T15:18:41Z

If gotest.tools/v3 supported an 'expected to fail' marker, then I'd suggest cherry-picking the test into #38469 with such a marker, so that this PR could just be a single commit that revendors go-winio and removes the 'expected fail' marker from that test.

But as far as I know, this testing framework supports no such marker.

olljanat · 2020-09-19T15:47:36Z

A revert of the revendoring (ab8518e), i.e. just the Windows enablement of TestBuildWithHugeFile demonstrates the test and pipeline failure on the Windows RS5 build.

Yes I see that it failed as expected and it happened already on 22 minutes when full CI takes ~60 minutes. Thanks.

To be honest, I'm not familiar with 'expected to fail' marker or frameworks which support it. However I didn't expect that you would update PR already today so maybe it is not needed to wait #38469

So LGTM (not maintainer).

@thaJeztah PTAL

TBBle · 2020-09-19T18:38:02Z

CI run failed in building e2e test image, I can't see how that'd be related to my changes...

[2020-09-19T15:44:49.076Z] #20 [frozen-images 2/2] RUN /download-frozen-image-v2.sh /build 	buildpack-d...

[2020-09-19T15:44:49.076Z] #20 2.309 Downloading 'library/buildpack-deps:buster@sha256:d0abb4b1e5c664828b93e8b6ac84d10bce45ee469999bef88304be04a2709491' (5 layers)...

[2020-09-19T15:44:49.076Z] #20 2.740 

[2020-09-19T15:44:49.076Z] #20 2.860 
                                                                           0.6%
##########                                                                15.1%
######################                                                    31.5%
#######################################                                   54.2%
##################################################                        69.9%
################################################################          89.4%
######################################################################## 100.0%

[2020-09-19T15:44:49.076Z] #20 4.120 

[2020-09-19T15:44:49.076Z] #20 4.237 
###################################                                       49.2%
######################################################################## 100.0%

[2020-09-19T15:44:49.076Z] #20 4.604 

[2020-09-19T15:44:49.076Z] #20 ERROR: executor failed running [/bin/sh -c /download-frozen-image-v2.sh /build 	buildpack-deps:buster@sha256:d0abb4b1e5c664828b93e8b6ac84d10bce45ee469999bef88304be04a2709491 	busybox:latest@sha256:95cf004f559831017cdf4628aaf1bb30133677be8702a8c5f2994629f637a209 	busybox:glibc@sha256:1f81263701cddf6402afe9f33fca0266d9fff379e59b1748f33d3072da71ee85 	debian:buster@sha256:46d659005ca1151087efa997f1039ae45a7bf7a2cbbe2d17d3dcbda632a3ee9a 	hello-world:latest@sha256:d58e752213a51785838f9eed2b7a498ffa1cb3aa7f946dda11af39286c3db9a9]: runc did not terminate sucessfully

[2020-09-19T15:44:49.076Z]

olljanat · 2020-09-19T18:52:39Z

@TBBle yep. Not related to this one , can be ignored.

thaJeztah

LGTM, thanks!

thaJeztah · 2020-09-23T12:56:37Z

 github.com/Azure/go-ansiterm                        d6e3b3328b783f23731bc4d058875b0371ff8109
 github.com/Microsoft/hcsshim                        9dcb42f100215f8d375b4a9265e5bba009217a85 # moby branch
-github.com/Microsoft/go-winio                       6c72808b55902eae4c5943626030429ff20f3b63 # v0.4.14
+github.com/Microsoft/go-winio                       5b44b70ab3ab4d291a7c1d28afe7b4afeced0ed4 # v0.4.15-0.20200908182639-5b44b70ab3ab


@kevpar @jterry75 is it possible to tag a new release with this change?

thaJeztah · 2020-09-24T18:16:23Z

Thanks!

TBBle requested a review from johnstep as a code owner September 9, 2020 13:14

TBBle marked this pull request as draft September 9, 2020 14:58

TBBle force-pushed the 40444-update-gowinio-for-8gB-file-fix branch 3 times, most recently from 55e5d75 to 29a5e80 Compare September 9, 2020 16:22

TBBle changed the title ~~Revendor Microsoft/go-winio for 8gB file fix when writing image layers~~ Revendor Microsoft/go-winio for 8gB file fix when importing or committing image layers Sep 9, 2020

TBBle marked this pull request as ready for review September 9, 2020 16:24

cpuguy83 approved these changes Sep 9, 2020

View reviewed changes

TBBle requested a review from tianon as a code owner September 9, 2020 17:54

marosset reviewed Sep 10, 2020

View reviewed changes

Comment thread integration/build/build_test.go Outdated

TBBle marked this pull request as draft September 10, 2020 09:00

TBBle mentioned this pull request Sep 10, 2020

diagnostics 8gig speed and doc improvement adamrehn/ue4-docker#97

Closed

TBBle force-pushed the 40444-update-gowinio-for-8gB-file-fix branch 3 times, most recently from d6c3997 to fd50269 Compare September 12, 2020 02:36

TBBle marked this pull request as ready for review September 12, 2020 02:36

TBBle requested a review from cpuguy83 September 15, 2020 01:28

TBBle mentioned this pull request Sep 17, 2020

go-winio fails to import layers with a file larger than 8gB microsoft/Windows-Containers#43

Closed

TBBle added 2 commits September 19, 2020 23:13

Enable TestBuildWithHugeFile on Windows

2dd9faa

This reproduces moby#40444, based on a suggestion from GitHub user @marosset Signed-off-by: Paul "TBBle" Hampson <[email protected]>

TBBle force-pushed the 40444-update-gowinio-for-8gB-file-fix branch 2 times, most recently from 395d687 to ab8518e Compare September 19, 2020 13:15

TBBle force-pushed the 40444-update-gowinio-for-8gB-file-fix branch from ab8518e to 35c531d Compare September 19, 2020 15:13

thaJeztah added platform/windows status/2-code-review labels Sep 23, 2020

thaJeztah approved these changes Sep 23, 2020

View reviewed changes

thaJeztah added status/4-merge and removed status/2-code-review labels Sep 23, 2020

thaJeztah added this to the 20.03.0 milestone Sep 23, 2020

tiborvass approved these changes Sep 24, 2020

View reviewed changes

tiborvass merged commit 29b149e into moby:master Sep 24, 2020

TBBle deleted the 40444-update-gowinio-for-8gB-file-fix branch September 25, 2020 07:02

TBBle mentioned this pull request Sep 25, 2020

ue4-docker build 4.22.2 fails with 'out of space' with new-enough Docker EE adamrehn/ue4-docker#44

Closed

TBBle mentioned this pull request Nov 5, 2020

docker commit > invalid tar header #33586

Closed

TBBle mentioned this pull request Mar 16, 2021

Docker Engine 20.10 for the DockerMsftProvider microsoft/Windows-Containers#99

Closed

Conversation

TBBle commented Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TBBle commented Sep 9, 2020

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

TBBle commented Sep 9, 2020

Uh oh!

TBBle commented Sep 9, 2020

Uh oh!

Uh oh!

TBBle commented Sep 12, 2020

Uh oh!

olljanat commented Sep 19, 2020

Uh oh!

TBBle commented Sep 19, 2020

Uh oh!

TBBle commented Sep 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olljanat commented Sep 19, 2020

Uh oh!

TBBle commented Sep 19, 2020

Uh oh!

olljanat commented Sep 19, 2020

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Sep 23, 2020

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Sep 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

TBBle commented Sep 9, 2020 •

edited

Loading

TBBle commented Sep 19, 2020 •

edited

Loading