feat(limit-count-redis): add sync_rate to reduce syncing frequency of counter data with redis #11374

shreemaan-abhishek · 2024-06-26T04:23:06Z

Description

Add the sync_rate parameter to control the frequency of synchronizing counters with redis in redis based limit-count plugins.

Real-time synchronization will put a lot of pressure on redis, refer to https://docs.konghq.com/hub/kong-inc/rate-limiting/configuration/#config-sync_rate.

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)

… counter data with redis

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

zhoujiexiong · 2024-07-02T10:30:49Z

apisix/utils/redis-schema.lua

@@ -42,6 +42,9 @@ local policy_to_additional_properties = {
            redis_ssl_verify = {
                type = "boolean", default = false,
            },
+            sync_interval = {


sync_interval should directly belong to plugin schema rather than Redis-schema?

only redis based rate limiting plugins will support this feature.

For further reuse purpose, suggest separating redis-schema definition from concrete plugin policy.

May be we could have structure like this:

apisix/utils/rate-limiting-policy.lua

- redis[_cluster]_cli_properties -> ref. redis-schema - sync_interval

this would be over-optimisation.

zhoujiexiong · 2024-07-03T09:21:48Z

apisix/utils/redis-schema.lua

@@ -42,6 +42,9 @@ local policy_to_additional_properties = {
            redis_ssl_verify = {
                type = "boolean", default = false,
            },
+            sync_interval = {
+                type = "integer", default = -1


default to 0 and with restriction >= 0?

I don't get you. But the default value denotes normal mode of operation. i.e sync with redis immediately.

Because delay(<=0) arg. to the ngx.timer.at function means executing the timeout callback as soon as the current handler yield.

But negative number for interval have no meaning, so restrict sync_interval >= 0.
And sync_interval = 0(default) means no delay(sync with redis immediately).

sync_interval is in seconds, 0 second would gives misleading idea of "sync immediately"

zhoujiexiong · 2024-07-03T09:24:48Z

apisix/plugins/limit-count/delayed_syncer.lua

+  end
+  core.log.info("num reqs passed since sync to redis: ", incr)
+
+  local ttl = 0


should update ttl before returning?

supporting ttl was complicating the code so I plan to support it in another PR.

TTL have been read from redis and wrote to SHDICT by the syncer

local res, err = red:eval(script, 1, key, conf.count, conf.time_window, num_reqs) if err then core.log.error("failed to sync shdict data to redis: ", err) return end local remaining = res[1] local ttl = res[2] core.log.info("syncing shdict num_req counter to redis. remaining: ", remaining, " ttl: ", ttl, " reqs: ", num_reqs) counter:set(key .. consts.SHDICT_REDIS_REMAINING, tonumber(remaining), tonumber(ttl))

So just read it from SHDICT is OK?

local remaining, err = counter:incr(key .. consts.SHDICT_REDIS_REMAINING, 0 - cost, limit, window) local ttl, err = counter:ttl(key .. consts.SHDICT_REDIS_REMAINING)

zhoujiexiong · 2024-07-03T09:25:53Z

apisix/plugins/limit-count/delayed_syncer.lua

+  to_be_synced[key] = true -- add to table for syncing
+  redis_confs[key] = conf
+
+  local incr, ierr = counter:incr(key .. consts.REDIS_COUNTER, cost, 1)


why init_val is 1 rather than 0?

because the first requests should also be counted.

But for the 1st request after counter:incr, the counter value is equal to 2.

-- run: resty --shdict 'counter 10m' incr.lua local counter = ngx.shared["counter"] local incr, _ = counter:incr("key", 1, 1) print("incr: ", incr) -- out: incr: 2

nice catch thank you.

should get remaining and judge remaining<0 before incr key..consts.REDIS_COUNTER ?Otherwise, the request that is rejected will be counted

zhoujiexiong · 2024-07-03T09:31:26Z

apisix/plugins/limit-count/delayed_syncer.lua

+
+function _M.rate_limit_with_delayed_sync(conf, counter, to_be_synced, redis_confs, key, cost,
+                                         limit, window, script)
+  local syncer_started = counter:get(consts.REDIS_SYNCER)


get and test not atomic, multiple timer would be started.

consider using DICT:add, if succ. then start timer

get and test not atomic

the document doesn't say anything like that. Could you share the source of this info? 🤔

So your idea is one syncer per worker process?

If so, why using SHDICT(consts.REDIS_SYNCER) which is globally to all worker processes to check syncer_started?

And this made me mistakenly think that your idea is one syncer per nginx server instance.

local syncer_started = counter:get(consts.REDIS_SYNCER) if not syncer_started then local ok, err = ngx_timer.at(conf.sync_interval, sync_counter_data, counter, to_be_synced, redis_confs, script) if ok then counter:set(consts.REDIS_SYNCER, true) else core.log.error("failed to create main redis syncer timer: ", err, ". Will retry next time.") end end

get and test not atomic

the document doesn't say anything like that. Could you share the source of this info? 🤔

If you want to ensure one syncer per nginx server instance(but not), the following logic(get and test) should be considered to be atomic.

local syncer_started = counter:get(consts.REDIS_SYNCER) if not syncer_started then

zhoujiexiong · 2024-07-03T09:35:23Z

apisix/plugins/limit-count/delayed_syncer.lua

+                                         limit, window, script)
+  local syncer_started = counter:get(consts.REDIS_SYNCER)
+  if not syncer_started then
+    local ok, err = ngx_timer.at(conf.sync_interval, sync_counter_data, counter, to_be_synced,


syncer is globally for a NGX server instance but conf.sync_interval is plugin instance's configuration.

I don't understand you.

Even if one syncer per worker process, but which plugin instance's conf.sync_interval the syncer apply?

Even if one syncer per worker process, but which plugin instance's conf.sync_interval the syncer apply?

but which plugin instance's conf.sync_interval the syncer apply?
->
but which plugin conf instance's sync_interval the syncer apply?

all plugins will have their own syncer as key is unique to each plugin instance.

apisix/apisix/plugins/limit-count/init.lua

Line 243 in 4dbecfd

key = gen_limit_key(conf, ctx, key)

all plugins will have their own syncer as key is unique to each plugin instance.

apisix/apisix/plugins/limit-count/init.lua

Line 243 in 4dbecfd

key = gen_limit_key(conf, ctx, key)

But per current creation logic, syncer is GLOBAL?

zhoujiexiong · 2024-07-03T09:42:16Z

apisix/plugins/limit-count/delayed_syncer.lua

+    end
+  end
+
+  to_be_synced[key] = true -- add to table for syncing


to_be_synced & redis_confs are module scoped variables, worker processes that not running syncer could not be able to tell syncer to sync the keys added by them through this way

each worker process will have it's copy of syncer so all worker processes will sync keys that they possess. What's the problem here? Or do you have any better alternative?

For the one syncer per worker process scenario, it's OK.

BTW:

to_be_synced & redis_confs are not necessary pass to ngx.timer.at as extra args?

using redis client instance instead of redis config?
could be like this: to_be_synced[key] = { redis_cli = red_cli }?

using redis client instance instead of redis config?

cosockets cannot be shared to ngx.timer functions.

to_be_synced & redis_confs are module scoped variables

I get it now. We need to make to_be_synced and redis_confs visible to the worker process which has syncer.

Thanks for your review again.

to_be_synced & redis_confs are module scoped variables

I get it now. We need to make to_be_synced and redis_confs visible to the worker process which has syncer.

Thanks for your review again.

:D

Yes.
But these module variables are originally visible to the per worker process syncer.

local ngx = ngx local ngx_timer = ngx.timer local ngx_sleep = ngx.sleep local ngx_worker_pid = ngx.worker.pid local table = table local table_insert = table.insert local table_remove = table.remove local str_fmt = string.format local chan = {} local timer_cb = function(premature, ...) if premature then return end local data = table_remove(chan, 1) local msg = str_fmt("--> timer is sched." .. " on worker process(%d)," .. " \n--> and got data from worker process(%s)" .. " through the `chan`", ngx_worker_pid(), tostring(data)) print(msg) end assert(ngx_timer.at(0, timer_cb)) local pid = ngx_worker_pid() table_insert(chan, pid) print(str_fmt("-> worker(%d)", pid), " will yield for running timer") ngx_sleep(0) print(str_fmt("<- worker(%d)", pid), " did yield") -- out -- -> worker(23037) will yield for running timer -- --> timer is sched. on worker process(23037), -- --> and got data from worker process(23037) through the `chan` -- <- worker(23037) did yield

using redis client instance instead of redis config?

cosockets cannot be shared to ngx.timer functions.

May be we could consider other implementation instead of ngx.timer. e.g. request driven syncer:
if reach count or time threshold, sync. immediately, in-place, in the req. handler context

zhoujiexiong · 2024-07-03T09:44:26Z

apisix/plugins/limit-count/delayed_syncer.lua

+    core.log.info("syncing shdict num_req counter to redis. remaining: ", remaining,
+                  " ttl: ", ttl, " reqs: ", num_reqs)
+    counter:set(key .. consts.SHDICT_REDIS_REMAINING, tonumber(remaining), tonumber(ttl))
+    counter:set(key .. consts.REDIS_COUNTER, 0)


Suggested change

counter:set(key .. consts.REDIS_COUNTER, 0)

counter:incr(key .. consts.REDIS_COUNTER, -num_reqs)

counter denotes the number of requests past since last sync to redis. So after syncing to redis the number of requests should be zero.

I get it now, you are right. Great catch!!

zhoujiexiong · 2024-07-03T09:47:55Z

apisix/plugins/limit-count/delayed_syncer.lua

+
+
+local function sync_counter_data(premature, counter, to_be_synced, redis_confs, script)
+  if premature then


should delete/clear the syncer flag here?

and what if the syncer worker process crash?

syncer flag? wdym?

For the one syncer per worker process scenario, ignore it. :D

zhoujiexiong · 2024-07-03T09:50:35Z

apisix/plugins/limit-count/delayed_syncer.lua

+  if premature then
+    return
+  end
+  for key, _ in pairs(to_be_synced) do


using sub-function for keys?

I didn't get you.

Encapsulate a function for synchronizing single key. :D

zhoujiexiong · 2024-07-04T05:54:40Z

apisix/plugins/limit-count/delayed_syncer.lua

format indent whitespace from 2 to 4?

and pass make lint

ERROR: apisix/plugins/limit-count/delayed_syncer.lua: line 33: getting the Lua global "pairs" ERROR: apisix/plugins/limit-count/delayed_syncer.lua: line 67: getting the Lua global "tonumber" ERROR: apisix/plugins/limit-count/delayed_syncer.lua: line 67: getting the Lua global "tonumber"

zhoujiexiong · 2024-07-04T05:58:21Z

@shreemaan-abhishek

one syncer per worker process vs one syncer per nginx server instance

What do you think of?

shreemaan-abhishek and others added 9 commits June 25, 2024 15:30

feat(limit-count-redis): add sync_rate to reduce syncing frequency of…

0c97dd9

… counter data with redis

add license

2258b21

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

line length

885a612

shebang

f3814cc

fix test

a31c732

typo

c02a0cd

fix(pm): shdict declaration

f3d25ad

fix docker compose error

e44c31d

fix lint

20ff375

zhoujiexiong reviewed Jul 3, 2024

View reviewed changes

apache deleted a comment from zhoujiexiong Jul 3, 2024

zhoujiexiong reviewed Jul 4, 2024

View reviewed changes

shreemaan-abhishek closed this Aug 9, 2024

	counter:set(key .. consts.REDIS_COUNTER, 0)
	counter:incr(key .. consts.REDIS_COUNTER, -num_reqs)



		local function sync_counter_data(premature, counter, to_be_synced, redis_confs, script)
		if premature then

feat(limit-count-redis): add sync_rate to reduce syncing frequency of counter data with redis #11374

feat(limit-count-redis): add sync_rate to reduce syncing frequency of counter data with redis #11374

Conversation

shreemaan-abhishek commented Jun 26, 2024 • edited Loading

Description

Checklist

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhoujiexiong Jul 4, 2024 • edited 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

zhoujiexiong Jul 3, 2024 • edited 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhoujiexiong Jul 3, 2024 • edited 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

zhoujiexiong Jul 5, 2024 • edited 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

Choose a reason for hiding this comment

zhoujiexiong Jul 4, 2024 • edited 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

zhoujiexiong commented Jul 4, 2024 • edited Loading

shreemaan-abhishek commented Jun 26, 2024 •

edited

Loading

zhoujiexiong Jul 4, 2024 •

edited

Loading

zhoujiexiong Jul 3, 2024 •

edited

Loading

zhoujiexiong Jul 3, 2024 •

edited

Loading

zhoujiexiong Jul 5, 2024 •

edited

Loading

zhoujiexiong Jul 4, 2024 •

edited

Loading

zhoujiexiong commented Jul 4, 2024 •

edited

Loading