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

[2018.3] Adding retry_dns_count to minion #49764

Merged

Conversation

Projects
None yet
6 participants
@garethgreenaway
Copy link
Member

commented Sep 25, 2018

What does this PR do?

Updating the resolve_dns function in minion.py to include a new minion configuration option which will control how many attempts will be made when the master hostname is unable to be resolved before giving up.

What issues does this PR fix or reference?

#49520

Previous Behavior

When a including a master hostname that doesn't resolve, particular in a multi-master setup, the resolve_dns function in the minion will continue indefinitely and never move onto other minions.

New Behavior

This change updates that function to look for a new minion configuration option, retry_dns_count, which will allow the minion to either move on or exit following a specified number of attempts.

Tests written?

Yes

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@garethgreenaway garethgreenaway requested a review from saltstack/team-core as a code owner Sep 25, 2018

@salt-jenkins salt-jenkins requested a review from saltstack/team-suse Sep 25, 2018

@gtmanfred
Copy link
Contributor

left a comment

Just one change

Show resolved Hide resolved salt/config/__init__.py
@isbm
Copy link
Contributor

left a comment

I would rather set this default to a long enough, reasonable amount of times and bail out. Say, put 50 if you want. If then anyone wants to keep it until forever, should do it explicitly.

Also maybe retry_dns_times would be a better name.

err = ('Minion unable to successfully connect to '
'a Salt Master. Exiting.')
log.error(err)
raise SaltSystemExit(code=42, msg=err)

This comment has been minimized.

Copy link
@isbm

isbm Sep 25, 2018

Contributor

We have constants for this. Please do not use hard-coded error codes.

This comment has been minimized.

Copy link
@garethgreenaway

garethgreenaway Sep 25, 2018

Author Member

This particular return code is used in several other places in the code base, as is, not referenced by a constant, including in minion.py.

if retry_dns_count is not None:
if retry_dns_count == 0:
raise SaltMasterUnresolvableError
retry_dns_count -= 1

This comment has been minimized.

Copy link
@isbm

isbm Sep 25, 2018

Contributor

Well, that's the thing. I'd set it default to something big enough. Any None/0 or negative is "until forever".

This comment has been minimized.

Copy link
@rallytime

rallytime Sep 27, 2018

Contributor

This looks correct to me. It should default "until forever" because that is the current behavior which we don't want to break for people.

Show resolved Hide resolved salt/minion.py Outdated

self._connect_minion(minion)

if not self.minions:

This comment has been minimized.

Copy link
@DmitryKuzmenko

DmitryKuzmenko Sep 25, 2018

Contributor

_connect_minion is async operation. If it will not done fast enough the list could be empty here. It's better to run this check with io_loop.call_later.

@rallytime
Copy link
Contributor

left a comment

I have some small doc requests.

I addition to the inline comments, can you also add this setting to the sample minion conf?

Otherwise, 👍 from me.

.. conf_minion:: retry_dns

``retry_dns_count``
-------------

This comment has been minimized.

Copy link
@rallytime

rallytime Sep 27, 2018

Contributor

I know this is picky, but the number of dashes needs to match the line above it or the doc build will have a warning. (We need to figure out a way to get that to bubble up in the PR tests still.)

Can you add a versionadded tag in here as well?

@@ -307,6 +307,21 @@ Set to zero if the minion should shutdown and not retry.
retry_dns: 30
.. conf_minion:: retry_dns

This comment has been minimized.

Copy link
@rallytime

rallytime Sep 27, 2018

Contributor

This needs to match the value below: retry_dns_count.

if retry_dns_count is not None:
if retry_dns_count == 0:
raise SaltMasterUnresolvableError
retry_dns_count -= 1

This comment has been minimized.

Copy link
@rallytime

rallytime Sep 27, 2018

Contributor

This looks correct to me. It should default "until forever" because that is the current behavior which we don't want to break for people.

garethgreenaway added some commits Sep 25, 2018

Updating the resolve_dns function in minion.py to include a new minio…
…n configuration option which will control how many attempts will be made when the master hostname is unable to be resolved before giving up.

@garethgreenaway garethgreenaway force-pushed the garethgreenaway:49520_multimaster_dns_issue_fix branch from ac118dd to 741928b Sep 27, 2018

@rallytime rallytime merged commit ec97806 into saltstack:2018.3 Oct 1, 2018

8 of 10 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
jenkins/pr/py2-windows-2016 The py2-windows-2016 job has failed
Details
WIP ready for review
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.