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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allows overriding 'timeout' and 'gather_job_timeout' to 'manage.up' runner call #40018

Conversation

meaksh
Copy link
Contributor

@meaksh meaksh commented Mar 14, 2017

What does this PR do?

This PR allows overriding timeout and gather_job_timeout parameters to manage.up and manage.status runners. These two parameters are set in our master configuration with a high value, as we want to wait by default for jobs that takes long to execute:

timeout: 120
gather_job_timeout: 120

When dealing with an scenario where some minions are down, if we execute manage.up runner to check for minions that are up and running, it will end up after reaching the two timeouts. This makes manage.up runner unusable as presence mechanism because we cannot get a response in some short and fixed time.

With this PR, we are able to call manage.up runner passing timeout and gather_job_timeout parameters which are used to perform the upcoming test.ping job that the runner is going to trigger.

This way we can get a quick response of manage.up runner in case of unreachable minions. Even if we call the runner via API.

Previous Behavior

$ time salt-run manage.up
- headref-minsles12sp2.tf.local

real	4m1.052s
user	0m3.729s
sys	0m0.264s

New Behavior

$ time salt-run manage.up timeout=2 gather_job_timeout=1
- headref-minsles12sp2.tf.local

real	0m3.964s
user	0m0.693s
sys	0m0.119s

Tests written?

No

What do you think about this?

I also tried with manage.present runner but it doesn't fit well in our scenario. Is there another quick way to check for the actual presence of minions in a context with unreachable minions?

Any feedback or opinions are more than welcome! 馃槃
Thanks

/cc @moio

@ghost
Copy link

ghost commented Mar 14, 2017

@meaksh, thanks for your PR! By analyzing the history of the files in this pull request, we identified @0xDEC0DE, @cachedout and @cvrebert to be potential reviewers.

Copy link
Contributor

@cachedout cachedout left a comment

Choose a reason for hiding this comment

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

This seems totally reasonable. I like it.

@meaksh
Copy link
Contributor Author

meaksh commented Mar 15, 2017

Great @cachedout. Do you think we should also allow those parameters for the general test.ping command?

BTW there're some conflicts merging this PR forward to 2016.11 branch. If needed, I can do another PR with the fixed version for 2016.11, or you can just ping me to verify the resolution.

@cachedout
Copy link
Contributor

@meaksh If you want to do it to test.ping I have no objection so long as it's done as a kwarg.

@cachedout cachedout merged commit 8dcffc7 into saltstack:2016.3 Mar 15, 2017
@rallytime
Copy link
Contributor

@meaksh If you have the time to put a PR together for 2016.11, that would be awesome. The merge conflict isn't too terrible, but a little bit trickier than normal. That way I won't miss something in resolving the conflict.

@meaksh
Copy link
Contributor Author

meaksh commented Mar 16, 2017

@rallytime - Here it is the PR for 2016.11 branch: #40072

@meaksh meaksh deleted the 2016.3-handling-timeouts-for-manage.up-runner branch March 16, 2017 09:15
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

3 participants