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

fix ulimit issue #18721

Merged
merged 1 commit into from
May 31, 2023
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
45 changes: 45 additions & 0 deletions libpod/container_internal_common.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ import (
"github.com/opencontainers/selinux/go-selinux"
"github.com/opencontainers/selinux/go-selinux/label"
"github.com/sirupsen/logrus"
"golang.org/x/sys/unix"
)

func parseOptionIDs(ctrMappings []idtools.IDMap, option string) ([]idtools.IDMap, error) {
Expand Down Expand Up @@ -624,6 +625,50 @@ func (c *Container) generateSpec(ctx context.Context) (s *spec.Spec, cleanupFunc
}
}

// setup rlimits
nofileSet := false
nprocSet := false
isRootless := rootless.IsRootless()
if isRootless {
for _, rlimit := range c.config.Spec.Process.Rlimits {
if rlimit.Type == "RLIMIT_NOFILE" {
nofileSet = true
} else if rlimit.Type == "RLIMIT_NPROC" {
nprocSet = true
}
}
if !nofileSet {
max := rlimT(define.RLimitDefaultValue)
current := rlimT(define.RLimitDefaultValue)
var rlimit unix.Rlimit
if err := unix.Getrlimit(unix.RLIMIT_NOFILE, &rlimit); err != nil {
logrus.Warnf("Failed to return RLIMIT_NOFILE ulimit %q", err)
}
if rlimT(rlimit.Cur) < current {
current = rlimT(rlimit.Cur)
}
if rlimT(rlimit.Max) < max {
max = rlimT(rlimit.Max)
}
g.AddProcessRlimits("RLIMIT_NOFILE", uint64(max), uint64(current))
}
if !nprocSet {
max := rlimT(define.RLimitDefaultValue)
current := rlimT(define.RLimitDefaultValue)
var rlimit unix.Rlimit
if err := unix.Getrlimit(unix.RLIMIT_NPROC, &rlimit); err != nil {
logrus.Warnf("Failed to return RLIMIT_NPROC ulimit %q", err)
}
if rlimT(rlimit.Cur) < current {
current = rlimT(rlimit.Cur)
}
if rlimT(rlimit.Max) < max {
max = rlimT(rlimit.Max)
}
g.AddProcessRlimits("RLIMIT_NPROC", uint64(max), uint64(current))
}
}

return g.Config, cleanupFunc, nil
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//go:build freebsd
// +build freebsd

package generate
package libpod

type rlimT int64
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//go:build linux || darwin
// +build linux darwin

package generate
package libpod

type rlimT uint64
34 changes: 0 additions & 34 deletions pkg/specgen/container_validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,11 @@ package specgen
import (
"errors"
"fmt"
"strconv"
"strings"

"github.com/containers/common/pkg/util"
"github.com/containers/podman/v4/libpod/define"
"github.com/containers/podman/v4/pkg/rootless"
"github.com/opencontainers/runtime-spec/specs-go"
)

var (
Expand Down Expand Up @@ -135,38 +133,6 @@ func (s *SpecGenerator) Validate() error {
// default:
// return errors.New("unrecognized option for cgroups; supported are 'default', 'disabled', 'no-conmon'")
// }
invalidUlimitFormatError := errors.New("invalid default ulimit definition must be form of type=soft:hard")
// set ulimits if not rootless
if len(s.ContainerResourceConfig.Rlimits) < 1 && !rootless.IsRootless() {
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove this? This sets the limit based on containers.conf defaults so we need to keep it. Although doing this in Validate() looks wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm under the impression that it's already set by

s.Rlimits, err = GenRlimits(c.Ulimit)

Doing a quick test it looks like a ulimit from containers.conf still makes it into a container, so this was redundant. However it's in there twice right now. So I think the containers.conf defaults also make it to:

rlimits, err := specgenutil.GenRlimits(rtc.Ulimits())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue with the limits being in the list twice is fixed.

// Containers common defines this as something like nproc=4194304:4194304
tmpnproc := containerConfig.Ulimits()
var posixLimits []specs.POSIXRlimit
for _, limit := range tmpnproc {
limitSplit := strings.SplitN(limit, "=", 2)
if len(limitSplit) < 2 {
return fmt.Errorf("missing = in %s: %w", limit, invalidUlimitFormatError)
}
valueSplit := strings.SplitN(limitSplit[1], ":", 2)
if len(valueSplit) < 2 {
return fmt.Errorf("missing : in %s: %w", limit, invalidUlimitFormatError)
}
hard, err := strconv.Atoi(valueSplit[0])
if err != nil {
return err
}
soft, err := strconv.Atoi(valueSplit[1])
if err != nil {
return err
}
posixLimit := specs.POSIXRlimit{
Type: limitSplit[0],
Hard: uint64(hard),
Soft: uint64(soft),
}
posixLimits = append(posixLimits, posixLimit)
}
s.ContainerResourceConfig.Rlimits = posixLimits
}
// Namespaces
if err := s.UtsNS.validate(); err != nil {
return err
Expand Down
57 changes: 1 addition & 56 deletions pkg/specgen/generate/oci.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,72 +7,17 @@ import (
"github.com/containers/common/libimage"
"github.com/containers/common/pkg/config"
"github.com/containers/podman/v4/libpod/define"
"github.com/containers/podman/v4/pkg/rootless"
"github.com/containers/podman/v4/pkg/specgen"
"github.com/opencontainers/runtime-tools/generate"
"github.com/sirupsen/logrus"
"golang.org/x/sys/unix"
)

func addRlimits(s *specgen.SpecGenerator, g *generate.Generator) {
var (
isRootless = rootless.IsRootless()
nofileSet = false
nprocSet = false
)

if s.Rlimits == nil {
g.Config.Process.Rlimits = nil
return
}
g.Config.Process.Rlimits = nil

for _, u := range s.Rlimits {
name := "RLIMIT_" + strings.ToUpper(u.Type)
if name == "RLIMIT_NOFILE" {
nofileSet = true
} else if name == "RLIMIT_NPROC" {
nprocSet = true
}
g.AddProcessRlimits(name, u.Hard, u.Soft)
}

// If not explicitly overridden by the user, default number of open
// files and number of processes to the maximum they can be set to
// (without overriding a sysctl)
if !nofileSet {
Copy link
Member

Choose a reason for hiding this comment

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

I am pretty sure we actually want this. The problem is that we only set it once on create instead of for each start.

So I think this must be moved into func (c *Container) generateSpec() instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a problem with leaving the ulimits in the spec file empty? The container process should inherit the limits, right?

Is that generateSpec invoked on each run and the spec passed to crun, or is it also saved on disk for the next time the container is started?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so func (c *Container) generateSpec() from container_internal_common.go gets executed each time the container is started if it's in ContainerStateConfigured or ContainerStateExited. I'll put setting the max ulimits in there and see if that fixes the CI issues for rootless.

I'll also disable the new test for rootfull, as this issue is not really applicable to rootfull.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this approach works on my end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Incorporated into latest version of this PR

max := rlimT(define.RLimitDefaultValue)
current := rlimT(define.RLimitDefaultValue)
if isRootless {
var rlimit unix.Rlimit
if err := unix.Getrlimit(unix.RLIMIT_NOFILE, &rlimit); err != nil {
logrus.Warnf("Failed to return RLIMIT_NOFILE ulimit %q", err)
}
if rlimT(rlimit.Cur) < current {
current = rlimT(rlimit.Cur)
}
if rlimT(rlimit.Max) < max {
max = rlimT(rlimit.Max)
}
}
g.AddProcessRlimits("RLIMIT_NOFILE", uint64(max), uint64(current))
}
if !nprocSet {
max := rlimT(define.RLimitDefaultValue)
current := rlimT(define.RLimitDefaultValue)
if isRootless {
var rlimit unix.Rlimit
if err := unix.Getrlimit(unix.RLIMIT_NPROC, &rlimit); err != nil {
logrus.Warnf("Failed to return RLIMIT_NPROC ulimit %q", err)
}
if rlimT(rlimit.Cur) < current {
current = rlimT(rlimit.Cur)
}
if rlimT(rlimit.Max) < max {
max = rlimT(rlimit.Max)
}
}
g.AddProcessRlimits("RLIMIT_NPROC", uint64(max), uint64(current))
}
}

// Produce the final command for the container.
Expand Down
28 changes: 28 additions & 0 deletions test/e2e/inspect_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +489,34 @@ var _ = Describe("Podman inspect", func() {
Expect(found).To(BeTrue(), "found RLIMIT_CORE")
})

It("Container inspect Ulimit test", func() {
SkipIfNotRootless("Only applicable to rootless")
ctrName := "testctr"
session := podmanTest.Podman([]string{"create", "--name", ctrName, ALPINE, "top"})
session.WaitWithDefaultTimeout()
Expect(session).Should(Exit(0))

inspect := podmanTest.Podman([]string{"inspect", ctrName})
inspect.WaitWithDefaultTimeout()
Expect(inspect).Should(Exit(0))

dataCreate := inspect.InspectContainerToJSON()
ulimitsCreate := dataCreate[0].HostConfig.Ulimits
Expect(ulimitsCreate).To(BeEmpty())

start := podmanTest.Podman([]string{"start", ctrName})
start.WaitWithDefaultTimeout()
Expect(start).Should(Exit(0))

inspect2 := podmanTest.Podman([]string{"inspect", ctrName})
inspect2.WaitWithDefaultTimeout()
Expect(inspect2).Should(Exit(0))

dataStart := inspect2.InspectContainerToJSON()
ulimitsStart := dataStart[0].HostConfig.Ulimits
Expect(ulimitsStart).ToNot(BeEmpty())
})

It("Dropped capabilities are sorted", func() {
ctrName := "testCtr"
session := podmanTest.Podman([]string{"run", "-d", "--cap-drop", "SETUID", "--cap-drop", "SETGID", "--cap-drop", "CAP_NET_BIND_SERVICE", "--name", ctrName, ALPINE, "top"})
Expand Down
39 changes: 39 additions & 0 deletions test/system/045-start.bats
Original file line number Diff line number Diff line change
Expand Up @@ -99,4 +99,43 @@ load helpers
run_podman rm -t 0 -f $ctrID $cname
}

@test "podman start again with lower ulimit -u" {
skip_if_not_rootless "tests ulimit -u changes in the rootless scenario"
skip_if_remote "test relies on control of ulimit -u (not possible for remote)"
# get current ulimit -u
nrpoc_limit=$(ulimit -Hu)

# create container
run_podman create $IMAGE echo "hello"
ctrID="$output"

# inspect
run_podman inspect $ctrID
assert "$output" =~ '"Ulimits": \[\]' "Ulimits has to be empty after create"

# start container for the first time
run_podman start $ctrID
is "$output" "$ctrID"

# inspect
run_podman inspect $ctrID --format '{{range .HostConfig.Ulimits}}{{if eq .Name "RLIMIT_NPROC" }}{{.Soft}}:{{.Hard}}{{end}}{{end}}'
assert "$output" == "${nrpoc_limit}:${nrpoc_limit}" "Ulimit has to match ulimit -Hu"

# lower ulimit -u by one
((nrpoc_limit--))

# set new ulimit -u
ulimit -u $nrpoc_limit

# start container for the second time
run_podman start $ctrID
is "$output" "$ctrID"

# inspect
run_podman inspect $ctrID --format '{{range .HostConfig.Ulimits}}{{if eq .Name "RLIMIT_NPROC" }}{{.Soft}}:{{.Hard}}{{end}}{{end}}'
assert "$output" == "${nrpoc_limit}:${nrpoc_limit}" "Ulimit has to match new ulimit -Hu"

run_podman rm -t 0 -f $ctrID $cname
}

# vim: filetype=sh