From 55f2f58145e9871c299456cff8285a6d2595da86 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Wed, 14 Mar 2018 15:14:49 -0400 Subject: Add StartAndAttach() API endpoint for containers This solves our prior problems with attach races by ensuring the order is correct. Also contains substantial cleanups to the attach code. Signed-off-by: Matthew Heon Closes: #482 Approved by: baude --- libpod/container_api.go | 135 +++++++++++++++++++------------------------ libpod/container_attach.go | 58 +++++++++++++++++-- libpod/container_internal.go | 122 +++++++++++++++++++++++--------------- libpod/pod.go | 9 +-- 4 files changed, 189 insertions(+), 135 deletions(-) (limited to 'libpod') diff --git a/libpod/container_api.go b/libpod/container_api.go index 499cf826f..d3afe82b7 100644 --- a/libpod/container_api.go +++ b/libpod/container_api.go @@ -9,14 +9,11 @@ import ( "github.com/containers/storage" "github.com/docker/docker/daemon/caps" "github.com/docker/docker/pkg/stringid" - "github.com/docker/docker/pkg/term" "github.com/pkg/errors" "github.com/projectatomic/libpod/libpod/driver" "github.com/projectatomic/libpod/pkg/inspect" "github.com/sirupsen/logrus" - "golang.org/x/crypto/ssh/terminal" "k8s.io/apimachinery/pkg/util/wait" - "k8s.io/client-go/tools/remotecommand" ) // Init creates a container in the OCI runtime @@ -34,27 +31,13 @@ func (c *Container) Init() (err error) { return errors.Wrapf(ErrCtrExists, "container %s has already been created in runtime", c.ID()) } - if err := c.mountStorage(); err != nil { + if err := c.prepare(); err != nil { return err } defer func() { if err != nil { - if err2 := c.cleanupStorage(); err2 != nil { - logrus.Errorf("Error cleaning up storage for container %s: %v", c.ID(), err2) - } - } - }() - - // Make a network namespace for the container - if c.config.CreateNetNS && c.state.NetNS == nil { - if err := c.runtime.createNetNS(c); err != nil { - return err - } - } - defer func() { - if err != nil { - if err2 := c.runtime.teardownNetNS(c); err2 != nil { - logrus.Errorf("Error tearing down network namespace for container %s: %v", c.ID(), err2) + if err2 := c.cleanup(); err2 != nil { + logrus.Errorf("error cleaning up container %s: %v", c.ID(), err2) } } }() @@ -81,61 +64,86 @@ func (c *Container) Start() (err error) { return errors.Wrapf(ErrCtrStateInvalid, "container %s must be in Created or Stopped state to be started", c.ID()) } - // Mount storage for the container if necessary - if err := c.mountStorage(); err != nil { + if err := c.prepare(); err != nil { return err } defer func() { if err != nil { - if err2 := c.cleanupStorage(); err2 != nil { - logrus.Errorf("Error cleaning up storage for container %s: %v", c.ID(), err2) + if err2 := c.cleanup(); err2 != nil { + logrus.Errorf("error cleaning up container %s: %v", c.ID(), err2) } } }() - // Create a network namespace if necessary - if c.config.CreateNetNS && c.state.NetNS == nil { - if err := c.runtime.createNetNS(c); err != nil { + // Reinitialize the container if we need to + if c.state.State == ContainerStateStopped { + if err := c.reinit(); err != nil { return err } } + + // Start the container + return c.start() +} + +// StartAndAttach starts a container and attaches to it +// StartAndAttach can start created or stopped containers +// Stopped containers will be deleted and re-created in runc, undergoing a fresh +// Init() +// If successful, an error channel will be returned containing the result of the +// attach call. +// The channel will be closed automatically after the result of attach has been +// sent +func (c *Container) StartAndAttach(noStdin bool, keys string) (attachResChan <-chan error, err error) { + if !c.locked { + c.lock.Lock() + defer c.lock.Unlock() + + if err := c.syncContainer(); err != nil { + return nil, err + } + } + + // Container must be created or stopped to be started + if !(c.state.State == ContainerStateCreated || c.state.State == ContainerStateStopped) { + return nil, errors.Wrapf(ErrCtrStateInvalid, "container %s must be in Created or Stopped state to be started", c.ID()) + } + + if err := c.prepare(); err != nil { + return nil, err + } defer func() { if err != nil { - if err2 := c.runtime.teardownNetNS(c); err2 != nil { - logrus.Errorf("Error tearing down network namespace for container %s: %v", c.ID(), err2) + if err2 := c.cleanup(); err2 != nil { + logrus.Errorf("error cleaning up container %s: %v", c.ID(), err2) } } }() // Reinitialize the container if we need to if c.state.State == ContainerStateStopped { - // If necessary, delete attach and ctl files - if err := c.removeConmonFiles(); err != nil { - return err + if err := c.reinit(); err != nil { + return nil, err } + } - // Delete the container in the runtime - if err := c.runtime.ociRuntime.deleteContainer(c); err != nil { - return errors.Wrapf(err, "error removing container %s from runtime", c.ID()) - } + attachChan := make(chan error) - // Our state is now Configured, as we've removed ourself from - // the runtime - // Set and save now to make sure that, if the init() below fails - // we still have a valid state - c.state.State = ContainerStateConfigured - if err := c.save(); err != nil { - return err + // Attach to the container before starting it + go func() { + if err := c.attach(noStdin, keys); err != nil { + attachChan <- err } + close(attachChan) + }() - // Reinitialize the container - if err := c.init(); err != nil { - return err - } + // Start the container + if err := c.start(); err != nil { + // TODO: interrupt the attach here if we error + return nil, err } - // Start the container - return c.start() + return attachChan, nil } // Stop uses the container's stop signal (or SIGTERM if no signal was specified) @@ -336,7 +344,7 @@ func (c *Container) Exec(tty, privileged bool, env, cmd []string, user string) e // Attach attaches to a container // Returns fully qualified URL of streaming server for the container -func (c *Container) Attach(noStdin bool, keys string, attached chan<- bool) error { +func (c *Container) Attach(noStdin bool, keys string) error { if !c.locked { c.lock.Lock() if err := c.syncContainer(); err != nil { @@ -351,24 +359,7 @@ func (c *Container) Attach(noStdin bool, keys string, attached chan<- bool) erro return errors.Wrapf(ErrCtrStateInvalid, "can only attach to created or running containers") } - // Check the validity of the provided keys first - var err error - detachKeys := []byte{} - if len(keys) > 0 { - detachKeys, err = term.ToBytes(keys) - if err != nil { - return errors.Wrapf(err, "invalid detach keys") - } - } - - resize := make(chan remotecommand.TerminalSize) - if terminal.IsTerminal(int(os.Stdin.Fd())) { - resizeTty(resize) - } else { - defer close(resize) - } - err = c.attachContainerSocket(resize, noStdin, detachKeys, attached) - return err + return c.attach(noStdin, keys) } // Mount mounts a container's filesystem on the host @@ -398,7 +389,6 @@ func (c *Container) Mount(label string) (string, error) { } c.state.Mountpoint = mountPoint c.state.Mounted = true - c.config.MountLabel = mountLabel if err := c.save(); err != nil { return "", err @@ -629,12 +619,7 @@ func (c *Container) Cleanup() error { return errors.Wrapf(ErrCtrStateInvalid, "container %s has active exec sessions, refusing to clean up", c.ID()) } - // Stop the container's network namespace (if it has one) - if err := c.cleanupNetwork(); err != nil { - logrus.Errorf("unable cleanup network for container %s: %q", c.ID(), err) - } - - return c.cleanupStorage() + return c.cleanup() } // Batch starts a batch operation on the given container diff --git a/libpod/container_attach.go b/libpod/container_attach.go index 25e8bcdac..c1579d731 100644 --- a/libpod/container_attach.go +++ b/libpod/container_attach.go @@ -5,8 +5,10 @@ import ( "io" "net" "os" + gosignal "os/signal" "path/filepath" + "github.com/docker/docker/pkg/signal" "github.com/docker/docker/pkg/term" "github.com/pkg/errors" "github.com/projectatomic/libpod/utils" @@ -24,8 +26,59 @@ const ( AttachPipeStderr = 3 ) +// resizeTty handles TTY resizing for Attach() +func resizeTty(resize chan remotecommand.TerminalSize) { + sigchan := make(chan os.Signal, 1) + gosignal.Notify(sigchan, signal.SIGWINCH) + sendUpdate := func() { + winsize, err := term.GetWinsize(os.Stdin.Fd()) + if err != nil { + logrus.Warnf("Could not get terminal size %v", err) + return + } + resize <- remotecommand.TerminalSize{ + Width: winsize.Width, + Height: winsize.Height, + } + } + go func() { + defer close(resize) + // Update the terminal size immediately without waiting + // for a SIGWINCH to get the correct initial size. + sendUpdate() + for range sigchan { + sendUpdate() + } + }() +} + +func (c *Container) attach(noStdin bool, keys string) error { + // Check the validity of the provided keys first + var err error + detachKeys := []byte{} + if len(keys) > 0 { + detachKeys, err = term.ToBytes(keys) + if err != nil { + return errors.Wrapf(err, "invalid detach keys") + } + } + + // TODO: allow resize channel to be passed in for CRI-O use + resize := make(chan remotecommand.TerminalSize) + if terminal.IsTerminal(int(os.Stdin.Fd())) { + resizeTty(resize) + } else { + defer close(resize) + } + + logrus.Debugf("Attaching to container %s", c.ID()) + + return c.attachContainerSocket(resize, noStdin, detachKeys) +} + // attachContainerSocket connects to the container's attach socket and deals with the IO -func (c *Container) attachContainerSocket(resize <-chan remotecommand.TerminalSize, noStdIn bool, detachKeys []byte, attached chan<- bool) error { +// TODO add a channel to allow interruptiong +func (c *Container) attachContainerSocket(resize <-chan remotecommand.TerminalSize, noStdIn bool, detachKeys []byte) error { inputStream := os.Stdin outputStream := os.Stdout errorStream := os.Stderr @@ -78,9 +131,6 @@ func (c *Container) attachContainerSocket(resize <-chan remotecommand.TerminalSi } defer conn.Close() - // signal back that the connection was made - attached <- true - receiveStdoutError := make(chan error) if outputStream != nil || errorStream != nil { go func() { diff --git a/libpod/container_internal.go b/libpod/container_internal.go index 6b375ef6f..4d1bce787 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -6,7 +6,6 @@ import ( "io" "io/ioutil" "os" - gosignal "os/signal" "path/filepath" "strings" "syscall" @@ -17,9 +16,7 @@ import ( "github.com/containers/storage/pkg/chrootarchive" "github.com/docker/docker/pkg/mount" "github.com/docker/docker/pkg/namesgenerator" - "github.com/docker/docker/pkg/signal" "github.com/docker/docker/pkg/stringid" - "github.com/docker/docker/pkg/term" spec "github.com/opencontainers/runtime-spec/specs-go" "github.com/opencontainers/runtime-tools/generate" "github.com/opencontainers/selinux/go-selinux/label" @@ -30,7 +27,6 @@ import ( "github.com/sirupsen/logrus" "github.com/ulule/deepcopier" "golang.org/x/sys/unix" - "k8s.io/client-go/tools/remotecommand" ) const ( @@ -375,6 +371,33 @@ func (c *Container) init() error { return c.save() } +// Reinitialize a container +// Deletes and recreates a container in the runtime +// Should only be done on ContainerStateStopped containers +func (c *Container) reinit() error { + // If necessary, delete attach and ctl files + if err := c.removeConmonFiles(); err != nil { + return err + } + + // Delete the container in the runtime + if err := c.runtime.ociRuntime.deleteContainer(c); err != nil { + return errors.Wrapf(err, "error removing container %s from runtime", c.ID()) + } + + // Our state is now Configured, as we've removed ourself from + // the runtime + // Set and save now to make sure that, if the init() below fails + // we still have a valid state + c.state.State = ContainerStateConfigured + if err := c.save(); err != nil { + return err + } + + // Initialize the container again + return c.init() +} + // Initialize (if necessary) and start a container // Performs all necessary steps to start a container that is not running // Does not lock or check validity @@ -393,28 +416,13 @@ func (c *Container) initAndStart() (err error) { return errors.Wrapf(ErrCtrStateInvalid, "cannot start paused container %s", c.ID()) } - // Mount if necessary - if err := c.mountStorage(); err != nil { + if err := c.prepare(); err != nil { return err } defer func() { if err != nil { - if err2 := c.cleanupStorage(); err2 != nil { - logrus.Errorf("Error cleaning up storage for container %s: %v", c.ID(), err2) - } - } - }() - - // Create a network namespace if necessary - if c.config.CreateNetNS && c.state.NetNS == nil { - if err := c.runtime.createNetNS(c); err != nil { - return err - } - } - defer func() { - if err != nil { - if err2 := c.runtime.teardownNetNS(c); err2 != nil { - logrus.Errorf("Error tearing down network namespace for container %s: %v", c.ID(), err2) + if err2 := c.cleanup(); err2 != nil { + logrus.Errorf("error cleaning up container %s: %v", c.ID(), err2) } } }() @@ -482,32 +490,6 @@ func (c *Container) stop(timeout uint) error { return c.cleanupStorage() } -// resizeTty handles TTY resizing for Attach() -func resizeTty(resize chan remotecommand.TerminalSize) { - sigchan := make(chan os.Signal, 1) - gosignal.Notify(sigchan, signal.SIGWINCH) - sendUpdate := func() { - winsize, err := term.GetWinsize(os.Stdin.Fd()) - if err != nil { - logrus.Warnf("Could not get terminal size %v", err) - return - } - resize <- remotecommand.TerminalSize{ - Width: winsize.Width, - Height: winsize.Height, - } - } - go func() { - defer close(resize) - // Update the terminal size immediately without waiting - // for a SIGWINCH to get the correct initial size. - sendUpdate() - for range sigchan { - sendUpdate() - } - }() -} - // mountStorage sets up the container's root filesystem // It mounts the image and any other requested mounts // TODO: Add ability to override mount label so we can use this for Mount() too @@ -554,6 +536,29 @@ func (c *Container) mountStorage() (err error) { return c.save() } +// prepare mounts the container and sets up other required resources like net +// namespaces +func (c *Container) prepare() (err error) { + // Mount storage if not mounted + if err := c.mountStorage(); err != nil { + return err + } + + // Set up network namespace if not already set up + if c.config.CreateNetNS && c.state.NetNS == nil { + if err := c.runtime.createNetNS(c); err != nil { + // Tear down storage before exiting to make sure we + // don't leak mounts + if err2 := c.cleanupStorage(); err2 != nil { + logrus.Errorf("Error cleaning up storage for container %s: %v", c.ID(), err2) + } + return err + } + } + + return nil +} + // cleanupNetwork unmounts and cleans up the container's network func (c *Container) cleanupNetwork() error { // Stop the container's network namespace (if it has one) @@ -593,6 +598,27 @@ func (c *Container) cleanupStorage() error { return c.save() } +// Unmount the a container and free its resources +func (c *Container) cleanup() error { + var lastError error + + // Clean up network namespace, if present + if err := c.cleanupNetwork(); err != nil { + lastError = nil + } + + // Unmount storage + if err := c.cleanupStorage(); err != nil { + if lastError != nil { + logrus.Errorf("Error unmounting container %s storage: %v", c.ID(), err) + } else { + lastError = err + } + } + + return lastError +} + // Make standard bind mounts to include in the container func (c *Container) makeBindMounts() error { if c.state.BindMounts == nil { diff --git a/libpod/pod.go b/libpod/pod.go index bd02efdae..16b590a1c 100644 --- a/libpod/pod.go +++ b/libpod/pod.go @@ -240,14 +240,7 @@ func (p *Pod) Stop(cleanup bool) (map[string]error, error) { } if cleanup { - // Clean up storage to ensure we don't leave dangling mounts - if err := ctr.cleanupStorage(); err != nil { - ctrErrors[ctr.ID()] = err - continue - } - - // Clean up network namespace - if err := ctr.cleanupNetwork(); err != nil { + if err := ctr.cleanup(); err != nil { ctrErrors[ctr.ID()] = err } } -- cgit v1.2.3-54-g00ecf