[OSSTEST PATCH 00/11] Test database handling

[OSSTEST PATCH 00/11] Test database handling

[Ian Jackson]

I'm intending to be able to do database schema updates.  But I don't
want to play around with such code on a live database.  So I need a
test database.

Thus, a yak: arrangements to make a test database.

I have tested this and it now does the right things in, apparently,
the right order, and seems to leave things in a good state even when
it collapses halfway or is ^C'd.

[OSSTEST PATCH 01/11] tcl daemons: log host and port number we bind to, at startup

[Ian Jackson]

If the socket setup fails, this makes it easier to see what the
program was trying to do.

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

diff --git a/tcl/daemonlib.tcl b/tcl/daemonlib.tcl
index 972b5e2..b76ff12 100644
--- a/tcl/daemonlib.tcl
+++ b/tcl/daemonlib.tcl
@@ -210,7 +210,7 @@ proc main-daemon {which setup} {
     fconfigure stdout -buffering line
     fconfigure stderr -buffering none
 
-    log "starting"
+    log "starting $host:$port"
 
     uplevel 1 $setup
 
-- 
1.7.10.4

[OSSTEST PATCH 02/11] cri-getconfig: Break out exec_resetting_sigint.

[Ian Jackson]

Move this oddity (and the associated comment) from
standalone-generate-dump-flight-runvars to cri-getconfig.  We are
going to want it elsewhere.

We put this in cri-getconfig because that is the one library of
generic shell functions which everything includes.  Perhaps this file
is misnamed.

No overall functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 cri-getconfig                           |   30 ++++++++++++++++++++++++++++++
 standalone-generate-dump-flight-runvars |   28 +---------------------------
 2 files changed, 31 insertions(+), 27 deletions(-)

diff --git a/cri-getconfig b/cri-getconfig
index 0589bf0..ee1cc40 100644
--- a/cri-getconfig
+++ b/cri-getconfig
@@ -39,3 +39,33 @@ getrepos() {
 	fi
 	echo $repos
 }
+
+# Good grief, handling background proceesses from shell is a pain.
+#
+# For stupid historical reasons, background processes start with
+# SIGINT (and QUIT) ignored (SuSv3 2.11).  bash does not currently
+# offer a way to ask it not to do this; nor is there a reliable way to
+# put the SIGINT handling back to normal.
+#
+# "trap - INT" can be used to put the handling back in recent versions
+# of bash (eg Debian jessie), but earlier versions are buggy, so
+# we use perl.
+#
+# However, there is still a race: if the signal arrives just after the
+# fork, after the shell has (in the child) set it to to IGN, but
+# before Perl has put it back, the child might still escape.
+# There is no reasonable way to deal with this race.  So the result
+# may still be slightly racy in the case that s-g-d-f-r is ^C'd right
+# after starting.
+#
+# Hopefully in the future we can fix this with something like
+# "shopt -s no_async_sig_ignore".  See
+# https://lists.gnu.org/archive/html/bug-bash/2015-10/msg00077.html
+#
+exec_resetting_sigint () {
+	exec perl -e '
+	        $SIG{$_}=DFL foreach qw(INT QUIT HUP);
+		kill 1, $$ unless getppid=='$$';
+		exec @ARGV or die $!;
+   	' "$@"
+}
diff --git a/standalone-generate-dump-flight-runvars b/standalone-generate-dump-flight-runvars
index e91026a..3b20c62 100755
--- a/standalone-generate-dump-flight-runvars
+++ b/standalone-generate-dump-flight-runvars
@@ -58,35 +58,9 @@ perbranch () {
     flight=check_${branch//[-._]/_}
 }
 
-# Good grief, handling background proceesses from shell is a pain.
-#
-# For stupid historical reasons, background processes start with
-# SIGINT (and QUIT) ignored (SuSv3 2.11).  bash does not currently
-# offer a way to ask it not to do this; nor is there a reliable way to
-# put the SIGINT handling back to normal.
-#
-# "trap - INT" can be used to put the handling back in recent versions
-# of bash (eg Debian jessie), but earlier versions are buggy, so
-# we use perl.
-#
-# However, there is still a race: if the signal arrives just after the
-# fork, after the shell has (in the child) set it to to IGN, but
-# before Perl has put it back, the child might still escape.
-# There is no reasonable way to deal with this race.  So the result
-# may still be slightly racy in the case that s-g-d-f-r is ^C'd right
-# after starting.
-#
-# Hopefully in the future we can fix this with something like
-# "shopt -s no_async_sig_ignore".  See
-# https://lists.gnu.org/archive/html/bug-bash/2015-10/msg00077.html
-
 for branch in $@; do
     perbranch
-    perl -e '
-        $SIG{$_}=DFL foreach qw(INT QUIT HUP);
-	kill 1, $$ unless getppid=='$$';
-	exec @ARGV or die $!;
-    ' \
+    exec_resetting_sigint \
     ./standalone make-flight -f $flight $branch >$log 2>&1 &
     procs+=" $branch=$!"
 done
-- 
1.7.10.4

[OSSTEST PATCH 03/11] Configuration: No longer set password=<~/.xen-osstest/db-password>

[Ian Jackson]

Instead, expect the user to provide ~/.pgpass.

This is a good idea because we don't really want to be handling
passwords ourselves if we can help it.  And, we are shortly going to
want to do some exciting mangling of the database access
configuration, which would be complicated by the presence of this
password expansion.

This may break for some users of existing Executive (non-standalone)
setups which are using production-config-cambridge or the default
built-in configuration.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 Osstest.pm                  |    3 +--
 production-config-cambridge |    2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/Osstest.pm b/Osstest.pm
index ec50d60..95e4d46 100644
--- a/Osstest.pm
+++ b/Osstest.pm
@@ -191,8 +191,7 @@ sub readglobalconfig () {
 
     # dynamic default config settings
     $c{ExecutiveDbnamePat} ||= "dbname=<dbname>;user=<whoami>;".
-	"host=<dbname>.db.$c{DnsDomain};".
-	"password=<~/.xen-osstest/db-password>"
+	"host=<dbname>.db.$c{DnsDomain}"
 	if defined $c{DnsDomain};
     # 1. <\w+> is replaced with variables:
     #         <dbname>    database name
diff --git a/production-config-cambridge b/production-config-cambridge
index f801303..412766c 100644
--- a/production-config-cambridge
+++ b/production-config-cambridge
@@ -23,7 +23,7 @@ HostDB_Executive_NoConfigDB 1
 
 OwnerDaemonHost owner.daemon.osstest.xs.citrite.net
 QueueDaemonHost queue.daemon.osstest.xs.citrite.net
-ExecutiveDbnamePat dbname=<dbname>;user=<whoami>;host=osstestdb.xs.citrite.net;password=<~/.xen-osstest/db-password>
+ExecutiveDbnamePat dbname=<dbname>;user=<whoami>;host=osstestdb.xs.citrite.net
 
 HostnameSortSwapWords 1
 
-- 
1.7.10.4

[OSSTEST PATCH 04/11] mg-debug-fail: New utility script for debugging

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 mg-debug-fail |   13 +++++++++++++
 1 file changed, 13 insertions(+)
 create mode 100755 mg-debug-fail

diff --git a/mg-debug-fail b/mg-debug-fail
new file mode 100755
index 0000000..64fa235
--- /dev/null
+++ b/mg-debug-fail
@@ -0,0 +1,13 @@
+#!/bin/sh
+#
+# This script can be provided anywhere an executable or command name is
+# wanted.  It prints its arguments, and its stdin, to its stderr, and
+# then exits nonzero.
+#
+# When using this it may be useful to provide </dev/null as a
+# redirection for the whole program under test.  Otherwise things
+# can mysteriously hang.
+
+bash -xc ': mg-debug-fail "$@"' x "$@"
+egrep . >&2
+exit 127
-- 
1.7.10.4

[OSSTEST PATCH 05/11] mg-debug-fail: Catch attempts to read from a tty

[Ian Jackson]

When stdin is a tty, do not try to dump it.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 mg-debug-fail |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/mg-debug-fail b/mg-debug-fail
index 64fa235..ffe9f50 100755
--- a/mg-debug-fail
+++ b/mg-debug-fail
@@ -3,10 +3,10 @@
 # This script can be provided anywhere an executable or command name is
 # wanted.  It prints its arguments, and its stdin, to its stderr, and
 # then exits nonzero.
-#
-# When using this it may be useful to provide </dev/null as a
-# redirection for the whole program under test.  Otherwise things
-# can mysteriously hang.
+
+if tty >/dev/null 2>&1; then
+	exec </dev/null
+fi
 
 bash -xc ': mg-debug-fail "$@"' x "$@"
 egrep . >&2
-- 
1.7.10.4

[OSSTEST PATCH 06/11] cri-getconfig: Provide get_psql_cmd and get_pgdump_cmd

[Ian Jackson]

This is for (non-standalone-mode) shell scripts which want to access
the postgresql database.

get_psql_command provides `-v ON_ERROR_STOP' because it is not the
default (!) and no sane caller would not want it.

No callers as yet.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 cri-getconfig |   31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/cri-getconfig b/cri-getconfig
index ee1cc40..48528b5 100644
--- a/cri-getconfig
+++ b/cri-getconfig
@@ -40,6 +40,37 @@ getrepos() {
 	echo $repos
 }
 
+get_psql_cmd () {
+	perl -we '
+	use Osstest;
+	use Osstest::Executive;
+	use DBI;
+	csreadconfig();
+	print "psql",
+	     " -d ", $dbh_tests->{pg_db},
+	     " -h ", $dbh_tests->{pg_host},
+	     " -p ", $dbh_tests->{pg_port},
+	     " -U ", $dbh_tests->{pg_user},
+	     " -v ON_ERROR_STOP=1\n"
+	     or die $!;
+'
+}
+
+get_pgdump_cmd () {
+	perl -we '
+	use Osstest;
+	use Osstest::Executive;
+	use DBI;
+	csreadconfig();
+	print "pg_dump",
+	     " -h ", $dbh_tests->{pg_host},
+	     " -p ", $dbh_tests->{pg_port},
+	     " -U ", $dbh_tests->{pg_user},
+	     " ",    $dbh_tests->{pg_db}, "\n"
+	     or die $!;
+'
+}
+
 # Good grief, handling background proceesses from shell is a pain.
 #
 # For stupid historical reasons, background processes start with
-- 
1.7.10.4

[OSSTEST PATCH 07/11] cri-getconfig: Provide debugging for get_psql_cmd

[Ian Jackson]

This allows us to execute only the first <some number> SQL
invocations.  The first non-executed one is dumped, instead, by having
get_psql_command print a rune involving ./mg-debug-fail (which the
caller will then execute).

The locking makes things work roughly-correctly if get_psql_cmd is run
in multiple processes at once: it is not defined exactly which
invocations get which counter values, but they will all work properly
and get exactly one counter value each.

If set -x is in force, turn it off for get_psql_cmd: our perl rune is
uninteresting to see repeated ad infinitum in debugging output.

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

diff --git a/cri-getconfig b/cri-getconfig
index 48528b5..a357d86 100644
--- a/cri-getconfig
+++ b/cri-getconfig
@@ -41,6 +41,22 @@ getrepos() {
 }
 
 get_psql_cmd () {
+	# To use this:
+	#  on each test run, rm -f t.psql-counter
+	#  and set OSSTEST_PSQL_ONLY_DO to an integer
+	if [ "x$OSSTEST_PSQL_ONLY_DO" != x ]; then
+		local f=t.psql-counter
+		psql_counter=$( with-lock-ex -w $f.lock bash -ec '
+			psql_counter=$(cat '$f' || echo 0)
+			echo $(( $psql_counter + 1 )) >'$f'.tmp
+			mv -f '$f'.tmp '$f'
+			echo $psql_counter' )
+		if ! [ $psql_counter -lt $OSSTEST_PSQL_ONLY_DO ]; then
+			printf './mg-debug-fail '
+		fi
+	fi
+
+	set +x
 	perl -we '
 	use Osstest;
 	use Osstest::Executive;
-- 
1.7.10.4

[OSSTEST PATCH 08/11] Osstest.pm: Break out and export globalconfigfiles

[Ian Jackson]

No functional change; no callers as yet.

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

diff --git a/Osstest.pm b/Osstest.pm
index 95e4d46..20b8f62 100644
--- a/Osstest.pm
+++ b/Osstest.pm
@@ -30,7 +30,7 @@ BEGIN {
     @ISA         = qw(Exporter);
     @EXPORT      = qw(
                       readglobalconfig %c $mjobdb $mhostdb
-                      augmentconfigdefaults
+                      augmentconfigdefaults globalconfigfiles
                       csreadconfig
                       getmethod
                       postfork
@@ -124,6 +124,10 @@ sub getmethod {
     return $r;
 }
 
+sub globalconfigfiles () {
+	$ENV{'OSSTEST_CONFIG'} || "$ENV{'HOME'}/.xen-osstest/config";
+}
+
 sub readglobalconfig () {
     our $readglobalconfig_done;
     return if $readglobalconfig_done;
@@ -135,7 +139,7 @@ sub readglobalconfig () {
     $c{AuthorizedKeysFiles} = '';
     $c{AuthorizedKeysAppend} = '';
 
-    my $cfgfiles = $ENV{'OSSTEST_CONFIG'} || "$ENV{'HOME'}/.xen-osstest/config";
+    my $cfgfiles = globalconfigfiles();
 
     my $readcfg;
     $readcfg = sub ($$) {
-- 
1.7.10.4

[OSSTEST PATCH 09/11] mg-schema-test-database: New script

[Ian Jackson]

This allows a user in non-standalone mode to make a whole new test
database, which is largely a clone of the original database.

The new db refers to the same resources (hosts), and more-or-less
safely borrows some of those hosts.

Currently we don't do anything about the queue and owner daemons.
This means that queue-daemon-based resource allocation is broken when
clients are pointed at the test db.  But non-queue-based allocation
(eg, ./mg-allocate without -U) works, and the test db can be used for
db-related experiments and even support individual ts-* scripts (other
than ts-hosts-allocate of course).

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

diff --git a/mg-schema-test-database b/mg-schema-test-database
new file mode 100755
index 0000000..64acdcf
--- /dev/null
+++ b/mg-schema-test-database
@@ -0,0 +1,458 @@
+#!/bin/bash
+#
+# usages:
+#
+#
+#  ./mg-schema-test-database create [_SUFFIX] [TASK...] \
+#		[-fMINFLIGHT | -f-NUMFLIGHTS]
+#
+# does `drop' and then creates
+#   - the database    osstestdb_test_SUFFIX
+#   - a file          local-config.test-database_SUFFIX
+#
+# default for SUFFIX is your local username
+#
+# Resources owned in the main db by a task in the list of specified
+# TASKs become idle in the test copy.  Others become allocated to
+# a specially-created `owned by someone in real db' task.
+#
+#
+#  ./mg-schema-test-database drop [_SUFFIX]
+#
+# deletes your test database and removes the local-config file
+#
+#
+
+set -e -o posix ${OSSTEST_DEBUG:+-x}
+
+. ./cri-getconfig
+. ./mgi-common
+
+if [ $# -lt 1 ]; then fail "need operation"; fi
+
+cmd="$1"; shift
+
+localconfig=local-config.test-database
+
+maindbname=$(perl -we '
+	use Osstest;
+	use Osstest::Executive;
+	use DBI;
+	csreadconfig();
+	print $dbh_tests->{pg_db},"\n"
+	     or die $!;
+')
+
+parse_only_suffix () {
+	for arg in "$@"; do
+		case "$arg" in
+		_*)	suffix="$arg" ;;
+		*)	fail 'bad usage' ;;
+		esac
+	done
+}
+
+dbname () {
+	dbname="${maindbname}_test$suffix"
+	t="tmp/testdb$suffix"
+	tcfg=local-config.test-database$suffix
+}
+
+psql_query_internal () {
+	$(get_psql_cmd) -At -R' ' -f- "$@"
+}
+
+psql_query () {
+        if [ x$OSSTEST_DEBUG != x ]; then
+		tee /dev/stderr | psql_query_internal "$@"
+	else
+		psql_query_internal "$@"
+	fi
+}
+
+psql_do_cmd () {
+	echo "$(get_psql_cmd) ${OSSTEST_DEBUG:+-e -a}" 
+}
+
+psql_do () {
+	$(psql_do_cmd) -q -f- "$@"
+}
+
+moretasks () {
+	local ifnone="$1"; shift
+	local where="$1"; shift
+	local r
+	r=$(
+		psql_query "$@" <<END
+			SELECT taskid
+			  FROM tasks
+			 $where
+END
+	)
+	if [ "x$r" = x ]; then
+		local m="no tasks matched \`$arg'"
+		case $ifnone in
+		error)
+			fail "error: $m"
+			;;
+		warning)
+			echo >&2 "warning: $m"
+			;;
+		*)
+			fail-bad-moretasks-ifnone-"$ifnone"
+			;;
+		esac
+	fi
+
+	tasks+=" $r"
+}
+
+withtest () {
+	OSSTEST_CONFIG="$test_cfg_setting" "$@"
+}
+
+each_copy_table () {
+	local tab=$1; shift
+	local cond=$1; shift
+
+	p=$t.tabledata.$tab
+	rm -f $p
+
+	cat <<END >>$t.export
+		\\COPY (SELECT * FROM $tab WHERE $cond) TO $p $copyhow
+END
+	cat <<END >>$t.import
+		\\COPY $tab FROM $p $copyhow
+END
+}
+
+make_xdbref_task () {
+	local refkey=$1; shift
+	local comment=$1; shift
+	local refinfo=$1; shift
+	echo "
+		INSERT INTO tasks
+			(type, refkey, username, comment, live, refinfo)
+		  VALUES ('xdbref','$refkey','$username@$nodename',
+			  '$comment','t','$refinfo');
+	"
+}
+
+taskid () {
+	local type=$1; shift
+	local refkey=$1; shift
+	local xcond=$1
+	echo "(SELECT taskid FROM tasks WHERE
+		type='$type' AND refkey='$refkey' $xcond)"
+}
+borrowtaskid () {
+	local bt=$1
+	taskid xdbref $dbname "
+		AND live AND refinfo IS NOT NULL AND refinfo='$bt'
+	"
+}
+
+username=`whoami`
+nodename=`uname -n`
+suffix=_$username
+invocation_now=`date +%s`
+
+case "$cmd" in
+
+#========== CREATE ==========
+
+create)
+	#---------- argument parsing ----------
+
+	tasks=''
+	minflight=-1000
+	for arg in "$@"; do
+		case "$arg" in
+		*@*)
+			moretasks warning			\
+				"WHERE type = 'static'
+				   AND refkey LIKE :'pattern'"	\
+				-v pattern="${arg//\*/%}"	\
+			;;
+		*" "*" "*)
+			local rhs="${arg#* }"
+			moretasks error				\
+				"WHERE taskid = :'taskid'
+				   AND type = :'type'
+				   AND refkey = :'refkey'"	\
+				-v taskid="${arg%% *}"		\
+				-v type="${rhs%% *}"		\
+				-v refkey="${rhs#* }"
+			;;
+		_)	suffix="$arg"
+			;;
+		-f*)	minflight="${arg#-f}"
+			;;
+		*)	fail "bad arg to create"
+			;;
+		esac
+	done
+
+	if [ "x$tasks" = x ]; then
+		moretasks error					\
+			"WHERE type = 'static'
+			   AND refkey = :'refkey'"		\
+			-v refkey="$(whoami)@$(uname -n)"
+	fi
+
+	tasks_cond=${tasks// / OR T=}
+	tasks_cond=${tasks_cond# OR }
+
+	case "$minflight" in
+	-*)
+		minflight=$( psql_query <<END
+                        SELECT flight FROM
+                                (SELECT flight FROM flights
+                                        ORDER BY flight DESC
+                                        LIMIT ${minflight#-})
+				AS last
+                                ORDER BY FLIGHT ASC
+                                LIMIT 1
+END
+		)
+		;;
+	esac
+
+	#---------- preparation and data-gathering ----------
+
+	bad=$(	psql_query <<END
+		SELECT * FROM tasks WHERE (${tasks_cond//T/taskid})
+				AND NOT live
+END
+	)
+	case "$bad" in
+	*[0-9]*)
+		fail "Borrowing from NON-LIVE TASKS $bad"
+		;;
+	esac
+
+	# drop any previous test db
+	"$0" drop $suffix
+
+	dbname
+
+	printf "Setting up %s (minflight=%d, tasks=%s)...\n" \
+		$dbname "$minflight" "${tasks# }"
+
+	# create the config overlay
+	perl >$tcfg.tmp -we '
+		use Osstest;
+		use Osstest::Executive;
+		use DBI;
+		csreadconfig();
+		print "ExecutiveDbname_osstestdb ".
+			"dbname='$dbname';".
+			"host=$dbh_tests->{pg_host};".
+			"user=$dbh_tests->{pg_user};".
+			"port=$dbh_tests->{pg_port}\n"
+			or die $!;
+	'
+	cat >>$tcfg.tmp <<END
+OwnerDaemonHost $ctrlhost
+QueueDaemonHost $ctrlhost
+OwnerDaemonPort ${ctrlports%,*}
+QueueDaemonPort ${ctrlports#*,}
+END
+	mv -f $tcfg.tmp $tcfg
+
+	# this makes `withtest' work
+	test_cfg_setting="$(perl -we '
+			use Osstest;
+			print globalconfigfiles() or die $!;
+		'):$tcfg"
+
+	# Extract the schema for reference
+	$(get_pgdump_cmd) -s -O -x >$t.schema
+
+	# Keep a copy as it came from dump, for comparison
+	cp $t.schema $t.schema.orig
+
+	# http://www.postgresql.org/message-id/26790.1306355327@sss.pgh.pa.us
+	perl -i~ -pe '
+		s/^/--/ if
+			m/^CREATE EXTENSION IF NOT EXISTS plpgsql / ||
+			m/^COMMENT ON EXTENSION plpgsql /;
+	' $t.schema
+
+	printf "Tables:"
+
+	# What tables are there ?
+	perl -ne <$t.schema >$t.tablevars '
+		if (m/^CREATE SEQUENCE (\w+)/) {
+			print "sequences+=\" $1\"\n";
+		} elsif (m/^CREATE TABLE (\w+)/) {
+			$table=$1;
+		} elsif (m/^\s*flight\s+integer/) {
+			print "ftables+=\" $table\"\n";
+		} elsif ($table && m/^\)\;$/) {
+			print "tables+=\" $table\"\n";
+		}
+	'
+
+	. $t.tablevars
+
+	>$t.tablesortlist
+
+	for table in $tables; do
+		LC_MESSAGES=C $(get_psql_cmd) <<END >$t.display.$table
+			\d $table
+END
+		echo >>$t.tablesortlist "$table $table"
+		perl -ne <$t.display.$table >>$t.tablesortlist '
+			next unless m/^Foreign-key constraints:/ ... m/^\S/;
+			next if m/DEFERRABLE/;
+			next unless m/FOREIGN KEY.*REFERENCES (\w+)/;
+			print "$1 '"$table"'\n" or die $!;
+		'
+		printf " $table"
+	done
+
+	tables=$(tsort <$t.tablesortlist)
+
+	# We don't want to set the permissions
+	perl <executive-postgresql-schema >$t.new-schema -pe '
+		s/^/--/ if
+			m/^ALTER TABLE .* OWNER TO / ||
+			m/^GRANT |^REVOKE /
+	'
+
+	#---------- create test db ----------
+
+	psql_do <<END
+		CREATE DATABASE $dbname;
+END
+	$(withtest get_psql_cmd) -q -f $t.new-schema
+
+	printf ".\n"
+
+	# Schema should now be identical to main DB
+	$(withtest get_pgdump_cmd) -s -O -x >$t.schema.created
+	diff -u $t.schema.orig $t.schema.created
+
+	#---------- mark resources that we are going to borrow ----------
+
+	for task in $tasks; do
+		psql_do <<END
+			BEGIN;
+			$(make_xdbref_task $dbname 'borrowed for test db' $task)
+			UPDATE resources SET owntaskid = $(borrowtaskid $task)
+				WHERE owntaskid=$task;
+			COMMIT;
+END
+	done
+
+	#---------- copy data from live to test db ----------
+
+	copyhow="CSV HEADER NULL e'\\\\n'"
+
+	rm -f $t.import $t.export
+
+	cat >>$t.import <<END
+		\o $t.import-output
+		BEGIN;
+		SET CONSTRAINTS ALL DEFERRED;
+END
+
+	$(get_pgdump_cmd) -a -O -x ${sequences// / -t } >$t.sequences-import
+	perl <$t.sequences-import >>$t.import -ne '
+		next if m/^--/;
+		next if m/^SET /;
+		next unless m/\S/;
+		print or die $!;
+	'
+
+	for table in $tables; do
+		case " $ftables " in
+		*" $table "*)	condition="flight >= $minflight" ;;
+		*)		condition="1=1" ;;
+		esac
+		each_copy_table $table "$condition"
+	done
+
+	# As we copy, we note everything we're not borrowing as
+	# belonging to the parent db.
+	cat >>$t.import <<END
+		$(make_xdbref_task $maindbname 'not borrowed' '')
+		UPDATE resources
+			SET owntaskid = $(taskid xdbref $maindbname)
+			WHERE owntaskid != $(borrowtaskid $task);
+		COMMIT;
+END
+
+	printf "Copy..."
+
+	printf "export..."
+	$(psql_do_cmd) -f $t.export
+
+	printf "import..."
+	$(withtest psql_do_cmd) -f $t.import
+
+	rm -f $t.tabledata.*
+
+	#---------- actually borrow resources ----------
+
+	printf "borrow..."
+
+	for task in $tasks; do
+		withtest psql_do <<END
+			BEGIN;
+			UPDATE resources
+				SET owntaskid = $(taskid magic idle)
+				WHERE owntaskid = $(borrowtaskid $task);
+			COMMIT;
+END
+	done
+	withtest psql_do <<END
+		DELETE FROM tasks
+			WHERE type='xdbref' AND refkey='$dbname';
+END
+
+	printf "\n"
+
+	cat <<END
+Test database $dbname now set up.
+export OSSTEST_CONFIG=$test_cfg_setting
+END
+	;;
+
+#========== DROP ==========
+
+drop)
+	parse_only_suffix "$@"
+
+	dbname
+
+	printf "Dropping %s.\n" "$dbname"
+
+	psql_do <<END
+                SET client_min_messages = WARNING;
+		DROP DATABASE IF EXISTS $dbname;
+		UPDATE resources
+			SET owntaskid = CAST(tasks.refinfo AS INTEGER)
+			FROM tasks
+			WHERE resources.owntaskid = tasks.taskid
+                          AND tasks.type = 'xdbref'
+			  AND tasks.refkey = '$dbname'
+			  AND tasks.live
+			  AND tasks.refinfo IS NOT NULL;
+		UPDATE tasks
+			SET live = 'f'
+			WHERE tasks.type = 'xdbref'
+			  AND tasks.refkey = '$dbname';
+END
+
+	rm -f $tcfg
+
+	;;
+
+#========== EPILOGUE ==========
+
+*)
+	fail "unknown operation \`$cmd'"
+	;;
+esac
-- 
1.7.10.4

[OSSTEST PATCH 10/11] mg-schema-test-database: Move setting of test_cfg_setting to dbname

[Ian Jackson]

This will makes it available to a wider subset of the script, which is
going to be important in a moment.

No functional change.

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

diff --git a/mg-schema-test-database b/mg-schema-test-database
index 64acdcf..fec7f20 100755
--- a/mg-schema-test-database
+++ b/mg-schema-test-database
@@ -56,6 +56,12 @@ dbname () {
 	dbname="${maindbname}_test$suffix"
 	t="tmp/testdb$suffix"
 	tcfg=local-config.test-database$suffix
+
+	test_cfg_setting="$(perl -we '
+			use Osstest;
+			print globalconfigfiles() or die $!;
+		'):$tcfg"
+
 }
 
 psql_query_internal () {
@@ -260,12 +266,6 @@ QueueDaemonPort ${ctrlports#*,}
 END
 	mv -f $tcfg.tmp $tcfg
 
-	# this makes `withtest' work
-	test_cfg_setting="$(perl -we '
-			use Osstest;
-			print globalconfigfiles() or die $!;
-		'):$tcfg"
-
 	# Extract the schema for reference
 	$(get_pgdump_cmd) -s -O -x >$t.schema
 
-- 
1.7.10.4

[OSSTEST PATCH 11/11] mg-schema-test-database: Sort out daemons; provide `daemons' subcommand

[Ian Jackson]

We arrange for the test configuration to look for the daemons on a
different host and port, and we provide a convenient way to run such a
pair of daemons.

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

diff --git a/mg-schema-test-database b/mg-schema-test-database
index fec7f20..73d92f3 100755
--- a/mg-schema-test-database
+++ b/mg-schema-test-database
@@ -4,7 +4,8 @@
 #
 #
 #  ./mg-schema-test-database create [_SUFFIX] [TASK...] \
-#		[-fMINFLIGHT | -f-NUMFLIGHTS]
+#		[-fMINFLIGHT | -f-NUMFLIGHTS] \
+#		[-hCTRL_DAEMONS_HOST] [-fOWNER_D_PORT[,QUEUE_D_PORT]]
 #
 # does `drop' and then creates
 #   - the database    osstestdb_test_SUFFIX
@@ -16,12 +17,24 @@
 # TASKs become idle in the test copy.  Others become allocated to
 # a specially-created `owned by someone in real db' task.
 #
+# Default for CTRL_DAEMONS_HOST is localhost; if no port is supplied,
+# we use the corresponding production port + 2; if only one port is
+# supplied we use that and the next port number.
+#
 #
 #  ./mg-schema-test-database drop [_SUFFIX]
 #
 # deletes your test database and removes the local-config file
 #
 #
+#  ./mg-schema-test-database daemons [_SUFFIX]
+#
+# synchronously runs owner and queue daemons for your test database
+#
+# NB that you can't drop a test database with these daemons running,
+# because Postgres will refuse to drop a database that anyone is
+# connected to.
+
 
 set -e -o posix ${OSSTEST_DEBUG:+-x}
 
@@ -114,6 +127,9 @@ END
 }
 
 withtest () {
+	if ! [ -e "$tcfg" ]; then
+		fail "test $dbname not set up ($tcfg does not exist)"
+	fi
 	OSSTEST_CONFIG="$test_cfg_setting" "$@"
 }
 
@@ -194,6 +210,10 @@ create)
 			;;
 		-f*)	minflight="${arg#-f}"
 			;;
+		-h*)	ctrlhost="${arg#-h}"
+			;;
+		-p*)	ctrlports="${arg#-p}"
+			;;
 		*)	fail "bad arg to create"
 			;;
 		esac
@@ -224,6 +244,18 @@ END
 		;;
 	esac
 
+	if [ "x$ctrlhost" = x ]; then
+		ctrlhost=localhost
+	fi
+	case "$ctrlports" in
+		*,*)	;;
+		?*)	ctrlports+=,$(( $ctrlports + 1 )) ;;
+		'')
+ ctrlports=$(( $(getconfig OwnerDaemonPort) + 2)),$((
+ 		$(getconfig QueueDaemonPort) + 2))
+ 			;;
+	esac
+
 	#---------- preparation and data-gathering ----------
 
 	bad=$(	psql_query <<END
@@ -244,6 +276,8 @@ END
 
 	printf "Setting up %s (minflight=%d, tasks=%s)...\n" \
 		$dbname "$minflight" "${tasks# }"
+	printf "Configuring for any daemons to be on %s:%s.\n" \
+		$ctrlhost $ctrlports
 
 	# create the config overlay
 	perl >$tcfg.tmp -we '
@@ -450,6 +484,27 @@ END
 
 	;;
 
+#========== DAEMONS ==========
+
+daemons)
+	parse_only_suffix "$@"
+
+	dbname
+
+	printf "Running daemons for %s....\n" "$dbname"
+
+	withtest \
+	exec_resetting_sigint ./ms-ownerdaemon &
+
+	sleep 1
+
+	withtest \
+	exec_resetting_sigint ./ms-queuedaemon &
+
+	wait
+
+	;;
+
 #========== EPILOGUE ==========
 
 *)
-- 
1.7.10.4

Re: [OSSTEST PATCH 01/11] tcl daemons: log host and port number we bind to, at startup

[Ian Campbell]

On Fri, 2015-12-04 at 19:35 +0000, Ian Jackson wrote:
> If the socket setup fails, this makes it easier to see what the
> program was trying to do.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

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

Re: [OSSTEST PATCH 02/11] cri-getconfig: Break out exec_resetting_sigint.

[Ian Campbell]

On Fri, 2015-12-04 at 19:35 +0000, Ian Jackson wrote:
> Move this oddity (and the associated comment) from
> standalone-generate-dump-flight-runvars to cri-getconfig.  We are
> going to want it elsewhere.
> 
> We put this in cri-getconfig because that is the one library of
> generic shell functions which everything includes.  Perhaps this file
> is misnamed.
> 
> No overall functional change.
> 
> 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

Re: [OSSTEST PATCH 03/11] Configuration: No longer set password=<~/.xen-osstest/db-password>

[Ian Campbell]

On Fri, 2015-12-04 at 19:35 +0000, Ian Jackson wrote:
> Instead, expect the user to provide ~/.pgpass.
> 
> This is a good idea because we don't really want to be handling
> passwords ourselves if we can help it.  And, we are shortly going to
> want to do some exciting mangling of the database access
> configuration, which would be complicated by the presence of this
> password expansion.
> 
> This may break for some users of existing Executive (non-standalone)
> setups which are using production-config-cambridge or the default
> built-in configuration.

I think you need to create a ~osstest/.pgpass in Cambridge before deploying
this.

It seems like there ought to be some docs update, but the existing bit
about DbnamePat in README doesn't mention password already, so I guess that
is ok!

Assuming Cambridge's .pgpass is created:
    Acked-by: Ian Campbell <    ian.campbell@citrix.com    >

My personal config seems OK already. I've just moved aside what I think is
a stale db-password so I'll know for sure next time I have play.

We should delete ~osstest/.xen-osstest/db-password once this passes the
Cambridge pushgate, otherwise we'll be confused in 6 months looking at it.

I've added some CC's based on:

osstestdb=> SELECT DISTINCT username FROM tasks;
     username     
------------------
 
 rogerpau@osstest
 ijc@woking
 iwj@osstest
 iwj@woking
 ianc@kazak
 osstest@osstest
 julieng@osstest
 ianc@woking
 ianj@osstest
 iwj@mariner
 
 ianc@osstest
(13 rows)

osstestdb=> 

FYI a reasonable .pgpass is:
osstestdb.xs.citrite.net:*:*:<<USERNAME>>:<<YOUR PASSWORD>>
osstestdb:*:*:<<USERNAME>>:<<YOUR PASSWORD>>


> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  Osstest.pm                  |    3 +--
>  production-config-cambridge |    2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/Osstest.pm b/Osstest.pm
> index ec50d60..95e4d46 100644
> --- a/Osstest.pm
> +++ b/Osstest.pm
> @@ -191,8 +191,7 @@ sub readglobalconfig () {
>  
>      # dynamic default config settings
>      $c{ExecutiveDbnamePat} ||= "dbname=<dbname>;user=<whoami>;".
> -	"host=<dbname>.db.$c{DnsDomain};".
> -	"password=<~/.xen-osstest/db-password>"
> +	"host=<dbname>.db.$c{DnsDomain}"
>  	if defined $c{DnsDomain};
>      # 1. <\w+> is replaced with variables:
>      #         <dbname>    database name
> diff --git a/production-config-cambridge b/production-config-cambridge
> index f801303..412766c 100644
> --- a/production-config-cambridge
> +++ b/production-config-cambridge
> @@ -23,7 +23,7 @@ HostDB_Executive_NoConfigDB 1
>  
>  OwnerDaemonHost owner.daemon.osstest.xs.citrite.net
>  QueueDaemonHost queue.daemon.osstest.xs.citrite.net
> -ExecutiveDbnamePat
> dbname=<dbname>;user=<whoami>;host=osstestdb.xs.citrite.net;password=<~/.
> xen-osstest/db-password>
> +ExecutiveDbnamePat
> dbname=<dbname>;user=<whoami>;host=osstestdb.xs.citrite.net
>  
>  HostnameSortSwapWords 1
>  

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

Re: [OSSTEST PATCH 04/11] mg-debug-fail: New utility script for debugging

[Ian Campbell]

On Fri, 2015-12-04 at 19:35 +0000, Ian Jackson wrote:
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  mg-debug-fail |   13 +++++++++++++
>  1 file changed, 13 insertions(+)
>  create mode 100755 mg-debug-fail
> 
> diff --git a/mg-debug-fail b/mg-debug-fail
> new file mode 100755
> index 0000000..64fa235
> --- /dev/null
> +++ b/mg-debug-fail
> @@ -0,0 +1,13 @@
> +#!/bin/sh
> +#
> +# This script can be provided anywhere an executable or command name is
> +# wanted.  It prints its arguments, and its stdin, to its stderr, and
> +# then exits nonzero.
> +#
> +# When using this it may be useful to provide </dev/null as a
> +# redirection for the whole program under test.  Otherwise things
> +# can mysteriously hang.

"egrep . - /dev/null" is too noisy, it adds a "(standard input):" prefix
which I don't think you want. But "egrep -h . - /dev/null" seems to remedy
this without the possibility of these mysterious hangs.

What do you think?

Also, what does egrep give us here over just cat?

> +
> +bash -xc ': mg-debug-fail "$@"' x "$@"
> +egrep . >&2
> +exit 127

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

Re: [OSSTEST PATCH 05/11] mg-debug-fail: Catch attempts to read from a tty

[Ian Campbell]

On Fri, 2015-12-04 at 19:35 +0000, Ian Jackson wrote:
> When stdin is a tty, do not try to dump it.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

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

> ---
>  mg-debug-fail |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/mg-debug-fail b/mg-debug-fail
> index 64fa235..ffe9f50 100755
> --- a/mg-debug-fail
> +++ b/mg-debug-fail
> @@ -3,10 +3,10 @@
>  # This script can be provided anywhere an executable or command name is
>  # wanted.  It prints its arguments, and its stdin, to its stderr, and
>  # then exits nonzero.
> -#
> -# When using this it may be useful to provide </dev/null as a
> -# redirection for the whole program under test.  Otherwise things
> -# can mysteriously hang.
> +
> +if tty >/dev/null 2>&1; then
> +	exec </dev/null
> +fi
>  
>  bash -xc ': mg-debug-fail "$@"' x "$@"
>  egrep . >&2

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

Re: [OSSTEST PATCH 06/11] cri-getconfig: Provide get_psql_cmd and get_pgdump_cmd

[Ian Campbell]

On Fri, 2015-12-04 at 19:35 +0000, Ian Jackson wrote:
> This is for (non-standalone-mode) shell scripts which want to access
> the postgresql database.
> 
> get_psql_command provides `-v ON_ERROR_STOP' because it is not the
> default (!) and no sane caller would not want it.
> 
> No callers as yet.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

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

(one query below)

> ---
>  cri-getconfig |   31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/cri-getconfig b/cri-getconfig
> index ee1cc40..48528b5 100644
> --- a/cri-getconfig
> +++ b/cri-getconfig
> @@ -40,6 +40,37 @@ getrepos() {
>  	echo $repos
>  }
>  
> +get_psql_cmd () {
> +	perl -we '
> +	use Osstest;
> +	use Osstest::Executive;
> +	use DBI;
> +	csreadconfig();
> +	print "psql",
> +	     " -d ", $dbh_tests->{pg_db},
> +	     " -h ", $dbh_tests->{pg_host},
> +	     " -p ", $dbh_tests->{pg_port},
> +	     " -U ", $dbh_tests->{pg_user},

I suppose none of those can (or at least should) contain any weird quote-
requiring chars etc?

>  # Good grief, handling background proceesses from shell is a pain.

I just spotted a typo here "proceesses".

Ian.

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

Re: [OSSTEST PATCH 07/11] cri-getconfig: Provide debugging for get_psql_cmd

[Ian Campbell]

On Fri, 2015-12-04 at 19:35 +0000, Ian Jackson wrote:
> This allows us to execute only the first <some number> SQL
> invocations.  The first non-executed one is dumped, instead, by having
> get_psql_command print a rune involving ./mg-debug-fail (which the
> caller will then execute).
> 
> The locking makes things work roughly-correctly if get_psql_cmd is run
> in multiple processes at once: it is not defined exactly which
> invocations get which counter values, but they will all work properly
> and get exactly one counter value each.
> 
> If set -x is in force, turn it off for get_psql_cmd: our perl rune is
> uninteresting to see repeated ad infinitum in debugging output.

I should probably know this, but what is the scope of the set +x here? Is
it confined to get_psql_cmd or will callers find things going surprisingly
quiet?

Experimentally I think it is the latter, which is a shame. Maybe just wrap
the set+perl in ()s ?

> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  cri-getconfig |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/cri-getconfig b/cri-getconfig
> index 48528b5..a357d86 100644
> --- a/cri-getconfig
> +++ b/cri-getconfig
> @@ -41,6 +41,22 @@ getrepos() {
>  }
>  
>  get_psql_cmd () {
> +	# To use this:
> +	#  on each test run, rm -f t.psql-counter
> +	#  and set OSSTEST_PSQL_ONLY_DO to an integer
> +	if [ "x$OSSTEST_PSQL_ONLY_DO" != x ]; then
> +		local f=t.psql-counter
> +		psql_counter=$( with-lock-ex -w $f.lock bash -ec '
> +			psql_counter=$(cat '$f' || echo 0)
> +			echo $(( $psql_counter + 1 )) >'$f'.tmp
> +			mv -f '$f'.tmp '$f'
> +			echo $psql_counter' )
> +		if ! [ $psql_counter -lt $OSSTEST_PSQL_ONLY_DO ]; then
> +			printf './mg-debug-fail '
> +		fi
> +	fi
> +
> +	set +x
>  	perl -we '
>  	use Osstest;
>  	use Osstest::Executive;

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

Re: [OSSTEST PATCH 08/11] Osstest.pm: Break out and export globalconfigfiles

[Ian Campbell]

On Fri, 2015-12-04 at 19:35 +0000, Ian Jackson wrote:
> No functional change; no callers as yet.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

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

Re: [OSSTEST PATCH 09/11] mg-schema-test-database: New script

[Ian Campbell]

On Fri, 2015-12-04 at 19:35 +0000, Ian Jackson wrote:
> This allows a user in non-standalone mode to make a whole new test
> database, which is largely a clone of the original database.
> 
> The new db refers to the same resources (hosts), and more-or-less
> safely borrows some of those hosts.

"more-or-less" ? ;-)

What's the overall idiom for use here, something like:

Against the production DB:
OSSTEST_TASK=ianc@testing-something ./mg-allocate [-U ...] a-host

./mg-schema-test-database create [_SUFFIX] ianc@testing-something

Ending up in a state where a-host is allocated in the production db and
idle in this new test db and all other hosts are allocated in the test db
and left to go about their usual business in the production db?

> 
> Currently we don't do anything about the queue and owner daemons.
> This means that queue-daemon-based resource allocation is broken when
> clients are pointed at the test db.  But non-queue-based allocation
> (eg, ./mg-allocate without -U) works, and the test db can be used for
> db-related experiments and even support individual ts-* scripts (other
> than ts-hosts-allocate of course).
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  mg-schema-test-database |  458
> +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 458 insertions(+)
>  create mode 100755 mg-schema-test-database
> 
> diff --git a/mg-schema-test-database b/mg-schema-test-database
> new file mode 100755
> index 0000000..64acdcf
> --- /dev/null
> +++ b/mg-schema-test-database
> @@ -0,0 +1,458 @@

I glanced through this, but TBH I don't think picking through it line by
line will be very productive, I trust you've tested it and its ok...

Ian.

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

Re: [OSSTEST PATCH 10/11] mg-schema-test-database: Move setting of test_cfg_setting to dbname

[Ian Campbell]

On Fri, 2015-12-04 at 19:35 +0000, Ian Jackson wrote:
> This will makes it available to a wider subset of the script, which is
> going to be important in a moment.
> 
> No functional change.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

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

Re: [OSSTEST PATCH 11/11] mg-schema-test-database: Sort out daemons; provide `daemons' subcommand

[Ian Campbell]

On Fri, 2015-12-04 at 19:35 +0000, Ian Jackson wrote:
> We arrange for the test configuration to look for the daemons on a
> different host and port, and we provide a convenient way to run such a
> pair of daemons.

I was missing where *Daemon{Host,Port} were set here, it turns out to be in
patch 9, which might be a bisection issue? I'm not sure we care for this
new code.

It also occurred to me while reading this that I didn't know what to do
with OSSTEST_CONFIG here, I've not spotted in patch 9 where withtest()
behaves in the obvious way, but it might be worth mentioning explicitly in
the comments that the script DTRT and it is never(/rarely) necessary to set
OSSTEST_CONFIG=local-config.test-database_SUFFIX yourself.

But that's a comment on a previous patch, so:

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

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

Re: [OSSTEST PATCH 06/11] cri-getconfig: Provide get_psql_cmd and get_pgdump_cmd

[Ian Jackson]

Ian Campbell writes ("Re: [OSSTEST PATCH 06/11] cri-getconfig: Provide get_psql_cmd and get_pgdump_cmd"):
> On Fri, 2015-12-04 at 19:35 +0000, Ian Jackson wrote:
> > +get_psql_cmd () {
> > +	perl -we '
> > +	use Osstest;
> > +	use Osstest::Executive;
> > +	use DBI;
> > +	csreadconfig();
> > +	print "psql",
> > +	     " -d ", $dbh_tests->{pg_db},
> > +	     " -h ", $dbh_tests->{pg_host},
> > +	     " -p ", $dbh_tests->{pg_port},
> > +	     " -U ", $dbh_tests->{pg_user},
> 
> I suppose none of those can (or at least should) contain any weird quote-
> requiring chars etc?

Host and ports definitely can't.  I think if someone makes database
names and usernames with shell metacharacters in they deserve the
resulting pile of debris.

> >  # Good grief, handling background proceesses from shell is a pain.
> 
> I just spotted a typo here "proceesses".

I'll fix that.

Ian.

Re: [OSSTEST PATCH 07/11] cri-getconfig: Provide debugging for get_psql_cmd

[Ian Jackson]

Ian Campbell writes ("Re: [OSSTEST PATCH 07/11] cri-getconfig: Provide debugging for get_psql_cmd"):
> On Fri, 2015-12-04 at 19:35 +0000, Ian Jackson wrote:
> > If set -x is in force, turn it off for get_psql_cmd: our perl rune is
> > uninteresting to see repeated ad infinitum in debugging output.
> 
> I should probably know this, but what is the scope of the set +x here? Is
> it confined to get_psql_cmd or will callers find things going surprisingly
> quiet?

get_psql_cmd is generally used inside $( ), which limits the scope of
the +x.  Code which wraps get_psql_cmd might do some things after
calling it, but not very much.

> Experimentally I think it is the latter, which is a shame. Maybe just wrap
> the set+perl in ()s ?

That would improve things if anyone is likely to redirect
get_psql_cmd into a file, but that seems unlikely.  I'll do that if
you want, though.

Ian.

Re: [OSSTEST PATCH 03/11] Configuration: No longer set password=<~/.xen-osstest/db-password>

[Ian Jackson]

Ian Campbell writes ("Re: [OSSTEST PATCH 03/11] Configuration: No longer set password=<~/.xen-osstest/db-password>"):
> I think you need to create a ~osstest/.pgpass in Cambridge before deploying
> this.

Yes.  I have just done so, and checked that it works:

 (cam)iwj@osstest:~$ psql -d osstestdb -h osstestdb.xs.citrite.net
 psql (9.1.18)
 SSL connection (cipher: DHE-RSA-AES256-GCM-SHA384, bits: 256)
 Type "help" for help.

 osstestdb=> \q
 (cam)iwj@osstest:~$

> We should delete ~osstest/.xen-osstest/db-password once this passes the
> Cambridge pushgate, otherwise we'll be confused in 6 months looking at it.

Yes.  I will add a note to the commit message.

Thanks for the attention to detail.

Ian.

Re: [OSSTEST PATCH 04/11] mg-debug-fail: New utility script for debugging

[Ian Jackson]

Ian Campbell writes ("Re: [OSSTEST PATCH 04/11] mg-debug-fail: New utility script for debugging"):
> On Fri, 2015-12-04 at 19:35 +0000, Ian Jackson wrote:
> > diff --git a/mg-debug-fail b/mg-debug-fail
> > new file mode 100755
> > index 0000000..64fa235
> > --- /dev/null
> > +++ b/mg-debug-fail
> > @@ -0,0 +1,13 @@
> > +#!/bin/sh
> > +#
> > +# This script can be provided anywhere an executable or command name is
> > +# wanted.  It prints its arguments, and its stdin, to its stderr, and
> > +# then exits nonzero.
> > +#
> > +# When using this it may be useful to provide </dev/null as a
> > +# redirection for the whole program under test.  Otherwise things
> > +# can mysteriously hang.
> 
> "egrep . - /dev/null" is too noisy, it adds a "(standard input):" prefix
> which I don't think you want. But "egrep -h . - /dev/null" seems to remedy
> this without the possibility of these mysterious hangs.

This is confusing to me.  The problem is that mg-debug-fail does not
know whether to try to read and print its stdin.  Sometimes its stdin
will be the stdin for some utility that it is standing in for, and
should be dumped.  Whereas sometimes its stdin is the user's tty,
which should be left alone.

This difficulty is fixed in the next patch.  None of your suggestions
seem to address it.  What problem do you think they are solving ?

> Also, what does egrep give us here over just cat?

I meant to write   egrep ''
and its advantage is that it sanitises missing trailing newline.

But maybe we want to preserve missing trailing newline.

An alternative to replace the egrep would be
   sed 's/^/mg-debug-fail-stdin: /' >&2
which preserves missing trailing newline but annotates the output.

Ian.

Re: [OSSTEST PATCH 09/11] mg-schema-test-database: New script

[Ian Jackson]

Ian Campbell writes ("Re: [OSSTEST PATCH 09/11] mg-schema-test-database: New script"):
> On Fri, 2015-12-04 at 19:35 +0000, Ian Jackson wrote:
> > This allows a user in non-standalone mode to make a whole new test
> > database, which is largely a clone of the original database.
> > 
> > The new db refers to the same resources (hosts), and more-or-less
> > safely borrows some of those hosts.
> 
> "more-or-less" ? ;-)

This is a reference to the lack of owner/queue daemon handling.  (And
of course, the general limitations of `safety' in this kind of
context.)

> What's the overall idiom for use here, something like:
> 
> Against the production DB:
> OSSTEST_TASK=ianc@testing-something ./mg-allocate [-U ...] a-host
> 
> ./mg-schema-test-database create [_SUFFIX] ianc@testing-something
> 
> Ending up in a state where a-host is allocated in the production db and
> idle in this new test db and all other hosts are allocated in the test db
> and left to go about their usual business in the production db?

Exactly.  Maybe I should copy this to the doc comment :-).

> I glanced through this, but TBH I don't think picking through it line by
> line will be very productive, I trust you've tested it and its ok...

Due to being incompetent I've hammered each part of it quite a bit and
the all the resulting database states (even intermediate ones) look
good.

Ian.

Re: [OSSTEST PATCH 11/11] mg-schema-test-database: Sort out daemons; provide `daemons' subcommand

[Ian Jackson]

Ian Campbell writes ("Re: [OSSTEST PATCH 11/11] mg-schema-test-database: Sort out daemons; provide `daemons' subcommand"):
> On Fri, 2015-12-04 at 19:35 +0000, Ian Jackson wrote:
> > We arrange for the test configuration to look for the daemons on a
> > different host and port, and we provide a convenient way to run such a
> > pair of daemons.
> 
> I was missing where *Daemon{Host,Port} were set here, it turns out to be in
> patch 9, which might be a bisection issue? I'm not sure we care for this
> new code.

Indeed that hunk is misplaced.

> It also occurred to me while reading this that I didn't know what to do
> with OSSTEST_CONFIG here, I've not spotted in patch 9 where withtest()
> behaves in the obvious way, but it might be worth mentioning explicitly in
> the comments that the script DTRT and it is never(/rarely) necessary to set
> OSSTEST_CONFIG=local-config.test-database_SUFFIX yourself.

After you have run mg-schema-test-database create, you want to set
OSSTEST_CONFIG somehow.  Otherwise everything still runs against the
original database.

I do have a patch (unposted, because I thought better of it) to make
the config file reader automatically find and slurp any files called
local-config.something, after (and thus overriding OSSTEST_CONFIG0.
Do you think that would be a good idea ?  It seems a bit hairy to me.

Ian.

Re: [OSSTEST PATCH 07/11] cri-getconfig: Provide debugging for get_psql_cmd

[Ian Campbell]

On Mon, 2015-12-07 at 15:02 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [OSSTEST PATCH 07/11] cri-getconfig: Provide
> debugging for get_psql_cmd"):
> > On Fri, 2015-12-04 at 19:35 +0000, Ian Jackson wrote:
> > > If set -x is in force, turn it off for get_psql_cmd: our perl rune is
> > > uninteresting to see repeated ad infinitum in debugging output.
> > 
> > I should probably know this, but what is the scope of the set +x here?
> > Is
> > it confined to get_psql_cmd or will callers find things going
> > surprisingly
> > quiet?
> 
> get_psql_cmd is generally used inside $( ), which limits the scope of
> the +x.  Code which wraps get_psql_cmd might do some things after
> calling it, but not very much.
> 
> > Experimentally I think it is the latter, which is a shame. Maybe just
> > wrap
> > the set+perl in ()s ?
> 
> That would improve things if anyone is likely to redirect
> get_psql_cmd into a file, but that seems unlikely.  I'll do that if
> you want, though.

You make a good point about it usually being in $(), so no need.
Acked-by: Ian Campbell <ian.campbell@citrix.com>


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

Re: [OSSTEST PATCH 04/11] mg-debug-fail: New utility script for debugging

[Ian Campbell]

On Mon, 2015-12-07 at 15:10 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [OSSTEST PATCH 04/11] mg-debug-fail: New
> utility script for debugging"):
> > On Fri, 2015-12-04 at 19:35 +0000, Ian Jackson wrote:
> > > diff --git a/mg-debug-fail b/mg-debug-fail
> > > new file mode 100755
> > > index 0000000..64fa235
> > > --- /dev/null
> > > +++ b/mg-debug-fail
> > > @@ -0,0 +1,13 @@
> > > +#!/bin/sh
> > > +#
> > > +# This script can be provided anywhere an executable or command name
> > > is
> > > +# wanted.  It prints its arguments, and its stdin, to its stderr,
> > > and
> > > +# then exits nonzero.
> > > +#
> > > +# When using this it may be useful to provide </dev/null as a
> > > +# redirection for the whole program under test.  Otherwise things
> > > +# can mysteriously hang.
> > 
> > "egrep . - /dev/null" is too noisy, it adds a "(standard input):"
> > prefix
> > which I don't think you want. But "egrep -h . - /dev/null" seems to
> > remedy
> > this without the possibility of these mysterious hangs.
> 
> This is confusing to me.  The problem is that mg-debug-fail does not
> know whether to try to read and print its stdin.  Sometimes its stdin
> will be the stdin for some utility that it is standing in for, and
> should be dumped.  Whereas sometimes its stdin is the user's tty,
> which should be left alone.
> 
> This difficulty is fixed in the next patch.  None of your suggestions
> seem to address it.  What problem do you think they are solving ?

I think I got off on a bit of a tangent, and hadn't seen the next patch
yet.

> > Also, what does egrep give us here over just cat?
> 
> I meant to write   egrep ''
> and its advantage is that it sanitises missing trailing newline.
> 
> But maybe we want to preserve missing trailing newline.

I don't think so, I just didn't know about that behaviour of egrep, it
sounds useful.

> 
> An alternative to replace the egrep would be
>    sed 's/^/mg-debug-fail-stdin: /' >&2
> which preserves missing trailing newline but annotates the output.
> 
> Ian.

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

Re: [OSSTEST PATCH 04/11] mg-debug-fail: New utility script for debugging

[Ian Campbell]

On Mon, 2015-12-07 at 15:40 +0000, Ian Campbell wrote:
> On Mon, 2015-12-07 at 15:10 +0000, Ian Jackson wrote:
> > Ian Campbell writes ("Re: [OSSTEST PATCH 04/11] mg-debug-fail: New
> > utility script for debugging"):
> > > On Fri, 2015-12-04 at 19:35 +0000, Ian Jackson wrote:
> > > > diff --git a/mg-debug-fail b/mg-debug-fail
> > > > new file mode 100755
> > > > index 0000000..64fa235
> > > > --- /dev/null
> > > > +++ b/mg-debug-fail
> > > > @@ -0,0 +1,13 @@
> > > > +#!/bin/sh
> > > > +#
> > > > +# This script can be provided anywhere an executable or command
> > > > name
> > > > is
> > > > +# wanted.  It prints its arguments, and its stdin, to its stderr,
> > > > and
> > > > +# then exits nonzero.
> > > > +#
> > > > +# When using this it may be useful to provide </dev/null as a
> > > > +# redirection for the whole program under test.  Otherwise things
> > > > +# can mysteriously hang.
> > > 
> > > "egrep . - /dev/null" is too noisy, it adds a "(standard input):"
> > > prefix
> > > which I don't think you want. But "egrep -h . - /dev/null" seems to
> > > remedy
> > > this without the possibility of these mysterious hangs.
> > 
> > This is confusing to me.  The problem is that mg-debug-fail does not
> > know whether to try to read and print its stdin.  Sometimes its stdin
> > will be the stdin for some utility that it is standing in for, and
> > should be dumped.  Whereas sometimes its stdin is the user's tty,
> > which should be left alone.
> > 
> > This difficulty is fixed in the next patch.  None of your suggestions
> > seem to address it.  What problem do you think they are solving ?
> 
> I think I got off on a bit of a tangent, and hadn't seen the next patch
> yet.

By which I mean:
Acked-by: Ian Campbell <ian.campbell@citrix.com>


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

Re: [OSSTEST PATCH 09/11] mg-schema-test-database: New script

[Ian Campbell]

On Mon, 2015-12-07 at 15:12 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [OSSTEST PATCH 09/11] mg-schema-test-database:
> New script"):
> > On Fri, 2015-12-04 at 19:35 +0000, Ian Jackson wrote:
> > > This allows a user in non-standalone mode to make a whole new test
> > > database, which is largely a clone of the original database.
> > > 
> > > The new db refers to the same resources (hosts), and more-or-less
> > > safely borrows some of those hosts.
> > 
> > "more-or-less" ? ;-)
> 
> This is a reference to the lack of owner/queue daemon handling.  (And
> of course, the general limitations of `safety' in this kind of
> context.)
> 
> > What's the overall idiom for use here, something like:
> > 
> > Against the production DB:
> > OSSTEST_TASK=ianc@testing-something ./mg-allocate [-U ...] a-host
> > 
> > ./mg-schema-test-database create [_SUFFIX] ianc@testing-something
> > 
> > Ending up in a state where a-host is allocated in the production db and
> > idle in this new test db and all other hosts are allocated in the test
> > db
> > and left to go about their usual business in the production db?
> 
> Exactly.  Maybe I should copy this to the doc comment :-).

Please feel free to nab it.

> > I glanced through this, but TBH I don't think picking through it line
> > by
> > line will be very productive, I trust you've tested it and its ok...
> 
> Due to being incompetent I've hammered each part of it quite a bit and
> the all the resulting database states (even intermediate ones) look
> good.

:-)

Ian.

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