feat(config_etcd): use a single long http connection to watch all resources by kingluo · Pull Request #9456 · apache/apisix

kingluo · 2023-05-10T17:22:19Z

Description

only one http connection to watch the prefix for all resources
use chunked streaming to receive events in the connection, timeout=50 second

Sequence Diagram:

/routes watcher->/routes watcher: first sync_data()
/routes watcher->main watcher: start
/routes watcher->/routes watcher: wait main watcher
/upstreams watcher->/upstreams watcher: first sync_data(), wait main watcher
main watcher->etcd: get current etcd revision and start watch there
main watcher-->/routes watcher: started
main watcher-->/upstreams watcher: started
/routes watcher->etcd: readdir() if need_reloaded
/routes watcher->main watcher: iterate history watch events, found
/upstreams watcher->main watcher: no history event, wait...
etcd-->main watcher: watch event
main watcher->main watcher: save event
main watcher->/upstreams watcher: notify
/upstreams watcher->main watcher: iterate history watch events, found
etcd-->main watcher: timeout
main watcher->etcd: reconnect and restart watch from max received revision
etcd-->main watcher: compacted
main watcher->etcd: reconnect and restart watch from compacted revision

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)

kingluo · 2023-05-11T16:31:48Z

 GET /tg
 --- response_body
--- error_log eval
-qr/.*\[\{\"body\":\{\"text\":\{\"text\":\".*this is an error message for test.*\"\}\},\"endpoint\":\"\",\"service\":\"APISIX\",\"serviceInstance\":\"instance\".*/


The new watch subsystem may print new logs.
All in all, strict log assumption is not a good practice.

kingluo · 2023-05-12T03:48:11Z

--- response_body
-prev_index updated
--- error_log eval
-qr/(create watch stream for key|cancel watch connection success)/


deprecated test. etcd watch is a single long connection now.

kingluo · 2023-05-13T15:50:28Z


 export OPENRESTY_VERSION=source
-export TEST_CI_USE_GRPC=true
+#export TEST_CI_USE_GRPC=true


We're going to start focusing on etcd's http tests.

kingluo · 2023-05-13T16:15:14Z

+    watch_ctx.wait_init = nil
+
+    local opts = {}
+    opts.timeout = 50 -- second


The choice of 50 seconds is to make it smaller than the default proxy_read_timeout value, 60 seconds, so that nginx will not print error logs, such as:

2023/05/10 23:43:44 [error] 3668019#3668019: *809 upstream timed out (110: Connection timed out) while reading upstream, client: unix:, server: , request: "POST /v3/watch HTTP/1.1", upstream: "http://127.0.0.1:2379/v3/watch", host: "127.0.0.1"

juststillthinking · 2023-05-19T13:38:41Z

+                if err ~= "closed" and
+                    err ~= "timeout" and
+                    err ~= "broken pipe" then
+                    log.error("wait watch event: ", err)
+                end


Suggested change

if err ~= "closed" and

err ~= "timeout" and

err ~= "broken pipe" then

log.error("wait watch event: ", err)

end

if err ~= "closed" and

err ~= "timeout" and

err ~= "broken pipe"

then

log.error("wait watch event: ", err)

end

juststillthinking · 2023-05-19T14:06:02Z

+                end
+            end
+
+            for i = 1,min_idx-1 do


Suggested change

for i = 1,min_idx-1 do

for i = 1, min_idx-1 do

juststillthinking · 2023-05-19T14:07:09Z

+
+            if min_idx > 100 then
+                for k, idx in pairs(watch_ctx.idx) do
+                    watch_ctx.idx[k] = idx-min_idx+1


Suggested change

watch_ctx.idx[k] = idx-min_idx+1

watch_ctx.idx[k] = idx - min_idx + 1

juststillthinking · 2023-05-19T14:09:04Z

+                    watch_ctx.idx[k] = idx-min_idx+1
+                end
+                -- trim the res table
+                for i = 1,min_idx-1 do


Suggested change

for i = 1,min_idx-1 do

for i = 1, min_idx - 1 do

juststillthinking · 2023-05-19T14:12:03Z

-        res, err = res_func()
-    end
+    ::iterate_events::
+    for i = watch_ctx.idx[key],#watch_ctx.res do


Suggested change

for i = watch_ctx.idx[key],#watch_ctx.res do

for i = watch_ctx.idx[key], #watch_ctx.res do

juststillthinking · 2023-05-19T14:12:22Z

-    end
+    ::iterate_events::
+    for i = watch_ctx.idx[key],#watch_ctx.res do
+        watch_ctx.idx[key] = i+1


Suggested change

watch_ctx.idx[key] = i+1

watch_ctx.idx[key] = i + 1

juststillthinking · 2023-05-19T15:42:57Z

Why do you call this "long http connection", the http_cli will be recreated every time: https://github.com/api7/lua-resty-etcd/blob/master/lib/resty/etcd/v3.lua#L803?

kingluo · 2023-05-19T15:50:10Z

Why do you call this "long http connection", the http_cli will be recreated every time: https://github.com/api7/lua-resty-etcd/blob/master/lib/resty/etcd/v3.lua#L803?

        ::watch_event::
        while true do
            local res, err = res_func()
            log.info("res_func: ", inspect(res))

Please read the code carefully.
Once the http connection for watching is created, it keeps receiving the watch event chunks, unless it is timeout or compacted.
So is it long or not?

…d streaming

juststillthinking · 2023-05-22T09:14:20Z

+
+        ::watch_event::
+        while true do
+            local res, err = res_func()


It's very good design here, the server response is a stream, if we don't close the connection here, we could get the response event one by one

juststillthinking · 2023-05-23T04:15:00Z

+    if log_level >= NGX_INFO then
+        log.info("append res: ", inspect(res), ", err: ", inspect(err))
+    end


Why not use json.delay_encode here?

the delay stuff has bug:

apisix/apisix/core/json.lua

Lines 116 to 120 in a943c03

function _M.delay_encode(data, force)

delay_tab.data = data

delay_tab.force = force

return delay_tab

end

It only uses a singleton table to log, but here I need two vars to log.
And inspect is more informational than json for debugging.

…ources (apache#9456)

kingluo changed the title ~~feat(config_etcd): use only one http connection to watch all resources~~ feat: config_etcd: use only one http connection to watch all resources May 10, 2023

kingluo changed the title ~~feat: config_etcd: use only one http connection to watch all resources~~ feat(config_etcd): use only one http connection to watch all resources May 11, 2023

kingluo changed the title ~~feat(config_etcd): use only one http connection to watch all resources~~ feat(config_etcd): use only one long http connection to watch all resources May 11, 2023

kingluo force-pushed the etcd-http-watch-optimize branch from 66bd5bc to 1c0102f Compare May 11, 2023 06:25

kingluo changed the title ~~feat(config_etcd): use only one long http connection to watch all resources~~ feat(config_etcd): use a single long http connection to watch all resources May 11, 2023

kingluo commented May 11, 2023

View reviewed changes

kingluo commented May 12, 2023

View reviewed changes

kingluo commented May 13, 2023

View reviewed changes

kingluo marked this pull request as ready for review May 13, 2023 16:08

kingluo commented May 13, 2023

View reviewed changes

kingluo requested review from juststillthinking, leslie-tsang and soulbird May 13, 2023 16:20

juststillthinking reviewed May 19, 2023

View reviewed changes

kingluo added 9 commits May 22, 2023 14:33

feat(config_etcd): use only one http connection to watch, with chunke…

f256cfb

…d streaming

fix PR

4c4d64a

fix PR

23a7287

fix PR

55bb0ae

fix PR

2dcf9bc

fix PR

6fc4613

fix PR

d09a6bb

fix PR

36e2a76

code formating fix

760f9e6

kingluo added 2 commits May 22, 2023 14:33

log inspect for ngx.INFO

a9de0f2

fix PR

4eb055b

kingluo force-pushed the etcd-http-watch-optimize branch from 683f936 to 4eb055b Compare May 22, 2023 06:33

juststillthinking reviewed May 22, 2023

View reviewed changes

juststillthinking reviewed May 23, 2023

View reviewed changes

Comment thread apisix/core/config_etcd.lua Outdated

juststillthinking reviewed May 23, 2023

View reviewed changes

Comment thread apisix/core/config_etcd.lua

fix PR

70f643b

juststillthinking reviewed May 23, 2023

View reviewed changes

leslie-tsang approved these changes May 23, 2023

View reviewed changes

juststillthinking approved these changes May 23, 2023

View reviewed changes

juststillthinking merged commit 4377d05 into apache:master May 23, 2023

leslie-tsang added the need backport label Jun 28, 2023

hansedong mentioned this pull request Jul 4, 2023

bug: error_log report error in the way apisix connected to etcd using gRPC #9336

Closed

AlinsRan pushed a commit to AlinsRan/apisix that referenced this pull request Jul 7, 2023

feat(config_etcd): use a single long http connection to watch all res…

30a7f4c

…ources (apache#9456)

This was referenced Jul 11, 2023

fix: update_count is reset once updated, cause cache key conflict #9811

Merged

bug: config_util.lua:99: attempt to index local 'item' (a boolean value) #9822

Closed

wklken mentioned this pull request Aug 2, 2023

bug: route 404 after upgrade to 3.2.2 #9951

Closed

jiangfucheng mentioned this pull request Aug 8, 2023

fix: worker not exited when executing quit or reload command #9909

Merged

5 tasks

juststillthinking mentioned this pull request Dec 7, 2023

fix: memory leak caused by timer that never quit #10614

Merged

5 tasks

wklken mentioned this pull request Dec 8, 2023

help request: memory growing up #10392

Closed

sheharyaar mentioned this pull request Feb 18, 2024

fix: remove grpc-client-nginx-module api7/apisix-build-tools#370

Merged

	watch_ctx.idx[k] = idx-min_idx+1
	watch_ctx.idx[k] = idx - min_idx + 1

	for i = watch_ctx.idx[key],#watch_ctx.res do
	for i = watch_ctx.idx[key], #watch_ctx.res do

	function _M.delay_encode(data, force)
	delay_tab.data = data
	delay_tab.force = force
	return delay_tab
	end

Uh oh!

Conversation

kingluo commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kingluo May 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kingluo May 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juststillthinking commented May 19, 2023

Uh oh!

kingluo commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kingluo commented May 10, 2023 •

edited

Loading

kingluo May 13, 2023 •

edited

Loading

kingluo May 13, 2023 •

edited

Loading

kingluo commented May 19, 2023 •

edited

Loading