Knife SSH should return exit code based on whether or not ssh command is successful or not

Implementation available Here https://github.com/versapay/chef/tree/CHEF-2627

We would like to avoid the changes to 'chef/lib/chef/application/knife.rb' because that requires plugin developers to return with a reasonable value. They totally should, but we've never asked them to in the past.

Instead, lets exit in 'chef/lib/chef/knife/ssh.rb' around line 350

Should we exit 1 if anything fails, or if everything fails? Tough question. Either is probably okay, since we currently exit 0 for everything there is no expectation.

Can we patch unix to accept an array of return codes?

I could see it being useful to have knife ssh return N > 0 on error where N = # of hosts that failed.

Hello, are Chef developers going to fix this ticket? I really want to know whether the command Chef::Knife::Bootstrap.run (which calls Chef::Knife::Ssh.run) is successful or not, so I can perform next step based on the result.

Seems this Ironfan code snippet gives a possible fix: https://github.com/infochimps-labs/ironfan/blob/master/lib/chef/knife/cluster_ssh.rb#L100

A good fix that would keep backwards compatibility would be adding Chef::Knife#set_exit_status! method so that plugins can notify knife that they want exit non-zero.

Maybe even, as per Steven Danna's suggestion, #bump_exit_status! method that would then return how many times it's been called.

The method could accept extra diagnostic message to be printed at end of the session. Use case for that: I'm using knife ssh to execute maintenance commands in parallel. Diagnostic output is hard to miss when there are many machines. This way, knife would be able to print list of machines that returned non-zero status at end of output.

knife ssh should return nonzero if knife ssh failed.

Not if something else failed.

I'm all for exposing hooks. E.g., plugins can register for notifications when knife-ssh starts a command & when the command completes, and the notification can include command line, stdout/stderr, and exit status.

Hello, I've send out a simple pull request ( https://github.com/opscode/chef/pull/285 ) to resolve this ticket. Please kindly review and verify!

The GitHub repository and branch should be pulled from is : https://github.com/jessehu/chef/tree/master/chef (master branch)

Reviewed and marked for merge.

Merged to master, thanks Hui.

Please use topic branches in the future. Your master branch had two copies of the same commit, the second likely from a merge commit at the top of your master branch. Merge commits are usually a source of problems; while they can make the code look right they often mess up the commit history and make rebasing difficult for others.

http://wiki.opscode.com/display/chef/Working+with+Git

So topic branches and rebasing in the future . Thanks again.

Thanks Bryan. I will make it clean in new pull request

It seems like this pull request works, but doesn't actually exit with the exit status - it just returns. Am I missing something, or should line 423 read exit exit_status ?

Hi Ben, assuming your knife command will be like this :

ssh = Chef::Knife::Ssh.new (or subclass of Chef::Knife::Ssh)
... ...
status = ssh.run
exit status

I think if 'exit exit_status' in Chef::Knife::Ssh.run() will not allow subclass of Chef::Knife::Ssh to handle the exit status before exiting.

I just ran into this issue. The code that was merged in the pull request does return an exit status, but the code in chef/lib/chef/application/knife.rb that calls Chef::Knife.run ignores it.

Running the command "knife ssh" on the command line, for instance, uses Chef::Application::Knife's run method.

Since Chef::Application::Knife#run does not capture the exit code and instead just does "exit 0", you can't tell whether or not the remote command was successful.

After reading through the comments in this thread, I'm not sure how to solve this problem. I take it that this approach is not acceptable?

def run
    Mixlib::Log::Formatter.show_time = false
    validate_and_parse_options
    quiet_traps
    exit_status = Chef::Knife.run(ARGV, options)
    exit exit_status.to_i
  end

Well, fwiw, I made this patch to Chef::Application::Knife. Here is my rebased topic branch with the code: https://github.com/jacqui/chef/tree/CHEF-2627

Thanks Jacqui. I think this patch makes sense when the knife command returns a integer (e.g. knife ssh command), but will fail when the knife command returns an object. So need to improve the patch a little more.

This does not work. Chef::Application::Knife#run always exits with 0. We have automated scripts which run knife ssh and expect a non-zero exit code if there is an error.

I think the expected behavior would be a non-zero exit code for any error during the ssh run, including authentication errors, etc. It doesn't matter what that exit code was, but if there was an error, we should not return 0.

Hi Matthew, Jacqui's patch https://github.com/jacqui/chef/tree/CHEF-2627 is to solve your problem and it needs to be improved as my comments above.

I've fixed this in https://github.com/opscode/chef/pull/507.

Hi Matthew, I think this is a better fix https://github.com/jacqui/chef/commit/57dfb45692592741f7ddffae6965840d8ef83a37, does it work for you? Your pull request https://github.com/opscode/chef/pull/507/files has a side effect that it prevents from writing code like this :

ssh = Chef::Knife::Ssh.new (or subclass of Chef::Knife::Ssh)
... ...
status = ssh.run
... ...
exit status

As pointed out earlier, not all Knife commands return an exit code. As this is not something that will likely be changed any time soon, the only thing we can do is exit from Knife::Ssh#run.

I understand this prevents one from calling #run on a Knife::Ssh instance without causing the process to exit on error, but if you look at http://git.io/kvFDDg we exit with 10 in the event that no nodes were found.

Subclasses should also not change the behavior of their ancestors. Scattered throughout the Knife commands we exit as appropriate. Bryan McLellan originally recommended we exit from Knife::Ssh#run as well.

Exiting from Knife::Ssh#run also has the side effect of causing Knife::Bootstrap#run to return an exit code on failure, which I would argue is the desired behavior.

The alternative would be to keep the exit status in an instance variable and exit with that. However, this complicates the case of Knife::Bootstrap, and other plugins which may use Knife::Ssh directly.

It'd be great if someone from Opscode could weigh in on the issue.

Hui's argument is valid, however we usually treat knife as an application rather than a library. If there's good use for parts of 'knife ssh' elsewhere, we should factor it into a library in Chef and populate it with a reasonable set of exceptions that can be captured. For now, we'll plan on merging PR#507. If anyone wants to go down this other route, please advise us.