Introduce containerd-shim-runhcs-v1 on Windows#2538
Conversation
|
@crosbymichael - PTAL |
jterry75
force-pushed
the
runtime_v2_windows
branch
from
32ea2fd to
d507eac
Compare
Codecov Report
@@ Coverage Diff @@
## master #2538 +/- ##
=======================================
Coverage 44.59% 44.59%
=======================================
Files 95 95
Lines 10007 10007
=======================================
Hits 4463 4463
Misses 4822 4822
Partials 722 722
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I think we may want to use a channel for this. There would be multiple people waiting on a single container and a channel handles this well. I cannot remember, but I tried this in the past with a waitgroup and had issues with multiple wait calls
There was a problem hiding this comment.
This logic is written such that N number of waiters can wait by calling p.waitForExit(). Once the process's background goroutine actually completes it calls p.exitWg.Done() so a process has exactly 1 call to p.exitWg.Add(1) done internally and all external waiters are blocked until this is completed. For a call like Stat(...) we expose a call p.safeExitCode() that is a non-blocking variant of p.waitForExit() with the expectation that the exitCode and exitTime might be invalid if it has not exited (255, time.Time{}). I could certainly expose a channel here but I don't see any benefit to the pattern over using wait groups. Am I missing something?
There was a problem hiding this comment.
Changed based on your feedback.
jterry75
force-pushed
the
runtime_v2_windows
branch
from
d507eac to
46ee2dd
Compare
|
@johnstep can you help review on this one as well? |
|
@crosbymichael Yes, but may not have time until Monday morning. |
There was a problem hiding this comment.
This looks like it can make use of https://godoc.org/golang.org/x/sync/errgroup
There was a problem hiding this comment.
Cool didnt know this existed!
There was a problem hiding this comment.
This struct field should be removed, CI is catching it as unused
jterry75
force-pushed
the
runtime_v2_windows
branch
2 times, most recently
from
77494ef to
8dfbb3d
Compare
|
@crosbymichael - Just rebased/squashed. This revendors |
jterry75
changed the title
jterry75
force-pushed
the
runtime_v2_windows
branch
2 times, most recently
from
e4cde6f to
d3588ed
Compare
|
It looks like the vendor failed or does not match the commits in the vendor.conf |
Implements the containerd-shim-runhcs-v1 shim on Windows for the runtime v2 shim API. Signed-off-by: Justin Terry (VM) <[email protected]>
jterry75
force-pushed
the
runtime_v2_windows
branch
from
d3588ed to
019b0c3
Compare
|
@crosbymichael - I updated hcsshim and pushed the changes here. Looks like some files were checked in with CRLF which caused them to not match the versions in the commit. |
|
LGTM I have a console change that can be updated after this that will fix windows Terminal usage but other than that, this is solid. |
6 participants
Implements the containerd-shim-runhcs-v1 shim on Windows for the runtime
v2 shim API.
Signed-off-by: Justin Terry (VM) [email protected]