some packages on github are having some issues right now, i'm looking into the CI issues

@danail-branekov for v2. You can use it via ctr run --runtime io.containerd.runc.v1. I feel like it should work as we already use a named pipe but I havn't testing your reproduction case yet.

@danail-branekov for v2. You can use it via ctr run --runtime io.containerd.runc.v1. I feel like it should work as we already use a named pipe but I havn't testing your reproduction case yet.

My question was more about how to test v2 in an automated manner. Is that possible?
Running the very same integration tests against v2 does make sense to me, still the Makefile does not provide that capability.

Ahh, the CI does run both runtimes.

You should be able to set TEST_RUNTIME=io.containerd.runc.v1 and run the tests locally

I have rebased the PR onto master and made the TestDaemonReconnectsToShimIOPipesOnRestart test verify that containerd reconnects to the shim log after restart for both v1 and v2. As a matter of fact this capability was already there for v2.

There are however some discrepancies between v1 and v2 logging:

• v1 has separate log files for stdout and stderr that are located in /<root-dir>/io.containerd.runtime.v1.linux/<namespace>/<id>
• v2 has a log file for stderr only located in /<state-dir>/io.containerd.runtime.v2.task/<namespace>/<id>

Do we need need to address these discrepancies and always put files in either root-dir or state-dir? I was also tempted to make v2 have separate log files for stdout/stderr but I saw that v2 uses stdout to communicate between the service and the task manager e.g. here.

@danail-branekov ya, i see what you are talking about. Let me look at the code again and see what would be a good solution.

Also don't worry about the windows tests, we have a PR to fix the gcc issue that poped up over night.

V2 shim logging is done via a Fifo on Linux and Named Pipe on Windows. It does not use stdout, stderr so this should not affect the V2 runtimes at all.

@jterry75 Actually, v2 would forward stderr to the named pipe and would use stdout to return the shim address. This PR does pretty much the same for v1, except that the PR would forward both stdout and stderr to named pipes.
And yes, this PR does not affect v2 (unless we decide to unify named pipes implementation) except for introducing a test relevant to both v1 and v2 that verifies that a) the server reconnects to the named pipe on restart and b) shim logs are forwarded to the server log if debug is enabled.

@crosbymichael Has that GCC windows issue been resolved? If so we can force push to our fork to trigger CI again.

@BooleanCat - Yes it has been fixed for some time now.

@danail-branekov We tried rebasing on master and we still need windows failures. Let's take a deeper look at this.

Codecov Report

Merging #2631 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2631   +/-   ##
=======================================
  Coverage   43.72%   43.72%           
=======================================
  Files         100      100           
  Lines       10734    10734           
=======================================
  Hits         4693     4693           
  Misses       5311     5311           
  Partials      730      730

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44b90df...1d4105c. Read the comment docs.

The windows failures were because we introduced a file with unix imports and had not disabled the build on windows (// +build !windows). This is resolved now, so we should be good to go.

bump @crosbymichael

Thanks, I'll review this today

LGTM

/poke Any thoughts on this? We'd like to get this merged before it gets stale.

cc @ehazlett @AkihiroSuda @dmcgowan

LGTM