Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revendor Microsoft/go-winio for 8gB file fix when importing or committing image layers #41430

Merged
merged 2 commits into from
Sep 24, 2020

Conversation

TBBle
Copy link
Contributor

@TBBle TBBle commented Sep 9, 2020

- 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.

  1. 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()
  1. And the following Dockerfile
FROM mcr.microsoft.com/windows/nanoserver:1909
COPY 8gfile 8gfile
  1. 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)

A platypus being photogenic

@TBBle TBBle requested a review from johnstep as a code owner September 9, 2020 13:14
@TBBle TBBle marked this pull request as draft September 9, 2020 14:58
@TBBle 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 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 TBBle marked this pull request as ready for review September 9, 2020 16:24
@TBBle
Copy link
Contributor Author

TBBle commented Sep 9, 2020

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.

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TBBle
Copy link
Contributor Author

TBBle commented Sep 9, 2020

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
Copy link
Contributor Author

TBBle commented Sep 9, 2020

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 TBBle requested a review from tianon as a code owner September 9, 2020 17:54
@TBBle TBBle marked this pull request as draft September 10, 2020 09:00
@TBBle 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 TBBle marked this pull request as ready for review September 12, 2020 02:36
@TBBle
Copy link
Contributor Author

TBBle commented Sep 12, 2020

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
Copy link
Contributor

@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:

  1. Force push version which only contains your updated test to here so we see that it really fails.
  2. 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.

This reproduces moby#40444, based on a suggestion from GitHub user @marosset

Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>
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 <Paul.Hampson@Pobox.com>
@TBBle 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
Copy link
Contributor Author

TBBle commented Sep 19, 2020

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 TBBle force-pushed the 40444-update-gowinio-for-8gB-file-fix branch from ab8518e to 35c531d Compare September 19, 2020 15:13
@TBBle
Copy link
Contributor Author

TBBle commented Sep 19, 2020

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
Copy link
Contributor

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
Copy link
Contributor Author

TBBle commented Sep 19, 2020

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
Copy link
Contributor

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

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@@ -1,6 +1,6 @@
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@tiborvass tiborvass merged commit 29b149e into moby:master Sep 24, 2020
@thaJeztah
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docker fails to create Windows Containers with a file larger than 8gB
6 participants