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

salt-api no longer forces the default timeout #38527

Merged
merged 1 commit into from Jan 4, 2017

Conversation

rbjorklin
Copy link
Contributor

What does this PR do?

What issues does this PR fix or reference?

Fixes #38524

Previous Behavior

default rest_timeout was forced

New Behavior

rest_timeout is usable again

Tests written?

No

@rallytime
Copy link
Contributor

Go Go Jenkins!

@cachedout cachedout merged commit 42fef27 into saltstack:2016.11 Jan 4, 2017
@rallytime
Copy link
Contributor

Hi @rbjorklin - Can you explain a little more about why rest_timeout is getting reset through in the opts.update(api_opts) here, before this change? I think this fix is going to break some other things, namely the key updates to pidfile (and also log_file as was recently fixed in #38560).

I ran across this change during a merge forward as there was a merge-conflict here between this change and the one made in #38560. The key overriding for pidfile and log_file are intentional, and without them I think #38479 will be broken still, which @Ch3LL fixed with #38560.

After looking at these two conflicting changes, it seems to me that the rest_timeout should be coming through in that dictionary update call. The rest_timeout key from api_opts will just be added to the opts dictionary. Unless I am missing something?

For the time being, I am using the changes from 2016.3 to merge forward, and we can certainly discuss this further here and make whatever changes are needed. I want to make sure all of the moving pieces here are covered.

Please see PR #38570 for the merge forward change.

@Ch3LL
Copy link
Contributor

Ch3LL commented Jan 4, 2017

I think what he is attempting to say is the api default rest_timeout value of 300 is overwriting the users rest_timeout in their config is that correct? I'm assuming this because of the issue here: #38524 but the PR title here confuses me a bit as the intention.

If the intention is to not overwrite the users rest_timeout config what do you think about adding this:

'rest_timeout': opts.get('rest_timeout', DEFAULT_API_OPTS['rest_timeout']), So we ensure we are keeping the default api timeout of 300 and grabbing the users settings? Note: I have not tested this.

@rbjorklin
Copy link
Contributor Author

rbjorklin commented Jan 5, 2017

@rallytime The way I'm reading the existing code we get the master.conf opts and then overwrite them with the default here. My proposed change get's the defaults first and then overwrite them with the ones from master.conf. This should not break the pidfile update as it is still updated, just a little differently.

@Ch3LL That's correct, sorry if I was being a bit unclear. That's basically what my change does but without the explicit step. By updating the entire config you don't have to maintain the explicit steps every time a new salt-api option is added. Did that make things any clearer?

@rallytime
Copy link
Contributor

rallytime commented Jan 5, 2017

@Ch3LL and @rbjorklin Thanks for explaining what needed to be done for the rest_timeout option. That makes sense and that piece definitely needs to be fixed, both on the 2016.11 and the 2016.3 branches.

My concern here with the way this change stands is that I am unsure of where the log_file and pidfile options get pulled in from down the line. The existing code is setting the log_file value to the value given in 'log_file': opts.get('api_logfile', DEFAULT_API_OPTS['api_logfile']). The value of log_file, with the code as it stands, changes from the value in the DEFAULT_MASTER_OPTS (or what was set by the user) to the value set in api_logfile in DEFAULT_API_OPTS (or by the user).

This change makes it so that api_logfile and log_file have different values, if I am reading that correctly. Which means this is a change in behavior for the log_file and pidfile values. We need to cover both cases in a fix.

It is OK with me if we keep the updates as they are here, as I agree with @rbjorklin that this is a better way to move forward so we don't have to update each default API value. However, we need to make sure we account for the change to the log_file and pidfile values.

@rallytime
Copy link
Contributor

@rbjorklin and @Ch3LL - Can you guys take a look at the change I made in #38585 on the 2016.3 branch? I think that explains in code more what I mean. Hopefully that makes more sense!

@Ch3LL
Copy link
Contributor

Ch3LL commented Jan 5, 2017

That looks great to me @rallytime ! That should grab all those configurations as you stated from what I can tell. 👍

@rbjorklin
Copy link
Contributor Author

@rallytime #38585 looks good! I initially missed that we stored the value from api_pidfile into pidfile and likewise for logfile.

@rallytime
Copy link
Contributor

Ok awesome! Thanks everyone. I'll write some tests for the change in my PR.

rallytime pushed a commit to rallytime/salt that referenced this pull request Jan 9, 2017
This will help prevent regressions in this area as discussed in
PR saltstack#38527.
cachedout pushed a commit that referenced this pull request Jan 17, 2017
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.

None yet

4 participants