fix(clustering/sync): avoiding long delay caused by race condition #13896
Conversation
github-actions
bot
added
core/clustering
cherry-pick kong-ee
StarlightIbuki
requested review from
dndx,
chronolaw and
chobits
and removed request for
dndx
chronolaw
changed the title
| return nil, err | ||
| end | ||
|
|
||
| return true | ||
| end | ||
|
|
||
|
|
||
| function sync_once(premature, retry_count) |
There was a problem hiding this comment.
It has the same name as _M:sync_once, should we choice another one?
There was a problem hiding this comment.
I think it should be the new sync_hanlder()
There was a problem hiding this comment.
No. sync_handler is a name for another function
|
Could we write a test case to verify this fix? |
| return nil, err | ||
| end | ||
|
|
||
| return true | ||
| end | ||
|
|
||
|
|
||
| function sync_once(premature, retry_count) |
There was a problem hiding this comment.
I think it should be the new sync_hanlder()
Not easily. We cannot be sure that the race conditions happen when we test. We could observe whether it relieves the flaky tests. |
StarlightIbuki
force-pushed
the
fix/sync-race
branch
from
6ac9004
to
c9f0ab7
Compare
| @@ -164,6 +167,8 @@ function _M:init_dp(manager) | |||
|
|
|||
| local lmdb_ver = tonumber(declarative.get_current_hash()) or 0 | |||
| if lmdb_ver < version then | |||
| -- set lastest version to shm | |||
| kong_shm:set(CLUSTERING_DATA_PLANES_LATEST_VERSION_KEY, version) | |||
There was a problem hiding this comment.
We only run sync in one worker, is it necessary to store it in shared memory?
There was a problem hiding this comment.
This is not true. @dndx Could you confirm?
There was a problem hiding this comment.
We run incremental sync inside all workers, it's just that only one worker can sync at the same time.
|
|
||
| return true | ||
| end) | ||
| local res, err = concurrency.with_worker_mutex(SYNC_MUTEX_OPTS, do_sync) |
There was a problem hiding this comment.
Not sure, could we use concurrency.with_coroutine_mutex?
There was a problem hiding this comment.
We deign the sync.v2 to work without privileged worker (and worker no.0)
| return nil, err | ||
| end | ||
|
|
||
| return true | ||
| end | ||
|
|
||
|
|
||
| function sync_once_impl(premature, retry_count) |
There was a problem hiding this comment.
Could we use a simple loop? like:
for i = 1, 5 do
sync_handler()
if updated then
break
end
ngx.sleep(0)
end
There was a problem hiding this comment.
No. Recreating the timer prevents long-live timer from causing resource leak
Summary
It will retry until the version matches
Checklist
CHANGELOG/unreleased/kongor addingskip-changeloglabel on PR if unnecessary. README.mdIssue reference
Fix KAG-5857