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?

• 13aal: Round two, +100 rep to best answer
• 200_success: It would help to specify what else you seek to learn from a review by posting a second bounty.
• 13aal: @200_Success Well I feel as though the first answer was, and don't get me wrong it was a great answer, very cliche.. It's just boring I guess, I want my code to stand out. Show me gems I can use, show me better syntax, show me different ways I can do things.

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

• 13aal: Pretty sure this is the best answer I'm gonna get this time around, thanks @200_Success
• 13aal: why would I use sprintf instead of just puts?
• 200_success: Better to compose the message once, then use it for both the log file and the screen.
• 200_success: You could also do string interpolation instead, but in my opinion the expression is too complex for interpolation.
• 200_success: In truth, only the up→down and the down→up transitions are noteworthy. However, it's easier just to log everything and analyze it later when you need to investigate a server problem. Also, logging explicit "up" events is helpful when trying to track down intermittent or micro-outages.