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/07/20 06:30:45 UTC
[GitHub] [incubator-apisix] shuaijinchao opened a new pull request #1871: bugfix: failed to get `host` in health check configuration.
shuaijinchao opened a new pull request #1871:
URL: https://github.com/apache/incubator-apisix/pull/1871
### 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. -->
### Pre-submission checklist:
FIX #1869
* [x] 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?
* [x] 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] [incubator-apisix] shuaijinchao commented on a change in pull request #1871: bugfix: failed to get `host` in health check configuration.
Posted by GitBox <gi...@apache.org>.
shuaijinchao commented on a change in pull request #1871:
URL: https://github.com/apache/incubator-apisix/pull/1871#discussion_r458490782
##########
File path: t/node/healthcheck.t
##########
@@ -548,3 +548,73 @@ code: 200 body: passed
delete code: 200
--- no_error_log
[error]
+
+
+
+=== TEST 12: add route (test health check config valid)
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "uri": "/server_port",
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": {
+ "127.0.0.1:1980": 1,
+ "127.0.0.1:1988": 1
+ },
+ "checks": {
+ "active": {
+ "http_path": "/status",
+ "host": "foo.com",
+ "healthy": {
+ "interval": 1,
+ "successes": 1
+ },
+ "unhealthy": {
+ "interval": 1,
+ "http_failures": 2
+ }
+ }
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+
+
+
+=== TEST 13: test health check config valid
+--- config
+ location /t {
+ content_by_lua_block {
+ local http = require "resty.http"
+ local uri = "http://127.0.0.1:" .. ngx.var.server_port
+ .. "/server_port"
+
+ local httpc = http.new()
+ local res, err = httpc:request_uri(uri, {method = "GET", keepalive = false})
+ ngx.say(res.status)
Review comment:
already fixed
----------------------------------------------------------------
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] [incubator-apisix] membphis commented on pull request #1871: bugfix: failed to get `host` in health check configuration.
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #1871:
URL: https://github.com/apache/incubator-apisix/pull/1871#issuecomment-664132215
> @membphis this is a bug fix, using the previous data structure, so it is backward compatible.
ok, got it. let us merge 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] [incubator-apisix] membphis merged pull request #1871: bugfix: failed to get `host` in health check configuration.
Posted by GitBox <gi...@apache.org>.
membphis merged pull request #1871:
URL: https://github.com/apache/incubator-apisix/pull/1871
----------------------------------------------------------------
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] [incubator-apisix] membphis commented on pull request #1871: bugfix: failed to get `host` in health check configuration.
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #1871:
URL: https://github.com/apache/incubator-apisix/pull/1871#issuecomment-661858517
I check the source code, and I found two points.
> the healthcheck was ignored
1. if the count of upstream node is 1, it will return the upstream node directly, the healthcheck was ignored.
https://github.com/apache/incubator-apisix/blob/master/apisix/balancer.lua#L180-L185
I think we can create a new PR to fix this bug first.
> The health check prints the log only when the status of the upstream node is changed
The default status is SUCCESS, if the status is SUCCESS always, we never can get a log like `healthy SUCCESS`.
----------------------------------------------------------------
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] [incubator-apisix] membphis commented on a change in pull request #1871: bugfix: failed to get `host` in health check configuration.
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #1871:
URL: https://github.com/apache/incubator-apisix/pull/1871#discussion_r458483388
##########
File path: t/node/healthcheck.t
##########
@@ -548,3 +548,73 @@ code: 200 body: passed
delete code: 200
--- no_error_log
[error]
+
+
+
+=== TEST 12: add route (test health check config valid)
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "uri": "/server_port",
+ "upstream": {
+ "type": "roundrobin",
+ "nodes": {
+ "127.0.0.1:1980": 1,
+ "127.0.0.1:1988": 1
+ },
+ "checks": {
+ "active": {
+ "http_path": "/status",
+ "host": "foo.com",
+ "healthy": {
+ "interval": 1,
+ "successes": 1
+ },
+ "unhealthy": {
+ "interval": 1,
+ "http_failures": 2
+ }
+ }
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+
+
+
+=== TEST 13: test health check config valid
+--- config
+ location /t {
+ content_by_lua_block {
+ local http = require "resty.http"
+ local uri = "http://127.0.0.1:" .. ngx.var.server_port
+ .. "/server_port"
+
+ local httpc = http.new()
+ local res, err = httpc:request_uri(uri, {method = "GET", keepalive = false})
+ ngx.say(res.status)
Review comment:
add a sleep time is safer, eg: `ngx.sleep(2)`
Checking the status of upstream nodes is asynchronous.
----------------------------------------------------------------
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] [incubator-apisix] shuaijinchao commented on pull request #1871: bugfix: failed to get `host` in health check configuration.
Posted by GitBox <gi...@apache.org>.
shuaijinchao commented on pull request #1871:
URL: https://github.com/apache/incubator-apisix/pull/1871#issuecomment-661591039
@membphis the test case cannot get the warn log thrown by the health check, please help to check.
----------------------------------------------------------------
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] [incubator-apisix] shuaijinchao commented on pull request #1871: bugfix: failed to get `host` in health check configuration.
Posted by GitBox <gi...@apache.org>.
shuaijinchao commented on pull request #1871:
URL: https://github.com/apache/incubator-apisix/pull/1871#issuecomment-661919283
> I check the source code, and I found two points.
>
> > the healthcheck was ignored
>
> 1. if the count of upstream node is 1, it will return the upstream node directly, the healthcheck was ignored.
>
> https://github.com/apache/incubator-apisix/blob/master/apisix/balancer.lua#L180-L185
>
> I think we can create a new PR to fix this bug first.
>
> > The health check prints the log only when the status of the upstream node is changed
>
> The default status is SUCCESS, if the status is SUCCESS always, we never can get a log like `healthy SUCCESS`.
1. If the number of nodes is 1 and the node is a faulty node, the health check is of little significance at this time, and APISIX will also have a retry mechanism.
2. adding a faulty node to trigger the status change of the health check, the verification of the test case is completed.
Thanks @membphis for your help.
----------------------------------------------------------------
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] [incubator-apisix] membphis commented on pull request #1871: bugfix: failed to get `host` in health check configuration.
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #1871:
URL: https://github.com/apache/incubator-apisix/pull/1871#issuecomment-664133602
@shuaijinchao merged, many thx
----------------------------------------------------------------
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] [incubator-apisix] membphis commented on pull request #1871: bugfix: failed to get `host` in health check configuration.
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #1871:
URL: https://github.com/apache/incubator-apisix/pull/1871#issuecomment-663918972
@shuaijinchao
> Is this PR backward compatible?
I think this PR is not backward compatible. please confirm this.
----------------------------------------------------------------
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] [incubator-apisix] shuaijinchao commented on pull request #1871: bugfix: failed to get `host` in health check configuration.
Posted by GitBox <gi...@apache.org>.
shuaijinchao commented on pull request #1871:
URL: https://github.com/apache/incubator-apisix/pull/1871#issuecomment-664131106
> @shuaijinchao
>
> > Is this PR backward compatible?
>
> I think this PR is not backward compatible. please confirm this.
@membphis this is a bug fix, using the previous data structure, so it is 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