Dynamic LB sync non-external backends only when necessary #5418

ZxYuan · 2020-04-22T09:55:06Z

The PR records a timestamp in nginx shared configuration when API "/configuration/backends" is called by ingress controller. Every second each nginx worker checks whether it is necessary to sync non-external backends.

What this PR does / why we need it:

In our Kubernetes cluster with thousands of service and ingress resources, each of nginx workers syncing backends every second leads to quite heavy load for CPUs. In my testing environment with no input requests, impact on cpu usage is showed as below. Heavy CPU load also seriously affects request time of Biz APIs.
Although it is necessary to sync backends of services using ExternalName since DNS may change anytime, frequent sync of non-external backends can be avoided.

Types of changes

I'm not sure about the type of change, but how dynamic LB works internally is actually modified. Perhaps Breaking change?

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Which issue/s this PR fixes

How Has This Been Tested?

Create N ingress rules and corresponding N non-ExternalName services. Each ingress rule makes a route to one of the services. Without the patch, when N is large, each nginx worker runs out much of its CPU core as the figure above shows.

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have added tests to cover my changes.
All new and existing tests passed.

k8s-ci-robot · 2020-04-22T09:55:14Z

Hi @ZxYuan. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

k8s-ci-robot · 2020-04-22T09:55:41Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ZxYuan
To complete the pull request process, please assign elvinefendi
You can assign the PR to them by writing /assign @elvinefendi in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

ElvinEfendi · 2020-04-22T12:28:19Z

@ZxYuan can you share some more numbers please, ideally graphs? I'd like to see how much impact does this change have on CPU usage.

aledbf · 2020-04-22T12:42:50Z

/ok-to-test

aledbf · 2020-04-22T12:42:58Z

/assign @ElvinEfendi

ElvinEfendi · 2020-04-22T12:44:32Z

rootfs/etc/nginx/lua/balancer.lua

@@ -36,6 +36,8 @@ local IMPLEMENTATIONS = {

 local _M = {}
 local balancers = {}
+local external_backends = {}


What about backends_with_external_name?

ElvinEfendi · 2020-04-22T12:45:58Z

rootfs/etc/nginx/lua/balancer.lua

@@ -36,6 +36,8 @@ local IMPLEMENTATIONS = {

 local _M = {}
 local balancers = {}
+local external_backends = {}
+local last_timestamp = 0


I think this should be more specific and indicate that it is specifically about backend sync - what about backends_last_synced_at?

ElvinEfendi · 2020-04-22T12:47:07Z

rootfs/etc/nginx/lua/balancer.lua

@@ -92,6 +94,12 @@ local function format_ipv6_endpoints(endpoints)
  return formatted_endpoints
 end

+local function use_external_name(backend)


Similar to https://github.com/kubernetes/ingress-nginx/pull/5418/files#r412948805, I'd go with is_backend_with_external_name.

ElvinEfendi · 2020-04-22T12:54:50Z

rootfs/etc/nginx/lua/balancer.lua

+    end
+    return
+  end
+  last_timestamp = timestamp


Are you deliberately updating this in the beginning of function body to avoid retries in case of i.e json decoding error happens? Intuitively I'd have expected this update to be in the end of the function body. I wonder how we can make the intention clearer for the reader.

ElvinEfendi · 2020-04-22T12:56:22Z

rootfs/etc/nginx/lua/configuration.lua

@@ -17,6 +17,14 @@ function _M.get_general_data()
  return configuration_data:get("general")
 end

+function _M.get_timestamp_data()


Similar to https://github.com/kubernetes/ingress-nginx/pull/5418/files#r412949876, get_raw_backends_last_sycned_at.

ElvinEfendi · 2020-04-22T12:57:05Z

rootfs/etc/nginx/lua/configuration.lua

@@ -17,6 +17,14 @@ function _M.get_general_data()
  return configuration_data:get("general")
 end

+function _M.get_timestamp_data()
+  local timestamp = configuration_data:get("timestamp")


s/timestamp/raw_backends_last_sycned_at

ElvinEfendi · 2020-04-22T12:57:34Z

rootfs/etc/nginx/lua/configuration.lua

@@ -177,6 +185,14 @@ local function handle_backends()
    return
  end

+  local timestamp = os.time()


Why not ngx.update_time and ngx.time?

ElvinEfendi · 2020-04-22T12:58:35Z

rootfs/etc/nginx/lua/configuration.lua

+  local timestamp = os.time()
+  success, err = configuration_data:set("timestamp", timestamp)
+  if not success then
+    ngx.log(ngx.ERR, "dynamic-configuration: error setting sync timestamp: " .. tostring(err))


Please include some information in the error message about the potential consequences of not being able to update this timestamp.

ElvinEfendi · 2020-04-22T13:02:17Z

rootfs/etc/nginx/lua/balancer.lua

@@ -129,6 +135,15 @@ local function sync_backend(backend)
 end

 local function sync_backends()
+  local timestamp = configuration.get_timestamp_data()
+  if timestamp <= last_timestamp then


Should we force sync if last_timestamp - timestamp is too large?

Do you mean we should force sync if current timestamp is too larger than backends_last_sycned_at stored in each worker, avoiding failure of backends syncing caused by error updating raw_backends_last_sycned_at in shared configuration data?

ZxYuan · 2020-04-23T17:20:58Z

@ZxYuan can you share some more numbers please, ideally graphs? I'd like to see how much impact does this change have on CPU usage.

Thanks for your nice suggestions. I'll give a patch later.
The figures below show how much impact current LB has on CPU usage. Current version=0.30.0 seems a little better than what I mentioned, but still has the issue.

1000 services (or ingresses)

2000 services (or ingresses)

3000 services (or ingresses)

4000 services (or ingresses)

5000 services (or ingresses)

6000 services (or ingresses)

…terval

…tyle

k8s-ci-robot · 2020-04-28T06:29:39Z

@ZxYuan: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-ingress-nginx-e2e-1-17	`7d4aea1`	link	`/test pull-ingress-nginx-e2e-1-17`
pull-ingress-nginx-e2e-1-15	`7d4aea1`	link	`/test pull-ingress-nginx-e2e-1-15`
pull-ingress-nginx-e2e-1-18	`7d4aea1`	link	`/test pull-ingress-nginx-e2e-1-18`
pull-ingress-nginx-e2e-1-16	`7d4aea1`	link	`/test pull-ingress-nginx-e2e-1-16`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

aledbf · 2020-04-28T23:15:22Z

@ZxYuan do you have a script to generate this scenario?

ZxYuan · 2020-04-30T07:02:13Z

@ZxYuan do you have a script to generate this scenario?

FYI. Create a nginx deployment on port 80 with label app: nginx
Save the following yaml as press.yaml.tmpl

kind: Service
apiVersion: v1
metadata:
  name: service-press-HAHA
spec:
  selector:
    app: nginx
  ports:
    - protocol: TCP
      port: 80
---
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: ingress-press-HAHA
spec:
  rules:
  - http:
      paths:
      - path: /press/HAHA
        backend:
          serviceName: service-press-HAHA
          servicePort: 80

Run the bash script

for i in {1..6000}
do
    sed s#HAHA#${i}#g press.yaml.tmpl > tmp.yaml
    kubectl apply -f tmp.yaml
done

lua sync non-external backends only when necessary

7e5b756

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 22, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 22, 2020

k8s-ci-robot requested review from cmluciano and ElvinEfendi April 22, 2020 09:55

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 22, 2020

k8s-ci-robot assigned ElvinEfendi Apr 22, 2020

ElvinEfendi reviewed Apr 22, 2020

View reviewed changes

ZxYuan added 2 commits April 28, 2020 13:24

lua sync non-external backends only when necessary: add force sync in…

5c78d37

…terval

lua sync non-external backends only when necessary: fix typo & code s…

7d4aea1

…tyle

aledbf mentioned this pull request Jun 16, 2020

Dynamic LB sync non-external backends only when necessary #5726

Merged

8 tasks

aledbf closed this in #5726 Jun 29, 2020

ZxYuan mentioned this pull request Jul 5, 2020

Nginx ingress controller动态负载均衡性能优化 ZxYuan/zxyuan.me#27

Open

ZxYuan mentioned this pull request Nov 22, 2021

Remove force sync for non-external backends #7958

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic LB sync non-external backends only when necessary #5418

Dynamic LB sync non-external backends only when necessary #5418

ZxYuan commented Apr 22, 2020 •

edited

Loading

k8s-ci-robot commented Apr 22, 2020

k8s-ci-robot commented Apr 22, 2020

ElvinEfendi commented Apr 22, 2020

aledbf commented Apr 22, 2020

aledbf commented Apr 22, 2020

ElvinEfendi Apr 22, 2020

ElvinEfendi Apr 22, 2020

ElvinEfendi Apr 22, 2020

ElvinEfendi Apr 22, 2020

ElvinEfendi Apr 22, 2020 •

edited

Loading

ElvinEfendi Apr 22, 2020

ElvinEfendi Apr 22, 2020

ElvinEfendi Apr 22, 2020 •

edited

Loading

ElvinEfendi Apr 22, 2020

ZxYuan Apr 24, 2020

ZxYuan commented Apr 23, 2020

k8s-ci-robot commented Apr 28, 2020

aledbf commented Apr 28, 2020

ZxYuan commented Apr 30, 2020

Dynamic LB sync non-external backends only when necessary #5418

Dynamic LB sync non-external backends only when necessary #5418

Conversation

ZxYuan commented Apr 22, 2020 • edited Loading

What this PR does / why we need it:

Types of changes

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

k8s-ci-robot commented Apr 22, 2020

k8s-ci-robot commented Apr 22, 2020

ElvinEfendi commented Apr 22, 2020

aledbf commented Apr 22, 2020

aledbf commented Apr 22, 2020

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

ElvinEfendi Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZxYuan commented Apr 23, 2020

k8s-ci-robot commented Apr 28, 2020

aledbf commented Apr 28, 2020

ZxYuan commented Apr 30, 2020

ZxYuan commented Apr 22, 2020 •

edited

Loading

ElvinEfendi Apr 22, 2020 •

edited

Loading

ElvinEfendi Apr 22, 2020 •

edited

Loading