Merge pull request #6560 from mheon/fix_exec_logdriver

Do not share container log driver for exec

12 files changed, 45 insertions, 20 deletions

diff --git a/contrib/cirrus/setup_environment.sh b/contrib/cirrus/setup_environment.sh
index 323e7c35b..ea2c7d8e0 100755
--- a/contrib/cirrus/setup_environment.sh
+++ b/contrib/cirrus/setup_environment.sh
@@ -50,6 +50,7 @@ case "${OS_RELEASE_ID}" in
 	    if [[ "$OS_RELEASE_VER" == "20" ]]; then
 		    apt-get install -y python-is-python3
 	    fi
+	    apt-get upgrade -y conmon
         ;;
     fedora)
         # All SELinux distros need this for systemd-in-a-container
diff --git a/docs/source/markdown/podman-create.1.md b/docs/source/markdown/podman-create.1.md
index 03ac8642f..3ec91a3ad 100644
--- a/docs/source/markdown/podman-create.1.md
+++ b/docs/source/markdown/podman-create.1.md
@@ -419,7 +419,7 @@ Not implemented
 
 **--log-driver**="*k8s-file*"
 
-Logging driver for the container.  Currently available options are *k8s-file* and *journald*, with *json-file* aliased to *k8s-file* for scripting compatibility.
+Logging driver for the container.  Currently available options are *k8s-file*, *journald*, and *none*, with *json-file* aliased to *k8s-file* for scripting compatibility.
 
 **--log-opt**=*path*
 
diff --git a/docs/source/markdown/podman-run.1.md b/docs/source/markdown/podman-run.1.md
index 539e62819..7e91a06a3 100644
--- a/docs/source/markdown/podman-run.1.md
+++ b/docs/source/markdown/podman-run.1.md
@@ -432,7 +432,7 @@ Not implemented.
 
 **--log-driver**="*driver*"
 
-Logging driver for the container. Currently available options are **k8s-file** and **journald**, with **json-file** aliased to **k8s-file** for scripting compatibility.
+Logging driver for the container. Currently available options are **k8s-file**, **journald**, and **none**, with **json-file** aliased to **k8s-file** for scripting compatibility.
 
 **--log-opt**=*name*=*value*
 
diff --git a/libpod/container_log.go b/libpod/container_log.go
index c3a84d048..071882bc2 100644
--- a/libpod/container_log.go
+++ b/libpod/container_log.go
@@ -21,12 +21,21 @@ func (r *Runtime) Log(containers []*Container, options *logs.LogOptions, logChan
 
 // ReadLog reads a containers log based on the input options and returns loglines over a channel.
 func (c *Container) ReadLog(options *logs.LogOptions, logChannel chan *logs.LogLine) error {
-	// TODO Skip sending logs until journald logs can be read
-	// TODO make this not a magic string
-	if c.LogDriver() == define.JournaldLogging {
+	switch c.LogDriver() {
+	case define.NoLogging:
+		return errors.Wrapf(define.ErrNoLogs, "this container is using the 'none' log driver, cannot read logs")
+	case define.JournaldLogging:
+		// TODO Skip sending logs until journald logs can be read
 		return c.readFromJournal(options, logChannel)
+	case define.JSONLogging:
+		// TODO provide a separate implementation of this when Conmon
+		// has support.
+		fallthrough
+	case define.KubernetesLogging, "":
+		return c.readFromLogFile(options, logChannel)
+	default:
+		return errors.Wrapf(define.ErrInternal, "unrecognized log driver %q, cannot read logs", c.LogDriver())
 	}
-	return c.readFromLogFile(options, logChannel)
 }
 
 func (c *Container) readFromLogFile(options *logs.LogOptions, logChannel chan *logs.LogLine) error {
diff --git a/libpod/define/config.go b/libpod/define/config.go
index 5ca4da4af..900a363d8 100644
--- a/libpod/define/config.go
+++ b/libpod/define/config.go
@@ -72,3 +72,6 @@ const KubernetesLogging = "k8s-file"
 
 // JSONLogging is the string conmon expects when specifying to use the json logging format
 const JSONLogging = "json-file"
+
+// NoLogging is the string conmon expects when specifying to use no log driver whatsoever
+const NoLogging = "none"
diff --git a/libpod/define/errors.go b/libpod/define/errors.go
index 083553b7e..e0c9811fe 100644
--- a/libpod/define/errors.go
+++ b/libpod/define/errors.go
@@ -79,6 +79,9 @@ var (
 	// ErrNoCgroups indicates that the container does not have its own
 	// CGroup.
 	ErrNoCgroups = errors.New("this container does not have a cgroup")
+	// ErrNoLogs indicates that this container is not creating a log so log
+	// operations cannot be performed on it
+	ErrNoLogs = errors.New("this container is not logging output")
 
 	// ErrRootless indicates that the given command cannot but run without
 	// root.
diff --git a/libpod/oci_conmon_exec_linux.go b/libpod/oci_conmon_exec_linux.go
index bc39100f8..6be8534d9 100644
--- a/libpod/oci_conmon_exec_linux.go
+++ b/libpod/oci_conmon_exec_linux.go
@@ -392,7 +392,7 @@ func (r *ConmonOCIRuntime) startExec(c *Container, sessionID string, options *Ex
 		return nil, nil, err
 	}
 
-	args := r.sharedConmonArgs(c, sessionID, c.execBundlePath(sessionID), c.execPidPath(sessionID), c.execLogPath(sessionID), c.execExitFileDir(sessionID), ociLog, "")
+	args := r.sharedConmonArgs(c, sessionID, c.execBundlePath(sessionID), c.execPidPath(sessionID), c.execLogPath(sessionID), c.execExitFileDir(sessionID), ociLog, define.NoLogging, "")
 
 	if options.PreserveFDs > 0 {
 		args = append(args, formatRuntimeOpts("--preserve-fds", fmt.Sprintf("%d", options.PreserveFDs))...)
diff --git a/libpod/oci_conmon_linux.go b/libpod/oci_conmon_linux.go
index 0921a532b..625a5bf70 100644
--- a/libpod/oci_conmon_linux.go
+++ b/libpod/oci_conmon_linux.go
@@ -881,7 +881,7 @@ func (r *ConmonOCIRuntime) createOCIContainer(ctr *Container, restoreOptions *Co
 		return err
 	}
 
-	args := r.sharedConmonArgs(ctr, ctr.ID(), ctr.bundlePath(), filepath.Join(ctr.state.RunDir, "pidfile"), ctr.LogPath(), r.exitsDir, ociLog, logTag)
+	args := r.sharedConmonArgs(ctr, ctr.ID(), ctr.bundlePath(), filepath.Join(ctr.state.RunDir, "pidfile"), ctr.LogPath(), r.exitsDir, ociLog, ctr.LogDriver(), logTag)
 
 	if ctr.config.Spec.Process.Terminal {
 		args = append(args, "-t")
@@ -1137,7 +1137,7 @@ func (r *ConmonOCIRuntime) configureConmonEnv(runtimeDir string) ([]string, []*o
 }
 
 // sharedConmonArgs takes common arguments for exec and create/restore and formats them for the conmon CLI
-func (r *ConmonOCIRuntime) sharedConmonArgs(ctr *Container, cuuid, bundlePath, pidPath, logPath, exitDir, ociLogPath, logTag string) []string {
+func (r *ConmonOCIRuntime) sharedConmonArgs(ctr *Container, cuuid, bundlePath, pidPath, logPath, exitDir, ociLogPath, logDriver, logTag string) []string {
 	// set the conmon API version to be able to use the correct sync struct keys
 	args := []string{
 		"--api-version", "1",
@@ -1155,12 +1155,14 @@ func (r *ConmonOCIRuntime) sharedConmonArgs(ctr *Container, cuuid, bundlePath, p
 		args = append(args, "-s")
 	}
 
-	var logDriver string
-	switch ctr.LogDriver() {
+	var logDriverArg string
+	switch logDriver {
 	case define.JournaldLogging:
-		logDriver = define.JournaldLogging
+		logDriverArg = define.JournaldLogging
 	case define.JSONLogging:
 		fallthrough
+	case define.NoLogging:
+		logDriverArg = define.NoLogging
 	default: //nolint-stylecheck
 		// No case here should happen except JSONLogging, but keep this here in case the options are extended
 		logrus.Errorf("%s logging specified but not supported. Choosing k8s-file logging instead", ctr.LogDriver())
@@ -1170,10 +1172,10 @@ func (r *ConmonOCIRuntime) sharedConmonArgs(ctr *Container, cuuid, bundlePath, p
 		// since the former case is obscure, and the latter case isn't an error, let's silently fallthrough
 		fallthrough
 	case define.KubernetesLogging:
-		logDriver = fmt.Sprintf("%s:%s", define.KubernetesLogging, logPath)
+		logDriverArg = fmt.Sprintf("%s:%s", define.KubernetesLogging, logPath)
 	}
 
-	args = append(args, "-l", logDriver)
+	args = append(args, "-l", logDriverArg)
 	if r.logSizeMax >= 0 {
 		args = append(args, "--log-size-max", fmt.Sprintf("%v", r.logSizeMax))
 	}
diff --git a/libpod/options.go b/libpod/options.go
index e1c4ddf06..ffc9c1018 100644
--- a/libpod/options.go
+++ b/libpod/options.go
@@ -993,7 +993,7 @@ func WithLogDriver(driver string) CtrCreateOption {
 		switch driver {
 		case "":
 			return errors.Wrapf(define.ErrInvalidArg, "log driver must be set")
-		case define.JournaldLogging, define.KubernetesLogging, define.JSONLogging:
+		case define.JournaldLogging, define.KubernetesLogging, define.JSONLogging, define.NoLogging:
 			break
 		default:
 			return errors.Wrapf(define.ErrInvalidArg, "invalid log driver")
diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go
index f0beb0941..0431861b5 100644
--- a/libpod/runtime_ctr.go
+++ b/libpod/runtime_ctr.go
@@ -321,7 +321,7 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (_ *Contai
 		ctrNamedVolumes = append(ctrNamedVolumes, newVol)
 	}
 
-	if ctr.config.LogPath == "" && ctr.config.LogDriver != define.JournaldLogging {
+	if ctr.config.LogPath == "" && ctr.config.LogDriver != define.JournaldLogging && ctr.config.LogDriver != define.NoLogging {
 		ctr.config.LogPath = filepath.Join(ctr.config.StaticDir, "ctr.log")
 	}
 
diff --git a/test/e2e/exec_test.go b/test/e2e/exec_test.go
index b152da9e6..f44d428d6 100644
--- a/test/e2e/exec_test.go
+++ b/test/e2e/exec_test.go
@@ -25,10 +25,6 @@ var _ = Describe("Podman exec", func() {
 		podmanTest = PodmanTestCreate(tempdir)
 		podmanTest.Setup()
 		podmanTest.SeedImages()
-		// HACK: Remove this once we get Conmon 2.0.17 on Ubuntu
-		if podmanTest.Host.Distribution == "ubuntu" {
-			Skip("Unable to perform test on Ubuntu distributions due to too-old Conmon (need 2.0.17)")
-		}
 	})
 
 	AfterEach(func() {
diff --git a/test/e2e/logs_test.go b/test/e2e/logs_test.go
index c12ae9c86..a4a59acb2 100644
--- a/test/e2e/logs_test.go
+++ b/test/e2e/logs_test.go
@@ -300,4 +300,15 @@ var _ = Describe("Podman logs", func() {
 			Expect(inspect.ErrorToString()).To(ContainSubstring("no such container"))
 		}
 	})
+
+	It("podman logs with log-driver=none errors", func() {
+		ctrName := "logsctr"
+		logc := podmanTest.Podman([]string{"run", "--name", ctrName, "-d", "--log-driver", "none", ALPINE, "top"})
+		logc.WaitWithDefaultTimeout()
+		Expect(logc).To(Exit(0))
+
+		logs := podmanTest.Podman([]string{"logs", "-f", ctrName})
+		logs.WaitWithDefaultTimeout()
+		Expect(logs).To(Not(Exit(0)))
+	})
 })