Fix bug in extracting hardlinks #284
Conversation
pkg/util/fs_util.go
Outdated
| @@ -247,6 +287,15 @@ func checkWhiteouts(path string, whiteouts map[string]struct{}) bool { | |||
| return false | |||
| } | |||
|
|
|||
| func checkSymlinks(path string, symlinks map[string]struct{}) bool { | |||
There was a problem hiding this comment.
I think I'm not sure why we need this one if we're checking the other stuff. Are you sure we need it?
There was a problem hiding this comment.
You're right, thanks for pointing that out! I'll remove it.
There was a problem hiding this comment.
I think we might actually need the symlink check, since we only want to specially hardlink at the end of extraction if the linked file exists but can't be hardlinked (which I think only happens if it's a symlink)
If a file was normally extracted in a previous layer than we can extract the file and create the hardlink normally
pkg/util/fs_util.go
Outdated
| } | ||
| hardlinks[linkname] = &hardlink{ | ||
| links: []*tar.Header{hdr}, | ||
| reader: tr, |
There was a problem hiding this comment.
Hmm, I'd be kinda surprised if saving off the tar reader here works later. Per the docs for tr.Next:
Next advances to the next entry in the tar archive. The Header.Size determines how many bytes can be read for the next file. Any remaining data in the current file is automatically discarded.
(emphasis mine)
One problem is that it's basically streaming through the archvie sequentially as it's downloading. It never buffers to disk or RAM, so I don't think you can go "back" to earlier entries in the archive. Tricky.
There was a problem hiding this comment.
Thanks for pointing this out, fixed.
priyawadhwa
force-pushed
the
hardlink
branch
5 times, most recently
from
e00f0d9 to
3dc172a
Compare
priyawadhwa
changed the title
|
Woot tests are finally passing, PTAL! |
pkg/util/fs_util.go
Outdated
| @@ -55,6 +62,10 @@ func GetFSFromImage(root string, img v1.Image) error { | |||
|
|
|||
| fs := map[string]struct{}{} | |||
| whiteouts := map[string]struct{}{} | |||
| hardlinks, err := retrieveHardlinks(layers) | |||
There was a problem hiding this comment.
Do you think it would make sense to process each layer separately instead of doing them all at once? I think we already disallow hardlinks to cross layer boundaries.
There was a problem hiding this comment.
Yah I think that would be better, I changed it to retrieve hardlinks as we process layers
|
I think this needs a rebase now. |
|
Done! :) |
Extracting the layers of the filesystem in order will make it easier to extract cached layers and deal with hardlinks, as mentioned in GoogleContainerTools#284 This PR implements extracting in order and adds an integration tests to test the bug hardlinks error in GoogleContainerTools#284 It also fixes GoogleContainerTools#325
Extracting the layers of the filesystem in order will make it easier to extract cached layers and deal with hardlinks, as mentioned in GoogleContainerTools#284 This PR implements extracting in order and adds an integration tests to test the bug hardlinks error in GoogleContainerTools#284 It also fixes GoogleContainerTools#325
|
Closing, fixed by #326 |
Should fix the issues brought up in #282