membphis commented on code in PR #13894: URL: https://github.com/apache/apisix/pull/13894#discussion_r4378655361
########## t/node/healthcheck-stale-worker-cache.t: ########## @@ -0,0 +1,200 @@ +# +# 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('info'); +no_root_location(); +no_shuffle(); +worker_connections(256); + +run_tests(); + +__DATA__ + +=== TEST 1: fetch_node_status reads shm, not the worker-local event cache +# resty.healthcheck.get_target_status reads a per-worker cache filled by +# resty.events. Active checks run on one worker and write shm first; a peer +# that missed the event (or whose checker was created after the flip) keeps +# a stale "healthy" cache. Routing must consult shm (apache/apisix#13888). +--- config + location /t { + content_by_lua_block { + local healthcheck = require("resty.healthcheck") + local hm = require("apisix.healthcheck_manager") + + local checks = { + active = { + type = "http", + http_path = "/status", + healthy = { interval = 100, successes = 1 }, + unhealthy = { interval = 100, http_failures = 2 }, + }, + } + + local checker, err = healthcheck.new({ + name = "upstream#test-stale-worker-cache", + shm_name = "upstream-healthcheck", + events_module = "resty.events", + checks = checks, + }) + if not checker then + ngx.say("new failed: ", err) + return + end + + local ok + ok, err = checker:add_target("127.0.0.1", 1980, nil, true) + if not ok then + ngx.say("add_target failed: ", err) + checker:stop() + return + end + + -- this worker's local cache still says healthy + assert(checker:get_target_status("127.0.0.1", 1980, "127.0.0.1") == true, Review Comment: [P2] Wait for the local target before injecting the shared-state divergence `add_target()` writes shared state and publishes an event, while the local target is installed by the event callback. This immediate assertion does not give that callback an opportunity to run, so a fresh checker can return `nil, "target not found"` here instead of `true`. Executing the unchanged TEST 1 Lua content in APISIX 3.19 with the PR runtime files and real `resty.events` returned HTTP 500 at `local cache should start healthy`, before the SHM mutation or the new status-reading logic was exercised. In 20 fresh-checker controls, all immediate reads returned `target not found`; after yielding for the event, all became healthy and the subsequent SHM-only unhealthy flip was correctly detected. This was a runtime check of the test content, not a full Test::Nginx run. Please use bounded readiness polling to establish that the local target is healthy before changing SHM, then run the complete `.t` test. This preserves the intended precondition and ensures the test actually checks the fix. ########## apisix/healthcheck_manager.lua: ########## @@ -273,6 +308,24 @@ function _M.fetch_node_status(checker, ip, port, hostname) end +-- Cache-key fragment that changes when shm (or the local status_ver) changes, +-- so a picker cached against a stale worker-local view is rebuilt. +function _M.node_status_ver(checker, nodes, host, port) + if not checker or not nodes then + return "x" + end + local n = #nodes + local parts = core.table.new(n + 1, 0) + parts[1] = tostring(checker.status_ver or 0) + for i = 1, n do Review Comment: [P2] Avoid scanning all upstream nodes on every selection `health_cache_ver()` calls this loop on the request path before using the cached picker. Even when every node stays healthy and the picker is already warm, each selection now reads every node's shared state and allocates a new version string. This changes the version check from O(1) to O(N). I measured actual `pick_server()` calls with the exact parent (`97e3800d`) and PR (`44cc0bd`) versions of the two changed runtime files, loaded into the same APISIX 3.19 worker. The checker and shared dict were real, all nodes were healthy, caches were warmed, and each call used a fresh request context with a fixed upstream key/version. Median CPU time over five alternating rounds: | Nodes | Parent, us/call | PR, us/call | | ---: | ---: | ---: | | 3 | 0.673 | 1.849 | | 32 | 0.821 | 12.647 | | 128 | 0.830 | 49.171 | | 1024 | 0.735 | 445.507 | This is a local selection-path measurement, not an end-to-end throughput estimate, but it demonstrates a substantial steady-state cost as upstream size grows. Could we use an O(1) shared change version maintained by the health-state writer, then rebuild the picker when that version changes, or another approach that avoids a full scan per request? The invalidation signal must remain independent of local event delivery; reverting to only `checker.status_ver` would reintroduce the original failure. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
