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

Fix for long wait in dig, when DNS servers are not set. #1319

Merged
merged 3 commits into from Apr 20, 2017

Conversation

gozora
Copy link
Member

@gozora gozora commented Apr 19, 2017

When no DNS server is defined in /etc/resolv.conf, every call to dig in 990_sysreqs.sh takes ~18 s to timeout.
In summary, this caused on my SLES12 SP1 with two configured IP addresses to idle for 3 minutes:

...
2017-04-19 17:24:42 Including rescue/GNU/Linux/990_sysreqs.sh
2017-04-19 17:27:42 Finished running 'rescue' stage in 181 seconds
...

This fix skips call to dig if no DNS server is defined in /etc/resolv.conf

V.

@gozora gozora requested review from gdha and jsmeix April 19, 2017 15:44
@gozora gozora self-assigned this Apr 19, 2017
@gozora gozora added the enhancement Adaptions and new features label Apr 19, 2017
@schlomo
Copy link
Member

schlomo commented Apr 19, 2017

Also, maybe better use has_binary instead of hash. And echo "$PRODUCT $VERSION / $RELEASE_DATE" instead of calling rear -V (for which one could also use $SCRIPT_FILE -V)

As DNS is an external view on the system which is also an external dependency we might want to be careful about depending on it.

BTW, why not simply reduce the timeout and retries: dig +time=1 +tries=1? Either we get a quick answer or we skip.

@gozora
Copy link
Member Author

gozora commented Apr 19, 2017

Hi @schlomo

BTW, why not simply reduce the timeout and retries: dig +time=1 +tries=1? Either we get a quick answer or we skip.

Good catch!

With parameters proposed by you, timeout was reduced as well (surprise, surprise :-) ):

...
2017-04-19 18:15:06 Including rescue/GNU/Linux/990_sysreqs.sh
2017-04-19 18:15:17 Finished running 'rescue' stage in 13 seconds
...

Now the question is, which approach is better to implement...
I personally don't care, as both tackle wait time problems pretty good.

@gdha, @jsmeix what is your opinion on this?

V.

@gozora
Copy link
Member Author

gozora commented Apr 19, 2017

I just updated check for nameserver in /etc/resolv.conf to be a bit simpler.
This of course does not drop idea of @schlomo from #1319 (comment).

@jsmeix
Copy link
Member

jsmeix commented Apr 20, 2017

@gozora
I wonder what the rason behind is why you use

[ $(grep -c '^[[:space:]]*nameserver' /etc/resolv.conf) -gt 0 ]

and not what seems simpler

grep -q '^[[:space:]]*nameserver' /etc/resolv.conf

?

Regarding reducing timeout and retries directly in the 'dig' call:

I would perfer that because the 'grep nameserver'
test looks somewhat indirect and I always like
to avoid issues because of RFC 1925 item 6 (a):
Probably Arch Linux uses /etc/RESOLV.conf
where the current 'grep nameserver' test
would wrongfully fail ;-)

@jsmeix
Copy link
Member

jsmeix commented Apr 20, 2017

For the fun of it just another case where the indirect test fails:
When there is a useless nameserver entry in /etc/resolv.conf
e.g. a nameserver that does not exist or is unaccessible.

@gozora
Copy link
Member Author

gozora commented Apr 20, 2017

@jsmeix

Probably Arch Linux uses /etc/RESOLV.conf
where the current 'grep nameserver' test
would wrongfully fail ;-)

LOL!

Ok, I'll implement idea of @schlomo!

@gozora gozora changed the title Fix for long wait in dig, when no DNS servers are defined in /etc/resolv.conf Fix for long wait in dig, when DNS servers are not set. Apr 20, 2017
Copy link
Member

@jsmeix jsmeix left a comment

Choose a reason for hiding this comment

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

Even on SLES10 "man dig" shows
the "+time=1 +tries=1" parameters
so that it should "just work" everywhere.

@jsmeix jsmeix merged commit 3868eb0 into rear:master Apr 20, 2017
@jsmeix jsmeix added this to the ReaR v2.1 milestone Apr 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adaptions and new features fixed / solved / done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants