Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

no_timer option can work in non debug mode #68

Closed
wants to merge 16 commits into from

Conversation

wangrzneu
Copy link

remove debug_mode limit of no_timer option

@wangrzneu
Copy link
Author

wangrzneu commented Apr 1, 2020

PTAL @thibaultcha

@wangrzneu
Copy link
Author

/rebuild

@rainingmaster
Copy link
Member

maybe you can add some test case, so we can know the effect?

@@ -54,7 +54,6 @@ local function errlog(...)
end

local function debug(...)
-- print("debug mode: ", debug_mode)
Copy link
Member

Choose a reason for hiding this comment

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

better not introduce unrelated changes.

Copy link
Author

Choose a reason for hiding this comment

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

I have reverted these unrelated changes.

@doujiang24
Copy link
Member

@wangrzneu
Copy link
Author

@wangrzneu will you take care of the Travis failure thing?
https://travis-ci.org/github/openresty/lua-resty-upstream-healthcheck/jobs/670113565?utm_medium=notification&utm_source=github_status

Yes, I add a new case and I will try to make it pass.

@wangrzneu
Copy link
Author

maybe you can add some test case, so we can know the effect?

Done. I add TEST 16.

@wangrzneu
Copy link
Author

@wangrzneu will you take care of the Travis failure thing?
https://travis-ci.org/github/openresty/lua-resty-upstream-healthcheck/jobs/670113565?utm_medium=notification&utm_source=github_status

I have fixed it. But something wrong happens in travis CI.

@wangrzneu
Copy link
Author

@wangrzneu will you take care of the Travis failure thing?
https://travis-ci.org/github/openresty/lua-resty-upstream-healthcheck/jobs/670113565?utm_medium=notification&utm_source=github_status

I have fixed it. But something wrong happens in travis CI.

I fix CI by updating OPENSSL_VER.

@doujiang24
Copy link
Member

@wangrzneu
can you explain why you need this change?

@rainingmaster
Copy link
Member

I think the no_timer should only run in the debug mode, otherwise the health check will run one time only, and it can not check the issue of upsteams, right?

@wangrzneu
Copy link
Author

@wangrzneu
can you explain why you need this change?

This change will help me change the health check options without reloading the openresty process.

And the health check can be triggered using a timer outside the openresty process.

@wangrzneu
Copy link
Author

I think the no_timer should only run in the debug mode, otherwise the health check will run one time only, and it can not check the issue of upsteams, right?

The health check can be triggered using a timer outside the openresty process.

@doujiang24
Copy link
Member

And the health check can be triggered using a timer outside the openresty process.

@wangrzneu can you give a sample that how you use it? I don't think I got you.

@wangrzneu
Copy link
Author

wangrzneu commented Apr 8, 2020

And the health check can be triggered using a timer outside the openresty process.

@wangrzneu can you give a sample that how you use it? I don't think I got you.

My nginx.conf is like:

worker_processes  1;
error_log logs/error.log warn;

events {
    worker_connections 1024;
}

http {
    upstream foo.com {
        server 127.0.0.1:12345;
    }

    server {
        listen 12345;

        location = /status {
            content_by_lua_block {
                ngx.say("ok")
            }
        }
        location / {
            content_by_lua_block {
                ngx.say("foo.com")
            }
        }
    }
    lua_shared_dict healthcheck 1m;
    server {
        listen 8080;
        location / {
            proxy_pass http://foo.com;
        }
    }

    server {
        listen 9090;
        location /healthcheck {
            content_by_lua_block {
                local hc = require "resty.upstream.healthcheck"
                local args = ngx.req.get_uri_args()
                local ok, err = hc.spawn_checker{
                    shm = "healthcheck",
                    upstream = args["upstream"],
                    type = args["type"],
                    http_req = args["probe"] .. " HTTP/1.0\r\nHost: localhost\r\n\r\n",
                    timeout = tonumber(args["timeout"]),
                    fall = tonumber(args["fail"]),
                    no_timer = true,
                }
                if not ok then
                    ngx.log(ngx.ERR, "failed to spawn health checker: ", err)
                    return
                end
                ngx.say(hc.status_page()) 
            }
        }
    }
}

I use another program (implemented by golang) to request the /healthcheck periodically, and the arguments can be changed without restart openresty worker process.

The healthcheck request is like http://127.0.0.1:9090/healthcheck?upstream=foo.com&probe=GET%20/status&timeout=100&fail=1&type=http.

@@ -1451,3 +1451,82 @@ healthcheck: peer 127\.0\.0\.1:12355 was checked to be not ok
healthcheck: peer 127\.0\.0\.1:12356 was checked to be not ok
healthcheck: peer 127\.0\.0\.1:12359 was checked to be not ok
$/

=== TEST 16: effect of no_timer (healthcheck options can be changed)
Copy link
Member

Choose a reason for hiding this comment

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

style: we need three blank lines before this line. you are recommended to use the reindex tool to do it automatically:
https://github.com/openresty/openresty-devel-utils/blob/master/reindex

doc is still in the PR: https://github.com/openresty/openresty-devel-utils/pull/29/files#diff-04c6e90faac2675aa89e2176d2eec7d8R237

Copy link
Author

Choose a reason for hiding this comment

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

done

@doujiang24
Copy link
Member

@wangrzneu I think I got you now.

my opinion, it should be ok to merge it into master though it's not the recommended way to use this library.
this PR is a small change and it really helps @wangrzneu, so I vote for it.
@thibaultcha @spacewander @rainingmaster thoughts?

@spacewander
Copy link
Member

no_timer is confusing for produce, maybe we can rename it as once?

@rainingmaster
Copy link
Member

rainingmaster commented Apr 9, 2020

@doujiang24 I agree with you, the change is small and no impact on existing functions and parameters. And I think if we not recommend use this lib like this way, maybe the test cases should be more simple and check no_timer(or once is better) only? Because test case like here maybe make other confuse.

@rainingmaster
Copy link
Member

@wangrzneu I think maybe you should test the case when has multiple upstream, because upstream_checker_statuses is init in content_by_lua_block, so maybe some worker do not have the specific key in upstream_checker_statuses.

In first request, check with foo.com, hit 0 worker:

Upstream foo.com
    Primary Peers
        127.0.0.1:12354 up
    Backup Peers

Upstream boo.com (NO checkers)
    Primary Peers
        127.0.0.1:12354 up
    Backup Peers

In second request, check with boo.com, hit 1 worker:

Upstream foo.com (NO checkers)
    Primary Peers
        127.0.0.1:12354 up
    Backup Peers

Upstream boo.com
    Primary Peers
        127.0.0.1:12354 up
    Backup Peers

In addition, welcome to the official form https://forum.openresty.us/ to share and discuss

@wangrzneu wangrzneu closed this Mar 14, 2021
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.

None yet

4 participants