First batch of resolutions to FIXMEs

Most of these are no longer relevant, just drop the comments.

Most notable change: allow `podman kill` on paused containers.
Works just fine when I test it.

Signed-off-by: Matthew Heon <mheon@redhat.com>

7 files changed, 25 insertions, 13 deletions

diff --git a/docs/source/markdown/podman-kill.1.md b/docs/source/markdown/podman-kill.1.md
index 35ca9f74f..a4f80ac81 100644
--- a/docs/source/markdown/podman-kill.1.md
+++ b/docs/source/markdown/podman-kill.1.md
@@ -14,7 +14,7 @@ The main process inside each container specified will be sent SIGKILL, or any si
 ## OPTIONS
 #### **--all**, **-a**
 
-Signal all running containers.  This does not include paused containers.
+Signal all running and paused containers.
 
 #### **--cidfile**
 
diff --git a/libpod/container.go b/libpod/container.go
index 64b4453fb..04a4ae64a 100644
--- a/libpod/container.go
+++ b/libpod/container.go
@@ -1331,8 +1331,7 @@ func (c *Container) getNetworkStatus() map[string]types.StatusBlock {
 		}
 		c.state.NetworkStatus = result
 		_ = c.save()
-		// TODO remove debug for final version
-		logrus.Debugf("converted old network result to new result %v", result)
+
 		return result
 	}
 	return nil
diff --git a/libpod/container_api.go b/libpod/container_api.go
index a6fcf709d..0fab36bdc 100644
--- a/libpod/container_api.go
+++ b/libpod/container_api.go
@@ -202,9 +202,8 @@ func (c *Container) Kill(signal uint) error {
 		}
 	}
 
-	// TODO: Is killing a paused container OK?
 	switch c.state.State {
-	case define.ContainerStateRunning, define.ContainerStateStopping:
+	case define.ContainerStateRunning, define.ContainerStateStopping, define.ContainerStatePaused:
 		// Note that killing containers in "stopping" state is okay.
 		// In that state, the Podman is waiting for the runtime to
 		// stop the container and if that is taking too long, a user
diff --git a/libpod/container_config.go b/libpod/container_config.go
index 3e85ad4d5..ae3bc5865 100644
--- a/libpod/container_config.go
+++ b/libpod/container_config.go
@@ -372,7 +372,6 @@ type ContainerMiscConfig struct {
 	// restart the container. Used only if RestartPolicy is set to
 	// "on-failure".
 	RestartRetries uint `json:"restart_retries,omitempty"`
-	// TODO log options for log drivers
 	// PostConfigureNetNS needed when a user namespace is created by an OCI runtime
 	// if the network namespace is created before the user namespace it will be
 	// owned by the wrong user namespace.
diff --git a/libpod/container_exec.go b/libpod/container_exec.go
index c05e7fd94..1e8fce4da 100644
--- a/libpod/container_exec.go
+++ b/libpod/container_exec.go
@@ -279,8 +279,6 @@ func (c *Container) ExecStart(sessionID string) error {
 
 // ExecStartAndAttach starts and attaches to an exec session in a container.
 // newSize resizes the tty to this size before the process is started, must be nil if the exec session has no tty
-// TODO: Should we include detach keys in the signature to allow override?
-// TODO: How do we handle AttachStdin/AttachStdout/AttachStderr?
 func (c *Container) ExecStartAndAttach(sessionID string, streams *define.AttachStreams, newSize *define.TerminalSize) error {
 	if !c.batched {
 		c.lock.Lock()
diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go
index 298eb1947..e19d75deb 100644
--- a/libpod/container_internal_linux.go
+++ b/libpod/container_internal_linux.go
@@ -1091,7 +1091,6 @@ func (c *Container) addNamespaceContainer(g *generate.Generator, ns LinuxNS, ctr
 		g.AddProcessEnv("HOSTNAME", hostname)
 	}
 
-	// TODO need unlocked version of this for use in pods
 	nsPath, err := nsCtr.NamespacePath(ns)
 	if err != nil {
 		return err
@@ -3230,10 +3229,8 @@ func (c *Container) fixVolumePermissions(v *ContainerNamedVolume) error {
 		return err
 	}
 
-	// TODO: For now, I've disabled chowning volumes owned by non-Podman
-	// drivers. This may be safe, but it's really going to be a case-by-case
-	// thing, I think - safest to leave disabled now and re-enable later if
-	// there is a demand.
+	// Volumes owned by a volume driver are not chowned - we don't want to
+	// mess with a mount not managed by us.
 	if vol.state.NeedsChown && !vol.UsesVolumeDriver() {
 		vol.state.NeedsChown = false
 
diff --git a/test/e2e/kill_test.go b/test/e2e/kill_test.go
index 552a7c15d..2a9a86729 100644
--- a/test/e2e/kill_test.go
+++ b/test/e2e/kill_test.go
@@ -128,6 +128,26 @@ var _ = Describe("Podman kill", func() {
 		Expect(podmanTest.NumberOfContainersRunning()).To(Equal(0))
 	})
 
+	It("podman kill paused container", func() {
+		ctrName := "testctr"
+		session := podmanTest.RunTopContainer(ctrName)
+		session.WaitWithDefaultTimeout()
+		Expect(session).Should(Exit(0))
+
+		pause := podmanTest.Podman([]string{"pause", ctrName})
+		pause.WaitWithDefaultTimeout()
+		Expect(pause).Should(Exit(0))
+
+		kill := podmanTest.Podman([]string{"kill", ctrName})
+		kill.WaitWithDefaultTimeout()
+		Expect(kill).Should(Exit(0))
+
+		inspect := podmanTest.Podman([]string{"inspect", "-f", "{{.State.Status}}", ctrName})
+		inspect.WaitWithDefaultTimeout()
+		Expect(inspect).Should(Exit(0))
+		Expect(inspect.OutputToString()).To(Or(Equal("stopped"), Equal("exited")))
+	})
+
 	It("podman kill --cidfile", func() {
 		tmpDir, err := ioutil.TempDir("", "")
 		Expect(err).To(BeNil())