From 1a5696fe23a54a89a97e83e49e857f3f167f0c70 Mon Sep 17 00:00:00 2001 From: Seebs Date: Thu, 15 Apr 2021 14:46:08 -0500 Subject: [PATCH] centralize attempts to set/check limits We check mmap limits, and try to set/increase our open file limits, and we check the mmap limit when we start the server, and try to set the open file limit every time we open a holder. It's useless to do these things more than once, though. We migrate these things to be run through a sync.Once, which runs all of them the first time a server starts up, and then thereafter just returns the error code from that first run. This should make test startup ever so slightly cheaper, saving us potentially several microseconds, but also reducing the spamminess of the message. I've taken out the `sudo ulimit` advice since it's wrong, and the documentation link is updated to point to our (now private!) customer documentation. --- holder.go | 55 ----------------------------- server/server.go | 92 +++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 79 insertions(+), 68 deletions(-) diff --git a/holder.go b/holder.go index 941f9466d..a2e8ea7cd 100644 --- a/holder.go +++ b/holder.go @@ -26,7 +26,6 @@ import ( "strconv" "strings" "sync" - "syscall" "time" "github.com/pilosa/pilosa/v2/disco" @@ -47,9 +46,6 @@ const ( // defaultCacheFlushInterval is the default value for Fragment.CacheFlushInterval. defaultCacheFlushInterval = 1 * time.Minute - // fileLimit is the maximum open file limit (ulimit -n) to automatically set. - fileLimit = 262144 // (512^2) - // existenceFieldName is the name of the internal field used to store existence values. existenceFieldName = "_exists" @@ -622,8 +618,6 @@ func (h *Holder) Open() error { // Reset closing in case Holder is being reopened. h.closing = make(chan struct{}) - h.setFileLimit() - h.Logger.Printf("open holder path: %s", h.path) if err := os.MkdirAll(h.IndexesPath(), 0777); err != nil { return errors.Wrap(err, "creating directory") @@ -1448,55 +1442,6 @@ func (h *Holder) recalculateCaches() { } } -// setFileLimit attempts to set the open file limit to the FileLimit constant defined above. -func (h *Holder) setFileLimit() { - oldLimit := &syscall.Rlimit{} - newLimit := &syscall.Rlimit{} - - if err := syscall.Getrlimit(syscall.RLIMIT_NOFILE, oldLimit); err != nil { - h.Logger.Errorf("checking open file limit: %s", err) - return - } - // If the soft limit is lower than the FileLimit constant, we will try to change it. - if oldLimit.Cur < fileLimit { - newLimit.Cur = fileLimit - // If the hard limit is not high enough, we will try to change it too. - if oldLimit.Max < fileLimit { - newLimit.Max = fileLimit - } else { - newLimit.Max = oldLimit.Max - } - - // Try to set the limit - if err := syscall.Setrlimit(syscall.RLIMIT_NOFILE, newLimit); err != nil { - // If we just tried to change the hard limit and failed, we probably don't have permission. Let's try again without setting the hard limit. - if newLimit.Max > oldLimit.Max { - newLimit.Max = oldLimit.Max - // Obviously the hard limit cannot be higher than the soft limit. - if newLimit.Cur >= newLimit.Max { - newLimit.Cur = newLimit.Max - } - // Try setting again with lowered Max (hard limit) - if err := syscall.Setrlimit(syscall.RLIMIT_NOFILE, newLimit); err != nil { - h.Logger.Errorf("setting open file limit: %s", err) - } - // If we weren't trying to change the hard limit, let the user know something is wrong. - } else { - h.Logger.Errorf("setting open file limit: %s", err) - } - } - - // Check the limit after setting it. OS may not obey Setrlimit call. - if err := syscall.Getrlimit(syscall.RLIMIT_NOFILE, oldLimit); err != nil { - h.Logger.Errorf("checking open file limit: %s", err) - } else { - if oldLimit.Cur < fileLimit { - h.Logger.Warnf("Tried to set open file limit to %d, but it is %d. You may consider running \"sudo ulimit -n %d\" before starting Pilosa to avoid \"too many open files\" error. See https://www.pilosa.com/docs/latest/administration/#open-file-limits for more information.", fileLimit, oldLimit.Cur, fileLimit) - } - } - } -} - // Log startup time and version to $DATA_DIR/.startup.log func (h *Holder) logStartup() error { RFC3339NanoFixedWidth := "2006-01-02T15:04:05.000000 07:00" diff --git a/server/server.go b/server/server.go index 1f70d29d0..63e208001 100644 --- a/server/server.go +++ b/server/server.go @@ -22,6 +22,7 @@ package server import ( "context" "crypto/tls" + "fmt" "io" "io/ioutil" "log" @@ -144,6 +145,81 @@ func NewCommand(stdin io.Reader, stdout, stderr io.Writer, opts ...CommandOption return c } +// defaultFileLimit is a suggested open file count limit for Pilosa to run with +const ( + defaultFileLimit = uint64(256 * 1024) +) + +// we want to set resource limits *exactly once*, and then be able +// to report on whether or not that succeeded. +var setupResourceLimitsOnce sync.Once +var setupResourceLimitsErr error + +// doSetupResourceLimits is the function which actually does the +// resource limit setup, possibly yielding an error. it's a Command +// method because it uses the command's logger, but is in fact +// expected to work globally. +func (m *Command) doSetupResourceLimits() error { + oldLimit := &syscall.Rlimit{} + + if err := syscall.Getrlimit(syscall.RLIMIT_NOFILE, oldLimit); err != nil { + return fmt.Errorf("checking open file limit: %w", err) + } + // inherit existing limit + targetFileLimit := defaultFileLimit + if targetFileLimit > oldLimit.Max { + m.logger.Warnf("open file maximum (%d) lower than suggested open files (%d)", + oldLimit.Max, defaultFileLimit) + targetFileLimit = oldLimit.Max + } + // If the soft limit is lower than the defaultFileLimit constant, we will try to change it. + if oldLimit.Cur < targetFileLimit { + newLimit := &syscall.Rlimit{ + Cur: targetFileLimit, + Max: oldLimit.Max, + } + // Try to set the limit + if err := syscall.Setrlimit(syscall.RLIMIT_NOFILE, newLimit); err != nil { + return fmt.Errorf("setting open file limit: %w", err) + } + + // Check the limit after setting it. OS may not obey Setrlimit call. + if err := syscall.Getrlimit(syscall.RLIMIT_NOFILE, oldLimit); err != nil { + return fmt.Errorf("checking open file limit: %w", err) + } else { + if oldLimit.Cur != targetFileLimit { + m.logger.Warnf("tried to set open file limit to %d, but it is %d; see https://docs.molecula.cloud/reference/hostsystem#operating-system-configuration", targetFileLimit, oldLimit.Cur) + } + } + } + // We don't have corresponding options for non-Linux right now, but probably should. + if runtime.GOOS == "linux" { + result, err := ioutil.ReadFile("/proc/sys/vm/max_map_count") + if err != nil { + m.logger.Infof("Tried unsuccessfully to check system mmap limit: %w", err) + } else { + sysMmapLimit, err := strconv.ParseUint(strings.TrimSuffix(string(result), "\n"), 10, 64) + if err != nil { + m.logger.Infof("Tried unsuccessfully to check system mmap limit: %w", err) + } else if m.Config.MaxMapCount > sysMmapLimit { + m.logger.Warnf("Config max map limit (%v) is greater than current system limits (%v)", m.Config.MaxMapCount, sysMmapLimit) + } + } + } + return nil +} + +// setupResourceLimits tries to set up resource limits, like mmap limits +// and open files, if that hasn't been done already, and returns an error +// if the attempt failed in a way that we didn't anticipate. Mere permission +// denied errors are not that concerning. +func (m *Command) setupResourceLimits() error { + setupResourceLimitsOnce.Do(func() { + setupResourceLimitsErr = m.doSetupResourceLimits() + }) + return setupResourceLimitsErr +} + // Start starts the pilosa server - it returns once the server is running. func (m *Command) Start() (err error) { // Seed random number generator @@ -153,19 +229,9 @@ func (m *Command) Start() (err error) { if err != nil { return errors.Wrap(err, "setting up server") } - - if runtime.GOOS == "linux" { - result, err := ioutil.ReadFile("/proc/sys/vm/max_map_count") - if err != nil { - m.logger.Infof("Tried unsuccessfully to check system mmap limit: %v", err) - } else { - sysMmapLimit, err := strconv.ParseUint(strings.TrimSuffix(string(result), "\n"), 10, 64) - if err != nil { - m.logger.Infof("Tried unsuccessfully to check system mmap limit: %v", err) - } else if m.Config.MaxMapCount > sysMmapLimit { - m.logger.Warnf("Config max map limit (%v) is greater than current system limits (%v)", m.Config.MaxMapCount, sysMmapLimit) - } - } + err = m.setupResourceLimits() + if err != nil { + return errors.Wrap(err, "setting resource limits") } // Initialize server.