patch hcsshim to work with go1.9 #144
Conversation
legacy.go
Outdated
| @@ -407,16 +407,7 @@ func (w *legacyLayerWriter) reset() { | |||
| } | |||
|
|
|||
| // copyFileWithMetadata copies a file using the backup/restore APIs in order to preserve metadata | |||
| func copyFileWithMetadata(srcPath, destPath string, isDir bool) (fileInfo *winio.FileBasicInfo, err error) { | |||
There was a problem hiding this comment.
I think this function should remain the same. There's multiple uses of info.IsDir() in the caller which make the same assumption about Symlink and Dir modes being able to both be set. This should be done similar to isReparsePoint in the caller, and all instances of info.IsDir() in the caller should be replaced.
Can you also leave a comment with link to the above Go commit for future reference so this doesn't get accidentally changed back?
There was a problem hiding this comment.
@darrenstahlmsft Not all the uses of info.IsDir() can be replaced in the caller, namely the check if we want to skip the directory or not in the filepath.Walk function cannot be replaced since it seems it causes us to skip over things that shouldn't be skipped.
When it is replaced, we get the error:
GetFileAttributesEx \\?\C:\Users\vagrant\AppData\Local\Temp\1\extract.integration.test022193475\
da87b55a9b6358a65462540faeaa97505b0a12e1a2c14f08893589181d32d00d\UtilityVM\Files\ProgramData
\Microsoft\Windows Defender\Scans\History\CacheManager\01D5215E-0000-0000-0000-100000000000-0.bin:
The system cannot find the file specified.
when calling Remove() for an unneeded file (something that starts with .wh.) on the LayerWriter for that layer since something is missing from our parent layer but is in the current layer tarball header.
There was a problem hiding this comment.
We will update with a comment linking to that Golang commit, sounds good.
If we are happy with just leaving info.IsDir() in that one place, we can repush with those changes, but that seems a bit messy and hard to read if there are two ways of checking for being a directory. This is sort of why we initially pushed the change down into copyFileWithMetadata as that is the only place we could see that needed to rely on the older Golang implementation.
There was a problem hiding this comment.
That seems odd. That check to skip the directory will fail every time with the new 1.9 behaviour unless I'm missing something. It was that check that I expected should have the old behaviour.
I agree that having them mixed is messy, and want to avoid that. I'll take a look
There was a problem hiding this comment.
@darrenstahlmsft have you had a chance to take a look at this?
There was a problem hiding this comment.
I'm having some issues debugging due to go1.9 bugs in Moby. I think I can work around them though. It looks like the tar archive fork in go-winio also relies on this now outdated functionality. Might need some fixes there too.
There was a problem hiding this comment.
Alright. @jhowardmsft and I understand the problem now. Due to a Go1.9 bug, during a filepath.Walk on Windows, if the filepath.Skip is returned from a Directory Reparse Point (as is the case when the above suggested changes are made), filepath.Walk exits early, and swallows the error, returning nil. This causes a failure in reapplyDirectoryTimes(di), as even though there was no error during the Walk, not all the expected files are populated. (hence The system cannot find the file specified.)
As for a fix, I'm still working out exactly what we want to do here. I'm currently leaning towards forking filepath.Walk in order to fix the bug, and I think we can get some perf improvement out of it too while I'm at it. I'll think on this some more, and update here soon.
There was a problem hiding this comment.
Was a bug reported upstream in Go?
There was a problem hiding this comment.
Turns out what we felt was a bug was actually per design, and the bug was in Go1.8 and before. Filepath.Walk used to treat Directory Symlinks (which are different from File Symlinks on Windows, unlike Linux) as Directories, which meant that they needed to be skipped or some files would be traversed twice. Now that they are considered files in 1.9 (intended design), they can no longer be skipped, as a skip on a file skips the rest of the contents of the directory.
I added a conditional skip based on Go version in #147 to fix this for Go1.8 and before once this change is merged.
There was a problem hiding this comment.
Cool, thanks for explaining 👍
|
@sunjayBhatia I just merged #147 which prevents the errors caused when using the raw Can you rebase on that change? |
|
@darrenstahlmsft we have a story to do so and will let you know soon about validating those changes! thanks so much for this |
|
FWIW I validated this PR using Docker and Containerd locally (based on master) with the proposed change on both Go1.8 and Go1.9 😄 |
|
@sunjayBhatia can you rebase this PR on master please? |
|
ping @sunjayBhatia any news on this one? 😅 |
|
@darrenstahlmsft @thaJeztah rebased on master and updated this PR branch |
|
@sunjayBhatia thanks, but can you also add the suggestion from #144 (comment) back in? The fixes in master should prevent the issues you were seeing in #144 (comment) from occurring now, and I think that keeps the code here much more readable. |
In go1.9, fi.IsDir() returns false if the directory is also a symlink. See: golang/go@1989921 This breaks copyFileWithMetadata as it will not pre-create the destination dir, causing the SetFileBasicInfo call to fail This fixes the problem by checking syscall.FILE_ATTRIBUTE_DIRECTORY directly. Signed-off-by: Sam Smith <[email protected]>
|
@darrenstahlmsft updated to to the same as |
|
Looks good. Thanks! |
|
Thanks! |
In go1.9, fi.IsDir() returns false if the directory is also a symlink.
(see golang/go@1989921)
This breaks copyFileWithMetadata as it will not pre-create the
destination dir, causing the SetFileBasicInfo call to fail
This fixes the problem by checking syscall.FILE_ATTRIBUTE_DIRECTORY
directly.