-
Notifications
You must be signed in to change notification settings - Fork 612
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
Support for nerdctl build
on Windows
#2587
Conversation
06c6dc4
to
313cd6d
Compare
Okay, I broke something when moving the socket-finding logic, the rootless tests are panicking. I'll look into that |
605a14c
to
91b5161
Compare
Needs rebase |
@TBBle do you have some free cycles to complete this? buildkit v0.13.0-rc2 includes the windows buildkitd binary and would be great to have nerdctl working as well. Happy to pick this up if you are swamped |
I can quickly rebase it tonight, as it looks like the conflicting changes are simply adjacent to my changes, rather than actual conflicting changes. The requested changes don't look too bad, so I can probably get those done too tonight. |
5ee1299
to
58625bf
Compare
Okay, I've rebased it, and we'll let CI confirm I didn't break anything. I am not currently in a position to test it locally tonight though. Also, I added b5859e1 for #2587 (review) but it's mildly out-of-scope and slightly touches a handful of files so happy to have that split out into a separate PR if maintainers prefer. That one also makes Go 1.19 the minimum, but go.mod already states Go 1.20 as minimum so I assume that's probably fine. (I noticed that Go 1.21 actually makes the go.mod version a hard minimum, rather than a strong recommendation, which is nice.) A future TODO is to get a buildkitd running on Windows under CI, which should then automatically try to run the existing build test, which ought to work as-is... Edit: Test failure looks unrelated, and the logs show that (unlike master) the build-test skip logs the expected (So an extra future TODO to consider: Rig up buildkitd to work with rootless in the test suite, perhaps. Although maybe there's no value in that except testing the relevant socket.) So yeah, apart from what looks like a spurious failure in one of the rootless integration passes, CI seems to say it's fine. |
58625bf
to
b5859e1
Compare
I should have some time to test this today, thanks. |
I got 1.Nerdctl sets a default windows path on container images:
This is a universal problem we've seen with Buildkit as well that we need to address but unrelated to this PR. 2.Nerdctl Build does not work when progress is When Nerdctl Command:
Buildkit Response:
This particular issue goes away when we switch to Nerdctl Command: Response:
Short Term Fix: Long Term Fix: |
Huh, weird, I didn't see the TTY issue in my original testing. I wonder if something changed since then, or if my test setup was so trivial it didn't trigger this. If I recall correctly, my test setup here didn't actually have anything in the context except the Dockerfile. |
Can we merge this and track other issues in follow-up ? |
Sounds good to me |
Although it was being shown in the help output, the value was ignored in favour of auto-detection by getBuildkitHost, which could give different results anyway. Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>
This of course requires functional BuildKit for Windows. Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>
Buildkit on Windows doesn't support rootless mode, and doesn't put the namespace into the pipe name currently, so the Windows version is near-trivial. This also corrects the autodetection path on FreeBSD to start in /var/run instead of current Linux FHS's /run, and doesn't bother trying to support Rootless mode and related path guessing on FreeBSD. Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>
This mostly involves replacing anything that was trying to be "Not Windows" with the new-in-Go-1.19 "unix" build-constraint. Effectively, anything that contained both "linux" and "freebsd" but not "windows", or excluded only "windows", is now "unix". A couple of needless constraints were removed when the filenames already carried the appropriate constraint. A handful of files were renamed, so that now "unix"-suffixed files all use the "unix" constraint, and "other" is used when the constraint is more-complex, for "no specific implementation" cases. Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>
Rebased to main since I was going to fix the typo that turned out to not be a typo, so instead we get a trivial rebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
Doesn't seem to work, see #3629 |
Oops, looks like we never opened an issue for number 2 in #2587 (comment). |
Trivially bring up BuildKit support in nerdctl. It was basically all-there already, just disabled.
Resolves containerd/containerd#627 assuming you have Buildkitd functional on Windows, of course.
Resolves containerd/containerd#2173 which just turned out to be a warning that no error had occurred.
Tested with a trivial Dockerfile that was already working with
buildctl build
andnerdctl run
, and buildkitd set up as a service matching the existing rootful instructions (i.e. configured to be in thedefault
containerd namespace instead of the defaultbuildkit
containerd namespace) per moby/buildkit#616 (comment).Possible outstanding things:
Removing the CLI defaults, while correct because they were not actually used, isn't super-nice from the UX perspective. It might be better to report which paths will be auto-detected, but that requires some further refactoring, and might also be too noisy? Simply saying
<AUTODETECTED>
or something would work, perhaps.I've only tested this on Windows, and haven't checked if the test suite covers the buildkit socket detection code on Linux/FreeBSD, so it's possible I mangled something when moving that code. And I only tested the default namespace, but since Windows buildkitd doesn't change its default listening address for different containerd namespaces, this seems fine for now.
Actually, the buildkitd source doesn't appear to use different sockets for different containerd namespaces on Linux either, AFAICT. Is that logic actually syncing with a systemd unit configuration or something, rather than buildkitd's logic? It would make sense for buildkitd to work this way, at least for the containerd worker, it just doesn't seem to, or I didn't find the implementation when I went looking.