Are your servers down...?

Question

If you came here because of the title, chances are your server isn't down. But if by some miraculous reason it is:

A while ago, I posted a program that would ssh and ping all the servers at work. I got some very good feedback and went ahead and rolled with it.

Here's what I came up with:

• It now is much more readable, in my opinion
• Pings the servers after SSHing into them
• Then if it finds one of the servers to be offline, or errors out connecting, writes data to a .txt file containing the server name, and the user who deployed the program.

I of course will be leaving some information off due to security.

Source code:

#!/usr/local/bin/ruby

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

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

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

def server_status(server)
  result = scanning_all_servers(server)

  message_server_online = /1 received, 0% packet loss/i
  message_server_offline = Regexp.union('0 recieved, +1 errors', 'Connection closed')

  result =~ message_server_online  ? :online  :
  result =~ message_server_offline ? :offline : :error
end

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

def write_data(server)
  x = "[#{Time.now}]: Server: #{server} down. Checked by #{@username}. ------------"
  if File.exist?("path/to/log")
     File.open("path/to/log", "a+"){ |s| s.write(x) }
  else
    new_file = File.new("path/to/log", "w")
    new_file.puts(x)
    new_file.close
  end
end

@username = Etc.getlogin
@password = nil

server_list = %w(all of our servers)

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

Example of usage:

server online: server
server online: server
SERVER OFFLINE: server
server online: server
server online: server
server online: server
server online: server
server online: server
Error loading server: server
server online: server
server online: server
server online: server
server online: server
server online: server
server online: server
server online: server
server online: server
server online: server
server online: server
server online: server

Example of log:

[2015-12-14 06:11:41 -0600]: Server: server down. Checked by user. ------------

[2015-12-14 06:11:45 -0600]: Server: server down. Checked by user. ------------

---

What I would like to know:

• Is there any better ways to write data to files?
• Is there also more readable syntax to use here?

Answer 1

I'll proceed top to bottom.

mail_error

Which would you rather read: x.body = e.message or mail.body = exception.message? Don't try to save a few characters on naming — your fellow programmers will like you better. In particular, x has a connotation of being a floating point number, which this isn't.

scanning_all_servers and server_status

Net::SSH#start does not return a "check". Rather, it returns a connection. conn = Net::SSH.start(…) would be more appropriate. Even better, you should call start with a block so that the connection gets closed without an explicit call to close.

As noted in a review of your previous question, scanning_all_servers should be named scan_server, since it acts on just one server. The :error symbol isn't doing anything. Either move it to the end as the implicit return value, or get rid of it.

However, the way that server_status and scanning_all_servers cooperate is weird. scanning_all_servers plays a part in interpreting the result, but only does a partial job. Yet, it tries to log that result anyway. It is also odd that the codepath for writing to the screen is so different from the codepath for writing to the log file. What you want is one function whose job is to scan the server and produce a status code. Everything else belongs in some other function.

status_message and write_data

status_message would look slightly tidier using a case.

write_data has a File.exist? test for no reason. Mode 'a+' will create the file if it doesn't already exist. But why does the append case call write instead of puts, so that there is no newline written? Also, minor note: s looks like a string, so prefer f as the variable name for a file.

You seem to be more careful about logging failures than successes, but the successes are just as important, and should also be written to the log file. Also, all results, whether printed to screen or to the log file, should have timestamps.

Suggested solution

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

def mail_error(exception)
  mail = AD::Mail.new
  mail.from     = '[email protected]'
  mail.to       = '[email protected]'
  mail.subject  = "Server down"
  mail.body     = exception.message
  mail.send
end

def scan_server(server)
  begin
    Net::SSH.start(server, @username, :password => @password) do |ssh|
      case ssh.exec!("ping -c 1 #{server}")
      when /1 received, 0% packet loss/i
        :online
      else
        :offline
      end
    end
  rescue => e
    :error
  end
end

def status_message(server, status)
  sprintf("[%s]: %s\n", Time.now,
    case status
    when :online
      "server online: #{server}"
    when :offline
      "SERVER OFFLINE: #{server}"
    else
      "Error connecting to server: #{server}"
    end
  )
end

def log(message)
  File.open('path/to/log', 'a+') { |f| f.puts(message) }
end

@username = Etc.getlogin
@password = …

server_list = %w(all of our servers)

server_list.each do |server|
  status = scan_server(server)
  message = status_message(server, status)
  log message
  puts message
end