Ubuntu Test Cases

Code review comment for lp:~asac/ubuntu-test-cases/use-top-and-always-dumb-toplog

use-top-and-always-dumb-toplog
Merge into touch

Revision history for this message

Alexander Sack (asac) wrote on 2013-08-19:

> I haven't looked too deeply just yet, but there are some things I see after a
> quick trial run locally:
>
> >Measurement:
> > + idle level: 148.50
> > + idle sum: 148.5 / count: 2
> >
> >system settled. SUCCESS
> I told it to only pass if it hit 99% or better, and my system was constantly
> at 80% or less idle... clearly this is not good.

I will fix the problem you saw with your 140% idle average ... need a way to reproduce. The script works here on my system and on the phone.

>
> Using top in the way you are using it right now means that we only see that
> it's starting the first pass, but then we don't get any more feedback that
> something is happening until the end of the entire run.

We see the result... not the pumping. Instead you get everything nicely at the end. I think its good that way. I will tweak the output to be clearer that its just "measuring now..."

>
> 27 +echo " cmd = 'top -b -d $vmstat_wait -n $vmstat_repeat' ignoring
> first $vmstat_ignore (tail: $vmstat_tail)"
> Instead of printing out 7 lines of verbose detail about the options we just
> passed it, I think it would make more sense to just have the above line do
> something like print the exact command line args. I had thought about changing
> this in the first pass, but waited.

Yeah let me do something about this. will probably remove that line because we have all the details now. And then using set -x set +x to turn on and off echoing the command line (so we really see what happens).

>
> 48 + top -b -d $vmstat_wait -n $vmstat_repeat >> $top_log
> 49 + cat $top_log | grep '.Cpu.*' | tail -n $vmstat_tail >
> $vmstat_log.reduced
> If we are no longer using vmstat, we may as well change the variable names to
> reflect that as well.
>
> Printing the entire top log of every run is not always desirable, and
> sometimes gets in the way. Considering our primary use, I think it's sensible
> to have it on by default, but we should at least have an option to turn it
> off.

I see that some might want it, but then I don't think that use case is really relevant to maintain right now... unless you have a real case where we dont want it in automation.

The

> I haven't looked too deeply just yet, but there are some things I see after a
> quick trial run locally:
> 
> >Measurement:
> >  + idle level: 148.50
> >  + idle sum: 148.5 / count: 2
> >
> >system settled. SUCCESS
> I told it to only pass if it hit 99% or better, and my system was constantly
> at 80% or less idle... clearly this is not good.

I will fix the problem you saw with your 140% idle average ... need a way to reproduce. The script works here on my system and on the phone.

> 
> Using top in the way you are using it right now means that we only see that
> it's starting the first pass, but then we don't get any more feedback that
> something is happening until the end of the entire run.

We see the result... not the pumping. Instead you get everything nicely at the end. I think its good that way. I will tweak the output to be clearer that its just "measuring now..."

> 
> 27      +echo " cmd = 'top -b -d $vmstat_wait -n $vmstat_repeat' ignoring
> first $vmstat_ignore (tail: $vmstat_tail)"
> Instead of printing out 7 lines of verbose detail about the options we just
> passed it, I think it would make more sense to just have the above line do
> something like print the exact command line args. I had thought about changing
> this in the first pass, but waited.

> 
> 48      + top -b -d $vmstat_wait -n $vmstat_repeat >> $top_log
> 49      + cat $top_log | grep '.Cpu.*' | tail -n $vmstat_tail >
> $vmstat_log.reduced
> If we are no longer using vmstat, we may as well change the variable names to
> reflect that as well.
> 
> Printing the entire top log of every run is not always desirable, and
> sometimes gets in the way. Considering our primary use, I think it's sensible
> to have it on by default, but we should at least have an option to turn it
> off.

I see that some might want it, but then I don't think that use case is really relevant to maintain right now... unless you have a real case where we dont want it in automation.

The

« Back to merge proposal