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

Revert max-worker-connections default value #3660

Merged
merged 1 commit into from
Jan 13, 2019

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Jan 12, 2019

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 12, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 12, 2019
@ramnes
Copy link
Contributor

ramnes commented Jan 12, 2019

I'd be in favor of defining the same default value for max-worker-open-files. I don't think that it makes sense to have a max-worker-open-files depending on the system configuration with a hard coded max-worker-connections at the same time.

Also, the default limit of opened files is 1024 on most systems, so we should avoid using a higher value without checking the limit first, or the user might experience nasty things in production when scaling up.

|[max-worker-open-files](#max-worker-open-files)|int|0|
|[map-hash-bucket-size](#max-worker-connections)|int|64|
|[map-hash-bucket-size](#max-hash-bucket-szie)|int|64|
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
|[map-hash-bucket-size](#max-hash-bucket-szie)|int|64|
|[map-hash-bucket-size](#max-hash-bucket-size)|int|64|

@ramnes
Copy link
Contributor

ramnes commented Jan 12, 2019

How do we define what is an acceptable or non-acceptable default RAM usage? It doesn't seems bad to me that if the server is configured to have a huge maximum of opened files, Nginx leverages that; then if some users want less RAM usage on idle, we could just advise them to reduce max-worker-open-files at the cost of a concurrency penalty.

If we want to base all the default settings on the available RAM, I feel that it really gets much more difficult. Doing this properly would require a available-ram-usage-ratio option or something like, which would calculate a good value for worker_connections and worker_rlimit_nofile based on the memory size of one event, on the total available RAM, and on the system's maximum number of opened files.

@aledbf
Copy link
Member Author

aledbf commented Jan 12, 2019

How do we define what is an acceptable or non-acceptable default RAM usage?

Right now, any considerable deviation from what is provided now is not acceptable. Keep in mind many users have CPU/RAM limit in place. A change like this one could break existing deployments

Edit: the first part sounds a bit extreme 😄

@ramnes
Copy link
Contributor

ramnes commented Jan 12, 2019

Makes sense. What would you think of 1/ putting that default value on max-worker-open-files but keeping 0 on max-worker-connections, and 2/ adding a check on top of this diff to verify that the value is not superior to the system maximum number of opened files, and if it is, to use that maximum? That would answer my concerns from my first comment here.

@aledbf
Copy link
Member Author

aledbf commented Jan 12, 2019

@ramnes right now we need to release 0.22. After that, we can try to improve the defaults but only max-worker-connections affects memory usage.

@ramnes
Copy link
Contributor

ramnes commented Jan 12, 2019

If you put 16384 as default for max-worker-open-files and keep max-worker-connections to 0, it will have the desired effect on RAM usage since max-worker-connections will take max-worker-open-files value.

@aledbf
Copy link
Member Author

aledbf commented Jan 12, 2019

If you put 16384 as default for max-worker-open-files and keep max-worker-connections to 0, it will have the desired effect on RAM usage since max-worker-connections will take max-worker-open-files value.

As I said, we can review this after the release. Right now I need at least two days in production before releasing a new version. I prefer to revert one default value than changing two. We already know how it behaves with the change in this PR.

@@ -362,8 +362,10 @@ _References:_
## max-worker-connections

Sets the [maximum number of simultaneous connections](http://nginx.org/en/docs/ngx_core_module.html#worker_connections) that can be opened by each worker process.
The default of 0 uses the value of [max-worker-open-files](#max-worker-open-files).
_**default:**_ 0
_**default:**_ 16384
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest keeping an explanation on what 0 means, even if it's not the default anymore:

Suggested change
_**default:**_ 16384
0 will use the value of [max-worker-open-files](#max-worker-open-files).
_**default:**_ 16384

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. done

@aledbf aledbf merged commit b10b60f into kubernetes:master Jan 13, 2019
@aledbf aledbf deleted the revert-mwc branch January 13, 2019 14:00
towolf pushed a commit to towolf/ingress-nginx that referenced this pull request Jun 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants