Re: [Xen-devel] [OSSTEST] ms-planner: add a flight summary html report

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-04-30 at 16:44 +0100, Ian Jackson wrote:

> > +    while (my ($reso,$evts) = each %{ $plan->{Events} }) {
> > +        # [osstest real] job 26010.test-amd64-amd64-xl-win7-amd64
> > +        foreach my $evt ( @{$evts} ) {
> > +            next unless $evt->{Type} =~ m/^(Start|End)$/;
> > +            next unless $evt->{Info} =~ m/^\[(\S+) (\S+)\] job ([0-9]+)\
> \.(\S+) (.*)/;
> 
> This is rather a layering violation.  Info is for the consumption of
> humans.  Perhaps the plan should have an explicit "flight.job" field.

I was a bit more scared of touching the actual planner daemon... But ok.

> You could compute the "[linux-3.4 real]" part by looking at the common
> prefix of all the Infos.
> 
> Also, can you wrap everything to 75 or so ?

Sure.

> > +            $flights{$flight}->{Jobs}{$job} = {
> > +                Reso => $reso
> > +            } unless $flights{$flight}->{Jobs}{$job};
> 
> This isn't right.  A job can have more than one resource.  You should
> either list each resource as its own line, accumulate a list of
> resources.

Oh yes, I'd forgotten this.

> > +            if ( $evt->{Time} > $flights{$flight}->{End} ) {
> > +                $flights{$flight}->{Last} = $evt;
> > +                $flights{$flight}->{LastReso} = $reso;
> 
> You don't seem to use LastReso and I can't seem to see why you'd want
> it.

I think I was just being a completest in case I decided I wanted it.

> 
> > +        flight_hdr("Flight $flight [$inf->{Branch} $inf->{Blessing}] ".
> > +               (keys %{$inf->{Jobs}})." jobs ".
> > +               "expected to run until ".strftime("%Y-%b-%d %a %H:%M:%S"\
> , localtime $inf->{End}));
> 
> You repeat the strftime rune many times.  You should probably use
> show_abs_time.  (Which produces UTC.  I see that ms-planner has a pair
> of localtimes in it, which should perhaps be changed.)

Ack

> > +        foreach my $job (sort { $inf->{Jobs}{$a}->{Start}->{Time} cmp $\
> inf->{Jobs}{$b}->{Start}->{Time} } keys %{ $inf->{Jobs} }) {
> 
> You repeat the $inf->{Jobs}{$x}->{Start}->{Time} expression.  Make it
> an anonymous subref or something and this will become much clearer.

You mean like:
   my $ta = sub { return $inf->{Jobs}{$a}->.... }
   my $tb = sub { return $inf->{Jobs}{$b}->.... }
   foreach ... (sort { $ta() cmp $tb() } keys ... ) { 

Or my $to = sub($) { my $it=shift; return $info-{Jobs}{$it}... } and
$to($a) ?

> And I think you should probably sort the jobs by end time, not start
> time.

OK.

I should note that I realised after I wrote this patch that as jobs
complete others might appear, so you can't necessarily know when the
flight will end, although once the build jobs are done you can generally
get a pretty good idea.

> > +            my $sevt = $inf->{Jobs}{$job}->{Start};
> > +            my $eevt = $inf->{Jobs}{$job}->{End};
> > +            print("<tr>\n");
> > +            cell($job);
> > +            cell(strftime("%Y-%b-%d %a %H:%M:%S", localtime $inf->{Jobs\
> }{$job}->{Start}->{Time}));
> > +            cell(strftime("%Y-%b-%d %a %H:%M:%S", localtime $inf->{Jobs\
> }{$job}->{End}->{Time}));
> 
> How about
>     foreach my $se (qw(Start End)) {
> ?

OK. I never would have come up with that myself.

> > diff --git a/ms-queuedaemon b/ms-queuedaemon
> > index 26d83e2..a5bebd3 100755
> > --- a/ms-queuedaemon
> > +++ b/ms-queuedaemon
> > @@ -224,12 +224,18 @@ proc queuerun-perhaps-step {} {
> >  proc report-plan {} {
> >      global c
> >      if {[catch {
> > -        exec ./ms-planner show-html > "$c(WebspaceFile)/resource-plan.html"
> > +        exec ./ms-planner show-resource-plan-html > "$c(WebspaceFile)/r\
> esource-plan.html"
> >      } emsg]} {
> > -        log "INTERNAL ERROR showing plan html: $emsg"
> > +        log "INTERNAL ERROR showing resource plan html: $emsg"
> >      } else {
> >          log "report-plan OK"
> >      }
> > +    if {[catch {
> > +        exec ./ms-planner show-flight-summary-html > "$c(WebspaceFile)/\
> flight-summary.html"
> > +    } emsg]} {
> > +        log "INTERNAL ERROR showing flight summary html: $emsg"
> > +    } else {
> > +        log "report-flight-summary OK"
> 
> Repetition.

I suppose I can probably figure out enough Tcl to avoid it... I was
being cowardly about making no-trivial mods ;-)

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel