feat(clusteriing): introduce incremental sync for clustering #13157

dndx · 2024-06-04T09:47:51Z

KAG-4865
KAG-2986
KAG-2987
KAG-3502
KAG-3258
KAG-5283

Flaky (perhaps need rerun):

spec/02-integration/09-hybrid_mode/03-pki_spec.lua
spec/02-integration/02-cmd/14-vault_spec.lua
spec/03-plugins/09-key-auth/04-hybrid_mode_spec.lua

This reverts commit ddda6a1.

* txn in strategy * register_dao_hooks * fix txn funcs * comments * hook dao:xxx:fail * dao:delete:fail * get_latest_version * Revert "get_latest_version" This reverts commit c97f868. * select max

incremental sync

This reverts commit 6307e91.

flrgh · 2024-09-30T23:25:18Z

kong/clustering/services/sync/hooks.lua

+
+
+local function get_all_nodes_with_sync_cap()
+  local res, err = kong.db.clustering_data_planes:page(DEFAULT_PAGE_SIZE)


Should we not use :each() here instead of simply fetching one page? It seems that this function will only ever return DEFAULT_PAGE_SIZE entries even if there are more in the database.

kong/clustering/services/sync/hooks.lua

flrgh · 2024-09-30T23:32:06Z

kong/clustering/services/sync/hooks.lua

+end
+
+
+function _M:notify_all_nodes(new_version)


This new_version param appears to be unused.

flrgh · 2024-09-30T23:45:16Z

kong/clustering/services/sync/rpc.lua

+  manager.callbacks:register("kong.sync.v2.notify_new_version", function(node_id, new_versions)
+    -- currently only default is supported, and anything else is ignored
+    for namespace, new_version in pairs(new_versions) do
+      if namespace == "default" then
+        local version = new_version.new_version
+        if not version then
+          return nil, "'new_version' key does not exist"
+        end
+
+        local lmdb_ver = tonumber(declarative.get_current_hash()) or 0
+        if lmdb_ver < version then
+          return self:sync_once()
+        end
+
+        return true
+      end
+    end
+
+    return nil, "default namespace does not exist inside params"
+  end)
+end


Is it possible for the value of new_versions to contain multiple entries for the default namespace, like this?

{ { namespace = "default", new_version = 1 }, { namespace = "default", new_version = 3 }, }

If so, then this logic might be faulty. If lmdb_ver = 2, then we will exit the loop after one iteration (because lmdb_ver (2) < version (1)) instead of evaluating the next entry which has new_version = 3 and would trigger a sync operation.

flrgh · 2024-09-30T23:52:21Z

kong/clustering/services/sync/rpc.lua

+
+
+function _M:sync_once(delay)
+  local hdl, err = ngx.timer.at(delay or 0, function(premature)


This timer handler doesn't capture any upvalues from the surrounding function scope of _M:sync_once. Should we transform it into a reusable function at the module level instead?

flrgh · 2024-09-30T23:56:13Z

kong/clustering/services/sync/rpc.lua

+    end
+
+    local res, err = concurrency.with_worker_mutex(SYNC_MUTEX_OPTS, function()
+      -- here must be 2 times


Can you explain why 2 times in the comment?

flrgh · 2024-10-01T00:02:09Z

kong/clustering/services/sync/rpc.lua

+        if ns_delta.wipe then
+          kong.core_cache:purge()
+          kong.cache:purge()
+
+        else
+          for _, event in ipairs(crud_events) do
+            -- delta_type, crud_event_type, delta.row, old_entity
+            db[event[1]]:post_crud_event(event[2], event[3], event[4])
+          end
+        end


It's not clear to me why purging the cache and executing the event handlers are mutually-exclusive outcomes. Can you explain why we skip the crud events when ns_delta.wipe is truthy?

flrgh · 2024-10-01T18:12:18Z

kong/clustering/services/sync/init.lua

+-- TODO: what is the proper value?
+local FIRST_SYNC_DELAY = 0.5  -- seconds


I would think we would want to sync as soon as possible, no?

Suggested change

-- TODO: what is the proper value?

local FIRST_SYNC_DELAY = 0.5 -- seconds

local FIRST_SYNC_DELAY = 0

In our test suits we may need to wait cp to start, here 0.5 second perhaps is a proper number, or else the next sync will be 30 seconds later.
Sure, we need to find the appropriate number.

flrgh · 2024-10-01T18:42:08Z

kong/clustering/services/sync/strategies/postgres.lua

+
+function _M:get_latest_version()
+  local sql = "SELECT MAX(version) AS max_version FROM clustering_sync_version"
+  return self.connector:query(sql)[1].max_version


The other methods in this file all return directly from connector:query(), so I assume they will return nil, string in case of error.

On the other hand, this method may raise an error due to invalid table access if there is a failure in connector:query().

I suggest we make the error-handling more consistent:

Suggested change

return self.connector:query(sql)[1].max_version

local rows, err = self.connector:query(sql)

if not rows then

return nil, err

end

-- should we raise/return an error if no rows/max_version are returned?

-- or should we return a default value of 0?

return rows[1] and rows[1].max_version

If it's intended to raise an error when connector:query() fails, then I suggest using assert() or error() to make the intention more explicit.

We should also codify the behavior in the case that clustering_sync_version is empty. I'm seeing code elsewhere that assumes it cannot return nil. Is the caller incorrect?

-- location: kong/clustering/services/sync/rpc.lua function _M:init_cp(manager) -- [...] if default_namespace_version == 0 or self.strategy:get_latest_version() - default_namespace_version > FULL_SYNC_THRESHOLD then -- [...]

flrgh · 2024-10-01T19:00:22Z

kong/db/migrations/core/024_370_to_380.lua

+      CREATE TABLE IF NOT EXISTS clustering_sync_delta (
+        "version" INT NOT NULL,
+        "type" TEXT NOT NULL,
+        "id" UUID NOT NULL,


What will happen if there is an entity without an id attribute or with an id that isn't a UUID type? Is it possible to create such an entity in Kong via a 3rd party plugin dao, or will the metaschema prevent this condition?

spec/02-integration/04-admin_api/15-off_spec.lua

pull-request-size bot added the size/XXL label Jun 4, 2024

github-actions bot added core/proxy core/db/migrations core/clustering core/templates core/db core/configuration schema-change-noteworthy cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Jun 4, 2024

github-actions bot assigned dndx Jun 4, 2024

dndx force-pushed the feat/incremental branch 3 times, most recently from 88e177a to e64c085 Compare June 4, 2024 17:13

chronolaw force-pushed the feat/incremental branch from e64c085 to 2692ea9 Compare July 4, 2024 01:33

dndx force-pushed the feat/incremental branch 2 times, most recently from 856ca04 to 50528fd Compare August 7, 2024 03:27

dndx force-pushed the feat/incremental branch from 50528fd to 2163b2b Compare August 19, 2024 08:27

chronolaw mentioned this pull request Aug 28, 2024

feat(dao): support diff transaction #13586

Merged

3 tasks

dndx force-pushed the feat/incremental branch from dbe6259 to 573852d Compare September 9, 2024 09:05

dndx changed the title ~~[WIP] incremental sync PoC~~ Incremental sync PoC Sep 11, 2024

dndx and others added 10 commits September 20, 2024 16:52

Revert "fix(rpc): disable cluster_rpc for 3.7"

3b97ce0

This reverts commit ddda6a1.

WIP incremental sync

a0e42f9

add migration to luarocks file

f841c48

fix bugs in RPC handler

1158448

fix pagination bug

970221a

feat(dao): support diff transaction (#13586)

7792f96

* txn in strategy * register_dao_hooks * fix txn funcs * comments * hook dao:xxx:fail * dao:delete:fail * get_latest_version * Revert "get_latest_version" This reverts commit c97f868. * select max

WIP

6594544

refactored import logic so it can be shared by DB-less import and

3f0dd3f

incremental sync

add update and upsert support

5617285

fixed default workspace recording issue

d60468a

chronolaw added 2 commits September 30, 2024 11:03

small style

c38c539

refactor _set_entity_for_txn()

68849c4

chronolaw force-pushed the feat/incremental branch from bebd9ed to 68849c4 Compare September 30, 2024 07:06

clean 01-db/10-declarative_spec.lua

a405cf5

chronolaw marked this pull request as ready for review September 30, 2024 08:17

chronolaw requested a review from chobits September 30, 2024 08:18

chronolaw added 4 commits September 30, 2024 16:21

09-hybrid_mode/05-ocsp_spec.lua

28916cb

clean hooks.lua

61b792b

clean rpc.lua

fb59d6f

fix delete event in rpc.lua

6307e91

chronolaw force-pushed the feat/incremental branch from a1be884 to 6307e91 Compare September 30, 2024 09:28

chronolaw added 2 commits September 30, 2024 18:05

Revert "fix delete event in rpc.lua"

81dfa41

This reverts commit 6307e91.

clean rpc.lua

8dd2884

RobSerafini requested review from gszr, flrgh, brentos and mikefero September 30, 2024 12:52

flrgh reviewed Sep 30, 2024

View reviewed changes

kong/clustering/services/sync/hooks.lua Show resolved Hide resolved

flrgh reviewed Sep 30, 2024

View reviewed changes

flrgh reviewed Oct 1, 2024

View reviewed changes

spec/02-integration/04-admin_api/15-off_spec.lua Show resolved Hide resolved

chronolaw added 2 commits October 2, 2024 08:42

clean hooks.lua

c57fa10

spec/02-integration/04-admin_api/15-off_spec.lua

1fafa4a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(clusteriing): introduce incremental sync for clustering #13157

feat(clusteriing): introduce incremental sync for clustering #13157

dndx commented Jun 4, 2024 •

edited by chronolaw

Loading

flrgh Sep 30, 2024

flrgh Sep 30, 2024

flrgh Sep 30, 2024

flrgh Sep 30, 2024

flrgh Sep 30, 2024

flrgh Oct 1, 2024

flrgh Oct 1, 2024

chronolaw Oct 2, 2024 •

edited

Loading

flrgh Oct 1, 2024

flrgh Oct 1, 2024



		local function get_all_nodes_with_sync_cap()
		local res, err = kong.db.clustering_data_planes:page(DEFAULT_PAGE_SIZE)



		function _M:sync_once(delay)
		local hdl, err = ngx.timer.at(delay or 0, function(premature)

		-- TODO: what is the proper value?
		local FIRST_SYNC_DELAY = 0.5 -- seconds

	-- TODO: what is the proper value?
	local FIRST_SYNC_DELAY = 0.5 -- seconds
	local FIRST_SYNC_DELAY = 0

-  return self.connector:query(sql)[1].max_version
+  local rows, err = self.connector:query(sql)
+  if not rows then
+    return nil, err
+  end
+  -- should we raise/return an error if no rows/max_version are returned?
+  -- or should we return a default value of 0?
+  return rows[1] and rows[1].max_version

feat(clusteriing): introduce incremental sync for clustering #13157

Are you sure you want to change the base?

feat(clusteriing): introduce incremental sync for clustering #13157

Conversation

dndx commented Jun 4, 2024 • edited by chronolaw Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chronolaw Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dndx commented Jun 4, 2024 •

edited by chronolaw

Loading

chronolaw Oct 2, 2024 •

edited

Loading