socketpair_windows: remove implementation for now#69
Conversation
|
@klihub if you know who to ping for windows review / testing |
I don't know whom we could ping about Windows-related stuff, sans git-blaming the Windows-specific bits in the containerd and picking folks based on that. Personally, I'd be ready/glad to merge this. Looking at the changes I can't really see this breaking anything that wouldn't be already broken. Also, we don't have Windows support in NRI core. @fuweid WDYT ? |
|
If someone can just launch the socketpair_test on a windows machine before merging (I don't have access to a windows box) |
|
Looking at containerd git log, maybe @dmcgowan can also review this ? |
It does not work, but it is not the fault of this PR. It was already broken. If we need/want to get NRI working on Windows, it will require bigger windows-specific changes, because there are other problems, too. For instance, the way how we try to pass the socket to a prelaunched plugin (exec/cmd.Command()'s ExtraFiles) is documented not to work on Windows. I'm not sure what to do with this. Maybe we should return an error instead in newSocketpairCLOEXEC for windows and add a comment there documenting that this will require more work and platform-specific code on Windows... @champtar WDYT ? |
|
@klihub I'm fine just having a stub returning an error for windows, just like what is commented at the start of NewSocketPair |
42137ec to
25d041e
Compare
Current implementation has the same issues that socketpair_unix.go had, namely double closes and leaking FDs. The initial author prefers to remove the code for now as the whole feature (NRI) doesn't work on windows for now. This allows us to rename socketpair_unix.go -> socketpair.go as everything in it is generic. Signed-off-by: Etienne Champetier <e.champetier@ateme.com>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #69 +/- ##
==========================================
- Coverage 64.58% 64.44% -0.14%
==========================================
Files 10 10
Lines 1838 1845 +7
==========================================
+ Hits 1187 1189 +2
- Misses 500 505 +5
Partials 151 151 ☔ View full report in Codecov by Sentry. |
25d041e to
e47f09b
Compare
|
@klihub new version just returning an error |
|
BTW once this is merged can we have a new tag on this repo so we can fix containerd ? |
Current implementation has the same issues that
socketpair_unix.go had, namely double closes and
leaking FDs. The initial author prefers to remove
the code for now as the whole feature (NRI) doesn't
work on windows for now.
This allows us to rename socketpair_unix.go -> socketpair.go
as everything in it is generic.