From f714ee4fb14085c326ee5d56432781a01ffabbb0 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Fri, 19 Oct 2018 13:53:15 -0400 Subject: Actually save changes from post-stop sync After stopping containers, we run updateContainerStatus to sync our state with runc (pick up exit code, for example). Then we proceed to not save this to the database, requiring us to grab it again on the next sync. This should remove the need to read the exit file more than once. Signed-off-by: Matthew Heon --- libpod/container_internal.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'libpod') diff --git a/libpod/container_internal.go b/libpod/container_internal.go index d928c4aed..867d539be 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -679,7 +679,7 @@ func (c *Container) stop(timeout uint) error { } // Container should clean itself up - return nil + return c.save() } // Internal, non-locking function to pause a container -- cgit v1.2.3-54-g00ecf From 140f87c474f5a075d5833335593895491b83dab6 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Fri, 19 Oct 2018 16:02:14 -0400 Subject: EXPERIMENTAL: Do not call out to runc for sync When syncing container state, we normally call out to runc to see the container's status. This does have significant performance implications, though, and we've seen issues with large amounts of runc processes being spawned. This patch attempts to use stat calls on the container exit file created by Conmon instead to sync state. This massively decreases the cost of calling updateContainer (it has gone from an almost-unconditional fork/exec of runc to a single stat call that can be avoided in most states). Signed-off-by: Matthew Heon --- libpod/container_api.go | 2 +- libpod/container_internal.go | 90 +++++++++++++++++++++++++++++++++++++++----- libpod/oci.go | 58 +++++++++++++++++----------- libpod/runtime_ctr.go | 2 +- 4 files changed, 118 insertions(+), 34 deletions(-) (limited to 'libpod') diff --git a/libpod/container_api.go b/libpod/container_api.go index 30c67eb2a..25c1fb5f9 100644 --- a/libpod/container_api.go +++ b/libpod/container_api.go @@ -685,7 +685,7 @@ func (c *Container) Sync() error { (c.state.State != ContainerStateConfigured) { oldState := c.state.State // TODO: optionally replace this with a stat for the exit file - if err := c.runtime.ociRuntime.updateContainerStatus(c); err != nil { + if err := c.runtime.ociRuntime.updateContainerStatus(c, true); err != nil { return err } // Only save back to DB if state changed diff --git a/libpod/container_internal.go b/libpod/container_internal.go index 867d539be..db1cf6c99 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -13,8 +13,10 @@ import ( "strconv" "strings" "syscall" + "time" "github.com/containers/buildah/imagebuildah" + "github.com/containers/libpod/pkg/ctime" "github.com/containers/libpod/pkg/hooks" "github.com/containers/libpod/pkg/hooks/exec" "github.com/containers/libpod/pkg/lookup" @@ -31,6 +33,7 @@ import ( "github.com/pkg/errors" "github.com/sirupsen/logrus" "golang.org/x/text/language" + kwait "k8s.io/apimachinery/pkg/util/wait" ) const ( @@ -147,6 +150,77 @@ func (c *Container) execPidPath(sessionID string) string { return filepath.Join(c.state.RunDir, "exec_pid_"+sessionID) } +// exitFilePath gets the path to the container's exit file +func (c *Container) exitFilePath() string { + return filepath.Join(c.runtime.ociRuntime.exitsDir, c.ID()) +} + +// Wait for the container's exit file to appear. +// When it does, update our state based on it. +func (c *Container) waitForExitFileAndSync() error { + exitFile := c.exitFilePath() + + err := kwait.ExponentialBackoff( + kwait.Backoff{ + Duration: 500 * time.Millisecond, + Factor: 1.2, + Steps: 6, + }, + func() (bool, error) { + _, err := os.Stat(exitFile) + if err != nil { + // wait longer + return false, nil + } + return true, nil + }) + if err != nil { + // Exit file did not appear + // Reset our state + c.state.ExitCode = -1 + c.state.FinishedTime = time.Now() + c.state.State = ContainerStateStopped + + if err2 := c.save(); err2 != nil { + logrus.Errorf("Error saving container %s state: %v", c.ID(), err2) + } + + return err + } + + if err := c.runtime.ociRuntime.updateContainerStatus(c, false); err != nil { + return err + } + + return c.save() +} + +// Handle the container exit file. +// The exit file is used to supply container exit time and exit code. +// This assumes the exit file already exists. +func (c *Container) handleExitFile(exitFile string, fi os.FileInfo) error { + c.state.FinishedTime = ctime.Created(fi) + statusCodeStr, err := ioutil.ReadFile(exitFile) + if err != nil { + return errors.Wrapf(err, "failed to read exit file for container %s", c.ID()) + } + statusCode, err := strconv.Atoi(string(statusCodeStr)) + if err != nil { + return errors.Wrapf(err, "error converting exit status code for container %s to int", + c.ID()) + } + c.state.ExitCode = int32(statusCode) + + oomFilePath := filepath.Join(c.bundlePath(), "oom") + if _, err = os.Stat(oomFilePath); err == nil { + c.state.OOMKilled = true + } + + c.state.Exited = true + + return nil +} + // Sync this container with on-disk state and runtime status // Should only be called with container lock held // This function should suffice to ensure a container's state is accurate and @@ -162,7 +236,7 @@ func (c *Container) syncContainer() error { (c.state.State != ContainerStateExited) { oldState := c.state.State // TODO: optionally replace this with a stat for the exit file - if err := c.runtime.ociRuntime.updateContainerStatus(c); err != nil { + if err := c.runtime.ociRuntime.updateContainerStatus(c, false); err != nil { return err } // Only save back to DB if state changed @@ -660,7 +734,10 @@ func (c *Container) start() error { } logrus.Debugf("Started container %s", c.ID()) - c.state.State = ContainerStateRunning + // We need to pick up full container state, including PID. + if err := c.runtime.ociRuntime.updateContainerStatus(c, true); err != nil { + return err + } return c.save() } @@ -673,13 +750,8 @@ func (c *Container) stop(timeout uint) error { return err } - // Sync the container's state to pick up return code - if err := c.runtime.ociRuntime.updateContainerStatus(c); err != nil { - return err - } - - // Container should clean itself up - return c.save() + // Wait until we have an exit file, and sync once we do + return c.waitForExitFileAndSync() } // Internal, non-locking function to pause a container diff --git a/libpod/oci.go b/libpod/oci.go index ca8f967c4..b390b1967 100644 --- a/libpod/oci.go +++ b/libpod/oci.go @@ -11,12 +11,10 @@ import ( "os/exec" "path/filepath" "runtime" - "strconv" "strings" "syscall" "time" - "github.com/containers/libpod/pkg/ctime" "github.com/containers/libpod/pkg/rootless" "github.com/containers/libpod/pkg/util" "github.com/coreos/go-systemd/activation" @@ -451,17 +449,47 @@ func (r *OCIRuntime) createOCIContainer(ctr *Container, cgroupParent string, res // updateContainerStatus retrieves the current status of the container from the // runtime. It updates the container's state but does not save it. -func (r *OCIRuntime) updateContainerStatus(ctr *Container) error { - state := new(spec.State) +// If useRunc is false, we will not directly hit runc to see the container's +// status, but will instead only check for the existance of the conmon exit file +// and update state to stopped if it exists. +func (r *OCIRuntime) updateContainerStatus(ctr *Container, useRunc bool) error { + exitFile := ctr.exitFilePath() runtimeDir, err := util.GetRootlessRuntimeDir() if err != nil { return err } + // If not using runc, we don't need to do most of this. + if !useRunc { + // If the container's not running, nothing to do. + if ctr.state.State != ContainerStateRunning { + return nil + } + + // Check for the exit file conmon makes + info, err := os.Stat(exitFile) + if err != nil { + if os.IsNotExist(err) { + // Container is still running, no error + return nil + } + + return errors.Wrapf(err, "error running stat on container %s exit file", ctr.ID()) + } + + // Alright, it exists. Transition to Stopped state. + ctr.state.State = ContainerStateStopped + + // Read the exit file to get our stopped time and exit code. + return ctr.handleExitFile(exitFile, info) + } + // Store old state so we know if we were already stopped oldState := ctr.state.State + state := new(spec.State) + cmd := exec.Command(r.path, "state", ctr.ID()) cmd.Env = append(cmd.Env, fmt.Sprintf("XDG_RUNTIME_DIR=%s", runtimeDir)) outPipe, err := cmd.StdoutPipe() @@ -514,7 +542,6 @@ func (r *OCIRuntime) updateContainerStatus(ctr *Container) error { // Only grab exit status if we were not already stopped // If we were, it should already be in the database if ctr.state.State == ContainerStateStopped && oldState != ContainerStateStopped { - exitFile := filepath.Join(r.exitsDir, ctr.ID()) var fi os.FileInfo err = kwait.ExponentialBackoff( kwait.Backoff{ @@ -538,24 +565,7 @@ func (r *OCIRuntime) updateContainerStatus(ctr *Container) error { return nil } - ctr.state.FinishedTime = ctime.Created(fi) - statusCodeStr, err := ioutil.ReadFile(exitFile) - if err != nil { - return errors.Wrapf(err, "failed to read exit file for container %s", ctr.ID()) - } - statusCode, err := strconv.Atoi(string(statusCodeStr)) - if err != nil { - return errors.Wrapf(err, "error converting exit status code for container %s to int", - ctr.ID()) - } - ctr.state.ExitCode = int32(statusCode) - - oomFilePath := filepath.Join(ctr.bundlePath(), "oom") - if _, err = os.Stat(oomFilePath); err == nil { - ctr.state.OOMKilled = true - } - - ctr.state.Exited = true + return ctr.handleExitFile(exitFile, fi) } return nil @@ -601,6 +611,8 @@ func (r *OCIRuntime) killContainer(ctr *Container, signal uint) error { // Does not set finished time for container, assumes you will run updateStatus // after to pull the exit code func (r *OCIRuntime) stopContainer(ctr *Container, timeout uint) error { + logrus.Debugf("Stopping container %s (PID %d)", ctr.ID(), ctr.state.PID) + // Ping the container to see if it's alive // If it's not, it's already stopped, return err := unix.Kill(ctr.state.PID, 0) diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go index b63726f29..09dc7c48b 100644 --- a/libpod/runtime_ctr.go +++ b/libpod/runtime_ctr.go @@ -256,7 +256,7 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force bool) } // Need to update container state to make sure we know it's stopped - if err := c.syncContainer(); err != nil { + if err := c.waitForExitFileAndSync(); err != nil { return err } } else if !(c.state.State == ContainerStateConfigured || -- cgit v1.2.3-54-g00ecf From 94763a47a6e86ccf13d8a3ded3365f9c4440a668 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Thu, 25 Oct 2018 13:28:19 -0400 Subject: If a container ceases to exist in runc, set exit status When we scan a container in runc and see that it no longer exists, we already set ContainerStatusExited to indicate that it no longer exists in runc. Now, also set an exit code and exit time, so PS output will make some sense. Signed-off-by: Matthew Heon --- libpod/oci.go | 2 ++ 1 file changed, 2 insertions(+) (limited to 'libpod') diff --git a/libpod/oci.go b/libpod/oci.go index b390b1967..e8a97226a 100644 --- a/libpod/oci.go +++ b/libpod/oci.go @@ -508,6 +508,8 @@ func (r *OCIRuntime) updateContainerStatus(ctr *Container, useRunc bool) error { } if strings.Contains(string(out), "does not exist") { ctr.removeConmonFiles() + ctr.state.ExitCode = -1 + ctr.state.FinishedTime = time.Now() ctr.state.State = ContainerStateExited return nil } -- cgit v1.2.3-54-g00ecf From 3286b0185debc809664d854dd2ee75bf75efadc4 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Thu, 25 Oct 2018 13:35:39 -0400 Subject: Retrieve container PID from conmon Instead of running a full sync after starting a container to pick up its PID, grab it from Conmon instead. Signed-off-by: Matthew Heon --- libpod/container_internal.go | 5 ----- libpod/oci.go | 1 + 2 files changed, 1 insertion(+), 5 deletions(-) (limited to 'libpod') diff --git a/libpod/container_internal.go b/libpod/container_internal.go index db1cf6c99..9bb45a08a 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -734,11 +734,6 @@ func (c *Container) start() error { } logrus.Debugf("Started container %s", c.ID()) - // We need to pick up full container state, including PID. - if err := c.runtime.ociRuntime.updateContainerStatus(c, true); err != nil { - return err - } - return c.save() } diff --git a/libpod/oci.go b/libpod/oci.go index e8a97226a..d3ce70133 100644 --- a/libpod/oci.go +++ b/libpod/oci.go @@ -441,6 +441,7 @@ func (r *OCIRuntime) createOCIContainer(ctr *Container, cgroupParent string, res } return errors.Wrapf(ErrInternal, "container create failed") } + ctr.state.PID = ss.si.Pid case <-time.After(ContainerCreateTimeout): return errors.Wrapf(ErrInternal, "container creation timeout") } -- cgit v1.2.3-54-g00ecf From 0f45403c9ba5eace4b48bad2b86ad540bae3435d Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Thu, 25 Oct 2018 13:49:39 -0400 Subject: Fix misspelling Signed-off-by: Matthew Heon --- libpod/oci.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'libpod') diff --git a/libpod/oci.go b/libpod/oci.go index d3ce70133..233bacfbb 100644 --- a/libpod/oci.go +++ b/libpod/oci.go @@ -451,7 +451,7 @@ func (r *OCIRuntime) createOCIContainer(ctr *Container, cgroupParent string, res // updateContainerStatus retrieves the current status of the container from the // runtime. It updates the container's state but does not save it. // If useRunc is false, we will not directly hit runc to see the container's -// status, but will instead only check for the existance of the conmon exit file +// status, but will instead only check for the existence of the conmon exit file // and update state to stopped if it exists. func (r *OCIRuntime) updateContainerStatus(ctr *Container, useRunc bool) error { exitFile := ctr.exitFilePath() -- cgit v1.2.3-54-g00ecf From c9e9ca5671775652b2e097641713084ea367a7fa Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Thu, 25 Oct 2018 14:11:44 -0400 Subject: Properly set Running state when starting containers Signed-off-by: Matthew Heon --- libpod/container_internal.go | 2 ++ 1 file changed, 2 insertions(+) (limited to 'libpod') diff --git a/libpod/container_internal.go b/libpod/container_internal.go index 9bb45a08a..5221ac5b6 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -734,6 +734,8 @@ func (c *Container) start() error { } logrus.Debugf("Started container %s", c.ID()) + c.state.State = ContainerStateRunning + return c.save() } -- cgit v1.2.3-54-g00ecf From 536af1f68944c68956e705d589e11cd52875273a Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Mon, 29 Oct 2018 13:20:26 -0400 Subject: Print error status code if we fail to parse it When we read the conmon error status file, if Atoi fails to parse the string we read from the file as an int, print the string as part of the error message so we know what might have gone wrong. Signed-off-by: Matthew Heon --- libpod/container_internal.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'libpod') diff --git a/libpod/container_internal.go b/libpod/container_internal.go index 5221ac5b6..54558f3fc 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -206,8 +206,8 @@ func (c *Container) handleExitFile(exitFile string, fi os.FileInfo) error { } statusCode, err := strconv.Atoi(string(statusCodeStr)) if err != nil { - return errors.Wrapf(err, "error converting exit status code for container %s to int", - c.ID()) + return errors.Wrapf(err, "error converting exit status code (%q) for container %s to int", + c.ID(), statusCodeStr) } c.state.ExitCode = int32(statusCode) -- cgit v1.2.3-54-g00ecf