[OSSTEST Nested PATCH v8 0/7] Introduction of netsted HVM test job

[OSSTEST Nested PATCH v8 0/7] Introduction of netsted HVM test job

[longtao.pang]

This patch set adds nested HVM test case for osstest.
In this test case, a Xen hypervisor (L1) runs on top of another Xen 
hypervisor (L0). 
Upon L1 hypervisor, we will then create a nested guest (L2), and test if the 
Linux guest can then be installed and run well.
About nested Xen virtualization, 
refer to http://wiki.xenproject.org/wiki/Nested_Virtualization_in_Xen.

Test steps
        0. To run osstest in standalone mode, write a config file in
           '~/.xen-osstest/config', and then create a standalone.config file 
           to define 'TREE_LINUX', 'REVISION_LINUX' which will be used for
           nested test. The directory path of 'Debian Images' could be defined 
           in '~/.xen-osstest/config'. 
        1. run 'build-amd64' job and then 'build-amd64-pvops', to prepare xen
           installation tarball and hvm guest kernel.
        2. run 'test-amd64-amd6-nested' job, it does following:
                a. invoke test step of 'ts-debain-hvm-install' to install 
                   a normal HVM guest
                b. invoke test step of 'ts-nested-setup' to make some
                   appropriate runvars which selecthost() would recognise and
                   prepare the configurations for installing L2 guest VM. 
                c. invoke test step of 'ts-xen-install' to install xen on 
                   the normal guest, alter it into a L1 hypervisor
                d. invoke test step of 'ts-debain-hvm-install' again, but 
                   take the L1 hypervisor as host, install the L2 guest on it
                e. invoke test step of 'ts-guest-stop', stop L2 guest.
                f. invoke test step of 'ts-guest-destroy' to destroy L1 guest.

This patch set reuse 'ts-debian-hvm-install' for both L1 installation and L2
installation, define 'nestedl1' and 'nestedl2' as L1 and L2's hostname,
define 'nested_l1 as L1's host identity.
It also reuses 'ts-xen-install' with L1's hostname 'nestedl1' input parameter 
to differentiate from L0 Xen installation.
This patch series has been tested on test machines of amd64 arch,
Debian-7.2.0-amd64 as guests OS, with hvm domain0 of Linux kernel 3.18.5, 
in standalone mode.
Also, we use linux-stable tree as domain0 kernel source.
----------------------------------------------------------------
longtao.pang (7):
      parsing grub which has 'submenu' primitive
      Changes to support '/boot' leading paths of kernel, xen, in grub
      Edit some APIs in TestSupport.pm for nested test
      Changes on test step of Debian hvm guest install
      Add new script to customize nested test configuration
      Compose the main recipe of nested test job
      Add test job for nest test case

 Osstest/Debian.pm      |   21 ++++++++++++++++-----
 Osstest/TestSupport.pm |    8 +++++++-
 make-flight            |   20 ++++++++++++++++++++
 sg-run-job             |   11 +++++++++++
 ts-debian-hvm-install  |    9 ++++++---
 ts-nested-setup        |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 112 insertions(+), 9 deletions(-)
 create mode 100755 ts-nested-setup

[OSSTEST Nested PATCH v8 1/7] parsing grub which has 'submenu' primitive

[longtao.pang]

>From a hvm kernel build from Linux stable Kernel tree,
the auto generated grub2 menu will have 'submenu' primitive, upon the
'menuentry' items. Xen boot entries will be grouped into a submenu. This
patch adds capability to support such grub formats.

Signed-off-by: longtao.pang <longtaox.pang@intel.com>
---
Changes in v8:
Separate unrelated codes to the use of submenu.
---
 Osstest/Debian.pm |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/Osstest/Debian.pm b/Osstest/Debian.pm
index 69530fb..378af54 100644
--- a/Osstest/Debian.pm
+++ b/Osstest/Debian.pm
@@ -404,10 +404,18 @@ sub setupboot_grub2 ($$$$) {
     
         my $count= 0;
         my $entry;
+        my $submenu;
         while (<$f>) {
             next if m/^\s*\#/ || !m/\S/;
             if (m/^\s*\}\s*$/) {
-                die unless $entry;
+                die unless $entry || $submenu;
+                if(!defined $entry && defined $submenu){
+                    logm("Met end of a submenu starting from ".
+                        "$submenu->{StartLine}. ".
+                        "Our want kern is $want_kernver");
+                    $submenu=undef;
+                    next;
+                }
                 my (@missing) =
                     grep { !defined $entry->{$_} } 
 		        (defined $xenhopt
@@ -438,6 +446,9 @@ sub setupboot_grub2 ($$$$) {
                 $entry= { Title => $1, StartLine => $., Number => $count };
                 $count++;
             }
+            if (m/^submenu\s+[\'\"](.*)[\'\"].*\{\s*$/) {
+                $submenu={ StartLine =>$.};
+            }
             if (m/^\s*multiboot\s*\/(xen\-[0-9][-+.0-9a-z]*\S+)/) {
                 die unless $entry;
                 $entry->{Hv}= $1;
-- 
1.7.10.4

[OSSTEST Nested PATCH v8 2/7] Changes to support '/boot' leading paths of kernel, xen, in grub

[longtao.pang]

Support situations of grub that have vmlinuz and other things starting
with path of '/boot' rather than '/'.

Signed-off-by: longtao.pang <longtaox.pang@intel.com>
---
 Osstest/Debian.pm |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Osstest/Debian.pm b/Osstest/Debian.pm
index 378af54..1a57cdd 100644
--- a/Osstest/Debian.pm
+++ b/Osstest/Debian.pm
@@ -449,21 +449,21 @@ sub setupboot_grub2 ($$$$) {
             if (m/^submenu\s+[\'\"](.*)[\'\"].*\{\s*$/) {
                 $submenu={ StartLine =>$.};
             }
-            if (m/^\s*multiboot\s*\/(xen\-[0-9][-+.0-9a-z]*\S+)/) {
+            if (m/^\s*multiboot\s*(?:\/boot)?\/(xen\S+)/) {
                 die unless $entry;
                 $entry->{Hv}= $1;
             }
-            if (m/^\s*multiboot\s*\/(vmlinu[xz]-(\S+))/) {
+            if (m/^\s*multiboot\s*(?:\/boot)?\/(vmlinu[xz]-(\S+))/) {
                 die unless $entry;
                 $entry->{KernOnly}= $1;
                 $entry->{KernVer}= $2;
             }
-            if (m/^\s*module\s*\/(vmlinu[xz]-(\S+))/) {
+            if (m/^\s*module\s*(?:\/boot)?\/(vmlinu[xz]-(\S+))/) {
                 die unless $entry;
                 $entry->{KernDom0}= $1;
                 $entry->{KernVer}= $2;
             }
-            if (m/^\s*module\s*\/(initrd\S+)/) {
+            if (m/^\s*module\s*(?:\/boot)?\/(initrd\S+)/) {
                 $entry->{Initrd}= $1;
             }
 	    if (m/^\s*module\s*\/(xenpolicy\S+)/) {
-- 
1.7.10.4

[OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm for nested test

[longtao.pang]

1. Designate vif model by make-flight.
2. In L2 installation context, its host (L1) IP address is not queried
from DNS, but after running "ts-nested-setup + host + nested", L1 IP is
stored in runvar.

Signed-off-by: longtao.pang <longtaox.pang@intel.com>
---
Changes in v8:
Remove the unnecessary symbol of ';' in 'TestSupport.pm'.
---
 Osstest/TestSupport.pm |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index 1bde67d..4cdacfc 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -818,6 +818,10 @@ sub selecthost ($) {
 	logm("Host $name is in HostGroup $ho->{Properties}{HostGroup}");
     }
 
+    if ( $r{"${name}_ip"} ) {
+        $setprop->("IpAddr", $r{"${name}_ip"});
+    }
+
     # Next, we use the config file's general properites as defaults
     foreach my $k (keys %c) {
 	next unless $k =~ m/^HostProp_([A-Z].*)$/;
@@ -1548,11 +1552,13 @@ sub prepareguest_part_xencfg ($$$$$) {
     my $oncrash= $xopts->{OnCrash} || 'preserve';
     my $vcpus= guest_var($gho, 'vcpus', $xopts->{DefVcpus} || 2);
     my $xoptcfg= $xopts->{ExtraConfig};
+    my $vif= guest_var($gho, 'vifmodel','');
+    my $vifmodel= $vif ? "model=$vif" : '';
     $xoptcfg='' unless defined $xoptcfg;
     my $xencfg= <<END;
 name        = '$gho->{Name}'
 memory = ${ram_mb}
-vif         = [ 'type=ioemu,mac=$gho->{Ether}' ]
+vif         = [ 'type=ioemu,${vifmodel},mac=$gho->{Ether}' ]
 #
 on_poweroff = '$onpoweroff'
 on_reboot   = '$onreboot'
-- 
1.7.10.4

[OSSTEST Nested PATCH v8 4/7] Changes on test step of Debian hvm guest install

[longtao.pang]

1. Increase disk size to accommodate to nested test requirement.
2. Since 'Debain-xxx-.iso' image will be stored in rootfs of L1 guest,
therefore needs more disk capacity, increase root partition size in
preseed generation.
3. In L1 installation context, assign more memory to it; Since it
acts as a nested hypervisor anyway.
4. Comment out CDROM entry in sources.list to make HTTP URL entry
available for L1 hvm guest.
5. Enable nestedhvm feature in ExtraConfig for nested job.

Signed-off-by: longtao.pang <longtaox.pang@intel.com>
---
Changes in v8:
1. Update a conventional way to comment out CDROM entry in sources.list.
2. Enable nestedhvm feature only for the nested job.
3. Set nested disk and memroy size to be driven from runvars in
'ts-debian-hvm-install'.
---
 ts-debian-hvm-install |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/ts-debian-hvm-install b/ts-debian-hvm-install
index cfd5144..13009d0 100755
--- a/ts-debian-hvm-install
+++ b/ts-debian-hvm-install
@@ -36,7 +36,7 @@ our $ho= selecthost($whhost);
 
 # guest memory size will be set based on host free memory, see below
 our $ram_mb;
-our $disk_mb= 10000;
+our $disk_mb= $r{'nested_disk'} ? $r{'nested_disk'} : 10000;
 
 our $guesthost= "$gn.guest.osstest";
 our $gho;
@@ -60,7 +60,7 @@ d-i partman-auto/expert_recipe string \\
                         use_filesystem{ } filesystem{ vfat } \\
                         mountpoint{ /boot/efi } \\
                 . \\
-                5000 50 5000 ext4 \\
+                10000 50 10000 ext4 \\
                         method{ format } format{ } \\
                         use_filesystem{ } filesystem{ ext4 } \\
                         mountpoint{ / } \\
@@ -75,6 +75,7 @@ d-i preseed/late_command string \\
         in-target mkdir -p /boot/efi/EFI/boot; \\
         in-target cp /boot/efi/EFI/debian/grubx64.efi /boot/efi/EFI/boot/bootx64.efi ;\\
         in-target mkdir -p /root/.ssh; \\
+        in-target sed -i 's/^deb *cdrom/#&/g' /etc/apt/sources.list; \\
         in-target sh -c "echo -e '$authkeys'> /root/.ssh/authorized_keys";
 END
     return $preseed_file;
@@ -149,9 +150,11 @@ sub prep () {
     target_putfilecontents_root_stash($ho, 10, preseed(),
                                       $preseed_file_path);
 
+    my $nestedhvm_feature = $gn eq 'nestedl1' ? 'nestedhvm=1' : '';
     more_prepareguest_hvm($ho,$gho, $ram_mb, $disk_mb,
                           OnReboot => 'preserve',
                           Bios => $r{bios},
+                          ExtraConfig => $nestedhvm_feature,
                           PostImageHook => sub {
         my $cmds = iso_copy_content_from_image($gho, $newiso);
         $cmds .= prepare_initrd($initrddir,$newiso,$preseed_file_path);
@@ -174,7 +177,7 @@ my $ram_lots = 5000;
 if ($host_freemem_mb > $ram_lots * 2 + $ram_minslop) {
     $ram_mb = $ram_lots;
 } else {
-    $ram_mb = 768;
+    $ram_mb = $r{"${gn}_memory"} ? $r{"${gn}_memory"} : 768;
 }
 logm("Host has $host_freemem_mb MB free memory, setting guest memory size to $ram_mb MB");
 
-- 
1.7.10.4

[OSSTEST Nested PATCH v8 5/7] Add new script to customize nested test configuration

[longtao.pang]

1. In this script, make some appropriate runvars which selecthost would
recognise.
2. Prepare the configurations for installing L2 guest VM.
3. Create a lv disk in L0 and hot-attach it to L1, need to restart L1 to
make the block disk to be recognized by L1 system, then using this disk
to create a VG that used for installing L2.

Signed-off-by: longtao.pang <longtaox.pang@intel.com>
---
Changes in v8:
1. Replace '$nested_host' by '$l1->{Guest}'.
---
 ts-nested-setup |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100755 ts-nested-setup

diff --git a/ts-nested-setup b/ts-nested-setup
new file mode 100755
index 0000000..41d5e80
--- /dev/null
+++ b/ts-nested-setup
@@ -0,0 +1,52 @@
+#!/usr/bin/perl -w
+# This is part of "osstest", an automated testing framework for Xen.
+# Copyright (C) 2015 Intel Inc.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+use strict qw(vars);
+use DBI;
+use Osstest;
+use Osstest::Debian;
+use Osstest::TestSupport;
+
+tsreadconfig();
+our ($l0,$l1) = ts_get_host_guest(@ARGV);
+
+guest_check_ip($l1);
+target_cmd_root($l1, "mkdir -p /home/osstest/.ssh && cp /root/.ssh/authorized_keys /home/osstest/.ssh/");
+my $url = $c{WebspaceUrl}.$c{WebspaceCommon}.$l0->{Name}."_".'overlay.tar';
+target_cmd_root($l1, <<END);
+    wget -O overlay.tar $url
+    tar -xf overlay.tar -C /
+    rm overlay.tar -f
+    update-rc.d osstest-confirm-booted start 99 2 .
+END
+
+store_runvar('nested_l1',$l1->{Guest});
+store_runvar("$l1->{Guest}_ip",$l1->{Ip});
+
+my $l2_disk_mb = 20000;
+my $l2_lv_name = 'nestedl2-disk';
+my $vgname = $l0->{Name};
+$vgname .= ".$c{TestHostDomain}" if ($l0->{Suite} =~ m/lenny/);
+target_cmd_root($l0, <<END);
+    lvremove -f /dev/${vgname}/${l2_lv_name} ||:
+    lvcreate -L ${l2_disk_mb}M -n $l2_lv_name $vgname
+    dd if=/dev/zero of=/dev/${vgname}/${l2_lv_name} count=10
+    xl block-attach $l1->{Name} /dev/${vgname}/${l2_lv_name},raw,sdb,rw
+END
+target_install_packages_norec($l1, qw(lvm2 rsync genisoimage));
+target_reboot($l1);
+target_cmd_root($l1, "pvcreate /dev/xvdb && vgcreate ${l2_lv_name}_vg /dev/xvdb");
-- 
1.7.10.4

[OSSTEST Nested PATCH v8 6/7] Compose the main recipe of nested test job

[longtao.pang]

Signed-off-by: longtao.pang <longtaox.pang@intel.com>
---
Changes in v8:
Change the patch order from 6 to 5.
---
 sg-run-job |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/sg-run-job b/sg-run-job
index eae159d..2ca5ebf 100755
--- a/sg-run-job
+++ b/sg-run-job
@@ -299,6 +299,17 @@ proc run-job/test-pair {} {
 #    run-ts . remus-failover ts-remus-check         src_host dst_host + debian
 }
 
+proc need-hosts/test-nested {} {return host}
+proc run-job/test-nested {} {
+    run-ts . = ts-debian-hvm-install + host + nestedl1
+    run-ts . = ts-nested-setup + host + nestedl1
+    run-ts . = ts-xen-install nested_l1
+    run-ts . = ts-host-reboot nested_l1
+    run-ts . = ts-debian-hvm-install nested_l1 nestedl2
+    run-ts . = ts-guest-stop nested_l1 nestedl2
+    run-ts . = ts-guest-destroy host nestedl1
+}
+
 proc test-guest-migr {g} {
     if {[catch { run-ts . = ts-migrate-support-check + host $g }]} return
 
-- 
1.7.10.4

[OSSTEST Nested PATCH v8 7/7] Add test job for nest test case

[longtao.pang]

1. This patch adds creation of the nested test job, when job creation
procedure is invoked.
2. Set nested L1's vif model as e1000 by make-flight.

Signed-off-by: longtao.pang <longtaox.pang@intel.com>
---
Changes in v8:
1. Use 'nestedl1' and 'nestedl2' as L1 and L2's clearer name in nested
test job.
2. Setting Debian_ISO as 'debian-7.2.0-amd64-CD-1.iso' in make-flight.
---
 make-flight |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/make-flight b/make-flight
index cc8ecdb..40ca808 100755
--- a/make-flight
+++ b/make-flight
@@ -204,6 +204,25 @@ do_hvm_win7_x64_tests () {
             all_hostflags=$most_hostflags,hvm
 }
 
+do_hvm_debian_nested_tests () {
+  if [ $xenarch != amd64 -a $dom0arch != amd64 ]; then
+    return
+  fi
+
+  job_create_test test-$xenarch$kern-$dom0arch-nested test-nested xl \
+                        $xenarch $dom0arch \
+            nestedl1_image=debian-7.2.0-amd64-CD-1.iso \
+            nestedl2_image=debian-7.2.0-amd64-CD-1.iso \
+            bios=seabios \
+            kernbuildjob=build-amd64-pvops \
+            kernkind=pvops \
+            nestedl1_vifmodel='e1000' \
+            nested_disk='15000' \
+            nestedl1_memory='3072' \
+            device_model_version=qemu-xen \
+            all_hostflags=$most_hostflags,hvm
+}
+
 do_hvm_debian_test_one () {
   testname=$1
   bios=$2
@@ -430,6 +449,7 @@ test_matrix_do_one () {
     done
 
   fi
+  do_hvm_debian_nested_tests
   #do_passthrough_tests
 }
 
-- 
1.7.10.4

Re: [OSSTEST Nested PATCH v8 1/7] parsing grub which has 'submenu' primitive

[Ian Campbell]

[-- Attachment #1: Type: text/plain, Size: 3899 bytes --]

On Mon, 2015-04-13 at 17:19 -0400, longtao.pang wrote:
> From a hvm kernel build from Linux stable Kernel tree,
> the auto generated grub2 menu will have 'submenu' primitive, upon the
> 'menuentry' items. Xen boot entries will be grouped into a submenu. This
> patch adds capability to support such grub formats.
> 
> Signed-off-by: longtao.pang <longtaox.pang@intel.com>

I think this won't work with nested submenus, but I assume we don't see
them at least with Jessie so we can live with this.

I'm also not sure it would cope with a submenu declared within a
function, but again I expect we aren't seeing those in practice.

So given this handles the grub.cfg files we see in practice:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

I'd also like to see some of our delta in
overlay/etc/grub.d/20_linux_xen which was added to remove the
possibility of submenus to be removed, see below.

Wei, per the comment below I think we should update the grub BR with a
fixed patch, like the attached.

Ian.

-----


>From e082a82a3036152797447ac4c809e3b67e68cd12 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Tue, 21 Apr 2015 11:06:06 +0100
Subject: [PATCH] grub: remove patch to disable submenu from 20_linux_xen
 overlay

setupboot_grub2 now supports submenus, so we can reduce our delta vs
upstream a bit.

I started by extracting 20_linux_xen from
http://snapshot.debian.org/archive/debian/20130703T094657Z/pool/main/g/grub2/grub-common_1.99-27%2Bdeb7u2_amd64.deb
and then applying the patch at
http://savannah.gnu.org/file/grub.patch?file_id=32276 (the patch from
grub bug #42420 at http://savannah.gnu.org/bugs/?43420) and
reinstating the comment at the top of the file (modified to drop the
reference to the Debian bug.

This left me with some spurious changes:

    @@ -93,7 +93,7 @@ linux_entry ()
           if test ! -e "${xen_dirname}/${xenpolicy}" ; then
              return
           fi
    -      xen_args=`echo $xen_args flask=enforcing`
    +      xen_args=`echo $xen_args flask_enabled=1 flask_enforcing=1`
           if ${recovery} ; then
              title="$(gettext_quoted "%s, with Xen %s (XSM enabled) and Linux %s (recovery mode)")"
           else
    @@ -137,7 +137,6 @@ EOF
            echo    '$message'
            module  ${rel_dirname}/${xenpolicy}
     EOF
    -  fi
       cat << EOF
     }
     EOF

I think these are bugs in the patch in the grub BTS, which were fixed
while iterating over the XSM series in osstest but didn't make it into
the upstream version, the fixes to those bugs are reverted byu the
above. So I have manually reverted them.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
Wei, if you agree wrt those changes I'll update the bug, or perhaps
you want to?
---
 overlay/etc/grub.d/20_linux_xen | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/overlay/etc/grub.d/20_linux_xen b/overlay/etc/grub.d/20_linux_xen
index 5315e2a..aaead1b 100755
--- a/overlay/etc/grub.d/20_linux_xen
+++ b/overlay/etc/grub.d/20_linux_xen
@@ -1,7 +1,7 @@
 #! /bin/sh
 
 # Copied from the identically named file in grub-common 1.99-27+deb7u2.
-# This version fixed Debian bug #690538 and GRUB bug #43420.
+# This version fixes GRUB bug #43420.
 
 set -e
 
@@ -173,6 +173,7 @@ while [ "x${xen_list}" != "x" ] ; do
     xen_dirname=`dirname ${current_xen}`
     rel_xen_dirname=`make_system_path_relative_to_its_root $xen_dirname`
     xen_version=`echo $xen_basename | sed -e "s,.gz$,,g;s,^xen-,,g"`
+    echo "submenu \"Xen ${xen_version}\" {"
     while [ "x$list" != "x" ] ; do
 	linux=`version_find_latest $list`
 	echo "Found linux image: $linux" >&2
@@ -214,5 +215,6 @@ while [ "x${xen_list}" != "x" ] ; do
 
 	list=`echo $list | tr ' ' '\n' | grep -vx $linux | tr '\n' ' '`
     done
+    echo "}"
     xen_list=`echo $xen_list | tr ' ' '\n' | grep -vx $current_xen | tr '\n' ' '`
 done
-- 
2.1.4



[-- Attachment #2: grub.v2.patch --]
[-- Type: text/x-patch, Size: 2670 bytes --]

--- /home/ianc/tmp/x/etc/grub.d/20_linux_xen	2013-07-03 04:39:20.000000000 +0100
+++ overlay/etc/grub.d/20_linux_xen	2015-04-21 11:09:57.777812773 +0100
@@ -81,10 +85,27 @@
   recovery="$4"
   args="$5"
   xen_args="$6"
-  if ${recovery} ; then
-    title="$(gettext_quoted "%s, with Xen %s and Linux %s (recovery mode)")"
+  xsm="$7"
+  # If user wants to enable XSM support, make sure there's
+  # corresponding policy file.
+  if ${xsm} ; then
+      xenpolicy=`echo xenpolicy-$xen_version`
+      if test ! -e "${xen_dirname}/${xenpolicy}" ; then
+	  return
+      fi
+      xen_args=`echo $xen_args flask=enforcing`
+      if ${recovery} ; then
+	  title="$(gettext_quoted "%s, with Xen %s (XSM enabled) and Linux %s (recovery mode)")"
+      else
+	  title="$(gettext_quoted "%s, with Xen %s (XSM enabled) and Linux %s")"
+      fi
   else
-    title="$(gettext_quoted "%s, with Xen %s and Linux %s")"
+      xenpolicy=""
+      if ${recovery} ; then
+	  title="$(gettext_quoted "%s, with Xen %s and Linux %s (recovery mode)")"
+      else
+	  title="$(gettext_quoted "%s, with Xen %s and Linux %s")"
+      fi
   fi
   printf "menuentry '${title}' ${CLASS} {\n" "${os}" "${xen_version}" "${version}"
   if ! ${recovery} ; then
@@ -110,6 +131,13 @@
 	module	${rel_dirname}/${initrd}
 EOF
   fi
+  if test -n "${xenpolicy}" ; then
+    message="$(gettext_printf "Loading XSM policy ...")"
+    cat << EOF
+	echo	'$message'
+	module	${rel_dirname}/${xenpolicy}
+EOF
+  fi
   cat << EOF
 }
 EOF
@@ -133,7 +161,7 @@
 if [ "x${linux_list}" = "x" ] ; then
     exit 0
 fi
-xen_list=`for i in /boot/xen*; do
+xen_list=`for i in /boot/xen[-.]*; do
         if grub_file_is_not_garbage "$i" ; then echo -n "$i " ; fi
       done`
 prepare_boot_cache=
@@ -175,10 +203,14 @@
 	fi
 
 	linux_entry "${OS}" "${version}" "${xen_version}" false \
-	    "${GRUB_CMDLINE_LINUX} ${GRUB_CMDLINE_LINUX_DEFAULT}" "${GRUB_CMDLINE_XEN} ${GRUB_CMDLINE_XEN_DEFAULT}"
+	    "${GRUB_CMDLINE_LINUX} ${GRUB_CMDLINE_LINUX_DEFAULT}" "${GRUB_CMDLINE_XEN} ${GRUB_CMDLINE_XEN_DEFAULT}" false
+	linux_entry "${OS}" "${version}" "${xen_version}" false \
+	    "${GRUB_CMDLINE_LINUX} ${GRUB_CMDLINE_LINUX_DEFAULT}" "${GRUB_CMDLINE_XEN} ${GRUB_CMDLINE_XEN_DEFAULT}" true
 	if [ "x${GRUB_DISABLE_RECOVERY}" != "xtrue" ]; then
 	    linux_entry "${OS}" "${version}" "${xen_version}" true \
-		"single ${GRUB_CMDLINE_LINUX}" "${GRUB_CMDLINE_XEN}"
+		"single ${GRUB_CMDLINE_LINUX}" "${GRUB_CMDLINE_XEN}" false
+	    linux_entry "${OS}" "${version}" "${xen_version}" true \
+		"single ${GRUB_CMDLINE_LINUX}" "${GRUB_CMDLINE_XEN}" true
 	fi
 
 	list=`echo $list | tr ' ' '\n' | grep -vx $linux | tr '\n' ' '`

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [OSSTEST Nested PATCH v8 2/7] Changes to support '/boot' leading paths of kernel, xen, in grub

[Ian Campbell]

On Mon, 2015-04-13 at 17:19 -0400, longtao.pang wrote:
> Support situations of grub that have vmlinuz and other things starting
> with path of '/boot' rather than '/'.
> 
> Signed-off-by: longtao.pang <longtaox.pang@intel.com>

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

> ---
>  Osstest/Debian.pm |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/Osstest/Debian.pm b/Osstest/Debian.pm
> index 378af54..1a57cdd 100644
> --- a/Osstest/Debian.pm
> +++ b/Osstest/Debian.pm
> @@ -449,21 +449,21 @@ sub setupboot_grub2 ($$$$) {
>              if (m/^submenu\s+[\'\"](.*)[\'\"].*\{\s*$/) {
>                  $submenu={ StartLine =>$.};
>              }
> -            if (m/^\s*multiboot\s*\/(xen\-[0-9][-+.0-9a-z]*\S+)/) {
> +            if (m/^\s*multiboot\s*(?:\/boot)?\/(xen\S+)/) {
>                  die unless $entry;
>                  $entry->{Hv}= $1;
>              }
> -            if (m/^\s*multiboot\s*\/(vmlinu[xz]-(\S+))/) {
> +            if (m/^\s*multiboot\s*(?:\/boot)?\/(vmlinu[xz]-(\S+))/) {
>                  die unless $entry;
>                  $entry->{KernOnly}= $1;
>                  $entry->{KernVer}= $2;
>              }
> -            if (m/^\s*module\s*\/(vmlinu[xz]-(\S+))/) {
> +            if (m/^\s*module\s*(?:\/boot)?\/(vmlinu[xz]-(\S+))/) {
>                  die unless $entry;
>                  $entry->{KernDom0}= $1;
>                  $entry->{KernVer}= $2;
>              }
> -            if (m/^\s*module\s*\/(initrd\S+)/) {
> +            if (m/^\s*module\s*(?:\/boot)?\/(initrd\S+)/) {
>                  $entry->{Initrd}= $1;
>              }
>  	    if (m/^\s*module\s*\/(xenpolicy\S+)/) {

Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm for nested test

[Ian Campbell]

On Mon, 2015-04-13 at 17:19 -0400, longtao.pang wrote:
> 1. Designate vif model by make-flight.
> 2. In L2 installation context, its host (L1) IP address is not queried
> from DNS, but after running "ts-nested-setup + host + nested", L1 IP is
> stored in runvar.
> 
> Signed-off-by: longtao.pang <longtaox.pang@intel.com>
> ---
> Changes in v8:
> Remove the unnecessary symbol of ';' in 'TestSupport.pm'.
> ---
>  Osstest/TestSupport.pm |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
> index 1bde67d..4cdacfc 100644
> --- a/Osstest/TestSupport.pm
> +++ b/Osstest/TestSupport.pm
> @@ -818,6 +818,10 @@ sub selecthost ($) {
>  	logm("Host $name is in HostGroup $ho->{Properties}{HostGroup}");
>      }
>  
> +    if ( $r{"${name}_ip"} ) {
> +        $setprop->("IpAddr", $r{"${name}_ip"});
> +    }

This is one for Ian I think, but I suspect this should use ${ident}
rather than ${name}. 

${ident} is e.g. 'host' or 'srchost' or 'nestedl1' it is the prefer used
on the runvar names. ${name} is the specific value assigned, i.e. an
actual host name.

For such properties we usually prefer ${ident}_foo. Ian, correct me if
I'm wrong please.

> +
>      # Next, we use the config file's general properites as defaults
>      foreach my $k (keys %c) {
>  	next unless $k =~ m/^HostProp_([A-Z].*)$/;
> @@ -1548,11 +1552,13 @@ sub prepareguest_part_xencfg ($$$$$) {
>      my $oncrash= $xopts->{OnCrash} || 'preserve';
>      my $vcpus= guest_var($gho, 'vcpus', $xopts->{DefVcpus} || 2);
>      my $xoptcfg= $xopts->{ExtraConfig};
> +    my $vif= guest_var($gho, 'vifmodel','');
> +    my $vifmodel= $vif ? "model=$vif" : '';
>      $xoptcfg='' unless defined $xoptcfg;
>      my $xencfg= <<END;
>  name        = '$gho->{Name}'
>  memory = ${ram_mb}
> -vif         = [ 'type=ioemu,mac=$gho->{Ether}' ]
> +vif         = [ 'type=ioemu,${vifmodel},mac=$gho->{Ether}' ]

If $vifmodel=='' then this will end up with a literal ",," in the
configuration. I've not checked if this is allowed but for forms sake I
would suggest to add the necessary , to $vifmodel when it is != ''. e.g.

+    my $vifmodel= $vif ? ",model=$vif" : '';
[...]
+vif         = [ 'type=ioemu,mac=$gho->{Ether}${vifmodel}' ]

Ian

Re: [OSSTEST Nested PATCH v8 4/7] Changes on test step of Debian hvm guest install

[Ian Campbell]

On Mon, 2015-04-13 at 17:19 -0400, longtao.pang wrote:
> 1. Increase disk size to accommodate to nested test requirement.
> 2. Since 'Debain-xxx-.iso' image will be stored in rootfs of L1 guest,
> therefore needs more disk capacity, increase root partition size in
> preseed generation.
> 3. In L1 installation context, assign more memory to it; Since it
> acts as a nested hypervisor anyway.
> 4. Comment out CDROM entry in sources.list to make HTTP URL entry
> available for L1 hvm guest.
> 5. Enable nestedhvm feature in ExtraConfig for nested job.
> 
> Signed-off-by: longtao.pang <longtaox.pang@intel.com>
> ---
> Changes in v8:
> 1. Update a conventional way to comment out CDROM entry in sources.list.
> 2. Enable nestedhvm feature only for the nested job.
> 3. Set nested disk and memroy size to be driven from runvars in
> 'ts-debian-hvm-install'.
> ---
>  ts-debian-hvm-install |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/ts-debian-hvm-install b/ts-debian-hvm-install
> index cfd5144..13009d0 100755
> --- a/ts-debian-hvm-install
> +++ b/ts-debian-hvm-install
> @@ -36,7 +36,7 @@ our $ho= selecthost($whhost);
>  
>  # guest memory size will be set based on host free memory, see below
>  our $ram_mb;
> -our $disk_mb= 10000;
> +our $disk_mb= $r{'nested_disk'} ? $r{'nested_disk'} : 10000;

This shouldn't hardcode 'nested' here, but use the guest ident. (nested
is the wrong name now anyway I think, since it is nestedl1 in this
iteration, which is why we avoid such hardcoding)

So you should arrange to do this somewhere that $gho is in scope and use
guest_var($gho, 'disk', 10000). $gho is in scope in $prep where $disk_mb
is used and AFAICT it is only used in that function, so I think you can
move this to a local variable

Perhaps also consider a more descriptive runvar name too, like disksize?

>  our $guesthost= "$gn.guest.osstest";
>  our $gho;
> @@ -60,7 +60,7 @@ d-i partman-auto/expert_recipe string \\
>                          use_filesystem{ } filesystem{ vfat } \\
>                          mountpoint{ /boot/efi } \\
>                  . \\
> -                5000 50 5000 ext4 \\
> +                10000 50 10000 ext4 \\
>                          method{ format } format{ } \\
>                          use_filesystem{ } filesystem{ ext4 } \\
>                          mountpoint{ / } \\
> @@ -75,6 +75,7 @@ d-i preseed/late_command string \\
>          in-target mkdir -p /boot/efi/EFI/boot; \\
>          in-target cp /boot/efi/EFI/debian/grubx64.efi /boot/efi/EFI/boot/bootx64.efi ;\\
>          in-target mkdir -p /root/.ssh; \\
> +        in-target sed -i 's/^deb *cdrom/#&/g' /etc/apt/sources.list; \\
>          in-target sh -c "echo -e '$authkeys'> /root/.ssh/authorized_keys";
>  END
>      return $preseed_file;
> @@ -149,9 +150,11 @@ sub prep () {
>      target_putfilecontents_root_stash($ho, 10, preseed(),
>                                        $preseed_file_path);
>  
> +    my $nestedhvm_feature = $gn eq 'nestedl1' ? 'nestedhvm=1' : '';

This should be based on guest_var($gho, "enable_nestedhvm", undef) not
$gn eq "nested1".

Also I think this would be better done as:
       my $extra_config = '';
       $extra_config .= 'nestedhvm=1\n'
           if guest_var($gho, "enable_nestedhvm", 'false') eq 'true';

Then use $extra_config below. This will make it easier to add more such
things in the future.

>      more_prepareguest_hvm($ho,$gho, $ram_mb, $disk_mb,
>                            OnReboot => 'preserve',
>                            Bios => $r{bios},
> +                          ExtraConfig => $nestedhvm_feature,
>                            PostImageHook => sub {
>          my $cmds = iso_copy_content_from_image($gho, $newiso);
>          $cmds .= prepare_initrd($initrddir,$newiso,$preseed_file_path);
> @@ -174,7 +177,7 @@ my $ram_lots = 5000;
>  if ($host_freemem_mb > $ram_lots * 2 + $ram_minslop) {
>      $ram_mb = $ram_lots;
>  } else {
> -    $ram_mb = 768;
> +    $ram_mb = $r{"${gn}_memory"} ? $r{"${gn}_memory"} : 768;

guest_var again and I think this can be local to prep() too..

>  }
>  logm("Host has $host_freemem_mb MB free memory, setting guest memory size to $ram_mb MB");
>  

Re: [OSSTEST Nested PATCH v8 5/7] Add new script to customize nested test configuration

[Ian Campbell]

On Mon, 2015-04-13 at 17:19 -0400, longtao.pang wrote:
> 1. In this script, make some appropriate runvars which selecthost would
> recognise.
> 2. Prepare the configurations for installing L2 guest VM.
> 3. Create a lv disk in L0 and hot-attach it to L1, need to restart L1 to
> make the block disk to be recognized by L1 system, then using this disk
> to create a VG that used for installing L2.
> 
> Signed-off-by: longtao.pang <longtaox.pang@intel.com>
> ---
> Changes in v8:
> 1. Replace '$nested_host' by '$l1->{Guest}'.
> ---
>  ts-nested-setup |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100755 ts-nested-setup
> 
> diff --git a/ts-nested-setup b/ts-nested-setup
> new file mode 100755
> index 0000000..41d5e80
> --- /dev/null
> +++ b/ts-nested-setup
> @@ -0,0 +1,52 @@
> +#!/usr/bin/perl -w
> +# This is part of "osstest", an automated testing framework for Xen.
> +# Copyright (C) 2015 Intel Inc.
> +#
> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU Affero General Public License as published by
> +# the Free Software Foundation, either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU Affero General Public License for more details.
> +#
> +# You should have received a copy of the GNU Affero General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +use strict qw(vars);
> +use DBI;
> +use Osstest;
> +use Osstest::Debian;
> +use Osstest::TestSupport;
> +
> +tsreadconfig();
> +our ($l0,$l1) = ts_get_host_guest(@ARGV);
> +
> +guest_check_ip($l1);
> +target_cmd_root($l1, "mkdir -p /home/osstest/.ssh && cp /root/.ssh/authorized_keys /home/osstest/.ssh/");
> +my $url = $c{WebspaceUrl}.$c{WebspaceCommon}.$l0->{Name}."_".'overlay.tar';
> +target_cmd_root($l1, <<END);
> +    wget -O overlay.tar $url
> +    tar -xf overlay.tar -C /
> +    rm overlay.tar -f
> +    update-rc.d osstest-confirm-booted start 99 2 .
> +END

I cc'd you on some patches which I think should help avoid this
duplication.

> +
> +store_runvar('nested_l1',$l1->{Guest});

I'm not sure what this is for and it would normally be wrong to hardcode
nested_l1 like that, where is it used?

Most places you seem to use nestedl1, not nested_l1. I think you ended
up with this due to not using guest_var and the various hardcoding
you've done elsewhere. I suspect with all that fixed and make-flight
updated accordingly you won't need this var any more.

> +store_runvar("$l1->{Guest}_ip",$l1->{Ip});
> +
> +my $l2_disk_mb = 20000;
> +my $l2_lv_name = 'nestedl2-disk';
> +my $vgname = $l0->{Name};
> +$vgname .= ".$c{TestHostDomain}" if ($l0->{Suite} =~ m/lenny/);
> +target_cmd_root($l0, <<END);
> +    lvremove -f /dev/${vgname}/${l2_lv_name} ||:
> +    lvcreate -L ${l2_disk_mb}M -n $l2_lv_name $vgname
> +    dd if=/dev/zero of=/dev/${vgname}/${l2_lv_name} count=10

I think you can do all of this by passing a suitable l2 $gho to
prepareguest_part_lvmdisk, can't you?

I think you can get that by my $l2 = selectguest($ARGV[????], $l1).

Where ARGV[????] is a new parameter passed by sg-run-job i.e. nestedl2,
i.e. the one after whatever ts_get_host_guest consumes at the top of the
file (so ARGV[2] perhaps?).

Once you have that $l2 you can then use guest_var for e.g. the size,
which will be good because it will be picked up by your modifications to
ts-debian-hvm-install such that they automatically match.

> +    xl block-attach $l1->{Name} /dev/${vgname}/${l2_lv_name},raw,sdb,rw

You use sdb here, but xvdb below. I think xvdb would work here, or to
avoid HVM confusion wrt SCSI vs PV perhaps use xvde throughout (since it
has no emulated counterpart by default IIRC)?

> +END
> +target_install_packages_norec($l1, qw(lvm2 rsync genisoimage));
> +target_reboot($l1);
> +target_cmd_root($l1, "pvcreate /dev/xvdb && vgcreate ${l2_lv_name}_vg /dev/xvdb");

Re: [OSSTEST Nested PATCH v8 6/7] Compose the main recipe of nested test job

[Ian Campbell]

On Mon, 2015-04-13 at 17:19 -0400, longtao.pang wrote:
> Signed-off-by: longtao.pang <longtaox.pang@intel.com>
> ---
> Changes in v8:
> Change the patch order from 6 to 5.
> ---
>  sg-run-job |   11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/sg-run-job b/sg-run-job
> index eae159d..2ca5ebf 100755
> --- a/sg-run-job
> +++ b/sg-run-job
> @@ -299,6 +299,17 @@ proc run-job/test-pair {} {
>  #    run-ts . remus-failover ts-remus-check         src_host dst_host + debian
>  }
>  
> +proc need-hosts/test-nested {} {return host}
> +proc run-job/test-nested {} {
> +    run-ts . = ts-debian-hvm-install + host + nestedl1
> +    run-ts . = ts-nested-setup + host + nestedl1
> +    run-ts . = ts-xen-install nested_l1
> +    run-ts . = ts-host-reboot nested_l1
> +    run-ts . = ts-debian-hvm-install nested_l1 nestedl2
> +    run-ts . = ts-guest-stop nested_l1 nestedl2
> +    run-ts . = ts-guest-destroy host nestedl1

Based on my comments to previous patches I think you'll want to use
nestedl1 and nestedl2 consistently and never nested_l1/nested_l2.

You may also need to pass an extra nestedl2 to ts-nested-setup based on
my feedback there.

What testid's does this all generate? If you run
        ./standalone run-job --simulate <jobname> | grep testid
(If your osstest doesn't include the --simulate option yet then either
upgrade or set OSSTEST_SIMULATE=1 in your environment)

Then it will include a load of lines like
        2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 1 testid build-check(1) ==========
        2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 2 testid hosts-allocate ==========
        2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 3 testid host-install(3) ==========
        2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 4 testid host-ping-check-native ==========
        2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 5 testid xen-install ==========
        2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 6 testid xen-boot ==========
        2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 7 testid host-ping-check-xen ==========
        2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 8 testid leak-check/basis(8) ==========
        2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 9 testid debian-install ==========
        2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 10 testid debian-fixup ==========
        2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 11 testid guest-start ==========
        2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 12 testid migrate-support-check ==========
        2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 13 testid guest-saverestore ==========
        2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 14 testid guest-localmigrate ==========
        2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 15 testid guest-saverestore.2 ==========
        2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 16 testid guest-localmigrate.2 ==========
        2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 17 testid guest-localmigrate/x10 ==========
        2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 18 testid guest-stop ==========
        2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 19 testid guest-start.2 ==========
        2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 20 testid guest-start/debian.repeat ==========
        2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 21 testid guest-destroy ==========
        2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 22 testid leak-check/check ==========
        2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 23 testid capture-logs(23) ==========

The placement of the + in the recipe affect whether a parameter is
included in the test id or not, I'm not sure but you might want/need one
or two in the ts-guest-destroy-step?

Ian.

Re: [OSSTEST Nested PATCH v8 7/7] Add test job for nest test case

[Ian Campbell]

On Mon, 2015-04-13 at 17:19 -0400, longtao.pang wrote:
> 1. This patch adds creation of the nested test job, when job creation
> procedure is invoked.
> 2. Set nested L1's vif model as e1000 by make-flight.
> 
> Signed-off-by: longtao.pang <longtaox.pang@intel.com>
> ---
> Changes in v8:
> 1. Use 'nestedl1' and 'nestedl2' as L1 and L2's clearer name in nested
> test job.
> 2. Setting Debian_ISO as 'debian-7.2.0-amd64-CD-1.iso' in make-flight.
> ---
>  make-flight |   20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/make-flight b/make-flight
> index cc8ecdb..40ca808 100755
> --- a/make-flight
> +++ b/make-flight
> @@ -204,6 +204,25 @@ do_hvm_win7_x64_tests () {
>              all_hostflags=$most_hostflags,hvm
>  }
>  
> +do_hvm_debian_nested_tests () {
> +  if [ $xenarch != amd64 -a $dom0arch != amd64 ]; then
> +    return
> +  fi
> +
> +  job_create_test test-$xenarch$kern-$dom0arch-nested test-nested xl \
> +                        $xenarch $dom0arch \
> +            nestedl1_image=debian-7.2.0-amd64-CD-1.iso \
> +            nestedl2_image=debian-7.2.0-amd64-CD-1.iso \
> +            bios=seabios \
> +            kernbuildjob=build-amd64-pvops \
> +            kernkind=pvops \
> +            nestedl1_vifmodel='e1000' \
> +            nested_disk='15000' \

Should be nestedl1_disk I think.

It would be best to group nestedl1_* and nestedl2_* together in the
list, rather than having image up above. i.e.

            nestedl1_vifmodel='e1000' \
            nestedl1_memory='3072' \
            nestedl1_disk='15000' \
            nestedl1_image='....' \
            nestedl1_foo='bar' \
            nestedl2_disk='20000' \
            nestedl2_foo='baz' \
etc.

Ian.

Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm for nested test

[Ian Jackson]

Ian Campbell writes ("Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm for nested test"):
> On Mon, 2015-04-13 at 17:19 -0400, longtao.pang wrote:
> > --- a/Osstest/TestSupport.pm
> > +++ b/Osstest/TestSupport.pm
...
> > +    if ( $r{"${name}_ip"} ) {
> > +        $setprop->("IpAddr", $r{"${name}_ip"});
> > +    }
> 
> This is one for Ian I think, but I suspect this should use ${ident}
> rather than ${name}. 

Yes.

> ${ident} is e.g. 'host' or 'srchost' or 'nestedl1' it is the prefer used
> on the runvar names. ${name} is the specific value assigned, i.e. an
> actual host name.
> 
> For such properties we usually prefer ${ident}_foo. Ian, correct me if
> I'm wrong please.

Ian C is right.


But, I think actually the principle behind this change is wrong.

It seems to be copying information from runvars into the in-memory
data structure for host properties.  But host properties are (by
definition) matters of (fixed) configuration, not runtime definition.

I haven't looked at the rest of the series, but the same effect should
be achieved in a different way.  Note that a $ho is a hash which
already contains (after selecthost, say) an element with key `Ip'.

So the right place to do this is indeed probably somewhere around
selectguest or selecthost, but the information should be put into
$ho->{Ip} without going via host properties.

Thanks,
Ian.

Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm for nested test

[Ian Campbell]

On Tue, 2015-04-21 at 13:33 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm for nested test"):
> > On Mon, 2015-04-13 at 17:19 -0400, longtao.pang wrote:
> > > --- a/Osstest/TestSupport.pm
> > > +++ b/Osstest/TestSupport.pm
> ...
> > > +    if ( $r{"${name}_ip"} ) {
> > > +        $setprop->("IpAddr", $r{"${name}_ip"});
> > > +    }
> > 
> > This is one for Ian I think, but I suspect this should use ${ident}
> > rather than ${name}. 
> 
> Yes.
> 
> > ${ident} is e.g. 'host' or 'srchost' or 'nestedl1' it is the prefer used
> > on the runvar names. ${name} is the specific value assigned, i.e. an
> > actual host name.
> > 
> > For such properties we usually prefer ${ident}_foo. Ian, correct me if
> > I'm wrong please.
> 
> Ian C is right.
> 
> 
> But, I think actually the principle behind this change is wrong.
> 
> It seems to be copying information from runvars into the in-memory
> data structure for host properties.  But host properties are (by
> definition) matters of (fixed) configuration, not runtime definition.

Remember that here the "host" is actual an L1 virtual machine, whose IP
is not fixed (since it mac address isn't).

I don't know if that affects your opinion at all?

> I haven't looked at the rest of the series, but the same effect should
> be achieved in a different way.  Note that a $ho is a hash which
> already contains (after selecthost, say) an element with key `Ip'.
> 
> So the right place to do this is indeed probably somewhere around
> selectguest or selecthost, but the information should be put into
> $ho->{Ip} without going via host properties.
> 
> Thanks,
> Ian.

Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm for nested test

[Ian Jackson]

Ian Campbell writes ("Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm for nested test"):
> On Tue, 2015-04-21 at 13:33 +0100, Ian Jackson wrote:
> > It seems to be copying information from runvars into the in-memory
> > data structure for host properties.  But host properties are (by
> > definition) matters of (fixed) configuration, not runtime definition.
> 
> Remember that here the "host" is actual an L1 virtual machine, whose IP
> is not fixed (since it mac address isn't).

Indeed.

> I don't know if that affects your opinion at all?

No, it doesn't.  I agree that the IP address should be dynamically
figured out, and passing it through a runvar is fine.

I'm only objecting to the involvement of the host property machinery.

Thanks,
Ian.

Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm for nested test

[Ian Campbell]

On Tue, 2015-04-21 at 14:28 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm for nested test"):
> > On Tue, 2015-04-21 at 13:33 +0100, Ian Jackson wrote:
> > > It seems to be copying information from runvars into the in-memory
> > > data structure for host properties.  But host properties are (by
> > > definition) matters of (fixed) configuration, not runtime definition.
> > 
> > Remember that here the "host" is actual an L1 virtual machine, whose IP
> > is not fixed (since it mac address isn't).
> 
> Indeed.
> 
> > I don't know if that affects your opinion at all?
> 
> No, it doesn't.  I agree that the IP address should be dynamically
> figured out, and passing it through a runvar is fine.
> 
> I'm only objecting to the involvement of the host property machinery.

I'm afraid I'm not 100% sure I understand, but do you just mean that
longtao should assign to $ho->{Ip} directly within selecthost instead of
using $setprop to set the IpAddr hostprop and therefore indirectly
->{Ip}?

IOW the existing code in selecthost which looks up the IpAddr host prop
into $ho->{IpStatic} and then into $ho->{Ip} should prefer the runvar to
the host db if it is set?

BTW, what is the difference between ->{Ip} and ->{IpStatic} and should
the runvar be reflected in both or just in ->{Ip}?

Ian.

Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm for nested test

[Ian Jackson]

Ian Campbell writes ("Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm for nested test"):
> On Tue, 2015-04-21 at 14:28 +0100, Ian Jackson wrote:
> > I'm only objecting to the involvement of the host property machinery.
> 
> I'm afraid I'm not 100% sure I understand, but do you just mean that
> longtao should assign to $ho->{Ip} directly within selecthost instead of
> using $setprop to set the IpAddr hostprop and therefore indirectly
> ->{Ip}?

Yes.

> IOW the existing code in selecthost which looks up the IpAddr host prop
> into $ho->{IpStatic} and then into $ho->{Ip} should prefer the runvar to
> the host db if it is set?

Yes.

> BTW, what is the difference between ->{Ip} and ->{IpStatic} and should
> the runvar be reflected in both or just in ->{Ip}?

AFAICT the only difference is that only IpStatic is used for expanding
`ipaddr' and `ipaddrhex' in the patterns for dhcp pxe configurations.

There is an incipient problem here: our existing setup does not
require that osstest has complete control of the pxe config file for
every host on the network.  And indeed the Citrix (Cambridge) instance
lacks such control.

Instead there are some symlinks etc.  The mg-hosts mkpxedir subcommand
arranges to delegate the DHCP from root to the osstest user (and
group), by making an appropriate subdirectory with the right
permissions, and a suitable symlink.

It's not quite clear to me how this should work for nested HVM hosts.
I don't think making 2^24 symlinks "pxelinux.cfg/5a:36:0e:??:??:??"
is a practical proposition.  Perhaps nested HVM hosts should choose
their MAC addresses from a smaller set, but then there might have to
be some inter-flight coordination.

Alternatively we could provide a service for making the right setup
(well, really, just a symlink) on demand.  In practice the number of
required symlinks is more likely to be around 2^8 * <total number of
different guests in a flight>, which will be thousands rather than
millions.

Ian.

Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm for nested test

[Ian Campbell]

On Tue, 2015-04-21 at 15:30 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm for nested test"):
> > On Tue, 2015-04-21 at 14:28 +0100, Ian Jackson wrote:
> > > I'm only objecting to the involvement of the host property machinery.
> > 
> > I'm afraid I'm not 100% sure I understand, but do you just mean that
> > longtao should assign to $ho->{Ip} directly within selecthost instead of
> > using $setprop to set the IpAddr hostprop and therefore indirectly
> > ->{Ip}?
> 
> Yes.
> 
> > IOW the existing code in selecthost which looks up the IpAddr host prop
> > into $ho->{IpStatic} and then into $ho->{Ip} should prefer the runvar to
> > the host db if it is set?
> 
> Yes.

Good. Longtao, do you understand what is needed here?

> > BTW, what is the difference between ->{Ip} and ->{IpStatic} and should
> > the runvar be reflected in both or just in ->{Ip}?
> 
> AFAICT the only difference is that only IpStatic is used for expanding
> `ipaddr' and `ipaddrhex' in the patterns for dhcp pxe configurations.
> 
> There is an incipient problem here: our existing setup does not
> require that osstest has complete control of the pxe config file for
> every host on the network.  And indeed the Citrix (Cambridge) instance
> lacks such control.
> 
> Instead there are some symlinks etc.  The mg-hosts mkpxedir subcommand
> arranges to delegate the DHCP from root to the osstest user (and
> group), by making an appropriate subdirectory with the right
> permissions, and a suitable symlink.
> 
> It's not quite clear to me how this should work for nested HVM hosts.

I think this is OK with the current design because the nested HVM host
is not PXE booted, it is installed as a guest but preseeded to look a
lot like a host (quite a lot of sharing in the preseed creation) and
then tailored into a more host-like thing by things like this IP addr
runvar.

So I think from what you are saying that the ->{Ip} and ->{IpStatic}
distinction here is correct and a host which is really a guest should
not have nor use ->{IpStatic} (we want e.g. attempts to create a pxe dir
to fail).

Longtao, the conclusion is that the runvar should only appear in ->{Ip}.

->{IpStatic} should be left untouched (i.e. not initialised).

Ian.

Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm for nested test

[Pang, LongtaoX]

> -----Original Message-----
> From: Ian Campbell [mailto:ian.campbell@citrix.com]
> Sent: Tuesday, April 21, 2015 10:44 PM
> To: Ian Jackson
> Cc: Pang, LongtaoX; xen-devel@lists.xen.org; wei.liu2@citrix.com; Hu, Robert
> Subject: Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm for
> nested test
> 
> On Tue, 2015-04-21 at 15:30 +0100, Ian Jackson wrote:
> > Ian Campbell writes ("Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in
> TestSupport.pm for nested test"):
> > > On Tue, 2015-04-21 at 14:28 +0100, Ian Jackson wrote:
> > > > I'm only objecting to the involvement of the host property machinery.
> > >
> > > I'm afraid I'm not 100% sure I understand, but do you just mean that
> > > longtao should assign to $ho->{Ip} directly within selecthost instead of
> > > using $setprop to set the IpAddr hostprop and therefore indirectly
> > > ->{Ip}?
> >
> > Yes.
> >
> > > IOW the existing code in selecthost which looks up the IpAddr host prop
> > > into $ho->{IpStatic} and then into $ho->{Ip} should prefer the runvar to
> > > the host db if it is set?
> >
> > Yes.
> 
> Good. Longtao, do you understand what is needed here?
> 
I think I get it, use ${ident} rather than ${name} and assign L1's IP to $ho->{Ip} directly.
The code will be like:
if ( $r{"${ident}_ip"} ) {
    $ho->{Ip}= $r{"${ident}_ip"};
}
> > > BTW, what is the difference between ->{Ip} and ->{IpStatic} and should
> > > the runvar be reflected in both or just in ->{Ip}?
> >
> > AFAICT the only difference is that only IpStatic is used for expanding
> > `ipaddr' and `ipaddrhex' in the patterns for dhcp pxe configurations.
> >
> > There is an incipient problem here: our existing setup does not
> > require that osstest has complete control of the pxe config file for
> > every host on the network.  And indeed the Citrix (Cambridge) instance
> > lacks such control.
> >
> > Instead there are some symlinks etc.  The mg-hosts mkpxedir subcommand
> > arranges to delegate the DHCP from root to the osstest user (and
> > group), by making an appropriate subdirectory with the right
> > permissions, and a suitable symlink.
> >
> > It's not quite clear to me how this should work for nested HVM hosts.
> 
> I think this is OK with the current design because the nested HVM host
> is not PXE booted, it is installed as a guest but preseeded to look a
> lot like a host (quite a lot of sharing in the preseed creation) and
> then tailored into a more host-like thing by things like this IP addr
> runvar.
> 
> So I think from what you are saying that the ->{Ip} and ->{IpStatic}
> distinction here is correct and a host which is really a guest should
> not have nor use ->{IpStatic} (we want e.g. attempts to create a pxe dir
> to fail).
> 
> Longtao, the conclusion is that the runvar should only appear in ->{Ip}.
> 
> ->{IpStatic} should be left untouched (i.e. not initialised).
> 
> Ian.

Re: [OSSTEST Nested PATCH v8 5/7] Add new script to customize nested test configuration

[Pang, LongtaoX]

> -----Original Message-----
> From: Ian Campbell [mailto:ian.campbell@citrix.com]
> Sent: Tuesday, April 21, 2015 6:40 PM
> To: Pang, LongtaoX
> Cc: xen-devel@lists.xen.org; Ian.Jackson@eu.citrix.com; wei.liu2@citrix.com; Hu,
> Robert
> Subject: Re: [OSSTEST Nested PATCH v8 5/7] Add new script to customize nested
> test configuration
> 
> On Mon, 2015-04-13 at 17:19 -0400, longtao.pang wrote:
> > 1. In this script, make some appropriate runvars which selecthost would
> > recognise.
> > 2. Prepare the configurations for installing L2 guest VM.
> > 3. Create a lv disk in L0 and hot-attach it to L1, need to restart L1 to
> > make the block disk to be recognized by L1 system, then using this disk
> > to create a VG that used for installing L2.
> >
> > Signed-off-by: longtao.pang <longtaox.pang@intel.com>
> > ---
> > Changes in v8:
> > 1. Replace '$nested_host' by '$l1->{Guest}'.
> > ---
> >  ts-nested-setup |   52
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 52 insertions(+)
> >  create mode 100755 ts-nested-setup
> >
> > diff --git a/ts-nested-setup b/ts-nested-setup
> > new file mode 100755
> > index 0000000..41d5e80
> > --- /dev/null
> > +++ b/ts-nested-setup
> > @@ -0,0 +1,52 @@
> > +use strict qw(vars);
> > +use DBI;
> > +use Osstest;
> > +use Osstest::Debian;
> > +use Osstest::TestSupport;
> > +
> > +tsreadconfig();
> > +our ($l0,$l1) = ts_get_host_guest(@ARGV);
> > +
> > +guest_check_ip($l1);
> > +target_cmd_root($l1, "mkdir -p /home/osstest/.ssh && cp
> /root/.ssh/authorized_keys /home/osstest/.ssh/");
> > +my $url =
> $c{WebspaceUrl}.$c{WebspaceCommon}.$l0->{Name}."_".'overlay.tar';
> > +target_cmd_root($l1, <<END);
> > +    wget -O overlay.tar $url
> > +    tar -xf overlay.tar -C /
> > +    rm overlay.tar -f
> > +    update-rc.d osstest-confirm-booted start 99 2 .
> > +END
> 
> I cc'd you on some patches which I think should help avoid this
> duplication.
> 
I am trying that.
> > +
> > +store_runvar('nested_l1',$l1->{Guest});
> 
> I'm not sure what this is for and it would normally be wrong to hardcode
> nested_l1 like that, where is it used?
> 
As mentioned in [0000 Patch], 'nested_l1' is ident for L1 guest VM, 'nestedl1' is hostname for L1 guest VM.
' store_runvar('nested_l1',$l1->{Guest});' is used to store L1's ident and L1's hostname to database, that will be used for 'selecthost()' function in later script.

> Most places you seem to use nestedl1, not nested_l1. I think you ended
> up with this due to not using guest_var and the various hardcoding
> you've done elsewhere. I suspect with all that fixed and make-flight
> updated accordingly you won't need this var any more.
> 
I think I should define L1 ident with " my $l1_ident = 'nested_l1' in 'ts-nested-setup'

> > +store_runvar("$l1->{Guest}_ip",$l1->{Ip});
> > +
> > +my $l2_disk_mb = 20000;
> > +my $l2_lv_name = 'nestedl2-disk';
> > +my $vgname = $l0->{Name};
> > +$vgname .= ".$c{TestHostDomain}" if ($l0->{Suite} =~ m/lenny/);
> > +target_cmd_root($l0, <<END);
> > +    lvremove -f /dev/${vgname}/${l2_lv_name} ||:
> > +    lvcreate -L ${l2_disk_mb}M -n $l2_lv_name $vgname
> > +    dd if=/dev/zero of=/dev/${vgname}/${l2_lv_name} count=10
> 
> I think you can do all of this by passing a suitable l2 $gho to
> prepareguest_part_lvmdisk, can't you?
> 
> I think you can get that by my $l2 = selectguest($ARGV[????], $l1).
> 
I think I could try it, that means I will add more parameters for 'ts-nested-setup' script, not just 'host' and 'nestedl1'.

> Where ARGV[????] is a new parameter passed by sg-run-job i.e. nestedl2,
> i.e. the one after whatever ts_get_host_guest consumes at the top of the
> file (so ARGV[2] perhaps?).
> 
Also, I think if use ' prepareguest_part_lvmdisk', I need set 'nestedl2_vg' and 'nestedl2 _disk_lv' by make-flight.
But, when reuse 'ts-debian-hvm-install' to install L2, the ' prepareguest_part_lvmdisk' will be executed again(in 'more_prepareguest_hvm' function), then there will be multi runvars for 'nestedl2_vg' and 'nestedl2_disk_lv' in standalone.db, is that OK? Please correct me if I am wrong.

> Once you have that $l2 you can then use guest_var for e.g. the size,
> which will be good because it will be picked up by your modifications to
> ts-debian-hvm-install such that they automatically match.
> 
> > +    xl block-attach $l1->{Name} /dev/${vgname}/${l2_lv_name},raw,sdb,rw
> 
> You use sdb here, but xvdb below. I think xvdb would work here, or to
> avoid HVM confusion wrt SCSI vs PV perhaps use xvde throughout (since it
> has no emulated counterpart by default IIRC)?
> 
I think so, need a try.
> > +END
> > +target_install_packages_norec($l1, qw(lvm2 rsync genisoimage));
> > +target_reboot($l1);
> > +target_cmd_root($l1, "pvcreate /dev/xvdb && vgcreate ${l2_lv_name}_vg
> /dev/xvdb");
> 

Re: [OSSTEST Nested PATCH v8 6/7] Compose the main recipe of nested test job

[Pang, LongtaoX]

> -----Original Message-----
> From: Ian Campbell [mailto:ian.campbell@citrix.com]
> Sent: Tuesday, April 21, 2015 6:49 PM
> To: Pang, LongtaoX
> Cc: xen-devel@lists.xen.org; Ian.Jackson@eu.citrix.com; wei.liu2@citrix.com; Hu,
> Robert
> Subject: Re: [OSSTEST Nested PATCH v8 6/7] Compose the main recipe of nested
> test job
> 
> On Mon, 2015-04-13 at 17:19 -0400, longtao.pang wrote:
> > Signed-off-by: longtao.pang <longtaox.pang@intel.com>
> > ---
> > Changes in v8:
> > Change the patch order from 6 to 5.
> > ---
> >  sg-run-job |   11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/sg-run-job b/sg-run-job
> > index eae159d..2ca5ebf 100755
> > --- a/sg-run-job
> > +++ b/sg-run-job
> > @@ -299,6 +299,17 @@ proc run-job/test-pair {} {
> >  #    run-ts . remus-failover ts-remus-check         src_host dst_host +
> debian
> >  }
> >
> > +proc need-hosts/test-nested {} {return host}
> > +proc run-job/test-nested {} {
> > +    run-ts . = ts-debian-hvm-install + host + nestedl1
> > +    run-ts . = ts-nested-setup + host + nestedl1
> > +    run-ts . = ts-xen-install nested_l1
> > +    run-ts . = ts-host-reboot nested_l1
> > +    run-ts . = ts-debian-hvm-install nested_l1 nestedl2
> > +    run-ts . = ts-guest-stop nested_l1 nestedl2
> > +    run-ts . = ts-guest-destroy host nestedl1
> 
> Based on my comments to previous patches I think you'll want to use
> nestedl1 and nestedl2 consistently and never nested_l1/nested_l2.
> 
nestedl1 nestedl2 are hostname for L1 guest VM and L2 guest VM, nested_l1 and nested_l2 are ident for L1 and L2.

> You may also need to pass an extra nestedl2 to ts-nested-setup based on
> my feedback there.
> 
> What testid's does this all generate? If you run
>         ./standalone run-job --simulate <jobname> | grep testid
> (If your osstest doesn't include the --simulate option yet then either
> upgrade or set OSSTEST_SIMULATE=1 in your environment)
> 
Could you please make it clear, since I failed to run the above command, or do I need some other settings?
For example: './standalone run-job --simulate test-amd64-amd64-xl | grep testid', just get below messages:
#./standalone run-job --simulate test-amd64-amd64-xl | grep testid
Could not open a connection to your authentication agent.
WARNING: Unable to access ssh-agent. Some tests may fail
run-job: Need a host

> Then it will include a load of lines like
>         2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 1
> testid build-check(1) ==========
>         2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 2
> testid hosts-allocate ==========
>         2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 3
> testid host-install(3) ==========
>         2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 4
> testid host-ping-check-native ==========
>         2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 5
> testid xen-install ==========
>         2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 6
> testid xen-boot ==========
>         2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 7
> testid host-ping-check-xen ==========
>         2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 8
> testid leak-check/basis(8) ==========
>         2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 9
> testid debian-install ==========
>         2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 10
> testid debian-fixup ==========
>         2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 11
> testid guest-start ==========
>         2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 12
> testid migrate-support-check ==========
>         2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 13
> testid guest-saverestore ==========
>         2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 14
> testid guest-localmigrate ==========
>         2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 15
> testid guest-saverestore.2 ==========
>         2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 16
> testid guest-localmigrate.2 ==========
>         2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 17
> testid guest-localmigrate/x10 ==========
>         2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 18
> testid guest-stop ==========
>         2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 19
> testid guest-start.2 ==========
>         2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 20
> testid guest-start/debian.repeat ==========
>         2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 21
> testid guest-destroy ==========
>         2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 22
> testid leak-check/check ==========
>         2015-04-21 10:44:57 Z standalone.test-amd64-amd64-xl ========== 23
> testid capture-logs(23) ==========
> 
> The placement of the + in the recipe affect whether a parameter is
> included in the test id or not, I'm not sure but you might want/need one
> or two in the ts-guest-destroy-step?
> 
> Ian.

Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm for nested test

[Ian Campbell]

On Wed, 2015-04-22 at 08:25 +0000, Pang, LongtaoX wrote:
> 
> 
> > -----Original Message-----
> > From: Ian Campbell [mailto:ian.campbell@citrix.com]
> > Sent: Tuesday, April 21, 2015 10:44 PM
> > To: Ian Jackson
> > Cc: Pang, LongtaoX; xen-devel@lists.xen.org; wei.liu2@citrix.com; Hu, Robert
> > Subject: Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm for
> > nested test
> > 
> > On Tue, 2015-04-21 at 15:30 +0100, Ian Jackson wrote:
> > > Ian Campbell writes ("Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in
> > TestSupport.pm for nested test"):
> > > > On Tue, 2015-04-21 at 14:28 +0100, Ian Jackson wrote:
> > > > > I'm only objecting to the involvement of the host property machinery.
> > > >
> > > > I'm afraid I'm not 100% sure I understand, but do you just mean that
> > > > longtao should assign to $ho->{Ip} directly within selecthost instead of
> > > > using $setprop to set the IpAddr hostprop and therefore indirectly
> > > > ->{Ip}?
> > >
> > > Yes.
> > >
> > > > IOW the existing code in selecthost which looks up the IpAddr host prop
> > > > into $ho->{IpStatic} and then into $ho->{Ip} should prefer the runvar to
> > > > the host db if it is set?
> > >
> > > Yes.
> > 
> > Good. Longtao, do you understand what is needed here?
> > 
> I think I get it, use ${ident} rather than ${name} and assign L1's IP to $ho->{Ip} directly.
> The code will be like:
> if ( $r{"${ident}_ip"} ) {
>     $ho->{Ip}= $r{"${ident}_ip"};
> }

It will, I think, need to be integrated with the existing assignment to
$ho->{Ip} in select host, so something like:

        if ( $r{"${ident}_ip"} ) {
            $ho->{Ip}= $r{"${ident}_ip"};
        } else {
            $ho->{Ip}= $ho->{IpStatic};
        }
        
or perhaps:

        $ho->{Ip} = $r{"${ident}_ip"} ? $r{"${ident}_ip"} : $ho->{IpStatic};

Ian.

Re: [OSSTEST Nested PATCH v8 5/7] Add new script to customize nested test configuration

[Ian Campbell]

On Wed, 2015-04-22 at 08:35 +0000, Pang, LongtaoX wrote:
> 
> 
> > -----Original Message-----
> > From: Ian Campbell [mailto:ian.campbell@citrix.com]
> > Sent: Tuesday, April 21, 2015 6:40 PM
> > To: Pang, LongtaoX
> > Cc: xen-devel@lists.xen.org; Ian.Jackson@eu.citrix.com; wei.liu2@citrix.com; Hu,
> > Robert
> > Subject: Re: [OSSTEST Nested PATCH v8 5/7] Add new script to customize nested
> > test configuration
> > 
> > On Mon, 2015-04-13 at 17:19 -0400, longtao.pang wrote:
> > > 1. In this script, make some appropriate runvars which selecthost would
> > > recognise.
> > > 2. Prepare the configurations for installing L2 guest VM.
> > > 3. Create a lv disk in L0 and hot-attach it to L1, need to restart L1 to
> > > make the block disk to be recognized by L1 system, then using this disk
> > > to create a VG that used for installing L2.
> > >
> > > Signed-off-by: longtao.pang <longtaox.pang@intel.com>
> > > ---
> > > Changes in v8:
> > > 1. Replace '$nested_host' by '$l1->{Guest}'.
> > > ---
> > >  ts-nested-setup |   52
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 52 insertions(+)
> > >  create mode 100755 ts-nested-setup
> > >
> > > diff --git a/ts-nested-setup b/ts-nested-setup
> > > new file mode 100755
> > > index 0000000..41d5e80
> > > --- /dev/null
> > > +++ b/ts-nested-setup
> > > @@ -0,0 +1,52 @@
> > > +use strict qw(vars);
> > > +use DBI;
> > > +use Osstest;
> > > +use Osstest::Debian;
> > > +use Osstest::TestSupport;
> > > +
> > > +tsreadconfig();
> > > +our ($l0,$l1) = ts_get_host_guest(@ARGV);
> > > +
> > > +guest_check_ip($l1);
> > > +target_cmd_root($l1, "mkdir -p /home/osstest/.ssh && cp
> > /root/.ssh/authorized_keys /home/osstest/.ssh/");
> > > +my $url =
> > $c{WebspaceUrl}.$c{WebspaceCommon}.$l0->{Name}."_".'overlay.tar';
> > > +target_cmd_root($l1, <<END);
> > > +    wget -O overlay.tar $url
> > > +    tar -xf overlay.tar -C /
> > > +    rm overlay.tar -f
> > > +    update-rc.d osstest-confirm-booted start 99 2 .
> > > +END
> > 
> > I cc'd you on some patches which I think should help avoid this
> > duplication.
> > 
> I am trying that.
> > > +
> > > +store_runvar('nested_l1',$l1->{Guest});
> > 
> > I'm not sure what this is for and it would normally be wrong to hardcode
> > nested_l1 like that, where is it used?
> > 
> As mentioned in [0000 Patch], 'nested_l1' is ident for L1 guest VM,
> 'nestedl1' is hostname for L1 guest VM.
> ' store_runvar('nested_l1',$l1->{Guest});' is used to store L1's ident
> and L1's hostname to database, that will be used for 'selecthost()'
> function in later script.

Having a runvar with the bare ident as a name doesn't seem right.
Perhaps you want to be writing to $r{"${ident}_hostname"} or some such?

What do you actually need the hostname for anyway?

> > Most places you seem to use nestedl1, not nested_l1. I think you ended
> > up with this due to not using guest_var and the various hardcoding
> > you've done elsewhere. I suspect with all that fixed and make-flight
> > updated accordingly you won't need this var any more.
> > 
> I think I should define L1 ident with " my $l1_ident = 'nested_l1' in 'ts-nested-setup'

Hardcoding anything like that in ts-nested-setup seems wrong to me.

The ident should come from the script's parameters (i.e. be part of the
sg-run-job invocation of the script).

Imagine a hypothetical nested-virt migration test, in that scenario you
would want to run ts-nested-setup on two hosts, both nestedl1src and
nestedl2dst in order to configure things. That's why we don't hardcode
such things.

> 
> > > +store_runvar("$l1->{Guest}_ip",$l1->{Ip});
> > > +
> > > +my $l2_disk_mb = 20000;
> > > +my $l2_lv_name = 'nestedl2-disk';
> > > +my $vgname = $l0->{Name};
> > > +$vgname .= ".$c{TestHostDomain}" if ($l0->{Suite} =~ m/lenny/);
> > > +target_cmd_root($l0, <<END);
> > > +    lvremove -f /dev/${vgname}/${l2_lv_name} ||:
> > > +    lvcreate -L ${l2_disk_mb}M -n $l2_lv_name $vgname
> > > +    dd if=/dev/zero of=/dev/${vgname}/${l2_lv_name} count=10
> > 
> > I think you can do all of this by passing a suitable l2 $gho to
> > prepareguest_part_lvmdisk, can't you?
> > 
> > I think you can get that by my $l2 = selectguest($ARGV[????], $l1).
> > 
> I think I could try it, that means I will add more parameters for
> 'ts-nested-setup' script, not just 'host' and 'nestedl1'.

I think so, that seems to make sense since the script is in effect
acting on three entities.

> > Where ARGV[????] is a new parameter passed by sg-run-job i.e. nestedl2,
> > i.e. the one after whatever ts_get_host_guest consumes at the top of the
> > file (so ARGV[2] perhaps?).
> > 
> Also, I think if use ' prepareguest_part_lvmdisk', I need set
> 'nestedl2_vg' and 'nestedl2 _disk_lv' by make-flight.

I think you need to have called guest_find_lv at some point so these are
set.

> But, when reuse 'ts-debian-hvm-install' to install L2, the '
> prepareguest_part_lvmdisk' will be executed again(in
> 'more_prepareguest_hvm' function), then there will be multi runvars
> for 'nestedl2_vg' and 'nestedl2_disk_lv' in standalone.db, is that OK?
> Please correct me if I am wrong.

You can't end up with multiple runvars, I think the subsequent
invocations should just be reusing things.

One wrinkle is whether the lv is in the l0 or l1 "namespace". I'm not
quite sure how that will fit together.

Ian.

Re: [OSSTEST Nested PATCH v8 6/7] Compose the main recipe of nested test job

[Ian Campbell]

On Wed, 2015-04-22 at 08:38 +0000, Pang, LongtaoX wrote:
> 
> 
> > -----Original Message-----
> > From: Ian Campbell [mailto:ian.campbell@citrix.com]
> > Sent: Tuesday, April 21, 2015 6:49 PM
> > To: Pang, LongtaoX
> > Cc: xen-devel@lists.xen.org; Ian.Jackson@eu.citrix.com; wei.liu2@citrix.com; Hu,
> > Robert
> > Subject: Re: [OSSTEST Nested PATCH v8 6/7] Compose the main recipe of nested
> > test job
> > 
> > On Mon, 2015-04-13 at 17:19 -0400, longtao.pang wrote:
> > > Signed-off-by: longtao.pang <longtaox.pang@intel.com>
> > > ---
> > > Changes in v8:
> > > Change the patch order from 6 to 5.
> > > ---
> > >  sg-run-job |   11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > >
> > > diff --git a/sg-run-job b/sg-run-job
> > > index eae159d..2ca5ebf 100755
> > > --- a/sg-run-job
> > > +++ b/sg-run-job
> > > @@ -299,6 +299,17 @@ proc run-job/test-pair {} {
> > >  #    run-ts . remus-failover ts-remus-check         src_host dst_host +
> > debian
> > >  }
> > >
> > > +proc need-hosts/test-nested {} {return host}
> > > +proc run-job/test-nested {} {
> > > +    run-ts . = ts-debian-hvm-install + host + nestedl1
> > > +    run-ts . = ts-nested-setup + host + nestedl1
> > > +    run-ts . = ts-xen-install nested_l1
> > > +    run-ts . = ts-host-reboot nested_l1
> > > +    run-ts . = ts-debian-hvm-install nested_l1 nestedl2
> > > +    run-ts . = ts-guest-stop nested_l1 nestedl2
> > > +    run-ts . = ts-guest-destroy host nestedl1
> > 
> > Based on my comments to previous patches I think you'll want to use
> > nestedl1 and nestedl2 consistently and never nested_l1/nested_l2.
> > 
> nestedl1 nestedl2 are hostname for L1 guest VM and L2 guest VM,
> nested_l1 and nested_l2 are ident for L1 and L2.

Hostnames shouldn't be appearing in this script, I don't think,
certainly not bare like that.

For the physical i.e. l0 case I think what happens is that
need-hosts/foo ends up returning a bunch of $ident=$an_actual_host. i.e.
srchost=$hosta and dsthost=$hostb.

I'm not sure how that would work in the nested world, but maybe you
should be writing $ident=$guestname (i.e. nested_l1=nestedl1)?

Ian?

Re: [OSSTEST Nested PATCH v8 6/7] Compose the main recipe of nested test job

[Ian Campbell]

On Wed, 2015-04-22 at 12:04 +0100, Ian Campbell wrote:
> On Wed, 2015-04-22 at 08:38 +0000, Pang, LongtaoX wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Ian Campbell [mailto:ian.campbell@citrix.com]
> > > Sent: Tuesday, April 21, 2015 6:49 PM
> > > To: Pang, LongtaoX
> > > Cc: xen-devel@lists.xen.org; Ian.Jackson@eu.citrix.com; wei.liu2@citrix.com; Hu,
> > > Robert
> > > Subject: Re: [OSSTEST Nested PATCH v8 6/7] Compose the main recipe of nested
> > > test job
> > > 
> > > On Mon, 2015-04-13 at 17:19 -0400, longtao.pang wrote:
> > > > Signed-off-by: longtao.pang <longtaox.pang@intel.com>
> > > > ---
> > > > Changes in v8:
> > > > Change the patch order from 6 to 5.
> > > > ---
> > > >  sg-run-job |   11 +++++++++++
> > > >  1 file changed, 11 insertions(+)
> > > >
> > > > diff --git a/sg-run-job b/sg-run-job
> > > > index eae159d..2ca5ebf 100755
> > > > --- a/sg-run-job
> > > > +++ b/sg-run-job
> > > > @@ -299,6 +299,17 @@ proc run-job/test-pair {} {
> > > >  #    run-ts . remus-failover ts-remus-check         src_host dst_host +
> > > debian
> > > >  }
> > > >
> > > > +proc need-hosts/test-nested {} {return host}
> > > > +proc run-job/test-nested {} {
> > > > +    run-ts . = ts-debian-hvm-install + host + nestedl1
> > > > +    run-ts . = ts-nested-setup + host + nestedl1
> > > > +    run-ts . = ts-xen-install nested_l1
> > > > +    run-ts . = ts-host-reboot nested_l1
> > > > +    run-ts . = ts-debian-hvm-install nested_l1 nestedl2
> > > > +    run-ts . = ts-guest-stop nested_l1 nestedl2
> > > > +    run-ts . = ts-guest-destroy host nestedl1
> > > 
> > > Based on my comments to previous patches I think you'll want to use
> > > nestedl1 and nestedl2 consistently and never nested_l1/nested_l2.
> > > 
> > nestedl1 nestedl2 are hostname for L1 guest VM and L2 guest VM,
> > nested_l1 and nested_l2 are ident for L1 and L2.
> 
> Hostnames shouldn't be appearing in this script, I don't think,
> certainly not bare like that.
> 
> For the physical i.e. l0 case I think what happens is that
> need-hosts/foo ends up returning a bunch of $ident=$an_actual_host. i.e.
> srchost=$hosta and dsthost=$hostb.
> 
> I'm not sure how that would work in the nested world, but maybe you
> should be writing $ident=$guestname (i.e. nested_l1=nestedl1)?
> 
> Ian?

Ian is still plugging in his computers after moving desks. But having
spoken to him IRL it seems I'm not quite right above.

The arguments passed to ts-* in sg-run-job should _always_ be just a
bare indent. The ident=foo thing is a special case for standalone mode.

However, Ian did also say (and I agreed) that there really isn't any
reason why the ident and the hostname should differ here (or indeed the
guest name from l0's point of view, modulo the suffix osstest sometimes
adds).

Ian.

Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm for nested test

[Ian Jackson]

Ian Campbell writes ("Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm for nested test"):
> It will, I think, need to be integrated with the existing assignment to
> $ho->{Ip} in select host, so something like:
> 
>         if ( $r{"${ident}_ip"} ) {
>             $ho->{Ip}= $r{"${ident}_ip"};
>         } else {
>             $ho->{Ip}= $ho->{IpStatic};
>         }

Yes.

> or perhaps:
> 
>         $ho->{Ip} = $r{"${ident}_ip"} ? $r{"${ident}_ip"} : $ho->{IpStatic};

The shortest way to spell this is to use //, eg:

      $ho->{Ip} = $r{"${ident}_ip"} // $ho->{IpStatic};

Ian.

Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm for nested test

[Hu, Robert]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: Wednesday, April 22, 2015 8:50 PM
> To: Ian Campbell
> Cc: Pang, LongtaoX; xen-devel@lists.xen.org; wei.liu2@citrix.com; Hu, Robert
> Subject: Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm
> for nested test
> 
> Ian Campbell writes ("Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in
> TestSupport.pm for nested test"):
> > It will, I think, need to be integrated with the existing assignment to
> > $ho->{Ip} in select host, so something like:
> >
> >         if ( $r{"${ident}_ip"} ) {
> >             $ho->{Ip}= $r{"${ident}_ip"};
> >         } else {
> >             $ho->{Ip}= $ho->{IpStatic};
> >         }
> 
> Yes.
Yes, otherwise the code would go 'die' somewhere later.
> 
> > or perhaps:
> >
> >         $ho->{Ip} = $r{"${ident}_ip"} ? $r{"${ident}_ip"} : $ho->{IpStatic};
> 
> The shortest way to spell this is to use //, eg:
> 
>       $ho->{Ip} = $r{"${ident}_ip"} // $ho->{IpStatic};
Ah, yes!
> 
> Ian.

Re: [OSSTEST Nested PATCH v8 4/7] Changes on test step of Debian hvm guest install

[Robert Hu]

On Tue, 2015-04-21 at 11:28 +0100, Ian Campbell wrote:
> On Mon, 2015-04-13 at 17:19 -0400, longtao.pang wrote:
> > 1. Increase disk size to accommodate to nested test requirement.
> > 2. Since 'Debain-xxx-.iso' image will be stored in rootfs of L1 guest,
> > therefore needs more disk capacity, increase root partition size in
> > preseed generation.
> > 3. In L1 installation context, assign more memory to it; Since it
> > acts as a nested hypervisor anyway.
> > 4. Comment out CDROM entry in sources.list to make HTTP URL entry
> > available for L1 hvm guest.
> > 5. Enable nestedhvm feature in ExtraConfig for nested job.
> > 
> > Signed-off-by: longtao.pang <longtaox.pang@intel.com>
> > ---
> > Changes in v8:
> > 1. Update a conventional way to comment out CDROM entry in sources.list.
> > 2. Enable nestedhvm feature only for the nested job.
> > 3. Set nested disk and memroy size to be driven from runvars in
> > 'ts-debian-hvm-install'.
> > ---
> >  ts-debian-hvm-install |    9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/ts-debian-hvm-install b/ts-debian-hvm-install
> > index cfd5144..13009d0 100755
> > --- a/ts-debian-hvm-install
> > +++ b/ts-debian-hvm-install
> > @@ -36,7 +36,7 @@ our $ho= selecthost($whhost);
> >  
> >  # guest memory size will be set based on host free memory, see below
> >  our $ram_mb;
> > -our $disk_mb= 10000;
> > +our $disk_mb= $r{'nested_disk'} ? $r{'nested_disk'} : 10000;
> 
> This shouldn't hardcode 'nested' here, but use the guest ident. (nested
> is the wrong name now anyway I think, since it is nestedl1 in this
> iteration, which is why we avoid such hardcoding)
Right, we shall store this runvar with name of ${l1_ident}_disksize in
ts-nested-setup.
> 
> So you should arrange to do this somewhere that $gho is in scope and use
> guest_var($gho, 'disk', 10000). $gho is in scope in $prep where $disk_mb
> is used and AFAICT it is only used in that function, so I think you can
> move this to a local variable
yes, it is supposed to be local of prep(). However, look at other ts-*
(ts-debian-install, ts-freebsd-install, etc.), $disk_mb is defined
'our'. They seems to be in alliance, are we going to it specifically in
ts-debian-hvm-install? or in some future day, we change these
ts-*-install in another patch series?

To use guest_var(), it needs $gho in scope as you said. But in prep() we
can see until prepareguest() returns, we don't have $gho in scope, while
prepareguest() needs $disk_mb as input. So here we get into a loop.
I think we shall in prepareguest(), check if $r{${guest_ident}_disksize}
is defined, then override passed in $mb value. How would you like this?
> 
> Perhaps also consider a more descriptive runvar name too, like disksize?
> 
> >  our $guesthost= "$gn.guest.osstest";
> >  our $gho;
> > @@ -60,7 +60,7 @@ d-i partman-auto/expert_recipe string \\
> >                          use_filesystem{ } filesystem{ vfat } \\
> >                          mountpoint{ /boot/efi } \\
> >                  . \\
> > -                5000 50 5000 ext4 \\
> > +                10000 50 10000 ext4 \\
> >                          method{ format } format{ } \\
> >                          use_filesystem{ } filesystem{ ext4 } \\
> >                          mountpoint{ / } \\
> > @@ -75,6 +75,7 @@ d-i preseed/late_command string \\
> >          in-target mkdir -p /boot/efi/EFI/boot; \\
> >          in-target cp /boot/efi/EFI/debian/grubx64.efi /boot/efi/EFI/boot/bootx64.efi ;\\
> >          in-target mkdir -p /root/.ssh; \\
> > +        in-target sed -i 's/^deb *cdrom/#&/g' /etc/apt/sources.list; \\
> >          in-target sh -c "echo -e '$authkeys'> /root/.ssh/authorized_keys";
> >  END
> >      return $preseed_file;
> > @@ -149,9 +150,11 @@ sub prep () {
> >      target_putfilecontents_root_stash($ho, 10, preseed(),
> >                                        $preseed_file_path);
> >  
> > +    my $nestedhvm_feature = $gn eq 'nestedl1' ? 'nestedhvm=1' : '';
> 
> This should be based on guest_var($gho, "enable_nestedhvm", undef) not
> $gn eq "nested1".
> 
> Also I think this would be better done as:
>        my $extra_config = '';
>        $extra_config .= 'nestedhvm=1\n'
>            if guest_var($gho, "enable_nestedhvm", 'false') eq 'true';
> 
> Then use $extra_config below. This will make it easier to add more such
> things in the future.
Ack.
> 
> >      more_prepareguest_hvm($ho,$gho, $ram_mb, $disk_mb,
> >                            OnReboot => 'preserve',
> >                            Bios => $r{bios},
> > +                          ExtraConfig => $nestedhvm_feature,
> >                            PostImageHook => sub {
> >          my $cmds = iso_copy_content_from_image($gho, $newiso);
> >          $cmds .= prepare_initrd($initrddir,$newiso,$preseed_file_path);
> > @@ -174,7 +177,7 @@ my $ram_lots = 5000;
> >  if ($host_freemem_mb > $ram_lots * 2 + $ram_minslop) {
> >      $ram_mb = $ram_lots;
> >  } else {
> > -    $ram_mb = 768;
> > +    $ram_mb = $r{"${gn}_memory"} ? $r{"${gn}_memory"} : 768;
> 
> guest_var again and I think this can be local to prep() too..
Also, agree to use guest_var() while may be still be global here as
other similar ts-*-install does, change them together in the future.
> 
> >  }
> >  logm("Host has $host_freemem_mb MB free memory, setting guest memory size to $ram_mb MB");
> >  
> 
> 

Re: [OSSTEST Nested PATCH v8 4/7] Changes on test step of Debian hvm guest install

[Ian Campbell]

On Thu, 2015-04-23 at 13:59 +0800, Robert Hu wrote:
> On Tue, 2015-04-21 at 11:28 +0100, Ian Campbell wrote:
> > On Mon, 2015-04-13 at 17:19 -0400, longtao.pang wrote:
> > > 1. Increase disk size to accommodate to nested test requirement.
> > > 2. Since 'Debain-xxx-.iso' image will be stored in rootfs of L1 guest,
> > > therefore needs more disk capacity, increase root partition size in
> > > preseed generation.
> > > 3. In L1 installation context, assign more memory to it; Since it
> > > acts as a nested hypervisor anyway.
> > > 4. Comment out CDROM entry in sources.list to make HTTP URL entry
> > > available for L1 hvm guest.
> > > 5. Enable nestedhvm feature in ExtraConfig for nested job.
> > > 
> > > Signed-off-by: longtao.pang <longtaox.pang@intel.com>
> > > ---
> > > Changes in v8:
> > > 1. Update a conventional way to comment out CDROM entry in sources.list.
> > > 2. Enable nestedhvm feature only for the nested job.
> > > 3. Set nested disk and memroy size to be driven from runvars in
> > > 'ts-debian-hvm-install'.
> > > ---
> > >  ts-debian-hvm-install |    9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/ts-debian-hvm-install b/ts-debian-hvm-install
> > > index cfd5144..13009d0 100755
> > > --- a/ts-debian-hvm-install
> > > +++ b/ts-debian-hvm-install
> > > @@ -36,7 +36,7 @@ our $ho= selecthost($whhost);
> > >  
> > >  # guest memory size will be set based on host free memory, see below
> > >  our $ram_mb;
> > > -our $disk_mb= 10000;
> > > +our $disk_mb= $r{'nested_disk'} ? $r{'nested_disk'} : 10000;
> > 
> > This shouldn't hardcode 'nested' here, but use the guest ident. (nested
> > is the wrong name now anyway I think, since it is nestedl1 in this
> > iteration, which is why we avoid such hardcoding)
> Right, we shall store this runvar with name of ${l1_ident}_disksize in
> ts-nested-setup.

This is the sort of thing which needs to be done from make-flight, not
ts-*.

> > So you should arrange to do this somewhere that $gho is in scope and use
> > guest_var($gho, 'disk', 10000). $gho is in scope in $prep where $disk_mb
> > is used and AFAICT it is only used in that function, so I think you can
> > move this to a local variable
> yes, it is supposed to be local of prep(). However, look at other ts-*
> (ts-debian-install, ts-freebsd-install, etc.), $disk_mb is defined
> 'our'. They seems to be in alliance, are we going to it specifically in
> ts-debian-hvm-install? or in some future day, we change these
> ts-*-install in another patch series?

If/when the other ts-*-install need to support sizing the disk based on
a runvar they would need similar changes. If you want to make those now
for consistency then that would be great, but I don't think it needs to
be mandatory.

> To use guest_var(), it needs $gho in scope as you said. But in prep() we
> can see until prepareguest() returns, we don't have $gho in scope, while
> prepareguest() needs $disk_mb as input. So here we get into a loop.
> I think we shall in prepareguest(), check if $r{${guest_ident}_disksize}
> is defined, then override passed in $mb value. How would you like this?

Hrm, perhaps this behaviour ought to be pushed down into prepareguest
itself, IOW the existing $mb argument to that function would become the
default, but the runvar would take precedence f set? That would mean
that all ts-*-install would gain support the runvar.

Ian, what do you think of this?

The alternative would be to use $r{${gn}_disksize}.

> > 
> > >      more_prepareguest_hvm($ho,$gho, $ram_mb, $disk_mb,
> > >                            OnReboot => 'preserve',
> > >                            Bios => $r{bios},
> > > +                          ExtraConfig => $nestedhvm_feature,
> > >                            PostImageHook => sub {
> > >          my $cmds = iso_copy_content_from_image($gho, $newiso);
> > >          $cmds .= prepare_initrd($initrddir,$newiso,$preseed_file_path);
> > > @@ -174,7 +177,7 @@ my $ram_lots = 5000;
> > >  if ($host_freemem_mb > $ram_lots * 2 + $ram_minslop) {
> > >      $ram_mb = $ram_lots;
> > >  } else {
> > > -    $ram_mb = 768;
> > > +    $ram_mb = $r{"${gn}_memory"} ? $r{"${gn}_memory"} : 768;
> > 
> > guest_var again and I think this can be local to prep() too..
> Also, agree to use guest_var() while may be still be global here as
> other similar ts-*-install does, change them together in the future.

This should be handled the same way disk is, however that ends up being.

Ian.

Re: [OSSTEST Nested PATCH v8 5/7] Add new script to customize nested test configuration

[Pang, LongtaoX]

> -----Original Message-----
> From: Ian Campbell [mailto:ian.campbell@citrix.com]
> Sent: Tuesday, April 21, 2015 6:40 PM
> To: Pang, LongtaoX
> Cc: xen-devel@lists.xen.org; Ian.Jackson@eu.citrix.com; wei.liu2@citrix.com; Hu,
> Robert
> Subject: Re: [OSSTEST Nested PATCH v8 5/7] Add new script to customize nested
> test configuration
> > Signed-off-by: longtao.pang <longtaox.pang@intel.com>
> > ---
> > Changes in v8:
> > 1. Replace '$nested_host' by '$l1->{Guest}'.
> > ---
> >  ts-nested-setup |   52
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 52 insertions(+)
> >  create mode 100755 ts-nested-setup
> >
> > diff --git a/ts-nested-setup b/ts-nested-setup
> > new file mode 100755
> > index 0000000..41d5e80
> > --- /dev/null
> > +++ b/ts-nested-setup
> > @@ -0,0 +1,52 @@
> > +#!/usr/bin/perl -w
> > +
> > +use strict qw(vars);
> > +use DBI;
> > +use Osstest;
> > +use Osstest::Debian;
> > +use Osstest::TestSupport;
> > +
> > +tsreadconfig();
> > +our ($l0,$l1) = ts_get_host_guest(@ARGV);
> > +
> > +guest_check_ip($l1);
> > +target_cmd_root($l1, "mkdir -p /home/osstest/.ssh && cp
> /root/.ssh/authorized_keys /home/osstest/.ssh/");
> > +my $url =
> $c{WebspaceUrl}.$c{WebspaceCommon}.$l0->{Name}."_".'overlay.tar';
> > +target_cmd_root($l1, <<END);
> > +    wget -O overlay.tar $url
> > +    tar -xf overlay.tar -C /
> > +    rm overlay.tar -f
> > +    update-rc.d osstest-confirm-booted start 99 2 .
> > +END
> 
> I cc'd you on some patches which I think should help avoid this
> duplication.
> 
For this question, I have merged the v5_patches[04,05,06] which you CC'd to me. 
Based on your patches, after finishing installing L1 hvm guest VM with 'ts-debian-hvm-install' script, I could ssh into L1 guest as 'osstest' user without password, that means I will not need to use below code anymore
target_cmd_root($l1, "mkdir -p /home/osstest/.ssh && cp /root/.ssh/authorized_keys /home/osstest/.ssh/");

But, inside L1 guest VM, the overly files(osstest-confirm-booted, xenbridge, xenlightdaemons ) does not exist at " /etc/init.d" directory. Since 'ts-host-reboot' script will use 'osstest-confirm-booted' shell script to confirm whether L1 guest boot up normally, these overlay files are necessary here.
If I add below patch based on your patches, and install L1 hvm guest VM again, all the overly files exist in "/etc/init.d" directory inside L1 guest.
diff --git a/Osstest/Debian.pm b/Osstest/Debian.pm
index 6691ff6..4af6957 100644
--- a/Osstest/Debian.pm
+++ b/Osstest/Debian.pm
@@ -624,6 +624,7 @@ sub preseed_base ($$$$;@) {

     preseed_ssh($ho, $sfx);
     preseed_hook_overlay($ho, $sfx, $c{OverlayLocal}, 'overlay-local.tar');
+    preseed_hook_overlay($ho, $sfx, 'overlay', 'overlay.tar');

     my $preseed = <<"END";
 d-i mirror/suite string $suite

Another question, based on your patches, I find that the below commands under ' d-i preseed/late_command string \\' do not work anymore in preseed () ' of 'ts-debian-hvm-install' script. For example, after finishing installing L1 guest, there is no directory of '/boot/efi/EFI/boot' created and 'sources.list' does not be edited by sed inside L1 guest. I think you have verified this, maybe something wrong of my test environment to cause the question?
d-i preseed/late_command string \\
        in-target mkdir -p /boot/efi/EFI/boot; \\
        in-target cp /boot/efi/EFI/debian/grubx64.efi /boot/efi/EFI/boot/bootx64.efi ;\\
        in-target sed -i 's/^deb *cdrom/#&/g' /etc/apt/sources.list;
END

Re: [OSSTEST Nested PATCH v8 6/7] Compose the main recipe of nested test job

[Pang, LongtaoX]

> -----Original Message-----
> From: Ian Campbell [mailto:ian.campbell@citrix.com]
> Sent: Wednesday, April 22, 2015 7:24 PM
> To: Pang, LongtaoX
> Cc: wei.liu2@citrix.com; Hu, Robert; Ian.Jackson@eu.citrix.com;
> xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [OSSTEST Nested PATCH v8 6/7] Compose the main recipe
> of nested test job
> 
> On Wed, 2015-04-22 at 12:04 +0100, Ian Campbell wrote:
> > On Wed, 2015-04-22 at 08:38 +0000, Pang, LongtaoX wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ian Campbell [mailto:ian.campbell@citrix.com]
> > > > Sent: Tuesday, April 21, 2015 6:49 PM
> > > > To: Pang, LongtaoX
> > > > Cc: xen-devel@lists.xen.org; Ian.Jackson@eu.citrix.com;
> wei.liu2@citrix.com; Hu,
> > > > Robert
> > > > Subject: Re: [OSSTEST Nested PATCH v8 6/7] Compose the main recipe of
> nested
> > > > test job
> > > >
> > > > On Mon, 2015-04-13 at 17:19 -0400, longtao.pang wrote:
> > > > > Signed-off-by: longtao.pang <longtaox.pang@intel.com>
> > > > > ---
> > > > > Changes in v8:
> > > > > Change the patch order from 6 to 5.
> > > > > ---
> > > > >  sg-run-job |   11 +++++++++++
> > > > >  1 file changed, 11 insertions(+)
> > > > >
> > > > > diff --git a/sg-run-job b/sg-run-job
> > > > > index eae159d..2ca5ebf 100755
> > > > > --- a/sg-run-job
> > > > > +++ b/sg-run-job
> > > > > @@ -299,6 +299,17 @@ proc run-job/test-pair {} {
> > > > >  #    run-ts . remus-failover ts-remus-check         src_host dst_host
> +
> > > > debian
> > > > >  }
> > > > >
> > > > > +proc need-hosts/test-nested {} {return host}
> > > > > +proc run-job/test-nested {} {
> > > > > +    run-ts . = ts-debian-hvm-install + host + nestedl1
> > > > > +    run-ts . = ts-nested-setup + host + nestedl1
> > > > > +    run-ts . = ts-xen-install nested_l1
> > > > > +    run-ts . = ts-host-reboot nested_l1
> > > > > +    run-ts . = ts-debian-hvm-install nested_l1 nestedl2
> > > > > +    run-ts . = ts-guest-stop nested_l1 nestedl2
> > > > > +    run-ts . = ts-guest-destroy host nestedl1
> > > >
> > > > Based on my comments to previous patches I think you'll want to use
> > > > nestedl1 and nestedl2 consistently and never nested_l1/nested_l2.
> > > >
> > > nestedl1 nestedl2 are hostname for L1 guest VM and L2 guest VM,
> > > nested_l1 and nested_l2 are ident for L1 and L2.
> >
> > Hostnames shouldn't be appearing in this script, I don't think,
> > certainly not bare like that.
> >
> > For the physical i.e. l0 case I think what happens is that
> > need-hosts/foo ends up returning a bunch of $ident=$an_actual_host. i.e.
> > srchost=$hosta and dsthost=$hostb.
> >
> > I'm not sure how that would work in the nested world, but maybe you
> > should be writing $ident=$guestname (i.e. nested_l1=nestedl1)?
> >
> > Ian?
> 
> Ian is still plugging in his computers after moving desks. But having
> spoken to him IRL it seems I'm not quite right above.
> 
> The arguments passed to ts-* in sg-run-job should _always_ be just a
> bare indent. The ident=foo thing is a special case for standalone mode.
> 
Do you means that the parameter passed to ts-* in sg-run-job should _always_ be just a bare ident?
If yes. I think it needs to pass l1's '$gn' which is not a bare ident to 'ts-debian-hvm-install' for installing L1 guest VM, since the default '$gn ||= 'debianhvm' if no other '$gn' was passed to this script, but for nested job we expect a different '$gn' from 'debianhvm', such as 'nestedl1'. So it need to pass 'host' and 'nestedl1' to  'ts-debian-hvm-install' script. 
+    run-ts . = ts-debian-hvm-install + host + nestedl1

For 'ts-nested-setup' script, it need to call 'ts_get_host_guest' which need two parameters 'host' and 'nestedl1' to get $l0 and $l1 scope, so it also need to pass 'nestedl1' to this script.
For ' ts-guest-destroy', it also need two parameters of 'host' and 'nestedl1' which is L1's '$gn'.

> However, Ian did also say (and I agreed) that there really isn't any
> reason why the ident and the hostname should differ here (or indeed the
> guest name from l0's point of view, modulo the suffix osstest sometimes
> adds).
> 
> Ian.
> 

Re: [OSSTEST Nested PATCH v8 5/7] Add new script to customize nested test configuration

[Robert Hu]

On Wed, 2015-04-22 at 10:56 +0100, Ian Campbell wrote:
> On Wed, 2015-04-22 at 08:35 +0000, Pang, LongtaoX wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Ian Campbell [mailto:ian.campbell@citrix.com]
> > > Sent: Tuesday, April 21, 2015 6:40 PM
> > > To: Pang, LongtaoX
> > > Cc: xen-devel@lists.xen.org; Ian.Jackson@eu.citrix.com; wei.liu2@citrix.com; Hu,
> > > Robert
> > > Subject: Re: [OSSTEST Nested PATCH v8 5/7] Add new script to customize nested
> > > test configuration
> > > 
> > > On Mon, 2015-04-13 at 17:19 -0400, longtao.pang wrote:
> > > > 1. In this script, make some appropriate runvars which selecthost would
> > > > recognise.
> > > > 2. Prepare the configurations for installing L2 guest VM.
> > > > 3. Create a lv disk in L0 and hot-attach it to L1, need to restart L1 to
> > > > make the block disk to be recognized by L1 system, then using this disk
> > > > to create a VG that used for installing L2.
> > > >
> > > > Signed-off-by: longtao.pang <longtaox.pang@intel.com>
> > > > ---
> > > > Changes in v8:
> > > > 1. Replace '$nested_host' by '$l1->{Guest}'.
> > > > ---
> > > >  ts-nested-setup |   52
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 52 insertions(+)
> > > >  create mode 100755 ts-nested-setup
> > > >
> > > > diff --git a/ts-nested-setup b/ts-nested-setup
> > > > new file mode 100755
> > > > index 0000000..41d5e80
> > > > --- /dev/null
> > > > +++ b/ts-nested-setup
> > > > @@ -0,0 +1,52 @@
> > > > +use strict qw(vars);
> > > > +use DBI;
> > > > +use Osstest;
> > > > +use Osstest::Debian;
> > > > +use Osstest::TestSupport;
> > > > +
> > > > +tsreadconfig();
> > > > +our ($l0,$l1) = ts_get_host_guest(@ARGV);
> > > > +
> > > > +guest_check_ip($l1);
> > > > +target_cmd_root($l1, "mkdir -p /home/osstest/.ssh && cp
> > > /root/.ssh/authorized_keys /home/osstest/.ssh/");
> > > > +my $url =
> > > $c{WebspaceUrl}.$c{WebspaceCommon}.$l0->{Name}."_".'overlay.tar';
> > > > +target_cmd_root($l1, <<END);
> > > > +    wget -O overlay.tar $url
> > > > +    tar -xf overlay.tar -C /
> > > > +    rm overlay.tar -f
> > > > +    update-rc.d osstest-confirm-booted start 99 2 .
> > > > +END
> > > 
> > > I cc'd you on some patches which I think should help avoid this
> > > duplication.
> > > 
> > I am trying that.
> > > > +
> > > > +store_runvar('nested_l1',$l1->{Guest});
> > > 
> > > I'm not sure what this is for and it would normally be wrong to hardcode
> > > nested_l1 like that, where is it used?
> > > 
> > As mentioned in [0000 Patch], 'nested_l1' is ident for L1 guest VM,
> > 'nestedl1' is hostname for L1 guest VM.
> > ' store_runvar('nested_l1',$l1->{Guest});' is used to store L1's ident
> > and L1's hostname to database, that will be used for 'selecthost()'
> > function in later script.
> 
> Having a runvar with the bare ident as a name doesn't seem right.
> Perhaps you want to be writing to $r{"${ident}_hostname"} or some such?
> 
> What do you actually need the hostname for anyway?
Look at selecthost()
sub selecthost ($) {
    my ($ident) = @_;
    # must be run outside transaction
    my $name;
    if ($ident =~ m/=/) {
        $ident= $`;
        $name= $'; #'
        $r{$ident}= $name;
    } else {
        $name= $r{$ident};
        die "no specified $ident" unless defined $name;
    }
...

When in L1 iteration invocation of ts-debian-hvm-install(), the ident
passed in is L1 ident ("nested_l1"). Here the 'else' branch will need
$r{$ident}, whose value shall be L1's name. Therefore we prepare this
runvar in advance.
> 
> > > Most places you seem to use nestedl1, not nested_l1. I think you ended
> > > up with this due to not using guest_var and the various hardcoding
> > > you've done elsewhere. I suspect with all that fixed and make-flight
> > > updated accordingly you won't need this var any more.
> > > 
> > I think I should define L1 ident with " my $l1_ident = 'nested_l1' in 'ts-nested-setup'
> 
> Hardcoding anything like that in ts-nested-setup seems wrong to me.
> 
> The ident should come from the script's parameters (i.e. be part of the
> sg-run-job invocation of the script).
> 
> Imagine a hypothetical nested-virt migration test, in that scenario you
> would want to run ts-nested-setup on two hosts, both nestedl1src and
> nestedl2dst in order to configure things. That's why we don't hardcode
> such things.
> 
so to summarize, ts-nested-setup will be invoked with parameters of
$l0_ident, $l1_ident and $l1_name?
or only parameters of $l0_ident and $l1_ident, if we have setup
$l1_ident->$l1_name mapping in runvar when in l0's iteration of
ts-debian-hvm-install.
which would you prefer?
> > 
> > > > +store_runvar("$l1->{Guest}_ip",$l1->{Ip});
> > > > +
> > > > +my $l2_disk_mb = 20000;
> > > > +my $l2_lv_name = 'nestedl2-disk';
> > > > +my $vgname = $l0->{Name};
> > > > +$vgname .= ".$c{TestHostDomain}" if ($l0->{Suite} =~ m/lenny/);
> > > > +target_cmd_root($l0, <<END);
> > > > +    lvremove -f /dev/${vgname}/${l2_lv_name} ||:
> > > > +    lvcreate -L ${l2_disk_mb}M -n $l2_lv_name $vgname
> > > > +    dd if=/dev/zero of=/dev/${vgname}/${l2_lv_name} count=10
> > > 
> > > I think you can do all of this by passing a suitable l2 $gho to
> > > prepareguest_part_lvmdisk, can't you?
> > > 
> > > I think you can get that by my $l2 = selectguest($ARGV[????], $l1).
> > > 
> > I think I could try it, that means I will add more parameters for
> > 'ts-nested-setup' script, not just 'host' and 'nestedl1'.
I think we can pass in 3 parameters: $l0_ident, $l1_ident, and
$l2_ident, as long as we in advance setup their $ident-->$name mappings
in runvar. Here we have 2 options:
1. setup such mapping in first iteration (l0 interation) of
ts-debian-hvm-install
2. setup this in make flight
I prefer 2. since such mapping can be fixed from beginning and shall not
be changed in run time.
> 
> I think so, that seems to make sense since the script is in effect
> acting on three entities.
> 
If we use selectguest($l2_name,$l1_host) in scope of ts-nested-setup,
then we probably need to change 
sub dhcp_watch_setup ($$) {
    my ($ho,$gho) = @_;

    my $meth = get_host_property($ho,'dhcp-watch-method',undef);
--> my $meth = get_target_property($ho,'dhcp-watch-method',undef);
    $gho->{DhcpWatch} = get_host_method_object($ho, 'DhcpWatch', $meth);
}
since here $l1_host is actually a Guest struct rather than Host, it
doesn't have 'dhcp-watch-method'.
Are you OK with this change? I think get_target_property() suites both
situations.
 
> > > Where ARGV[????] is a new parameter passed by sg-run-job i.e. nestedl2,
> > > i.e. the one after whatever ts_get_host_guest consumes at the top of the
> > > file (so ARGV[2] perhaps?).
> > > 
> > Also, I think if use ' prepareguest_part_lvmdisk', I need set
> > 'nestedl2_vg' and 'nestedl2 _disk_lv' by make-flight.
> 
> I think you need to have called guest_find_lv at some point so these are
> set.
> 
> > But, when reuse 'ts-debian-hvm-install' to install L2, the '
> > prepareguest_part_lvmdisk' will be executed again(in
> > 'more_prepareguest_hvm' function), then there will be multi runvars
> > for 'nestedl2_vg' and 'nestedl2_disk_lv' in standalone.db, is that OK?
> > Please correct me if I am wrong.
> 
> You can't end up with multiple runvars, I think the subsequent
> invocations should just be reusing things.
I think not reusing, but override, with same values. So I think it's
fine.
> 
> One wrinkle is whether the lv is in the l0 or l1 "namespace". I'm not
> quite sure how that will fit together.
yeah, you hit it. Theoretically, it shall be in l1 namespace, i.e. using
l1's LV, but since currently L1 doesn't use LV formatting but raw
partition (you mentioned before Wei is doing the uniformity), so here it
is actually in l0's namespace.
In sub guest_find_lv ($) {
    my ($gho) = @_;
    my $gn= $gho->{Guest};
    $gho->{Vg}= $r{"${gn}_vg"};
    $gho->{Lv}= $r{"${gn}_disk_lv"};
    $gho->{Lvdev}= (defined $gho->{Vg} && defined $gho->{Lv})
        ? '/dev/'.$gho->{Vg}.'/'.$gho->{Lv} : undef;
}
you can see l2's Lvdev has different path from l1's, while both of them
provided by l0's LV layer.
> 
> Ian.
> 

Re: [OSSTEST Nested PATCH v8 4/7] Changes on test step of Debian hvm guest install

[Hu, Robert]

> -----Original Message-----
> From: Ian Campbell [mailto:ian.campbell@citrix.com]
> Sent: Thursday, April 23, 2015 2:53 PM
> To: Hu, Robert
> Cc: Pang, LongtaoX; xen-devel@lists.xen.org; Ian.Jackson@eu.citrix.com;
> wei.liu2@citrix.com
> Subject: Re: [OSSTEST Nested PATCH v8 4/7] Changes on test step of Debian
> hvm guest install
> 
> On Thu, 2015-04-23 at 13:59 +0800, Robert Hu wrote:
> > On Tue, 2015-04-21 at 11:28 +0100, Ian Campbell wrote:
> > > On Mon, 2015-04-13 at 17:19 -0400, longtao.pang wrote:
> > > > 1. Increase disk size to accommodate to nested test requirement.
> > > > 2. Since 'Debain-xxx-.iso' image will be stored in rootfs of L1 guest,
> > > > therefore needs more disk capacity, increase root partition size in
> > > > preseed generation.
> > > > 3. In L1 installation context, assign more memory to it; Since it
> > > > acts as a nested hypervisor anyway.
> > > > 4. Comment out CDROM entry in sources.list to make HTTP URL entry
> > > > available for L1 hvm guest.
> > > > 5. Enable nestedhvm feature in ExtraConfig for nested job.
> > > >
> > > > Signed-off-by: longtao.pang <longtaox.pang@intel.com>
> > > > ---
> > > > Changes in v8:
> > > > 1. Update a conventional way to comment out CDROM entry in
> sources.list.
> > > > 2. Enable nestedhvm feature only for the nested job.
> > > > 3. Set nested disk and memroy size to be driven from runvars in
> > > > 'ts-debian-hvm-install'.
> > > > ---
> > > >  ts-debian-hvm-install |    9 ++++++---
> > > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/ts-debian-hvm-install b/ts-debian-hvm-install
> > > > index cfd5144..13009d0 100755
> > > > --- a/ts-debian-hvm-install
> > > > +++ b/ts-debian-hvm-install
> > > > @@ -36,7 +36,7 @@ our $ho= selecthost($whhost);
> > > >
> > > >  # guest memory size will be set based on host free memory, see below
> > > >  our $ram_mb;
> > > > -our $disk_mb= 10000;
> > > > +our $disk_mb= $r{'nested_disk'} ? $r{'nested_disk'} : 10000;
> > >
> > > This shouldn't hardcode 'nested' here, but use the guest ident. (nested
> > > is the wrong name now anyway I think, since it is nestedl1 in this
> > > iteration, which is why we avoid such hardcoding)
> > Right, we shall store this runvar with name of ${l1_ident}_disksize in
> > ts-nested-setup.
> 
> This is the sort of thing which needs to be done from make-flight, not
> ts-*.
> 
Agree.
> > > So you should arrange to do this somewhere that $gho is in scope and use
> > > guest_var($gho, 'disk', 10000). $gho is in scope in $prep where $disk_mb
> > > is used and AFAICT it is only used in that function, so I think you can
> > > move this to a local variable
> > yes, it is supposed to be local of prep(). However, look at other ts-*
> > (ts-debian-install, ts-freebsd-install, etc.), $disk_mb is defined
> > 'our'. They seems to be in alliance, are we going to it specifically in
> > ts-debian-hvm-install? or in some future day, we change these
> > ts-*-install in another patch series?
> 
> If/when the other ts-*-install need to support sizing the disk based on
> a runvar they would need similar changes. If you want to make those now
> for consistency then that would be great, but I don't think it needs to
> be mandatory.
I would prefer to leaving $disk_mb 'our' at present; to be consistent with other
ts-*-installs. 
Meanwhile do the overriding by runvar value in prepareguest().
Are you OK with this?
> 
> > To use guest_var(), it needs $gho in scope as you said. But in prep() we
> > can see until prepareguest() returns, we don't have $gho in scope, while
> > prepareguest() needs $disk_mb as input. So here we get into a loop.
> > I think we shall in prepareguest(), check if $r{${guest_ident}_disksize}
> > is defined, then override passed in $mb value. How would you like this?
> 
> Hrm, perhaps this behaviour ought to be pushed down into prepareguest
> itself, IOW the existing $mb argument to that function would become the
> default, but the runvar would take precedence f set? That would mean
> that all ts-*-install would gain support the runvar.
> 
> Ian, what do you think of this?
> 
> The alternative would be to use $r{${gn}_disksize}.
> 
> > >
> > > >      more_prepareguest_hvm($ho,$gho, $ram_mb, $disk_mb,
> > > >                            OnReboot => 'preserve',
> > > >                            Bios => $r{bios},
> > > > +                          ExtraConfig => $nestedhvm_feature,
> > > >                            PostImageHook => sub {
> > > >          my $cmds = iso_copy_content_from_image($gho, $newiso);
> > > >          $cmds .=
> prepare_initrd($initrddir,$newiso,$preseed_file_path);
> > > > @@ -174,7 +177,7 @@ my $ram_lots = 5000;
> > > >  if ($host_freemem_mb > $ram_lots * 2 + $ram_minslop) {
> > > >      $ram_mb = $ram_lots;
> > > >  } else {
> > > > -    $ram_mb = 768;
> > > > +    $ram_mb = $r{"${gn}_memory"} ? $r{"${gn}_memory"} : 768;
> > >
> > > guest_var again and I think this can be local to prep() too..
> > Also, agree to use guest_var() while may be still be global here as
> > other similar ts-*-install does, change them together in the future.
> 
> This should be handled the same way disk is, however that ends up being.
> 
> Ian.

Re: [OSSTEST Nested PATCH v8 6/7] Compose the main recipe of nested test job

[Robert Hu]

On Wed, 2015-04-22 at 12:23 +0100, Ian Campbell wrote:
> On Wed, 2015-04-22 at 12:04 +0100, Ian Campbell wrote:
> > On Wed, 2015-04-22 at 08:38 +0000, Pang, LongtaoX wrote:
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Ian Campbell [mailto:ian.campbell@citrix.com]
> > > > Sent: Tuesday, April 21, 2015 6:49 PM
> > > > To: Pang, LongtaoX
> > > > Cc: xen-devel@lists.xen.org; Ian.Jackson@eu.citrix.com; wei.liu2@citrix.com; Hu,
> > > > Robert
> > > > Subject: Re: [OSSTEST Nested PATCH v8 6/7] Compose the main recipe of nested
> > > > test job
> > > > 
> > > > On Mon, 2015-04-13 at 17:19 -0400, longtao.pang wrote:
> > > > > Signed-off-by: longtao.pang <longtaox.pang@intel.com>
> > > > > ---
> > > > > Changes in v8:
> > > > > Change the patch order from 6 to 5.
> > > > > ---
> > > > >  sg-run-job |   11 +++++++++++
> > > > >  1 file changed, 11 insertions(+)
> > > > >
> > > > > diff --git a/sg-run-job b/sg-run-job
> > > > > index eae159d..2ca5ebf 100755
> > > > > --- a/sg-run-job
> > > > > +++ b/sg-run-job
> > > > > @@ -299,6 +299,17 @@ proc run-job/test-pair {} {
> > > > >  #    run-ts . remus-failover ts-remus-check         src_host dst_host +
> > > > debian
> > > > >  }
> > > > >
> > > > > +proc need-hosts/test-nested {} {return host}
> > > > > +proc run-job/test-nested {} {
> > > > > +    run-ts . = ts-debian-hvm-install + host + nestedl1
> > > > > +    run-ts . = ts-nested-setup + host + nestedl1
> > > > > +    run-ts . = ts-xen-install nested_l1
> > > > > +    run-ts . = ts-host-reboot nested_l1
> > > > > +    run-ts . = ts-debian-hvm-install nested_l1 nestedl2
> > > > > +    run-ts . = ts-guest-stop nested_l1 nestedl2
> > > > > +    run-ts . = ts-guest-destroy host nestedl1
> > > > 
> > > > Based on my comments to previous patches I think you'll want to use
> > > > nestedl1 and nestedl2 consistently and never nested_l1/nested_l2.
> > > > 
> > > nestedl1 nestedl2 are hostname for L1 guest VM and L2 guest VM,
> > > nested_l1 and nested_l2 are ident for L1 and L2.
> > 
> > Hostnames shouldn't be appearing in this script, I don't think,
> > certainly not bare like that.
> > 
> > For the physical i.e. l0 case I think what happens is that
> > need-hosts/foo ends up returning a bunch of $ident=$an_actual_host. i.e.
> > srchost=$hosta and dsthost=$hostb.
> > 
> > I'm not sure how that would work in the nested world, but maybe you
> > should be writing $ident=$guestname (i.e. nested_l1=nestedl1)?
> > 
> > Ian?
> 
> Ian is still plugging in his computers after moving desks. But having
> spoken to him IRL it seems I'm not quite right above.
> 
> The arguments passed to ts-* in sg-run-job should _always_ be just a
> bare indent. The ident=foo thing is a special case for standalone mode.
> 
Yes, we're going to remove 'name's here. While in make flight, settled
'ident' --> 'name' mapping in runvar, so that when in some ts-* needs
name, we can deduce.
> However, Ian did also say (and I agreed) that there really isn't any
> reason why the ident and the hostname should differ here (or indeed the
> guest name from l0's point of view, modulo the suffix osstest sometimes
> adds).
Pardon? I don't quite understand 'modulo the suffix osstest sometimes
adds'.


> 
> Ian.
> 
> 

Re: [OSSTEST Nested PATCH v8 6/7] Compose the main recipe of nested test job

[Ian Jackson]

Pang, LongtaoX writes ("RE: [Xen-devel] [OSSTEST Nested PATCH v8 6/7] Compose the main recipe of nested test job"):
> Ian Campbell [mailto:ian.campbell@citrix.com]:
> > The arguments passed to ts-* in sg-run-job should _always_ be just a
> > bare indent. The ident=foo thing is a special case for standalone mode.
> > 
> Do you means that the parameter passed to ts-* in sg-run-job should _always_ be just a bare ident?

Yes, that's what Ian means.

> If yes. I think it needs to pass l1's '$gn' which is not a bare ident to 'ts-debian-hvm-install' for installing L1 guest VM, since the default '$gn ||= 'debianhvm' if no other '$gn' was passed to this script, but for nested job we expect a different '$gn' from 'debianhvm', such as 'nestedl1'. So it need to pass 'host' and 'nestedl1' to  'ts-debian-hvm-install' script. 
> +    run-ts . = ts-debian-hvm-install + host + nestedl1

Indeed.

> For 'ts-nested-setup' script, it need to call 'ts_get_host_guest'
> which need two parameters 'host' and 'nestedl1' to get $l0 and $l1
> scope, so it also need to pass 'nestedl1' to this script.

Yes.

> For ' ts-guest-destroy', it also need two parameters of 'host' and 'nestedl1' which is L1's '$gn'.

If you're intending to destroy the l1, then yes.  If you want to
destroy theh l2 then you need to pass `nestedl1' and whatever the L2
guest name/ident is.

Ian.

Re: [OSSTEST Nested PATCH v8 4/7] Changes on test step of Debian hvm guest install

[Ian Jackson]

Ian Campbell writes ("Re: [OSSTEST Nested PATCH v8 4/7] Changes on test step of Debian hvm guest install"):
> Hrm, perhaps this behaviour ought to be pushed down into prepareguest
> itself, IOW the existing $mb argument to that function would become the
> default, but the runvar would take precedence f set? That would mean
> that all ts-*-install would gain support the runvar.
> 
> Ian, what do you think of this?

I think that sounds like a goood idea (although I haven't
double-checked all the code paths being discussed here).

Ian.

Re: [OSSTEST Nested PATCH v8 5/7] Add new script to customize nested test configuration

[Ian Campbell]

On Thu, 2015-04-23 at 17:38 +0800, Robert Hu wrote:
> > > As mentioned in [0000 Patch], 'nested_l1' is ident for L1 guest VM,
> > > 'nestedl1' is hostname for L1 guest VM.
> > > ' store_runvar('nested_l1',$l1->{Guest});' is used to store L1's ident
> > > and L1's hostname to database, that will be used for 'selecthost()'
> > > function in later script.
> > 
> > Having a runvar with the bare ident as a name doesn't seem right.
> > Perhaps you want to be writing to $r{"${ident}_hostname"} or some such?
> > 
> > What do you actually need the hostname for anyway?
> Look at selecthost()
> sub selecthost ($) {
>     my ($ident) = @_;
>     # must be run outside transaction
>     my $name;
>     if ($ident =~ m/=/) {
>         $ident= $`;
>         $name= $'; #'
>         $r{$ident}= $name;
>     } else {
>         $name= $r{$ident};
>         die "no specified $ident" unless defined $name;
>     }
> ...
> 
> When in L1 iteration invocation of ts-debian-hvm-install(), the ident
> passed in is L1 ident ("nested_l1"). Here the 'else' branch will need
> $r{$ident}, whose value shall be L1's name. Therefore we prepare this
> runvar in advance.

Ah I see, today usually the ident is "host" or "dst_host" or "src_host"
so I got confused by it just being "nested_l1" here.

In the normal case today the ident runvars are set by ts-hosts-allocate.
That can't apply to the nested case since it is allocating a guest and
not a host, so your ts-nested-setup seems like a reasonable enough place
to do it.

However, I don't think there is any reason for the indent, hostname and
guest name to differ, and I think perhaps it would be clearer to write
this as:
        our $l1_ident = $gho->{Guest};
        store_runvar($l1_ident, $gho->{Guest});
So that it is clear to the reader that this runvar is an ident. I
suppose a code comment would work as well (or in addition perhaps).

> > 
> > > > Most places you seem to use nestedl1, not nested_l1. I think you ended
> > > > up with this due to not using guest_var and the various hardcoding
> > > > you've done elsewhere. I suspect with all that fixed and make-flight
> > > > updated accordingly you won't need this var any more.
> > > > 
> > > I think I should define L1 ident with " my $l1_ident = 'nested_l1' in 'ts-nested-setup'
> > 
> > Hardcoding anything like that in ts-nested-setup seems wrong to me.
> > 
> > The ident should come from the script's parameters (i.e. be part of the
> > sg-run-job invocation of the script).
> > 
> > Imagine a hypothetical nested-virt migration test, in that scenario you
> > would want to run ts-nested-setup on two hosts, both nestedl1src and
> > nestedl2dst in order to configure things. That's why we don't hardcode
> > such things.
> > 
> so to summarize, ts-nested-setup will be invoked with parameters of
> $l0_ident, $l1_ident and $l1_name?
> or only parameters of $l0_ident and $l1_ident, if we have setup
> $l1_ident->$l1_name mapping in runvar when in l0's iteration of
> ts-debian-hvm-install.
> which would you prefer?

$l1_ident and $l1_name should be the same, I think. Semantically the
script should be taking $l0_ident and $l1_ident, where $l0 here is a
host and $l1 here is a guest, so infact isn't $l1_nae just $l1->{Guest}?

(Things get confusing because $l1 can be a Guest or a Host depending on
which test script we are in)

> > > > > +store_runvar("$l1->{Guest}_ip",$l1->{Ip});
> > > > > +
> > > > > +my $l2_disk_mb = 20000;
> > > > > +my $l2_lv_name = 'nestedl2-disk';
> > > > > +my $vgname = $l0->{Name};
> > > > > +$vgname .= ".$c{TestHostDomain}" if ($l0->{Suite} =~ m/lenny/);
> > > > > +target_cmd_root($l0, <<END);
> > > > > +    lvremove -f /dev/${vgname}/${l2_lv_name} ||:
> > > > > +    lvcreate -L ${l2_disk_mb}M -n $l2_lv_name $vgname
> > > > > +    dd if=/dev/zero of=/dev/${vgname}/${l2_lv_name} count=10
> > > > 
> > > > I think you can do all of this by passing a suitable l2 $gho to
> > > > prepareguest_part_lvmdisk, can't you?
> > > > 
> > > > I think you can get that by my $l2 = selectguest($ARGV[????], $l1).
> > > > 
> > > I think I could try it, that means I will add more parameters for
> > > 'ts-nested-setup' script, not just 'host' and 'nestedl1'.
> I think we can pass in 3 parameters: $l0_ident, $l1_ident, and
> $l2_ident, as long as we in advance setup their $ident-->$name mappings
> in runvar. Here we have 2 options:
> 1. setup such mapping in first iteration (l0 interation) of
> ts-debian-hvm-install
> 2. setup this in make flight
> I prefer 2. since such mapping can be fixed from beginning and shall not
> be changed in run time.

The issue we are coming across here is that we are trying to talk about
the l2 guest before it really exists, since that happens later when
prepareguest is called in the l1 context and here we are really in l0
context where the concept of a specific l2 guest doesn't really exist.
So my suggestion to use prepareguest_part_lvmdisk doesn't really hold
together.

Perhaps we could avoid this whole issue by avoiding any reference to a
specific l2 guest at this point completely, and just consider things as
"guest storage for use by l1 hypervisor".

IOW the naming would be based on the l1 ident and some suffix, e.g.
"_guest_storage". So, given $l1_ident from above, you would do:

$guest_storage_lv_name = "${l1_ident}_guest_storage"
# make the LV
store_runvar("${l1_ident}_guest_storage_lv", $guest_storage_lv_name)
store_runvar("${l1_ident}_guest_storage_vdev", "xvde")

(for whichever info needs preserving for later)

> > 
> > I think so, that seems to make sense since the script is in effect
> > acting on three entities.
> > 
> If we use selectguest($l2_name,$l1_host) in scope of ts-nested-setup,
> then we probably need to change 
> sub dhcp_watch_setup ($$) {
>     my ($ho,$gho) = @_;
> 
>     my $meth = get_host_property($ho,'dhcp-watch-method',undef);
> --> my $meth = get_target_property($ho,'dhcp-watch-method',undef);
>     $gho->{DhcpWatch} = get_host_method_object($ho, 'DhcpWatch', $meth);
> }
> since here $l1_host is actually a Guest struct rather than Host, it
> doesn't have 'dhcp-watch-method'.
> Are you OK with this change? I think get_target_property() suites both
> situations.

I think the discussion above my have invalidate the need to do this, but
I think there wouldn't be much harm in having the $l1 Guest struct take
on some of the $l0 Host struct's properties, e.g. copy over some list of
which dhcp-watch-method would be one.

This is the same problem I mentioned above arising from $l1 doing double
duty as both host and guest. There are places in osstest which are
pretty fluid about this (if the object has the approrpaite props you can
use it as either), but it can be a bit confusing for the poor programmer
or reader...

Perhaps having a
   our $l1ho = make_nested_host($l0ho, $l1guest);
construct, which does copies $l1guest and propagates some properties as
above and then using $l1guest + $l1host in the appropriate places only
would help keep things straight?

(Best to wait for Ian's feedback on this before doing anything, he may
not like it)

Ian.

Re: [OSSTEST Nested PATCH v8 5/7] Add new script to customize nested test configuration

[Ian Campbell]

On Thu, 2015-04-23 at 07:27 +0000, Pang, LongtaoX wrote:
> 
> 
> > -----Original Message-----
> > From: Ian Campbell [mailto:ian.campbell@citrix.com]
> > Sent: Tuesday, April 21, 2015 6:40 PM
> > To: Pang, LongtaoX
> > Cc: xen-devel@lists.xen.org; Ian.Jackson@eu.citrix.com; wei.liu2@citrix.com; Hu,
> > Robert
> > Subject: Re: [OSSTEST Nested PATCH v8 5/7] Add new script to customize nested
> > test configuration
> > > Signed-off-by: longtao.pang <longtaox.pang@intel.com>
> > > ---
> > > Changes in v8:
> > > 1. Replace '$nested_host' by '$l1->{Guest}'.
> > > ---
> > >  ts-nested-setup |   52
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 52 insertions(+)
> > >  create mode 100755 ts-nested-setup
> > >
> > > diff --git a/ts-nested-setup b/ts-nested-setup
> > > new file mode 100755
> > > index 0000000..41d5e80
> > > --- /dev/null
> > > +++ b/ts-nested-setup
> > > @@ -0,0 +1,52 @@
> > > +#!/usr/bin/perl -w
> > > +
> > > +use strict qw(vars);
> > > +use DBI;
> > > +use Osstest;
> > > +use Osstest::Debian;
> > > +use Osstest::TestSupport;
> > > +
> > > +tsreadconfig();
> > > +our ($l0,$l1) = ts_get_host_guest(@ARGV);
> > > +
> > > +guest_check_ip($l1);
> > > +target_cmd_root($l1, "mkdir -p /home/osstest/.ssh && cp
> > /root/.ssh/authorized_keys /home/osstest/.ssh/");
> > > +my $url =
> > $c{WebspaceUrl}.$c{WebspaceCommon}.$l0->{Name}."_".'overlay.tar';
> > > +target_cmd_root($l1, <<END);
> > > +    wget -O overlay.tar $url
> > > +    tar -xf overlay.tar -C /
> > > +    rm overlay.tar -f
> > > +    update-rc.d osstest-confirm-booted start 99 2 .
> > > +END
> > 
> > I cc'd you on some patches which I think should help avoid this
> > duplication.
> > 
> For this question, I have merged the v5_patches[04,05,06] which you CC'd to me. 
> Based on your patches, after finishing installing L1 hvm guest VM with 'ts-debian-hvm-install' script, I could ssh into L1 guest as 'osstest' user without password, that means I will not need to use below code anymore
> target_cmd_root($l1, "mkdir -p /home/osstest/.ssh && cp /root/.ssh/authorized_keys /home/osstest/.ssh/");

Great.

> 
> But, inside L1 guest VM, the overly files(osstest-confirm-booted,
> xenbridge, xenlightdaemons ) does not exist at " /etc/init.d"
> directory. Since 'ts-host-reboot' script will use
> 'osstest-confirm-booted' shell script to confirm whether L1 guest boot
> up normally, these overlay files are necessary here.
> If I add below patch based on your patches, and install L1 hvm guest
> VM again, all the overly files exist in "/etc/init.d" directory inside
> L1 guest.

That sounds ok to me, the overlay contains some host level stuff but it
is either stuff which is useful at guest level too
(osstest-configmr-booted) or harmless (most of the rest).

If we think that is a problem then splitting into overlay-host,
overlay-guest and overlay-common would be one way to address that.

Although I would have expect the preseed_hook_overlay to be removed from
elsewhere too?
> diff --git a/Osstest/Debian.pm b/Osstest/Debian.pm
> index 6691ff6..4af6957 100644
> --- a/Osstest/Debian.pm
> +++ b/Osstest/Debian.pm
> @@ -624,6 +624,7 @@ sub preseed_base ($$$$;@) {
> 
>      preseed_ssh($ho, $sfx);
>      preseed_hook_overlay($ho, $sfx, $c{OverlayLocal}, 'overlay-local.tar');
> +    preseed_hook_overlay($ho, $sfx, 'overlay', 'overlay.tar');
> 
>      my $preseed = <<"END";
>  d-i mirror/suite string $suite
> 
> Another question, based on your patches, I find that the below
> commands under ' d-i preseed/late_command string \\' do not work
> anymore in preseed () ' of 'ts-debian-hvm-install' script. For
> example, after finishing installing L1 guest, there is no directory of
> '/boot/efi/EFI/boot' created and 'sources.list' does not be edited by
> sed inside L1 guest. I think you have verified this, maybe something
> wrong of my test environment to cause the question?

I'm not sure. I suspect the issue is that multiple preseed/late_commands
are not supported and the osstest preseed hook commands have used it
already.

IOW the below should probably be switch to use preseed_hook_command($ho,
'late_command'. Do you think this is a bug in my patches or yours?
Smells like mine on first glance.

> d-i preseed/late_command string \\
>         in-target mkdir -p /boot/efi/EFI/boot; \\
>         in-target cp /boot/efi/EFI/debian/grubx64.efi /boot/efi/EFI/boot/bootx64.efi ;\\
>         in-target sed -i 's/^deb *cdrom/#&/g' /etc/apt/sources.list;
> END

Re: [OSSTEST Nested PATCH v8 4/7] Changes on test step of Debian hvm guest install

[Ian Campbell]

On Thu, 2015-04-23 at 10:43 +0000, Hu, Robert wrote:
> > If/when the other ts-*-install need to support sizing the disk based on
> > a runvar they would need similar changes. If you want to make those now
> > for consistency then that would be great, but I don't think it needs to
> > be mandatory.
> I would prefer to leaving $disk_mb 'our' at present; to be consistent with other
> ts-*-installs. 
> Meanwhile do the overriding by runvar value in prepareguest().
> Are you OK with this?

It sounds like Ian is OK with this idea at least in principal, so sure.
Please do this the same for both disk and memory.

Ian.

Re: [OSSTEST Nested PATCH v8 5/7] Add new script to customize nested test configuration

[Ian Campbell]

On Thu, 2015-04-23 at 12:30 +0100, Ian Campbell wrote:
> This is the same problem I mentioned above arising from $l1 doing double
> duty as both host and guest. There are places in osstest which are
> pretty fluid about this (if the object has the approrpaite props you can
> use it as either), but it can be a bit confusing for the poor programmer
> or reader...
> 
> Perhaps having a
>    our $l1ho = make_nested_host($l0ho, $l1guest);
> construct, which does copies $l1guest and propagates some properties as
> above and then using $l1guest + $l1host in the appropriate places only
> would help keep things straight?

I had a thought here: would it be at all useful to add some light syntax
to the idents to help represent the nested scenario in sg-run-jobs and
to make select* behave more naturally in these cases?

e.g. "l0host:l1guest" as a host ident would, when given to selecthost,
return a $ho relating to the nested l1 hypervisor running in the guest
with ident l1guest on the physical host with ident l0host. Certain
properties of the l0host (such as dhcp-watch-method) would automatically
be applied to that host.

Along those lines ts_get_host_guest would handle ("l0host:l1guest",
"l2guest") by returning a host object for the l1 hypervisor running in
l1guest on l0host and a guest object for the l2guest. This falls out
basically for free given ts_get_host_guest just calls selecthost.

I considered whether a similar syntax for a guest ident, i.e.
"l1host:l2guest" would be useful, but given that selectguest already
takes the host (in this case that would be l1host) as an argument I
couldn't see the need.

> (Best to wait for Ian's feedback on this before doing anything, he may
> not like it)

This is still true of course...

Ian.

Re: [OSSTEST Nested PATCH v8 6/7] Compose the main recipe of nested test job

[Robert Hu]

On Thu, 2015-04-23 at 12:05 +0100, Ian Jackson wrote:
> Pang, LongtaoX writes ("RE: [Xen-devel] [OSSTEST Nested PATCH v8 6/7] Compose the main recipe of nested test job"):
> > Ian Campbell [mailto:ian.campbell@citrix.com]:
> > > The arguments passed to ts-* in sg-run-job should _always_ be just a
> > > bare indent. The ident=foo thing is a special case for standalone mode.
> > > 
> > Do you means that the parameter passed to ts-* in sg-run-job should _always_ be just a bare ident?
> 
> Yes, that's what Ian means.
> 
> > If yes. I think it needs to pass l1's '$gn' which is not a bare ident to 'ts-debian-hvm-install' for installing L1 guest VM, since the default '$gn ||= 'debianhvm' if no other '$gn' was passed to this script, but for nested job we expect a different '$gn' from 'debianhvm', such as 'nestedl1'. So it need to pass 'host' and 'nestedl1' to  'ts-debian-hvm-install' script. 
> > +    run-ts . = ts-debian-hvm-install + host + nestedl1
> 
> Indeed.
> 
> > For 'ts-nested-setup' script, it need to call 'ts_get_host_guest'
> > which need two parameters 'host' and 'nestedl1' to get $l0 and $l1
> > scope, so it also need to pass 'nestedl1' to this script.
> 
> Yes.
> 
> > For ' ts-guest-destroy', it also need two parameters of 'host' and 'nestedl1' which is L1's '$gn'.
> 
> If you're intending to destroy the l1, then yes.  If you want to
> destroy theh l2 then you need to pass `nestedl1' and whatever the L2
> guest name/ident is.
Here comes a common issue we need to settle down:
To not break the principle that we shall only pass in ident in recipe,
we shall either
1. for l1 and l2, identical their ident/name/hostname, make this a
principle as well. This shall avoid much complex design but adding one
additional rule to bear in mind.
2. setup $ident --> $name mapping somewhere at the beginning (either
alloc host or make flight, you name it, but somewhere).

I prefer Ian C's suggestion, option 1, that why should we differ the
ident and name for l1 and l2.
> 
> Ian.

Re: [OSSTEST Nested PATCH v8 5/7] Add new script to customize nested test configuration

[Pang, LongtaoX]

> -----Original Message-----
> From: Ian Campbell [mailto:ian.campbell@citrix.com]
> Sent: Thursday, April 23, 2015 7:30 PM
> To: Hu, Robert
> Cc: Pang, LongtaoX; xen-devel@lists.xen.org; Ian.Jackson@eu.citrix.com;
> wei.liu2@citrix.com
> Subject: Re: [OSSTEST Nested PATCH v8 5/7] Add new script to customize nested
> test configuration
> 
> On Thu, 2015-04-23 at 17:38 +0800, Robert Hu wrote:
> > > > As mentioned in [0000 Patch], 'nested_l1' is ident for L1 guest VM,
> > > > 'nestedl1' is hostname for L1 guest VM.
> > > > ' store_runvar('nested_l1',$l1->{Guest});' is used to store L1's ident
> > > > and L1's hostname to database, that will be used for 'selecthost()'
> > > > function in later script.
> > >
> > > Having a runvar with the bare ident as a name doesn't seem right.
> > > Perhaps you want to be writing to $r{"${ident}_hostname"} or some such?
> > >
> > > What do you actually need the hostname for anyway?
> > Look at selecthost()
> > sub selecthost ($) {
> >     my ($ident) = @_;
> >     # must be run outside transaction
> >     my $name;
> >     if ($ident =~ m/=/) {
> >         $ident= $`;
> >         $name= $'; #'
> >         $r{$ident}= $name;
> >     } else {
> >         $name= $r{$ident};
> >         die "no specified $ident" unless defined $name;
> >     }
> > ...
> >
> > When in L1 iteration invocation of ts-debian-hvm-install(), the ident
> > passed in is L1 ident ("nested_l1"). Here the 'else' branch will need
> > $r{$ident}, whose value shall be L1's name. Therefore we prepare this
> > runvar in advance.
> 
> Ah I see, today usually the ident is "host" or "dst_host" or "src_host"
> so I got confused by it just being "nested_l1" here.
> 
> In the normal case today the ident runvars are set by ts-hosts-allocate.
> That can't apply to the nested case since it is allocating a guest and
> not a host, so your ts-nested-setup seems like a reasonable enough place
> to do it.
> 
> However, I don't think there is any reason for the indent, hostname and
> guest name to differ, and I think perhaps it would be clearer to write
> this as:
>         our $l1_ident = $gho->{Guest};
>         store_runvar($l1_ident, $gho->{Guest});
> So that it is clear to the reader that this runvar is an ident. I
> suppose a code comment would work as well (or in addition perhaps).
> 
> > >
> > > > > Most places you seem to use nestedl1, not nested_l1. I think you ended
> > > > > up with this due to not using guest_var and the various hardcoding
> > > > > you've done elsewhere. I suspect with all that fixed and make-flight
> > > > > updated accordingly you won't need this var any more.
> > > > >
> > > > I think I should define L1 ident with " my $l1_ident = 'nested_l1' in
> 'ts-nested-setup'
> > >
> > > Hardcoding anything like that in ts-nested-setup seems wrong to me.
> > >
> > > The ident should come from the script's parameters (i.e. be part of the
> > > sg-run-job invocation of the script).
> > >
> > > Imagine a hypothetical nested-virt migration test, in that scenario you
> > > would want to run ts-nested-setup on two hosts, both nestedl1src and
> > > nestedl2dst in order to configure things. That's why we don't hardcode
> > > such things.
> > >
> > so to summarize, ts-nested-setup will be invoked with parameters of
> > $l0_ident, $l1_ident and $l1_name?
> > or only parameters of $l0_ident and $l1_ident, if we have setup
> > $l1_ident->$l1_name mapping in runvar when in l0's iteration of
> > ts-debian-hvm-install.
> > which would you prefer?
> 
> $l1_ident and $l1_name should be the same, I think. Semantically the
> script should be taking $l0_ident and $l1_ident, where $l0 here is a
> host and $l1 here is a guest, so infact isn't $l1_nae just $l1->{Guest}?
> 
> (Things get confusing because $l1 can be a Guest or a Host depending on
> which test script we are in)
> 
> > > > > > +store_runvar("$l1->{Guest}_ip",$l1->{Ip});
> > > > > > +
> > > > > > +my $l2_disk_mb = 20000;
> > > > > > +my $l2_lv_name = 'nestedl2-disk';
> > > > > > +my $vgname = $l0->{Name};
> > > > > > +$vgname .= ".$c{TestHostDomain}" if ($l0->{Suite} =~ m/lenny/);
> > > > > > +target_cmd_root($l0, <<END);
> > > > > > +    lvremove -f /dev/${vgname}/${l2_lv_name} ||:
> > > > > > +    lvcreate -L ${l2_disk_mb}M -n $l2_lv_name $vgname
> > > > > > +    dd if=/dev/zero of=/dev/${vgname}/${l2_lv_name} count=10
> > > > >
> > > > > I think you can do all of this by passing a suitable l2 $gho to
> > > > > prepareguest_part_lvmdisk, can't you?
> > > > >
> > > > > I think you can get that by my $l2 = selectguest($ARGV[????], $l1).
> > > > >
> > > > I think I could try it, that means I will add more parameters for
> > > > 'ts-nested-setup' script, not just 'host' and 'nestedl1'.
> > I think we can pass in 3 parameters: $l0_ident, $l1_ident, and
> > $l2_ident, as long as we in advance setup their $ident-->$name mappings
> > in runvar. Here we have 2 options:
> > 1. setup such mapping in first iteration (l0 interation) of
> > ts-debian-hvm-install
> > 2. setup this in make flight
> > I prefer 2. since such mapping can be fixed from beginning and shall not
> > be changed in run time.
> 
> The issue we are coming across here is that we are trying to talk about
> the l2 guest before it really exists, since that happens later when
> prepareguest is called in the l1 context and here we are really in l0
> context where the concept of a specific l2 guest doesn't really exist.
> So my suggestion to use prepareguest_part_lvmdisk doesn't really hold
> together.
>
So, is it means that will not use ' prepareguest_part_lvmdisk' to create lv storage disk inside L0 host, which used for installing L2 guest, right?

> Perhaps we could avoid this whole issue by avoiding any reference to a
> specific l2 guest at this point completely, and just consider things as
> "guest storage for use by l1 hypervisor".
> 
Yes, I think so.

> IOW the naming would be based on the l1 ident and some suffix, e.g.
> "_guest_storage". So, given $l1_ident from above, you would do:
> 
> $guest_storage_lv_name = "${l1_ident}_guest_storage"
> # make the LV
> store_runvar("${l1_ident}_guest_storage_lv", $guest_storage_lv_name)
> store_runvar("${l1_ident}_guest_storage_vdev", "xvde")
> 
Could you please make it clear, what's it used for that store above two runvars?

> (for whichever info needs preserving for later)
> 
> > >
> > > I think so, that seems to make sense since the script is in effect
> > > acting on three entities.
> > >
> > If we use selectguest($l2_name,$l1_host) in scope of ts-nested-setup,
> > then we probably need to change
> > sub dhcp_watch_setup ($$) {
> >     my ($ho,$gho) = @_;
> >
> >     my $meth = get_host_property($ho,'dhcp-watch-method',undef);
> > --> my $meth = get_target_property($ho,'dhcp-watch-method',undef);
> >     $gho->{DhcpWatch} = get_host_method_object($ho, 'DhcpWatch',
> $meth);
> > }
> > since here $l1_host is actually a Guest struct rather than Host, it
> > doesn't have 'dhcp-watch-method'.
> > Are you OK with this change? I think get_target_property() suites both
> > situations.
> 
> I think the discussion above my have invalidate the need to do this, but
> I think there wouldn't be much harm in having the $l1 Guest struct take
> on some of the $l0 Host struct's properties, e.g. copy over some list of
> which dhcp-watch-method would be one.
> 
> This is the same problem I mentioned above arising from $l1 doing double
> duty as both host and guest. There are places in osstest which are
> pretty fluid about this (if the object has the approrpaite props you can
> use it as either), but it can be a bit confusing for the poor programmer
> or reader...
> 
> Perhaps having a
>    our $l1ho = make_nested_host($l0ho, $l1guest);
> construct, which does copies $l1guest and propagates some properties as
> above and then using $l1guest + $l1host in the appropriate places only
> would help keep things straight?
> 
> (Best to wait for Ian's feedback on this before doing anything, he may
> not like it)
> 
> Ian.

Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm for nested test

[Robert Hu]

On Thu, 2015-04-23 at 00:34 +0000, Hu, Robert wrote:
> > -----Original Message-----
> > From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> > Sent: Wednesday, April 22, 2015 8:50 PM
> > To: Ian Campbell
> > Cc: Pang, LongtaoX; xen-devel@lists.xen.org; wei.liu2@citrix.com; Hu, Robert
> > Subject: Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm
> > for nested test
> > 
> > Ian Campbell writes ("Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in
> > TestSupport.pm for nested test"):
> > > It will, I think, need to be integrated with the existing assignment to
> > > $ho->{Ip} in select host, so something like:
> > >
> > >         if ( $r{"${ident}_ip"} ) {
> > >             $ho->{Ip}= $r{"${ident}_ip"};
> > >         } else {
> > >             $ho->{Ip}= $ho->{IpStatic};
> > >         }
> > 
> > Yes.
> Yes, otherwise the code would go 'die' somewhere later.
Seems inevitably run into die in our nested test case, since our l1 host
shall not have IpStatic, while currently seems this is a necessary.
$ho->{IpStatic} = get_host_property($ho,'ip-addr');
    if (!defined $ho->{IpStatic}) {
	my $ip_packed= gethostbyname($ho->{Fqdn});
	die "$ho->{Fqdn} ?" unless $ip_packed;
	$ho->{IpStatic}= inet_ntoa($ip_packed);
	die "$ho->{Fqdn} ?" unless defined $ho->{IpStatic};
    }
Shall we loosen this for nested l1 host? 
i.e. die "$ho->{Fqdn} ?" unless $ip_packed ||
$r{$ident_enable_nestedhvm} eq 'true';
> > 
> > > or perhaps:
> > >
> > >         $ho->{Ip} = $r{"${ident}_ip"} ? $r{"${ident}_ip"} : $ho->{IpStatic};
> > 
> > The shortest way to spell this is to use //, eg:
> > 
> >       $ho->{Ip} = $r{"${ident}_ip"} // $ho->{IpStatic};
> Ah, yes!
> > 
> > Ian.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [OSSTEST Nested PATCH v8 5/7] Add new script to customize nested test configuration

[Ian Campbell]

On Fri, 2015-04-24 at 08:45 +0000, Pang, LongtaoX wrote:
> 
> 
> > -----Original Message-----
> > From: Ian Campbell [mailto:ian.campbell@citrix.com]
> > Sent: Thursday, April 23, 2015 7:30 PM
> > To: Hu, Robert
> > Cc: Pang, LongtaoX; xen-devel@lists.xen.org; Ian.Jackson@eu.citrix.com;
> > wei.liu2@citrix.com
> > Subject: Re: [OSSTEST Nested PATCH v8 5/7] Add new script to customize nested
> > test configuration
> > 
> > On Thu, 2015-04-23 at 17:38 +0800, Robert Hu wrote:
> > > > > As mentioned in [0000 Patch], 'nested_l1' is ident for L1 guest VM,
> > > > > 'nestedl1' is hostname for L1 guest VM.
> > > > > ' store_runvar('nested_l1',$l1->{Guest});' is used to store L1's ident
> > > > > and L1's hostname to database, that will be used for 'selecthost()'
> > > > > function in later script.
> > > >
> > > > Having a runvar with the bare ident as a name doesn't seem right.
> > > > Perhaps you want to be writing to $r{"${ident}_hostname"} or some such?
> > > >
> > > > What do you actually need the hostname for anyway?
> > > Look at selecthost()
> > > sub selecthost ($) {
> > >     my ($ident) = @_;
> > >     # must be run outside transaction
> > >     my $name;
> > >     if ($ident =~ m/=/) {
> > >         $ident= $`;
> > >         $name= $'; #'
> > >         $r{$ident}= $name;
> > >     } else {
> > >         $name= $r{$ident};
> > >         die "no specified $ident" unless defined $name;
> > >     }
> > > ...
> > >
> > > When in L1 iteration invocation of ts-debian-hvm-install(), the ident
> > > passed in is L1 ident ("nested_l1"). Here the 'else' branch will need
> > > $r{$ident}, whose value shall be L1's name. Therefore we prepare this
> > > runvar in advance.
> > 
> > Ah I see, today usually the ident is "host" or "dst_host" or "src_host"
> > so I got confused by it just being "nested_l1" here.
> > 
> > In the normal case today the ident runvars are set by ts-hosts-allocate.
> > That can't apply to the nested case since it is allocating a guest and
> > not a host, so your ts-nested-setup seems like a reasonable enough place
> > to do it.
> > 
> > However, I don't think there is any reason for the indent, hostname and
> > guest name to differ, and I think perhaps it would be clearer to write
> > this as:
> >         our $l1_ident = $gho->{Guest};
> >         store_runvar($l1_ident, $gho->{Guest});
> > So that it is clear to the reader that this runvar is an ident. I
> > suppose a code comment would work as well (or in addition perhaps).
> > 
> > > >
> > > > > > Most places you seem to use nestedl1, not nested_l1. I think you ended
> > > > > > up with this due to not using guest_var and the various hardcoding
> > > > > > you've done elsewhere. I suspect with all that fixed and make-flight
> > > > > > updated accordingly you won't need this var any more.
> > > > > >
> > > > > I think I should define L1 ident with " my $l1_ident = 'nested_l1' in
> > 'ts-nested-setup'
> > > >
> > > > Hardcoding anything like that in ts-nested-setup seems wrong to me.
> > > >
> > > > The ident should come from the script's parameters (i.e. be part of the
> > > > sg-run-job invocation of the script).
> > > >
> > > > Imagine a hypothetical nested-virt migration test, in that scenario you
> > > > would want to run ts-nested-setup on two hosts, both nestedl1src and
> > > > nestedl2dst in order to configure things. That's why we don't hardcode
> > > > such things.
> > > >
> > > so to summarize, ts-nested-setup will be invoked with parameters of
> > > $l0_ident, $l1_ident and $l1_name?
> > > or only parameters of $l0_ident and $l1_ident, if we have setup
> > > $l1_ident->$l1_name mapping in runvar when in l0's iteration of
> > > ts-debian-hvm-install.
> > > which would you prefer?
> > 
> > $l1_ident and $l1_name should be the same, I think. Semantically the
> > script should be taking $l0_ident and $l1_ident, where $l0 here is a
> > host and $l1 here is a guest, so infact isn't $l1_nae just $l1->{Guest}?
> > 
> > (Things get confusing because $l1 can be a Guest or a Host depending on
> > which test script we are in)
> > 
> > > > > > > +store_runvar("$l1->{Guest}_ip",$l1->{Ip});
> > > > > > > +
> > > > > > > +my $l2_disk_mb = 20000;
> > > > > > > +my $l2_lv_name = 'nestedl2-disk';
> > > > > > > +my $vgname = $l0->{Name};
> > > > > > > +$vgname .= ".$c{TestHostDomain}" if ($l0->{Suite} =~ m/lenny/);
> > > > > > > +target_cmd_root($l0, <<END);
> > > > > > > +    lvremove -f /dev/${vgname}/${l2_lv_name} ||:
> > > > > > > +    lvcreate -L ${l2_disk_mb}M -n $l2_lv_name $vgname
> > > > > > > +    dd if=/dev/zero of=/dev/${vgname}/${l2_lv_name} count=10
> > > > > >
> > > > > > I think you can do all of this by passing a suitable l2 $gho to
> > > > > > prepareguest_part_lvmdisk, can't you?
> > > > > >
> > > > > > I think you can get that by my $l2 = selectguest($ARGV[????], $l1).
> > > > > >
> > > > > I think I could try it, that means I will add more parameters for
> > > > > 'ts-nested-setup' script, not just 'host' and 'nestedl1'.
> > > I think we can pass in 3 parameters: $l0_ident, $l1_ident, and
> > > $l2_ident, as long as we in advance setup their $ident-->$name mappings
> > > in runvar. Here we have 2 options:
> > > 1. setup such mapping in first iteration (l0 interation) of
> > > ts-debian-hvm-install
> > > 2. setup this in make flight
> > > I prefer 2. since such mapping can be fixed from beginning and shall not
> > > be changed in run time.
> > 
> > The issue we are coming across here is that we are trying to talk about
> > the l2 guest before it really exists, since that happens later when
> > prepareguest is called in the l1 context and here we are really in l0
> > context where the concept of a specific l2 guest doesn't really exist.
> > So my suggestion to use prepareguest_part_lvmdisk doesn't really hold
> > together.
> >
> So, is it means that will not use ' prepareguest_part_lvmdisk' to
> create lv storage disk inside L0 host, which used for installing L2
> guest, right?

Right. It might be nice to refactor some of prepareguest_part_lvmdisk
into a more generic helper which can be called to create an arbitrary
LVM LV though.

> > IOW the naming would be based on the l1 ident and some suffix, e.g.
> > "_guest_storage". So, given $l1_ident from above, you would do:
> > 
> > $guest_storage_lv_name = "${l1_ident}_guest_storage"
> > # make the LV
> > store_runvar("${l1_ident}_guest_storage_lv", $guest_storage_lv_name)
> > store_runvar("${l1_ident}_guest_storage_vdev", "xvde")
> > 
> Could you please make it clear, what's it used for that store above two runvars?

Those were just two random examples of properties of the second device
which I thought other code _might_ be interested in which I picked to
illustrate what I was talking about, they might not be needed in
reality.

Ian.

Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm for nested test

[Ian Campbell]

On Mon, 2015-04-27 at 17:36 +0800, Robert Hu wrote:
> On Thu, 2015-04-23 at 00:34 +0000, Hu, Robert wrote:
> > > -----Original Message-----
> > > From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> > > Sent: Wednesday, April 22, 2015 8:50 PM
> > > To: Ian Campbell
> > > Cc: Pang, LongtaoX; xen-devel@lists.xen.org; wei.liu2@citrix.com; Hu, Robert
> > > Subject: Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm
> > > for nested test
> > > 
> > > Ian Campbell writes ("Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in
> > > TestSupport.pm for nested test"):
> > > > It will, I think, need to be integrated with the existing assignment to
> > > > $ho->{Ip} in select host, so something like:
> > > >
> > > >         if ( $r{"${ident}_ip"} ) {
> > > >             $ho->{Ip}= $r{"${ident}_ip"};
> > > >         } else {
> > > >             $ho->{Ip}= $ho->{IpStatic};
> > > >         }
> > > 
> > > Yes.
> > Yes, otherwise the code would go 'die' somewhere later.
> Seems inevitably run into die in our nested test case, since our l1 host
> shall not have IpStatic, while currently seems this is a necessary.
> $ho->{IpStatic} = get_host_property($ho,'ip-addr');
>     if (!defined $ho->{IpStatic}) {
> 	my $ip_packed= gethostbyname($ho->{Fqdn});
> 	die "$ho->{Fqdn} ?" unless $ip_packed;
> 	$ho->{IpStatic}= inet_ntoa($ip_packed);
> 	die "$ho->{Fqdn} ?" unless defined $ho->{IpStatic};
>     }
> Shall we loosen this for nested l1 host? 
> i.e. die "$ho->{Fqdn} ?" unless $ip_packed ||
> $r{$ident_enable_nestedhvm} eq 'true';

I think all of the code you mentioned above should actually be in the
else case of the example I gave, before the "$ho->{Ip}=
$ho->{IpStatic};"

Ian.