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

Use system fs.max-files as limits instead of hard-coded value #142

Merged
merged 1 commit into from
Jan 19, 2017
Merged
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
5 changes: 5 additions & 0 deletions controllers/nginx/pkg/cmd/controller/nginx.go
Original file line number Diff line number Diff line change
@@ -288,7 +288,12 @@ func (n NGINXController) OnUpdate(cmap *api.ConfigMap, ingressCfg ingress.Config
cfg.ServerNameHashMaxSize = serverNameHashMaxSize
}

// the limit of open files is per worker process
// and we leave some room to avoid consuming all the FDs available
maxOpenFiles := (sysctlFSFileMax() / cfg.WorkerProcesses) - 1024

return n.t.Write(config.TemplateConfig{
MaxOpenFiles: maxOpenFiles,
BacklogSize: sysctlSomaxconn(),
Backends: ingressCfg.Backends,
PassthroughBackends: ingressCfg.PassthroughBackends,
13 changes: 13 additions & 0 deletions controllers/nginx/pkg/cmd/controller/utils.go
Original file line number Diff line number Diff line change
@@ -39,6 +39,19 @@ func sysctlSomaxconn() int {
return maxConns
}

// sysctlFSFileMax returns the value of fs.file-max, i.e.
// maximum number of open file descriptors
func sysctlFSFileMax() int {
maxConns, err := sysctl.New().GetSysctl("fs/file-max")
if err != nil {
glog.Errorf("unexpected error reading system maximum number of open file descriptors (fs.file-max): %v", err)
// returning 0 means don't render the value
return 0
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a comment here that this means "don't render", not that the max will be 0 :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

done

}

return maxConns
Copy link
Member

Choose a reason for hiding this comment

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

Should we have some "padding" e.g. maxConns - 1024 ? Also, is worker_rlimit_nofile per worker process . I honestly just don't know - I'm happy to follow your lead here if you say "this is correct", just not sure I follow the logic of letting a single nginx process use up all the FDs available to the whole 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.

Excellent point.

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 honestly just don't know - I'm happy to follow your lead here if you say "this is correct", just not sure I follow the logic of letting a single nginx process use up all the FDs available to the whole system?

Actually this process can use just the available FDs in the system.
If nginx tries to use more than the available the log just will show "Too many open files" which means:

  • the current fs.file-max value is low
  • the node is small
  • there is a process using/leaking descriptors

This setting tries to use the available resources (if needed) and not require scale the pod just because we are using the default values and we cannot get more than a couple of thousand connections.

}

func diff(b1, b2 []byte) ([]byte, error) {
f1, err := ioutil.TempFile("", "a")
if err != nil {
2 changes: 2 additions & 0 deletions controllers/nginx/pkg/config/config.go
Original file line number Diff line number Diff line change
@@ -271,7 +271,9 @@ func NewDefault() Configuration {
return cfg
}

// TemplateConfig contains the nginx configuration to render the file nginx.conf
type TemplateConfig struct {
MaxOpenFiles int
BacklogSize int
Backends []*ingress.Backend
PassthroughBackends []*ingress.SSLPassthroughBackend
4 changes: 3 additions & 1 deletion controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl
Original file line number Diff line number Diff line change
@@ -3,7 +3,9 @@ daemon off;

worker_processes {{ $cfg.WorkerProcesses }};
pid /run/nginx.pid;
worker_rlimit_nofile 131072;
{{ if ne .MaxOpenFiles 0 }}
worker_rlimit_nofile {{ .MaxOpenFiles }};
{{ end}}

events {
multi_accept on;