Server status checker

I've created a program for work that checks the status of all our servers quickly. What we use to have to do is ssh into each server manually.

I'm looking for some critique on what I've created here. Any input on it would be greatly appreciated, such as better syntax I can use, better variables, easier ways to perform the rescue, etc. I will be leaving out a couple of things for security purposes.

#!/local/usr/bin/env ruby

require 'rubygems'
require 'net/ssh/gateway'
require 'work/mail/gem'

begin
  def scanning_all_ssh_servers(server_check, cmd)
    check = Net::SSH.start(server_check, @username, :password => @password)
    ping = check.exec!(cmd)
    case 
    when ping.include?("1 received, 0% packet loss")
      puts "server online: #{server_check}"
    when ping.include?("0 received, +1 errors")
       puts "SERVER OFFLINE: #{server_check}"
    when check.include?("Connection closed")
      puts "SERVER OFFLINE: #{server_check}"
    else
      raise "Error loading server: #{server_check}. Exiting.."
    end
  end

  @username = "username"
  @password = "password"

  ssh_server_list = %w(these are all the servers that we can ssh too)
  ssh_server_list.each do |server_check|
    cmd = "ping -c 1 #{server_check}"
    scanning_all_ssh_servers(server_check, cmd)
  end
rescue => e
  x = AD::Mail.new
  x.from        = "email@email.email"
  x.to          = "email@email.email"
  x.subject     = "Server Loading Error"
  x.body        = e.message
  x.send
end

Read my rewrite from the bottom up. To appreciate the benefit, you notice how it clarifies the high level logic: your whole script is really only 4 lines long.

Some notes:

• Isolate your begin... rescue code. This way all servers will be processed even if there's an error

• Break up each of your tasks into smaller, more focused methods (see below)

• Make sure your names are accurate: eg, scanning_all_ssh_servers really means "ping a particular server"

• Watch out for hidden entanglements in your code: your scan function takes a cmd argument, but this is a false abstraction, since the cody of the function assumes the cmd is a ping. In this case, better to just leave the argument out.

• breaking out the mailer into its own method will make the high level logic clearer

• You can shorten up the ping function, while making it clearer imo, by losing the case statment (see below). But if you don't like that, you should probably still use the regex version of case to avoid the repeated ping.include? calls.

• Another small point on naming: Avoid unnecessary adjectives. In this context, all your "servers" are "ssh servers", so better to use names like server_list rather than ssh_server_list, which adds visual and auditory cruft without adding precision.

Rewrite:

def mail_error(e)
  x = AD::Mail.new
  x.from     = "email@email.email"
  x.to       = "email@email.email"
  x.subject  = "Server Loading Error"
  x.body     = e.message
  x.send
end

def ping_server(server)
  begin
    check = Net::SSH.start(server, @username, :password => @password)
    cmd = "ping -c 1 #{server}"
    check.exec!(cmd)
  rescue => e
    mail_error(e)
    :error
  end
end

def server_status(server)
  ping_result =  ping_server(server)

  online_text = /1 received, 0% packet loss/
  offline_text = Regexp.union('0 received, +1 errors', 'Connection closed')

  ping_result =~ online_text  ? :online  :
  ping_result =~ offline_text ? :offline : :error
end

def status_message(server, status)
  status == :online  ? "server online: #{server}"  :
  status == :offline ? "SERVER OFFLINE: #{server}" :
                       "Error loading server: #{server}. Exiting.."
end

@username = "username"
@password = "password"
server_list = %w(these are all the servers that we can ssh too)

server_list.each do |server|
  status = server_status(server)
  puts status_message(server,status)
end

UPDATE: Additional Explanation

Regexp.union allows you to easily create an "or"ed regexp from a number of strings. So that when you match strings against the regexp you created, you'll get a match if any of the strings match. It's equivalent to the regexp | "or" operator, but can often be a more readable way to create the regexp.

Re: your question about the rescue block, it depends, but I would think you only want the "server can't be reached" email when the "reach server" code fails. That is, even though this code is simple, it's still possible you get some other error executing it, which should be reported as a script error (using a library like airbrake) rather than spawning a "server can't be reached" email. Again, just separating you concerns.

Re: the distinction between ssh and rdp servers, that makes sense.

StackExchange Code Review Q#113521, answer score: 2