Added support for refresh_in time and optional debug log

[rohitjoshi]

I have added support for

1. resolve now takes an optional value for refresh_in. eg

  server abc.con resolve;
   or 
  server abc.com resolve=100000;

1. debug_log support: during development, this module generates lot of debug logs so I have made it optional

   server abc.com resolve debug_log;

2. Bug fix for integer over flow

[GUI]

Thanks for the contributions! Making resolve refresh interval configurable sounds like a great idea. A couple of notes on that:

• My one concern with this approach of setting this on the resolve option is that @wandenberg recently made changes so our syntax matches nginx plus's syntax. By making resolve take an optional argument, we begin to deviate from that syntax. I don't necessarily think this is bad, but I'm wondering if perhaps making it a completely separate option might be better so it's easier to distinguish it from the nginx plus syntax (for example, refresh_interval=). Do you have any thoughts on this? @wandenberg?
• It looks like these changes cause our test suite to fail. I believe this might be due to the change in the default resolve refresh time from 1 second to 10 seconds. I think several of our tests were built around this assumption of it being hard-coded to 1 second. If we switch the default value back to 1 second, then I believe that would fix the tests. I'd also probably prefer to keep the default refresh interval at 1 second for backwards compatibility (but again, making this configurable seems great).
• Would it be possible for you to add tests to the test suite for this new functionality and update the documentation in the README? If you're not familiar with Nginx::Test, I can also try to help lend a hand with this at some point.

Regarding the debug_log option, is there a reason for wanting this option instead of setting the error_log level in nginx to a different level? For example, setting it to error_log logs/error.log info; in your nginx config would also eliminate all the debug logs. Would that work for your purposes? I'm just trying to understand this option and trying to avoid re-implementing log levels in our own way.

But overall this looks great, so thanks again for implementing these things!

[rohitjoshi]

@GUI Thanks for your feedback. I will make following changes and resubmit.

1. move refresh_interval= out size of resolve
2. default refresh_in time 1000
3. update documentation

regarding debug_log, we use 100+ upstream servers and during development, it dumps too much logs that our tail on error log becomes useless. This was feedback from my development team so I had to implement it. Either I can remove debug_log from this PR and might keep in my local GitHub repo or change it to make debug_log=true by default and can be explicitly overridden by passing debug_log=false.

By the way, I had to disable this module due to some corruption/conflict with keepalive module. See https://github.com/GUI/nginx-upstream-dynamic-servers/issues/18

[wandenberg]

Hi @rohitjoshi and @GUI concerning the refresh interval would be better to use the resolver configuration for that. This way we keep the syntax closer to the original. Unless you really need to use different intervals to each server the resolver configuration will do exactly what you need.

About the log level I agree with @GUI. We can review the number of messages and their sizes when debug is enabled, but if you use a lot of third party modules you will have the same issue with them. I propose to review the messages and if possible remove some or promote some to INFO level making possible filter them just changing the error_log directive.

I commented on #18 asking for your help. That queue should never goes on infinity loop. Just to be on the same step, the loop happens with your modifications also?

[rohitjoshi]

@wandenberg Sure, we remove debug_log and move refresh interval separately.

[wandenberg]

@rohitjoshi I was reviewing your code and remembered a good reason to not allow change the refresh interval. If you set this configuration to a big value and try to do a reload on your nginx the process will stay in the "worker process is shutting down" step until this time expires. Assuming that your idea is avoid the DNS lookup step and all logging messages each 1 second I'm proposing this implementation. Basically, it will keep when the DNS resolution will expires and each second will verify if it's time to renew it or not. If not, a new timer is scheduled and the other steps are skiped. This way the reload still working with a minimum delay, and all unnecessary tasks to check DNS and log messages are avoided. This check is safe to be done since is the same check nginx does on its internals of ngx_resolve functions.

Please, try this code and if it's OK I will submit a pull request instead of this one.

[rohitjoshi]

@wandenberg I like your approach over mine and will give a try. I had to disable this feature due to issue https://github.com/GUI/nginx-upstream-dynamic-servers/issues/18

[felixbuenemann]

Is there a reason not to use the resolver valid option to control TTL, that's the way it works in NGINX Plus.

[rohitjoshi]

'resolver valid' is still honored when TTL is used. This is about how often we should check if TTL is expired.