Configurable load balancing with EWMA #2229

zrdaley · 2018-03-21T14:53:53Z

What this PR does / why we need it:

This PR is an extension of #2174 which enables optional dynamic configuration via the --enable-dynamic-configuration flag. This allows NGINX upstreams/K8 endpoints can be updated without reloading NGINX.

The purpose of this PR is to add EWMA (exponential weighted moving average) as a configurable load balancing algorithm. EWMA load balancing uses a moving average of the roundtrip time of an upstream in order to determine where to forward a request. Using EWMA has the potential to decrease queueing delays and response time for ends users.

Which issue this PR fixes: n/a

k8s-ci-robot · 2018-03-21T14:54:13Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

zrdaley · 2018-03-21T15:05:31Z

/assign @nicksardo

aledbf · 2018-03-21T15:19:20Z

@zrdaley thank you for adding this feature.
Please add the new lb algorithm in the docs https://github.com/kubernetes/ingress-nginx/blob/master/docs/user-guide/configmap.md#load-balance

aledbf · 2018-03-21T15:21:59Z

rootfs/etc/nginx/template/nginx.tmpl

@@ -42,6 +42,8 @@ http {
    lua_shared_dict configuration_data 5M;
    lua_shared_dict round_robin_state 1M;
    lua_shared_dict locks 512k;
+    lua_shared_dict balancer_ewma 1M;
+    lua_shared_dict balancer_ewma_last_touched_at 1M;


Is this size enough?
Do you have an estimation of how many endpoints fit in 1M?
What happens if the dict is full?

@shivnagarajan Do you know the answer to any of these questions?

@zrdaley you can do some back of the napkin math here. balancer_ewma holds key value data where keys are upstream names(strings) and values are EWMAs(float value - number in Lua). Similarly for balancer_ewma_last_touched_at where keys are the same and values are time(I think this will be stored as string, refer to https://github.com/openresty/lua-nginx-module#ngxshareddictset).

What happens if the dict is full?

When the dict is full we fail to store the EWMA values which means the load balancing will start misbehaving.

balancer_ewma and balancer_ewma_last_touched_at are both dictionaries that have a string key and floating-point value.

Do you have an estimation of how many endpoints fit in 1M?

Each dict entry
~10 char key + float = 4B*10 + 4B
= 44B = 0.000044MB

Total entries
1MB/0.000044MB = 22,727

What happens if the dict is full?

When the dict is full, values will be overwritten or fail to write entirely. I created nginx logging around round_robin_state, balancer_ewma and balancer_ewma_last_touched_at for both of these cases (see round_robin_state example).

The full ngx.shared.DICT.set docs can be found here.

ElvinEfendi · 2018-03-21T15:31:54Z

rootfs/etc/nginx/lua/balancer.lua

  else
-    ngx.log(ngx.ERR, "error while setting current upstream peer to: " .. tostring(err))
+    return error('must be called in balancer or log, but was called in: ' .. phase)


nitpick: please use double quote for string here to be consistent with the the rest of code base.

ElvinEfendi · 2018-03-21T15:35:14Z

rootfs/etc/nginx/lua/balancer.lua

+    else
+      ngx.log(ngx.ERR, "error while setting current upstream peer to: " .. tostring(err))
+    end
+  elseif phase == "log" then


style: instead of using if .. elseif construct please use something like

if phase == "log" then after_balance() return end if phase ~= "balancer" then error('must be called in balancer or log, but was called in: ' .. phase) return end ...

ElvinEfendi · 2018-03-21T15:35:29Z

rootfs/etc/nginx/lua/balancer.lua


-  local host, port = balance()
+    local host, port = balance()


redundant space

oilbeater · 2018-03-21T16:24:04Z

maybe we should add the copyright header in every lua file

aledbf · 2018-03-21T17:00:49Z

maybe we should add the copyright header in every lua file

Yes. I will add an additional check for lua files

ElvinEfendi · 2018-03-21T17:06:15Z

rootfs/etc/nginx/lua/balancer/ewma.lua

+  end
+
+  ngx.shared.balancer_ewma_last_touched_at:set(upstream, now)
+  ngx.shared.balancer_ewma:set(upstream, ewma)


We should check for err in return values and log correspondingly.

ElvinEfendi · 2018-03-21T17:06:46Z

rootfs/etc/nginx/lua/balancer.lua

  local index, endpoint = next(backend.endpoints, last_index)
  if not index then
    index = 1
    endpoint = backend.endpoints[index]
  end
-  round_robin_state:set(backend_name, index)
-  round_robin_lock:unlock(backend_name .. ROUND_ROBIN_LOCK_KEY)
+  round_robin_state:set(backend.name, index)


we should check for return value err and log correspondingly

ElvinEfendi · 2018-03-21T17:08:50Z

docs/user-guide/configmap.md

@@ -518,6 +518,7 @@ The value can either be:
 - round_robin: to use the default round robin loadbalancer
 - least_conn: to use the least connected method
 - ip_hash: to use a hash of the server for routing.
+- ewma: to use the peak ewma method for routing 


Currently this LB algorithm is available only when dynamic configuration is enabled, we should somehow convey that info here.

aledbf · 2018-03-21T18:33:14Z

@zrdaley you need to run make code-generator and commit the changes to pass the first check in CI

codecov-io · 2018-03-21T19:21:42Z

Codecov Report

Merging #2229 into master will increase coverage by 0.11%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #2229      +/-   ##
==========================================
+ Coverage   37.02%   37.13%   +0.11%     
==========================================
  Files          71       71              
  Lines        5013     5025      +12     
==========================================
+ Hits         1856     1866      +10     
- Misses       2873     2874       +1     
- Partials      284      285       +1

Impacted Files	Coverage Δ
internal/file/bindata.go	`53.93% <83.33%> (+2.12%)`	⬆️

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 b0a63fe...0928e86. Read the comment docs.

aledbf · 2018-03-22T03:25:08Z

@zrdaley please squash the commits

aledbf · 2018-03-22T03:25:30Z

@ElvinEfendi please post if you are ok with this PR

ElvinEfendi · 2018-03-22T13:15:04Z

@ElvinEfendi please post if you are ok with this PR

@aledbf I'll take another look during the day.

ElvinEfendi · 2018-03-23T03:20:01Z

LGTM. Please squash the commits and rebase with master.

aledbf · 2018-03-23T15:06:11Z

/approve

aledbf · 2018-03-23T15:06:17Z

@zrdaley thanks!

k8s-ci-robot · 2018-03-23T15:06:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, zrdaley

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:

~~OWNERS~~ [aledbf]

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

* Correct typo (kubernetes#2238) * correct spelling * correct typo * fix-link (kubernetes#2239) * Add missing configuration in kubernetes#2235 (kubernetes#2236) * to kubernetes (kubernetes#2240) to kubernetes * fix: cannot set $service_name if use rewrite (kubernetes#2220) $path here is the regular expression formatted nginx location not the origin path in ingress rules. Fix kubernetes#2131 * Revert "Get file max from fs/file-max. (kubernetes#2050)" (kubernetes#2241) This reverts commit d8efd39. * add http/2 * fix: empty ingress path (kubernetes#2244) If the origin ingress rule has no field `path`, the default value will be an empty string which will cause issues when rendering template as other place will use `/` as the default value. Set the default value of path to `/` when retrieve ingress rules from api-server. Thie will fix kubernetes#1980 * Fix grpc json tag name (kubernetes#2246) * Add EWMA as configurable load balancing algorithm (kubernetes#2229) * Update go dependencies (kubernetes#2234) * Add deployment docs for AWS NLB (kubernetes#1785) * Update annotations.md (kubernetes#2255) a typo fix * Update README.md (kubernetes#2267) It should be "your Ingress targets" in line 7. * Managing a whitelist for _/nginx_status (kubernetes#2187) Signed-off-by: Sylvain Rabot <s.rabot@lectra.com> * Revert deleted assignment in kubernetes#2146 (kubernetes#2270) * Use SharedIndexInformers in place of Informers (kubernetes#2271) * clean up tmpl (kubernetes#2263) The nginx.conf generated now is too messy remove some section only useful when dynamic configure enabled and headers only useful for https. * Disable opentracing for nginx internal urls (kubernetes#2272) * Typo fixes in modsecurity.md (kubernetes#2274) * Update modsecurity.md Some typo fixes * Update modsecurity.md * Update go to 1.10.1 (kubernetes#2273) * Update README.md (kubernetes#2276) Small typo fix . * Fix bug when auth req is enabled(external authentication) (kubernetes#2280) * set proxy_upstream_name correctly when auth_req module is used * log a more meaningful message when backend is not found * Fix nlb instructions (kubernetes#2282) * e2e tests for dynamic configuration and Lua features and a bug fix (kubernetes#2254) * e2e tests for dynamic configuration and Lua features * do not rely on force reload to dynamically configure when reload is needed * fix misspelling * skip dynamic configuration in the first template rendering * dont error on first sync * Fix flaky e2e tests by always waiting after redeploying the ingress controller (kubernetes#2283) * Add NoAuthLocations and default it to "/.well-known/acme-challenge" (kubernetes#2243) * Add NoAuthLocations and default it to "/.well-known/acme-challenge" * Add e2e tests for no-auth-location * Improve wording of no-auth-location tests * Update controller.go (kubernetes#2285) * Fix custom-error-pages image publication script (kubernetes#2289) * Update nginx to 1.13.11 (kubernetes#2290) * Fix HSTS without preload (kubernetes#2294) * Disable dynamic configuration in s390x and ppc64le (kubernetes#2298) * Improve indentation of generated nginx.conf (kubernetes#2296) * Escape variables in add-base-url annotation * Fix race condition when Ingress does not contains a secret (kubernetes#2300) * include lua-resty-waf and its dependencies in the base Nginx image (kubernetes#2301) * install lua-resty-waf * bump version * include Kubernetes header * include the rest of lua-resty-waf dependencies (kubernetes#2303) * Fix issues building nginx image in different platforms (kubernetes#2305) * Disable lua waf where luajit is not available (kubernetes#2306) * Add verification of lua load balancer to health check (kubernetes#2308) * Configure upload limits for setup of lua load balancer (kubernetes#2309) * lua-resty-waf controller (kubernetes#2304) * annotation to ignore given list of WAF rulesets (kubernetes#2314) * extra waf rules per ingress (kubernetes#2315) * extra waf rules per ingress * document annotation nginx.ingress.kubernetes.io/lua-resty-waf-extra-rules * regenerate internal/file/bindata.go * run lua-resty-waf in different modes (kubernetes#2317) * run lua-resty-waf in different modes * update docs * Add ingress-nginx survey (kubernetes#2319) * Fix survey link (kubernetes#2321) * Update nginx to 1.13.12 (kubernetes#2327) * Update nginx image (kubernetes#2328) * Update nginx image * Update minikube start script * fix nil pointer when ssl with ca.crt (kubernetes#2331) * disable lua for arch s390x and ppc64le LuaJIT is not available for s390x and ppc64le, disable the lua part in nginx.tmpl on these platform. * Fix buildupstream name to work with dynamic session affinity * fix make verify-all failures * Add session affinity to custom load balancing * Fix nginx template * Fixed tests * Sync secrets (SSL certificates) on events Remove scheduled check for missing secrets. * Include missing secrets in secretIngressMap Update secretIngressMap independently from stored annotations, which may miss some secret references. * Add test for channel events with referenced secret * Release nginx ingress controller 0.13.0 * Update owners * Use same convention, curl + kubectl for GKE * Correct some returned messages in server_tokens.go should not exists->should not exist should exists->should exist * Typo fix in cli-arguments.md it's endpoints->its endpoints * Correct some info in flags.go Correct some info in flags.go * Add proxy-add-original-uri-header config flag This makes it configurable if a location adds an X-Original-Uri header to the backend request. Default is "true", the current behaviour. * Check ingress rule contains HTTP paths * Detect if header injected request_id before creating one * fix: fill missing patch yaml config. The patch-service yaml missing livenessProbe, readinessProbe and prometheus annotation parts. * Add vts-sum-key config flag * Introduce ConfigMap updating helpers into e2e/framework and retain default nginx-configuration state between tests Group sublogic * Update nginx image to fix modsecurity crs issues * Move the resetting logic into framework Stylistic fixes based on feedback * Fix leaky test * fix the default cookie name in doc * DOCS: Add clarification regarding ssl passthrough * Remove most of the time.Sleep from the e2e tests * Accept ns/name Secret reference in annotations * Document changes to annotations with Secret reference * Improve speed of e2e tests * include lua-resty-balancer in nginx image * Silence unnecessary MissingAnnotations errors * Ensure dep fix fsnotify * Update nginx image * fix flaky dynamic configuration test * shave off some more seconds * cleanup redundant code * Update go dependencies * Allow tls section without hosts in Ingress rule * Add test for store helper ListIngresses * Add tests for controller getEndpoints * Add busted unit testing framework for lua code * Add deployment instructions for Docker for Mac (Edge) * Update nginx-opentracing to 0.3.0 This version includes a new `http.host` header to make searching by vhost in zipkin or jaeger more trivial. * Fix golint installation * add balancer unit tests * Endpoint Awareness: Read backends data from tmp file as well Actually read from the file Logs probably shouldn't assume knowledge of implementation detail Typos Added integration test, and dynamic update config refactor Don't force the 8k default Minimal test case to make the configuration/backends request body write to temp file Leverage new safe config updating methods, and use 2 replicas instead of 4 Small refactor Better integration test, addresses other feedback Update bindata * Update nginx image * automate dev environment build * Remove unnecessary externalTrafficPolicy on Docker for Mac service * Apply gometalinter suggestions * Move all documentation under docs/ * Move miscellaneous tidbits from README to miscellaneous.md and other files * Fix some document titles * Move deployment documentation under docs/deploy/ * Remove empty ingress-annotations document; fix up annotations.md's layout slightly * Configure mkdocs with mkdocs-material and friends * Move "Customizing NGINX" documentation under "NGINX Configuration" * Regenerate cli-arguments.md from the actual usage of 0.13 * Remove default-ssl-certificate.md (the content is already in tls.md) * Move documents related to third-party extensions under third-party-addons * Add buffer configuration to external auth location config * make code-generator * Clean JSON before post request to update configuration * Add scripts and tasks to publish docs to github pages * Improve readme file * Fix broken links in the docs * Remove data races from tests * Check ginkgo is installed before running e2e tests * Update exposing-tcp-udp-services.md Minor tick missing for syntax highlighting which makes it look ugly on https://kubernetes.github.io/ingress-nginx/user-guide/exposing-tcp-udp-services/ * Update custom-errors.md Fix grammatical errors * Update README.md Fix broken link to `CONTRIBUTING.md`. Also update other links to `CONTRIBUTING.md` for consistency. * Add annotation to enable rewrite logs in a location * upstream-hash-by annotation support for dynamic configuraton mode * luacheck ignore subfolders too * Release nginx ingress controller 0.14.0 * Use local image name for e2e tests * Bump echoserver version used in e2e test (1.10) * Refactor e2e framework for TLS tests * Add tests for global TLS settings * improve build-dev-env.sh script * always use x-request-id * Add basic security context to deployment YAMLs * Update GitHub pull request template * Improve documentation format * Add google analytics [ci skip] * Add gRPC annotation doc * Adjust size of tables and only adjust the first column on mobile * Assert or install go-bindata before incanting * Add Getting the Code section to Quick Start * TLS.md: Move the TLS secret misc bit to the TLS document * TLS.md: Clarify how to set --default-ssl-certificate * TLS.md: Remove the frankly useless curl output in the default certificate section * TLS.md: Reformat and grammar check * TLS.md: Remove useless manual TOC * multiple-ingress.md: rework page for clarity and less repetition * Add upgrade documentation Closes kubernetes#2458 * Reformat log-format.md * Add note about changing annotation prefixes * Clean up annotations.md; extract default backend from miscellaneous * Index all examples and fix their titles * Example of using nginx-ingress with gRPC * Exclude grpc-fortune-teller from go list Deps are managed by bazel so these will fail to show up in the vendor tree, triggering false positive build fail. * Fixed broken link in deploy README * Change TrimLeft for TrimPrefix on the from-to-www redirect * use roundrobin from lua-resty-balancer library and refactor balancer.lua * upstream-hash-by should override load-balance annotation * add resty cookie * [ci skip] bump nginx baseimage version * Add some clarification around multiple ingress controller behavior * Update go version in fortune teller image * Refactor update of status removing initial check for loadbalancer * Add KubeCon Europe 2018 Video to documentation Adds Make Ingress-Nginx Work for you, and the Community Video to the documentation. * force backend sync when worker starts * Remove warning when secret is used only for authentication * Fix and simplify local dev workflow and execution of e2e tests * Release nginx ingress controller 0.15.0

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 21, 2018

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Mar 21, 2018

k8s-ci-robot assigned nicksardo Mar 21, 2018

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 21, 2018

aledbf assigned aledbf and unassigned nicksardo Mar 21, 2018

aledbf reviewed Mar 21, 2018

View reviewed changes

ElvinEfendi reviewed Mar 21, 2018

View reviewed changes

rootfs/etc/nginx/lua/balancer.lua Outdated

local host, port = balance()

local host, port = balance()

Copy link

Member

ElvinEfendi Mar 21, 2018

Choose a reason for hiding this comment

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

redundant space

zrdaley force-pushed the ewma-upstream branch 2 times, most recently from 659eaa8 to 34df36f Compare March 21, 2018 16:09

zrdaley force-pushed the ewma-upstream branch from ff51768 to d57fa46 Compare March 21, 2018 16:46

ElvinEfendi reviewed Mar 21, 2018

View reviewed changes

zrdaley force-pushed the ewma-upstream branch from d159a65 to f135471 Compare March 23, 2018 13:09

Add EWMA as configurable load balancing algorithm

0928e86

zrdaley force-pushed the ewma-upstream branch from f135471 to 0928e86 Compare March 23, 2018 13:22

aledbf merged commit 6e099c5 into kubernetes:master Mar 23, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 23, 2018

zrdaley deleted the ewma-upstream branch April 2, 2018 21:01

jizhuozhi mentioned this pull request Mar 17, 2023

L7 protocol supports latency-based load balancing algorithm mosn/mosn#2252

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configurable load balancing with EWMA #2229

Configurable load balancing with EWMA #2229

zrdaley commented Mar 21, 2018

k8s-ci-robot commented Mar 21, 2018

zrdaley commented Mar 21, 2018

aledbf commented Mar 21, 2018

aledbf Mar 21, 2018 •

edited

Loading

zrdaley Mar 21, 2018

ElvinEfendi Mar 21, 2018 •

edited

Loading

zrdaley Mar 21, 2018

zrdaley Mar 21, 2018

ElvinEfendi Mar 21, 2018

ElvinEfendi Mar 21, 2018

ElvinEfendi Mar 21, 2018

oilbeater commented Mar 21, 2018

aledbf commented Mar 21, 2018

ElvinEfendi Mar 21, 2018

ElvinEfendi Mar 21, 2018 •

edited

Loading

ElvinEfendi Mar 21, 2018

aledbf commented Mar 21, 2018

codecov-io commented Mar 21, 2018 •

edited

Loading

aledbf commented Mar 22, 2018

aledbf commented Mar 22, 2018

ElvinEfendi commented Mar 22, 2018

ElvinEfendi commented Mar 23, 2018

aledbf commented Mar 23, 2018

aledbf commented Mar 23, 2018

k8s-ci-robot commented Mar 23, 2018

Configurable load balancing with EWMA #2229

Configurable load balancing with EWMA #2229

Conversation

zrdaley commented Mar 21, 2018

k8s-ci-robot commented Mar 21, 2018

zrdaley commented Mar 21, 2018

aledbf commented Mar 21, 2018

aledbf Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oilbeater commented Mar 21, 2018

aledbf commented Mar 21, 2018

Choose a reason for hiding this comment

ElvinEfendi Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aledbf commented Mar 21, 2018

codecov-io commented Mar 21, 2018 • edited Loading

Codecov Report

aledbf commented Mar 22, 2018

aledbf commented Mar 22, 2018

ElvinEfendi commented Mar 22, 2018

ElvinEfendi commented Mar 23, 2018

aledbf commented Mar 23, 2018

aledbf commented Mar 23, 2018

k8s-ci-robot commented Mar 23, 2018

aledbf Mar 21, 2018 •

edited

Loading

ElvinEfendi Mar 21, 2018 •

edited

Loading

ElvinEfendi Mar 21, 2018 •

edited

Loading

codecov-io commented Mar 21, 2018 •

edited

Loading