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

Feature: Allow keepalive to be disabled when set to 0 #3716

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
2 participants
@pryorda
Copy link
Contributor

pryorda commented Aug 18, 2018

Summary

Allow Engineers to disable keeaplive by setting upstream_keepalive to 0. In our infrastructure we did not want to have nginx workers keeping the backend connections alive that were to externally lb services. This fixed various connection resets for us.

Full changelog

  • Implemented ability to disable upstream_keepalive

Issues resolved

Possibly: #3287

@pryorda

This comment has been minimized.

Copy link
Contributor Author

pryorda commented Aug 18, 2018

Let me know if I missed something. This is my first lua pull.

@thibaultcha
Copy link
Member

thibaultcha left a comment

Hi @pryorda!

Thank you for the patch! We'd be happy to merge it, but would need a few things out of you before that can happen:

  • Please address the review comments (including the need for unit tests)
  • Changes to the nginx config template must target the next branch, since they are breaking for everyone using a custom template; please rebase your patch on top of the next branch, and change the base branch of your PR

Thank you!

Dockerfile Outdated
WORKDIR /build
RUN apk add --no-cache openssl openssl-dev git
RUN make dev && make test

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Aug 19, 2018

Member

Please remove unrelated changes from this PR, such as this Dockerfile and the Makefile rule added below, along with the README change. Thank you!

keepalive ${{UPSTREAM_KEEPALIVE}};
> end

This comment was marked as resolved.

Copy link
@thibaultcha

thibaultcha Aug 19, 2018

Member

We would need you to provide a test for this behaviour. Nginx config template test can be found here: https://github.com/Kong/kong/blob/master/spec/01-unit/003-prefix_handler_spec.lua#L55

@@ -213,6 +213,8 @@
# preserved in the cache of each worker
# process. When this number is exceeded, the
# least recently used connections are closed.
# 0 will remove the entry conf nginx-kong.conf
# when using the default template

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Aug 19, 2018

Member

Will you please update this description to something like:

A value of `0` will disable this behavior altogether,
forcing each upstream request to open a new connection.

nginx-kong.conf and "the default template" are implementation details that should not be mentioned here. Additionally, the proposed text is more explicit as to what the behavior is when the given value is 0.

@pryorda pryorda changed the base branch from master to next Aug 20, 2018

@pryorda pryorda changed the base branch from next to master Aug 20, 2018

@pryorda pryorda changed the base branch from master to next Aug 20, 2018

@pryorda pryorda force-pushed the pryorda:feat/keapalive_disable branch from 37a3908 to b81ca71 Aug 20, 2018

@pryorda

This comment has been minimized.

Copy link
Contributor Author

pryorda commented Aug 20, 2018

@thibaultcha Should be good to go now. Let me know if I missed anything or did it wrong.

thibaultcha added a commit that referenced this pull request Aug 21, 2018

feat(conf) allow disabling upstream keepalive
When `upstream_keepalive = 0`, we do not inject the `keepalive`
directive in the Nginx config template's upstream block.

From #3716
@thibaultcha

This comment has been minimized.

Copy link
Member

thibaultcha commented Aug 21, 2018

@pryorda Manually merged to next with a few edits, thanks!

Will you be able to provide documentation for this change as well? Thank you!

@pryorda

This comment has been minimized.

Copy link
Contributor Author

pryorda commented Aug 21, 2018

@thibaultcha Created here: Kong/docs.konghq.com#827

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.