19
\$\begingroup\$

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@email.email"
  x.to          = "email@email.email"
  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?
\$\endgroup\$
  • \$\begingroup\$ Round two, +100 rep to best answer \$\endgroup\$ – 13aal Dec 18 '15 at 22:39
  • \$\begingroup\$ It would help to specify what else you seek to learn from a review by posting a second bounty. \$\endgroup\$ – 200_success Dec 19 '15 at 4:45
  • \$\begingroup\$ @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. \$\endgroup\$ – 13aal Dec 19 '15 at 13:37
11
+150
\$\begingroup\$

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@example.com'
  mail.to       = 'email@example.com'
  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
\$\endgroup\$
  • \$\begingroup\$ Pretty sure this is the best answer I'm gonna get this time around, thanks @200_Success \$\endgroup\$ – 13aal Dec 18 '15 at 22:13
  • \$\begingroup\$ why would I use sprintf instead of just puts? \$\endgroup\$ – 13aal Dec 21 '15 at 22:08
  • \$\begingroup\$ Better to compose the message once, then use it for both the log file and the screen. \$\endgroup\$ – 200_success Dec 21 '15 at 22:56
  • 1
    \$\begingroup\$ You could also do string interpolation instead, but in my opinion the expression is too complex for interpolation. \$\endgroup\$ – 200_success Dec 21 '15 at 22:57
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – 200_success Dec 22 '15 at 4:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.