Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update go-winio package to latest version #42307

Closed
wants to merge 1 commit into from
Closed

Update go-winio package to latest version #42307

wants to merge 1 commit into from

Conversation

@awmirantis
Copy link
Contributor

@awmirantis awmirantis commented Apr 20, 2021

Signed-off-by: Adam Williams awilliams@mirantis.com

Update Microsoft/go-winio to latest version. A change in this version is needed for a future fix to using docker data root on a VHDX.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Apr 20, 2021

Heads up; this will conflict with #42270 (once merged)

Loading

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Apr 20, 2021

Looks like there's a breaking change in this version of go-winio

..\..\vendor\github.com\containerd\containerd\archive\tar_windows.go:234:3: cannot use syscall.NsecToFiletime(hdr.AccessTime.UnixNano()) (type syscall.Filetime) as type
"github.com/docker/docker/vendor/golang.org/x/sys/windows".Filetime in field value
..\..\vendor\github.com\containerd\containerd\archive\tar_windows.go:235:3: cannot use syscall.NsecToFiletime(hdr.ModTime.UnixNano()) (type syscall.Filetime) as type
"github.com/docker/docker/vendor/golang.org/x/sys/windows".Filetime in field value
..\..\vendor\github.com\containerd\containerd\archive\tar_windows.go:236:3: cannot use syscall.NsecToFiletime(hdr.ChangeTime.UnixNano()) (type syscall.Filetime) as type
"github.com/docker/docker/vendor/golang.org/x/sys/windows".Filetime in field value
..\..\vendor\github.com\containerd\containerd\archive\tar_windows.go:239:3: cannot use syscall.NsecToFiletime(hdr.ModTime.UnixNano()) (type syscall.Filetime) as type
"github.com/docker/docker/vendor/golang.org/x/sys/windows".Filetime in field value
..\..\vendor\github.com\containerd\containerd\archive\tar_windows.go:257:25: cannot use syscall.NsecToFiletime(createTime.UnixNano()) (type syscall.Filetime) as type
"github.com/docker/docker/vendor/golang.org/x/sys/windows".Filetime in assignment

Looking at the diff since v0.4.16 (which is green in #42270); microsoft/go-winio@v0.4.16...v0.4.18

I think this is the one that causes the failure; microsoft/go-winio#185
There actually was a discussion on that in microsoft/go-winio#197 (comment), but looks like this one already went into v0.4.x (instead of v0.5.x)

/cc @kevpar @TBBle

Looks like that change requires an updated version of hcsshim (with microsoft/hcsshim#947) and containerd as well

Loading

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Apr 20, 2021

Heads up; this will conflict with #42270 (once merged)

#42270 was merged, so this needs a rebase now 🤗

Loading

@TBBle
Copy link
Contributor

@TBBle TBBle commented Apr 20, 2021

Hmm, this one's going to be a little tricky to unwind: The removal of the forked go-winio/backuptar (in containerd) where this compile failure is happening (containerd/containerd#4395) is only in containerd 1.5.0.

The API change in question is in go-winio 0.4.17, (microsoft/go-winio@bfd5468) which is also where the fix I assume we're trying to get (microsoft/go-winio@83fd176) shipped.

hcsshim itself hit this problem in its integration test suite (microsoft/hcsshim#946) and ended up pulling containerd 1.5 beta in order to unblock go-winio upgrades: microsoft/hcsshim#946

A structure type used by that deduplicated has changed in go-winio recently, and so pulling in both older containerd and newer go-winio leads to compile failures.

where "that deduplicated" (sic) means containerd/containerd#4395.

hcsshim itself isn't using these APIs, but newer containerd will require newer hcsshim due to containerd/containerd@a64a768 which depends on hcsshim 0.8.15 (microsoft/hcsshim#916) (We're on hcsshim 0.8.16 already, so I think we don't need to upgrade it any further to resolve this).

I'm actually somewhat surprised this has occurred, because I thought moby/moby didn't use this code, providing its own GraphDriver instead, but I guess Go can't avoid compiling it, even if it's never run. Turns out it's used in func (c *client) Start and func (c *client) CreateCheckpoint of libcontainerd/remote.

Anyway, some options:

  • Fork go-winio 0.4.18 for moby/moby with reverted microsoft/go-winio#185. The PR that prompted that PR hasn't landed yet, so it (or just the two commits regarding FileInfo) should be safe to revert.
  • Fork go-winio 0.4.16 for moby/moby and backport the desired VHDX fix.
  • Revert the two FileInfo commits in go-winio, release a new 0.4.x version, and then unrevert them to start the 0.5 series.
  • Fork containerd 1.4 for moby/moby with containerd/containerd#4395 backported.
  • Backport containerd/containerd#4395 to containerd 1.4 and release a new patch version.
  • Wait for containerd 1.5.0 to ship, and upgrade to it first.
  • Pull a containerd 1.5 beta release.

Of those, I would prefer the new containerd 1.4.x release. containerd/containerd#4395 was originally prepared and tested against containerd 1.4.0-beta.2 (it just took a while to be merged) so I expect no problems with doing that. It's also the most-pain to co-ordinate though.

A new hcsshim 0.4.x release would be my second preference, but again, takes co-ordination.

The other options can be done unilaterally here.

Loading

@katiewasnothere
Copy link

@katiewasnothere katiewasnothere commented Apr 21, 2021

@TBBle I'll be working on option 3 (Revert the two FileInfo commits in go-winio, release a new 0.4.x version, and then unrevert them to start the 0.5 series) today.

Loading

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Apr 22, 2021

for easier finding: pull request for the above is microsoft/go-winio#204

Loading

tianon
tianon approved these changes Apr 22, 2021
Copy link
Member

@tianon tianon left a comment

LGTM

Loading

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Apr 22, 2021

@awmirantis could you squash the commits?

Loading

Signed-off-by: Adam Williams <awilliams@mirantis.com>
@awmirantis awmirantis closed this Apr 26, 2021
@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Apr 26, 2021

continued in #42327

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants