Fix MaxWorkerOpenFiles calculation on high cores nodes

[nanorobocop]

Re-open of my stale PR #5627.
Added tests and fixed doc.

What this PR does / why we need it:

Default MaxWorkerOpenFiles (max-worker-open-files) value becomes too low on high spec nodes with multiple of cores.
As workaround it always possible to provide exact value through configMap: https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/configmap/#max-worker-open-files

Currently max opened files is calculated based on RLIMIT_NOFILE divided on amount of working processes.

ingress-nginx/internal/ingress/controller/nginx.go

Lines 539 to 544 in 07b70f6

	maxOpenFiles := (rlimitMaxNumFiles() / wp) - 1024
	klog.V(3).Infof("Maximum number of open file descriptors: %d", maxOpenFiles)
	if maxOpenFiles < 1024 {
	// this means the value of RLIMIT_NOFILE is too low.
	maxOpenFiles = 1024
	}

Working processes is calculated based on amount of cores.

ingress-nginx/internal/ingress/controller/config/config.go

Line 751 in bef2efc

WorkerProcesses: strconv.Itoa(runtime.NumCPU()),

Our example:

$ sysctl fs.file-max
fs.file-max = 6506158
$ ulimit -Sn
65536
$ ulimit -Hn
65536
$ lscpu  | grep 'CPU(s)' | head -n 1
CPU(s):              40

As a result, we have 65536 / 40 - 1024 = 614.4. This is less than 1024, so 1024 is actually used.
This value is too small for single worker on heavy loaded server with thousands total connections.

Since RLIMIT_NOFILE is already a value per single process, there's no need divide by worker_processes.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Which issue/s this PR fixes

• Fix MaxWorkerOpenFiles calculation on high cores nodes #5627 - my previous stale PR about the same
• Set min value of maxOpenFiles as 64000 #2055 - similar problem
• Wrong number of open file calculated #2157 - similar problem
• Get file max from fs/file-max. #2050 (this tries use Global value of max opened files)
• Revert "Get file max from fs/file-max. (#2050)" #2241 (this reverts that change and per-process value of max opened files)

How Has This Been Tested?

E2e test

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I've read the CONTRIBUTION guide
• I have added tests to cover my changes.
• All new and existing tests passed.

[k8s-ci-robot]

Hi @nanorobocop. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[tao12345666333]

/ok-to-test

[nanorobocop]

/retest

[tao12345666333]

error log:

    nginx_test.go:154: creating tcp listener: listen tcp :10246: bind: address already in use
--- FAIL: TestConfigureDynamically (0.00s)
=== RUN   TestConfigureCertificates
    nginx_test.go:308: creating tcp listener: listen tcp :10246: bind: address already in use
--- FAIL: TestConfigureCertificates (0.00s)

[nanorobocop]

I guess that's some flaky test. It failed for me couple of times out of few dozens.

[nanorobocop]

/retest

[nanorobocop]

Hm, seems other PRs have same problem https://prow.k8s.io/?repo=kubernetes%2Fingress-nginx&job=pull-ingress-nginx-test

[nanorobocop]

Tests passed after I disabled parallel test execution 26e267a.
t.Parallel() not used anywhere in tests, so that should be OK.

[joostschriek]

hey 👋 I was peeking at this too, my PR has the same issues. The testgrid pointed me to this PR. I haven't looked at it yet, but it could be the source :)

EDIT man i should've looked at it before posting 🙃

[nanorobocop]

Hi @cmluciano, @rikatz, could you please help to review this PR?
Is anything else required to proceed?

[nanorobocop]

/assign @rikatz

[rikatz]

/lgtm
/approve

If you want this one also to be available in v0.X releases (the stable ones), please cherry pick this PR to branch release-v1beta1

Thanks!!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinmchase, nanorobocop, rikatz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [rikatz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment