Skip to content

fix(nacos): keep cached nodes when a per-service refresh fails - #13971

Open
lordcheng10 wants to merge 1 commit into
apache:masterfrom
lordcheng10:fix/nacos-retain-cache-on-partial-refresh-failure
Open

lordcheng10 wants to merge 1 commit into
apache:masterfrom
lordcheng10:fix/nacos-retain-cache-on-partial-refresh-failure

Conversation

@lordcheng10

Copy link
Copy Markdown

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_registry builds the set of
service 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 nacos shared dict. Subsequent requests
to B can fail with no valid upstream node, although a usable cached snapshot
existed 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:

  • Rename service_names to configured_services in
    apisix/discovery/nacos/client.lua, and document the new contract in the
    fetch_from_host docstring.
  • Compute the cache key and set configured_services[key] = true before
    calling the instance-list API, so a service is reported as configured
    regardless of whether its query succeeds.
  • On a query error, keep logging get_url: <path> err: <error> and skip only
    that service's cache update. The cleanup loop in fetch_full_registry now
    compares against configured_services, so the previous snapshot survives.
  • Delete a cached key only when it was configured in the preceding refresh but
    is absent from the current configured_services set, i.e. the service was
    actually removed from APISIX configuration.

The change is limited to the error path. The behavior for a successful
response with an empty hosts list is intentionally unchanged from 3.13.0: an
empty 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

  • 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)

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:

  • a partial refresh failure keeps the failed service's cached nodes and still
    updates the service that succeeded;
  • a service removed from the configuration is still cleaned up on the next
    refresh;
  • the unchanged successful-but-empty hosts path;
  • the unchanged total-outage path, where no cleanup runs at all.

Reviewer note on verification: t/discovery/nacos2.t needs OpenResty plus the
Nacos 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.

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.
@lordcheng10

Copy link
Copy Markdown
Author

PTAL,thanks! @nic-6443 @AlinsRan @membphis @Baoyuantop

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: retain Nacos service cache when a per-service refresh query fails

1 participant