Merge pull request #3536 from darfux/ignore_closed_fifo_error_under_multi_cntr

Ignore fifo error when using v2 multi-container shim

runtime/v2/binary.go

@@ -85,13 +85,10 @@ func (b *binary) Start(ctx context.Context, opts *types.Any, onClose func()) (_
 	// copy the shim's logs to containerd's output
 	go func() {
 		defer f.Close()
-		if _, err := io.Copy(os.Stderr, f); err != nil {
+		_, err := io.Copy(os.Stderr, f)
-			// When using a multi-container shim the 2nd to Nth container in the
+		err = checkCopyShimLogError(ctx, err)
-			// shim will not have a separate log pipe. Ignore the failure log
+		if err != nil {
-			// message here when the shim connect times out.
+			log.G(ctx).WithError(err).Error("copy shim log")
-			if !os.IsNotExist(errors.Cause(err)) {
-				log.G(ctx).WithError(err).Error("copy shim log")
-			}
 		}
 	}()
 	out, err := cmd.CombinedOutput()

runtime/v2/shim_unix.go

@@ -30,3 +30,17 @@ import (
 func openShimLog(ctx context.Context, bundle *Bundle) (io.ReadCloser, error) {
 	return fifo.OpenFifo(ctx, filepath.Join(bundle.Path, "log"), unix.O_RDONLY|unix.O_CREAT|unix.O_NONBLOCK, 0700)
 }
+
+func checkCopyShimLogError(ctx context.Context, err error) error {
+	// When using a multi-container shim, the fifo of the 2nd to Nth
+	// container will not be opened when the ctx is done. This will
+	// cause an ErrReadClosed that can be ignored.
+	select {
+	case <-ctx.Done():
+		if err == fifo.ErrReadClosed {
+			return nil
+		}
+	default:
+	}
+	return err
+}

runtime/v2/shim_unix_test.go

@@ -0,0 +1,49 @@
+// +build linux
+
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package v2
+
+import (
+	"context"
+	"testing"
+
+	"github.com/containerd/fifo"
+)
+
+func TestCheckCopyShimLogError(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+
+	if err := checkCopyShimLogError(ctx, fifo.ErrReadClosed); err != fifo.ErrReadClosed {
+		t.Fatalf("should return the actual error before context is done, but %v", err)
+	}
+	if err := checkCopyShimLogError(ctx, nil); err != nil {
+		t.Fatalf("should return the actual error before context is done, but %v", err)
+	}
+
+	cancel()
+
+	if err := checkCopyShimLogError(ctx, fifo.ErrReadClosed); err != nil {
+		t.Fatalf("should return nil when error is ErrReadClosed after context is done, but %v", err)
+	}
+	if err := checkCopyShimLogError(ctx, nil); err != nil {
+		t.Fatalf("should return the actual error after context is done, but %v", err)
+	}
+	if err := checkCopyShimLogError(ctx, fifo.ErrRdFrmWRONLY); err != fifo.ErrRdFrmWRONLY {
+		t.Fatalf("should return the actual error after context is done, but %v", err)
+	}
+}

runtime/v2/shim_windows.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"io"
 	"net"
+	"os"
 	"sync"
 	"time"
 
@@ -85,3 +86,13 @@ func openShimLog(ctx context.Context, bundle *Bundle) (io.ReadCloser, error) {
 	}()
 	return dpc, nil
 }
+
+func checkCopyShimLogError(ctx context.Context, err error) error {
+	// When using a multi-container shim the 2nd to Nth container in the
+	// shim will not have a separate log pipe. Ignore the failure log
+	// message here when the shim connect times out.
+	if os.IsNotExist(errors.Cause(err)) {
+		return nil
+	}
+	return err
+}

runtime/v2/shim_windows_test.go

@@ -0,0 +1,41 @@
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package v2
+
+import (
+	"context"
+	"os"
+	"testing"
+
+	"github.com/pkg/errors"
+)
+
+func TestCheckCopyShimLogError(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	testError := errors.New("test error")
+
+	if err := checkCopyShimLogError(ctx, nil); err != nil {
+		t.Fatalf("should return the actual error except ErrNotExist, but %v", err)
+	}
+	if err := checkCopyShimLogError(ctx, testError); err != testError {
+		t.Fatalf("should return the actual error except ErrNotExist, but %v", err)
+	}
+	if err := checkCopyShimLogError(ctx, os.ErrNotExist); err != nil {
+		t.Fatalf("should return nil for ErrNotExist, but %v", err)
+	}
+}