Implement Archive.apply on Windows by darstahl · Pull Request #1903 · containerd/containerd

darstahl · 2017-12-09T01:05:39Z

This implements archive.Apply on Windows. It uses opts similar to Snapshots.Opt, etc.

The current options on Windows are BaseLayers (which is required for all layers except the base layer) and isolation, which is not currently used, but will be enabled in another PR, and is necessary for Windows layer extraction.

I have a local updated version of snapshotter and differ PR that I've tested this against. I'll be pushing that one soon.

The second commit is a temp hack as vndr is deleting the file even though it is referenced. I'll work that out on Monday.

@crosbymichael @stevvooe @dmcgowan @jessvalarezo

dnephin · 2017-12-11T16:43:28Z

vendor failures are because vendor.conf needs to be updated to use the correct version

dmcgowan · 2017-12-11T18:21:15Z

Thanks @darrenstahlmsft, this is very clean! Is there ever a case where the existing Apply logic which is now in tar_unix.go would be called in Windows, I was thinking maybe if it was used with the naive snapshot driver for something?

Signed-off-by: Darren Stahl <[email protected]>

darstahl · 2017-12-11T20:27:37Z

@dnephin Oops. Silly mistake on my part. Fixed. Thanks.

@dmcgowan It would not be used for any container layers on Windows, but I guess the naive snapshotter can be used to manage things other than container layers. I can restructure in a way that the old tar apply can be used as well from the naive snapshotter.

darstahl · 2017-12-11T21:11:36Z

Added a new ApplyOpt called AsWindowsContainerLayer to trigger using the Windows Container Layer apply code, otherwise it does the naive apply as it used to. That way naive differ will work the same without changes, but the new Windows Differ can apply Windows layers.

codecov-io · 2017-12-11T21:27:40Z

Codecov Report

Merging #1903 into master will decrease coverage by 1.48%.
The diff coverage is 7.83%.

@@            Coverage Diff            @@
##           master   #1903      +/-   ##
=========================================
- Coverage   47.29%   45.8%   -1.49%     
=========================================
  Files          89      91       +2     
  Lines        8830    9127     +297     
=========================================
+ Hits         4176    4181       +5     
- Misses       3968    4258     +290     
- Partials      686     688       +2

Flag	Coverage Δ
#linux	`50.66% <36.66%> (-0.01%)`	⬇️
#windows	`40.7% <5.7%> (-1.54%)`	⬇️

Impacted Files	Coverage Δ
archive/tar_opts_windows.go	`0% <0%> (ø)`
archive/strconv.go	`0% <0%> (ø)`
archive/tar_windows.go	`8.49% <0.8%> (-32.19%)`	⬇️
archive/tar_unix.go	`52.38% <100%> (+1.56%)`	⬆️
archive/tar.go	`43.68% <32.05%> (-0.31%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6393165...539192a. Read the comment docs.

darstahl · 2017-12-14T22:00:52Z

This closes #1678 when merged.

dmcgowan · 2017-12-14T23:18:21Z

Why move above? Is there a reason we would want to do the done check after the blocking call rather than before?

Not intentional. I moved this when attempting to combine the tar extract loop with Windows apply, but decided against it. I'll move it back where it was.

dmcgowan · 2017-12-14T23:35:26Z

Could these privilege calls be done by the caller?

Certainly could be. Moving it to the client would also mean that the archive.Apply would not need to be aware if it is running isolated or not, which is likely better.

dmcgowan · 2017-12-14T23:38:13Z

Do we want to remove this for now based on previous discussion? What would happen if 2 unpacks are happening simultaneously. Personally I would rather not have the archive package know about isolation and privileges and leave that to the higher level caller.

Yes. I'll remove this for now.

dmcgowan · 2017-12-14T23:47:10Z

I am not familiar enough with links in Windows, does any of the Window's apply code need to be leveraging fs.RootPath?

darstahl · 2017-12-15T00:05:44Z

This is handled by the layer extraction code in the platform. All paths should be relative to the layer root, rather than full paths, which is why the paths here do not need to be rooted.

Edit: Note that the links in this code are not OS links, but rather adding links via the hcsshim layer writer APIs

Signed-off-by: Darren Stahl <[email protected]>

codecov-io · 2017-12-15T19:46:55Z

Codecov Report

Merging #1903 into master will decrease coverage by 1.37%.
The diff coverage is 8.04%.

@@            Coverage Diff             @@
##           master    #1903      +/-   ##
==========================================
- Coverage   47.14%   45.76%   -1.38%     
==========================================
  Files          89       91       +2     
  Lines        8833     9109     +276     
==========================================
+ Hits         4164     4169       +5     
- Misses       3981     4250     +269     
- Partials      688      690       +2

Flag	Coverage Δ
#linux	`50.48% <35.59%> (-0.01%)`	⬇️
#windows	`40.65% <5.78%> (-1.43%)`	⬇️

Impacted Files	Coverage Δ
archive/tar_opts_windows.go	`0% <0%> (ø)`
archive/strconv.go	`0% <0%> (ø)`
archive/tar_windows.go	`8.96% <0.86%> (-31.72%)`	⬇️
archive/tar_unix.go	`52.38% <100%> (+1.56%)`	⬆️
archive/tar.go	`43.68% <31.16%> (-0.31%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5956e15...c195ebb. Read the comment docs.

dmcgowan · 2017-12-18T21:39:30Z

+		if err != nil {
+			return nil, err
+		}
+		if ahdr.Typeflag != tar.TypeReg || !strings.HasPrefix(ahdr.Name, hdr.Name+":") {


Is : safe here because Windows doesn't allow : as part of a file name?

dmcgowan · 2017-12-18T21:42:45Z

+}
+
+// fileInfoFromHeader retrieves basic Win32 file information from a tar header, using the additional metadata written by
+// WriteTarFileFromBackupStream.


Where is WriteTarFileFromBackupStream?

Current implementation used by Docker is at https://github.com/Microsoft/go-winio/blob/master/backuptar/tar.go#L119

A second PR to implement the Diff side of archive will include the containerd implementation.

dmcgowan · 2017-12-18T21:43:08Z

A couple questions then LGTM

crosbymichael · 2018-01-02T21:07:08Z

LGTM

stevvooe · 2018-01-02T21:44:55Z

+// ApplyOptions provides additional options for an Apply operation
+type ApplyOptions struct {
+	ParentLayerPaths        []string // Parent layer paths used for Windows layer apply
+	IsWindowsContainerLayer bool     // True if the tar stream to be applied is a Windows Container Layer


Would it help to make this more generic? It seems like this should be set via a mediatype or other mechanism.

Vendor go-winio

c71bee7

Signed-off-by: Darren Stahl <[email protected]>

darstahl force-pushed the ArchiveOpts branch from f88c661 to 3eb364d Compare December 11, 2017 20:05

darstahl force-pushed the ArchiveOpts branch from 3eb364d to 539192a Compare December 11, 2017 21:06

stevvooe added this to the 1.1 milestone Dec 14, 2017

dmcgowan reviewed Dec 14, 2017

View reviewed changes

darstahl mentioned this pull request Dec 14, 2017

Windows: Roadmap to WCOW #1920

Closed

8 tasks

Implement archive.Apply on Windows

c195ebb

Signed-off-by: Darren Stahl <[email protected]>

darstahl force-pushed the ArchiveOpts branch from 539192a to c195ebb Compare December 15, 2017 19:33

dmcgowan reviewed Dec 18, 2017

View reviewed changes

crosbymichael merged commit e479165 into containerd:master Jan 2, 2018

stevvooe reviewed Jan 2, 2018

View reviewed changes

darstahl deleted the ArchiveOpts branch January 17, 2018 01:01

Conversation

darstahl commented Dec 9, 2017

Uh oh!

dnephin commented Dec 11, 2017

Uh oh!

dmcgowan commented Dec 11, 2017

Uh oh!

darstahl commented Dec 11, 2017

Uh oh!

darstahl commented Dec 11, 2017

Uh oh!

codecov-io commented Dec 11, 2017

Codecov Report

Uh oh!

darstahl commented Dec 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmcgowan commented Dec 14, 2017

Uh oh!

darstahl commented Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Dec 15, 2017

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmcgowan commented Dec 18, 2017

Uh oh!

crosbymichael commented Jan 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

darstahl commented Dec 15, 2017 •

edited

Loading