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

Back-port #35463 to 2016.3 #35489

Merged
merged 2 commits into from
Aug 18, 2016
Merged

Back-port #35463 to 2016.3 #35489

merged 2 commits into from
Aug 18, 2016

Conversation

rallytime
Copy link
Contributor

@rallytime rallytime commented Aug 16, 2016

Back-port #35463 to 2016.3

@cachedout Please review this back-port from @skizunov's PR to develop. There was a merge-conflict in the back-port, but I think this is the correct resolution because the auth_timeout option should come from through in the s_opts = copy.deepcopy(self.opts), right?

There was code that was overriding the value of `auth_timeout` regardless
of what was set in the configuration. It was overriding it to be 5
seconds. Although 5 seconds is sufficient for most cases, in testing, I
have found it insufficient for the following use case (running both
Windows master and minions):

1) Start salt-master
2) Start salt-minion
3) Stop salt-master
4) Start salt-master

The minion reconnect will timeout after step 4. Due to this, we set the
`auth_timeout` to be 15 seconds in our own configuration file. However,
due to this issue, that configuration value was ignored and thus we
fail on reconnects.

`salt/minion.py`:
- Don't hard-code the `auth_timeout` to 5 seconds. Use the user configured
value (or the default value if the user didn't configure a value).

`salt/config/__init__.py`:
- Made the default be 5 seconds for `auth_timeout` instead of 60. Chose 5
seconds because this is the current hard-coded behavior.

Signed-off-by: Sergey Kizunov <sergey.kizunov@ni.com>
@cachedout
Copy link
Contributor

I'd rather just have @skizunov review it if he's available. :]

@skizunov
Copy link
Contributor

In MultiMinion._connect_minion there is a self.MINION_CONNECT_TIMEOUT that needs to be changed to s_opts['auth_timeout'].

Curious as to why the linter didn't tell us that self.MINION_CONNECT_TIMEOUT is undefined but used.

@skizunov
Copy link
Contributor

Actually, it should be changed to opts['auth_timeout']

@rallytime
Copy link
Contributor Author

Ah, good catch @skizunov. I guess because of the self the linter didn't catch that? I agree that is odd. How does it look now?

@skizunov
Copy link
Contributor

@rallytime : It looks fine now

@cachedout cachedout merged commit f2eb3dc into saltstack:2016.3 Aug 18, 2016
@rallytime rallytime deleted the bp-35463 branch August 18, 2016 15:05
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