-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 broken salt cmd return codes - issue #7013 #11337
Fix for broken salt cmd return codes - issue #7013 #11337
Conversation
- new test related states that can be used to provide success / failure on request. - fixed the broken logic chain that handles return codes both when the minion decides what retcode to propagate to the master and when the master decides what return code to return to the invoked salt cmd - added new log.debug and log.trace statements to assist with further work on ensuring return codes are properly handled by all components of salt.
Test FAILed. Build Log
|
Test FAILed. Build Log
|
Test FAILed. |
Test FAILed. |
Test FAILed. |
Test FAILed. |
- Added newlines to fix E8302(expected-2-blank-lines,-found-0) - Disabled pylint checks for E8712(comparison-to-True-should-be-'if-cond-is-True:'-or-'if-cond:') and documented my reason
Test FAILed. |
Test FAILed. |
The console log doesnt provide enough information on why the profile error is being propagated. Investigating back through the salt cloud code in order to identify a possible cause of the error.
http://jenkins.saltstack.com/job/salt-pr-rs-ubuntu12.04/2091/consoleFull |
err = 'All Minions did not return a retcode of 0. One or more minions had a problem' | ||
# NOTE: This could probably be made more informative. | ||
# I chose 11 since its not in use. | ||
raise SaltSystemExit(code=11, msg=err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might not want to raise an error like this, but just return with the error return code, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you mean just passing on the return code that is generated from the minion. The problem is what to do when you get multiple different return codes. For instance one minion has no errors, another has a single state return retcode 1, another minion returns retcode 2. How to represent this? I've been contemplating options on how to provide the information and feel that given the possible permutations we really have little option as far as the process return/exit code but to choose a distinct error code for this 'minion failures' use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, no, I like your approach with a static exit code! I just think we should use sys.exit instead of raise SaltSystemExit
Sorry about the test system failures, we were having issues yesterday with the rackspace API (or rather, they stopped working for some 20+ hours) |
Test FAILed. |
Still getting issues with the bootstraping code failing.
But it seems the cause is #11338 and #11357 |
…ly runs on my PR.
Test FAILed. |
- fixed pep8 issue - added more logic to check_state_result logic to handle list results properly and return false. - ordered the check_state_result unit tests more logically. All the data type tests are now before the content & logic handling checks - discovered duplicate test in check_state_result unit tests and removed it, both tests were asserting using the same data {'host': []}
Test FAILed. |
Fix for broken salt cmd return codes - issue #7013
'new': 'Were pretending really hard that we changed something' | ||
} | ||
} | ||
elif changes == True: # pylint: disable=E8712 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why would
elif changes is True:
be less readable?
I think this breaked the saltmod module |
cc @thatch45 . |
we used to check the highstate return in this way:
and it seems that now the function waits, something like that:
|
Im sending a PR but it would be good to have a confirmation on this |
Was this actually fixed? Currently "salt-call state.highstate" always returns 0 for me! |
@nw0428 which version of salt? |
@nw0428 I think this only goes live in the next feature release (codenamed Helium) -- so you'll only see the new behavior if you're running off of the |
both when the minion decides what retcode to propagate to the master
and when the master decides what return code to return to the invoked salt cmd
codes are properly handled by all components of salt.