Skip to content

feat: server info#2884

Closed
tokers wants to merge 7 commits into
masterfrom
feat/status
Closed

feat: server info#2884
tokers wants to merge 7 commits into
masterfrom
feat/status

Conversation

@tokers

@tokers tokers commented Nov 28, 2020

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

Close #2821.

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

@tokers
tokers requested review from juzhiyuan, membphis and spacewander and removed request for membphis November 28, 2020 09:13
@membphis

Copy link
Copy Markdown
Member

CI failed, you need to fix it first

@tokers

tokers commented Nov 30, 2020

Copy link
Copy Markdown
Contributor Author

CI failed, you need to fix it first

Fixed, except the markdown link errors.

Comment thread apisix/admin/init.lua Outdated
handler = post_reload_plugins,
},
{
paths = [[/apisix/admin/server_info]],

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need do this in DP, not admin API

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, will change it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@membphis @tokers
The API should go to the control API: #2798.
There is security risk if we put this feature into a plugin. Like the prometheus metrics, we don't have a default way to protect them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the control API's milestone?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is not milestone. So far, we can hide the entry first.

## 测试插件

```bash
curl http://127.0.0.1:9080/apisix/admin/server_info -s | jq

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to update the path


## 注意事项

当使用 etcd 作为 APISIX 的数据中心的说话,服务信息将被周期性地上报到 etcd(目前的上报间隔是 5

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"当使用 etcd 作为 APISIX 的数据中心的说话"

I can not understand it

Comment thread t/plugin/server-info.t
--- timeout: 6
--- no_error_log
[error]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one blank line is enough at the end of file

Comment thread apisix/server_info.lua
end

local opts = {
check_interval = 5, -- in seconds

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 seconds is short, I think 10 mins should be good

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and we should allow the user to specify this field

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think 10 mins is too long, also for now, all the basic server info are static (except the up_time and last_report_time), a relative short period is more suitable so we can easily judge whether a node is healthy or unstable when we are viewing the server info on Dashboard.

What about 1 min or 2 mins?

Comment thread apisix/server_info.lua
check_interval = 5, -- in seconds
}

if core.config ~= require("apisix.core.config_etcd") then

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need some comment for why call return here

Comment thread apisix/server_info.lua
return nil, err
end

local key = "/data_plane/server_info/" .. server_info.id

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can not use data_plane here. CP will write the server info too.

how about /nodes/server_info/{node_id}?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key /nodes/server_info/{node_id} is good, but why CP also writes the server info?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@membphis @tokers
I think /data_plane/ is better.
It would be better to limit the write of DP to certain prefix, so that we can grant the privilege strictly.

Comment thread apisix/admin/init.lua
local core = require("apisix.core")
local route = require("resty.radixtree")
local plugin = require("apisix.plugin")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert this line

Comment thread apisix/admin/init.lua
local ipairs = ipairs
local error = error
local events

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

not related to this title of PR


By removing `server-info` in the plugin list of configure file `apisix/conf/config.yaml` and restart APISIX, you can diable `server-info` plugin easily.

```

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

format language?

@agile6v

agile6v commented Dec 2, 2020

Copy link
Copy Markdown
Member

Hi @tokers @membphis Remember to discuss on #962 If the data that change frequently is reported periodically to etcd. Not sure if etcd is suitable. What do you think on this point?

@tokers

tokers commented Dec 2, 2020

Copy link
Copy Markdown
Contributor Author

@agile6v We can add some limit to the report interval, such as the minimal report interval is 10s.

@membphis

membphis commented Dec 2, 2020

Copy link
Copy Markdown
Member

@agile6v We can add some limit to the report interval, such as the minimal report interval is 10s.

agree. I think the minimal report interval can be 1 min

@agile6v

agile6v commented Dec 2, 2020

Copy link
Copy Markdown
Member

@agile6v We can add some limit to the report interval, such as the minimal report interval is 10s.

agree. I think the minimal report interval can be 1 min

If only time changes frequently. I'm not sure if the lease mechanism of etcd can get the creation time to achieve this.

@tokers

tokers commented Dec 2, 2020

Copy link
Copy Markdown
Contributor Author

This PR was pushed directly at apache/apisix, i will close it and please see the new one #2926.

@tokers tokers closed this Dec 2, 2020
@spacewander spacewander mentioned this pull request Dec 2, 2020
4 tasks
@tokers
tokers deleted the feat/status branch January 4, 2021 01:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement status API

6 participants