enable dynamic backend configuration by default #2794
Conversation
k8s-ci-robot
added
size/S
aledbf
added
do-not-merge/hold
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
#2797 has been a significant regression for us - Making this flag the default with that outstanding seems like a bad idea? At the very least it should be called out in the release notes. |
|
/lgtm |
This will be added to the release notes. |
Actually, this is a good idea. We just need to add the missing e2e tests (like externalName) to make sure there are no regressions. |
|
@mikebryant if you have the time and are willing to help you can add such e2e test in a new PR |
| @@ -77,7 +77,7 @@ const ( | |||
| sslSessionCacheSize = "10m" | |||
|
|
|||
| // Default setting for load balancer algorithm | |||
| defaultLoadBalancerAlgorithm = "least_conn" | |||
| defaultLoadBalancerAlgorithm = "" | |||
There was a problem hiding this comment.
@diazjf technically it would not matter in our case since we have a guard in buildLoadBalancingConfig to return "" when it is round_robin, but I'd like to stick to Nginx standard. And Nginx does not let you configure it as round_robin inside an upstream section. You would get:
unknown directive "round_robin"
if you do so.
|
Worth noting https://kubernetes.github.io/ingress-nginx/how-it-works/ would benefit from an update after this gets merged. |
|
Similarly to the discussion above, #2513 is a significant issue for us and should either be fixed before this becomes the default or should be documented as a breaking regression when consuming this version. |
|
/lgtm cancel Considering the edges cases uncovered by users I think we should be careful and either:
|
k8s-ci-robot
removed
the
lgtm
|
I am already addressing the one related to externa name service. I will then address custom error one before getting back to this PR. I don’t think we should merge this without addressing those two issues. |
|
These are only the ones reported by users, I'm almost certain there will be more. Ideally we should run the entire e2e test suite for both modes, but e2e tests are also incomplete at the moment. |
👍 I'll adjust it accordingly in another PR. @antoineco regarding to #2794 (comment), I think users have been running dynamic mode since few versions ago already and they have reported several issues the two above-mentioned being the recent ones which they are fixed now as well. I like the idea of highlighted message in release notes to make sure people are aware of this big change. Given above I think we can make this part of 0.18 release. |
+1 |
|
/hold let's wait for @antoineco |
k8s-ci-robot
added
the
do-not-merge/hold
|
After discussing with Elvin, I'm in favor of merging this and drafting a beta. /lgtm |
k8s-ci-robot
added
the
lgtm
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
k8s-ci-robot
added
size/M
| var _ = framework.IngressNginxDescribe("Annotations - Affinity", func() { | ||
| f := framework.NewDefaultFramework("affinity") | ||
|
|
||
| BeforeEach(func() { | ||
| err := f.NewEchoDeploymentWithReplicas(2) | ||
| err := f.DisableDynamicConfiguration() | ||
| Expect(err).NotTo(HaveOccurred()) |
There was a problem hiding this comment.
This is a known limitation of affinity implementation in dynamic mode as described at #2662. The core affinity features are supported in dynamic mode and there are tests for them at test/e2e/lua/dynamic_configuration.go. It is only https://github.com/kubernetes/ingress-nginx/pull/2794/files#diff-7d332a0e5ba59e06193678d5a83909d0R102 that's not supported yet as described in the issue.
|
The CI will keep failing until #2853 gets merged. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aledbf, antoineco, ElvinEfendi 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 |
Codecov Report
@@ Coverage Diff @@
## master #2794 +/- ##
==========================================
+ Coverage 47.39% 47.41% +0.01%
==========================================
Files 75 75
Lines 5433 5433
==========================================
+ Hits 2575 2576 +1
Misses 2530 2530
+ Partials 328 327 -1
Continue to review full report at Codecov.
|
What this PR does / why we need it:
We have introduced dynamic backend configuration feature at #2174 on 18th March and since then improved the initial implementation and fixed bugs. Several people/companies have already been successfully running ingress-nginx with this feature enabled. In this mode ingress-nginx also provides a more advanced load balancing algorithm that can be enabled using
load-balance(set toewma) configmap setting or annotation. Howeverleast_connandip_hashare not available in this mode. Also note #2662.For an idea of how many reloads this feature can skip check out the following graph (EDT time):
As you can see during work hours on average 25 reloads every 5m. Note that this ingress-nginx instance has more than 500 upstreams and the app behind gets deployed dozens of times during working hours.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #Special notes for your reviewer: