4
\$\begingroup\$

I wrote a script which logs memory usage of a process (pid in file), and reacts in case a limit is reached. I run the script nightly. In my case, the process concerned propably has a memory leak and locks up the whole system after a couple of days. Hence, the script should reboot the system if excess memory is used. Logged memory usage values are formated to have 1 decimal place.

The code works, but I'd like to know if and how I could optimize it, and if there are no mistakes.

#!/bin/sh

# process_monitor.sh
#
# monitor specific process
# customized for memory usage monitor (can easily be changed)
# customized to perform system reboot if limit is exceeded (can easily be changed)
# by geohei <xxxx@xxxx.xx>
# created : 14.10.2016
# revised : n/a
#
# use cron to trigger (e.g. nightly at 04:15 '15 4 * * * ~/process_monitor.sh'
# /bin/bc is used (possibly not installed by default)
#
# $pidfile must include path
# $logpath is used to save 'process_monitor.log' file (omit trailing '/')
# $pidlimit can be floating point (e.g. 14.1)

pidfile='/tmp/process.pid'
logpath='~'
pidlimit=20.0

pidvalue=$(ps -p $(eval "cat $echo $pidfile") -o %mem --no-headers)

if [ "$(echo "${pidvalue} > ${pidlimit}" | bc)" -eq 1 ]; then
    # log $pidvalue, $pidlimit and reboot system
    echo "$(date '+%Y-%m-%d %H:%M:%S')   pidvalue: $pidvalue, pidlimit: $(echo $pidlimit | awk '{printf "%.1f\n", int($1)}') - reboot" >> $logpath/process_monitor.log
    shutdown -r -t 10
    exit 1
else
    # log $pidvalue, $pidlimit
    echo "$(date '+%Y-%m-%d %H:%M:%S')   pidvalue: $pidvalue, pidlimit: $pidlimit" >> $logpath/process_monitor.log
fi

exit 0

That's how the log looks like:

...
2016-10-14 04:15:04   pidvalue: 19.2, pidlimit: 20.0
2016-10-15 04:15:03   pidvalue: 20.4, pidlimit: 20.0 - reboot
...

Any remarks, comments or suggestions?

\$\endgroup\$
  • \$\begingroup\$ Rebooting the machine because one process allocated too much memory sounds like a drastic measure. Why wouldn't you just stop and restart the offending process? \$\endgroup\$ – 200_success Oct 15 '16 at 13:51
  • \$\begingroup\$ Yes, you are absolutely right. The script was designed to have the basic structure for all kind of things which might be triggered. Of course I will adapt the script on the real productive server to first restart the monitored process at a first stage. Thanks for the hint. \$\endgroup\$ – geohei Oct 15 '16 at 14:06
3
\$\begingroup\$

Don't write custom scripts for common sysadmin tasks. Using the proper tools will get the job done better, and the solution will be more maintainable — especially by any colleagues you might have.

Using monit, for example, this would just be a matter of writing a simple configuration file.

set logfile /root/process_monitor.log
check process myproc with pidfile /tmp/pidfile.pid
    restart program = "shutdown -r -t 10"  # draconian!
    every "15 4 * * *"
    if memory > 20 MB then restart myproc
\$\endgroup\$
  • \$\begingroup\$ Wow! I had no clue such a tool existed. Going to dig myself through the man pages. Thanks a lot! \$\endgroup\$ – geohei Oct 15 '16 at 18:49
2
\$\begingroup\$

Although @200_success already gave a great answer, there are some glaring issues with the posted code that should be pointed so you can avoid them in the future.


eval is evil. Not really, but it's prone to abuses, and it's very hard to justify its use. This is a very twisted example:

pidvalue=$(ps -p $(eval "cat $echo $pidfile") -o %mem --no-headers)

That should be simply:

pidvalue=$(ps -p $(cat "$pidfile") -o %mem --no-headers)

Using bc for a boolean evaluation is extremely awkward:

if [ "$(echo "${pidvalue} > ${pidlimit}" | bc)" -eq 1 ]; then

Write like this:

if [ "${pidvalue}" -gt "${pidlimit}" ]; then

I used -gt because you are using /bin/sh. If you don't mind changing that to Bash, then it would become even more simple and natural:

if (( pidvalue > pidlimit )); then

These lines are very similar:

echo "$(date '+%Y-%m-%d %H:%M:%S')   pidvalue: $pidvalue, pidlimit: $(echo $pidlimit | awk '{printf "%.1f\n", int($1)}') - reboot" >> $logpath/process_monitor.log
# ...
echo "$(date '+%Y-%m-%d %H:%M:%S')   pidvalue: $pidvalue, pidlimit: $pidlimit" >> $logpath/process_monitor.log

Probably you copy-pasted and customized. That's a very bad habit. Try to generalize instead. Extract common values to variables, such as the common prefix $(date '+%Y-%m-%d %H:%M:%S') pidvalue: $pidvalue, pidlimit:. Extract common operations to helper functions, such as logging to $logpath/process_monitor.log.


The last command of the program is exit 0. That's strange, for several reasons:

  • The script would exit anyway, you don't need to call exit explicitly.
  • By default, the exit code is the status code of the last command. An exit code of 0 means success. By running exit 0 as the last statement, you may cover up the failure of the last command. That's a suspicious thing to do.
\$\endgroup\$

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.