[Gllug] Simple Bash Script

Tue Jun 3 11:22:31 UTC 2008

Thank  you for all your responses it has been a major help, I haven't had
the chance to implement these changes as yet but I shall give you all a
update when I have. My Bash scripting isn't the greatest at any stretch of
the imagination but it is improving so thanks

Kind Regards,
Will

On Tue, Jun 3, 2008 at 11:04 AM, Philip Hands <phil at hands.com> wrote:

> On Tue, Jun 03, 2008 at 08:49:45AM +0100, John Edwards wrote:
> > On Mon, Jun 02, 2008 at 10:41:16AM +0100, william pink wrote:
> > > Hello All,
> > >
> > > I have a script that checks for system load then sends a email if it is
> over
> > > a certain threshold ( Its a cut down version from the one on Nixcraft )
> > > Here is the script
> > >
> > >
> > > #!/bin/bash
> > >
> > > # Script to check system load and remove from cluster if higher than
> 6.0
> > >
> > > TEMPFILE="$(mktemp)"
> > > NOTIFY="6.0"
> > > HOST=`hostname`
>
> I'd suggest replacing all ``s with $() for readability (and sanity if
> you ever have to nest them):
>
>  HOST=$(hostname)
>
> Although, given that it only gets used once, and not every time through
> the script, I'd generally not bother, and instead replace the $HOST with
> $(hostname) in the mail command
>
> > > RCLUSTER=`/usr/local/bin/in-cluster no`
> > > #load at 1 minute
> > > TRUE="1"
> > > LOAD=`uptime | sed -e "s/.*load average: \(.*\...\), \(.*\...\),
> > > \(.*\...\)/\1/" -e "s/ //g"`
>
> That's mostly redundant:
>
>  LOAD=$(uptime | sed -e "s/.*load average: \([0-9.]*\),.*/\1/")
>
> > >
> > > #input in tempfile
> > > echo "Load average to high removed from cluster, Load currently at
> $LOAD
> > > allowed limit $NOTIFY." > $TEMPFILE
>
> I agree with the anti-TMPFILE comments, see below -- also, that should
> be "too"
>
> > >
> > > # Compare results to benchmark
> > > RESULT=$(echo "$LOAD > $NOTIFY" | bc)
>
> If it's OK to assume bash, then we can use bash's built in ((maths))
> ability.  Of course, bash only deals with integers, but that can either
> be dealt with by using the integer portion of the load:
>
>  INTLOAD=$(uptime | sed -e "s/.*load average: \([0-9]*\)\..*/\1/")
>
> or use the 100/ths as units:
>
>  LOADx100=$(uptime | sed -e "s/.*load average: \([0-9.]*\),.*/\1/; s/\.//")
>
> > >
> > > # IF so do your thing
> > >
> > > if [ "$RESULT == $TRUE" ]; then
>
> then instead of that, use:
>
>  if (($INTLOAD > $INTNOTIFY)); then
>
> or
>
>  if (($LOADx100 > $NOTIFY * 100)); then
>
> or, if you're wanting to preserve the detail, then you could compare
> just the integer portion of the LOAD thus:
>
>
>  LOAD=$(uptime | sed -e "s/.*load average: \([0-9.]*\),.*/\1/")
>  INTLOAD=$(echo $LOAD | sed -e 's/\..*//')
>  ...
>  if (($INTLOAD > $INTNOTIFY)); then
>  ...
>     mail ... $LOAD
>
> or similarly, strip out the decimal point and use the LOADx100 approach
> (which of course is vulnerable to uptime's number of decimal places
> changing at some point)
>
>
> > >         $RCLUSTER
>
> Is that supposed to be a delayed invocation of:
>
>  /usr/local/bin/in-cluster no
>
> If so, I don't think it is -- that looks to me like the
> /usr/local/bin/in-cluster no will have run at the time of assignment,
> and this will be trying to execute the STDOUT as though it was a
> command which seems unlikely to be what you were after.
>
> I'd have thought you should just replace the $RCLUSTER with
>
>  /usr/local/bin/in-cluster no
>
> assuming that that's the command that's meant to take a node out of the
> cluster.
>
> > >         mail -s "$HOST has been removed from the cluster" "$EMAIL" <
> > > $TEMPFILE
> > > fi
> > >
> > > rm -f $TEMPFILE
>
> If you ever do need to create tempfiles like this (which in this case
> you don't but ...), I'd suggest setting up a trap before you create the
> file:
>
>  trap "rm -f $TEMPFILE" EXIT
>
> so that if the script fails somewhere in the middle, you won't end up
> with an infinite number of tempfiles.
>
> >
> > Hi
> >
> > I see that others have helped you with the if statements and other
> > things, but I would like to ask about if the TEMPFILE is really
> > neccesary.
> >
> > You should be able to send an email by echo'ing the contents to
> > mail (or mutt), eg something like:
> >
> > echo "Load average to high removed from cluster
> > Load currently at $LOAD
> > allowed limit $NOTIFY" | mail -s "$HOST has been removed from the
> cluster" "$EMAIL"
> >
> >
> > Getting rid of tempfiles is a little personnal mission of mine after
> > seeing thousands of them generated on some systems. Not only are they
> > messy if the script ends halfways through, but can also be a security
> > risk if the name is not suitably random or the contents are not
> > sanitised.
>
> Quite right, although for multi-line stuff like this, I tend to think
> that Here Documents are less fragile, so would suggest this:
>
> mail -s "$HOST has been removed from the cluster" "$EMAIL" <<!EOF!
> Load average too high removed from cluster
> Load currently at $LOAD
> allowed limit $NOTIFY
> !EOF!
>
>
> The string after << can be anything, and indicates the string that
> terminates the section that will be fed to the command as STDIN
>
> I settled on using ``!EOF!'' for that years ago, and have never found
> the need to vary that, as it's not something that crops up in normal
> use.
>
> If you're into indentation for readbility, you can prefix the terminator
> with a -, and then the input will have leading TABs removed:
>
> mail -s "$HOST has been removed from the cluster" "$EMAIL" <<-!EOF!
>                Load average too high removed from cluster
>                 Load currently at $LOAD
>                allowed limit $NOTIFY
>         !EOF!
>
> > Apart from that the other thing that might be cleaned up is the maths
> > evaluations. Rather than defining $TRUE and using bc to generate
> > $RESULT, I wonder if it's possible to use something like:
> >
> > if [ $LOAD -gt $NOTIFY ]
>
> That won't work with the decimal numbers in the original script, but
> would be fine with either the integer or x100 approaches above, and is
> probably more portable than the ((...)) approach.
>
> Cheers, Phil.
> --
> |)|  Philip Hands [+44 (0)20 8530 9560]    http://www.hands.com/
> |-|  HANDS.COM Ltd.                    http://www.uk.debian.org/
> |(|  10 Onslow Gardens, South Woodford, London  E18 1NE  ENGLAND
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.6 (GNU/Linux)
>
> iD8DBQFIRRc0YgOKS92bmRARArebAKCeuEX1oKp2NLTSQ+phhMVHfW++DACfQDHb
> hHSD/yTLAR5PlYg9e/dGTOQ=
> =jQTv
> -----END PGP SIGNATURE-----
>
> --
> Gllug mailing list  -  Gllug at gllug.org.uk
> http://lists.gllug.org.uk/mailman/listinfo/gllug
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.lug.org.uk/pipermail/gllug/attachments/20080603/3a910cbe/attachment.html>
-------------- next part --------------
-- 
Gllug mailing list  -  Gllug at gllug.org.uk
http://lists.gllug.org.uk/mailman/listinfo/gllug