containerd · AkihiroSuda · Nov 11, 2022 · Nov 3, 2022 · Nov 3, 2022 · Oct 17, 2022
diff --git a/cmd/nerdctl/login_linux_test.go b/cmd/nerdctl/login_linux_test.go
@@ -115,12 +115,15 @@ func TestLoginWithPlainHttp(t *testing.T) {
 			shouldUseInSecure: true,
 		},
 		{
-			regHost:           "127.0.0.1",
-			regPort:           5000,
-			useRegPort:        true,
-			username:          "admin",
-			password:          "validTestPassword",
-			shouldSuccess:     false,
+			regHost:    "127.0.0.1",
+			regPort:    5000,
+			useRegPort: true,
+			username:   "admin",
+			password:   "validTestPassword",
+			// Following the merging of the below, any localhost/loopback registries will
+			// get automatically downgraded to HTTP so this will still succceed:
+			// https://github.com/containerd/containerd/pull/7393
+			shouldSuccess:     true,
 			registry:          reg5000,
 			shouldUseInSecure: false,
 		},
@@ -145,12 +148,15 @@ func TestLoginWithPlainHttp(t *testing.T) {
 			shouldUseInSecure: true,
 		},
 		{
-			regHost:           "127.0.0.1",
-			regPort:           80,
-			useRegPort:        false,
-			username:          "admin",
-			password:          "validTestPassword",
-			shouldSuccess:     false,
+			regHost:    "127.0.0.1",
+			regPort:    80,
+			useRegPort: false,
+			username:   "admin",
+			password:   "validTestPassword",
+			// Following the merging of the below, any localhost/loopback registries will
+			// get automatically downgraded to HTTP so this will still succceed:
+			// https://github.com/containerd/containerd/pull/7393
+			shouldSuccess:     true,
 			registry:          reg80,
 			shouldUseInSecure: false,
 		},

diff --git a/cmd/nerdctl/logs.go b/cmd/nerdctl/logs.go
@@ -18,20 +18,16 @@ package main
 
 import (
 	"context"
-	"encoding/json"
 	"fmt"
-	"io"
 	"os"
-	"os/exec"
+	"os/signal"
 	"strconv"
-	"time"
+	"syscall"
 
 	"github.com/containerd/containerd"
 	"github.com/containerd/nerdctl/pkg/idutil/containerwalker"
 	"github.com/containerd/nerdctl/pkg/labels"
 	"github.com/containerd/nerdctl/pkg/logging"
-	"github.com/containerd/nerdctl/pkg/logging/jsonfile"
-	timetypes "github.com/docker/docker/api/types/time"
 	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 )
@@ -79,10 +75,17 @@ func logsAction(cmd *cobra.Command, args []string) error {
 	if err != nil {
 		return err
 	}
-	tail, err := cmd.Flags().GetString("tail")
+	tailArg, err := cmd.Flags().GetString("tail")
 	if err != nil {
 		return err
 	}
+	var tail uint
+	if tailArg != "" {
+		tail, err = getTailArgAsUint(tailArg)
+		if err != nil {
+			return err
+		}
+	}
 	timestamps, err := cmd.Flags().GetBool("timestamps")
 	if err != nil {
 		return err
@@ -96,6 +99,10 @@ func logsAction(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
+	stopChannel := make(chan os.Signal, 1)
+	// catch OS signals:
+	signal.Notify(stopChannel, syscall.SIGTERM, syscall.SIGINT)
+
 	walker := &containerwalker.ContainerWalker{
 		Client: client,
 		OnFound: func(ctx context.Context, found containerwalker.Found) error {
@@ -106,83 +113,48 @@ func logsAction(cmd *cobra.Command, args []string) error {
 			if err != nil {
 				return err
 			}
-			logConfigFilePath := logging.LogConfigFilePath(dataStore, l[labels.Namespace], found.Container.ID())
-			var logConfig logging.LogConfig
-			logConfigFileB, err := os.ReadFile(logConfigFilePath)
+			task, err := found.Container.Task(ctx, nil)
 			if err != nil {
 				return err
 			}
-			if err = json.Unmarshal(logConfigFileB, &logConfig); err != nil {
+			status, err := task.Status(ctx)
+			if err != nil {
 				return err
 			}
-			switch logConfig.Driver {
-			// TODO: move these logics to pkg/logging
-			case "json-file":
-				logJSONFilePath := jsonfile.Path(dataStore, ns, found.Container.ID())
-				if _, err := os.Stat(logJSONFilePath); err != nil {
-					return fmt.Errorf("failed to open %q, container is not created with `nerdctl run -d`?: %w", logJSONFilePath, err)
-				}
-				task, err := found.Container.Task(ctx, nil)
-				if err != nil {
-					return err
-				}
-				status, err := task.Status(ctx)
-				if err != nil {
-					return err
-				}
-				if status.Status != containerd.Running {
-					follow = false
-				}
+			if status.Status != containerd.Running {
+				follow = false
+			}
 
-				reader, execCmd, err := newTailReader(ctx, logJSONFilePath, follow, tail)
+			if follow {
+				waitCh, err := task.Wait(ctx)
 				if err != nil {
-					return err
+					return fmt.Errorf("failed to get wait channel for task %#v: %s", task, err)
 				}
+
+				// Setup goroutine to send stop event if container task finishes:
 				go func() {
-					if follow {
-						if waitCh, err := task.Wait(ctx); err == nil {
-							<-waitCh
-						}
-						execCmd.Process.Kill()
-					}
+					<-waitCh
+					logrus.Debugf("container task has finished, sending kill signal to log viewer")
+					stopChannel <- os.Interrupt
 				}()
-				return jsonfile.Decode(os.Stdout, os.Stderr, reader, timestamps, since, until)
-			case "journald":
-				shortID := found.Container.ID()[:12]
-				var journalctlArgs = []string{fmt.Sprintf("SYSLOG_IDENTIFIER=%s", shortID), "--output=cat"}
-				if follow {
-					journalctlArgs = append(journalctlArgs, "-f")
-				}
-				if since != "" {
-					// using GetTimestamp from moby to keep time format consistency
-					ts, err := timetypes.GetTimestamp(since, time.Now())
-					if err != nil {
-						return fmt.Errorf("invalid value for \"since\": %w", err)
-					}
-					date, err := prepareJournalCtlDate(ts)
-					if err != nil {
-						return err
-					}
-					journalctlArgs = append(journalctlArgs, "--since", date)
-				}
-				if timestamps {
-					logrus.Warnf("unsupported timestamps option for jounrald driver")
-				}
-				if until != "" {
-					// using GetTimestamp from moby to keep time format consistency
-					ts, err := timetypes.GetTimestamp(until, time.Now())
-					if err != nil {
-						return fmt.Errorf("invalid value for \"until\": %w", err)
-					}
-					date, err := prepareJournalCtlDate(ts)
-					if err != nil {
-						return err
-					}
-					journalctlArgs = append(journalctlArgs, "--until", date)
-				}
-				return logging.FetchLogs(journalctlArgs)
 			}
-			return nil
+
+			logViewOpts := logging.LogViewOptions{
+				ContainerID:       found.Container.ID(),
+				Namespace:         l[labels.Namespace],
+				DatastoreRootPath: dataStore,
+				Follow:            follow,
+				Timestamps:        timestamps,
+				Tail:              tail,
+				Since:             since,
+				Until:             until,
+			}
+			logViewer, err := logging.InitContainerLogViewer(logViewOpts, stopChannel)
+			if err != nil {
+				return err
+			}
+
+			return logViewer.PrintLogsTo(os.Stdout, os.Stderr)
 		},
 	}
 	req := args[0]
@@ -200,43 +172,18 @@ func logsShellComplete(cmd *cobra.Command, args []string, toComplete string) ([]
 	return shellCompleteContainerNames(cmd, nil)
 }
 
-func newTailReader(ctx context.Context, filePath string, follow bool, tail string) (io.Reader, *exec.Cmd, error) {
-	var args []string
-
-	if tail != "" {
-		args = append(args, "-n")
-		if tail == "all" {
-			args = append(args, "+0")
-		} else {
-			args = append(args, tail)
-		}
+// Attempts to parse the argument given to `-n/--tail` as a uint.
+func getTailArgAsUint(arg string) (uint, error) {
+	if arg == "all" {
+		return 0, nil
 	} else {
-		args = append(args, "-n")
-		args = append(args, "+0")
-	}
-
-	if follow {
-		args = append(args, "-f")
-	}
-	args = append(args, filePath)
-	cmd := exec.CommandContext(ctx, "tail", args...)
-	cmd.Stderr = os.Stderr
-	r, err := cmd.StdoutPipe()
-	if err != nil {
-		return nil, nil, err
-	}
-	if err := cmd.Start(); err != nil {
-		return nil, nil, err
-	}
-	return r, cmd, nil
-}
-
-func prepareJournalCtlDate(t string) (string, error) {
-	i, err := strconv.ParseInt(t, 10, 64)
-	if err != nil {
-		return "", err
+		num, err := strconv.Atoi(arg)
+		if err != nil {
+			return 0, fmt.Errorf("failed to parse `-n/--tail` argument %q: %s", arg, err)
+		}
+		if num < 0 {
+			return 0, fmt.Errorf("`-n/--tail` argument must be positive, got: %d", num)
+		}
+		return uint(num), nil
 	}
-	tm := time.Unix(i, 0)
-	s := tm.Format("2006-01-02 15:04:05")
-	return s, nil
 }
diff --git a/cmd/nerdctl/logs_test.go b/cmd/nerdctl/logs_test.go
@@ -18,7 +18,7 @@ package main
 
 import (
 	"fmt"
-	"runtime"
+	"os/exec"
 	"strings"
 	"testing"
 	"time"
@@ -28,9 +28,6 @@ import (
 
 func TestLogs(t *testing.T) {
 	t.Parallel()
-	if runtime.GOOS == "windows" {
-		t.Skip("`nerdctl logs` is not implemented on Windows (why?)")
-	}
 	base := testutil.NewBase(t)
 	containerName := testutil.Identifier(t)
 	const expected = `foo
@@ -50,6 +47,7 @@ bar`
 
 	//test tail flag
 	base.Cmd("logs", "-n", "all", containerName).AssertOutContains(expected)
+
 	base.Cmd("logs", "-n", "1", containerName).AssertOutWithFunc(func(stdout string) error {
 		if !(stdout == "bar\n" || stdout == "") {
 			return fmt.Errorf("expected %q or %q, got %q", "bar", "", stdout)
@@ -66,11 +64,23 @@ bar`
 	base.Cmd("rm", "-f", containerName).AssertOK()
 }
 
+// Tests whether `nerdctl logs` properly separates stdout/stderr output
+// streams for containers using the jsonfile logging driver:
+func TestLogsOutStreamsSeparated(t *testing.T) {
+	t.Parallel()
+	base := testutil.NewBase(t)
+	containerName := testutil.Identifier(t)
+
+	defer base.Cmd("rm", containerName).Run()
+	base.Cmd("run", "-d", "--name", containerName, testutil.CommonImage,
+		"sh", "-euc", "echo stdout1; echo stderr1 >&2; echo stdout2; echo stderr2 >&2").AssertOK()
+	time.Sleep(3 * time.Second)
+
+	base.Cmd("logs", containerName).AssertOutStreamsExactly("stdout1\nstdout2\n", "stderr1\nstderr2\n")
+}
+
 func TestLogsWithInheritedFlags(t *testing.T) {
 	t.Parallel()
-	if runtime.GOOS == "windows" {
-		t.Skip("`nerdctl logs` is not implemented on Windows (why?)")
-	}
 	base := testutil.NewBase(t)
 	for k, v := range base.Args {
 		if strings.HasPrefix(v, "--namespace=") {
@@ -94,8 +104,9 @@ func TestLogsWithInheritedFlags(t *testing.T) {
 
 func TestLogsOfJournaldDriver(t *testing.T) {
 	t.Parallel()
-	if runtime.GOOS == "windows" {
-		t.Skip("`nerdctl logs` is not implemented on Windows (why?)")
+	_, err := exec.LookPath("journalctl")
+	if err != nil {
+		t.Skipf("`journalctl` executable is required for this test: %s", err)
 	}
 	base := testutil.NewBase(t)
 	containerName := testutil.Identifier(t)
@@ -124,9 +135,6 @@ func TestLogsOfJournaldDriver(t *testing.T) {
 
 func TestLogsWithFailingContainer(t *testing.T) {
 	t.Parallel()
-	if runtime.GOOS == "windows" {
-		t.Skip("`nerdctl logs` is not implemented on Windows (why?)")
-	}
 	base := testutil.NewBase(t)
 	containerName := testutil.Identifier(t)
 	defer base.Cmd("rm", containerName).Run()

diff --git a/cmd/nerdctl/run.go b/cmd/nerdctl/run.go
@@ -893,9 +893,6 @@ func generateLogURI(dataStore string) (*url.URL, error) {
 	args := map[string]string{
 		logging.MagicArgv1: dataStore,
 	}
-	if runtime.GOOS == "windows" {
-		return nil, nil
-	}
 
 	return cio.LogURIGenerator("binary", selfExe, args)
 }