change(server-info): use a new approach(keepalive) to report DP info #6202

zaunist · 2022-01-25T14:29:09Z

What this PR does / why we need it:

fix #6176

Pre-submission checklist:

Did you explain what problem does this PR solve? Or what new features have been added?
Have you added corresponding test cases?
Have you modified the corresponding document?
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

moonming · 2022-02-09T15:00:03Z

@zaunist any update？

zaunist · 2022-02-09T15:38:00Z

@zaunist any update？

I have update the plugin code, still need to update test case.

zaunist · 2022-02-10T06:36:02Z

cc @tzssangglass @shuaijinchao. Please take a look, thanks.

zaunist · 2022-02-10T07:31:42Z

@bisakhmondal @bzp2010

shuaijinchao · 2022-02-10T11:16:55Z

apisix/plugins/server-info.lua

+        if not newres then
+            core.log.error("failed to set server_info: ", err)
+        end
+
+        -- set lease_id to ngx dict
+        local ok, err = internal_status:set("lease_id", newres.body.lease_id)
+        if not ok then
+            core.log.error("failed to save boot_time to shdict: ", err)
+        end


should not continue to set ngx dict if newres fetch fails

shuaijinchao · 2022-02-10T11:18:59Z

apisix/plugins/server-info.lua

    local key = "/data_plane/server_info/" .. server_info.id
-    local ok, err = core.etcd.set(key, server_info, report_ttl)
+    local res, _ = core.etcd.get(key)


need to judge failure

shuaijinchao · 2022-02-10T11:19:35Z

apisix/plugins/server-info.lua

+        return
+    end
+
+    local res, _ = core.etcd.get(key)


shuaijinchao · 2022-02-10T11:21:10Z

apisix/plugins/server-info.lua

@@ -203,8 +236,7 @@ end


 function _M.init()
-    core.log.info("server info: ", core.json.delay_encode(get()))
-
+    -- core.log.info("server info: ", core.json.delay_encode(get()))


remove debug and redundant code

shuaijinchao · 2022-02-10T11:21:18Z

apisix/plugins/server-info.lua

@@ -217,13 +249,14 @@ function _M.init()
        return
    end

+    -- local report_ttl = attr and attr.report_ttl or default_report_ttl


shuaijinchao · 2022-02-10T11:25:39Z

misoperation approved, please correct the above problems first.

apisix/plugins/server-info.lua

tokers · 2022-02-11T01:22:54Z

apisix/plugins/server-info.lua

+    local ok = core.table.deep_eq(server_info, res.body.node.value)
+    -- not equal, update it
    if not ok then
-        core.log.error("failed to report server info to etcd: ", err)
+        local newres, err = core.etcd.set(key, server_info, report_ttl)
+        if not newres then
+            core.log.error("failed to set server_info to etcd: ", err)
+            return false, err
+        end
+
+        -- update lease_id
+        local _, err = internal_status:set("lease_id", res.body.lease_id)
+        if err ~= nil then
+            core.log.error("failed to set lease_id to shdict: ", err)
+        end


This paragraph is same to the one in the if not res.body.node then .... end block, we can create a function to reuse it.

apisix/plugins/server-info.lua

tokers · 2022-02-11T01:27:45Z

apisix/plugins/server-info.lua

+    end
+
+    if  not res.body.node then
+        local newres, err = core.etcd.set(key, server_info, report_ttl)


Also the key for individual instances should be unique but should we use the atomic_set method?

tokers · 2022-02-11T09:43:49Z

apisix/plugins/server-info.lua

    local server_info, err = get()
    if not server_info then
        core.log.error("failed to get server_info: ", err)
-        return


Why removing this?

Sorry, careless.

tokers · 2022-02-11T09:44:54Z

apisix/plugins/server-info.lua

+        core.log.error("failed to get server_info from etcd: ", err)
+    end
+
+    if  not res.body.node then


Suggested change

if not res.body.node then

if not res.body.node then

tokers · 2022-02-11T09:45:43Z

apisix/plugins/server-info.lua

-    local ok, err = core.etcd.set(key, server_info, report_ttl)
+    local res, err = core.etcd.get(key)
+    if err ~= nil then
+        core.log.error("failed to get server_info from etcd: ", err)


if err is not nil, why can we go ahead?

When a apisix node be initialed, the ETCD don't have server-info data, I think the get func will report error, so I continue the code and set the info to ETCD. Maybe use if not res be better.

No, if get method reports error when no data, you should check the error type and decide whether returning or going ahead.

tokers · 2022-02-11T09:47:32Z

apisix/plugins/server-info.lua

+    end
+
+    if  not res.body.node then
+        local res_new, err = core.etcd.set(key, server_info, report_ttl)


I think the logic for initial setting can be merged to the below logic (modified_version is zero).

So we don't have two similar code blocks.

Can we set modified_version to nil, I'm not sure if it can be set to nil.

I have already said you can set it to 0.

updated it.

tzssangglass · 2022-02-14T03:02:57Z

origin issue has only when the apisix node system information changes will there be a put operation. (e.g., hostname changes)

we need to add a test case to cover this?

zaunist · 2022-02-14T03:03:03Z

Hi, Community.

I have encountered follow problems.

When the server-info plugin is turned on by default, other plugins report errors, and I'm not sure if the errors reported here are the expected behaviour. https://github.com/apache/apisix/runs/5174938000?check_suite_focus=true#step:10:354
When the server-info plugin is turned off by default, errors are also reported in other checks, here the errors are due to a test of the configuration file. https://github.com/apache/apisix/runs/5177477142?check_suite_focus=true#step:10:355

Should I choose to turn on the server-info plugin by default or turn it off by default? If it is turned on by default, I don't know how to deal with the error here, can anyone give me some help?

zaunist · 2022-02-14T03:05:46Z

origin issue has only when the apisix node system information changes will there be a put operation. (e.g., hostname changes)

we need to add a test case to cover this?

How do I change the hostname in the .t test file? I'm not quite sure how to do it.

tzssangglass · 2022-02-14T03:05:03Z

apisix/plugins/server-info.lua

@@ -132,51 +125,107 @@ local function get_server_info()
 end


+local function set(key,value,ttl, modified_index)


Suggested change

local function set(key,value,ttl, modified_index)

local function set(key, value, ttl, modified_index)

tzssangglass · 2022-02-14T03:17:34Z

apisix/plugins/server-info.lua

+        return 503, {error_msg = err}
+    end
+
+    lease_id = res_new.body.lease_id


should we check if lease_id is not nil?

tzssangglass · 2022-02-14T03:18:55Z

apisix/plugins/server-info.lua

+    local res_new, err = core.etcd.atomic_set(key, value, ttl, modified_index)
+    if not res_new then
+        core.log.error("failed to set server_info: ", err)
+        return 503, {error_msg = err}
+    end
+
+    lease_id = res_new.body.lease_id
+
+    -- set or update lease_id
+    local ok, err = internal_status:set("lease_id", lease_id)
+    if not ok then
+        core.log.error("failed to set lease_id to shdict: ", err)
+        return 503, {error_msg = err}
+    end
+
+    return 200, nil
+end


It looks like the two steps should be in sync, and we can use lock in the next PR to keep them in sync.

tzssangglass · 2022-02-14T03:26:52Z

How do I change the hostname in the .t test file? I'm not quite sure how to do it.

It doesn't have to be hostname, it can be any other action that can be used to punish service info refresh.

zaunist · 2022-02-19T12:52:57Z

Hi, @tzssangglass . I checked core.id.get() and core.utils.gethostname(). It doesn't support updating ids and hostnames, it only gets them once when apisix starts, so it's not possible to update them in real time in the server-info plugin either. I'm sorry that this doesn't do what the proposal is trying to do. Maybe we can enhance core.id.get and core.utils.hostname to support regularly update id and hostname in a subsequent PR.

nic-chen

What we need to test is keepalive instead of getting server info, so we could modify the info stored in shared DICT to test.

nic-chen · 2022-02-19T14:13:47Z

apisix/plugins/server-info.lua

+    local res_new, err = core.etcd.set(key, value, ttl)
+    if not res_new then
+        core.log.error("failed to set server_info: ", err)
+        return 503, {error_msg = err}


why should we use 503 here?

503 Service Unavailable, I think it is OK to return this error when the plugin cannot provide the service, what do you suggestion?

I think 500 is better. 503 is commonly used for overload.

zaunist · 2022-02-19T15:37:50Z

What we need to test is keepalive instead of getting server info, so we could modify the info stored in shared DICT to test.

I have tried to change the server-info information in the share dict in the .t test file, but it did not work.

nic-chen · 2022-02-20T11:22:09Z

What we need to test is keepalive instead of getting server info, so we could modify the info stored in shared DICT to test.

I have tried to change the server-info information in the share dict in the .t test file, but it did not work.

you could push the test case again to see why it failed.

tzssangglass · 2022-02-20T12:40:27Z

Hi, @tzssangglass . I checked core.id.get() and core.utils.gethostname(). It doesn't support updating ids and hostnames, it only gets them once when apisix starts, so it's not possible to update them in real time in the server-info plugin either. I'm sorry that this doesn't do what the proposal is trying to do. Maybe we can enhance core.id.get and core.utils.hostname to support regularly update id and hostname in a subsequent PR.

My mistake, I should have pointed out that the test cases are written in shell script. Let's drop this test case for now and focus on finishing this PR.

zaunist · 2022-02-20T13:42:28Z

Hi, @tzssangglass . I checked core.id.get() and core.utils.gethostname(). It doesn't support updating ids and hostnames, it only gets them once when apisix starts, so it's not possible to update them in real time in the server-info plugin either. I'm sorry that this doesn't do what the proposal is trying to do. Maybe we can enhance core.id.get and core.utils.hostname to support regularly update id and hostname in a subsequent PR.

My mistake, I should have pointed out that the test cases are written in shell script. Let's drop this test case for now and focus on finishing this PR.

Yes, if we need to get the latest id and hostname etc., then we also need to update the core.id.lua and the code in core.utils.lua to support getting the latest status on a regular basis. My question is, do we have to do these changes in this PR?

tzssangglass · 2022-02-21T01:27:07Z

My question is, do we have to do these changes in this PR?

No need to change in this PR.

spacewander · 2022-02-22T02:29:45Z

apisix/plugins/server-info.lua

    end

    if server_info.etcd_version == "unknown" then
        local res, err = core.etcd.server_version()
        if not res then
            core.log.error("failed to fetch etcd version: ", err)
-            return
+            return 500, {error_msg = err}


Please use 503 like other similar cases.

spacewander · 2022-02-22T02:30:44Z

apisix/plugins/server-info.lua

+    lease_id, err = internal_status:get("lease_id")
+    if not lease_id then
+        core.log.error("failed to get lease_id from shdict: ", err)
+    end


Missing a return?

Already add return code block

spacewander · 2022-02-22T02:35:37Z

apisix/plugins/server-info.lua

    local data, err = core.json.encode(server_info)
    if not data then
        core.log.error("failed to encode server_info: ", err)
-        return
+        return false, err


The returned value is inconsistent?

And it seems the returned value of report is not checked:

apisix/apisix/plugins/server-info.lua

Line 278 in d5166c4

report(nil, report_ttl)

spacewander · 2022-02-22T02:44:06Z

@zaunist
I am curious why a shorter ttl is chosen?

zaunist · 2022-02-22T07:46:13Z

@zaunist I am curious why a shorter ttl is chosen?

Hi, @spacewander. Previously, server-info would write a lot of dirty data and cause performance problems with ETCD, but with keepalive, this problem is avoided and a simple http request does not take up any more performance. So a shorter ttl would allow for more timely reporting of Apache APISIX node status. One minute heartbeat is too long in my opinion, so it was changed to 36 seconds, or we could even set a shorter heartbeat, such as nacos, which has a default of 5 seconds.

https://github.com/alibaba/nacos/blob/47d3afd0e4de5019c09a921fff08ae66aed52600/api/src/main/java/com/alibaba/nacos/api/common/Constants.java#L173

spacewander · 2022-02-23T01:38:51Z

Yes. But keepalive request is still a write operation. Since this PR is to reduce the usage of etcd, using a lower interval will increase the number of requests to etcd.

One minute heartbeat is too long in my opinion, so it was changed to 36 seconds, or we could even set a shorter heartbeat, such as nacos, which has a default of 5 seconds

Nacos doesn't write the heartbeat to etcd...
Are there any issues relative to the heartbeat interval? We don't want to get in trouble because of an unnecessary change.

zaunist · 2022-02-23T02:36:09Z

Yes. But keepalive request is still a write operation. Since this PR is to reduce the usage of etcd, using a lower interval will increase the number of requests to etcd.

One minute heartbeat is too long in my opinion, so it was changed to 36 seconds, or we could even set a shorter heartbeat, such as nacos, which has a default of 5 seconds

Nacos doesn't write the heartbeat to etcd... Are there any issues relative to the heartbeat interval? We don't want to get in trouble because of an unnecessary change.

OK, I will change it to 60s.

spacewander · 2022-02-23T07:34:19Z

apisix/plugins/server-info.lua

+    end
+
+    if not res.body.node then
+        local ok, err = set(key, server_info, report_ttl)


It's strange that get returns nil, err while set returns status, table...
Can we use nil, err in both cases?

spacewander · 2022-02-23T07:34:34Z

apisix/plugins/server-info.lua

+        local ok, err = set(key, server_info, report_ttl)
+        if not ok then
+            core.log.error("failed to set server_info to etcd: ", err)
+            return 503, {error_msg = err}


Now we get a nested err table

Do I just return it here without the err? That means if there is an error, it will just be printed to error.log

see #6202 (comment)

Thanks for your reply.

spacewander · 2022-02-23T07:35:18Z

apisix/plugins/server-info.lua

+        local ok, err = set(key, server_info, report_ttl)
+        if not ok then
+            core.log.error("failed to set server_info to etcd: ", err)
+            return 503, {error_msg = err}


I think we should return directly as we don't (and can't) care about the result of report.

OK, I will fix it.

zaunist added 2 commits February 9, 2022 23:34

feat: use keepalive in server-info plugin

5d0b8cc

feat: support keepalive

a4a71a5

fix: lint style

847e967

zaunist marked this pull request as ready for review February 9, 2022 15:45

zaunist added 3 commits February 10, 2022 09:27

fix: test case

5da8ae6

fix: test case

3708a0e

docs: update description

431228f

shuaijinchao previously approved these changes Feb 10, 2022

View reviewed changes

fix: review

2a93790

zaunist dismissed shuaijinchao’s stale review via 2a93790 February 10, 2022 15:43

zaunist requested a review from shuaijinchao February 10, 2022 17:00

tokers reviewed Feb 11, 2022

View reviewed changes

zaunist added 2 commits February 11, 2022 14:18

fix: merge code to function for reuse

4c28650

fix: ci failed

eef4ff4

zaunist requested a review from tokers February 11, 2022 09:02

tokers reviewed Feb 11, 2022

View reviewed changes

fix: review

97a1447

zaunist requested a review from tokers February 13, 2022 16:57

fix: disable default server_info plugin

b1217f5

shuaijinchao requested a review from spacewander February 14, 2022 02:13

tzssangglass reviewed Feb 14, 2022

View reviewed changes

nic-chen reviewed Feb 19, 2022

View reviewed changes

tzssangglass previously approved these changes Feb 21, 2022

View reviewed changes

juzhiyuan requested review from nic-chen and tokers February 22, 2022 02:00

spacewander reviewed Feb 22, 2022

View reviewed changes

spacewander changed the title ~~feat: use keepalive in server-info plugin~~ change(server-info): use a new approach(keepalive) to report DP info Feb 23, 2022

nic-chen mentioned this pull request Feb 23, 2022

plugin server-info will write data to etcd #3775

Closed

fix: return error uniformly

dfc7601

zaunist dismissed tzssangglass’s stale review via dfc7601 February 23, 2022 02:48

zaunist requested review from spacewander and tzssangglass February 23, 2022 06:04

spacewander reviewed Feb 23, 2022

View reviewed changes

fix: remove redundant return message

e3ee7aa

spacewander approved these changes Feb 25, 2022

View reviewed changes

shuaijinchao approved these changes Feb 25, 2022

View reviewed changes

spacewander merged commit bcabe5f into apache:master Feb 25, 2022

zaunist mentioned this pull request Feb 25, 2022

feat(server_info): update gateway status apache/apisix-dashboard#2348

Open

leslie-tsang mentioned this pull request Mar 11, 2022

etcd cpu high in my test machine #6576

Closed

tzssangglass mentioned this pull request Jun 9, 2022

Bug: Etcd has heigh cpu usage #7207

Closed

		@@ -132,51 +125,107 @@ local function get_server_info()
		end


		local function set(key,value,ttl, modified_index)

	local function set(key,value,ttl, modified_index)
	local function set(key, value, ttl, modified_index)

change(server-info): use a new approach(keepalive) to report DP info #6202

change(server-info): use a new approach(keepalive) to report DP info #6202

Conversation

zaunist commented Jan 25, 2022 • edited Loading

What this PR does / why we need it:

Pre-submission checklist:

moonming commented Feb 9, 2022

zaunist commented Feb 9, 2022

zaunist commented Feb 10, 2022

zaunist commented Feb 10, 2022

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

shuaijinchao commented Feb 10, 2022

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

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

tzssangglass commented Feb 14, 2022

zaunist commented Feb 14, 2022

zaunist commented Feb 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzssangglass commented Feb 14, 2022

zaunist commented Feb 19, 2022

nic-chen left a 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

zaunist commented Feb 19, 2022 • edited Loading

nic-chen commented Feb 20, 2022

tzssangglass commented Feb 20, 2022

zaunist commented Feb 20, 2022

tzssangglass commented Feb 21, 2022

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

spacewander commented Feb 22, 2022

zaunist commented Feb 22, 2022

spacewander commented Feb 23, 2022

zaunist commented Feb 23, 2022

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

zaunist commented Jan 25, 2022 •

edited

Loading

zaunist commented Feb 19, 2022 •

edited

Loading