Revendor hcsshim to fix image import bug #41144
Conversation
thaJeztah
added
kind/bugfix
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
Left a comment; open for discussion though, just curious what the path forward is 😅
vendor.conf
Outdated
There was a problem hiding this comment.
full diff is microsoft/hcsshim@v0.8.9...9dcb42f
perhaps would be good to add a comment with git describe output, to give a bit of context what version we're vendoring;
| github.com/Microsoft/hcsshim 9dcb42f100215f8d375b4a9265e5bba009217a85 | |
| github.com/Microsoft/hcsshim 9dcb42f100215f8d375b4a9265e5bba009217a85 # v0.8.9-1-g9dcb42f |
There was a problem hiding this comment.
Oh, actually, I see this is vendored from the moby branch? I see microsoft/hcsshim@9dcb42f was cherry-picked from microsoft/hcsshim#832 (which was against master)
In that case we should at least have a comment for that;
| github.com/Microsoft/hcsshim 9dcb42f100215f8d375b4a9265e5bba009217a85 | |
| github.com/Microsoft/hcsshim 9dcb42f100215f8d375b4a9265e5bba009217a85 # moby branch |
Will this cause issues if we update containerd vendoring (which does not vendor from that branch?); are there breaking changes in master that we cannot vendor? I'm a bit concerned if we start vendoring from custom branches as that would definitely complicate things for users using go modules (and if we perhaps one day make that transition as well)
There was a problem hiding this comment.
I should have given more context on this. :)
We changed to the moby branch for a couple of reasons:
- Allows us to take this important change without needing to also pull in all of the other work that has been going on in the repo.
- moby uses an older set of APIs exposed from hcsshim, based on the HCS v1 functionality. Going forwards, we have discussed deprecating/removing these APIs from the mainline branch in hcsshim, so our thinking was we could keep this moby branch around to ensure we don't break compatibility there.
I can definitely add in a # moby branch comment, so it's clear where this is coming from.
Do you know what hcsshim functionality moby uses through containerd package imports? I'm not aware of anything, but that is something we would need to be careful of with this approach. Long term, the best path here is to get moby using containerd as the backend on Windows, which should alleviate these issues.
There was a problem hiding this comment.
This is fine for now. We'll want to migrate windows to use containerd and we'll need to be careful to update this vendoring (assuming we need it even) once that is done.
This change brings in a single new commit from Microsoft/hcsshim. The commit fixes an issue when unpacking a Windows container layer which could result in incorrect directory timestamps. This manifested most significantly in an impact to startup times of some Windows container images (such as anything based on servercore). Signed-off-by: Kevin Parsons <[email protected]>
4 participants
This change brings in a single new commit from Microsoft/hcsshim. The
commit fixes an issue when unpacking a Windows container layer which
could result in incorrect directory timestamps.
This manifested most significantly in an impact to startup times of
some Windows container images (such as anything based on servercore). This can be verified by running the mcr.microsoft.com/windows/nanoserver:2004 image both with and without this fix.
Fixes #41066
Signed-off-by: Kevin Parsons [email protected]