Use go-winio for applying tarballs#4395
Conversation
|
Build succeeded.
|
TBBle
force-pushed
the
use_gowinio_for_reading_tarballs
branch
from
613d3c6 to
9ad6300
Compare
|
Build succeeded.
|
|
I'd note that it makes sense to keep the API passing the |
|
@tonistiigi PTAL |
TBBle
force-pushed
the
use_gowinio_for_reading_tarballs
branch
from
9ad6300 to
4230671
Compare
|
Build succeeded.
|
TBBle
changed the title
TBBle
force-pushed
the
use_gowinio_for_reading_tarballs
branch
from
4230671 to
31362ec
Compare
|
Build succeeded.
|
|
@kevpar - ptal |
|
@ambarve - PTAL |
TBBle
force-pushed
the
use_gowinio_for_reading_tarballs
branch
from
31362ec to
206d7a4
Compare
|
Build succeeded.
|
|
LGTM! |
TBBle
force-pushed
the
use_gowinio_for_reading_tarballs
branch
from
206d7a4 to
9229633
Compare
|
Build succeeded.
|
|
recheck |
|
Build succeeded.
|
|
recheck |
|
Build succeeded.
|
|
recheck |
|
Build succeeded.
|
|
As noted at microsoft/go-winio#175 (comment), we might need to merge and vendor microsoft/go-winio#175 before this goes in, as there's possibly a regression here in applying any Windows Container layers containing a file larger than 8gB (although I haven't confirmed this regression: the go-winio bug is about creating layers (microsoft/Windows-Containers#43 (comment)), so I'm assuming it has the same bug in applying layers, but it is only an assumption). However, the only code I'm aware of for creating WIndows Container layers is in https://github.com/microsoft/go-winio/ anyway (containerd can't do it until #4399 which depends on this PR, and Docker Engine uses the go-winio code: moby/moby#40444) so I suspect there's literally no existing Windows Container images containing a file larger than 8gB anyway. Unless moby/moby#40444 was a regression, and it used to work, but I've never seen a working version. That would imply Docker Engine used to contain better layer creation code, which would be weird. |
|
@jterry75 I think that it is time to you look this one as @ambarve did leave LGTM comment after your request but it this be reviewed by someone on maintainer role (+ this is very Windows specific change). |
|
LGTM (not a maintainer). This looks to be mostly changing from an implementation in containerd/containerd to the implementation that was added in Microsoft/go-winio, but the actual code is pretty much the same. As far as I can tell, any concern about working with 8GB+ files would have already been present, unless I'm missing something. |
|
Justin is currently on leave, so I don't think he will be able to look. We should try to get another maintainer to take a look. |
|
The 8gB file bug was in this PR before I rebased on microsoft/go-winio#175. So it's not here now, but was here up until September 9th. The bug doesn't exist in current master, because it's using archive/tar from Go, the same thing go-winio was changed to use in that PR. |
TBBle
force-pushed
the
use_gowinio_for_reading_tarballs
branch
from
1e3629c to
b29981a
Compare
|
Rebased to current master, and go-winio revendored to v0.4.15 (no relevant changes compared to the previous commit-SHA I was using). |
|
Build succeeded.
|
|
@kevpar - You still good with this? I can sign off |
Signed-off-by: Paul "TBBle" Hampson <[email protected]>
applyFunc now takes an io.Reader instead of a tar.Reader because I'm trying to mirror the API of the not-yet-exposed implementation of this same behaviour in github.com/Microsoft/hcsshim/internal/ociwclayer, with an eye to later moving to that implementation it is ever exposed. Signed-off-by: Paul "TBBle" Hampson <[email protected]>
Specifically, all the functions above applyWindowsLayer are actually used by the (generic) applyNaive code, while the functions below this point are specific to applyWindowsLayer. Signed-off-by: Paul "TBBle" Hampson <[email protected]>
TBBle
force-pushed
the
use_gowinio_for_reading_tarballs
branch
from
b29981a to
78f31af
Compare
|
Rebased for Go Modules support, no code-changes were required. Also, huzzah, Modules. |
|
Build succeeded.
|
|
Test failure in Linux Integration is surely unrelated to my changes. I'm sure I've seen that losetup randomly in other CI runs. |
|
LGTM! |
|
@TBBle - Agree this change should not have affected the test issue here. |
|
@thaJeztah / @tonistiigi - Either of you want to take a look here. I am good with this change. |
estesp
left a comment
estesp
left a comment
There was a problem hiding this comment.
LGTM - I re-ran tests but the actions runner IP actually hit DockerHub pull rate limits.. our first time, that I'm aware of :)
Windows and Linux integration ran against the PR (as well as unit tests); cgroups/SELinux testing is immaterial for this PR.
8 participants
archive/tar_windows.go mostly consisted of two large functions reimplementing functions in https://github.com/microsoft/go-winio/, which is already a dependency of this file.
This uses those functions instead, as a deduplication, and preparation for implementing
diff.Comparer, per #4394.Note that until microsoft/go-winio#175 is merged and vendored, this brings in a fork of Go's<== This is now done in v0.4.15.archive/tar, used aswinio_taronly in archive/tar_windows.go. Once that lands, the changes in archive/tar.go and archive/tar_opts.go can be reverted.https://github.com/microsoft/go-winio/ is revendored from v0.4.14 to v0.4.15, but most of the upstream changes were in code thatEdit: Rebased on the move from vndr to Go Modules, which had already pulled in 0.4.15.vndrexcluded before this PR anyway, and the changes that are included (see 8b05980ba5467b2c61815bf74cf82f73dd3dff64) are trivial refactorings.