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/oci.go | 58 +++++++++++++++++++++++++++++++++++----------------------- 1 file changed, 35 insertions(+), 23 deletions(-) (limited to 'libpod/oci.go') 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) -- 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/oci.go') 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/oci.go') 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/oci.go') 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