[OSSTEST PATCH 0/6] Bisector fixes

[OSSTEST PATCH 0/6] Bisector fixes

[Ian Jackson]

This series seems to me to fix some problems with the bisector that
Ian C observed and which he and I have discussed in person.

I have done various ad-hoc tests in the Cambridge instance, but the
bisector itself is not tested by the osstest self-push-gate, and it is
of course difficult to foresee all the implications.  So after this
goes in we should keep an eye on the progress of the bisector in the
various branches.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

[OSSTEST PATCH 1/6] cs-bisection-step: Refer to jobs we create just by job name

[Ian Jackson]

When we make a fresh build job, rather than referring to an existing
job in another flight, pass to the rest of the machinery only the job
name, not <flight>.<job>.

This means that the generated flight refers to its own jobs without
specifying the flight name.  This allows the flail detector to operate
properly: without this, we might have repeated attempts to test and
build the same thing, but they would look identical because their
self-referential runvars would be different due to their different
flight numbers.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 cs-bisection-step |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cs-bisection-step b/cs-bisection-step
index 99dbcf2..e8b95ca 100755
--- a/cs-bisection-step
+++ b/cs-bisection-step
@@ -1152,7 +1152,7 @@ END
                                  )
 END
 
-        $usejob= "$popflight.$popjob";
+        $usejob= $popjob;
         $jobs_created{$popjob}= $usejob;
     }
 
-- 
1.7.10.4

Re: [OSSTEST PATCH 1/6] cs-bisection-step: Refer to jobs we create just by job name

[Ian Campbell]

On Wed, 2015-05-20 at 13:54 +0100, Ian Jackson wrote:
> When we make a fresh build job, rather than referring to an existing
> job in another flight, pass to the rest of the machinery only the job
> name, not <flight>.<job>.
> 
> This means that the generated flight refers to its own jobs without
> specifying the flight name.  This allows the flail detector to operate
> properly: without this, we might have repeated attempts to test and
> build the same thing, but they would look identical because their
> self-referential runvars would be different due to their different
> flight numbers.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[OSSTEST PATCH 2/6] cs-bisection-step: Clarify and correct flailing message

[Ian Jackson]

There are many other possible reasons for this, besides a bug in the
build version machinery.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 cs-bisection-step |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/cs-bisection-step b/cs-bisection-step
index e8b95ca..4891e93 100755
--- a/cs-bisection-step
+++ b/cs-bisection-step
@@ -1275,10 +1275,9 @@ END
 	print DEBUG "nequalflights=$nequalflights\n$explanation\n";
 	if ($nequalflights > $maxequalflights) {
 	    summary_report(<<END, <<END, 32);
-Flailing - >$nequalflights identical flights to no useful effect
+Flailing - >=$nequalflights identical flights to no useful effect
 END
-Bisector is flailing: >$nequalflights identical flights generated.
-Perhaps machinery for building correct version is broken ?
+Bisector is flailing: >=$nequalflights identical flights generated.
 Problem occurs when attempting to test this revision:
  $choose->{Rtuple}
 
-- 
1.7.10.4

Re: [OSSTEST PATCH 2/6] cs-bisection-step: Clarify and correct flailing message

[Ian Campbell]

On Wed, 2015-05-20 at 13:54 +0100, Ian Jackson wrote:
> There are many other possible reasons for this, besides a bug in the
> build version machinery.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[OSSTEST PATCH 3/6] cs-bisection-step: Flail detection: look only at our blessing

[Ian Jackson]

When looking for identical previously flights, consider only ones
which have the same blessing as our prospective flight will have.

There are good reasons why apparently identical flights might appear
with other in-scope blessings: notably, a single-test-job main branch
might produce many failures from its push gate, which would all have
the main branch blessing (rather than the bisector's blessing).

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 cs-bisection-step |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/cs-bisection-step b/cs-bisection-step
index 4891e93..3035912 100755
--- a/cs-bisection-step
+++ b/cs-bisection-step
@@ -1230,7 +1230,10 @@ END
     # one side is NULL (runvar was missing) it still counts.
     my $equalflightsqt = <<END;
         SELECT flight, blessing, started FROM flights
-	    WHERE branch=? AND $blessingscond $maxflight_cond
+	    WHERE branch=?  $maxflight_cond
+              AND blessing = (
+		SELECT intended FROM flights WHERE flight = ?
+                )
 	      AND NOT EXISTS (
 		SELECT 1
 		  FROM $runvarsqt1 r1 FULL OUTER JOIN
@@ -1260,7 +1263,7 @@ END
         $minflight //= 0;
 	print DEBUG "minflight=$minflight\n";
 
-	$equalflightsq->execute($branch, $popflight, $minflight);
+	$equalflightsq->execute($branch, $popflight, $popflight, $minflight);
 	my $nequalflights = 0;
 	my $explanation = '';
 	while (my $identical = $equalflightsq->fetchrow_hashref()) {
-- 
1.7.10.4

Re: [OSSTEST PATCH 3/6] cs-bisection-step: Flail detection: look only at our blessing

[Ian Campbell]

On Wed, 2015-05-20 at 13:54 +0100, Ian Jackson wrote:
> When looking for identical previously flights, consider only ones
> which have the same blessing as our prospective flight will have.
> 
> There are good reasons why apparently identical flights might appear
> with other in-scope blessings: notably, a single-test-job main branch
> might produce many failures from its push gate, which would all have
> the main branch blessing (rather than the bisector's blessing).
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[OSSTEST PATCH 4/6] Revert "cs-bisection-step: Abandon repro attempts after a bit"

[Ian Jackson]

This safety catch is unnecessary and unhelpful.

It is unnecessary because 489773b4 "Detect flailing" will detect
attempts by the bisector to repeatedly run the same flight and hope
for different results.

It is unhelpful because it can happen for good reasons that a
particular revision has been tested many times.  In particular:

 - The osstest push gate input tree may have not been advanced for a
   long time and been failing its push gate.

 - The bisector may have (for some reason[1]) restarted with a new
   baseline, and the temporarally-stripy pass/fail requirement would
   then require the basis fail to be repro'd, again.

[1] Currently this happens much more often than is desirable.  This
will be fixed in a moment.

This reverts commit 2676277181599a889657354028b992379aa6142b.
---
 cs-bisection-step |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/cs-bisection-step b/cs-bisection-step
index 3035912..743a3e5 100755
--- a/cs-bisection-step
+++ b/cs-bisection-step
@@ -769,10 +769,6 @@ sub need_repro ($$$) {
     return 1 if conflicted_warning($n, $what);
 
     my $fl= $n->{Flights} || [];
-
-    return report_conflict($n, $what, 'Too many attempts')
-	if @$fl > 5;
-
     foreach my $f (sort { $a->{Flight} <=> $b->{Flight} } @$fl) {
         next unless $f->{Flight} > $repro_lastflight;
 	if ($f->{Result} ne $st) {
-- 
1.7.10.4

Re: [OSSTEST PATCH 4/6] Revert "cs-bisection-step: Abandon repro attempts after a bit"

[Ian Campbell]

On Wed, 2015-05-20 at 13:54 +0100, Ian Jackson wrote:
> This safety catch is unnecessary and unhelpful.
> 
> It is unnecessary because 489773b4 "Detect flailing" will detect
> attempts by the bisector to repeatedly run the same flight and hope
> for different results.
> 
> It is unhelpful because it can happen for good reasons that a
> particular revision has been tested many times.  In particular:
> 
>  - The osstest push gate input tree may have not been advanced for a
>    long time and been failing its push gate.
> 
>  - The bisector may have (for some reason[1]) restarted with a new
>    baseline, and the temporarally-stripy pass/fail requirement would
>    then require the basis fail to be repro'd, again.
> 
> [1] Currently this happens much more often than is desirable.  This
> will be fixed in a moment.
> 
> This reverts commit 2676277181599a889657354028b992379aa6142b.

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[OSSTEST PATCH 5/6] cs-bisection-step: Do not treat repro attempts as flail

[Ian Jackson]

The need_repro machinery deliberarely makes attempts to reproduce
various results.

This can cause the flail detector to trigger when not intended.  In
particular, the bisector may have (for some reason[1]) restarted with
a new baseline, and the temporarally-stripy pass/fail requirement
would then require the basis fail to be repro'd, again.

[1] Currently this happens much more often than is desirable.  This
will be fixed in a moment.

Fix this by only considering, for the purposes of flail, flights which
are no older than the first repro (the basis pass).

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 cs-bisection-step |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/cs-bisection-step b/cs-bisection-step
index 743a3e5..da13927 100755
--- a/cs-bisection-step
+++ b/cs-bisection-step
@@ -742,6 +742,7 @@ sub conflicted_warning ($$) {
 }
 
 our $repro_lastflight;
+our $repro_firstflight = 0;
 our $repro_count;
 
 sub need_repro_sequence ($$) {
@@ -780,6 +781,7 @@ sub need_repro ($$$) {
             ($repro_count ? "Repro" : "Result").
             " found: flight $f->{Flight} ($st), for $what\n";
         $repro_lastflight= $f->{Flight};
+        $repro_firstflight ||= $f->{Flight};
         return 0;
     }
     print STDERR "Need to reproduce $what ($st); had $repro_count already.\n";
@@ -1246,12 +1248,13 @@ END
     my $equalflightsq = $dbh_tests->prepare($equalflightsqt);
 
     db_retry($popflight,'constructing', $dbh_tests,[qw(flights)], sub {
-        print STDERR "Checking for flail...\n";
+        print STDERR "Checking for flail (since $repro_firstflight)...\n";
 
 	my ($minflight) = $dbh_tests->selectrow_array(<<END, {}, $branch);
             SELECT flight FROM (
 		SELECT flight FROM flights
 		    WHERE branch=? AND $blessingscond $maxflight_cond
+                       AND flight >= $repro_firstflight
 		    ORDER BY flight DESC
 		    LIMIT 100
             ) last100 ORDER BY FLIGHT ASC LIMIT 1
-- 
1.7.10.4

Re: [OSSTEST PATCH 5/6] cs-bisection-step: Do not treat repro attempts as flail

[Ian Campbell]

On Wed, 2015-05-20 at 13:54 +0100, Ian Jackson wrote:
> The need_repro machinery deliberarely makes attempts to reproduce
> various results.
> 
> This can cause the flail detector to trigger when not intended.  In
> particular, the bisector may have (for some reason[1]) restarted with
> a new baseline, and the temporarally-stripy pass/fail requirement
> would then require the basis fail to be repro'd, again.
> 
> [1] Currently this happens much more often than is desirable.  This
> will be fixed in a moment.
> 
> Fix this by only considering, for the purposes of flail, flights which
> are no older than the first repro (the basis pass).
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[OSSTEST PATCH 6/6] Revert "cs-bisection-step: allow -bisect blessed flights for basis pass"

[Ian Jackson]

If the bisector is allowed to consider its own output flights as
candidates for the basis pass, then it will (generally) restart with a
new basis each time it finds a pass.

This is very slow and wasteful.

There is not much explanation in 01edca47 of the change I am now
reverting, but I think I probably created by some semi-manual process
a flight to serve as the basis.  I now think that my mistake was to
bless that flight `adhoc-bisect' or some such, rather than `adhoc'.

This reverts commit 01edca47be3742a1660b1956c1f06ca934b97352.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 cs-bisection-step |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/cs-bisection-step b/cs-bisection-step
index da13927..67a92b7 100755
--- a/cs-bisection-step
+++ b/cs-bisection-step
@@ -320,7 +320,7 @@ END
         SELECT * FROM flights JOIN steps USING (flight)
 	   WHERE job = ?
 	     AND testid = ?
-	     AND $blessingscond
+	     AND blessing = ?
 	     AND status = 'pass'
              AND branch = ?
              AND $flight_is_not_broken
@@ -371,7 +371,7 @@ END
 
         print STDERR "/";
         my ($basisrow,$trybasis);
-        $basisq->execute($job,$testid,$branch);
+        $basisq->execute($job,$testid,$blessings[0],$branch);
         while ($trybasis= $basisq->fetchrow_hashref()) {
             print STDERR " $trybasis->{flight}";
             my $basishosts= relevant_hosts($trybasis->{flight});
-- 
1.7.10.4

Re: [OSSTEST PATCH 6/6] Revert "cs-bisection-step: allow -bisect blessed flights for basis pass"

[Ian Campbell]

On Wed, 2015-05-20 at 13:54 +0100, Ian Jackson wrote:
> If the bisector is allowed to consider its own output flights as
> candidates for the basis pass, then it will (generally) restart with a
> new basis each time it finds a pass.
> 
> This is very slow and wasteful.
> 
> There is not much explanation in 01edca47 of the change I am now
> reverting, but I think I probably created by some semi-manual process
> a flight to serve as the basis.  I now think that my mistake was to
> bless that flight `adhoc-bisect' or some such, rather than `adhoc'.
> 
> This reverts commit 01edca47be3742a1660b1956c1f06ca934b97352.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>