Skip to content

Conversation

Revolyssup
Copy link
Contributor

@Revolyssup Revolyssup commented May 28, 2025

Description

  • Currently nacos discovery uses event library and during testing it was found out that when using lua-resty-events, sometimes not all workers get the events. And inconsistencies emerge. Moreover the idiomatic way to share data between workers is through a shared dict. Therefore this PR migrates nacos from older events mechanism to newer shared dict way. Now only priviliged agent will fetch data from nacos and write to shdict while all workers will read from it.
  • Currently nacos supports only a single nacos registry but users have use case of supporting multiple nacos registries. This PR adds support to specify discovery.nacos as an array of nacos instances each with unique ID.

NOTE: The older way of configuring nacos is still supported and older data will be valid. The older configuration now gets converted internally to new configuration before being passed onto the logic therefore making this change backwards compatible. Meaning users can use the single-instance configuration as well as multi instance configuration.

Fixes #11252

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)

@dosubot dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label May 28, 2025
@Revolyssup Revolyssup marked this pull request as draft May 28, 2025 08:23
@dosubot dosubot bot added the enhancement New feature or request label May 28, 2025
@Revolyssup Revolyssup marked this pull request as ready for review May 28, 2025 19:28
@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels May 28, 2025
if not curr_services_in_use[config.id] then
curr_services_in_use[config.id] = {}
end
for name in pairs(service_names) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not assign service_names to curr_services_in_use[config.id] directly?

        for name in pairs(curr_services_in_use[config.id]) do
            if not service_names[name] then
                nacos_dict:delete(name)
            end
        end
        curr_services_in_use[config.id] = service_names

Comment on lines +45 to +56
-- maximum waiting time: 5 seconds
local waiting_time = 5
local step = 0.1
local logged = false
while not value and waiting_time > 0 do
if not logged then
logged = true
end

if body and 'table' == type(body) then
local err
body, err = core.json.encode(body)
if not body then
return nil, 'invalid body : ' .. err
ngx.sleep(step)
waiting_time = waiting_time - step
value = nacos_dict:get(key)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Revolyssup why no any changes for this? #12263 (comment), we already discuss to remove it?

Comment on lines +117 to +120
-- Now we use control plane to list the services
function _M.list_all_services()
return {}
end
Copy link
Member

@nic-6443 nic-6443 Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's this ? it's unused function, please remove it.

@Revolyssup Revolyssup marked this pull request as draft June 22, 2025 07:52
@Baoyuantop Baoyuantop moved this from 👀 In review to 📋 Backlog in ⚡️ Apache APISIX Roadmap Jun 27, 2025
Copy link

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the [email protected] list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Aug 21, 2025
Copy link

This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

@github-actions github-actions bot closed this Sep 18, 2025
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in ⚡️ Apache APISIX Roadmap Sep 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request size:XXL This PR changes 1000+ lines, ignoring generated files. stale

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

bug: nacos node failure should not corrupt in-memory data

4 participants