fix: keep healthcheck target state when upstream changes by juststillthinking · Pull Request #10312 · apache/apisix

juststillthinking · 2023-10-10T13:03:56Z

Description

When we modify the upstream (adding or deleting nodes), APISIX first deletes the health check object and then recreates the health check object. The process of deletion and recreation may affect other parallel requests searching for healthy nodes, leading to the following error:

So we will replace checker:clear() with function checker:delayed_clear(). This function marks all targets to be removed, but do not actually remove them. If before the delay parameter any of them is re-added, it is unmarked for removal.
This function makes it possible to keep target state during config changes, where the targets might be removed and then re-added.

It's very hard to add test cases. I tried but failed.

Follow these steps to reproduce:

create a router

curl http://127.0.0.1:9180/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
{
    "uri": "/get",
    "upstream_id": 1
}'

Keep updating the upstream object

curl http://127.0.0.1:9180/apisix/admin/upstreams/1 \
-H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
{
    "type": "chash",
    "key": "remote_addr",
    "nodes": {
        "127.0.0.1:8080": 1,
        "127.0.0.1:8081": 1,
        "127.0.0.1:8082": 1,
        "127.0.0.1:8083": 1,
        "127.0.0.1:8084": 1,
        "127.0.0.1:8085": 1,
        "127.0.0.1:8086": 1,
        "127.0.0.1:8087": 1,
        "127.0.0.1:8088": 1,
        "127.0.0.1:8089": 1,
        "127.0.0.1:8090": 1
    },
    "retries": 2,
    "checks": {
        "active": {
            "timeout": 5,
            "http_path": "/status",
            "healthy": {
                "interval": 2,
                "successes": 1
            },
            "unhealthy": {
                "interval": 1,
                "http_failures": 2
            }
        }
    }
}'

Keep sending requests

wrk -c 10 -t 5 -d 500s -R 200 http://127.0.0.1:9080/get

Pay attention to the error.log

Fixes # (issue)

Checklist

I have explained the need for this PR and the problem it solves
I have explained the changes or the new features added to this PR
I have added tests corresponding to this change
I have updated the documentation to reflect this change
I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

shreemaan-abhishek · 2023-10-10T19:58:23Z

  error_log: logs/error.log       # Location of the error log.
  error_log_level:  warn          # Logging level: info, debug, notice, warn, error, crit, alert, or emerg.
-  worker_processes: auto          # Automatically determine the optimal number of worker processes based
+  worker_processes: 4          # Automatically determine the optimal number of worker processes based


Why do you change this?

shreemaan-abhishek · 2023-10-10T19:59:47Z

Keep updating the upstream object

Do I need to add/remove nodes from upstream frequently?

juststillthinking · 2023-10-11T04:13:38Z

Do I need to add/remove nodes from upstream frequently?

Yeah, but keep setting the same upstream is ok

juststillthinking · 2023-10-11T04:24:59Z

    end

-    item.clean_handlers = nil
+    item.clean_handlers = {}


if the clean_handler is put into the parents (routers), it will cause error like this:

2023/10/10 20:53:16 [error] 1057074#1057074: *2020 lua entry thread aborted: runtime error: /home/liuwei/api7/apisix/apisix/core/config_util.lua:61: attempt to index field 'clean_handlers' (a nil value) stack traceback: coroutine 0: /home/liuwei/api7/apisix/apisix/core/config_util.lua: in function 'add_clean_handler' /home/liuwei/api7/apisix/apisix/upstream.lua:152: in function 'fetch_healthchecker' /home/liuwei/api7/apisix/apisix/upstream.lua:342: in function 'set_upstream' /home/liuwei/api7/apisix/apisix/init.lua:549: in function 'handle_upstream' /home/liuwei/api7/apisix/apisix/init.lua:730: in function 'http_access_phase'

shreemaan-abhishek · 2023-10-12T03:19:26Z

@monkeyDluffy6017 I get this error when I run the wrk command that you shared:

juststillthinking · 2023-10-12T07:48:27Z

@shreemaan-abhishek You need to use wrk2 or remove the -R option

shreemaan-abhishek · 2023-10-12T09:54:22Z

I got the following error, when I ran the commands you mentioned:

2023/10/12 15:37:06 [error] 65019#1620486: *4429371 lua entry thread aborted: runtime error: ...abhishek/Desktop/repos/xisipa/apisix/apisix/balancer.lua:126: attempt to index local 'picker' (a number value)
stack traceback:
coroutine 0:
	...abhishek/Desktop/repos/xisipa/apisix/apisix/balancer.lua: in function 'create_obj_fun'
	...hek/Desktop/repos/xisipa/apisix/apisix/core/lrucache.lua:95: in function 'lrucache_server_picker'
	...abhishek/Desktop/repos/xisipa/apisix/apisix/balancer.lua:247: in function 'pick_server'
	...aan-abhishek/Desktop/repos/xisipa/apisix/apisix/init.lua:555: in function 'handle_upstream'
	...aan-abhishek/Desktop/repos/xisipa/apisix/apisix/init.lua:730: in function 'http_access_phase'
	access_by_lua(nginx.conf:303):2: in main chunk, client: 127.0.0.1, server: _, request: "GET /get HTTP/1.1", host: "127.0.0.1:9080"

I hope this is the same expected error.

juststillthinking · 2023-10-12T10:06:31Z

@shreemaan-abhishek I don't know how you get this error, it has nothing to do with my changes.

Signed-off-by: Ashish Tiwari <[email protected]>

* upstream/master: (83 commits) fix: make install failed on mac (apache#10403) feat(zipkin): add variable (apache#10361) test(clickhouse-logger): to show that different endpoints will be chosen randomly (apache#8777) chore(deps): bump actions/setup-node from 3.8.1 to 4.0.0 (apache#10381) ci: fix the grpc test error (apache#10388) ci: trigger ci when doc-lint.yml changes (apache#10382) docs: fix usage of incorrect default admin api port (apache#10391) feat: Add authorization params to openid-connect plugin (apache#10058) feat: integrate authz-keycloak with secrets resource (apache#10353) fix(traffic-split): post_arg match fails because content-type contains charset (apache#10372) fix(consul): worker will not exit while reload or quit (apache#10342) chore: update rules for unresponded issues (apache#10354) docs: Update APISIX usecases in README (apache#10358) test: use http2 to test limit-req plugin (apache#10334) test: use http2 to test limit-conn plugin (apache#10332) chore: remove stream_proxy.only in config-default.yaml (apache#10337) docs: update underscore to hyphen in HTTP headers in `response-rewrite` plugin (apache#10347) fix: typos in comments (apache#10330) feat: support config stream_route upstream in service (apache#10298) fix: keep healthcheck target state when upstream changes (apache#10312) ...

juststillthinking added 3 commits October 10, 2023 20:03

feat: keep healthcheck target state when upstream changes

9903a10

fix: clean_handlers should be a table

dfb0844

test: fix failed test case

bd68979

shreemaan-abhishek reviewed Oct 10, 2023

View reviewed changes

test: recover config-default.yaml

0e8c886

juststillthinking requested a review from kingluo October 11, 2023 04:22

juststillthinking commented Oct 11, 2023

View reviewed changes

kingluo approved these changes Oct 11, 2023

View reviewed changes

juststillthinking requested a review from shreemaan-abhishek October 13, 2023 06:08

Revolyssup mentioned this pull request Oct 13, 2023

feat: release 3.6.1 #10328

Closed

5 tasks

Revolyssup self-requested a review October 13, 2023 07:18

Revolyssup approved these changes Oct 13, 2023

View reviewed changes

juststillthinking merged commit 6fa5a89 into apache:master Oct 13, 2023

Revolyssup added a commit to Revolyssup/apisix that referenced this pull request Oct 15, 2023

add commit for apache#10312

762297e

Signed-off-by: Ashish Tiwari <[email protected]>

Revolyssup pushed a commit to Revolyssup/apisix that referenced this pull request Oct 15, 2023

fix: keep healthcheck target state when upstream changes (apache#10312)

fddce7e

Revolyssup pushed a commit to Revolyssup/apisix that referenced this pull request Oct 24, 2023

fix: keep healthcheck target state when upstream changes (apache#10312)

7f1ce59

Baoyuantop mentioned this pull request Apr 14, 2026

fix(healthcheck): use immediate clear() to ensure k8s endpoint change… #13029

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: keep healthcheck target state when upstream changes#10312

fix: keep healthcheck target state when upstream changes#10312
juststillthinking merged 4 commits into
apache:masterfrom
juststillthinking:keep-healthcheck-target-state-when-upstream-changes

juststillthinking commented Oct 10, 2023 •

edited

Loading

Uh oh!

shreemaan-abhishek Oct 10, 2023

Uh oh!

juststillthinking Oct 11, 2023

Uh oh!

shreemaan-abhishek commented Oct 10, 2023

Uh oh!

juststillthinking commented Oct 11, 2023

Uh oh!

juststillthinking Oct 11, 2023 •

edited

Loading

Uh oh!

shreemaan-abhishek commented Oct 12, 2023

Uh oh!

juststillthinking commented Oct 12, 2023

Uh oh!

shreemaan-abhishek commented Oct 12, 2023

Uh oh!

juststillthinking commented Oct 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

juststillthinking commented Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

shreemaan-abhishek Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

juststillthinking Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

shreemaan-abhishek commented Oct 10, 2023

Uh oh!

juststillthinking commented Oct 11, 2023

Uh oh!

juststillthinking Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shreemaan-abhishek commented Oct 12, 2023

Uh oh!

juststillthinking commented Oct 12, 2023

Uh oh!

shreemaan-abhishek commented Oct 12, 2023

Uh oh!

juststillthinking commented Oct 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

juststillthinking commented Oct 10, 2023 •

edited

Loading

juststillthinking Oct 11, 2023 •

edited

Loading