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

[dbode] Call syscall.Setrlimit to set num files open hard limit with setcap for DB docker image #1666

Merged
merged 6 commits into from
May 28, 2019
Merged
Show file tree
Hide file tree
Changes from 4 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
4 changes: 4 additions & 0 deletions docker/m3dbnode/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,9 @@ COPY --from=builder /go/src/github.com/m3db/m3/bin/m3dbnode /bin/
COPY --from=builder /go/src/github.com/m3db/m3/src/dbnode/config/m3dbnode-local-etcd.yml /etc/m3dbnode/m3dbnode.yml
COPY --from=builder /go/src/github.com/m3db/m3/scripts/m3dbnode_bootstrapped.sh /bin/

# Use setcap and run as specific user
RUN apk add libcap && \
Copy link
Contributor

Choose a reason for hiding this comment

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

Had not seen this before. Did some reading. Kind of bizarre that the capabilities get set on the file/binary level

Copy link
Contributor

Choose a reason for hiding this comment

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

What does the +ep do? Can you just add a comment to this line generally also

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not opposed to adding a comment, the +e is "effective" and +p is for "permitted".

setcap cap_sys_resource=+ep /bin/m3dbnode

ENTRYPOINT [ "/bin/m3dbnode" ]
CMD [ "-f", "/etc/m3dbnode/m3dbnode.yml" ]
2 changes: 1 addition & 1 deletion src/dbnode/server/limits.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ package server
import (
"fmt"

xos "github.com/m3db/m3/src/x/os"
xerror "github.com/m3db/m3/src/x/errors"
xos "github.com/m3db/m3/src/x/os"
)

const (
Expand Down
14 changes: 14 additions & 0 deletions src/dbnode/server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ import (
"github.com/m3db/m3/src/x/instrument"
"github.com/m3db/m3/src/x/lockfile"
"github.com/m3db/m3/src/x/mmap"
xos "github.com/m3db/m3/src/x/os"
"github.com/m3db/m3/src/x/pool"
"github.com/m3db/m3/src/x/serialize"
xsync "github.com/m3db/m3/src/x/sync"
Expand Down Expand Up @@ -150,6 +151,19 @@ func Run(runOpts RunOptions) {
}
defer logger.Sync()

// Raise fd limits to nr_open system limit
result, err := xos.RaiseProcessNoFileToNROpen()
if err != nil {
logger.Warn("unable to raise rlimit", zap.Error(err))
} else {
logger.Info("raised rlimit no file fds limit",
zap.Bool("required", result.RaiseRequired),
zap.Uint64("sysNROpenValue", result.NROpenValue),
zap.Uint64("noFileMaxValue", result.NoFileMaxValue),
zap.Uint64("noFileCurrValue", result.NoFileCurrValue))
}

// Parse file and directory modes
newFileMode, err := cfg.Filesystem.ParseNewFileMode()
if err != nil {
logger.Fatal("could not parse new file mode", zap.Error(err))
Expand Down
9 changes: 9 additions & 0 deletions src/x/os/limits.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,12 @@ type ProcessLimits struct {
VMMaxMapCount int64 // corresponds to /proc/sys/vm/max_map_count
VMSwappiness int64 // corresponds to /proc/sys/vm/swappiness
}

// RaiseProcessNoFileToNROpenResult captures the result of trying to
// raise the process num files open limit to the nr_open system value.
type RaiseProcessNoFileToNROpenResult struct {
RaiseRequired bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe RaisePerformed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure thing.

NROpenValue uint64
NoFileMaxValue uint64
NoFileCurrValue uint64
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a comment here saying that this will be the curr value before the raise was performed (if it was performed)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's not true though, it will be the curr value (after the raise) or if no raise then it will be the curr value (unadjusted).

}
45 changes: 45 additions & 0 deletions src/x/os/limits_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ const (
sysctlDir = "/proc/sys/"
vmMaxMapCountKey = "vm.max_map_count"
vmSwappinessKey = "vm.swappiness"
fsNROpenKey = "fs.nr_open"
)

// CanGetProcessLimits returns a boolean to signify if it can return limits,
Expand Down Expand Up @@ -66,6 +67,50 @@ func GetProcessLimits() (ProcessLimits, error) {
}, nil
}

// RaiseProcessNoFileToNROpen first determines the NROpen limit by reading
// the corresponding proc sys file and then if the hard or soft limits
// are below this number, the limits are raised using a call to setrlimit.
func RaiseProcessNoFileToNROpen() (RaiseProcessNoFileToNROpenResult, error) {
value, err := sysctlInt64(fsNROpenKey)
if err != nil {
return RaiseProcessNoFileToNROpenResult{}, fmt.Errorf(
"unable to raise nofile limits: nr_open_parse_err=%v", err)
}

limit := uint64(value)

var limits syscall.Rlimit
if err := syscall.Getrlimit(syscall.RLIMIT_NOFILE, &limits); err != nil {
return RaiseProcessNoFileToNROpenResult{}, fmt.Errorf(
"unable to raise nofile limits: rlimit_get_err=%v", err)
}

if limits.Max >= limit && limits.Cur >= limit {
// Limit already set correctly
return RaiseProcessNoFileToNROpenResult{
RaiseRequired: false,
NROpenValue: limit,
NoFileMaxValue: limits.Max,
NoFileCurrValue: limits.Cur,
}, nil
}

limits.Max = limit
limits.Cur = limit

if err := syscall.Setrlimit(syscall.RLIMIT_NOFILE, &limits); err != nil {
return RaiseProcessNoFileToNROpenResult{}, fmt.Errorf(
"unable to raise nofile limits: rlimit_set_err=%v", err)
}

return RaiseProcessNoFileToNROpenResult{
RaiseRequired: true,
NROpenValue: limit,
NoFileMaxValue: limits.Max,
NoFileCurrValue: limits.Cur,
}, nil
}

func sysctlInt64(key string) (int64, error) {
str, err := sysctl(key)
if err != nil {
Expand Down
13 changes: 11 additions & 2 deletions src/x/os/limits_other.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,17 @@

package xos

import "errors"
import (
"errors"
)

const (
nonLinuxWarning = "unable to determine process limits on non-linux os"
)

var (
errUnableToDetermineProcessLimits = errors.New(nonLinuxWarning)
errUnableToDetermineProcessLimits = errors.New(nonLinuxWarning)
errUnableToRaiseProcessNoFileNonLinux = errors.New("unable to raise no file limits on non-linux os")
)

// CanGetProcessLimits returns a boolean to signify if it can return limits,
Expand All @@ -42,3 +45,9 @@ func CanGetProcessLimits() (bool, string) {
func GetProcessLimits() (ProcessLimits, error) {
return ProcessLimits{}, errUnableToDetermineProcessLimits
}

// RaiseProcessNoFileToNROpen attempts to raise the process num files
// open limit to the nr_open system value.
func RaiseProcessNoFileToNROpen() (RaiseProcessNoFileToNROpenResult, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're gonna start doing this it might be nice to have a generic AttemptToSetProcessLimits which calls this function. That way we don't have to change server.go each time and we have an established pattern

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is that kind of method however, it is meant to be abstract and returns enough information (without taking a logger itself). That way callers can either log it as a warning or a hard error, depending on their situation.

return RaiseProcessNoFileToNROpenResult{}, errUnableToRaiseProcessNoFileNonLinux
}