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

Allow nginx to start even if resolution failed #12

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rs
Copy link
Contributor

@rs rs commented Aug 17, 2016

If nginx can't access the resolver during startup for any reason, do not prevent nginx from starting and let it recover later.

Also fixes a crash in case the resolution would suddently return no entities.

(This PR is based on #11)

rs added 2 commits August 16, 2016 22:59
Just cosmetic but helps to understand the code.
If nginx can't access the resolver during startup for any reason, do not
prevent nginx from starting and let it recover later.

Also fixes a crash in case the resolution would suddently return no
entities.
@rs
Copy link
Contributor Author

rs commented Sep 25, 2016

Are you still maintaining this module?

@viciious
Copy link

viciious commented Oct 12, 2016

@rs I think I have what might be a better solution to the problem.

    if (ngx_parse_url(cf->pool, &u) != NGX_OK) {
        if (u.err) {
            ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
                "%s in upstream \"%V\"", u.err, &u.url);
        }

        return NGX_CONF_ERROR;
    }

I think this approach is a bit better/cleaner than both the original and your versions:

    // first pass: parse and validate config option and drop out if failed
    // second pass: actually perform name resolution, continue in case of failure
    for (i=0; i<2; i++) {
        u.no_resolve = (i == 0);
        if (ngx_parse_url(cf->pool, &u) != NGX_OK) {
            if (u.err) {
                ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
                    "%s in upstream \"%V\"", u.err, &u.url);
            }
            if (u.no_resolve)
                return NGX_CONF_ERROR;
        }
    }

@wdaike
Copy link
Owner

wdaike commented Oct 12, 2016

if do not prevent nginx from starting , all request to upstream will fail.i don't think its a good idea.

@viciious
Copy link

viciious commented Oct 12, 2016

@wdaike imo there's no practical difference whether requests fail at startup or at some later stage in case the DNS server becomes temporarily unavailable. On the other hand, a service that fails to startup due to a temporary error can cause a lot of maintenance trouble.

@rs Please see https://github.com/viciious/ngx_upstream_jdomain/tree/lazy-startup

@viciious
Copy link

@wdaike I can see how this might be a bad idea in case of a restart though. Perhaps we should add a config option to allow tolerating name resolution failures at startup then.

@viciious
Copy link

Added a "no_fail" option which defaults to 0. Turning the option on allows nginx to startup even in case of domain name resolution failure.

https://github.com/viciious/ngx_upstream_jdomain/commits/lazy-startup

@wdaike If that's ok with you, i'll make a new pull request which will supersede this one.

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.

3 participants