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