You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2020/08/03 04:19:26 UTC
[GitHub] [apisix] membphis opened a new pull request #1971: bugfix: create the health checker in `access` phase, allow to call `y…
membphis opened a new pull request #1971:
URL: https://github.com/apache/apisix/pull/1971
…ield` function,
the `healchecheck` need this when creating new object.
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
as title.
In `balancer` phase, it not support `yield`, then we can not call `resty.lock`, here the source link: https://github.com/api7/lua-resty-healthcheck/blob/master/lib/resty/healthcheck.lua#L186
so we need to create the health checker in another phase like `access` which is support `yield`.
### 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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #1971: bugfix: create the health checker in `access` phase, allow to call `y…
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #1971:
URL: https://github.com/apache/apisix/pull/1971#issuecomment-755049314
I will take care of it.
But since it is on @membphis's branch (which I can't easily operate) and there are many conflicts, I prefer to do it in a new PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #1971: bugfix: create the health checker in `access` phase, allow to call `y…
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #1971:
URL: https://github.com/apache/apisix/pull/1971#issuecomment-754693992
Wow, this is an old PR. I will process it when I have time.
If any other guys want to handle this feature, please let me know, then you can continue this job. ^_^
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] nic-chen commented on pull request #1971: bugfix: create the health checker in `access` phase, allow to call `y…
Posted by GitBox <gi...@apache.org>.
nic-chen commented on pull request #1971:
URL: https://github.com/apache/apisix/pull/1971#issuecomment-697153410
add some test cases for the changing?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on pull request #1971: bugfix: create the health checker in `access` phase, allow to call `y…
Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #1971:
URL: https://github.com/apache/apisix/pull/1971#issuecomment-754299790
ping @membphis
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #1971: bugfix: create the health checker in `access` phase, allow to call `y…
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #1971:
URL: https://github.com/apache/apisix/pull/1971#issuecomment-757938548
Surpassed by https://github.com/apache/apisix/pull/3240
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #1971: bugfix: create the health checker in `access` phase, allow to call `y…
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #1971:
URL: https://github.com/apache/apisix/pull/1971#issuecomment-755926244
we can close this PR after the new PR is created ^_^
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] redynasc commented on a change in pull request #1971: bugfix: create the health checker in `access` phase, allow to call `y…
Posted by GitBox <gi...@apache.org>.
redynasc commented on a change in pull request #1971:
URL: https://github.com/apache/apisix/pull/1971#discussion_r466086252
##########
File path: apisix/upstream.lua
##########
@@ -52,41 +60,112 @@ end
_M.set = set_directly
+local function create_checker(upstream, healthcheck_parent)
+ if healthcheck == nil then
+ healthcheck = require("resty.healthcheck")
+ end
+ local checker = healthcheck.new({
+ name = "upstream#" .. healthcheck_parent.key,
+ shm_name = "upstream-healthcheck",
+ checks = upstream.checks,
+ })
+
+ local host = upstream.checks and upstream.checks.active
+ and upstream.checks.active.host
+ local port = upstream.checks and upstream.checks.active
+ and upstream.checks.active.port
+ for _, node in ipairs(upstream.nodes) do
+ local ok, err = checker:add_target(node.host, port or node.port, host)
+ if not ok then
+ core.log.error("failed to add new health check target: ",
+ node.host, ":", port or node.port, " err: ", err)
+ end
+ end
+
+ if upstream.parent then
+ core.table.insert(upstream.parent.clean_handlers, function ()
+ core.log.info("try to release checker: ", tostring(checker))
+ checker:stop()
+ end)
+
+ else
+ core.table.insert(healthcheck_parent.clean_handlers, function ()
+ core.log.info("try to release checker: ", tostring(checker))
+ checker:stop()
+ end)
+ end
+
+ core.log.info("create new checker: ", tostring(checker))
+ return checker
+end
+
+
+local function fetch_healthchecker(upstream, healthcheck_parent, version)
+ if not upstream.checks then
+ return
+ end
+
+ local checker = lrucache_checker(upstream, version,
+ create_checker, upstream,
+ healthcheck_parent)
+ return checker
+end
+
+
function _M.set_by_route(route, api_ctx)
if api_ctx.upstream_conf then
core.log.warn("upstream node has been specified, ",
"cannot be set repeatedly")
- return true
+ return
end
local up_id = route.value.upstream_id
if up_id then
if not upstreams then
- return false, "need to create a etcd instance for fetching "
+ return 500, "need to create a etcd instance for fetching "
.. "upstream information"
end
local up_obj = upstreams:get(tostring(up_id))
if not up_obj then
- return false, "failed to find upstream by id: " .. up_id
+ return 500, "failed to find upstream by id: " .. up_id
end
core.log.info("upstream: ", core.json.delay_encode(up_obj))
local up_conf = up_obj.dns_value or up_obj.value
set_directly(api_ctx, up_conf.type .. "#upstream_" .. up_id,
up_obj.modifiedIndex, up_conf, up_obj)
- return true
+ return
end
local up_conf = (route.dns_value and route.dns_value.upstream)
or route.value.upstream
if not up_conf then
- return false, "missing upstream configuration in Route or Service"
+ return 500, "missing upstream configuration in Route or Service"
+ end
+
+ if up_conf.service_name then
+ if not discovery then
+ return 500, "discovery is uninitialized"
+ end
+ up_conf.nodes = discovery.nodes(up_conf.service_name)
end
set_directly(api_ctx, up_conf.type .. "#route_" .. route.value.id,
api_ctx.conf_version, up_conf, route)
- return true
+
+ local nodes_count = up_conf.nodes and #up_conf.nodes or 0
+ if nodes_count == 0 then
+ return 502, "no valid upstream node"
+ end
+
+ if nodes_count > 1 then
+ local checker = fetch_healthchecker(up_conf, route,
Review comment:
I think it might be better to move the creation of health-checker object to the set_directly function，
Sometimes, I call the set_directly function directly in the plugin，proxy to another uptream instead of the configuration in the route.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #1971: bugfix: create the health checker in `access` phase, allow to call `y…
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #1971:
URL: https://github.com/apache/apisix/pull/1971#discussion_r472149623
##########
File path: apisix/upstream.lua
##########
@@ -52,41 +60,112 @@ end
_M.set = set_directly
+local function create_checker(upstream, healthcheck_parent)
+ if healthcheck == nil then
+ healthcheck = require("resty.healthcheck")
+ end
+ local checker = healthcheck.new({
+ name = "upstream#" .. healthcheck_parent.key,
+ shm_name = "upstream-healthcheck",
+ checks = upstream.checks,
+ })
+
+ local host = upstream.checks and upstream.checks.active
+ and upstream.checks.active.host
+ local port = upstream.checks and upstream.checks.active
+ and upstream.checks.active.port
+ for _, node in ipairs(upstream.nodes) do
+ local ok, err = checker:add_target(node.host, port or node.port, host)
+ if not ok then
+ core.log.error("failed to add new health check target: ",
+ node.host, ":", port or node.port, " err: ", err)
+ end
+ end
+
+ if upstream.parent then
+ core.table.insert(upstream.parent.clean_handlers, function ()
+ core.log.info("try to release checker: ", tostring(checker))
+ checker:stop()
+ end)
+
+ else
+ core.table.insert(healthcheck_parent.clean_handlers, function ()
+ core.log.info("try to release checker: ", tostring(checker))
+ checker:stop()
+ end)
+ end
+
+ core.log.info("create new checker: ", tostring(checker))
+ return checker
+end
+
+
+local function fetch_healthchecker(upstream, healthcheck_parent, version)
+ if not upstream.checks then
+ return
+ end
+
+ local checker = lrucache_checker(upstream, version,
+ create_checker, upstream,
+ healthcheck_parent)
+ return checker
+end
+
+
function _M.set_by_route(route, api_ctx)
if api_ctx.upstream_conf then
core.log.warn("upstream node has been specified, ",
"cannot be set repeatedly")
- return true
+ return
end
local up_id = route.value.upstream_id
if up_id then
if not upstreams then
- return false, "need to create a etcd instance for fetching "
+ return 500, "need to create a etcd instance for fetching "
.. "upstream information"
end
local up_obj = upstreams:get(tostring(up_id))
if not up_obj then
- return false, "failed to find upstream by id: " .. up_id
+ return 500, "failed to find upstream by id: " .. up_id
end
core.log.info("upstream: ", core.json.delay_encode(up_obj))
local up_conf = up_obj.dns_value or up_obj.value
set_directly(api_ctx, up_conf.type .. "#upstream_" .. up_id,
up_obj.modifiedIndex, up_conf, up_obj)
- return true
+ return
end
local up_conf = (route.dns_value and route.dns_value.upstream)
or route.value.upstream
if not up_conf then
- return false, "missing upstream configuration in Route or Service"
+ return 500, "missing upstream configuration in Route or Service"
+ end
+
+ if up_conf.service_name then
+ if not discovery then
+ return 500, "discovery is uninitialized"
+ end
+ up_conf.nodes = discovery.nodes(up_conf.service_name)
end
set_directly(api_ctx, up_conf.type .. "#route_" .. route.value.id,
api_ctx.conf_version, up_conf, route)
- return true
+
+ local nodes_count = up_conf.nodes and #up_conf.nodes or 0
+ if nodes_count == 0 then
+ return 502, "no valid upstream node"
+ end
+
+ if nodes_count > 1 then
+ local checker = fetch_healthchecker(up_conf, route,
Review comment:
we can add this in new PR ^_^
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #1971: bugfix: create the health checker in `access` phase, allow to call `y…
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #1971:
URL: https://github.com/apache/apisix/pull/1971#issuecomment-697146835
@nic-chen @spacewander @moonming Do you have time to take a look at this PR?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander closed pull request #1971: bugfix: create the health checker in `access` phase, allow to call `y…
Posted by GitBox <gi...@apache.org>.
spacewander closed pull request #1971:
URL: https://github.com/apache/apisix/pull/1971
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org