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

enable reuse port by default #2466

Closed
wants to merge 1 commit into from

Conversation

rajatjindal
Copy link
Contributor

What this PR does / why we need it:

This enables the reuse port by default as https://trac.nginx.org/nginx/ticket/1300 seems to be fixed.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

N/A

Special notes for your reviewer:

I tested it using steps on the ticket with ingress controller version 0.14.0, with nginx/1.13.12 and it seems to be working fine.

screen shot 2018-05-03 at 9 29 08 am

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 3, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rajatjindal
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: justinsb

Assign the PR to them by writing /assign @justinsb in a comment when ready.

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 size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 3, 2018
@codecov-io
Copy link

Codecov Report

Merging #2466 into master will decrease coverage by 0.12%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2466      +/-   ##
==========================================
- Coverage   41.75%   41.62%   -0.13%     
==========================================
  Files          74       74              
  Lines        5291     5292       +1     
==========================================
- Hits         2209     2203       -6     
- Misses       2786     2792       +6     
- Partials      296      297       +1
Impacted Files Coverage Δ
internal/ingress/controller/config/config.go 98.23% <100%> (+0.01%) ⬆️
cmd/nginx/main.go 21.37% <0%> (-4.83%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87d1b8b...8efb991. Read the comment docs.

@aledbf
Copy link
Member

aledbf commented May 3, 2018

@rajatjindal before a change in this default we need to make sure do not introduce this issue again #1905
So for that, we need some e2e tests that check this.

@aledbf aledbf added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 3, 2018
@JordanP
Copy link
Contributor

JordanP commented May 9, 2018

Just to make sure, is the e2e test supposed to check whether "reuse" is present in the Nginx conf or do we aim to have a non regression test for https://trac.nginx.org/nginx/ticket/1300 ?

@aledbf
Copy link
Member

aledbf commented May 9, 2018

or do we aim to have a non regression test for https://trac.nginx.org/nginx/ticket/1300 ?

Not sure how we can do that.

I am not sure this PR should be merged. The user must be aware of this potential issue and turn on the feature with the configmap

@JordanP
Copy link
Contributor

JordanP commented May 9, 2018

I see. The configmap is always a good choice.

But this was a bug in Nginx and this bug is now fixed, so I am not sure what justifies the change in default (from reuse to noreuse) now.

That being said, I still think we should default to noreuse because "it feels" the safest.

@aledbf
Copy link
Member

aledbf commented May 10, 2018

Closing. The side effect of this setting is too high to change the default. This can enabled in the configuration configmap

@aledbf aledbf closed this May 10, 2018
@rajatjindal
Copy link
Contributor Author

thanks for the feedback guys, it totally make sense.

@rajatjindal rajatjindal deleted the enable-reuse-port branch May 11, 2018 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants