All of lore.kernel.org
 help / color / mirror / Atom feed
* [OSSTEST PATCH 1/9] README.dev: Document the blessings
@ 2015-12-17 17:06 Ian Jackson
  2015-12-17 17:06 ` [OSSTEST PATCH 2/9] mg-schema-test-database: Provide some timeouts which are better for testing Ian Jackson
                   ` (8 more replies)
  0 siblings, 9 replies; 31+ messages in thread
From: Ian Jackson @ 2015-12-17 17:06 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Jackson, Ian Campbell

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 README.dev |  101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/README.dev b/README.dev
index 65ec111..879af60 100644
--- a/README.dev
+++ b/README.dev
@@ -232,3 +232,104 @@ probably are after a reboot) by looking for processes relating to the
 lists flight,job combinations and then:
 
 $ ./mg-hosts previoustasks --clear
+
+
+Flight blessings and the blessed-* host flags
+=============================================
+
+Both flights and hosts have a `blessing'.
+
+Blessings are used:
+
+ * To avoid accidentally tools operating on flights which are in the
+   wrong phase of their existence.
+
+ * To control which automatic archaeologists will look at which
+   flights.  (Archeaology tools take arguments to control which
+   blessings they will consider.)
+
+ * To control which hosts are considered suitable for use with which
+   flight.  (Flights with blessing `foo' use only hosts which have
+   flag `blessed-foo'.)
+
+Each flight has a `blessing' and an `intended blessing'.  The
+`intended blessing' is what the flight is going to be blessed as when
+its execution has completed.  The intended blessing controls host
+allocation.
+
+(Normally the `intended blessing' is the same as the bless argument to
+sg-execute-flight aka the -B argument to mg-execute-flight.)
+
+These are the principal (intended) blessings:
+
+ * `real': Flights which are fully production runs.  They use only
+   clean working trees with no `funny' environment variables, and
+   versions of osstest intended for production.  The data from such
+   flights is used, where applicable, to control production push gate
+   decisions, etc.
+
+   `blessed-real' hosts are fully production-ready.
+
+ * `play': Playground.  Flights may use weird software, dirty working
+   trees, strange environment variables, and so on.  It does not make
+   sense to point archaeology tools at such flights.
+
+   `blessed-play' hosts may be partially or completely nonfunctional,
+   or strange in some way.  For this reason `play' flights should not
+   normally use the automatic host allocator.
+
+ * `adhoc': Special-purpose flights.  They should be run with software
+   nearly as clean as `real': the version of the software used may not
+   be a branch intended for-production, but any enhancements or
+   modifications should not leave false or misleading information in
+   the history database.
+
+   `blessed-adhoc' hosts are just as good as `blessed-real' ones.
+
+   The `adhoc' blessing exists mostly to protect the main production
+   workflow from unintentional violations (for example, unexpectedly
+   buggy historical flight data).
+
+ * `commission-<something>': Flights used for testing that a
+   particular subset of the hosts are working properly.  The hosts to
+   be tested should be blessed `commission-whatever' during
+   commissioning, and that blessing removed and replaced with `real'
+   when the hosts are ready.
+
+ * `real-bisect' and `adhoc-bisect': These are found only as the
+   blessing of finished flights.  (This is achieved by passing *-adhoc
+   to sg-execute-flight.)  This allows the archaeologist tools to
+   distinguish full flights from bisection steps.
+
+   The corresponding intended blessing (as found in the `intended'
+   column of the flights table) is `real'.  So the hosts used by the
+   automatic host allocator are those flagged `blessed-real' or
+   `blessed-adhoc' - there are no separate blessed-*-bisect hostflags.
+
+There are also blessings for unfinished flights:
+
+ * `constructing': The flight metadata (jobs, runvars, etc.) is still
+   being populated.  The tools for flight construction (and flight
+   construct editing) generally insist that they operate only on
+   flights in this state.
+
+ * `running': At least one of the jobs in this flight has started.
+   Flight execution software will generally (perhaps via standard
+   library calls) mark a flight `running' if they find it
+   `constructing', and refuse to operate on other flights.
+
+ * `broken': Something went catastrophically wrong.
+
+Note that osstest is generally `crash-only software': nothing will
+`clean up' a crashed flight and set its blessing to `broken'.  Flights
+which were in progress once upon a time but never completed, because
+they crashed, are simply left with whatever blessing they had at the
+time.
+
+There is a special exception to the tools' flight status checks: any
+flight whose blessing contains `play' can be operated on out of order.
+
+Flights blessings can be manually changed with cs-flight-bless.  Eg
+  ./cs-flight-bless FLIGHT broken-real real
+updates FLIGHT to be marked `broken' rather than `real'.  This can be
+useful if a flight's data is misleading to the archaeologists.
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [OSSTEST PATCH 2/9] mg-schema-test-database: Provide some timeouts which are better for testing
  2015-12-17 17:06 [OSSTEST PATCH 1/9] README.dev: Document the blessings Ian Jackson
@ 2015-12-17 17:06 ` Ian Jackson
  2015-12-17 17:19   ` Ian Campbell
  2015-12-17 17:06 ` [OSSTEST PATCH 3/9] mg-schema-test-database: Wipe previous local plan data Ian Jackson
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 31+ messages in thread
From: Ian Jackson @ 2015-12-17 17:06 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Jackson, Ian Campbell

The default timeouts mean that after starting a test db queue daemon
and a test db allocation attempt, we have to wait two minutes.

Lower timeouts increase the risk that we might lose noncritical races
and allocate resources to the `wrong' tasks.  And they reduce the
duration of an outage which will cause a planned allocation attempt to
fail.

I think we don't care about those problems for test instances.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 mg-schema-test-database |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/mg-schema-test-database b/mg-schema-test-database
index bf82c75..818cf89 100755
--- a/mg-schema-test-database
+++ b/mg-schema-test-database
@@ -336,6 +336,8 @@ QueueDaemonHost $ctrlhost
 OwnerDaemonPort ${ctrlports%,*}
 QueueDaemonPort ${ctrlports#*,}
 ExecutiveDbOwningRoleRegexp .*
+QueueDaemonHoldoff 3
+QueueDaemonRetry 5
 END
 	mv -f $tcfg.tmp $tcfg
 
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [OSSTEST PATCH 3/9] mg-schema-test-database: Wipe previous local plan data
  2015-12-17 17:06 [OSSTEST PATCH 1/9] README.dev: Document the blessings Ian Jackson
  2015-12-17 17:06 ` [OSSTEST PATCH 2/9] mg-schema-test-database: Provide some timeouts which are better for testing Ian Jackson
@ 2015-12-17 17:06 ` Ian Jackson
  2015-12-17 17:22   ` Ian Campbell
  2015-12-17 17:06 ` [OSSTEST PATCH 4/9] mg-schema-test-database: Borrow shares properly Ian Jackson
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 31+ messages in thread
From: Ian Jackson @ 2015-12-17 17:06 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Jackson, Ian Campbell

Whatever is in the user's cwd is unlikely to correspond to anything
real.  In principle it might be possible to obtain an official copy
from the real daemons, and massage it, or something, but that's a lot
of work.

Instead, just remove it when we start the test db daemons.

In principle it would be more correct to remove it when we set up the
test db, because it is at that point that we create the new view of
the world.  Removing the old plan data when we start daemons means
that if the user, who is testing, restarts the daemons, the
newly-created queue daemon does not have information about allocations
made with the previous daemon, and instead regards those allocations
as rogue.

However, removing the file only when the daemons are started means
that if the user has saved a data-plan.pl in their cwd for some other
reason we don't remove it unless the user is actually going to run the
daemons.  So I think this is preferable.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 mg-schema-test-database |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/mg-schema-test-database b/mg-schema-test-database
index 818cf89..a4cb732 100755
--- a/mg-schema-test-database
+++ b/mg-schema-test-database
@@ -553,6 +553,8 @@ daemons)
 	withtest \
 	exec_resetting_sigint ./ms-queuedaemon &
 
+	rm -f data-plan.pl
+
 	wait
 
 	;;
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [OSSTEST PATCH 4/9] mg-schema-test-database: Borrow shares properly
  2015-12-17 17:06 [OSSTEST PATCH 1/9] README.dev: Document the blessings Ian Jackson
  2015-12-17 17:06 ` [OSSTEST PATCH 2/9] mg-schema-test-database: Provide some timeouts which are better for testing Ian Jackson
  2015-12-17 17:06 ` [OSSTEST PATCH 3/9] mg-schema-test-database: Wipe previous local plan data Ian Jackson
@ 2015-12-17 17:06 ` Ian Jackson
  2015-12-17 17:26   ` Ian Campbell
  2015-12-17 17:06 ` [OSSTEST PATCH 5/9] ms-planner: Improve an error message Ian Jackson
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 31+ messages in thread
From: Ian Jackson @ 2015-12-17 17:06 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Jackson, Ian Campbell

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 mg-schema-test-database |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/mg-schema-test-database b/mg-schema-test-database
index a4cb732..4e0ee68 100755
--- a/mg-schema-test-database
+++ b/mg-schema-test-database
@@ -448,12 +448,22 @@ END
 	done
 
 	# As we copy, we note everything we're not borrowing as
-	# belonging to the parent db.
+	# belonging to the parent db.  We borrow shares of a shared
+	# resource.  If we borrow only some rather than all of the
+	# shares, neither DB will be able to unshare it.
+
+	# In principle it might be possible to actually use different
+	# shares of the same resource with different dbs.  However the
+	# `sharetype' contains the osstest revision, which prevents
+	# sharing between test and real versions of osstest code.
+
 	cat >>$t.import <<END
 		$(make_xdbref_task $maindbname 'not borrowed' '' PARENT)
 		UPDATE resources
 			SET owntaskid = $(taskid xdbref $maindbname)
-			WHERE owntaskid != $(borrowtaskid $task);
+			WHERE owntaskid != $(borrowtaskid $task)
+			  AND owntaskid != $(taskid magic shared)
+			  AND owntaskid != $(taskid magic preparing);
 		COMMIT;
 END
 
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [OSSTEST PATCH 5/9] ms-planner: Improve an error message
  2015-12-17 17:06 [OSSTEST PATCH 1/9] README.dev: Document the blessings Ian Jackson
                   ` (2 preceding siblings ...)
  2015-12-17 17:06 ` [OSSTEST PATCH 4/9] mg-schema-test-database: Borrow shares properly Ian Jackson
@ 2015-12-17 17:06 ` Ian Jackson
  2015-12-17 17:26   ` Ian Campbell
  2015-12-17 17:06 ` [OSSTEST PATCH 6/9] db_retry: Suppress an "exiting via last" warning Ian Jackson
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 31+ messages in thread
From: Ian Jackson @ 2015-12-17 17:06 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Jackson, Ian Campbell

I experienced this `die' due to mg-schema-test-database failing to
borrow shared hosts properly, and added this Dumper for debugging.

I have not bothered to improve any of the other quite terse `die's in
ms-planner.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 ms-planner |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ms-planner b/ms-planner
index fc65a5b..8c2e06f 100755
--- a/ms-planner
+++ b/ms-planner
@@ -319,7 +319,9 @@ END
 	my ($reso, $shareix) = ($`, $1);
 
 	my $share= $currentshare{$reso};
-	die unless !!$share == !!$shareix;
+	die Dumper($reskey, $reso, $shareix,
+		   $plan->{Allocations}{$reskey}, $currentshare{$reso})
+	    unless !!$share == !!$shareix;
 
         if ($share && $share->{OnlyPreparing}) {
             delete $plan->{Allocations}{$reskey};
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [OSSTEST PATCH 6/9] db_retry: Suppress an "exiting via last" warning
  2015-12-17 17:06 [OSSTEST PATCH 1/9] README.dev: Document the blessings Ian Jackson
                   ` (3 preceding siblings ...)
  2015-12-17 17:06 ` [OSSTEST PATCH 5/9] ms-planner: Improve an error message Ian Jackson
@ 2015-12-17 17:06 ` Ian Jackson
  2015-12-17 17:31   ` Ian Campbell
  2015-12-17 17:06 ` [OSSTEST PATCH 7/9] Executive DB retry: Avoid an undefined warning Ian Jackson
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 31+ messages in thread
From: Ian Jackson @ 2015-12-17 17:06 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Jackson, Ian Campbell

This warning appears when db_retry_abort is used, since 2b069b6c
"Database locking: Perl: Retry all deadlocks in PostgreSQL".

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 Osstest.pm |    1 +
 1 file changed, 1 insertion(+)

diff --git a/Osstest.pm b/Osstest.pm
index c77d960..a398b1e 100644
--- a/Osstest.pm
+++ b/Osstest.pm
@@ -299,6 +299,7 @@ sub db_retry ($$$;$$) {
 	    $r= &$body;
 	    if ($db_retry_stop) {
 		$dbh->rollback();
+		no warnings qw(exiting);
 		last if $db_retry_stop eq 'abort';
 		next;
 	    }
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [OSSTEST PATCH 7/9] Executive DB retry: Avoid an undefined warning
  2015-12-17 17:06 [OSSTEST PATCH 1/9] README.dev: Document the blessings Ian Jackson
                   ` (4 preceding siblings ...)
  2015-12-17 17:06 ` [OSSTEST PATCH 6/9] db_retry: Suppress an "exiting via last" warning Ian Jackson
@ 2015-12-17 17:06 ` Ian Jackson
  2015-12-17 17:31   ` Ian Campbell
  2015-12-17 17:06 ` [OSSTEST PATCH 8/9] mg-allocate: Better error handling when no candidates Ian Jackson
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 31+ messages in thread
From: Ian Jackson @ 2015-12-17 17:06 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Jackson, Ian Campbell

If something other than the DB statements inside need_retry throws an
exception, ->err will normally be undef (because
$dbh_tests->begin_work will clear it, if nothing else).

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 Osstest/JobDB/Executive.pm |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Osstest/JobDB/Executive.pm b/Osstest/JobDB/Executive.pm
index 6fb77a4..c0af21c 100644
--- a/Osstest/JobDB/Executive.pm
+++ b/Osstest/JobDB/Executive.pm
@@ -50,7 +50,7 @@ sub begin_work ($$$) { #method
 sub need_retry ($$$) {
     my ($jd, $dbh,$committing) = @_;
     return
-	$dbh_tests->err()==7 &&
+	($dbh_tests->err() // 0)==7 &&
 	($dbh_tests->state =~ m/^40P01/); # DEADLOCK DETECTED
 }
 
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [OSSTEST PATCH 8/9] mg-allocate: Better error handling when no candidates
  2015-12-17 17:06 [OSSTEST PATCH 1/9] README.dev: Document the blessings Ian Jackson
                   ` (5 preceding siblings ...)
  2015-12-17 17:06 ` [OSSTEST PATCH 7/9] Executive DB retry: Avoid an undefined warning Ian Jackson
@ 2015-12-17 17:06 ` Ian Jackson
  2015-12-17 17:33   ` Ian Campbell
  2015-12-17 17:06 ` [OSSTEST PATCH 9/9] mg-allocate: In planner mode, pre-check the arguments Ian Jackson
  2015-12-17 17:18 ` [OSSTEST PATCH 1/9] README.dev: Document the blessings Ian Campbell
  8 siblings, 1 reply; 31+ messages in thread
From: Ian Jackson @ 2015-12-17 17:06 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Jackson, Ian Campbell

Spot when our db search revealed no candidates for the resources to
allocate, and:
 - when doing an immediate allocation, call it an error
 - when doing a planned allocation, cause it to prevent allocation
   on this iteration, and print a suitably unreassuring message

Previously it would simply say `nothing available'.

Implement this as follows:
 - Report lack of candidates as $ok=-1 from alloc_1rescand
 - In alloc_1res, return this -1 as with any non-zero $ok
 - Handle the new $ok at all the call sites, in particular
 - In plan(), rename `allok' to `worstok' and have it be
   the worst relevant $ok value.  If $ok gives -1, return
   undef, rather than a booking list, to the allocator core.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 mg-allocate |   24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/mg-allocate b/mg-allocate
index 4c32fa8..54a7af3 100755
--- a/mg-allocate
+++ b/mg-allocate
@@ -211,6 +211,9 @@ END
         $got_shareix= $candrow->{shareix};
         $ok=1; last;
     }
+    if (!$resq->rows) {
+	return (-1,undef);
+    }
     return ($ok, { Allocate => $allocate,
 		   Shareix => $got_shareix,
 		   Info => "$resname ($restype/$resname/$got_shareix)"
@@ -248,6 +251,8 @@ sub execute () {
             if (!$ok) {
                 logm("nothing available for $res, sorry");
                 $allok=0;
+            } elsif ($ok < 0) {
+		die "*** no candidates for $res! ***\n";
             } else {
                 logm("processed $res (shareix=$got->{Shareix})");
 		push @got, $got;
@@ -314,17 +319,19 @@ sub plan () {
 	logm("best at $planned->{Start} is ".showposs(\@reqlist));
 	die unless $planned;
 
-        my $allok=0;
+        my $worstok=0;
         if ($mayalloc && !$planned->{Start}) {
-            $allok=1;
+            $worstok=1;
 
             alloc_prep();
 
             foreach my $req (@reqlist) {
                 my ($ok, $got) = alloc_1res($req->{Ident});
+		$worstok = $ok if $ok < $worstok;
                 if (!$ok) {
                     logm("failed to allocate $req->{Ident}!");
-                    $allok=0;
+                } elsif ($ok < 0) {
+		    logm("*** no candidates for $req->{Ident}! ***");
                 } else {
                     $req->{GotShareix}= $got->{Shareix};
 		    push @got, $got;
@@ -332,9 +339,12 @@ sub plan () {
             }
         }
 
-        if ($allok) {
+        if ($worstok > 0) {
             logm("allocated, notifying...");
-        } else {
+        } elsif ($worstok < 0) {
+	    logm("*** problems, maybe missing resources? ***");
+	    return (0, undef);
+	} else {
             logm("booking...");
         }
 
@@ -346,7 +356,7 @@ sub plan () {
                 Start => $planned->{Start},
                 End => $planned->{Start} + $duration,
             };
-            if ($allok) {
+            if ($worstok > 0) {
                 $book->{Allocated}= {
                     Task => $tid,
                     Shareix => $req->{GotShareix},
@@ -355,7 +365,7 @@ sub plan () {
             push @bookings, $book;
         }
 
-        return ($allok, { Bookings => \@bookings });
+        return ($worstok, { Bookings => \@bookings });
     });
     loggot(@got);
 }
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [OSSTEST PATCH 9/9] mg-allocate: In planner mode, pre-check the arguments
  2015-12-17 17:06 [OSSTEST PATCH 1/9] README.dev: Document the blessings Ian Jackson
                   ` (6 preceding siblings ...)
  2015-12-17 17:06 ` [OSSTEST PATCH 8/9] mg-allocate: Better error handling when no candidates Ian Jackson
@ 2015-12-17 17:06 ` Ian Jackson
  2015-12-17 17:33   ` Ian Campbell
  2015-12-17 17:18 ` [OSSTEST PATCH 1/9] README.dev: Document the blessings Ian Campbell
  8 siblings, 1 reply; 31+ messages in thread
From: Ian Jackson @ 2015-12-17 17:06 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Jackson, Ian Campbell

Now, attempting to allocate a nonexistent host fails immediately with
a sensible message, rather than queueing up and then reporting the
message only later:

mariner:testing.git> OSSTEST_CONFIG=/u/iwj/.xen-osstest/config:local-config.test-database_iwj ./mg-allocate -U 1h spong
2015-12-17 17:05:14 Z pre-checking resources (dry run)...
2015-12-17 17:05:14 Z (precheck) task 196916 static iwj@mariner: iwj@mariner manual
*** no candidates for spong! ***
mariner:testing.git>

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 mg-allocate |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/mg-allocate b/mg-allocate
index 54a7af3..3730c23 100755
--- a/mg-allocate
+++ b/mg-allocate
@@ -238,7 +238,8 @@ sub loggot {
 	foreach @got;
 }
 
-sub execute () {
+sub execute (;$) {
+    my ($dryrun) = @_;
     my @got;
     db_retry($dbh_tests, \@all_lock_tables, sub {
 
@@ -259,10 +260,15 @@ sub execute () {
             }
         }
 
+	if ($dryrun) {
+	    db_retry_abort();
+	    return;
+	}
         if (!$allok) {
             die "allocation/deallocation unsuccessful\n";
         }
     });
+    return if $dryrun;
     loggot(@got);
     logm("done.");
 }
@@ -388,6 +394,11 @@ while (@ARGV && $ARGV[0] =~ m/^[-0-9]/) {
 }
 
 if ($duration) {
+    {
+	logm("pre-checking resources (dry run)...");
+	local $Osstest::TestSupport::logm_prefix = $logm_prefix.' (precheck)';
+	execute(1);
+    };
     plan();
 } else {
     execute();
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [OSSTEST PATCH 1/9] README.dev: Document the blessings
  2015-12-17 17:06 [OSSTEST PATCH 1/9] README.dev: Document the blessings Ian Jackson
                   ` (7 preceding siblings ...)
  2015-12-17 17:06 ` [OSSTEST PATCH 9/9] mg-allocate: In planner mode, pre-check the arguments Ian Jackson
@ 2015-12-17 17:18 ` Ian Campbell
  2015-12-17 17:59   ` Ian Jackson
  8 siblings, 1 reply; 31+ messages in thread
From: Ian Campbell @ 2015-12-17 17:18 UTC (permalink / raw)
  To: Ian Jackson, xen-devel

On Thu, 2015-12-17 at 17:06 +0000, Ian Jackson wrote:
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  README.dev |  101
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 101 insertions(+)
> 
> diff --git a/README.dev b/README.dev
> index 65ec111..879af60 100644
> --- a/README.dev
> +++ b/README.dev
> @@ -232,3 +232,104 @@ probably are after a reboot) by looking for
> processes relating to the
>  lists flight,job combinations and then:
>  
>  $ ./mg-hosts previoustasks --clear
> +
> +
> +Flight blessings and the blessed-* host flags
> +=============================================
> +
> +Both flights and hosts have a `blessing'.
> +
> +Blessings are used:
> +
> + * To avoid accidentally tools operating on flights which are in the
> +   wrong phase of their existence.
> +
> + * To control which automatic archaeologists will look at which
> +   flights.  (Archeaology tools take arguments to control which

                 Archaeology

> +   blessings they will consider.)
> +
> + * To control which hosts are considered suitable for use with which
> +   flight.  (Flights with blessing `foo' use only hosts which have
> +   flag `blessed-foo'.)
> +
> +Each flight has a `blessing' and an `intended blessing'.  The
> +`intended blessing' is what the flight is going to be blessed as when
> +its execution has completed.  The intended blessing controls host
> +allocation.

I think this could also usefully mention that `blessing' is the current
blessing, which may go through several phases during the life of the
flight, hopefully culminating in being set to `intended blessing' if the
flight is successful and that the archaeology tools only look at `blessing'
and never `intended blessing'

> +(Normally the `intended blessing' is the same as the bless argument to
> +sg-execute-flight aka the -B argument to mg-execute-flight.)

How does this interact with the intended blessing passed to cs-flight-
create?

> + * `real-bisect' and `adhoc-bisect': These are found only as the
> +   blessing of finished flights.  (This is achieved by passing *-adhoc
> +   to sg-execute-flight.)  This allows the archaeologist tools to
> +   distinguish full flights from bisection steps.

I don't follow the reference to *-adhoc here, since it matches neither
`real-bisect' nor `adhoc-bisect'.

> +There is a special exception to the tools' flight status checks: any
> +flight whose blessing contains `play' can be operated on out of order.
> +
> +Flights blessings can be manually changed with cs-flight-bless.  Eg
> +  ./cs-flight-bless FLIGHT broken-real real
> +updates FLIGHT to be marked `broken' rather than `real'.  This can be

The example says `broken-real' but the text says `broken' (if the text
didn't quote the `broken' it probably wouldn't matter.

Maybe mention the need to pass the current blessing as a safety catch,
since it is part of the example command?

Ian.

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [OSSTEST PATCH 2/9] mg-schema-test-database: Provide some timeouts which are better for testing
  2015-12-17 17:06 ` [OSSTEST PATCH 2/9] mg-schema-test-database: Provide some timeouts which are better for testing Ian Jackson
@ 2015-12-17 17:19   ` Ian Campbell
  0 siblings, 0 replies; 31+ messages in thread
From: Ian Campbell @ 2015-12-17 17:19 UTC (permalink / raw)
  To: Ian Jackson, xen-devel

On Thu, 2015-12-17 at 17:06 +0000, Ian Jackson wrote:
> The default timeouts mean that after starting a test db queue daemon
> and a test db allocation attempt, we have to wait two minutes.
> 
> Lower timeouts increase the risk that we might lose noncritical races
> and allocate resources to the `wrong' tasks.  And they reduce the
> duration of an outage which will cause a planned allocation attempt to
> fail.
> 
> I think we don't care about those problems for test instances.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

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


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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [OSSTEST PATCH 3/9] mg-schema-test-database: Wipe previous local plan data
  2015-12-17 17:06 ` [OSSTEST PATCH 3/9] mg-schema-test-database: Wipe previous local plan data Ian Jackson
@ 2015-12-17 17:22   ` Ian Campbell
  2015-12-17 17:37     ` Ian Jackson
  0 siblings, 1 reply; 31+ messages in thread
From: Ian Campbell @ 2015-12-17 17:22 UTC (permalink / raw)
  To: Ian Jackson, xen-devel

On Thu, 2015-12-17 at 17:06 +0000, Ian Jackson wrote:
> Whatever is in the user's cwd is unlikely to correspond to anything
> real.  In principle it might be possible to obtain an official copy
> from the real daemons, and massage it, or something, but that's a lot
> of work.
> 
> Instead, just remove it when we start the test db daemons.
> 
> In principle it would be more correct to remove it when we set up the
> test db, because it is at that point that we create the new view of
> the world.  Removing the old plan data when we start daemons means
> that if the user, who is testing, restarts the daemons, the
> newly-created queue daemon does not have information about allocations
> made with the previous daemon, and instead regards those allocations
> as rogue.
> 
> However, removing the file only when the daemons are started means
> that if the user has saved a data-plan.pl in their cwd for some other
> reason we don't remove it unless the user is actually going to run the
> daemons.  So I think this is preferable.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

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

Would it make sense to remove data-projection.pl too? I suppose it isn't
consumed anywhere so is more or less harmless.

OOI do these test mode daemons do anything sensible with resource-
{plan,projection}.html and summary.html? Looks like they would clobber
things in $c{WebspaceFile}, which might be surprising to a user with two
test instances on the go. But that's a pretty obscure corner case I
suspect...

> ---
>  mg-schema-test-database |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mg-schema-test-database b/mg-schema-test-database
> index 818cf89..a4cb732 100755
> --- a/mg-schema-test-database
> +++ b/mg-schema-test-database
> @@ -553,6 +553,8 @@ daemons)
>  	withtest \
>  	exec_resetting_sigint ./ms-queuedaemon &
>  
> +	rm -f data-plan.pl
> +
>  	wait
>  
>  	;;

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [OSSTEST PATCH 4/9] mg-schema-test-database: Borrow shares properly
  2015-12-17 17:06 ` [OSSTEST PATCH 4/9] mg-schema-test-database: Borrow shares properly Ian Jackson
@ 2015-12-17 17:26   ` Ian Campbell
  2015-12-17 17:43     ` Ian Jackson
  0 siblings, 1 reply; 31+ messages in thread
From: Ian Campbell @ 2015-12-17 17:26 UTC (permalink / raw)
  To: Ian Jackson, xen-devel

On Thu, 2015-12-17 at 17:06 +0000, Ian Jackson wrote:
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  mg-schema-test-database |   14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/mg-schema-test-database b/mg-schema-test-database
> index a4cb732..4e0ee68 100755
> --- a/mg-schema-test-database
> +++ b/mg-schema-test-database
> @@ -448,12 +448,22 @@ END
>  	done
>  
>  	# As we copy, we note everything we're not borrowing as
> -	# belonging to the parent db.
> +	# belonging to the parent db.  We borrow shares of a shared
> +	# resource.  If we borrow only some rather than all of the
> +	# shares, neither DB will be able to unshare it.

This is what actually happens, rather than this comment explaining a thing
we are avoiding, right?

I ask because although the text (and the use of "properly" in the subject)
seems pretty clear it's the former I was surprised to find this code was
borrowing partial shares and therefore setting up such a situation. Given
the caveat below would it not be better to just never allow this? I suppose
that given the list of tasks to borrow came from the user this might be
considered a "keep both pieces" situation.

> +
> +	# In principle it might be possible to actually use different
> +	# shares of the same resource with different dbs.  However the
> +	# `sharetype' contains the osstest revision, which prevents
> +	# sharing between test and real versions of osstest code.
> +
>  	cat >>$t.import <<END
>  		$(make_xdbref_task $maindbname 'not borrowed' '' PARENT)
>  		UPDATE resources
>  			SET owntaskid = $(taskid xdbref $maindbname)
> -			WHERE owntaskid != $(borrowtaskid $task);
> +			WHERE owntaskid != $(borrowtaskid $task)
> +			  AND owntaskid != $(taskid magic shared)
> +			  AND owntaskid != $(taskid magic preparing);
>  		COMMIT;
>  END
>  

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [OSSTEST PATCH 5/9] ms-planner: Improve an error message
  2015-12-17 17:06 ` [OSSTEST PATCH 5/9] ms-planner: Improve an error message Ian Jackson
@ 2015-12-17 17:26   ` Ian Campbell
  0 siblings, 0 replies; 31+ messages in thread
From: Ian Campbell @ 2015-12-17 17:26 UTC (permalink / raw)
  To: Ian Jackson, xen-devel

On Thu, 2015-12-17 at 17:06 +0000, Ian Jackson wrote:
> I experienced this `die' due to mg-schema-test-database failing to
> borrow shared hosts properly, and added this Dumper for debugging.
> 
> I have not bothered to improve any of the other quite terse `die's in
> ms-planner.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [OSSTEST PATCH 6/9] db_retry: Suppress an "exiting via last" warning
  2015-12-17 17:06 ` [OSSTEST PATCH 6/9] db_retry: Suppress an "exiting via last" warning Ian Jackson
@ 2015-12-17 17:31   ` Ian Campbell
  2015-12-17 17:48     ` Ian Jackson
  0 siblings, 1 reply; 31+ messages in thread
From: Ian Campbell @ 2015-12-17 17:31 UTC (permalink / raw)
  To: Ian Jackson, xen-devel

On Thu, 2015-12-17 at 17:06 +0000, Ian Jackson wrote:
> This warning appears when db_retry_abort is used, since 2b069b6c
> "Database locking: Perl: Retry all deadlocks in PostgreSQL".

Is the reason for not turning this into a return related to the fact this
is within an eval, not a proper sub? But then I'm not really sure why it is
warning (I suppose eval has some sub like properties?).

Related to https://rt.perl.org/Public/Bug/Display.html?id=29238 perhaps.

> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  Osstest.pm |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Osstest.pm b/Osstest.pm
> index c77d960..a398b1e 100644
> --- a/Osstest.pm
> +++ b/Osstest.pm
> @@ -299,6 +299,7 @@ sub db_retry ($$$;$$) {
>  	    $r= &$body;
>  	    if ($db_retry_stop) {
>  		$dbh->rollback();
> +		no warnings qw(exiting);
>  		last if $db_retry_stop eq 'abort';
>  		next;
>  	    }

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [OSSTEST PATCH 7/9] Executive DB retry: Avoid an undefined warning
  2015-12-17 17:06 ` [OSSTEST PATCH 7/9] Executive DB retry: Avoid an undefined warning Ian Jackson
@ 2015-12-17 17:31   ` Ian Campbell
  0 siblings, 0 replies; 31+ messages in thread
From: Ian Campbell @ 2015-12-17 17:31 UTC (permalink / raw)
  To: Ian Jackson, xen-devel

On Thu, 2015-12-17 at 17:06 +0000, Ian Jackson wrote:
> If something other than the DB statements inside need_retry throws an
> exception, ->err will normally be undef (because
> $dbh_tests->begin_work will clear it, if nothing else).
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

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

> ---
>  Osstest/JobDB/Executive.pm |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Osstest/JobDB/Executive.pm b/Osstest/JobDB/Executive.pm
> index 6fb77a4..c0af21c 100644
> --- a/Osstest/JobDB/Executive.pm
> +++ b/Osstest/JobDB/Executive.pm
> @@ -50,7 +50,7 @@ sub begin_work ($$$) { #method
>  sub need_retry ($$$) {
>      my ($jd, $dbh,$committing) = @_;
>      return
> -	$dbh_tests->err()==7 &&
> +	($dbh_tests->err() // 0)==7 &&
>  	($dbh_tests->state =~ m/^40P01/); # DEADLOCK DETECTED
>  }
>  

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [OSSTEST PATCH 8/9] mg-allocate: Better error handling when no candidates
  2015-12-17 17:06 ` [OSSTEST PATCH 8/9] mg-allocate: Better error handling when no candidates Ian Jackson
@ 2015-12-17 17:33   ` Ian Campbell
  0 siblings, 0 replies; 31+ messages in thread
From: Ian Campbell @ 2015-12-17 17:33 UTC (permalink / raw)
  To: Ian Jackson, xen-devel

On Thu, 2015-12-17 at 17:06 +0000, Ian Jackson wrote:
> Spot when our db search revealed no candidates for the resources to
> allocate, and:
>  - when doing an immediate allocation, call it an error
>  - when doing a planned allocation, cause it to prevent allocation
>    on this iteration, and print a suitably unreassuring message
> 
> Previously it would simply say `nothing available'.
> 
> Implement this as follows:
>  - Report lack of candidates as $ok=-1 from alloc_1rescand
>  - In alloc_1res, return this -1 as with any non-zero $ok
>  - Handle the new $ok at all the call sites, in particular
>  - In plan(), rename `allok' to `worstok' and have it be
>    the worst relevant $ok value.  If $ok gives -1, return
>    undef, rather than a booking list, to the allocator core.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

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


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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [OSSTEST PATCH 9/9] mg-allocate: In planner mode, pre-check the arguments
  2015-12-17 17:06 ` [OSSTEST PATCH 9/9] mg-allocate: In planner mode, pre-check the arguments Ian Jackson
@ 2015-12-17 17:33   ` Ian Campbell
  0 siblings, 0 replies; 31+ messages in thread
From: Ian Campbell @ 2015-12-17 17:33 UTC (permalink / raw)
  To: Ian Jackson, xen-devel

On Thu, 2015-12-17 at 17:06 +0000, Ian Jackson wrote:
> Now, attempting to allocate a nonexistent host fails immediately with
> a sensible message, rather than queueing up and then reporting the
> message only later:
> 
> mariner:testing.git> OSSTEST_CONFIG=/u/iwj/.xen-osstest/config:local-
> config.test-database_iwj ./mg-allocate -U 1h spong
> 2015-12-17 17:05:14 Z pre-checking resources (dry run)...
> 2015-12-17 17:05:14 Z (precheck) task 196916 static iwj@mariner: iwj@mari
> ner manual
> *** no candidates for spong! ***
> mariner:testing.git>
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [OSSTEST PATCH 3/9] mg-schema-test-database: Wipe previous local plan data
  2015-12-17 17:22   ` Ian Campbell
@ 2015-12-17 17:37     ` Ian Jackson
  2015-12-17 17:42       ` Ian Campbell
  0 siblings, 1 reply; 31+ messages in thread
From: Ian Jackson @ 2015-12-17 17:37 UTC (permalink / raw)
  To: Ian Campbell; +Cc: xen-devel

Ian Campbell writes ("Re: [OSSTEST PATCH 3/9] mg-schema-test-database: Wipe previous local plan data"):
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Thanks.

> Would it make sense to remove data-projection.pl too? I suppose it isn't
> consumed anywhere so is more or less harmless.

Precisely.

> OOI do these test mode daemons do anything sensible with resource-
> {plan,projection}.html and summary.html? Looks like they would clobber
> things in $c{WebspaceFile}, which might be surprising to a user with two
> test instances on the go. But that's a pretty obscure corner case I
> suspect...

If you don't run it as the same user, this is not a problem, because
the default WebspaceFile has $HOME in it.  I guess it might be
possible to have something check for this.

I would like to leave this for another day.  I don't see us running
test instances as the osstest user.

Ian.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [OSSTEST PATCH 3/9] mg-schema-test-database: Wipe previous local plan data
  2015-12-17 17:37     ` Ian Jackson
@ 2015-12-17 17:42       ` Ian Campbell
  2015-12-17 17:50         ` Ian Jackson
  0 siblings, 1 reply; 31+ messages in thread
From: Ian Campbell @ 2015-12-17 17:42 UTC (permalink / raw)
  To: Ian Jackson; +Cc: xen-devel

On Thu, 2015-12-17 at 17:37 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [OSSTEST PATCH 3/9] mg-schema-test-database:
> Wipe previous local plan data"):
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Thanks.
> 
> > Would it make sense to remove data-projection.pl too? I suppose it
> > isn't
> > consumed anywhere so is more or less harmless.
> 
> Precisely.
> 
> > OOI do these test mode daemons do anything sensible with resource-
> > {plan,projection}.html and summary.html? Looks like they would clobber
> > things in $c{WebspaceFile}, which might be surprising to a user with
> > two
> > test instances on the go. But that's a pretty obscure corner case I
> > suspect...
> 
> If you don't run it as the same user, this is not a problem, because
> the default WebspaceFile has $HOME in it.

Right, I should have been clearer, I meant clobber things in the user's
WebspaceFile if they run two test instance at the same time.

>   I guess it might be
> possible to have something check for this.
> 
> I would like to leave this for another day.  I don't see us running
> test instances as the osstest user.

In any case another day is fine, it's pretty obscure for sure.

Ian.


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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [OSSTEST PATCH 4/9] mg-schema-test-database: Borrow shares properly
  2015-12-17 17:26   ` Ian Campbell
@ 2015-12-17 17:43     ` Ian Jackson
  2015-12-17 18:08       ` Ian Campbell
  0 siblings, 1 reply; 31+ messages in thread
From: Ian Jackson @ 2015-12-17 17:43 UTC (permalink / raw)
  To: Ian Campbell; +Cc: xen-devel

Ian Campbell writes ("Re: [OSSTEST PATCH 4/9] mg-schema-test-database: Borrow shares properly"):
> On Thu, 2015-12-17 at 17:06 +0000, Ian Jackson wrote:
> >  	# As we copy, we note everything we're not borrowing as
> > -	# belonging to the parent db.
> > +	# belonging to the parent db.  We borrow shares of a shared
> > +	# resource.  If we borrow only some rather than all of the
> > +	# shares, neither DB will be able to unshare it.
> 
> This is what actually happens, rather than this comment explaining a thing
> we are avoiding, right?

Yes.

> I ask because although the text (and the use of "properly" in the subject)
> seems pretty clear it's the former I was surprised to find this code was
> borrowing partial shares and therefore setting up such a situation. Given
> the caveat below would it not be better to just never allow this?

I have added this to the commit message:

 Previously, the test database would be generated in a broken state:
 resources share-host/foo/{1,2,...} exist but the resource host/foo/0
 is allocated to magic/xdbref rather than to magic/shared.  This causes
 various resource allocation machinery to crash.  (Even if the host is
 entirely un-borrowed.)

I guess it might be possible for mg-schema-test-database to check that
either none or all of the shares of a host are being borrowed, but I
don't think it's worth it.  The osstest git hash in the sharetype will
in practice mostly prevent undesirable sharing of the same resource
between test and real db instances.

> I suppose that given the list of tasks to borrow came from the user
> this might be considered a "keep both pieces" situation.

Yes.  This partial sharing can only affect you if you have allocated
individual shares to the task you say you want to borrow from.

Ian.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [OSSTEST PATCH 6/9] db_retry: Suppress an "exiting via last" warning
  2015-12-17 17:31   ` Ian Campbell
@ 2015-12-17 17:48     ` Ian Jackson
  2015-12-17 18:10       ` Ian Campbell
  0 siblings, 1 reply; 31+ messages in thread
From: Ian Jackson @ 2015-12-17 17:48 UTC (permalink / raw)
  To: Ian Campbell; +Cc: xen-devel

Ian Campbell writes ("Re: [OSSTEST PATCH 6/9] db_retry: Suppress an "exiting via last" warning"):
> On Thu, 2015-12-17 at 17:06 +0000, Ian Jackson wrote:
> > This warning appears when db_retry_abort is used, since 2b069b6c
> > "Database locking: Perl: Retry all deadlocks in PostgreSQL".
> 
> Is the reason for not turning this into a return related to the fact this
> is within an eval, not a proper sub? But then I'm not really sure why it is
> warning (I suppose eval has some sub like properties?).

I think it's a warning because eval does not trap `last's (nor
`return's).  This means that if you think `eval' can be used to make
try-finally, you will be disappointed.

(Unlike, in Tcl, say, where `break' and `continue' and indeed `return'
are implemented as exceptions.)

Why would `return' be better ?  It would still generate the same
warning.  Leaving the loop via `last' seems less heavyweight than
using `return' and less likely to produce bugs in future patches whose
authors don't spot the non-local exit.

> Related to https://rt.perl.org/Public/Bug/Display.html?id=29238 perhaps.

I don't think so, although there seems to be a fair amount of
confusion there.

Ian.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [OSSTEST PATCH 3/9] mg-schema-test-database: Wipe previous local plan data
  2015-12-17 17:42       ` Ian Campbell
@ 2015-12-17 17:50         ` Ian Jackson
  2015-12-17 18:11           ` Ian Campbell
  0 siblings, 1 reply; 31+ messages in thread
From: Ian Jackson @ 2015-12-17 17:50 UTC (permalink / raw)
  To: Ian Campbell; +Cc: xen-devel

Ian Campbell writes ("Re: [OSSTEST PATCH 3/9] mg-schema-test-database: Wipe previous local plan data"):
> On Thu, 2015-12-17 at 17:37 +0000, Ian Jackson wrote:
> > If you don't run it as the same user, this is not a problem, because
> > the default WebspaceFile has $HOME in it.
> 
> Right, I should have been clearer, I meant clobber things in the user's
> WebspaceFile if they run two test instance at the same time.

Ah, right.  I think the first person who wants to do that can do
something to separate out the WebspaceFiles :-).  It's not likely that
you wouldn't notice that you had a problem.

Ian.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [OSSTEST PATCH 1/9] README.dev: Document the blessings
  2015-12-17 17:18 ` [OSSTEST PATCH 1/9] README.dev: Document the blessings Ian Campbell
@ 2015-12-17 17:59   ` Ian Jackson
  2015-12-17 18:12     ` Ian Campbell
  0 siblings, 1 reply; 31+ messages in thread
From: Ian Jackson @ 2015-12-17 17:59 UTC (permalink / raw)
  To: Ian Campbell; +Cc: xen-devel, Ian Jackson

Ian Campbell writes ("Re: [OSSTEST PATCH 1/9] README.dev: Document the blessings"):
> On Thu, 2015-12-17 at 17:06 +0000, Ian Jackson wrote:
> > +Each flight has a `blessing' and an `intended blessing'.  The
> > +`intended blessing' is what the flight is going to be blessed as when
> > +its execution has completed.  The intended blessing controls host
> > +allocation.
> 
> I think this could also usefully mention that `blessing' is the current
> blessing, which may go through several phases during the life of the
> flight, hopefully culminating in being set to `intended blessing' if the
> flight is successful and that the archaeology tools only look at `blessing'
> and never `intended blessing'

Yes.

> > +(Normally the `intended blessing' is the same as the bless argument to
> > +sg-execute-flight aka the -B argument to mg-execute-flight.)
> 
> How does this interact with the intended blessing passed to cs-flight-
> create?

So actually there a flight has three blessings.

> > + * `real-bisect' and `adhoc-bisect': These are found only as the
> > +   blessing of finished flights.  (This is achieved by passing *-adhoc
> > +   to sg-execute-flight.)  This allows the archaeologist tools to
> > +   distinguish full flights from bisection steps.
> 
> I don't follow the reference to *-adhoc here, since it matches neither
> `real-bisect' nor `adhoc-bisect'.

`Passing *-adhoc' should read `Passing *-bisect'.

> > +There is a special exception to the tools' flight status checks: any
> > +flight whose blessing contains `play' can be operated on out of order.
> > +
> > +Flights blessings can be manually changed with cs-flight-bless.  Eg
> > +  ./cs-flight-bless FLIGHT broken-real real
> > +updates FLIGHT to be marked `broken' rather than `real'.  This can be
> 
> The example says `broken-real' but the text says `broken' (if the text
> didn't quote the `broken' it probably wouldn't matter.

I have clarified this.

> Maybe mention the need to pass the current blessing as a safety catch,
> since it is part of the example command?

Yes.

squash! patch and complete v2, below.

Ian.

>From 73903500006c0451cc81697872096fe2d6fe02ab Mon Sep 17 00:00:00 2001
From: Ian Jackson <ian.jackson@eu.citrix.com>
Date: Thu, 17 Dec 2015 17:58:41 +0000
Subject: [OSSTEST PATCH] squash! README.dev: Document the blessings

v2: Improvements from review
---
 README.dev |   39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/README.dev b/README.dev
index 879af60..351cd25 100644
--- a/README.dev
+++ b/README.dev
@@ -252,13 +252,17 @@ Blessings are used:
    flight.  (Flights with blessing `foo' use only hosts which have
    flag `blessed-foo'.)
 
-Each flight has a `blessing' and an `intended blessing'.  The
-`intended blessing' is what the flight is going to be blessed as when
-its execution has completed.  The intended blessing controls host
-allocation.
-
-(Normally the `intended blessing' is the same as the bless argument to
-sg-execute-flight aka the -B argument to mg-execute-flight.)
+Each flight has a `blessing' (flights.blessing in the DB) and an
+`intended blessing' (flights.intended).  The `blessing' changes as the
+flight goes through the stages of its lifecycle.  The `intended
+blessing' is roughly what the flight is going to be blessed as when
+its execution has completed, and controls host allocation.
+
+There is also the blessing which the flight will actually get when it
+is finished, which is not represented in the DB; rather, it is the
+blessing argument to sg-execute-flight (aka the -B argument to
+mg-execute-flight).  Normally this is the same as the intended
+blessing in the DB, except for bisection flights.
 
 These are the principal (intended) blessings:
 
@@ -297,9 +301,9 @@ These are the principal (intended) blessings:
    when the hosts are ready.
 
  * `real-bisect' and `adhoc-bisect': These are found only as the
-   blessing of finished flights.  (This is achieved by passing *-adhoc
-   to sg-execute-flight.)  This allows the archaeologist tools to
-   distinguish full flights from bisection steps.
+   blessing of finished flights.  (This is achieved by passing
+   *-bisect to sg-execute-flight.)  This allows the archaeologist
+   tools to distinguish full flights from bisection steps.
 
    The corresponding intended blessing (as found in the `intended'
    column of the flights table) is `real'.  So the hosts used by the
@@ -320,6 +324,16 @@ There are also blessings for unfinished flights:
 
  * `broken': Something went catastrophically wrong.
 
+ * `broken-*': Conventionally set by hand when a flight needs to be
+   `un-blessed', perhaps because it contains data misleading to the
+   archaeologists.
+
+   A flight's blessings can be manually changed with cs-flight-bless:
+      ./cs-flight-bless FLIGHT broken-real real
+   updates FLIGHT to be marked `broken' rather than `real'.
+   (cs-flight-bless matches the existing blessing against its final
+   argument, to avoid accidents.)
+
 Note that osstest is generally `crash-only software': nothing will
 `clean up' a crashed flight and set its blessing to `broken'.  Flights
 which were in progress once upon a time but never completed, because
@@ -328,8 +342,3 @@ time.
 
 There is a special exception to the tools' flight status checks: any
 flight whose blessing contains `play' can be operated on out of order.
-
-Flights blessings can be manually changed with cs-flight-bless.  Eg
-  ./cs-flight-bless FLIGHT broken-real real
-updates FLIGHT to be marked `broken' rather than `real'.  This can be
-useful if a flight's data is misleading to the archaeologists.
-- 
1.7.10.4



>From d677a8bfe576b44d28df4d5d62cbe9dec9fd5578 Mon Sep 17 00:00:00 2001
From: Ian Jackson <ian.jackson@eu.citrix.com>
Date: Thu, 17 Dec 2015 16:51:17 +0000
Subject: [OSSTEST PATCH] README.dev: Document the blessings

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v2: Improvements from review
---
 README.dev |  110 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 110 insertions(+)

diff --git a/README.dev b/README.dev
index 65ec111..351cd25 100644
--- a/README.dev
+++ b/README.dev
@@ -232,3 +232,113 @@ probably are after a reboot) by looking for processes relating to the
 lists flight,job combinations and then:
 
 $ ./mg-hosts previoustasks --clear
+
+
+Flight blessings and the blessed-* host flags
+=============================================
+
+Both flights and hosts have a `blessing'.
+
+Blessings are used:
+
+ * To avoid accidentally tools operating on flights which are in the
+   wrong phase of their existence.
+
+ * To control which automatic archaeologists will look at which
+   flights.  (Archeaology tools take arguments to control which
+   blessings they will consider.)
+
+ * To control which hosts are considered suitable for use with which
+   flight.  (Flights with blessing `foo' use only hosts which have
+   flag `blessed-foo'.)
+
+Each flight has a `blessing' (flights.blessing in the DB) and an
+`intended blessing' (flights.intended).  The `blessing' changes as the
+flight goes through the stages of its lifecycle.  The `intended
+blessing' is roughly what the flight is going to be blessed as when
+its execution has completed, and controls host allocation.
+
+There is also the blessing which the flight will actually get when it
+is finished, which is not represented in the DB; rather, it is the
+blessing argument to sg-execute-flight (aka the -B argument to
+mg-execute-flight).  Normally this is the same as the intended
+blessing in the DB, except for bisection flights.
+
+These are the principal (intended) blessings:
+
+ * `real': Flights which are fully production runs.  They use only
+   clean working trees with no `funny' environment variables, and
+   versions of osstest intended for production.  The data from such
+   flights is used, where applicable, to control production push gate
+   decisions, etc.
+
+   `blessed-real' hosts are fully production-ready.
+
+ * `play': Playground.  Flights may use weird software, dirty working
+   trees, strange environment variables, and so on.  It does not make
+   sense to point archaeology tools at such flights.
+
+   `blessed-play' hosts may be partially or completely nonfunctional,
+   or strange in some way.  For this reason `play' flights should not
+   normally use the automatic host allocator.
+
+ * `adhoc': Special-purpose flights.  They should be run with software
+   nearly as clean as `real': the version of the software used may not
+   be a branch intended for-production, but any enhancements or
+   modifications should not leave false or misleading information in
+   the history database.
+
+   `blessed-adhoc' hosts are just as good as `blessed-real' ones.
+
+   The `adhoc' blessing exists mostly to protect the main production
+   workflow from unintentional violations (for example, unexpectedly
+   buggy historical flight data).
+
+ * `commission-<something>': Flights used for testing that a
+   particular subset of the hosts are working properly.  The hosts to
+   be tested should be blessed `commission-whatever' during
+   commissioning, and that blessing removed and replaced with `real'
+   when the hosts are ready.
+
+ * `real-bisect' and `adhoc-bisect': These are found only as the
+   blessing of finished flights.  (This is achieved by passing
+   *-bisect to sg-execute-flight.)  This allows the archaeologist
+   tools to distinguish full flights from bisection steps.
+
+   The corresponding intended blessing (as found in the `intended'
+   column of the flights table) is `real'.  So the hosts used by the
+   automatic host allocator are those flagged `blessed-real' or
+   `blessed-adhoc' - there are no separate blessed-*-bisect hostflags.
+
+There are also blessings for unfinished flights:
+
+ * `constructing': The flight metadata (jobs, runvars, etc.) is still
+   being populated.  The tools for flight construction (and flight
+   construct editing) generally insist that they operate only on
+   flights in this state.
+
+ * `running': At least one of the jobs in this flight has started.
+   Flight execution software will generally (perhaps via standard
+   library calls) mark a flight `running' if they find it
+   `constructing', and refuse to operate on other flights.
+
+ * `broken': Something went catastrophically wrong.
+
+ * `broken-*': Conventionally set by hand when a flight needs to be
+   `un-blessed', perhaps because it contains data misleading to the
+   archaeologists.
+
+   A flight's blessings can be manually changed with cs-flight-bless:
+      ./cs-flight-bless FLIGHT broken-real real
+   updates FLIGHT to be marked `broken' rather than `real'.
+   (cs-flight-bless matches the existing blessing against its final
+   argument, to avoid accidents.)
+
+Note that osstest is generally `crash-only software': nothing will
+`clean up' a crashed flight and set its blessing to `broken'.  Flights
+which were in progress once upon a time but never completed, because
+they crashed, are simply left with whatever blessing they had at the
+time.
+
+There is a special exception to the tools' flight status checks: any
+flight whose blessing contains `play' can be operated on out of order.
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [OSSTEST PATCH 4/9] mg-schema-test-database: Borrow shares properly
  2015-12-17 17:43     ` Ian Jackson
@ 2015-12-17 18:08       ` Ian Campbell
  0 siblings, 0 replies; 31+ messages in thread
From: Ian Campbell @ 2015-12-17 18:08 UTC (permalink / raw)
  To: Ian Jackson; +Cc: xen-devel

On Thu, 2015-12-17 at 17:43 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [OSSTEST PATCH 4/9] mg-schema-test
> -database: Borrow shares properly"):
> > On Thu, 2015-12-17 at 17:06 +0000, Ian Jackson wrote:
> > >  	# As we copy, we note everything we're not borrowing as
> > > -	# belonging to the parent db.
> > > +	# belonging to the parent db.  We borrow shares of a
> shared
> > > +	# resource.  If we borrow only some rather than all of
> the
> > > +	# shares, neither DB will be able to unshare it.
> > 
> > This is what actually happens, rather than this comment explaining
> a thing
> > we are avoiding, right?
> 
> Yes.
> 
> > I ask because although the text (and the use of "properly" in the subject)
> > seems pretty clear it's the former I was surprised to find this code was
> > borrowing partial shares and therefore setting up such a situation. Given
> > the caveat below would it not be better to just never allow this?
> 
> I have added this to the commit message:
> 
>  Previously, the test database would be generated in a broken state:
>  resources share-host/foo/{1,2,...} exist but the resource host/foo/0
>  is allocated to magic/xdbref rather than to magic/shared.  This causes
>  various resource allocation machinery to crash.  (Even if the host is
>  entirely un-borrowed.)
> 
> I guess it might be possible for mg-schema-test-database to check that
> either none or all of the shares of a host are being borrowed, but I
> don't think it's worth it.  The osstest git hash in the sharetype will
> in practice mostly prevent undesirable sharing of the same resource
> between test and real db instances.
> 
> > I suppose that given the list of tasks to borrow came from the user
> > this might be considered a "keep both pieces" situation.
> 
> Yes.  This partial sharing can only affect you if you have allocated
> individual shares to the task you say you want to borrow from.

Grand, with the extra commit log bit from above:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [OSSTEST PATCH 6/9] db_retry: Suppress an "exiting via last" warning
  2015-12-17 17:48     ` Ian Jackson
@ 2015-12-17 18:10       ` Ian Campbell
  2015-12-17 18:38         ` Ian Jackson
  0 siblings, 1 reply; 31+ messages in thread
From: Ian Campbell @ 2015-12-17 18:10 UTC (permalink / raw)
  To: Ian Jackson; +Cc: xen-devel

On Thu, 2015-12-17 at 17:48 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [OSSTEST PATCH 6/9] db_retry: Suppress an
> "exiting via last" warning"):
> > On Thu, 2015-12-17 at 17:06 +0000, Ian Jackson wrote:
> > > This warning appears when db_retry_abort is used, since 2b069b6c
> > > "Database locking: Perl: Retry all deadlocks in PostgreSQL".
> > 
> > Is the reason for not turning this into a return related to the
> fact this
> > is within an eval, not a proper sub? But then I'm not really sure
> why it is
> > warning (I suppose eval has some sub like properties?).
> 
> I think it's a warning because eval does not trap `last's (nor
> `return's).  This means that if you think `eval' can be used to make
> try-finally, you will be disappointed.
> 
> (Unlike, in Tcl, say, where `break' and `continue' and indeed 
> `return
> are implemented as exceptions.)
> 
> Why would `return' be better ?  It would still generate the same
> warning.  Leaving the loop via `last' seems less heavyweight than
> using `return' and less likely to produce bugs in future patches whose
> authors don't spot the non-local exit.

I think I'm probably confused about the scope of the eval vs
{last,return} etc. In any case, that's no reason to block this patch
AFAICT: Acked-by: Ian Campbell <ian.campbell@citrix.com>

Ian.

> 
> > Related to 
> https://rt.perl.org/Public/Bug/Display.html?id=29238 perhaps.
> 
> I don't think so, although there seems to be a fair amount of
> confusion there.
> 
> Ian.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [OSSTEST PATCH 3/9] mg-schema-test-database: Wipe previous local plan data
  2015-12-17 17:50         ` Ian Jackson
@ 2015-12-17 18:11           ` Ian Campbell
  0 siblings, 0 replies; 31+ messages in thread
From: Ian Campbell @ 2015-12-17 18:11 UTC (permalink / raw)
  To: Ian Jackson; +Cc: xen-devel

On Thu, 2015-12-17 at 17:50 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [OSSTEST PATCH 3/9] mg-schema-test
> -database: Wipe previous local plan data"):
> > On Thu, 2015-12-17 at 17:37 +0000, Ian Jackson wrote:
> > > If you don't run it as the same user, this is not a problem,
> because
> > > the default WebspaceFile has $HOME in it.
> > 
> > Right, I should have been clearer, I meant clobber things in the
> user's
> > WebspaceFile if they run two test instance at the same time.
> 
> Ah, right.  I think the first person who wants to do that can do
> something to separate out the WebspaceFiles :-).  It's not likely
> that
> you wouldn't notice that you had a problem.

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [OSSTEST PATCH 1/9] README.dev: Document the blessings
  2015-12-17 17:59   ` Ian Jackson
@ 2015-12-17 18:12     ` Ian Campbell
  0 siblings, 0 replies; 31+ messages in thread
From: Ian Campbell @ 2015-12-17 18:12 UTC (permalink / raw)
  To: Ian Jackson; +Cc: xen-devel

On Thu, 2015-12-17 at 17:59 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [OSSTEST PATCH 1/9] README.dev: Document
> the blessings"):
> > On Thu, 2015-12-17 at 17:06 +0000, Ian Jackson wrote:
> > > +Each flight has a `blessing' and an `intended blessing'.  The
> > > +`intended blessing' is what the flight is going to be blessed as
> when
> > > +its execution has completed.  The intended blessing controls
> host
> > > +allocation.
> > 
> > I think this could also usefully mention that `blessing' is the
> current
> > blessing, which may go through several phases during the life of
> the
> > flight, hopefully culminating in being set to `intended blessing'
> if the
> > flight is successful and that the archaeology tools only look at
> `blessing'
> > and never `intended blessing'
> 
> Yes.
> 
> > > +(Normally the `intended blessing' is the same as the bless
> argument to
> > > +sg-execute-flight aka the -B argument to mg-execute-flight.)
> > 
> > How does this interact with the intended blessing passed to cs
> -flight-
> > create?
> 
> So actually there a flight has three blessings.
> 
> > > + * `real-bisect' and `adhoc-bisect': These are found only as the
> > > +   blessing of finished flights.  (This is achieved by passing *
> -adhoc
> > > +   to sg-execute-flight.)  This allows the archaeologist tools
> to
> > > +   distinguish full flights from bisection steps.
> > 
> > I don't follow the reference to *-adhoc here, since it matches
> neither
> > `real-bisect' nor `adhoc-bisect'.
> 
> `Passing *-adhoc' should read `Passing *-bisect'.
> 
> > > +There is a special exception to the tools' flight status checks:
> any
> > > +flight whose blessing contains `play' can be operated on out of
> order.
> > > +
> > > +Flights blessings can be manually changed with cs-flight-bless. 
>  Eg
> > > +  ./cs-flight-bless FLIGHT broken-real real
> > > +updates FLIGHT to be marked `broken' rather than `real'.  This
> can be
> > 
> > The example says `broken-real' but the text says `broken' (if the
> text
> > didn't quote the `broken' it probably wouldn't matter.
> 
> I have clarified this.
> 
> > Maybe mention the need to pass the current blessing as a safety
> catch,
> > since it is part of the example command?
> 
> Yes.
> 
> squash! patch and complete v2, below.
> 
> Ian.
> 
> From 73903500006c0451cc81697872096fe2d6fe02ab Mon Sep 17 00:00:00
> 2001
> From: Ian Jackson <ian.jackson@eu.citrix.com>
> Date: Thu, 17 Dec 2015 17:58:41 +0000
> Subject: [OSSTEST PATCH] squash! README.dev: Document the blessings
> 
> v2: Improvements from review

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [OSSTEST PATCH 6/9] db_retry: Suppress an "exiting via last" warning
  2015-12-17 18:10       ` Ian Campbell
@ 2015-12-17 18:38         ` Ian Jackson
  2015-12-18 11:14           ` Ian Campbell
  0 siblings, 1 reply; 31+ messages in thread
From: Ian Jackson @ 2015-12-17 18:38 UTC (permalink / raw)
  To: Ian Campbell; +Cc: xen-devel

Ian Campbell writes ("Re: [OSSTEST PATCH 6/9] db_retry: Suppress an "exiting via last" warning"):
> On Thu, 2015-12-17 at 17:48 +0000, Ian Jackson wrote:
> > Why would `return' be better ?  It would still generate the same
> > warning.  Leaving the loop via `last' seems less heavyweight than
> > using `return' and less likely to produce bugs in future patches whose
> > authors don't spot the non-local exit.
> 
> I think I'm probably confused about the scope of the eval vs
> {last,return} etc. In any case, that's no reason to block this patch
> AFAICT: Acked-by: Ian Campbell <ian.campbell@citrix.com>

Thanks.

FYI, it's (eliding lots) like this:

  sub db_retry ($$$;$$) {
      for (;;) {
          $pre->();
          eval {
              $r= &$body;
              if ($db_retry_stop) {
                  no warnings qw(exiting);
                  last if $db_retry_stop eq 'abort';
                  next;
              }
              $dbh->commit();
          };
          last if !length $@;
      }
      return $r;
  }

The warning from perl is because when `last if $db_retry_stop'
happens, `last' exits the eval in order to exit the outer for (;;)
loop.

The same would apply for the `next'.  If there were any callers of
db_retry_retry, they would trigger the `next', which would exit the
eval in order to restart the outer for.

Both of these are intentional.  Perl thinks they might not be.


Thanks for all the acks.  I'll maybe push this at some point, or
combine it with something else.  It's not urgent.

Ian.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [OSSTEST PATCH 6/9] db_retry: Suppress an "exiting via last" warning
  2015-12-17 18:38         ` Ian Jackson
@ 2015-12-18 11:14           ` Ian Campbell
  2015-12-18 14:39             ` Ian Jackson
  0 siblings, 1 reply; 31+ messages in thread
From: Ian Campbell @ 2015-12-18 11:14 UTC (permalink / raw)
  To: Ian Jackson; +Cc: xen-devel

On Thu, 2015-12-17 at 18:38 +0000, Ian Jackson wrote:

Continuing purely for my own edification...

> FYI, it's (eliding lots) like this:
> 
>   sub db_retry ($$$;$$) {
>       for (;;) {
>           $pre->();
>           eval {
>               $r= &$body;
>               if ($db_retry_stop) {
>                   no warnings qw(exiting);
>                   last if $db_retry_stop eq 'abort';
>                   next;
>               }
>               $dbh->commit();
>           };
>           last if !length $@;
>       }
>       return $r;
>   }
> 
> The warning from perl is because when `last if $db_retry_stop'
> happens, `last' exits the eval in order to exit the outer for (;;)
> loop.

Meaning that last (or next) will exit the eval and immediately end (or
restart) the containing loop, in particular without executing the "last if
!length $@;".

> Both of these are intentional.  Perl thinks they might not be.

... and that's because it is a common enough mistake to think that next or
last exits only the scope of the eval and therefore in this case to expect
it to run "last if !length $@;", while it actually wont.

I would have expected that if you want to successfully exit (i.e. not die)
the eval early and continue on with the remainder of the loop you should
use return, and http://perldoc.perl.org/functions/eval.html seems to back
that up ("the value returned is the value of the last expression evaluated
inside the mini-program; a return statement may be also used,"), but
earlier you said that return would generate the same warning, so I remain a
bit confused.

According to my reading of eval.html "return if $db_retry_stop eq 'abort'"
would exit the eval with $@="" ("If there was no error, $@ is set to the
empty string", it mentions last and goto bypassing this, but not return).
Then since length "" is false that outer last would be run insteadand we
are done with the loop as we wanted. AFAICT that would avoid the warning
for the inner last, but is not usable for the inner next, and in any case
using last inside the eval is actually more obvious in this instance, once
one thinks one understands an eval (which in my case is still debatable I
think)

Ian.

> Thanks for all the acks.  I'll maybe push this at some point, or
> combine it with something else.  It's not urgent.
> 
> Ian.

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [OSSTEST PATCH 6/9] db_retry: Suppress an "exiting via last" warning
  2015-12-18 11:14           ` Ian Campbell
@ 2015-12-18 14:39             ` Ian Jackson
  0 siblings, 0 replies; 31+ messages in thread
From: Ian Jackson @ 2015-12-18 14:39 UTC (permalink / raw)
  To: Ian Campbell; +Cc: xen-devel

Ian Campbell writes ("Re: [OSSTEST PATCH 6/9] db_retry: Suppress an "exiting via last" warning"):
> On Thu, 2015-12-17 at 18:38 +0000, Ian Jackson wrote:
> > Both of these are intentional.  Perl thinks they might not be.
> 
> ... and that's because it is a common enough mistake to think that next or
> last exits only the scope of the eval and therefore in this case to expect
> it to run "last if !length $@;", while it actually wont.

Yes.

> I would have expected that if you want to successfully exit (i.e. not die)
> the eval early and continue on with the remainder of the loop you should
> use return, and http://perldoc.perl.org/functions/eval.html seems to back
> that up ("the value returned is the value of the last expression evaluated
> inside the mini-program; a return statement may be also used,"), but
> earlier you said that return would generate the same warning, so I remain a
> bit confused.

Oh!  I was confused about the effect of `return' in an eval.  I
expected it to exit past the eval and return out of the calling
function.

> According to my reading of eval.html "return if $db_retry_stop eq 'abort'"
> would exit the eval with $@="" ("If there was no error, $@ is set to the
> empty string", it mentions last and goto bypassing this, but not return).
> Then since length "" is false that outer last would be run insteadand we
> are done with the loop as we wanted. AFAICT that would avoid the warning
> for the inner last, but is not usable for the inner next, and in any case
> using last inside the eval is actually more obvious in this instance, once
> one thinks one understands an eval (which in my case is still debatable I
> think)

I still think this reasoning about `return' is more fiddly than just
saying `last' or `next' when we mean `last' or `next'...

Particularly given that, now, there is a `no warnings qw(exiting)' to
cue the reader that they may need to check the FM.

Ian.

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2015-12-18 14:39 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-17 17:06 [OSSTEST PATCH 1/9] README.dev: Document the blessings Ian Jackson
2015-12-17 17:06 ` [OSSTEST PATCH 2/9] mg-schema-test-database: Provide some timeouts which are better for testing Ian Jackson
2015-12-17 17:19   ` Ian Campbell
2015-12-17 17:06 ` [OSSTEST PATCH 3/9] mg-schema-test-database: Wipe previous local plan data Ian Jackson
2015-12-17 17:22   ` Ian Campbell
2015-12-17 17:37     ` Ian Jackson
2015-12-17 17:42       ` Ian Campbell
2015-12-17 17:50         ` Ian Jackson
2015-12-17 18:11           ` Ian Campbell
2015-12-17 17:06 ` [OSSTEST PATCH 4/9] mg-schema-test-database: Borrow shares properly Ian Jackson
2015-12-17 17:26   ` Ian Campbell
2015-12-17 17:43     ` Ian Jackson
2015-12-17 18:08       ` Ian Campbell
2015-12-17 17:06 ` [OSSTEST PATCH 5/9] ms-planner: Improve an error message Ian Jackson
2015-12-17 17:26   ` Ian Campbell
2015-12-17 17:06 ` [OSSTEST PATCH 6/9] db_retry: Suppress an "exiting via last" warning Ian Jackson
2015-12-17 17:31   ` Ian Campbell
2015-12-17 17:48     ` Ian Jackson
2015-12-17 18:10       ` Ian Campbell
2015-12-17 18:38         ` Ian Jackson
2015-12-18 11:14           ` Ian Campbell
2015-12-18 14:39             ` Ian Jackson
2015-12-17 17:06 ` [OSSTEST PATCH 7/9] Executive DB retry: Avoid an undefined warning Ian Jackson
2015-12-17 17:31   ` Ian Campbell
2015-12-17 17:06 ` [OSSTEST PATCH 8/9] mg-allocate: Better error handling when no candidates Ian Jackson
2015-12-17 17:33   ` Ian Campbell
2015-12-17 17:06 ` [OSSTEST PATCH 9/9] mg-allocate: In planner mode, pre-check the arguments Ian Jackson
2015-12-17 17:33   ` Ian Campbell
2015-12-17 17:18 ` [OSSTEST PATCH 1/9] README.dev: Document the blessings Ian Campbell
2015-12-17 17:59   ` Ian Jackson
2015-12-17 18:12     ` Ian Campbell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.