Merge pull request #3127 from mheon/fix_start_race

Ensure that start() in StartAndAttach() is locked

7 files changed, 40 insertions, 36 deletions

diff --git a/.cirrus.yml b/.cirrus.yml
index 5beed5c40..d26c1ec11 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -298,12 +298,14 @@ testing_task:
     setup_environment_script: '$SCRIPT_BASE/setup_environment.sh |& ${TIMESTAMP}'
     unit_test_script: '$SCRIPT_BASE/unit_test.sh |& ${TIMESTAMP}'
     integration_test_script: '$SCRIPT_BASE/integration_test.sh |& ${TIMESTAMP}'
+    ginkgo_node_logs_script: 'cat $CIRRUS_WORKING_DIR/test/e2e/ginkgo-node-*.log || echo "Ginkgo node logs not found"'
     audit_log_script: 'cat /var/log/audit/audit.log || cat /var/log/kern.log'
     journalctl_b_script: 'journalctl -b'
 
     on_failure:
         failed_master_script: '$CIRRUS_WORKING_DIR/$SCRIPT_BASE/notice_master_failure.sh'
         # Job has already failed, don't fail again and miss collecting data
+        failed_ginkgo_node_logs_script: 'cat $CIRRUS_WORKING_DIR/test/e2e/ginkgo-node-*.log || echo "Ginkgo node logs not found"'
         failed_audit_log_script: 'cat /var/log/audit/audit.log || cat /var/log/kern.log || echo "Uh oh, cat audit.log failed"'
         failed_journalctl_b_script: 'journalctl -b || echo "Uh oh, journalctl -b failed"'
 
diff --git a/Makefile b/Makefile
index d464b3f5a..10de8ad4e 100644
--- a/Makefile
+++ b/Makefile
@@ -207,7 +207,7 @@ localunit: test/goecho/goecho varlink_generate
 	./contrib/cirrus/lib.sh.t
 
 ginkgo:
-	ginkgo -v -tags "$(BUILDTAGS)" $(GINKGOTIMEOUT) -cover -flakeAttempts 3 -progress -trace -noColor -nodes 3 test/e2e/.
+	ginkgo -v -tags "$(BUILDTAGS)" $(GINKGOTIMEOUT) -cover -flakeAttempts 3 -progress -trace -noColor -nodes 3 -debug test/e2e/.
 
 ginkgo-remote:
 	ginkgo -v -tags "$(BUILDTAGS) remoteclient" $(GINKGOTIMEOUT) -cover -flakeAttempts 3 -progress -trace -noColor test/e2e/.
diff --git a/README.md b/README.md
index a5545bb36..3abe27fcf 100644
--- a/README.md
+++ b/README.md
@@ -10,7 +10,7 @@ popularized by Kubernetes.  Libpod also contains the Pod Manager tool `(Podman)`
 
 ## Overview and scope
 
-At a high level, the scope of libpod and podman is the following:
+At a high level, the scope of libpod and Podman is the following:
 
 * Support multiple image formats including the OCI and Docker image formats.
 * Support for multiple means to download images including trust & image verification.
diff --git a/libpod/container_api.go b/libpod/container_api.go
index 5bb610aab..06a31da11 100644
--- a/libpod/container_api.go
+++ b/libpod/container_api.go
@@ -7,6 +7,7 @@ import (
 	"io/ioutil"
 	"os"
 	"strconv"
+	"sync"
 	"time"
 
 	"github.com/containers/libpod/libpod/driver"
@@ -119,13 +120,20 @@ func (c *Container) StartAndAttach(ctx context.Context, streams *AttachStreams,
 
 	attachChan := make(chan error)
 
+	// We need to ensure that we don't return until start() fired in attach.
+	// Use a WaitGroup to sync this.
+	wg := new(sync.WaitGroup)
+	wg.Add(1)
+
 	// Attach to the container before starting it
 	go func() {
-		if err := c.attach(streams, keys, resize, true); err != nil {
+		if err := c.attach(streams, keys, resize, true, wg); err != nil {
 			attachChan <- err
 		}
 		close(attachChan)
 	}()
+
+	wg.Wait()
 	c.newContainerEvent(events.Attach)
 	return attachChan, nil
 }
@@ -398,7 +406,7 @@ func (c *Container) Attach(streams *AttachStreams, keys string, resize <-chan re
 		return errors.Wrapf(ErrCtrStateInvalid, "can only attach to created or running containers")
 	}
 	defer c.newContainerEvent(events.Attach)
-	return c.attach(streams, keys, resize, false)
+	return c.attach(streams, keys, resize, false, nil)
 }
 
 // Mount mounts a container's filesystem on the host
diff --git a/libpod/container_attach_linux.go b/libpod/container_attach_linux.go
index 3ff6ddc76..7e9b7697b 100644
--- a/libpod/container_attach_linux.go
+++ b/libpod/container_attach_linux.go
@@ -8,6 +8,7 @@ import (
 	"net"
 	"os"
 	"path/filepath"
+	"sync"
 
 	"github.com/containers/libpod/pkg/kubeutils"
 	"github.com/containers/libpod/utils"
@@ -31,7 +32,7 @@ const (
 
 // Attach to the given container
 // Does not check if state is appropriate
-func (c *Container) attach(streams *AttachStreams, keys string, resize <-chan remotecommand.TerminalSize, startContainer bool) error {
+func (c *Container) attach(streams *AttachStreams, keys string, resize <-chan remotecommand.TerminalSize, startContainer bool, wg *sync.WaitGroup) error {
 	if !streams.AttachOutput && !streams.AttachError && !streams.AttachInput {
 		return errors.Wrapf(ErrInvalidArg, "must provide at least one stream to attach to")
 	}
@@ -48,12 +49,17 @@ func (c *Container) attach(streams *AttachStreams, keys string, resize <-chan re
 
 	logrus.Debugf("Attaching to container %s", c.ID())
 
-	return c.attachContainerSocket(resize, detachKeys, streams, startContainer)
+	return c.attachContainerSocket(resize, detachKeys, streams, startContainer, wg)
 }
 
-// attachContainerSocket connects to the container's attach socket and deals with the IO
+// attachContainerSocket connects to the container's attach socket and deals with the IO.
+// wg is only required if startContainer is true
 // TODO add a channel to allow interrupting
-func (c *Container) attachContainerSocket(resize <-chan remotecommand.TerminalSize, detachKeys []byte, streams *AttachStreams, startContainer bool) error {
+func (c *Container) attachContainerSocket(resize <-chan remotecommand.TerminalSize, detachKeys []byte, streams *AttachStreams, startContainer bool, wg *sync.WaitGroup) error {
+	if startContainer && wg == nil {
+		return errors.Wrapf(ErrInternal, "wait group not passed when startContainer set")
+	}
+
 	kubeutils.HandleResizing(resize, func(size remotecommand.TerminalSize) {
 		controlPath := filepath.Join(c.bundlePath(), "ctl")
 		controlFile, err := os.OpenFile(controlPath, unix.O_WRONLY, 0)
@@ -84,10 +90,13 @@ func (c *Container) attachContainerSocket(resize <-chan remotecommand.TerminalSi
 	}
 	defer conn.Close()
 
+	// If starting was requested, start the container and notify when that's
+	// done.
 	if startContainer {
 		if err := c.start(); err != nil {
 			return err
 		}
+		wg.Done()
 	}
 
 	receiveStdoutError := make(chan error)
diff --git a/libpod/container_attach_unsupported.go b/libpod/container_attach_unsupported.go
index 068652b29..9e8badeaf 100644
--- a/libpod/container_attach_unsupported.go
+++ b/libpod/container_attach_unsupported.go
@@ -3,9 +3,11 @@
 package libpod
 
 import (
+	"sync"
+
 	"k8s.io/client-go/tools/remotecommand"
 )
 
-func (c *Container) attach(streams *AttachStreams, keys string, resize <-chan remotecommand.TerminalSize, startContainer bool) error {
+func (c *Container) attach(streams *AttachStreams, keys string, resize <-chan remotecommand.TerminalSize, startContainer bool, wg *sync.WaitGroup) error {
 	return ErrNotImplemented
 }
diff --git a/test/e2e/cp_test.go b/test/e2e/cp_test.go
index 273668f35..1dfa8f50d 100644
--- a/test/e2e/cp_test.go
+++ b/test/e2e/cp_test.go
@@ -40,15 +40,11 @@ var _ = Describe("Podman cp", func() {
 
 	It("podman cp file", func() {
 		path, err := os.Getwd()
-		if err != nil {
-			os.Exit(1)
-		}
+		Expect(err).To(BeNil())
 		filePath := filepath.Join(path, "cp_test.txt")
 		fromHostToContainer := []byte("copy from host to container")
 		err = ioutil.WriteFile(filePath, fromHostToContainer, 0644)
-		if err != nil {
-			os.Exit(1)
-		}
+		Expect(err).To(BeNil())
 
 		session := podmanTest.Podman([]string{"create", ALPINE, "cat", "foo"})
 		session.WaitWithDefaultTimeout()
@@ -69,15 +65,12 @@ var _ = Describe("Podman cp", func() {
 
 	It("podman cp file to dir", func() {
 		path, err := os.Getwd()
-		if err != nil {
-			os.Exit(1)
-		}
+		Expect(err).To(BeNil())
 		filePath := filepath.Join(path, "cp_test.txt")
 		fromHostToContainer := []byte("copy from host to container directory")
 		err = ioutil.WriteFile(filePath, fromHostToContainer, 0644)
-		if err != nil {
-			os.Exit(1)
-		}
+		Expect(err).To(BeNil())
+
 		session := podmanTest.Podman([]string{"create", ALPINE, "ls", "foodir/"})
 		session.WaitWithDefaultTimeout()
 		Expect(session.ExitCode()).To(Equal(0))
@@ -97,14 +90,10 @@ var _ = Describe("Podman cp", func() {
 
 	It("podman cp dir to dir", func() {
 		path, err := os.Getwd()
-		if err != nil {
-			os.Exit(1)
-		}
+		Expect(err).To(BeNil())
 		testDirPath := filepath.Join(path, "TestDir")
 		err = os.Mkdir(testDirPath, 0777)
-		if err != nil {
-			os.Exit(1)
-		}
+		Expect(err).To(BeNil())
 
 		session := podmanTest.Podman([]string{"create", ALPINE, "ls", "/foodir"})
 		session.WaitWithDefaultTimeout()
@@ -124,19 +113,13 @@ var _ = Describe("Podman cp", func() {
 
 	It("podman cp stdin/stdout", func() {
 		path, err := os.Getwd()
-		if err != nil {
-			os.Exit(1)
-		}
+		Expect(err).To(BeNil())
 		testDirPath := filepath.Join(path, "TestDir")
 		err = os.Mkdir(testDirPath, 0777)
-		if err != nil {
-			os.Exit(1)
-		}
+		Expect(err).To(BeNil())
 		cmd := exec.Command("tar", "-zcvf", "file.tar.gz", testDirPath)
 		_, err = cmd.Output()
-		if err != nil {
-			os.Exit(1)
-		}
+		Expect(err).To(BeNil())
 
 		session := podmanTest.Podman([]string{"create", ALPINE, "ls", "foo"})
 		session.WaitWithDefaultTimeout()