Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libpod: improve heuristic to detect cgroup #12403

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions libpod/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@ import (
"io/ioutil"
"net"
"os"
"strings"
"time"

types040 "github.com/containernetworking/cni/pkg/types/040"
"github.com/containers/common/pkg/config"
"github.com/containers/common/pkg/secrets"
"github.com/containers/image/v5/manifest"
"github.com/containers/podman/v3/libpod/define"
Expand Down Expand Up @@ -963,6 +965,29 @@ func (c *Container) cGroupPath() (string, error) {
return "", errors.Errorf("could not find any cgroup in %q", procPath)
}

cgroupManager := c.CgroupManager()
switch {
case c.config.CgroupsMode == cgroupSplit:
name := fmt.Sprintf("/libpod-payload-%s/", c.ID())
if index := strings.LastIndex(cgroupPath, name); index >= 0 {
return cgroupPath[:index+len(name)-1], nil
}
case cgroupManager == config.CgroupfsCgroupsManager:
name := fmt.Sprintf("/libpod-%s/", c.ID())
if index := strings.LastIndex(cgroupPath, name); index >= 0 {
return cgroupPath[:index+len(name)-1], nil
}
case cgroupManager == config.SystemdCgroupsManager:
// When running under systemd, try to detect the scope that was requested
// to be created. It improves the heuristic since we report the first
// cgroup that was created instead of the cgroup where PID 1 might have
// moved to.
name := fmt.Sprintf("/libpod-%s.scope/", c.ID())
if index := strings.LastIndex(cgroupPath, name); index >= 0 {
return cgroupPath[:index+len(name)-1], nil
}
}

return cgroupPath, nil
}

Expand Down
11 changes: 11 additions & 0 deletions libpod/container_inspect.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,16 @@ func (c *Container) getContainerInspectData(size bool, driverData *define.Driver
return nil, err
}

cgroupPath, err := c.cGroupPath()
if err != nil {
// Handle the case where the container is not running or has no cgroup.
if errors.Is(err, define.ErrNoCgroups) || errors.Is(err, define.ErrCtrStopped) {
cgroupPath = ""
} else {
return nil, err
}
}

data := &define.InspectContainerData{
ID: config.ID,
Created: config.CreatedTime,
Expand All @@ -116,6 +126,7 @@ func (c *Container) getContainerInspectData(size bool, driverData *define.Driver
StartedAt: runtimeInfo.StartedTime,
FinishedAt: runtimeInfo.FinishedTime,
Checkpointed: runtimeInfo.Checkpointed,
CgroupPath: cgroupPath,
},
Image: config.RootfsImageID,
ImageName: config.RootfsImageName,
Expand Down
2 changes: 1 addition & 1 deletion libpod/container_internal_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -2618,7 +2618,7 @@ func (c *Container) getOCICgroupPath() (string, error) {
if err != nil {
return "", err
}
return filepath.Join(selfCgroup, "container"), nil
return filepath.Join(selfCgroup, fmt.Sprintf("libpod-payload-%s", c.ID())), nil
case cgroupManager == config.SystemdCgroupsManager:
// When the OCI runtime is set to use Systemd as a cgroup manager, it
// expects cgroups to be passed as follows:
Expand Down
1 change: 1 addition & 0 deletions libpod/define/container_inspect.go
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ type InspectContainerState struct {
FinishedAt time.Time `json:"FinishedAt"`
Health HealthCheckResults `json:"Health,omitempty"`
Checkpointed bool `json:"Checkpointed,omitempty"`
CgroupPath string `json:"CgroupPath,omitempty"`
}

// Healthcheck returns the HealthCheckResults. This is used for old podman compat
Expand Down
24 changes: 16 additions & 8 deletions libpod/stats.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
package libpod

import (
"math"
"strings"
"syscall"
"time"
Expand Down Expand Up @@ -68,7 +69,7 @@ func (c *Container) GetContainerStats(previousStats *define.ContainerStats) (*de
stats.AvgCPU = calculateAvgCPU(stats.CPU, previousStats.AvgCPU, previousStats.DataPoints)
stats.DataPoints = previousStats.DataPoints + 1
stats.MemUsage = cgroupStats.Memory.Usage.Usage
stats.MemLimit = getMemLimit(cgroupStats.Memory.Usage.Limit)
stats.MemLimit = c.getMemLimit()
stats.MemPerc = (float64(stats.MemUsage) / float64(stats.MemLimit)) * 100
stats.PIDs = 0
if conState == define.ContainerStateRunning || conState == define.ContainerStatePaused {
Expand All @@ -91,22 +92,29 @@ func (c *Container) GetContainerStats(previousStats *define.ContainerStats) (*de
return stats, nil
}

// getMemory limit returns the memory limit for a given cgroup
// If the configured memory limit is larger than the total memory on the sys, the
// physical system memory size is returned
func getMemLimit(cgroupLimit uint64) uint64 {
// getMemory limit returns the memory limit for a container
func (c *Container) getMemLimit() uint64 {
memLimit := uint64(math.MaxUint64)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

99% sure this is a break from Docker's behavior - we should report a limit of 0 for containers that do not have a resource limit, not the maximum amount of RAM on the current system.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed it after a test was failing.

The current Podman behavior, AFAICS, is to report the maximum amount of RAM:

$ /usr/bin/podman run --rm -ti -d alpine top
58efadc49b77ef300eccb02715235616fb5729135198665f5ca7745006f93940
$ /usr/bin/podman stats -l --no-reset --no-stream
ID            NAME          CPU %       MEM USAGE / LIMIT  MEM %       NET IO      BLOCK IO    PIDS        CPU TIME    AVG CPU %
58efadc49b77  crazy_fermat  4.40%       233.5kB / 33.39GB  0.00%       -- / --     -- / --     1           7.2ms       4.40%

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you verify that podman inspect still reports 0 if not limited? If so, I'm fine with this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They return the same values:

$ diff <(bin/podman inspect -l | grep -i memory) <(/usr/bin/podman inspect -l | grep -i memory) | wc -l
0


if c.config.Spec.Linux != nil && c.config.Spec.Linux.Resources != nil &&
c.config.Spec.Linux.Resources.Memory != nil && c.config.Spec.Linux.Resources.Memory.Limit != nil {
memLimit = uint64(*c.config.Spec.Linux.Resources.Memory.Limit)
}

si := &syscall.Sysinfo_t{}
err := syscall.Sysinfo(si)
if err != nil {
return cgroupLimit
return memLimit
}

//nolint:unconvert
physicalLimit := uint64(si.Totalram)
if cgroupLimit > physicalLimit {

if memLimit <= 0 || memLimit > physicalLimit {
return physicalLimit
}
return cgroupLimit

return memLimit
}

// calculateCPUPercent calculates the cpu usage using the latest measurement in stats.
Expand Down
5 changes: 5 additions & 0 deletions test/e2e/systemd_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,11 @@ WantedBy=multi-user.target
stats := podmanTest.Podman([]string{"stats", "--no-stream", ctrName})
stats.WaitWithDefaultTimeout()
Expect(stats).Should(Exit(0))

cgroupPath := podmanTest.Podman([]string{"inspect", "--format='{{.State.CgroupPath}}'", ctrName})
cgroupPath.WaitWithDefaultTimeout()
Expect(cgroupPath).Should(Exit(0))
Expect(result.OutputToString()).To(Not(ContainSubstring("init.scope")))
})

It("podman create container with systemd entrypoint triggers systemd mode", func() {
Expand Down