fix(healthcheck): update targets incrementally instead of rebuilding

AlinsRan · AlinsRan · commit 79941680a682 · 2026-06-22T11:38:58.000+08:00
When an upstream's nodes change but its `checks` config is unchanged, the health-check manager destroyed the existing checker and built a new one. Two problems followed: 1. fetch_checker() keys the working checker by a version derived from both modifiedIndex and the nodes version, so a node-only change makes it return nil until the timer rebuilds the checker. During that window api_ctx.up_checker is nil and the balancer routes traffic to nodes already known to be unhealthy (#13282). 2. The rebuild throws away the checker's accumulated health state and re-probes every node from scratch. The manager now reconciles the existing checker's targets in place with add_target/remove_target when the `checks` config is unchanged (compared with core.table.deep_eq), keeping the checker and its state alive. timer_working_pool_check no longer destroys a checker for a node-only version change, and when a rebuild is genuinely required (the `checks` config changed) the new checker is created and inserted into the working pool before the old one is released, so fetch_checker never observes a nil gap. Bumps the lua-resty-healthcheck-api7 rockspec dependency to 3.3.0-0, which contains the companion library fix (clean every checker each window + release the periodic lock when idle) required by this change. Adds t/node/healthcheck-incremental-update.t: a node-only change must not destroy/rebuild the checker (no "clear checker"), while a checks-config change still rebuilds it.
diff --git a/apisix/healthcheck_manager.lua b/apisix/healthcheck_manager.lua
@@ -31,7 +31,8 @@ local jp = require("jsonpath")
 local config_util = require("apisix.core.config_util")
 
 local _M = {}
-local working_pool = {}     -- resource_path -> {version = ver, checker = checker}
+-- resource_path -> {version = ver, checker = checker, checks = checks}
+local working_pool = {}
 local waiting_pool = {}      -- resource_path -> resource_ver
 
 local DELAYED_CLEAR_TIMEOUT = 10
@@ -44,6 +45,31 @@ end
 _M.get_healthchecker_name = get_healthchecker_name
 
 
+-- Compute the desired set of health-check targets for an upstream config.
+-- Returns a map keyed by "host:port:hostheader" so the working set can be
+-- diffed cheaply against a checker's current targets.
+local function compute_targets(up_conf)
+    local host = up_conf.checks and up_conf.checks.active and up_conf.checks.active.host
+    local port = up_conf.checks and up_conf.checks.active and up_conf.checks.active.port
+    local up_hdr = up_conf.pass_host == "rewrite" and up_conf.upstream_host
+    local use_node_hdr = up_conf.pass_host == "node" or nil
+
+    local targets = {}
+    for _, node in ipairs(up_conf.nodes) do
+        local host_hdr = up_hdr or (use_node_hdr and node.domain) or nil
+        local target = {
+            host = node.host,
+            port = port or node.port,
+            check_host = host,
+            host_hdr = host_hdr,
+        }
+        local key = target.host .. ":" .. tostring(target.port) .. ":" .. tostring(host_hdr or "")
+        targets[key] = target
+    end
+    return targets
+end
+
+
 local function create_checker(up_conf)
     if not up_conf.checks then
         return nil
@@ -71,25 +97,66 @@ local function create_checker(up_conf)
     end
 
     -- Add target nodes
-    local host = up_conf.checks and up_conf.checks.active and up_conf.checks.active.host
-    local port = up_conf.checks and up_conf.checks.active and up_conf.checks.active.port
-    local up_hdr = up_conf.pass_host == "rewrite" and up_conf.upstream_host
-    local use_node_hdr = up_conf.pass_host == "node" or nil
-
-    for _, node in ipairs(up_conf.nodes) do
-        local host_hdr = up_hdr or (use_node_hdr and node.domain)
-        local ok, err = checker:add_target(node.host, port or node.port, host,
-                                        true, host_hdr)
+    for _, target in pairs(compute_targets(up_conf)) do
+        local ok, err = checker:add_target(target.host, target.port, target.check_host,
+                                        true, target.host_hdr)
         if not ok then
-            core.log.error("failed to add healthcheck target: ", node.host, ":",
-                          port or node.port, " err: ", err)
+            core.log.error("failed to add healthcheck target: ", target.host, ":",
+                          target.port, " err: ", err)
         end
     end
 
     return checker
 end
 
 
+-- Incrementally reconcile an existing checker's targets to match up_conf.
+-- Used when only the upstream nodes changed but the `checks` config did not,
+-- so the checker can keep running (and keep its accumulated health state)
+-- instead of being destroyed and rebuilt.
+local function sync_checker_targets(checker, up_conf)
+    local desired = compute_targets(up_conf)
+
+    -- index current targets the same way as desired. Read the authoritative
+    -- shm target list (the per-worker checker.targets array can lag behind a
+    -- recent add/remove event).
+    if not healthcheck then
+        healthcheck = require("resty.healthcheck")
+    end
+    local current = {}
+    local target_list = healthcheck.get_target_list(get_healthchecker_name(up_conf),
+                                                    healthcheck_shdict_name) or {}
+    for _, t in ipairs(target_list) do
+        -- target_list entries carry hostheader; map it back to our key shape
+        local key = t.ip .. ":" .. tostring(t.port) .. ":" .. tostring(t.hostheader or "")
+        current[key] = t
+    end
+
+    -- add targets that are desired but not present
+    for key, target in pairs(desired) do
+        if not current[key] then
+            local ok, err = checker:add_target(target.host, target.port, target.check_host,
+                                            true, target.host_hdr)
+            if not ok then
+                core.log.error("failed to add healthcheck target: ", target.host, ":",
+                              target.port, " err: ", err)
+            end
+        end
+    end
+
+    -- remove targets that are present but no longer desired
+    for key, t in pairs(current) do
+        if not desired[key] then
+            local ok, err = checker:remove_target(t.ip, t.port, t.hostname)
+            if not ok then
+                core.log.error("failed to remove healthcheck target: ", t.ip, ":",
+                              t.port, " err: ", err)
+            end
+        end
+    end
+end
+
+
 function _M.fetch_checker(resource_path, resource_ver)
     local working_item = working_pool[resource_path]
     if working_item and working_item.version == resource_ver then
@@ -130,10 +197,11 @@ function _M.fetch_node_status(checker, ip, port, hostname)
 end
 
 
-local function add_working_pool(resource_path, resource_ver, checker)
+local function add_working_pool(resource_path, resource_ver, checker, checks)
     working_pool[resource_path] = {
         version = resource_ver,
-        checker = checker
+        checker = checker,
+        checks = checks,
     }
 end
 
@@ -202,22 +270,43 @@ local function timer_create_checker()
                 goto continue
             end
 
-            -- if a checker exists then delete it before creating a new one
+            -- If a checker already exists and the `checks` config is unchanged
+            -- (only the upstream nodes changed), reconcile its targets in place
+            -- instead of destroying and rebuilding it. A destroy-and-rebuild
+            -- leaves `up_checker == nil` for the rebuild window, during which
+            -- traffic is routed to nodes already known to be unhealthy, and it
+            -- throws away the checker's accumulated health state.
             local existing_checker = working_pool[resource_path]
+            if existing_checker and existing_checker.checker
+               and not existing_checker.checker.dead
+               and upstream.checks
+               and core.table.deep_eq(existing_checker.checks, upstream.checks) then
+                sync_checker_targets(existing_checker.checker, upstream)
+                add_working_pool(resource_path, resource_ver, existing_checker.checker,
+                                 upstream.checks)
+                core.log.info("reused checker with incremental targets: ",
+                              tostring(existing_checker.checker), " for resource: ",
+                              resource_path, " and version: ", resource_ver)
+                goto continue
+            end
+
+            -- The checks config changed (or no checker exists): build a fresh
+            -- checker first, and only release the old one *after* the new one is
+            -- in the working pool, so fetch_checker never observes a nil gap.
+            local checker = create_checker(upstream)
+            if not checker then
+                goto continue
+            end
             if existing_checker then
                 existing_checker.checker:delayed_clear(DELAYED_CLEAR_TIMEOUT)
                 existing_checker.checker:stop()
                 core.log.info("releasing existing checker: ", tostring(existing_checker.checker),
                               " for resource: ", resource_path, " and version: ",
                               existing_checker.version)
             end
-            local checker = create_checker(upstream)
-            if not checker then
-                goto continue
-            end
             core.log.info("create new checker: ", tostring(checker), " for resource: ",
                         resource_path, " and version: ", resource_ver)
-            add_working_pool(resource_path, resource_ver, checker)
+            add_working_pool(resource_path, resource_ver, checker, upstream.checks)
         end
 
         ::continue::
@@ -258,6 +347,12 @@ local function timer_working_pool_check()
                         " current version: ", current_ver, " item version: ", item.version)
             if item.version == current_ver then
                 need_destroy = false
+            elseif upstream.checks and core.table.deep_eq(item.checks, upstream.checks) then
+                -- Version changed but only because of the upstream nodes; the
+                -- `checks` config is identical. Keep the checker alive so
+                -- timer_create_checker can reconcile its targets incrementally
+                -- (avoids a destroy-and-rebuild nil window for the checker).
+                need_destroy = false
             end
         end
 
diff --git a/t/node/healthcheck-incremental-update.t b/t/node/healthcheck-incremental-update.t
@@ -0,0 +1,149 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+log_level('warn');
+no_root_location();
+no_shuffle();
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: node-only change reuses the checker (no destroy-and-rebuild)
+--- extra_init_worker_by_lua
+    local healthcheck = require("resty.healthcheck")
+    local new = healthcheck.new
+    healthcheck.new = function(...)
+        ngx.log(ngx.WARN, "create new checker")
+        local obj = new(...)
+        local clear = obj.delayed_clear
+        obj.delayed_clear = function(...)
+            ngx.log(ngx.WARN, "clear checker")
+            return clear(...)
+        end
+        return obj
+    end
+
+--- config
+location /t {
+    content_by_lua_block {
+        local checks = [[{
+            "active":{
+                "http_path":"/hello",
+                "timeout":1,
+                "type":"http",
+                "healthy":{ "interval":1, "successes":1 },
+                "unhealthy":{ "interval":1, "http_failures":2 }
+            }
+        }]]
+        local function cfg(nodes)
+            return [[{
+                "upstream": {
+                    "nodes": ]] .. nodes .. [[,
+                    "type": "roundrobin",
+                    "checks": ]] .. checks .. [[
+                },
+                "uri": "/hello"
+            }]]
+        end
+
+        local t = require("lib.test_admin").test
+        -- initial config: one node -> creates the checker
+        assert(t('/apisix/admin/routes/1', ngx.HTTP_PUT,
+                 cfg('{"127.0.0.1:1980": 1}')) < 300)
+        t('/hello', ngx.HTTP_GET)
+        ngx.sleep(2)
+
+        -- node-only change (checks unchanged): should reconcile in place,
+        -- NOT create a new checker nor delayed_clear the old one
+        assert(t('/apisix/admin/routes/1', ngx.HTTP_PUT,
+                 cfg('{"127.0.0.1:1980": 1, "127.0.0.1:1981": 1}')) < 300)
+        t('/hello', ngx.HTTP_GET)
+        ngx.sleep(2)
+        ngx.say("done")
+    }
+}
+
+--- request
+GET /t
+--- response_body
+done
+--- no_error_log
+clear checker
+--- error_log
+create new checker
+--- timeout: 8
+
+
+
+=== TEST 2: checks-config change still rebuilds the checker
+--- extra_init_worker_by_lua
+    local healthcheck = require("resty.healthcheck")
+    local new = healthcheck.new
+    healthcheck.new = function(...)
+        local obj = new(...)
+        local clear = obj.delayed_clear
+        obj.delayed_clear = function(...)
+            ngx.log(ngx.WARN, "clear checker")
+            return clear(...)
+        end
+        return obj
+    end
+
+--- config
+location /t {
+    content_by_lua_block {
+        local function cfg(interval)
+            return [[{
+                "upstream": {
+                    "nodes": {"127.0.0.1:1980": 1},
+                    "type": "roundrobin",
+                    "checks": {
+                        "active":{
+                            "http_path":"/hello",
+                            "timeout":1,
+                            "type":"http",
+                            "healthy":{ "interval":]] .. interval .. [[, "successes":1 },
+                            "unhealthy":{ "interval":1, "http_failures":2 }
+                        }
+                    }
+                },
+                "uri": "/hello"
+            }]]
+        end
+
+        local t = require("lib.test_admin").test
+        assert(t('/apisix/admin/routes/1', ngx.HTTP_PUT, cfg(1)) < 300)
+        t('/hello', ngx.HTTP_GET)
+        ngx.sleep(2)
+        -- change the checks config -> must rebuild (delayed_clear old checker)
+        assert(t('/apisix/admin/routes/1', ngx.HTTP_PUT, cfg(2)) < 300)
+        t('/hello', ngx.HTTP_GET)
+        ngx.sleep(2)
+        ngx.say("done")
+    }
+}
+
+--- request
+GET /t
+--- response_body
+done
+--- error_log
+clear checker
+--- timeout: 8
