[OSSTEST PATCH 00/33] Database locking and retry

[OSSTEST PATCH 00/33] Database locking and retry

[Ian Jackson]

I've just pushed this to production pretest, since I'm not really
expecting anyone else to review it.  But it ought to have some
visibility, I guess.

This series contains:
 * Overhaul of database locking in Tcl daemons to improve
   concurrency and correctness
 * Overhaul of database error handling in Tcl daemons to improve
   survivability after database restart
 * Update to use Tcl 8.5 (needed for jessie)
 * New planner feature ClientNotes (to support diverse hostalloc flag)
 * Fixes to command line management utilities
 * etc.

Thanks for your (in)attention :-).

Ian.

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

[OSSTEST PATCH 01/33] mg-allocate: Fix "issteallable" call

[Ian Jackson]

81cac5a1656e "mg-allocate: Support --steal" introduced an erroneous
call to the subref $issteallable, using { } instead of ( ), producing
this error:
  Not a HASH reference at ./mg-allocate line 225.

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

diff --git a/mg-allocate b/mg-allocate
index 42e35b3..d295997 100755
--- a/mg-allocate
+++ b/mg-allocate
@@ -222,7 +222,7 @@ END
                 next if $isallocatable->($sharerow);
                 next if $sharerow->{taskid} == $tid;
                 next if $sharerow->{taskid} == $magictask{preparing};
-		next if $isstealable->{$sharerow};
+		next if $isstealable->($sharerow);
                 logm("$desc: shared, $sharerow->{shareix} locked by ".
                      $findowner->($sharerow));
                 $allshareok= 0;
-- 
2.1.4


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

[OSSTEST PATCH 02/33] mg-allocate: Do not treat already-allocated resources as satisfactory

[Ian Jackson]

This was always rather odd for ./mg-allocate HOSTNAME but makes the
more sophisticated uses like ./mg-allocate '{FLAG,FLAG,...}' very much
less useful.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 mg-allocate | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/mg-allocate b/mg-allocate
index d295997..9b66114 100755
--- a/mg-allocate
+++ b/mg-allocate
@@ -234,8 +234,7 @@ END
         if ($allocate) {
             if ($candrow->{owntaskid} == $tid) {
                 logm("$desc: already allocated to $tid");
-                $got_shareix= $candrow->{shareix};
-                $ok=1; last;
+		next;
             }
             if ($isshared) {
                 logm("$desc: available, unsharing");
-- 
2.1.4


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

[OSSTEST PATCH 03/33] mg-schema-test-database: Direct logs to local directory

[Ian Jackson]

Do not pollute a shared log area with logs of flights whose numbers
are valid only in the context of our test database.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 mg-schema-test-database | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/mg-schema-test-database b/mg-schema-test-database
index 4e0ee68..88a75cf 100755
--- a/mg-schema-test-database
+++ b/mg-schema-test-database
@@ -338,6 +338,14 @@ QueueDaemonPort ${ctrlports#*,}
 ExecutiveDbOwningRoleRegexp .*
 QueueDaemonHoldoff 3
 QueueDaemonRetry 5
+Logs $PWD/logs
+Stash $PWD/logs
+Results $PWD/results
+Publish=undef
+LogsPublish=undef
+ResultsPublish=undef
+HarnessPublishGitUserHost=undef
+HarnessPublishGitRepoDir=undef
 END
 	mv -f $tcfg.tmp $tcfg
 
-- 
2.1.4


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

[OSSTEST PATCH 04/33] mg-schema-test-database: Prepare for `daemons' to be cleverer

[Ian Jackson]

We are going to want to be able to specify daemons individually.

Replace the call to parse_only_suffix so that we have somewhere to
parse extra arguments.  No functional change yet.

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

diff --git a/mg-schema-test-database b/mg-schema-test-database
index 88a75cf..85cc050 100755
--- a/mg-schema-test-database
+++ b/mg-schema-test-database
@@ -557,7 +557,12 @@ END
 #========== DAEMONS ==========
 
 daemons)
-	parse_only_suffix "$@"
+	for arg in "$@"; do
+		case "$arg" in
+		_*)	suffix="$arg" ;;
+		*)	fail 'bad usage' ;;
+		esac
+	done
 
 	dbname
 
-- 
2.1.4


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

[OSSTEST PATCH 05/33] mg-schema-test-database: Make `daemons' be cleverer

[Ian Jackson]

Now you can tell it which daemons to run.  This is helpful if you want
to run them separately.

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

diff --git a/mg-schema-test-database b/mg-schema-test-database
index 85cc050..e95b6c5 100755
--- a/mg-schema-test-database
+++ b/mg-schema-test-database
@@ -27,9 +27,14 @@
 # deletes your test database and removes the local-config file
 #
 #
-#  ./mg-schema-test-database daemons [_SUFFIX]
+#  ./mg-schema-test-database daemons [_SUFFIX] [DAEMON...]
 #
-# synchronously runs owner and queue daemons for your test database
+# Synchronously runs owner and queue daemons for your test database.
+# If any DAEMON is specified, runs only those daemons.  DAEMON
+# may be `queue' or `owner'; otherwise it is a command which will
+# be broken at spaces, and have `./' prepended if it contains no `/'.
+# If any DAEMON contains the string `queue', it causes the data-plan.pl
+# to be cleared.
 #
 # NB that you can't drop a test database with these daemons running,
 # because Postgres will refuse to drop a database that anyone is
@@ -557,26 +562,36 @@ END
 #========== DAEMONS ==========
 
 daemons)
+	wantdaemons=()
+
 	for arg in "$@"; do
 		case "$arg" in
 		_*)	suffix="$arg" ;;
-		*)	fail 'bad usage' ;;
+		owner|queue)	wantdaemons+=("./ms-${arg}daemon") ;;
+		*/*)		wantdaemons+=("$arg") ;;
+		*)		wantdaemons+=("./$arg") ;;
 		esac
 	done
 
+	if [ "${#wantdaemons[*]}" = 0 ]; then
+		wantdaemons=(./ms-ownerdaemon ./ms-queuedaemon)
+	fi
+
 	dbname
 
-	printf "Running daemons for %s....\n" "$dbname"
+	printf "Running daemons (${wantdaemons[*]}) for %s....\n" "$dbname"
 
-	withtest \
-	exec_resetting_sigint ./ms-ownerdaemon &
+	for d in "${wantdaemons[@]}"; do
 
-	sleep 1
+		case "$d" in
+		*queuedaemon*)	rm -f data-plan.pl	;;
+		esac
 
-	withtest \
-	exec_resetting_sigint ./ms-queuedaemon &
+		withtest \
+		exec_resetting_sigint $d &
 
-	rm -f data-plan.pl
+		sleep 1
+	done
 
 	wait
 
-- 
2.1.4


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

[OSSTEST PATCH 06/33] mg-schema-test-database: Change default minflight to -100

[Ian Jackson]

It is tiresome to try to create a test db for playing with and have to
wait for a big copy.  Better to create a small one by default; if the
user has forgotten to specify a minflight, they can always drop it and
run it again.

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

diff --git a/mg-schema-test-database b/mg-schema-test-database
index e95b6c5..892e41a 100755
--- a/mg-schema-test-database
+++ b/mg-schema-test-database
@@ -230,7 +230,7 @@ create)
 	#---------- argument parsing ----------
 
 	tasks=''
-	minflight=-1000
+	minflight=-100
 	for arg in "$@"; do
 		case "$arg" in
 		*@*)
-- 
2.1.4


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

[OSSTEST PATCH 07/33] invoke-daemon: Honour OSSTEST_DAEMON_TCLSH

[Ian Jackson]

It appears that tcl8.5 in wheezy has a serious bug which makes `after
idle' not always work.  tcl8.4 has been working well in wheezy but is
not in jessie, where tcl8.5 works (and tcl8.6 has a serious event loop
bug - Debian #826741).

So we need to use different versions of Tcl on different hosts.
Allow this to be specified in ~/.xen-osstest/settings.

This affects only:
 - invoke-daemon (which is normally run from inittab)
 - mg-schema-test-database

sg-run-job and sg-execute-flight are not affected.  They do not
currently use `after idle' so that is OK for now.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 invoke-daemon           |  2 +-
 mg-schema-test-database | 10 +++++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/invoke-daemon b/invoke-daemon
index ad1434a..e4a47bb 100755
--- a/invoke-daemon
+++ b/invoke-daemon
@@ -24,4 +24,4 @@ fi
 
 cd "${0%/*}"
 if [ "x$2" != x ]; then sleep $2; fi
-exec ./$1 2>&1 | exec logger -t $1 -p local4.info
+exec $OSSTEST_DAEMON_TCLSH ./$1 2>&1 | exec logger -t $1 -p local4.info
diff --git a/mg-schema-test-database b/mg-schema-test-database
index 892e41a..5ebba39 100755
--- a/mg-schema-test-database
+++ b/mg-schema-test-database
@@ -567,7 +567,15 @@ daemons)
 	for arg in "$@"; do
 		case "$arg" in
 		_*)	suffix="$arg" ;;
-		owner|queue)	wantdaemons+=("./ms-${arg}daemon") ;;
+		owner|queue)
+			tcl="$(bash -ec '
+				set -o posix
+				if [ -e $HOME/.xen-osstest/settings ]; then
+					source $HOME/.xen-osstest/settings
+				fi
+				echo "$OSSTEST_DAEMON_TCLSH"
+			')"
+			wantdaemons+=("$tcl ./ms-${arg}daemon") ;;
 		*/*)		wantdaemons+=("$arg") ;;
 		*)		wantdaemons+=("./$arg") ;;
 		esac
-- 
2.1.4


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

[OSSTEST PATCH 08/33] Tcl: Use tclsh8.5

[Ian Jackson]

I have checked that tclsh8.5 and TclX work on osstest.test-lab (and
also osstest.xs.citrite.net).  TclX seems to be provided by tcl8.4 but
work with tcl8.5 (at least on wheezy and jessie).

Deployment note: hosts running Debian wheezy (including
osstest.xs.citrite.net, the Citrix Cambridge instance), will need
OSSTEST_DAEMON_TCLSH=tclsh8.4 in ~/.xen-osstest/settings.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 README            | 2 +-
 ms-ownerdaemon    | 2 +-
 ms-queuedaemon    | 2 +-
 ms-reportuptime   | 2 +-
 sg-execute-flight | 2 +-
 sg-run-job        | 2 +-
 6 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/README b/README
index 0a346dc..3aa35d2 100644
--- a/README
+++ b/README
@@ -246,7 +246,7 @@ To run osstest in standalone mode:
 
  - You need to install
      sqlite3
-     tcl8.4 tclx8.4 libsqlite3-tcl
+     tcl8.5 tclx8.4 libsqlite3-tcl
      libdbi-perl libdbd-sqlite3-perl
      pax rsync
      curl
diff --git a/ms-ownerdaemon b/ms-ownerdaemon
index 33ee238..5b52339 100755
--- a/ms-ownerdaemon
+++ b/ms-ownerdaemon
@@ -1,4 +1,4 @@
-#!/usr/bin/tclsh8.4
+#!/usr/bin/tclsh8.5
 # -*- Tcl -*- 
 # usage: ./ms-ownerdaemon  ... | logger
 
diff --git a/ms-queuedaemon b/ms-queuedaemon
index 2b8d621..8affacc 100755
--- a/ms-queuedaemon
+++ b/ms-queuedaemon
@@ -1,4 +1,4 @@
-#!/usr/bin/tclsh8.4
+#!/usr/bin/tclsh8.5
 # -*- Tcl -*- 
 # usage: ./ms-queuedaemon  ... | logger
 
diff --git a/ms-reportuptime b/ms-reportuptime
index aeac483..fe72b78 100755
--- a/ms-reportuptime
+++ b/ms-reportuptime
@@ -1,4 +1,4 @@
-#!/usr/bin/tclsh8.4
+#!/usr/bin/tclsh8.5
 # -*- Tcl -*- 
 # usage: ./ms-reportuptime
 
diff --git a/sg-execute-flight b/sg-execute-flight
index 4e3fcf2..27fc04c 100755
--- a/sg-execute-flight
+++ b/sg-execute-flight
@@ -1,4 +1,4 @@
-#!/usr/bin/tclsh8.4
+#!/usr/bin/tclsh8.5
 # -*- Tcl -*- 
 # usage: ./sg-execute-flight FLIGHT BLESSING
 
diff --git a/sg-run-job b/sg-run-job
index 8b2d5e1..920834a 100755
--- a/sg-run-job
+++ b/sg-run-job
@@ -1,4 +1,4 @@
-#!/usr/bin/tclsh8.4
+#!/usr/bin/tclsh8.5
 
 # This is part of "osstest", an automated testing framework for Xen.
 # Copyright (C) 2009-2013 Citrix Inc.
-- 
2.1.4


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

[OSSTEST PATCH 09/33] ms-flights-summary: Remove spurious \ in keys \%{ something }

[Ian Jackson]

With some versions of Perl this generates a warning which causes
ms-flights-summary to fail.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 ms-flights-summary | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ms-flights-summary b/ms-flights-summary
index ea72088..c703d81 100755
--- a/ms-flights-summary
+++ b/ms-flights-summary
@@ -359,7 +359,7 @@ printf("<p>%d flight(s) consisting of %s job(s)<br />%s<br />%s anonymous/rogue
 
 my %summarycounts;
 foreach my $fi (values %flights) {
-    $summarycounts{$_} += $fi->{Stats}{$_} foreach (keys \%{ $fi->{Stats} });
+    $summarycounts{$_} += $fi->{Stats}{$_} foreach (keys %{ $fi->{Stats} });
 }
 my @summarycounts = sort_stats \%summarycounts;
 
-- 
2.1.4


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

[OSSTEST PATCH 10/33] ms-planner: Support ClientNotes

[Ian Jackson]

No users yet.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 README.planner | 22 ++++++++++++++++++++++
 ms-planner     | 23 +++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/README.planner b/README.planner
index daccef5..80c2506 100644
--- a/README.planner
+++ b/README.planner
@@ -286,6 +286,13 @@ Plan is:
 						Live
 			Job	flight.job which made the allocation
 
+	ClientNotes
+		map from note key
+		to one of
+			scalar
+			array of values supplied by clients
+			hash of keys/values supplied by clients
+
 	* = internal to plan
 	** = computed by launder_check_plan
 	+ = as shown to clients
@@ -302,6 +309,12 @@ Booking list is:
 			Share		optional struct containing
 						Type
 						Shares
+	[ClientNotes]
+		as for Plan; merged by
+		for scalar, updating from Booking if specified
+		for array, appending Booking to Plan
+		for hash, updating individual values from Booking
+		type must not have changed!
 
 Sharing resources:
 
@@ -313,3 +326,12 @@ Bookings which do not create the share do not mention the master.
 
 Note that whether a resource is free, or simply nonexistent, is not
 represented.
+
+
+Note keys
+---------
+
+Note key name (key in ClientNotes):
+	Note value format:
+
+(none yet defined)
diff --git a/ms-planner b/ms-planner
index abf5fac..bce6e13 100755
--- a/ms-planner
+++ b/ms-planner
@@ -245,6 +245,7 @@ sub cmd_reset () {
     $plan->{Start}= time;
     $plan->{Events}= { };
     $plan->{Unprocessed}= [ ];
+    $plan->{ClientNotes}= { };
 
     my %magictask;
     foreach my $taskrefkey (qw(preparing shared)) {
@@ -415,6 +416,9 @@ sub cmd_get_plan () {
 	}
 	$jplan->{Events}{$reso}= \@jevts;
     }
+
+    $jplan->{ClientNotes} = $plan->{ClientNotes};
+
     print to_json($jplan),"\n" or die $!;
 }
 
@@ -529,6 +533,25 @@ sub cmd_book_resources () {
 		  );
     }
 
+    my $jnotes = $jbookings->{ClientNotes} // { };
+    foreach my $k (sort keys %$jnotes) {
+	my $v = $jnotes->{$k};
+	my $newt = ref $v // '(none)';
+	if (exists $plan->{ClientNotes}{$k}) {
+	    my $oldt = ref $plan->{ClientNotes}{$k} // '(none)';
+	    die "$k $oldt -> $newt" unless $oldt eq $newt;
+	}
+	if (!ref $v) {
+	    $plan->{ClientNotes}{$k} = $v;
+	} elsif (ref $v eq 'HASH') {
+	    $plan->{ClientNotes}{$k}{$_} = $v->{$_} foreach keys %$v;
+	} elsif (ref $v eq 'ARRAY') {
+	    push @{ $plan->{ClientNotes}{$_} }, @{ $jnotes->{$_} };
+	} else {
+	    die "$k $newt";
+	}
+    }
+
     check_write_new_plan();
 }
 
-- 
2.1.4


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

[OSSTEST PATCH 11/33] Tcl database debugging: Actually work

[Ian Jackson]

Setting OSSTEST_TCL_JOBDB_DEBUG was ineffective (ever since it was
introduced in 44dad3d8) because `env' wasn't imported from the global
scope.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tcl/JobDB-Executive.tcl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 7dba497..239f22c 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -116,6 +116,7 @@ proc db-update-1 {stmt} {
 }
 
 proc db-execute-debug {stmt} {
+    global env
     if {[var-or-default env(OSSTEST_TCL_JOBDB_DEBUG) 0]} {
 	puts stderr "EXECUTING >$stmt<"
     }
-- 
2.1.4


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

[OSSTEST PATCH 12/33] Database locking: Tcl: Use db-execute-array

[Ian Jackson]

Replace open-coded uses of pg_execute -array ARRAYVAR dbh STMT
with jobdb::db-execute-array ARRAYVAR STMT.

The only functional change is that if OSSTEST_TCL_JOBDB_DEBUG is set,
there will be debugging output.

But we are going to want to make db-execute-array do something more
complicated involving pg_exec.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 ms-ownerdaemon | 2 +-
 ms-queuedaemon | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/ms-ownerdaemon b/ms-ownerdaemon
index 5b52339..cc2f361 100755
--- a/ms-ownerdaemon
+++ b/ms-ownerdaemon
@@ -67,7 +67,7 @@ proc cmd/create-task {chan desc} {
                         ( type,  refkey,               refinfo,         live)
                  VALUES ('ownd', [pg_quote $taskdesc], [clock seconds], 't')
         "
-        set nrows [pg_execute -array av dbh "
+        set nrows [jobdb::db-execute-array av "
             SELECT taskid
               FROM tasks
              WHERE live AND refkey = [pg_quote $taskdesc]
diff --git a/ms-queuedaemon b/ms-queuedaemon
index 8affacc..6e93288 100755
--- a/ms-queuedaemon
+++ b/ms-queuedaemon
@@ -444,7 +444,7 @@ proc cmd/unwait {chan desc} {
 
 proc for-free-resources {varname body} {
     jobdb::transaction resources {
-	pg_execute -array free_resources_row dbh {
+	jobdb::db-execute-array free_resources_row {
 		SELECT (restype || '/' || resname || '/' || shareix) AS r
 		  FROM resources
 	     WHERE NOT (SELECT live FROM tasks WHERE taskid=owntaskid)
@@ -636,7 +636,7 @@ proc cmd/uptime {chan desc seconds} {
     set descpat "[regsub {\:\d+$} $desc {:%}]"
     transaction resources {
         set keys {}
-        pg_execute -array task dbh "
+        jobdb::db-execute-array task "
             SELECT * FROM tasks
                     WHERE type = 'ownd'
                       AND ( refkey LIKE [pg_quote $descpat]
-- 
2.1.4


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

[OSSTEST PATCH 13/33] Database locking: Tcl: Use db-execute

[Ian Jackson]

Replace open-coded uses of pg_execute dbh STMT with
jobdb::db-execute STMT.

The only functional change is that if OSSTEST_TCL_JOBDB_DEBUG is set,
there will be debugging output.

But we are going to want to make db-execute do something more
complicated involving pg_exec.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 ms-ownerdaemon | 6 +++---
 ms-queuedaemon | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/ms-ownerdaemon b/ms-ownerdaemon
index cc2f361..3623d19 100755
--- a/ms-ownerdaemon
+++ b/ms-ownerdaemon
@@ -32,7 +32,7 @@ proc chan-destroy-stuff {chan} {
     jobdb::transaction resources {
         puts-chan-desc $chan "-- $tasks"
         foreach task $tasks {
-            pg_execute dbh "
+            jobdb::db-execute "
                 UPDATE tasks
                    SET live = 'f'
                  WHERE taskid = $task
@@ -62,7 +62,7 @@ proc cmd/create-task {chan desc} {
         set taskdesc $desc
     }
     jobdb::transaction resources {
-        pg_execute dbh "
+        jobdb::db-execute "
             INSERT INTO tasks
                         ( type,  refkey,               refinfo,         live)
                  VALUES ('ownd', [pg_quote $taskdesc], [clock seconds], 't')
@@ -93,7 +93,7 @@ main-daemon Owner {
     jobdb::db-open
     
     jobdb::transaction resources {
-        set nrows [pg_execute dbh "
+        set nrows [jobdb::db-execute "
             UPDATE tasks
                SET refkey = 'previous ' || refkey
              WHERE type = 'ownd'
diff --git a/ms-queuedaemon b/ms-queuedaemon
index 6e93288..9db8b05 100755
--- a/ms-queuedaemon
+++ b/ms-queuedaemon
@@ -155,7 +155,7 @@ proc runneeded-perhaps-start {} {
     }
 
     jobdb::transaction resources {
-        set nrows [pg_execute dbh {
+        set nrows [jobdb::db-execute {
             UPDATE resources
                SET owntaskid= (SELECT taskid FROM tasks
                                WHERE type='magic' AND refkey='allocatable')
@@ -167,7 +167,7 @@ proc runneeded-perhaps-start {} {
     if {!($nrows || $needed>=2)} return
 
     jobdb::transaction resources {
-        set cleaned [pg_execute dbh {
+        set cleaned [jobdb::db-execute {
             DELETE FROM tasks
              WHERE type='ownd'
                AND live='f'
@@ -650,7 +650,7 @@ proc cmd/uptime {chan desc seconds} {
                 continue
             }
             if {$refinfo > $before} continue
-            pg_execute dbh "
+            jobdb::db-execute "
                 UPDATE tasks
                    SET live = false,
                        refinfo = refinfo || ' stale'
-- 
2.1.4


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

[OSSTEST PATCH 14/33] Database locking: Tcl: Always use db-execute-array for SELECT

[Ian Jackson]

We are going to make it wrong to use db-execute for SELECT statements.

Convert the two existing violation sites (providing a dummy arrayvar).

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 sg-execute-flight       | 2 +-
 tcl/JobDB-Executive.tcl | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/sg-execute-flight b/sg-execute-flight
index 27fc04c..14c8cda 100755
--- a/sg-execute-flight
+++ b/sg-execute-flight
@@ -30,7 +30,7 @@ proc check {} {
 
     jobdb::db-open
 
-    set nqueued [jobdb::db-execute "
+    set nqueued [jobdb::db-execute-array dummy "
         SELECT job FROM jobs j
          WHERE j.flight = $flight
            AND j.status = 'queued'
diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 239f22c..bbce6fc 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -275,7 +275,7 @@ proc become-task {comment} {
     set username "[id user]@$hostname"
 
     transaction resources {
-        set nrows [db-execute "
+        set nrows [db-execute-array dummy "
             UPDATE tasks
                SET username = [pg_quote $username],
                    comment = [pg_quote $comment]
-- 
2.1.4


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

[OSSTEST PATCH 15/33] Database locking: Tcl: for errorCode, use pg_exec, not pg_execute

[Ian Jackson]

We would like to be able to retry db transactions.  To do this we need
to know why they failed (if they did).

But pg_execute does not set errorCode.  (This is clearly a bug.)  And
since it immediately discards a failed statement, any error
information has been lost by the time pg_execute returns.

So, instead, use pg_exec, and manually mess about with fishing
suitable information out of a failed statement handle, and generating
an appropriate errorCode.

There are no current consumers of this errorCode: that will come in a
moment.

A wrinkle is that as a result it is no longer possible to use
db-execute on a SELECT statement nor db-execute-array on a non-SELECT
statement.  This is because 1. the `ok' status that we have to
check for is different for statements which are commands and ones
which return tuples and 2. we need to fish a different return value out
of the statement handle (-cmdTuples vs -numTuples).  But all uses in
the codebase are now fine for this distinction.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tcl/JobDB-Executive.tcl | 54 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 51 insertions(+), 3 deletions(-)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index bbce6fc..ed9abbb 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -121,13 +121,61 @@ proc db-execute-debug {stmt} {
 	puts stderr "EXECUTING >$stmt<"
     }
 }
+
+proc db--exec-check {shvar stmt expected_status body} {
+    # pg_execute does not set errorCode and it throws away the
+    # statement handle so we can't get the error out.  So
+    # use pg_exec, as wrapped up here.
+
+    # db--exec-check executes stmt and checks that the status is
+    # `expected_status'.  If OK, executes body with $shvar set to the
+    # stmt handle.   Otherwise throws with errorCode
+    #   {OSSTEST-PSQL <pg-status> <pg-sqlstate>}
+
+    global errorInfo errorCode
+    upvar 1 $shvar sh
+
+    set sh [pg_exec dbh $stmt]
+
+    set rc [catch {
+	set status [pg_result $sh -status]
+	if {[string compare $status $expected_status]} {
+	    set emsg [pg_result $sh -error]
+	    set sqlstate [pg_result $sh -error sqlstate]
+	    if {![string length $emsg]} {
+		set emsg "osstest expected status $expected_status got $status"
+	    }
+	    set context [pg_result $sh -error context]
+	    error $emsg \
+		"    while executing SQL\n$stmt\n    in SQL context\n$context" \
+		[list OSSTEST-PSQL $status $sqlstate]
+	}
+	uplevel 1 $body
+    } emsg]
+
+    set ei $errorInfo
+    set ec $errorCode
+    catch { pg_result $sh -clear }
+
+    return -code $rc -errorinfo $ei -errorcode $ec $emsg
+}
+
 proc db-execute {stmt} {
     db-execute-debug $stmt
-    uplevel 1 [list pg_execute dbh $stmt]
+    db--exec-check sh $stmt PGRES_COMMAND_OK {
+	return [pg_result $sh -cmdTuples]
+    }
 }
-proc db-execute-array {arrayvar stmt args} {
+proc db-execute-array {arrayvar stmt {body {}}} {
     db-execute-debug $stmt
-    uplevel 1 [list pg_execute -array $arrayvar dbh $stmt] $args
+    db--exec-check sh $stmt PGRES_TUPLES_OK {
+	set nrows [pg_result $sh -numTuples]
+	for {set row 0} {$row < $nrows} {incr row} {
+	    uplevel 1 [list pg_result $sh -tupleArray $row $arrayvar]
+	    uplevel 1 $body
+	}
+	return $nrows
+    }
 }
 
 proc lock-tables {tables} {
-- 
2.1.4


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

[OSSTEST PATCH 16/33] Database locking: Tcl: Retry only on DEADLOCK DETECTED

[Ian Jackson]

Use the new errorCode coming out of db-execute* to tell when the error
is that we got a database deadlock, which is the situation in which we
should retry.

This involves combining the two catch blocks, so that there is only
one error handling strategy.  Previously errors on COMMIT would be
retried and others would not.  Now errors anywhere might be retried
but only if the DB indicated deadlock.

We now unconditionally execute ROLLBACK.  This is more correct, since
we always previously executed BEGIN.

And, we pass the errorInfo and errorCode from the $body to the caller.

I have tested this with a test db instance, using contrived means to
generate a database deadlock, and it does actually retry.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tcl/JobDB-Executive.tcl | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index ed9abbb..63db4f0 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -283,25 +283,27 @@ proc transaction {tables script} {
 	    db-execute "SET TRANSACTION ISOLATION LEVEL SERIALIZABLE"
 	    lock-tables $tables
 	    uplevel 1 $script
+	    db-execute COMMIT
 	} result]
-	if {!$rc} {
-	    if {[catch {
-		db-execute COMMIT
-	    } emsg]} {
-		puts "commit failed: $emsg; retrying ..."
-		db-execute ROLLBACK
-		if {[incr retries -1] <= 0} {
-		    error \
- "commit failed, too many retries: $emsg\n$errorInfo\n$errorCode\n"
+	set ei $errorInfo
+	set ec $errorCode
+	if {$rc} {
+	    db-execute ROLLBACK
+	    switch -glob $errorCode {
+		{OSSTEST-PSQL * 40P01} {
+		    # DEADLOCK DETECTED
+		    puts "transaction deadlock ($result) retrying ..."
+		    if {[incr retries -1] <= 0} {
+			error \
+ "transaction failed, too many retries: $result\n$errorInfo\n$errorCode\n"
+		    }
+		    after 500
+		    continue
 		}
-		after 500
-		continue
 	    }
-	} else {
-	    db-execute ROLLBACK
 	}
         db-close
-	return -code $rc $result
+	return -code $rc -errorinfo $ei -errorcode $ec $result
     }
 }
 
-- 
2.1.4


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

[OSSTEST PATCH 17/33] ms-ownerdaemon: Cope with db restart. Retry recording dead tasks.

[Ian Jackson]

In chan-destroy-stuff, instead of accessing the db directly, add the
dead task(s) to a queue, and arrange to look at that queue.

Errors are handled by setting an `after' handler which we cancel if we
are successful.

The after handler requeues a queue run attempt as the first thing
(which will arrange that a further retry will occur if things are
still broken) and then attempts to reconnect to the database.

I have tested this with a test instance by renaming the `tasks' table
under its feet, and it functions as expected.

DEPLOYMENT NOTE: The owner daemon cannot be restarted without shutting
everything down.  So this update should first be deployed in
Cambridge, probably, to see how it goes.  Also, it is less critical in
the main Xen production test lab because there the db and the owner
daemon are co-hosted on the same VM.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: Put back the `unset tasks' which was mistakenly removed.  The
    effect of its lack is to fail to clear out the task list for
    previous uses of the channel (which is named after the fd); this
    is mostly harmless apart from log spam but causes the usual
    case to be something like
       OK created-task 456354 ownd [10.80.227.94]:44852-876
    rather than
       OK created-task 456354 ownd [10.80.227.94]:44852-876
    which some of the clients (rightly) don't expect.
---
 Osstest/Executive.pm |  1 +
 ms-ownerdaemon       | 38 ++++++++++++++++++++++++++++++++++----
 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/Osstest/Executive.pm b/Osstest/Executive.pm
index 468031c..0602925 100644
--- a/Osstest/Executive.pm
+++ b/Osstest/Executive.pm
@@ -113,6 +113,7 @@ augmentconfigdefaults(
 augmentconfigdefaults(
     OwnerDaemonHost => $c{ControlDaemonHost},
     QueueDaemonHost => $c{ControlDaemonHost},
+    OwnerDaemonDbRetry => $c{QueueDaemonRetry},
 );
 
 #---------- configuration reader etc. ----------
diff --git a/ms-ownerdaemon b/ms-ownerdaemon
index 3623d19..62ca645 100755
--- a/ms-ownerdaemon
+++ b/ms-ownerdaemon
@@ -22,16 +22,38 @@
 source ./tcl/daemonlib.tcl
 
 
+set dead_tasks {}
+
 proc chan-destroy-stuff {chan} {
+    global dead_tasks
+
     upvar #0 chanawait($chan) await
     catch { unset await }
 
     upvar #0 chantasks($chan) tasks
     if {![info exists tasks]} return
 
+    puts-chan-desc $chan "-- $tasks"
+
+    foreach task $tasks {
+	lappend dead_tasks $task
+    }
+    unset tasks
+    after idle record-dead-tasks
+}
+
+proc record-dead-tasks {} {
+    global c dead_tasks
+
+    if {![llength $dead_tasks]} return
+
+    puts "record-dead-tasks ... $dead_tasks"
+
+    set retry [expr {$c(OwnerDaemonDbRetry) * 1000}]
+    set eafter [after $retry record-dead-tasks-retry]
+
     jobdb::transaction resources {
-        puts-chan-desc $chan "-- $tasks"
-        foreach task $tasks {
+        foreach task $dead_tasks {
             jobdb::db-execute "
                 UPDATE tasks
                    SET live = 'f'
@@ -39,12 +61,20 @@ proc chan-destroy-stuff {chan} {
             "
         }
     }
-    puts-chan-desc $chan "== $tasks"
-    unset tasks
 
+    after cancel $eafter
+    puts "record-dead-tasks OK. $dead_tasks"
+    set dead_tasks {}
     after idle await-endings-notify
 }
 
+proc record-dead-tasks-retry {} {
+    after idle record-dead-tasks
+    puts "** reconnecting/retrying **"
+    catch { jobdb::db-close }
+    jobdb::db-open
+}
+
 proc await-endings-notify {} {
     global chanawait
     foreach chan [array names chanawait] {
-- 
2.1.4


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

[OSSTEST PATCH 18/33] ms-ownerdaemon: Break out db-reopen, and move it to JobDB-Executive

[Ian Jackson]

Also, change the `puts' to a `logputs'.  No other functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 ms-ownerdaemon          | 4 +---
 tcl/JobDB-Executive.tcl | 6 ++++++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/ms-ownerdaemon b/ms-ownerdaemon
index 62ca645..31e6fdd 100755
--- a/ms-ownerdaemon
+++ b/ms-ownerdaemon
@@ -70,9 +70,7 @@ proc record-dead-tasks {} {
 
 proc record-dead-tasks-retry {} {
     after idle record-dead-tasks
-    puts "** reconnecting/retrying **"
-    catch { jobdb::db-close }
-    jobdb::db-open
+    jobdb::db-reopen
 }
 
 proc await-endings-notify {} {
diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 63db4f0..29ab59a 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -109,6 +109,12 @@ proc db-close {} {
     pg_disconnect dbh
 }
 
+proc db-reopen {} {
+    logputs stdout "** reopening database **"
+    catch { db-close }
+    db-open
+}
+
 proc db-update-1 {stmt} {
     # must be in transaction
     set nrows [db-execute $stmt]
-- 
2.1.4


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

[OSSTEST PATCH 19/33] tcl daemons: Move BEGIN within scope of transaction error trapping

[Ian Jackson]

If the db connection has failed, BEGIN will fail.  We want to to
handle this properly.

Right now the effect is that we will now close the connection and it
will then be reopened by the next command.

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

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 29ab59a..f2322c4 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -284,8 +284,8 @@ proc transaction {tables script} {
     db-open
     while 1 {
         set ol {}
-        db-execute BEGIN
 	set rc [catch {
+	    db-execute BEGIN
 	    db-execute "SET TRANSACTION ISOLATION LEVEL SERIALIZABLE"
 	    lock-tables $tables
 	    uplevel 1 $script
-- 
2.1.4


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

[OSSTEST PATCH 20/33] tcl daemons: jobdb::transaction: Improve two message generation sites

[Ian Jackson]

* Use logputs rather than puts to report transaction deadlock retry

* Use $ei and $ec rather than $errorInfo and $errorCode when calling
  error due to too many deadlock retries.  This has no functional change
  but is less fragile in case of future addition of new calls to catch
  between the main catch and this throw.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tcl/JobDB-Executive.tcl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index f2322c4..c1e5c63 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -298,10 +298,10 @@ proc transaction {tables script} {
 	    switch -glob $errorCode {
 		{OSSTEST-PSQL * 40P01} {
 		    # DEADLOCK DETECTED
-		    puts "transaction deadlock ($result) retrying ..."
+		    logputs stdout "transaction deadlock ($result) retrying ..."
 		    if {[incr retries -1] <= 0} {
 			error \
- "transaction failed, too many retries: $result\n$errorInfo\n$errorCode\n"
+ "transaction failed, too many retries: $result\n$ei\n$ec\n"
 		    }
 		    after 500
 		    continue
-- 
2.1.4


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

[OSSTEST PATCH 21/33] tcl daemons: Remove obsolete `global g'

[Ian Jackson]

The global g was removed in 2012 in beb4240a346e "wip reorg,
testing..." but this instance seems to have escaped.

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

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index c1e5c63..3ad3693 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -81,7 +81,6 @@ proc set-flight {} {
 variable dbusers 0
 
 proc db-open {} {
-    global g
     variable dbusers
 
     if {$dbusers > 0} { incr dbusers; return }
-- 
2.1.4


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

[OSSTEST PATCH 22/33] tcl daemons: Break out db-ensure-open and db-ensure-closed

[Ian Jackson]

To be able to deliberately reconnect to the database, in case of
error, we need functions which actually work with dbh, rather than
simply the refcount.

No functional change as yet.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tcl/JobDB-Executive.tcl | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 3ad3693..da9b7e9 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -85,6 +85,17 @@ proc db-open {} {
 
     if {$dbusers > 0} { incr dbusers; return }
 
+    db-ensure-open
+    incr dbusers
+}
+proc db-close {} {
+    variable dbusers
+    incr dbusers -1
+    if {$dbusers > 0} return
+    if {$dbusers} { error "$dbusers ?!" }
+    db-ensure-closed
+}
+proc db-ensure-open {} {
     set pl {
 	use Osstest;
 	use Osstest::Executive;
@@ -98,13 +109,8 @@ proc db-open {} {
     # is whitespace-separated.
     regsub -all {;} $db_pg_dsn { } conninfo
     pg_connect -conninfo $conninfo -connhandle dbh
-    incr dbusers
 }
-proc db-close {} {
-    variable dbusers
-    incr dbusers -1
-    if {$dbusers > 0} return
-    if {$dbusers} { error "$dbusers ?!" }
+proc db-ensure-closed {} {
     pg_disconnect dbh
 }
 
-- 
2.1.4


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

[OSSTEST PATCH 23/33] tcl daemons: db-ensure-open, -close: Make idempotent

[Ian Jackson]

Track whether we think the connection is open in dbopen.

It is now therefore OK to call db-ensure-open and db-ensure-closed in
other contexts.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tcl/JobDB-Executive.tcl | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index da9b7e9..1763b69 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -79,6 +79,7 @@ proc set-flight {} {
 }
 
 variable dbusers 0
+variable dbopen 0 ;# 1 means known to be open and good; 0 might mean broken
 
 proc db-open {} {
     variable dbusers
@@ -96,6 +97,11 @@ proc db-close {} {
     db-ensure-closed
 }
 proc db-ensure-open {} {
+    variable dbopen
+
+    if {$dbopen} return
+    catch { db-ensure-closed } ;# clean up any detritus
+
     set pl {
 	use Osstest;
 	use Osstest::Executive;
@@ -109,8 +115,12 @@ proc db-ensure-open {} {
     # is whitespace-separated.
     regsub -all {;} $db_pg_dsn { } conninfo
     pg_connect -conninfo $conninfo -connhandle dbh
+
+    set dbopen 1
 }
 proc db-ensure-closed {} {
+    variable dbopen
+    set dbopen 0
     pg_disconnect dbh
 }
 
-- 
2.1.4


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

[OSSTEST PATCH 24/33] tcl daemons: make db-reopen actually work

[Ian Jackson]

Even if the refcount is >0, we want to actually reconnect.
Also, log something if the close fails.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tcl/JobDB-Executive.tcl | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 1763b69..6c7e067 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -125,9 +125,12 @@ proc db-ensure-closed {} {
 }
 
 proc db-reopen {} {
+    variable dbusers
     logputs stdout "** reopening database **"
-    catch { db-close }
-    db-open
+    if {[catch { db-ensure-closed } emsg]} {
+	logputs stdout "(db disconnect: $emsg)"
+    }
+    if {$dbusers > 0} db-ensure-open
 }
 
 proc db-update-1 {stmt} {
-- 
2.1.4


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

[OSSTEST PATCH 25/33] tcl daemons: More info in db--exec-check error

[Ian Jackson]

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

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 6c7e067..8f4ed98 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -170,7 +170,7 @@ proc db--exec-check {shvar stmt expected_status body} {
 		set emsg "osstest expected status $expected_status got $status"
 	    }
 	    set context [pg_result $sh -error context]
-	    error $emsg \
+	    error "db exec failed ($status, $sqlstate) $emsg" \
 		"    while executing SQL\n$stmt\n    in SQL context\n$context" \
 		[list OSSTEST-PSQL $status $sqlstate]
 	}
-- 
2.1.4


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

[OSSTEST PATCH 26/33] tcl daemons: Recognise `SSL SYSCALL' errors with their own errorCode

[Ian Jackson]

This has no real effect right now but will be useful in a moment.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tcl/JobDB-Executive.tcl | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 8f4ed98..3c2b4db 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -170,9 +170,16 @@ proc db--exec-check {shvar stmt expected_status body} {
 		set emsg "osstest expected status $expected_status got $status"
 	    }
 	    set context [pg_result $sh -error context]
+	    set ecode OSSTEST-PSQL
+	    if {![string length $sqlstate] &&
+		[string match {SSL SYSCALL *} $emsg]} {
+		# sadly the pg client library doesn't provide a code
+		# for this so we match the error message
+		set ecode OSSTEST-PSQL-SSL-SYSCALL
+	    }
 	    error "db exec failed ($status, $sqlstate) $emsg" \
 		"    while executing SQL\n$stmt\n    in SQL context\n$context" \
-		[list OSSTEST-PSQL $status $sqlstate]
+		[list $ecode $status $sqlstate]
 	}
 	uplevel 1 $body
     } emsg]
-- 
2.1.4


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

[OSSTEST PATCH 27/33] tcl daemons: transaction: Properly match db-open and db-close

[Ian Jackson]

* Do the db-open inside the catch, so that if it fails we do the
  rest of the error handling.

* Do the db-close before deconstructing the error, so that we
  necessarily get the db-open reference count right.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tcl/JobDB-Executive.tcl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 3c2b4db..fe1c946 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -306,10 +306,10 @@ proc step-set-status {flight job stepno st} {
 proc transaction {tables script} {
     global errorInfo errorCode
     set retries 100
-    db-open
     while 1 {
         set ol {}
 	set rc [catch {
+	    db-open
 	    db-execute BEGIN
 	    db-execute "SET TRANSACTION ISOLATION LEVEL SERIALIZABLE"
 	    lock-tables $tables
@@ -318,6 +318,7 @@ proc transaction {tables script} {
 	} result]
 	set ei $errorInfo
 	set ec $errorCode
+	db-close
 	if {$rc} {
 	    db-execute ROLLBACK
 	    switch -glob $errorCode {
@@ -333,7 +334,6 @@ proc transaction {tables script} {
 		}
 	    }
 	}
-        db-close
 	return -code $rc -errorinfo $ei -errorcode $ec $result
     }
 }
-- 
2.1.4


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

[OSSTEST PATCH 28/33] tcl daemons: if error occurs, ensure db is closed afterwards

[Ian Jackson]

When an error occurs which we are not handling, we want to
unconditionally close the db connection.  The next transaction will
reopen it.

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

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index fe1c946..c0cd4e9 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -333,6 +333,7 @@ proc transaction {tables script} {
 		    continue
 		}
 	    }
+	    catch { db-ensure-closed }
 	}
 	return -code $rc -errorinfo $ei -errorcode $ec $result
     }
-- 
2.1.4


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

[OSSTEST PATCH 29/33] tcl daemons: transaction: Only try ROLLBACK when necessary

[Ian Jackson]

In the deadlock case, we need to ROLLBACK.  In other error cases we
are going to close the connection.  And in those other cases the
ROLLBACK might fail, causing our error recovery to go wrong.

So do ROLLBACK only on the single path where we might continue to use
the connection.

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

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index c0cd4e9..535fbd7 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -320,10 +320,10 @@ proc transaction {tables script} {
 	set ec $errorCode
 	db-close
 	if {$rc} {
-	    db-execute ROLLBACK
 	    switch -glob $errorCode {
 		{OSSTEST-PSQL * 40P01} {
 		    # DEADLOCK DETECTED
+		    db-execute ROLLBACK
 		    logputs stdout "transaction deadlock ($result) retrying ..."
 		    if {[incr retries -1] <= 0} {
 			error \
-- 
2.1.4


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

[OSSTEST PATCH 30/33] tcl daemons: transaction: Support db autoreconnect

[Ian Jackson]

Provide an `autoreconnect' argument which will automatically reconnect
to the db if the connection has been lost.  It will make only one
reconnection attempt.

No functional change yet because no call sites have been changed.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tcl/JobDB-Executive.tcl | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 535fbd7..0fc0a6d 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -303,7 +303,7 @@ proc step-set-status {flight job stepno st} {
     }
 }
 
-proc transaction {tables script} {
+proc transaction {tables script {autoreconnect 0}} {
     global errorInfo errorCode
     set retries 100
     while 1 {
@@ -332,6 +332,20 @@ proc transaction {tables script} {
 		    after 500
 		    continue
 		}
+		{OSSTEST-PSQL * 08*} -
+		{OSSTEST-PSQL * 57*} -
+		{OSSTEST-PSQL-SSL-SYSCALL *} {
+		    # Class 08 — Connection Exception
+		    # Class 57 — Operator Intervention
+		    #    (includes various shutdowns)
+		    logputs stderr \
+ "db connection exception (autoreconnect=$autoreconnect): $result\n$ei\n$ec\n"
+		    if {$autoreconnect} {
+			set autoreconnect 0; # try this only once
+			db-reopen
+			continue
+		    }
+		}
 	    }
 	    catch { db-ensure-closed }
 	}
-- 
2.1.4


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

[OSSTEST PATCH 31/33] tcl-daemons: ms-ownerdaemon: Use autoreconnect

[Ian Jackson]

When creating a job, we ask jobdb::transaction to retry on failure.
This makes the error handling more effective.

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

diff --git a/ms-ownerdaemon b/ms-ownerdaemon
index 31e6fdd..bf0b595 100755
--- a/ms-ownerdaemon
+++ b/ms-ownerdaemon
@@ -102,7 +102,7 @@ proc cmd/create-task {chan desc} {
         "]
         if {$nrows != 1} { error "multiple $taskdesc!" }
         set task $av(taskid)
-    }
+    } 1 ;#autoreconnect
     lappend tasks $task
     puts-chan $chan "OK created-task $task ownd $taskdesc"
 }
-- 
2.1.4


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

[OSSTEST PATCH 32/33] tcl daemons: Provide with-db

[Ian Jackson]

This makes it easier to get the matching of db-open and db-close right.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tcl/JobDB-Executive.tcl | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 0fc0a6d..07a0438 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -303,6 +303,22 @@ proc step-set-status {flight job stepno st} {
     }
 }
 
+proc with-db {script} {
+    global errorInfo errorCode
+    set rc [catch {
+	db-open
+	uplevel 1 $script
+    } result]
+    set ei $errorInfo
+    set ec $errorCode
+    if {$rc} {
+	catch { db-close }
+    } else {
+	db-close
+    }
+    return -code $rc -errorinfo $ei -errorcode $ec $result
+}
+    
 proc transaction {tables script {autoreconnect 0}} {
     global errorInfo errorCode
     set retries 100
-- 
2.1.4


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

[OSSTEST PATCH 33/33] tcl daemons: Use with-db

[Ian Jackson]

Fixes a number of latent bugs where the jobdb refcount might get out
of step.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 sg-execute-flight       |  82 ++++++++++++++++++-------------------
 tcl/JobDB-Executive.tcl | 106 ++++++++++++++++++++++++------------------------
 2 files changed, 93 insertions(+), 95 deletions(-)

diff --git a/sg-execute-flight b/sg-execute-flight
index 14c8cda..7932c29 100755
--- a/sg-execute-flight
+++ b/sg-execute-flight
@@ -28,48 +28,48 @@ proc check {} {
 
     if {$stopping} return
 
-    jobdb::db-open
-
-    set nqueued [jobdb::db-execute-array dummy "
-        SELECT job FROM jobs j
-         WHERE j.flight = $flight
-           AND j.status = 'queued'
-         LIMIT 1
-    "]
-
-    set nrunning [llength $running]
-    log "flight $flight nqueued=$nqueued nrunning=$nrunning"
-
-    if {!$nqueued && !$nrunning} {
-        prequit finished
-        exec ./cs-flight-bless $flight $blessing running 2>@ stderr
-        exit 0
-    }
+    jobdb::with-db {
+
+	set nqueued [jobdb::db-execute-array dummy "
+	    SELECT job FROM jobs j
+	     WHERE j.flight = $flight
+	       AND j.status = 'queued'
+	     LIMIT 1
+	"]
+
+	set nrunning [llength $running]
+	log "flight $flight nqueued=$nqueued nrunning=$nrunning"
+
+	if {!$nqueued && !$nrunning} {
+	    prequit finished
+	    exec ./cs-flight-bless $flight $blessing running 2>@ stderr
+	    exit 0
+	}
+
+	jobdb::db-execute-array jobinfo "
+	    SELECT * FROM jobs j
+	     WHERE j.flight = $flight
+	       AND j.status = 'queued'
+	       AND 0 = (SELECT count(*) FROM jobs d
+			 WHERE d.flight = $flight
+			   AND ( d.status = 'queued'
+			      OR d.status = 'preparing'
+			      OR d.status = 'running'
+			      OR d.status = 'retriable' )
+			   AND (d.job IN (SELECT val FROM runvars r
+					 WHERE r.flight = $flight
+					   AND r.job = j.job
+					   AND r.name LIKE '%job')
+			    OR (d.flight || '.' || d.job) IN
+					 (SELECT val FROM runvars r
+					 WHERE r.flight = $flight
+					   AND r.job = j.job
+					   AND r.name LIKE '%job'))
+		       )
+	     ORDER BY job
+	" maybe-spawn-job
 
-    jobdb::db-execute-array jobinfo "
-        SELECT * FROM jobs j
-         WHERE j.flight = $flight
-           AND j.status = 'queued'
-           AND 0 = (SELECT count(*) FROM jobs d
-                     WHERE d.flight = $flight
-                       AND ( d.status = 'queued'
-                          OR d.status = 'preparing'
-                          OR d.status = 'running'
-                          OR d.status = 'retriable' )
-                       AND (d.job IN (SELECT val FROM runvars r
-                                     WHERE r.flight = $flight
-                                       AND r.job = j.job
-                                       AND r.name LIKE '%job')
-                        OR (d.flight || '.' || d.job) IN
-                                     (SELECT val FROM runvars r
-                                     WHERE r.flight = $flight
-                                       AND r.job = j.job
-                                       AND r.name LIKE '%job'))
-                   )
-         ORDER BY job
-    " maybe-spawn-job
-
-    jobdb::db-close
+    }
 }
 
 proc prequit {why} {
diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 07a0438..0f450c1 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -28,36 +28,36 @@ proc logputs {f m} {
 
 proc prepare {job} {
     global flight jobinfo
-    db-open
-    set found 0
-    db-execute-array jobinfo "
-        SELECT job, status, recipe FROM jobs
-			WHERE	flight = [pg_quote $flight]
-			AND	job = [pg_quote $job]
-    " {
-	switch -exact -- $jobinfo(status) {
-	    queued - preparing - retriable - play { incr found }
-	    default {
-		error "job $flight.$job status $jobinfo(status)"
+    with-db {
+	set found 0
+	db-execute-array jobinfo "
+	    SELECT job, status, recipe FROM jobs
+			    WHERE	flight = [pg_quote $flight]
+			    AND	job = [pg_quote $job]
+	" {
+	    switch -exact -- $jobinfo(status) {
+		queued - preparing - retriable - play { incr found }
+		default {
+		    error "job $flight.$job status $jobinfo(status)"
+		}
 	    }
 	}
-    }
-    if {!$found} {
-	error "job $flight.$job not found"
-    }
+	if {!$found} {
+	    error "job $flight.$job not found"
+	}
 
-    setstatus preparing
-    db-close
+	setstatus preparing
+    }
 }
 
 proc job-set-status-unlocked {flight job st} {
-    db-open
-    db-execute "
-            UPDATE jobs SET status='$st'
-                WHERE flight=$flight AND job='$job'
-                  AND status<>'aborted' AND status<>'broken'
-    "
-    db-close
+    with-db {
+	db-execute "
+		UPDATE jobs SET status='$st'
+		    WHERE flight=$flight AND job='$job'
+		      AND status<>'aborted' AND status<>'broken'
+	"
+    }
 }
 
 proc job-set-status {flight job st} {
@@ -221,38 +221,36 @@ proc lock-tables {tables} {
 proc spawn-step-begin {flight job ts stepnovar} {
     upvar 1 $stepnovar stepno
 
-    db-open
-
-    db-execute BEGIN
-    db-execute "SET TRANSACTION ISOLATION LEVEL SERIALIZABLE"
-    if {[catch {
-        lock-tables flights
-	db-execute-array stepinfo "
-            SELECT max(stepno) AS maxstep FROM steps
-                WHERE flight=[pg_quote $flight] AND job=[pg_quote $job]
-        "
-        set stepno $stepinfo(maxstep)
-	if {[string length $stepno]} {
-	    incr stepno
-	} else {
-	    set stepno 1
+    with-db {
+	db-execute BEGIN
+	db-execute "SET TRANSACTION ISOLATION LEVEL SERIALIZABLE"
+	if {[catch {
+	    lock-tables flights
+	    db-execute-array stepinfo "
+		SELECT max(stepno) AS maxstep FROM steps
+		    WHERE flight=[pg_quote $flight] AND job=[pg_quote $job]
+	    "
+	    set stepno $stepinfo(maxstep)
+	    if {[string length $stepno]} {
+		incr stepno
+	    } else {
+		set stepno 1
+	    }
+	    db-execute "
+		INSERT INTO steps
+		    VALUES ([pg_quote $flight], [pg_quote $job], $stepno,
+			    [pg_quote $ts], 'running',
+			    'STARTING')
+	    "
+	    db-execute COMMIT
+	} emsg]} {
+	    global errorInfo errorCode
+	    set ei $errorInfo
+	    set ec $errorCode
+	    catch { db-execute ROLLBACK }
+	    error $emsg $ei $ec
 	}
-	db-execute "
-            INSERT INTO steps
-                VALUES ([pg_quote $flight], [pg_quote $job], $stepno,
-                        [pg_quote $ts], 'running',
-                        'STARTING')
-        "
-	db-execute COMMIT
-    } emsg]} {
-	global errorInfo errorCode
-	set ei $errorInfo
-	set ec $errorCode
-	catch { db-execute ROLLBACK }
-        db-close
-	error $emsg $ei $ec
     }
-    db-close
 }
 
 proc spawn-step-commit {flight job stepno testid} {
-- 
2.1.4


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