socketpair_windows: remove implementation for now

[champtar]

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.

[champtar]

@klihub if you know who to ping for windows review / testing

[klihub]

@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 ?

[champtar]

If someone can just launch the socketpair_test on a windows machine before merging (I don't have access to a windows box)

[champtar]

Looking at containerd git log, maybe @dmcgowan can also review this ?

[klihub]

If someone can just launch the socketpair_test on a windows machine before merging (I don't have access to a windows box)

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 ?

[champtar]

@klihub I'm fine just having a stub returning an error for windows, just like what is commented at the start of NewSocketPair

[codecov-commenter]

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (4298b41) 64.58% compared to head (269bae5) 64.44%.
Report is 5 commits behind head on main.

❗ Current head 269bae5 differs from pull request most recent head e47f09b. Consider uploading reports for the commit e47f09b to get more accurate results

Files	Patch %	Lines
pkg/adaptation/adaptation.go	0.00%	5 Missing ⚠️

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.
📢 Have feedback on the report? Share it here.

[champtar]

@klihub new version just returning an error

[klihub]

Thank you ! LGTM.

[champtar]

BTW once this is merged can we have a new tag on this repo so we can fix containerd ?

[fuweid]

LGTM

Sorry for late reply

[fuweid]

@champtar https://github.com/containerd/nri/releases/tag/v0.6.0 done
cc @klihub