Merge pull request containerd#3896 from Shubhranshu153/fix-logs-on-attach

fix: logs updated on starting of a stopped container

Author: AkihiroSuda

cmd/nerdctl/compose/compose_start.go

@@ -112,7 +112,7 @@ func startContainers(ctx context.Context, client *containerd.Client, containers
 			}
 
 			// in compose, always disable attach
-			if err := containerutil.Start(ctx, c, false, client, ""); err != nil {
+			if err := containerutil.Start(ctx, c, false, false, client, ""); err != nil {
 				return err
 			}
 			info, err := c.Info(ctx, containerd.WithoutRefreshedMetadata)

cmd/nerdctl/container/container_logs_test.go

@@ -19,8 +19,10 @@ package container
 import (
 	"errors"
 	"fmt"
+	"io"
 	"os/exec"
 	"runtime"
+	"strconv"
 	"strings"
 	"testing"
 	"time"
@@ -391,3 +393,74 @@ func TestLogsWithDetails(t *testing.T) {
 
 	testCase.Run(t)
 }
+
+func TestLogsWithStartContainer(t *testing.T) {
+	testCase := nerdtest.Setup()
+
+	// For windows we  havent added support for dual logging so not adding the test.
+	testCase.Require = require.Not(require.Windows)
+
+	testCase.SubTests = []*test.Case{
+		{
+			Description: "Test logs are directed correctly for container start of a interactive container",
+			Setup: func(data test.Data, helpers test.Helpers) {
+				cmd := helpers.Command("run", "-it", "--name", data.Identifier(), testutil.CommonImage)
+				cmd.WithPseudoTTY()
+				cmd.WithFeeder(func() io.Reader {
+					return strings.NewReader("echo foo\nexit\n")
+				})
+
+				cmd.Run(&test.Expected{
+					ExitCode: 0,
+				})
+
+			},
+			Cleanup: func(data test.Data, helpers test.Helpers) {
+				helpers.Anyhow("rm", "-f", data.Identifier())
+			},
+			Command: func(data test.Data, helpers test.Helpers) test.TestableCommand {
+				cmd := helpers.Command("start", "-ia", data.Identifier())
+				cmd.WithPseudoTTY()
+				cmd.WithFeeder(func() io.Reader {
+					return strings.NewReader("echo bar\nexit\n")
+				})
+				cmd.Run(&test.Expected{
+					ExitCode: 0,
+				})
+				cmd = helpers.Command("logs", data.Identifier())
+
+				return cmd
+			},
+			Expected: test.Expects(0, nil, expect.Contains("foo", "bar")),
+		},
+		{
+			Description: "Test logs are captured after stopping and starting a non-interactive container and continue capturing new logs",
+			Setup: func(data test.Data, helpers test.Helpers) {
+				helpers.Ensure("run", "-d", "--name", data.Identifier(), testutil.CommonImage, "sh", "-c", "while true; do echo foo; sleep 1; done")
+			},
+			Cleanup: func(data test.Data, helpers test.Helpers) {
+				helpers.Anyhow("rm", "-f", data.Identifier())
+			},
+			Command: func(data test.Data, helpers test.Helpers) test.TestableCommand {
+				helpers.Ensure("stop", data.Identifier())
+				initialLogs := helpers.Capture("logs", data.Identifier())
+				initialFooCount := strings.Count(initialLogs, "foo")
+				data.Labels().Set("initialFooCount", strconv.Itoa(initialFooCount))
+				helpers.Ensure("start", data.Identifier())
+				nerdtest.EnsureContainerStarted(helpers, data.Identifier())
+				return helpers.Command("logs", data.Identifier())
+			},
+			Expected: func(data test.Data, helpers test.Helpers) *test.Expected {
+				return &test.Expected{
+					ExitCode: 0,
+					Output: func(stdout string, info string, t *testing.T) {
+						finalLogsCount := strings.Count(stdout, "foo")
+						initialFooCount, _ := strconv.Atoi(data.Labels().Get("initialFooCount"))
+						assert.Assert(t, finalLogsCount > initialFooCount, "Expected 'foo' count to increase after restart", info)
+					},
+				}
+			},
+		},
+	}
+	testCase.Run(t)
+}

cmd/nerdctl/container/container_start.go

@@ -43,7 +43,7 @@ func StartCommand() *cobra.Command {
 	cmd.Flags().SetInterspersed(false)
 	cmd.Flags().BoolP("attach", "a", false, "Attach STDOUT/STDERR and forward signals")
 	cmd.Flags().String("detach-keys", consoleutil.DefaultDetachKeys, "Override the default detach keys")
-
+	cmd.Flags().BoolP("interactive", "i", false, "Attach container's STDIN")
 	return cmd
 }
 
@@ -60,11 +60,16 @@ func startOptions(cmd *cobra.Command) (types.ContainerStartOptions, error) {
 	if err != nil {
 		return types.ContainerStartOptions{}, err
 	}
+	interactive, err := cmd.Flags().GetBool("interactive")
+	if err != nil {
+		return types.ContainerStartOptions{}, err
+	}
 	return types.ContainerStartOptions{
-		Stdout:     cmd.OutOrStdout(),
-		GOptions:   globalOptions,
-		Attach:     attach,
-		DetachKeys: detachKeys,
+		Stdout:      cmd.OutOrStdout(),
+		GOptions:    globalOptions,
+		Attach:      attach,
+		DetachKeys:  detachKeys,
+		Interactive: interactive,
 	}, nil
 }
 

cmd/nerdctl/container/container_start_linux_test.go

@@ -52,13 +52,7 @@ func TestStartDetachKeys(t *testing.T) {
 	}
 
 	testCase.Command = func(data test.Data, helpers test.Helpers) test.TestableCommand {
-		flags := "-a"
-		// Started container must be interactive - which is apparently the default for nerdctl, which does not support
-		// the -i flag, while docker requires it explicitly
-		if nerdtest.IsDocker() {
-			flags += "i"
-		}
-		cmd := helpers.Command("start", flags, "--detach-keys=ctrl-a,ctrl-b", data.Identifier())
+		cmd := helpers.Command("start", "-ai", "--detach-keys=ctrl-a,ctrl-b", data.Identifier())
 		cmd.WithPseudoTTY()
 		cmd.WithFeeder(func() io.Reader {
 			// ctrl+a and ctrl+b (see https://en.wikipedia.org/wiki/C0_and_C1_control_codes)

pkg/api/types/container_types.go

@@ -30,6 +30,8 @@ type ContainerStartOptions struct {
 	Attach bool
 	// The key sequence for detaching a container.
 	DetachKeys string
+	// Attach stdin
+	Interactive bool
 }
 
 // ContainerKillOptions specifies options for `nerdctl (container) kill`.

pkg/cmd/container/restart.go

@@ -38,7 +38,7 @@ func Restart(ctx context.Context, client *containerd.Client, containers []string
 			if err := containerutil.Stop(ctx, found.Container, options.Timeout, options.Signal); err != nil {
 				return err
 			}
-			if err := containerutil.Start(ctx, found.Container, false, client, ""); err != nil {
+			if err := containerutil.Start(ctx, found.Container, false, false, client, ""); err != nil {
 				return err
 			}
 			_, err := fmt.Fprintln(options.Stdout, found.Req)

pkg/cmd/container/start.go

@@ -40,7 +40,7 @@ func Start(ctx context.Context, client *containerd.Client, reqs []string, option
 			if found.MatchCount > 1 {
 				return fmt.Errorf("multiple IDs found with provided prefix: %s", found.Req)
 			}
-			if err := containerutil.Start(ctx, found.Container, options.Attach, client, options.DetachKeys); err != nil {
+			if err := containerutil.Start(ctx, found.Container, options.Attach, options.Interactive, client, options.DetachKeys); err != nil {
 				return err
 			}
 			if !options.Attach {

pkg/containerutil/containerutil.go

@@ -213,7 +213,7 @@ func GenerateSharingPIDOpts(ctx context.Context, targetCon containerd.Container)
 }
 
 // Start starts `container` with `attach` flag. If `attach` is true, it will attach to the container's stdio.
-func Start(ctx context.Context, container containerd.Container, flagA bool, client *containerd.Client, detachKeys string) (err error) {
+func Start(ctx context.Context, container containerd.Container, flagA bool, flagI bool, client *containerd.Client, detachKeys string) (err error) {
 	// defer the storage of start error in the dedicated label
 	defer func() {
 		if err != nil {
@@ -243,7 +243,7 @@ func Start(ctx context.Context, container containerd.Container, flagA bool, clie
 	}
 	flagT := process.Process.Terminal
 	var con console.Console
-	if flagA && flagT {
+	if (flagI || flagA) && flagT {
 		con, err = consoleutil.Current()
 		if err != nil {
 			return err
@@ -284,7 +284,7 @@ func Start(ctx context.Context, container containerd.Container, flagA bool, clie
 		// source: https://github.com/containerd/nerdctl/blob/main/docs/command-reference.md#whale-nerdctl-start
 		attachStreamOpt = []string{"STDOUT", "STDERR"}
 	}
-	task, err := taskutil.NewTask(ctx, client, container, attachStreamOpt, false, flagT, true, con, logURI, detachKeys, namespace, detachC)
+	task, err := taskutil.NewTask(ctx, client, container, attachStreamOpt, flagI, flagT, true, con, logURI, detachKeys, namespace, detachC)
 	if err != nil {
 		return err
 	}

pkg/errutil/exit_coder.go

@@ -16,7 +16,9 @@
 
 package errutil
 
-import "os"
+import (
+	"os"
+)
 
 type ExitCoder interface {
 	error
@@ -46,7 +48,6 @@ func HandleExitCoder(err error) {
 	if err == nil {
 		return
 	}
-
 	if exitErr, ok := err.(ExitCoder); ok {
 		os.Exit(exitErr.ExitCode())
 	}

pkg/taskutil/taskutil.go

@@ -66,15 +66,24 @@ func NewTask(ctx context.Context, client *containerd.Client, container container
 	var ioCreator cio.Creator
 	if len(attachStreamOpt) != 0 {
 		log.G(ctx).Debug("attaching output instead of using the log-uri")
+		// when attaching a TTY we use writee for stdio and binary for log persistence
 		if flagT {
-			in, err := consoleutil.NewDetachableStdin(con, detachKeys, closer)
-			if err != nil {
-				return nil, err
+			var in io.Reader
+			if flagI {
+				// FIXME: check IsTerminal on Windows too
+				if runtime.GOOS != "windows" && !term.IsTerminal(0) {
+					return nil, errors.New("the input device is not a TTY")
+				}
+				var err error
+				in, err = consoleutil.NewDetachableStdin(con, detachKeys, closer)
+				if err != nil {
+					return nil, err
+				}
 			}
-			ioCreator = cio.NewCreator(cio.WithStreams(in, con, nil), cio.WithTerminal)
+			ioCreator = cioutil.NewContainerIO(namespace, logURI, true, in, con, nil)
 		} else {
 			streams := processAttachStreamsOpt(attachStreamOpt)
-			ioCreator = cio.NewCreator(cio.WithStreams(streams.stdIn, streams.stdOut, streams.stdErr))
+			ioCreator = cioutil.NewContainerIO(namespace, logURI, false, streams.stdIn, streams.stdOut, streams.stdErr)
 		}
 
 	} else if flagT && flagD {