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
| if not latest_notified_version or current_version < latest_notified_version then | ||
| retry_count = retry_count or 0 | ||
| if retry_count > MAX_RETRY then | ||
| ngx_log(ngx_ERR, "sync_once retry count exceeded. retry_count: ", retry_count) |
There was a problem hiding this comment.
I found that DP started without getting notify rpc call from CP, it will always report ERROR log.
There was a problem hiding this comment.
so could we do this, if no one updated latest version, we could just sync one time then return directly without adding task again for intialization
if not lastest_notified_version then
return
end
Summary
It will retry until the version matches
Checklist
CHANGELOG/unreleased/kongor addingskip-changeloglabel on PR if unnecessary. README.mdIssue reference
Fix KAG-5857 KAG-5876