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

Modify bin/apisix to support the SO_REUSEPORT #1085

Merged
merged 7 commits into from
Feb 9, 2020

Conversation

Miss-you
Copy link
Member

NOTE: Please read the Contributing.md guidelines before submitting your patch:

https://github.com/apache/incubator-apisix/blob/master/Contributing.md#how-to-add-a-new-feature-or-change-an-existing-one

Summary

Modify bin/apisix to increase the capacity so that the generated nginx.conf file can open the reuseport configuration
Judgment logic:
1. If it is a mac system, do not open reuseport
2. If it is a linux system and the kernel version is higher than 3.9.0, then turn on the reuserport switch, and the reuseport option will be added when generating nginx.conf> configuration

Full changelog

bin/apisix

Issues resolved

Fix #342

…generated nginx.conf file can open the reuseport configuration

    Judgment logic:
    1. If it is a mac system, do not open reuseport
    2. If it is a linux system and the kernel version is higher than 3.9.0, then turn on the reuserport switch, and the reuseport option will be added when generating nginx.conf> configuration
bin/apisix Outdated
{% end %}

{% else %} {% --if enable_reuseport %}

{% for _, port in ipairs(stream_proxy.tcp or {}) do %}
listen {*port*};
Copy link
Member

Choose a reason for hiding this comment

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

listen {*port*} {% if enable_reuseport then %} reuseport {% end %};

This style is simpler.

Copy link
Member

@moonming moonming left a comment

Choose a reason for hiding this comment

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

please add test cases.

Test cases to be added after pull request # 1063

apache#1063
@Miss-you
Copy link
Member Author

Miss-you commented Jan 22, 2020

@moonming Test cases will be added after pull request # 1063
#1063

@membphis
Copy link
Member

@Miss-you #1063 had been merged already.

@spacewander
Copy link
Member

Strictly speaking, detect if a feature is available or not via kernel version is not reliable. Linux distributions may backport kernel patch to an old release (search for "SO_REUSEPORT backport for linux distribution"), not even mentioning self-patched kernel.

I think a better solution is to allow the users to configure it, since they know better about their servers than us. Maybe we can enable this feature by default, because most of the servers have kernel newer than 3.9.

@Miss-you
Copy link
Member Author

Miss-you commented Feb 3, 2020

Considering that the kernel version of the current production environment basically supports reuseport, I think it is more reasonable to open the REUSEPORT configuration file by default and supporting configuration it.

@Miss-you
Copy link
Member Author

Miss-you commented Feb 3, 2020

I remember that there are two main pits in reuseport, one is the kernel version, and the other is the support of http2 and http3.

@membphis
Copy link
Member

membphis commented Feb 3, 2020

open the REUSEPORT configuration file by default and supporting configuration it.

this way is simpler. thx @spacewander

@Miss-you Expect your new commit ^_^

…uction environment basically supports reuseport, I think it is more reasonable to open the REUSEPORT configuration file by default and supporting configuration it.

So, it's a better solution is to allow the users to configure the 'enable_reuseport'.
@Miss-you
Copy link
Member Author

Miss-you commented Feb 5, 2020

@membphis @spacewander
hi, I have add the 'enable_reuseport' config into config.yaml. Please take a look.

bin/apisix Outdated
@@ -507,6 +507,7 @@ local function init()
with_module_status = with_module_status,
node_ssl_listen = 9443, -- default value
error_log = {level = "warn"},
enable_reuseport = true, -- default true
Copy link
Member

Choose a reason for hiding this comment

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

the default value should be set in config.yaml, not here.

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

bin/apisix Outdated
Comment on lines 534 to 537
if sys_conf["enable_reuseport"] == false then
enable_reuseport = false
end

Copy link
Member

Choose a reason for hiding this comment

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

we can use the enable_reuseport in config.yaml

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

@membphis
Copy link
Member

membphis commented Feb 6, 2020

waiting your test case ^_^

update apache/incubator-apisix
conf/config.yaml Outdated
@@ -21,7 +21,7 @@ apisix:
enable_admin_cors: true # Admin API support CORS response headers.
enable_debug: false
enable_dev_mode: false # Sets nginx worker_processes to 1 if set to true
enable_reuseport: true # Enable nginx SO_REUSEPORT switch if set to true.
enable_reuseport: false # Enable nginx SO_REUSEPORT switch if set to true.
Copy link
Member

Choose a reason for hiding this comment

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

default value true is better

Copy link
Member Author

Choose a reason for hiding this comment

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

setting enable_reuseport false to verify that test cases are working.

Copy link
Member

Choose a reason for hiding this comment

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

For testing, you can write a separate script to modify config.yaml.

@Miss-you Miss-you force-pushed the master branch 2 times, most recently from 2b61386 to 56e1d97 Compare February 7, 2020 02:29
@Miss-you Miss-you force-pushed the master branch 10 times, most recently from 5a7ec3d to 62dc89d Compare February 8, 2020 04:01
…_runner.sh

1.fix issue 'can not find openresty'
2.remove function 'check_result'
3.fix .gitignore
@membphis
Copy link
Member

membphis commented Feb 8, 2020

@membphis
Copy link
Member

membphis commented Feb 9, 2020

ok, we can merge it to master branch first for checking it can work fine.

@membphis membphis merged commit 60b2493 into apache:master Feb 9, 2020
SaberMaster pushed a commit to SaberMaster/incubator-apisix that referenced this pull request Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI: check linux kernel and enable REUSEPORT if possible.
4 participants