fix(nacos): keep cached nodes when a per-service refresh fails - #13971
Open
lordcheng10 wants to merge 1 commit into
Open
lordcheng10 wants to merge 1 commit into
lordcheng10 wants to merge 1 commit into
Conversation
A per-service query failure during the full-registry refresh dropped that service from the name set used for cache cleanup, so its last successful snapshot was deleted while it was still referenced by APISIX configuration. A transient Nacos, API or network error for one service could therefore break requests to that service with "no valid upstream node". Record the cache key in configured_services before each instance-list query, and only delete a cached entry when the service is no longer referenced by APISIX configuration. A failed refresh now keeps the previous snapshot until a later refresh succeeds or the service is removed from the configuration. The behavior for a successful response with an empty hosts list is unchanged. Add regression tests covering a partial refresh failure, cleanup after the service is no longer referenced, and the unchanged empty-hosts and total-outage paths.
Author
|
PTAL,thanks! @nic-6443 @AlinsRan @membphis @Baoyuantop |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fix a Nacos discovery refresh bug where a per-service query failure could
delete that service's last successful cache entry, even though the service was
still referenced by APISIX configuration.
In
apisix/discovery/nacos/init.lua,fetch_full_registrybuilds the set ofservice names from the per-host query result and then deletes every cached key
that is not in that set. The set was only populated after an instance-list
query succeeded, so a service whose query failed was missing from it and was
treated exactly like a service that had been removed from APISIX
configuration.
With two Nacos-discovered services A and B: a refresh where A succeeds and B
times out (or returns a network/HTTP error) leaves B out of the name set. The
cleanup loop then sees B in the in-use set but not in the name set, and deletes
B's last successful snapshot from the
nacosshared dict. Subsequent requeststo B can fail with
no valid upstream node, although a usable cached snapshotexisted before the refresh. This is disruptive during a partial Nacos/API/network
failure, and only the failing service loses its cache.
A failed query means the latest state is unknown, not that the service has been
removed from APISIX configuration. This PR makes that distinction explicit:
service_namestoconfigured_servicesinapisix/discovery/nacos/client.lua, and document the new contract in thefetch_from_hostdocstring.configured_services[key] = truebeforecalling the instance-list API, so a service is reported as configured
regardless of whether its query succeeds.
get_url: <path> err: <error>and skip onlythat service's cache update. The cleanup loop in
fetch_full_registrynowcompares against
configured_services, so the previous snapshot survives.is absent from the current
configured_servicesset, i.e. the service wasactually removed from APISIX configuration.
The change is limited to the error path. The behavior for a successful
response with an empty
hostslist is intentionally unchanged from 3.13.0: anempty instance list is still written as an empty node set. Confirming that
semantics is desired is left as a separate question, as this PR only addresses
the failure path.
Which issue(s) this PR fixes:
Fixes #13931
Checklist
Additional notes
No documentation update was made: this change adds no configuration option,
endpoint or documented behavior, it only stops the discovery cache from being
dropped when a refresh query fails.
Tests added to
t/discovery/nacos2.t:updates the service that succeeded;
refresh;
hostspath;Reviewer note on verification:
t/discovery/nacos2.tneeds OpenResty plus theNacos and etcd test containers, which were not available in the environment
used to prepare this patch. The change was validated there with a Lua syntax
check and a behavioral harness that replayed the refresh sequence against both
the 3.13.0 code and this patch (3.13.0 drops the failed service's entry; the
patch retains it while still deleting entries for services no longer
referenced). CI is expected to run the full test file.