feat: server info by tokers · Pull Request #2926 · apache/apisix

tokers · 2020-12-02T06:56:02Z

What this PR does / why we need it:

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

membphis · 2020-12-02T08:32:50Z

+## Test Plugin
+
+```bash
+curl http://127.0.0.1:9080/apisix/admin/server_info -s | jq


the URI should be /apisix/server_info

membphis · 2020-12-02T08:32:59Z

+## 测试插件
+
+```bash
+curl http://127.0.0.1:9080/apisix/admin/server_info -s | jq


@membphis FIxed.

membphis · 2020-12-02T08:33:52Z

+            type = "integer",
+            description = "server info reporting interval (unit: second)",
+            default = 60,
+            minimum = 5,


need to specify a maximum value

spacewander · 2020-12-02T08:46:34Z

+        {
+            methods = {"GET"},
+            uri = "/apisix/server_info",
+            handler = get_server_info,


@tokers @membphis
As #2884 (comment), we should not expose the data from the route API.
plugin interceptor is a hack, a fix! Don't rely on it any more!

That is to say we haven't a way to expose these data in data plane, at least for now.

Yes, except we can expose it via etcd.

spacewander · 2020-12-02T08:49:02Z

+    -- 1. core.timer is not cancalable, timers will be leaked if plugin
+    -- reloading happens.
+    -- 2. the background timer in apisix.timers fires per 500 milliseconds, if
+    -- report_interval is not multiple of 0.5, the real report interval will be


The unit of report_interval is second, and it is an integer. Do I miss some cases that an integer is not a multiple of 0.5?

I forgot to remove the second comment.

But the backend timer is not suitable, we use backend timer without caring the fire period, if we use it here, we have to do some calculations according to the 500 milliseconds, which must be updated once the backend timer fire period changed.

we have to do some calculations according to the 500 milliseconds, which must be updated once the backend timer fire period changed

We can expose the 0.5 as a constant, and used in this plugin. It would be better than inventing a new cycle.

Well, if we do so, the interval must be a multiple of 0.2, 0.5. I think it should be done in another PR.

Well, if we do so, the interval must be a multiple of 0.2, 0.5. I think it should be done in another PR.

All right, i changed my mind, i will also export the check interval in this PR.

spacewander · 2020-12-02T09:02:10Z

+    end
+
+    local key = "/data_plane/server_info/" .. server_info.id
+    local ok, err = core.etcd.set(key, data, 180)


I think it would be better to let the CP clean up the data. For example, if the DP is just shutdowned for maintain, it would be better to keep the info data in etcd.

BTW, 180 is rather short compared with the 60 second report internal. And the number is undocumented. People will shout at us when they find their data is disappeared.

I think it would be better to let the CP clean up the data. For example, if the DP is just shutdowned for maintain, it would be better to keep the info data in etcd.

That can be solved by a long TTL.

BTW, 180 is rather short compared with the 60 second report internal. And the number is undocumented. People will shout at us when they find their data is disappeared.

OK, we can add another item like report_ttl in the plugin_attr.

spacewander · 2020-12-02T09:52:21Z

+        return
+    end
+
+    if server_info.etcd_version == "unknown" then


What if APISIX runs under standalone mode?

Since now server-info plugin only reports the basic info to etcd, when APISIX runs under standalone mode, we don't need to launch the timer.

tokers · 2020-12-06T03:00:32Z

@membphis @moonming Please take a look when you have time~

membphis · 2020-12-06T13:11:42Z

  - uri-blocker
  - wolf-rbac
  - zipkin
+  - server-info


we should disable this plugin by default

What's the reason for disabling this plugin by default? Users are no feeling about the behavior of server-info.

membphis · 2020-12-06T13:13:16Z

+| 名称    | 类型 | 描述 |
+|---------|------|-------------|
+| up_time | integer | APISIX 服务实例当前的运行时间, 如果对 APSIX
+进行热更新操作，该值将被重置；普通的 reload 操作不会影响该值。 |


need a test case to confirm this

membphis · 2020-12-06T13:17:46Z

+        hostname = ffi_string(buf, ffi.C.strlen(buf))
+
+    else
+        log.alert("ffi.C.gethostname() failed")


make a try:

use io.popen("/bin/hostname") to fetch the host name

That'd be concise, i will try it.

membphis · 2020-12-07T08:51:20Z

@tokers CI is broken. You need to fix it:

https://github.com/apache/apisix/pull/2926/checks?check_run_id=1509266841#step:11:286

#   Failed test 'TEST 3: fix: ensure only one timer is running - response_body - response is expected (repeated req 0, req 0)'
#   at /home/runner/work/apisix/apisix/test-nginx/lib/Test/Nginx/Socket.pm line 1589.
#          got: 'done
# not a single rotater run at the same time: 2020-12-07_08-05-09__error.log
# '
#     expected: 'done
# '
# Looks like you failed 1 test of 12.
t/plugin/log-rotate.t .....................

membphis

LGTM

tokers mentioned this pull request Dec 2, 2020

feat: server info #2884

Closed

4 tasks

juzhiyuan requested review from liuxiran, membphis, moonming and spacewander and removed request for spacewander December 2, 2020 07:28

membphis reviewed Dec 2, 2020

View reviewed changes

spacewander requested changes Dec 2, 2020

View reviewed changes

starsz reviewed Dec 2, 2020

View reviewed changes

Comment thread apisix/plugins/server-info.lua

spacewander reviewed Dec 2, 2020

View reviewed changes

tokers added 14 commits December 6, 2020 11:01

feat: server info

c66232a

fix: fixed broken markdown links and test cases failure

b47597b

fix: bad server_info module prefix

a2fa893

fix: keep the require statement for ngx.process inside http subsystem

f2f3e39

fix: required server_info module at init_worker phase in admin init

c84ad97

fix: fixed CI failure

4bd7a3e

chore: use plugin way to expose seever info endpoint

61d2695

chore: tweaked code according to the code review comments

921b660

style: revered some styles

f67fdc8

fix: fixed the broken test cases

acd5726

fix: fixed the broken debug-mode.t

3dd0086

fix: tweaked code according to the code review comments

fc11d3d

fix: removed plugin API and use background timer

02093a2

fix: fixed the broken test case

803d75b

tokers force-pushed the feat/status branch from 2a04e5e to 803d75b Compare December 6, 2020 03:08

membphis reviewed Dec 6, 2020

View reviewed changes

tokers added 2 commits December 7, 2020 14:26

chore: tweaked code according to the code review comments.

48bb247

doc: fixed the abrupt bold link

3561437

doc: fixed the unexpected broken line

e57628d

membphis added this to the 2.2 milestone Dec 7, 2020

membphis added the enhancement New feature or request label Dec 7, 2020

fix: broken test

2b65de4

membphis mentioned this pull request Dec 7, 2020

frontend APISIX instance status page apache/apisix-dashboard#850

Closed

spacewander requested changes Dec 7, 2020

View reviewed changes

Comment thread doc/plugins/server-info.md Outdated

Comment thread apisix/plugins/server-info.lua

Comment thread apisix/plugins/server-info.lua Outdated

Comment thread apisix/plugins/server-info.lua

Comment thread doc/plugins/server-info.md Outdated

fix: fixed some problems according to the code review

8ac598f

spacewander approved these changes Dec 9, 2020

View reviewed changes

membphis approved these changes Dec 9, 2020

View reviewed changes

membphis merged commit 7855d9e into apache:master Dec 9, 2020

juzhiyuan mentioned this pull request Mar 7, 2021

plugin server-info will write data to etcd #3775

Closed

Uh oh!

Conversation

tokers commented Dec 2, 2020

What this PR does / why we need it:

Pre-submission checklist:

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tokers commented Dec 6, 2020

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!

Choose a reason for hiding this comment

Uh oh!

membphis commented Dec 7, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

membphis left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants