[PATCH OSSTEST 00/12] Add nested xen on xen test case

[PATCH OSSTEST 00/12] Add nested xen on xen test case

[Robert Ho]

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.

In this test set, we add a build job 'build-amd64-hvm' to 
build hvm kernel for nest test steps use.

Test steps
	0. run 'build-amd64' job and 'build-amd64-hvm', to prepare xen 
installation
	   tarball and hvm guest kernel
	1. 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-xen-install' to install xen on 
		   the normal guest, alter it into a L1 hypervisor
		c. invoke test step of 'ts-debain-hvm-install' again, but 
	           take the L1 hypervisor as host, install the L2 guest on it
		d. invoke test step of 'ts-guest-destroy', clean up L2 guest.

This patch set reuses 'ts-debian-hvm-install' for both L1 host installation 
and L2 host installation, using 'nested' input param with value of 'nested_L1'
 and 'nested_L1' to distinguish the 2 context.
It also reuses 'ts-xen-install' with 'nested' input param to differentiate L1
 Xen installation from L0 Xen installation.
This patch series  has been tested on test machines of amd64 arch, with hvm 
domain0 of Linux kernel 3.18.5, in standalone mode.


Robert Ho (12):
      Add support of parsing grub which has 'submenu' primitive
      In nested test case, guest boot will take more time.
      Designate vif device model to e1000.   
      Just some indentation adustments.
      Add and expose some testsupport APIs
      Manipulate $ho IP assignment for nest L2 situation
      For hvm guest configuration, config console to 'hvc0'
      Add test job for nest test case.
      Add build-debain-hvm build job.
      Compose the main body of test-nested test job.  
      Changes on test step of debain hvm guest install
      Changes to test step of xen install


Osstest/Debian.pm      |  60 +++++++++++++++++++++++++++++++++-----------------------
 Osstest/TestSupport.pm |  34 ++++++++++++++++++++++++--------
 make-flight            |  20 +++++++++++++++++++
 mfi-common             |   8 ++++++++
 sg-run-job             |   8 ++++++++
 ts-debian-hvm-install  |  63 +++++++++++++++++++++++++++++++++++++++++++++++++----------
 ts-xen-install         | 126 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------
 7 files changed, 230 insertions(+), 89 deletions(-)

[PATCH OSSTEST 01/12] Add support of parsing grub which has 'submenu' primitive

[Robert Ho]

 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. Also, this patch adjust
 some indent alignments.

---
 Osstest/Debian.pm | 60 +++++++++++++++++++++++++++++++++----------------------
 1 file changed, 36 insertions(+), 24 deletions(-)

diff --git a/Osstest/Debian.pm b/Osstest/Debian.pm
index 3e2d1c3..b85b9fd 100644
--- a/Osstest/Debian.pm
+++ b/Osstest/Debian.pm
@@ -1,5 +1,6 @@
 # This is part of "osstest", an automated testing framework for Xen.
 # Copyright (C) 2009-2013 Citrix Inc.
+# Copyright (C) 2014-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
@@ -286,26 +287,34 @@ 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
-			 ? qw(Title Hv KernDom0 KernVer)
-			 : qw(Title Hv KernOnly KernVer));
-		if (@missing) {
-		    logm("(skipping entry at $entry->{StartLine};".
-			 " no @missing)");
-		} elsif (defined $want_kernver &&
-			 $entry->{KernVer} ne $want_kernver) {
-		    logm("(skipping entry at $entry->{StartLine};".
-			 " kernel $entry->{KernVer}, not $want_kernver)");
-		} else {
-		    # yes!
-		    last;
-		}
+        		    grep { !defined $entry->{$_} }
+        		        (defined $xenhopt 
+        		        ? qw(Title Hv KernDom0 KernVer)
+        		        : qw(Title Hv KernOnly KernVer));
+        		if (@missing) {
+        		    logm("(skipping entry at $entry->{StartLine};".
+            			 " no @missing)");
+        		} elsif (defined $want_kernver &&
+    			 $entry->{KernVer} ne $want_kernver) {
+        		    logm("(skipping entry at $entry->{StartLine};".
+        			 " kernel $entry->{KernVer}, not $want_kernver)");
+        		} else {
+    		    # yes!
+    		    last;
+        		}
                 $entry= undef;
                 next;
             }
@@ -317,21 +326,24 @@ sub setupboot_grub2 ($$$) {
                 $entry= { Title => $1, StartLine => $., Number => $count };
                 $count++;
             }
-            if (m/^\s*multiboot\s*\/(xen\-[0-9][-+.0-9a-z]*\S+)/) {
+    	    if(m/^submenu\s+[\'\"](.*)[\'\"].*\{\s*$/){
+        		$submenu={ StartLine =>$.};
+    	    }
+            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;
             }
         }
@@ -341,10 +353,10 @@ sub setupboot_grub2 ($$$) {
 
         logm("boot check: grub2, found $entry->{Title}");
 
-	die unless $entry->{$kernkey};
-	if (defined $xenhopt) {
-	    die unless $entry->{Hv};
-	}
+    	die unless $entry->{$kernkey};
+    	if (defined $xenhopt) {
+    	    die unless $entry->{Hv};
+    	}
 
         return $entry;
     };
-- 
1.8.3.1

[PATCH OSSTEST 02/12] Increase boot timer to accomodate to nest test

[Robert Ho]

In nested test case, guest boot will take more time.
 Increase the timer to 200 seconds.

---
 
 Osstest/TestSupport.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index c4da8ce..f3c515e 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -953,7 +953,7 @@ sub compress_stashed($) {
 sub host_reboot ($) {
     my ($ho) = @_;
     target_reboot($ho);
-    poll_loop(40,2, 'reboot-confirm-booted', sub {
+    poll_loop(200,2, 'reboot-confirm-booted', sub {
         my $output;
         if (!eval {
             $output= target_cmd_output($ho, <<END, 40);
-- 
1.8.3.1

[PATCH OSSTEST 03/12] Designate vif device model to e1000

[Robert Ho]

Designate vif model to 'e1000', otherwise, with default
 device model, the L1 eth0 interface disappear, hence xenbridge cannot work.
 Maybe this limitation can be removed later after some fix it. For now, we
 have to accomodate to it.

---

 Osstest/TestSupport.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index f3c515e..8f8638b 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -1483,7 +1483,7 @@ sub prepareguest_part_xencfg ($$$$$) {
     my $xencfg= <<END;
 name        = '$gho->{Name}'
 memory = ${ram_mb}
-vif         = [ 'type=ioemu,mac=$gho->{Ether}' ]
+vif         = [ 'type=ioemu,model=e1000,mac=$gho->{Ether}' ]
 #
 on_poweroff = '$onpoweroff'
 on_reboot   = '$onreboot'
-- 
1.8.3.1

[PATCH OSSTEST 04/12] Just some indentation adustments.

[Robert Ho]

---
 Osstest/TestSupport.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index 8f8638b..1053409 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -53,7 +53,7 @@ BEGIN {
                       target_getfile target_getfile_root
                       target_putfile target_putfile_root
                       target_putfilecontents_stash
-		      target_putfilecontents_root_stash
+                      target_putfilecontents_root_stash
                       target_put_guest_image target_editfile
                       target_editfile_root target_file_exists
                       target_run_apt
@@ -94,7 +94,7 @@ BEGIN {
                       guest_find_domid guest_check_up guest_check_up_quick
                       guest_get_state guest_await_reboot guest_destroy
                       guest_vncsnapshot_begin guest_vncsnapshot_stash
-		      guest_check_remus_ok guest_editconfig
+                      guest_check_remus_ok guest_editconfig
                       host_involves_pcipassthrough host_get_pcipassthrough_devs
                       toolstack guest_create
 
-- 
1.8.3.1

[PATCH OSSTEST 05/12] Add and expose some testsupport APIs

[Robert Ho]

When install L2 guest, we will need to invoke
 'select_ether' to get guest MAC address. So here expose select_ether(). And
 also, we added another function 'guest_editconfig_cd' and expose it. This
 function bascically changes guest boot device sequence and alter its
 on_reboot behavior to restart.
---
 Osstest/TestSupport.pm | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index 1053409..8975652 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -67,7 +67,7 @@ BEGIN {
                       selecthost get_hostflags get_host_property
                       get_host_native_linux_console
                       power_state power_cycle power_cycle_time
-                      serial_fetch_logs
+                      serial_fetch_logs select_ether
                       propname_massage
          
                       get_stashed open_unique_stashfile compress_stashed
@@ -109,6 +109,7 @@ BEGIN {
                       iso_gen_flags_basic
                       iso_copy_content_from_image
                       guest_editconfig_nocd
+                      guest_editconfig_cd
                       );
     %EXPORT_TAGS = ( );
 
@@ -2081,4 +2082,14 @@ sub guest_editconfig_nocd ($$) {
     });
 }
 
+sub guest_editconfig_cd ($) {
+    my ($gho) = @_;
+    guest_editconfig($gho->{Host}, $gho, sub {
+        if (m/^\s*boot\s*= '\s*d\s*c\s*'/) {
+            s/dc/cd/;
+        }
+        s/^on_reboot.*/on_reboot='restart'/;
+    });
+}
+
 1;
-- 
1.8.3.1

[PATCH OSSTEST 06/12] Manipulate $ho IP assignment for nest L2 situation

[Robert Ho]

 In L2 installation context, its host (L1) IP address is
 not queried from DNS, but from previous step of L1 installation, in which, L1
 IP is stored in run var.

---
 Osstest/TestSupport.pm | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index 8975652..c23bbc7 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -835,8 +835,11 @@ sub selecthost ($) {
     dhcp_watch_setup($ho,$ho);
     power_cycle_host_setup($ho);
     serial_host_setup($ho);
-
-    $ho->{IpStatic} = get_host_property($ho,'ip-addr');
+    if ($name eq 'nested') {
+        $ho->{IpStatic} = $r{'L1_IP'};
+    } else {
+        $ho->{IpStatic} = get_host_property($ho,'ip-addr');
+    }
     if (!defined $ho->{IpStatic}) {
 	my $ip_packed= gethostbyname($ho->{Fqdn});
 	die "$ho->{Fqdn} ?" unless $ip_packed;
-- 
1.8.3.1

[PATCH OSSTEST 07/12] For hvm guest configuration, config console to 'hvc0'

[Robert Ho]

---
 Osstest/TestSupport.pm | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index c23bbc7..864805e 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -1753,7 +1753,11 @@ sub target_kernkind_check ($) {
     if ($kernkind eq 'pvops') {
         store_runvar($pfx."rootdev", 'xvda') if $isguest;
         store_runvar($pfx."console", 'hvc0');
-    } elsif ($kernkind !~ m/2618/) {
+    }
+    elsif ($kernkind eq 'hvm'){
+        store_runvar($pfx."console", 'hvc0');       #nested hvm guest shall not append console=xvc0; I guess this applies to all hvm guests.
+    }
+    elsif ($kernkind !~ m/2618/) {
         store_runvar($pfx."console", 'xvc0') if $isguest;
     }
 }
-- 
1.8.3.1

[PATCH OSSTEST 08/12] Add test job for nest test case

[Robert Ho]

This patch adds creation of the nested test job; when
 job creation procedure is invoked.

---
 
 make-flight | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/make-flight b/make-flight
index a91f256..4f6ce91 100755
--- a/make-flight
+++ b/make-flight
@@ -197,6 +197,25 @@ do_hvm_win7_x64_tests () {
             all_hostflags=$most_hostflags,hvm
 }
 
+do_hvm_debian_nested_tests () {
+  if [ $xenarch != amd64 ]; then
+    return
+  fi
+  if [ $dom0arch != amd64 ]; then
+    return
+  fi
+
+  job_create_test test-$xenarch$kern-$dom0arch-nested test-nested xl \
+			$xenarch $dom0arch \
+            nested_image=$NESTED_OS_IMAGE \
+            nested2_image=$NESTED_OS_IMAGE \
+            bios=seabios \
+            kernbuildjob=build-amd64-hvm \
+            kernkind=hvm \
+            device_model_version=qemu-xen \
+            all_hostflags=$most_hostflags,hvm
+}
+
 do_hvm_debian_test_one () {
   testname=$1
   bios=$2
@@ -387,6 +406,7 @@ test_matrix_do_one () {
     done
 
   fi
+  do_hvm_debian_nested_tests
   do_passthrough_tests
 }
 
-- 
1.8.3.1

[PATCH OSSTEST 09/12] Add build hvm job for nested test use

[Robert Ho]

 Add build-debain-hvm build job. The $TREE_LINUX and
 $REVISION_LINU can be designaged in standalone.config.

---
 mfi-common | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/mfi-common b/mfi-common
index 27d6b31..0d43959 100644
--- a/mfi-common
+++ b/mfi-common
@@ -166,6 +166,14 @@ create_build_jobs () {
                 revision_qemu=$REVISION_QEMU                                 \
                 revision_qemuu=$REVISION_QEMU_UPSTREAM
     fi
+    ./cs-job-create $flight build-$arch-hvm build-kern                       \
+                arch=$arch kconfighow=xen-enable-xen-config                  \
+                $RUNVARS $BUILD_RUNVARS $BUILD_LINUX_RUNVARS $arch_runvars   \
+                $suite_runvars                                               \
+                host_hostflags=$build_hostflags                              \
+                revision_linux=$REVISION_LINUX								 \
+                tree_linux=$TREE_LINUX                                       \
+                ${TREEVCS_LINUX:+treevcs_linux=}${TREEVCS_LINUX}
 
     ./cs-job-create $flight build-$arch-pvops build-kern                     \
                 arch=$arch kconfighow=xen-enable-xen-config                  \
-- 
1.8.3.1

[PATCH OSSTEST 10/12] Compose the main body of test-nested test job.

[Robert Ho]

 Compose the main body of test-nested test job. 
1. invoke ts-debian-hvm-install with host of 'host', gn of 'nested', 
 nested of 'nested_L1' 
2. then install xen (ts-xen-install) with host of 'host', gn of
 'nested', nested_build defined 
3. then again invoke ts-debian-hvm-install to
 install L2, with a different gn and nested param from L1 
4. finally destory L2 guest

---
 sg-run-job | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/sg-run-job b/sg-run-job
index cdd065f..6021971 100755
--- a/sg-run-job
+++ b/sg-run-job
@@ -288,6 +288,14 @@ 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 + nested + nested_L1
+    run-ts . = ts-xen-install + host + nested + nested_build
+    run-ts . = ts-debian-hvm-install + host + nested2 + nested_L2
+    run-ts . = ts-guest-destroy + host + nested
+}
+
 proc test-guest-migr {g} {
     if {[catch { run-ts . = ts-migrate-support-check + host $g }]} return
 
-- 
1.8.3.1

[PATCH OSSTEST 11/12] Changes on test step of debain hvm guest install

[Robert Ho]

 This patch is to make ts-debian-hvm-install accomodate
 to nested L1 and L2 guest installation context. 1. Add an input param
 'nested' to indicate which installation    context is. and manipulating 'gn'
 accordingly. 2. increase disk size to accomodate to nested test requirment.
 3. increase root partition size in preseed generation. 4. in L2 installation
 context, need to explicitly get its MAC    address from run var. 5. in hvm
 guest configuration file, add '#nestedhvm=1', which will    later be
 uncommented by guest_editconfig_cd() after xen installed    in L1, and about
 to boot into a nested xen environment. 6. in L1 installation context, assign
 more memory to it; since it    acts as a nested hypervisor anyway. 7.
 allocate a vg (vollum group) from L0 disk space, and attach it    it to L1,
 used dedicatedly for L2 disk space. 8. In L1 installation context, need to
 store some run vars ( its IP,    name, ident), so that in later L2
 installation context, can fetch    these information. 9. In L2 installation,
 after it's created, installed and reboot    successfully, we then power it
 off.

---
 ts-debian-hvm-install | 63 +++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 53 insertions(+), 10 deletions(-)

diff --git a/ts-debian-hvm-install b/ts-debian-hvm-install
index 37eade2..e905698 100755
--- a/ts-debian-hvm-install
+++ b/ts-debian-hvm-install
@@ -28,22 +28,30 @@ if (@ARGV && $ARGV[0] =~ m/^--stage(\d+)$/) { $stage=$1; shift @ARGV; }
 
 defined($r{bios}) or die "Need to define which bios to use";
 
-our ($whhost,$gn) = @ARGV;
+our ($whhost,$gn,$nested) = @ARGV;
 $whhost ||= 'host';
-$gn ||= 'debianhvm';
-
+$nested ||= '';
+our $guesthost;
+our $L1_IP;
+if ($nested eq 'nested_L1') {
+    $gn ||= 'nested';
+    $guesthost ||= "$gn.l1.osstest";
+} elsif ($nested eq 'nested_L2') {
+    $whhost = 'L1_host';
+    $gn ||= 'nested2';
+    $guesthost ||= "$gn.l2.osstest";
+} else {
+    $gn ||= 'debianhvm';
+    $guesthost= "$gn.guest.osstest";
+}
 our $ho= selecthost($whhost);
-
+our $disk_mb= 15000;
 # guest memory size will be set based on host free memory, see below
 our $ram_mb;
-our $disk_mb= 10000;
-
-our $guesthost= "$gn.guest.osstest";
 our $gho;
 
 our $toolstack= toolstack()->{Command};
 
-
 sub preseed () {
 
     my $preseed_file = preseed_base('wheezy','',());
@@ -63,7 +71,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{ / } \\
@@ -135,6 +143,7 @@ sub prep () {
     $gho= prepareguest($ho, $gn, $guesthost, 22,
                        $disk_mb + 1,
                        200);
+    $gho->{"${gn}_ether"} = select_ether($ho,"${gn}_ether") if $nested eq 'nested_L2';
     my $base = "/root/$flight.$job.$gn-";
     my $newiso= $base . "newiso";
     my $emptydir= $base . "empty-dir";
@@ -155,6 +164,7 @@ sub prep () {
     more_prepareguest_hvm($ho,$gho, $ram_mb, $disk_mb,
                           OnReboot => 'preserve',
                           Bios => $r{bios},
+                          ExtraConfig => '#nestedhvm=1',
                           PostImageHook => sub {
         my $cmds = iso_copy_content_from_image($gho, $newiso);
         $cmds .= prepare_initrd($initrddir,$newiso,$preseed_file_path);
@@ -176,11 +186,28 @@ my $ram_minslop = 100;
 my $ram_lots = 5000;
 if ($host_freemem_mb > $ram_lots * 2 + $ram_minslop) {
     $ram_mb = $ram_lots;
+} elsif ($nested eq 'nested_L1') {
+    $ram_mb = 3072;
 } else {
     $ram_mb = 768;
 }
 logm("Host has $host_freemem_mb MB free memory, setting guest memory size to $ram_mb MB");
 
+if ($nested eq 'nested_L2') {
+    my $L2_disk_mb = 20000;
+    my $L0= selecthost($r{'L0_Ident'});
+    my $vg = $L0->{Name};
+    $vg .= ".$c{TestHostDomain}" if ($L0->{Suite} =~ m/lenny/);
+    my $L1_domname = $r{"$ho->{Name}_domname"};
+    target_cmd_root($L0, "lvremove -f /dev/${vg}/${gn} ||:");
+    target_cmd_root($L0, "lvcreate -L ${L2_disk_mb}M -n ${gn} ${vg}");
+    target_cmd_root($L0, "dd if=/dev/zero of=/dev/${vg}/${gn} count=10");
+    target_cmd_root($L0, "xl block-attach $L1_domname /dev/${vg}/${gn},raw,sdb,rw");
+    target_install_packages_norec($ho, qw(lvm2 rsync genisoimage));
+    target_reboot($ho);
+    target_cmd_root($ho, "pvcreate /dev/sdb && vgcreate ${gn}_vg /dev/sdb");
+}
+
 if (!$stage) {
     prep();
     guest_create($gho,$toolstack);
@@ -192,7 +219,23 @@ if ($stage<2) {
     guest_destroy($ho,$gho);
 }
 
-guest_editconfig_nocd($gho,$emptyiso);
+if ($nested) {
+    guest_editconfig_cd($gho);
+} else {
+    guest_editconfig_nocd($gho,$emptyiso);
+}
 guest_create($gho,$toolstack);
 guest_await_dhcp_tcp($gho,300);
 guest_check_up($gho);
+
+if ($nested eq 'nested_L2') {
+    target_cmd_root($gho, "init 0");
+    target_await_down($gho,60);
+    target_ping_check_down($gho);
+}
+if ($nested eq 'nested_L1') {
+    store_runvar("L1_host", $gn);
+    store_runvar("L1_IP", $gho->{Ip});
+    store_runvar("L0_Ident", $whhost);
+    target_cmd_root($gho, "mkdir -p /home/osstest/.ssh && cp /root/.ssh/authorized_keys /home/osstest/.ssh/");
+}
-- 
1.8.3.1

[PATCH OSSTEST 12/12] Changes to test step of xen install

[Robert Ho]

 This patch accomodates ts-xen-install to nested L1 xen
 installation usage. Its change is relatively simpler than
 ts-debain-hvm-install. We simply alter '$ho' usage to 'w_ho', which is
 assigned to '$ho' in original L0 installation context, while assigned to
 '$gho' in L1 Xen installation context. Other subroutines keeps unchanged. The
 main procedure of L1 context installation is similar to native L0
 installations: 1. install necessary packages 2. extract built xen dist and
 kernel dist tarballs 3. But we skip forbidden() for L1 xen installation 4.
 then adjustconfig() 5. setup init daemons 6. then uncomment 'nestedhvm' in L1
 configuration and sync to disk 7. ready to reboot L1 normal guest to get into
 a nested Xen environment 8. wait until the L1 Xen boot up.

---
 ts-xen-install | 126 ++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 79 insertions(+), 47 deletions(-)

diff --git a/ts-xen-install b/ts-xen-install
index 4d34d1f..6248157 100755
--- a/ts-xen-install
+++ b/ts-xen-install
@@ -28,39 +28,45 @@ use Osstest::CXFabric;
 my $checkmode= 0;
 
 tsreadconfig();
-
+our $w_ho;
 our @hos;
-
-if (@ARGV and $ARGV[0] eq '--check') {
-    $checkmode= 1;
-    shift @ARGV;
-    logm("checking builds are done...");
+our ($whhost,$gn,$nested_build) = @ARGV;
+$nested_build ||= '';
+if ($nested_build eq 'nested_build') {
+    $whhost ||= 'host';
+    $gn ||= 'nested';
 } else {
-    if (!@ARGV) {
-	push @ARGV, 'host';
-    }
-    foreach my $k (@ARGV) {
-        push @hos, selecthost($k);
+    if (@ARGV and $ARGV[0] eq '--check') {
+        $checkmode= 1;
+        shift @ARGV;
+        logm("checking builds are done...");
+    } else {
+        if (!@ARGV) {
+            push @ARGV, 'host';
+        }
+        foreach my $k (@ARGV) {
+            push @hos, selecthost($k);
+        }
     }
 }
-
 our $ho;
-
+our $gho;
+our $toolstack= toolstack()->{Command};
 my %distpath;
 
 sub packages () {
-    target_install_packages($ho,
+    target_install_packages($w_ho,
                             qw(bridge-utils vncsnapshot libaio1 libpixman-1-0
                                libsdl1.2debian libglib2.0-0 liblzma5));
-    target_install_packages($ho,
+    target_install_packages($w_ho,
 			    $ho->{Suite} =~ /squeeze/ ? "libyajl1" : "libyajl2");
     if ($ho->{Suite} !~ m/lenny|squeeze/) {
-        target_install_packages($ho, 'libfdt1');
+        target_install_packages($w_ho, 'libfdt1');
     }
     if ($r{arch} eq 'i386') {
-	target_install_packages($ho, 'libc6-xen');
+	target_install_packages($w_ho, 'libc6-xen');
     }
-    target_install_packages($ho, @{toolstack()->{ExtraPackages}})
+    target_install_packages($w_ho, @{toolstack()->{ExtraPackages}})
         if toolstack()->{ExtraPackages};
 }
 
@@ -69,14 +75,14 @@ sub extract () {
     push @parts, 'libvirt' if $r{toolstack} eq "libvirt";
 
     foreach my $part (@parts) {
-        target_extract_jobdistpath($ho, $part, "path_${part}dist",
+        target_extract_jobdistpath($w_ho, $part, "path_${part}dist",
 				   $r{"${part}buildjob"}, \%distpath);
     }
-    target_cmd_root($ho, '/sbin/ldconfig');
+    target_cmd_root($w_ho, '/sbin/ldconfig');
 }
 
 sub adjustconfig () {
-    target_editfile_root($ho, "/etc/xen/xend-config.sxp",
+    target_editfile_root($w_ho, "/etc/xen/xend-config.sxp",
 			 "xend-config.sxp", sub {
 	my (@domains) = (qw(localhost localhost.localdomain),
 			 ".".$c{DnsDomain}, ".".$c{TestHostDomain});
@@ -108,13 +114,13 @@ sub adjustconfig () {
                         /etc/sysconfig/xencommons
                         /etc/default/xend
                         /etc/sysconfig/xend)) {
-        next unless target_file_exists($ho, $try);
+        next unless target_file_exists($w_ho, $try);
         $trace_config_file= $try;
         last;
     }
     die unless defined $trace_config_file;
 
-    target_editfile_root($ho, $trace_config_file, sub {
+    target_editfile_root($w_ho, $trace_config_file, sub {
         my $prnow;
         $prnow= sub {
             print EO "XENCONSOLED_TRACE=guest\n" or die $!;
@@ -128,7 +134,7 @@ sub adjustconfig () {
         $prnow->();
     });
 
-    target_cmd_root($ho, 'mkdir -p /var/log/xen/console');
+    target_cmd_root($w_ho, 'mkdir -p /var/log/xen/console');
 
     setup_cxfabric($ho);
 }
@@ -136,13 +142,13 @@ sub adjustconfig () {
 sub setupboot () {
     my $xenhopt= "conswitch=x watchdog";
 
-    my $cons= get_host_property($ho, 'XenSerialConsole', 'com1');
+    my $cons= get_host_property($w_ho, 'XenSerialConsole', 'com1');
 
     if ( $cons eq "com1" ) {
 	$xenhopt .= " com1=$c{Baud},8n1 console=com1,vga gdb=com1";
     } elsif ( $cons eq "dtuart" ) {
 	$xenhopt .= " console=dtuart";
-	my $dtuart= get_host_property($ho, 'XenDTUARTPath', undef);
+	my $dtuart= get_host_property($w_ho, 'XenDTUARTPath', undef);
 	$xenhopt .= " dtuart=$dtuart" if $dtuart;
     } else {
 	logm("No Xen console device defined for host");
@@ -152,37 +158,38 @@ sub setupboot () {
     }
     my $append= $r{xen_boot_append};
     $xenhopt .= " $append" if defined $append;
-    $append = get_host_property($ho, 'xen-commandline-append', undef);
+    $append = get_host_property($w_ho, 'xen-commandline-append', undef);
     $xenhopt .= " $append" if defined $append;
 
     my @hooks;
 
-    if (host_involves_pcipassthrough($ho)) {
+    if (host_involves_pcipassthrough($w_ho)) {
         push @hooks, {
             EditBootOptions => sub {
-                my ($ho,$hopt,$kopt) = @_;
+                my ($w_ho,$hopt,$kopt) = @_;
                 $$hopt .= ' iommu=on';
                 my $hide= ' xen-pciback.hide='. join '',map { "($_->{Bdf})" }
-                    host_get_pcipassthrough_devs($ho);
+                host_get_pcipassthrough_devs($w_ho);
                 logm("pci passthrough: hiding in dom0: $hide");
                 $$kopt .= $hide;
-            }
-        };
-    }
+                }
+            };
+        }
 
     my $want_kernver = get_runvar('kernel_ver',$r{'kernbuildjob'});
-    debian_boot_setup($ho, $want_kernver, $xenhopt, \%distpath, \@hooks);
+    debian_boot_setup($w_ho, $want_kernver, $xenhopt, \%distpath, \@hooks);
 
     logm("ready to boot Xen");
 }
 
+
 our $initscripts_nobridge;
 
 sub setupinitd () {
     my $ts= toolstack();
     my $xencommons= '/etc/init.d/xencommons';
     my $have_xencommons=
-        !!target_cmd_output_root($ho, <<END);
+        !!target_cmd_output_root($w_ho, <<END);
  if test -f $xencommons && ! grep 'FOR USE WITH LIBXL' $xencommons >/dev/null
  then
    echo y
@@ -211,13 +218,13 @@ END
         $updatercd->($initd,93) if defined $initd;
         $updatercd->('xenbridge',38) if $ts->{OldSeparateBridgeInitd};
     }
-    target_cmd_root($ho, $cmd);
+    target_cmd_root($w_ho, $cmd);
 }
 
 sub nodhcp () {
-    target_editfile_root($ho, "/etc/network/interfaces",
+    target_editfile_root($w_ho, "/etc/network/interfaces",
                          "etc-network-interfaces", sub {
-        my $physif= get_host_property($ho,'interface force',undef);
+        my $physif= get_host_property($w_ho,'interface force',undef);
 
 	if (!defined $physif) {
 	    # preread /etc/network/interfaces to figure out the interface
@@ -267,7 +274,7 @@ END
 	    $bridgex= '';
         }
 
-	my $routes= target_cmd_output_root($ho, "route -n");
+	my $routes= target_cmd_output_root($w_ho, "route -n");
 
 	$routes =~ m/^ [0-9.]+ \s+ 0\.0\.0\.0 \s+ ([0-9.]+) \s+ \S*U\S* \s /mxi
 	    or die "no own local network in route ?  $routes ";
@@ -289,7 +296,7 @@ END
                 $suppress= 1;
                 print EO <<END;
 iface $iface inet static
-    address $ho->{Ip}
+    address $w_ho->{Ip}
     netmask $netmask
     gateway $gateway
 $bridgex
@@ -322,17 +329,42 @@ sub forbidden () {
 END
 }
 
-if ($checkmode) {
-    extract();
-} else {
-    die if @hos > 1;
-    $ho= $hos[0];
-    
+if ($nested_build eq 'nested_build') {
+    $ho= selecthost($whhost);
+    $gho= selectguest($gn,$ho);
+    $w_ho = $gho;
+    store_runvar("$gho->{Guest}_kernkind",$r{'kernkind'});
+    $gho->{Suite}=$ho->{Suite};
+    guest_check_ip($gho);
     packages();
     extract();
-    forbidden();
     adjustconfig();
     setupboot();
     setupinitd();
+    guest_editconfig($gho->{Host}, $gho, sub {
+        s/#nestedhvm/nestedhvm/;
+        });
+    target_cmd_root($gho,"sync");
     nodhcp();
+    logm("Ready to boot L1 Xen");
+    target_cmd_root($gho,"init 0");
+    target_await_down($gho,60);
+    guest_create($gho,$toolstack);
+    guest_await($gho, target_var($gho,'boot_timeout'));
+    guest_check_up($gho);
+} else {
+    if ($checkmode) {
+        extract();
+    } else {
+        die if @hos > 1;
+        $ho= $hos[0];
+        $w_ho = $ho;
+        packages();
+        extract();
+        forbidden();
+        adjustconfig();
+        setupboot();
+        setupinitd();
+        nodhcp();
+    }
 }
-- 
1.8.3.1

Re: [PATCH OSSTEST 01/12] Add support of parsing grub which has 'submenu' primitive

[Ian Jackson]

Robert Ho writes ("[PATCH OSSTEST 01/12] Add support of parsing grub which has 'submenu' primitive"):
>  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. Also, this patch adjust
>  some indent alignments.

Thanks for this submission.  Dealing with submenus is definitely
something we want to do.

I haven't looked at the code in detail yet but I have a general
question: we currently count menu entries and eventually write
GRUB_DEFAULT=<some number>  into /etc/default/grub.

Does this work properly if the entry is in a submenu ?  I guess you
have probably tested this but I thought I should ask...

Can you please not adjust the whitespace ?  osstest in general doesn't
have a requirement for any particular whitespace use, and certainly if
there are to be any whitespace changes they ought to be in a separate
patch.

Thanks,
Ian.

Re: [PATCH OSSTEST 02/12] Increase boot timer to accomodate to nest test

[Ian Jackson]

Robert Ho writes ("[PATCH OSSTEST 02/12] Increase boot timer to accomodate to nest test"):
> In nested test case, guest boot will take more time.
>  Increase the timer to 200 seconds.

Can we make this conditional somehow ?

I think it should probably be picked up from a runvar.

We don't currently have timeouts directly in runvars and we probably
don't want them in make-flight.  So perhaps we should have a runvar
named after the host indicating that it is a nested virt host.

Obviously that would mean this patch would have to come later in the
series.  I'll have to look at the rest of the series to have a clearer
idea what the right thing looks like.

Thanks,
Ian.

Re: [PATCH OSSTEST 03/12] Designate vif device model to e1000

[Ian Jackson]

Robert Ho writes ("[PATCH OSSTEST 03/12] Designate vif device model to e1000"):
> Designate vif model to 'e1000', otherwise, with default
>  device model, the L1 eth0 interface disappear, hence xenbridge cannot work.
>  Maybe this limitation can be removed later after some fix it. For now, we
>  have to accomodate to it.

I don't understand this, I'm afraid.  Can you please explain the bug
in more detail in the commit message ?

It is definitely not acceptable to change the default network card for
all guests in prepareguest_part_xencfg.  It would be OK to provide a
guest-specific runvar to specify the guest network card, and it might
be OK to set that in the nested-specific test job creation.

Thanks,
Ian.

Re: [PATCH OSSTEST 04/12] Just some indentation adustments.

[Ian Jackson]

Robert Ho writes ("[PATCH OSSTEST 04/12] Just some indentation adustments."):
> -		      target_putfilecontents_root_stash
> +                      target_putfilecontents_root_stash

This seems to be just tab/space changes.  I don't think we really need
to bother about these.  Do you find the discrepancies very annoying
for some reason ?

Thanks,
Ian.

Re: [PATCH OSSTEST 05/12] Add and expose some testsupport APIs

[Ian Jackson]

Robert Ho writes ("[PATCH OSSTEST 05/12] Add and expose some testsupport APIs"):
> When install L2 guest, we will need to invoke
>  'select_ether' to get guest MAC address. So here expose select_ether().

I'm not sure whether you actually need to do this.  I will look at the
rest of your series to see why prepareguest() isn't suitable.  But
this part of the patch is fine in principle.

> And

These seem like two indepenedent patches.  They should be split up.

>  also, we added another function 'guest_editconfig_cd' and expose it.
>   This function bascically changes guest boot device sequence and
>  alter its on_reboot behavior to restart.

I don't understand why guest_editconfig_nocd isn't sufficient.

Ian.

Re: [PATCH OSSTEST 06/12] Manipulate $ho IP assignment for nest L2 situation

[Ian Jackson]

Robert Ho writes ("[PATCH OSSTEST 06/12] Manipulate $ho IP assignment for nest L2 situation"):
>  In L2 installation context, its host (L1) IP address is not queried
> from DNS, but from previous step of L1 installation, in which, L1 IP
> is stored in run var.

> -    $ho->{IpStatic} = get_host_property($ho,'ip-addr');
> +    if ($name eq 'nested') {

This is definitely the wrong test.

It would be easier to read this series if you introduced the framework
first, and then applied all the specific differences afterwards.

Instead of keying off $name I think you probably need to make a
variant of selecthost that takes an existing guest ($gho) and converts
it into a useable host ($ho).

It would probably be necessary to split out the bulk of the existing
selecthost into a core function.

I think you also want a general way to specify how the L1's host
properties are set.

Ian.

Re: [PATCH OSSTEST 08/12] Add test job for nest test case

[Ian Jackson]

Robert Ho writes ("[PATCH OSSTEST 08/12] Add test job for nest test case"):
> This patch adds creation of the nested test job; when
>  job creation procedure is invoked.
...
> +  job_create_test test-$xenarch$kern-$dom0arch-nested test-nested xl \
> +			$xenarch $dom0arch \

Have I missed the patch where the recipe is defined ?

> +            nested_image=$NESTED_OS_IMAGE \
> +            nested2_image=$NESTED_OS_IMAGE \

I don't seem to have seen definities for these either.

Ian.

Re: [PATCH OSSTEST 07/12] For hvm guest configuration, config console to 'hvc0'

[Ian Jackson]

Robert Ho writes ("[PATCH OSSTEST 07/12] For hvm guest configuration, config console to 'hvc0'"):
> ---
>  Osstest/TestSupport.pm | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
> index c23bbc7..864805e 100644
> --- a/Osstest/TestSupport.pm
> +++ b/Osstest/TestSupport.pm
> @@ -1753,7 +1753,11 @@ sub target_kernkind_check ($) {
>      if ($kernkind eq 'pvops') {
>          store_runvar($pfx."rootdev", 'xvda') if $isguest;
>          store_runvar($pfx."console", 'hvc0');
> -    } elsif ($kernkind !~ m/2618/) {
> +    }
> +    elsif ($kernkind eq 'hvm'){
> +        store_runvar($pfx."console", 'hvc0');       #nested hvm guest shall not append console=xvc0; I guess this applies to all hvm guests.
> +    }
> +    elsif ($kernkind !~ m/2618/) {

I don't understand why this is necessary.  Surely all the kernels here
are pvops so the kernkind should be 'pvops' in all cases and the
console will be set to hvc0 anyway ?

Ian.

Re: [PATCH OSSTEST 09/12] Add build hvm job for nested test use

[Ian Jackson]

Robert Ho writes ("[PATCH OSSTEST 09/12] Add build hvm job for nested test use"):
>  Add build-debain-hvm build job. The $TREE_LINUX and
>  $REVISION_LINU can be designaged in standalone.config.

What is this for ?  It seems very similar to the build-$arch-pvops
job.

Ian.

Re: [PATCH OSSTEST 10/12] Compose the main body of test-nested test job.

[Ian Jackson]

Robert Ho writes ("[PATCH OSSTEST 10/12] Compose the main body of test-nested test job."):
>  Compose the main body of test-nested test job. 

Ah, this is what I was missing earlier.  You really need to order this
so that things come after things which depend on them.

Typically:
 * cleanups
 * define new TestSupport facilities
 * define new ts-* scripts if any
 * define new recipies
 * updates to make-flight to define new jobs.

> +proc need-hosts/test-nested {} {return host}
> +proc run-job/test-nested {} {
> +    run-ts . = ts-debian-hvm-install + host + nested + nested_L1

ts-debian-hvm-install takes only two arguments.  You are passing 3.
I guess this is in further patches...

Ian.

Re: [PATCH OSSTEST 12/12] Changes to test step of xen install

[Ian Jackson]

Robert Ho writes ("[PATCH OSSTEST 12/12] Changes to test step of xen install"):
>  This patch accomodates ts-xen-install to nested L1 xen

Ah yes, here is the meat.  I have run out of time today but will reply
tomorrow with some design observations.

Thanks,
Ian.

Re: [PATCH OSSTEST 01/12] Add support of parsing grub which has 'submenu' primitive

[Hu, Robert]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: Wednesday, February 11, 2015 10:44 PM
> To: Hu, Robert
> Cc: xen-devel@lists.xen.org; jfehlig@suse.com; wei.liu2@citrix.com;
> ian.campbell@citrix.com; Pang, LongtaoX
> Subject: Re: [PATCH OSSTEST 01/12] Add support of parsing grub which has
> 'submenu' primitive
> 
> Robert Ho writes ("[PATCH OSSTEST 01/12] Add support of parsing grub which
> has 'submenu' primitive"):
> >  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. Also, this patch adjust
> >  some indent alignments.
> 
> Thanks for this submission.  Dealing with submenus is definitely
> something we want to do.
> 
> I haven't looked at the code in detail yet but I have a general
> question: we currently count menu entries and eventually write
> GRUB_DEFAULT=<some number>  into /etc/default/grub.
> 
> Does this work properly if the entry is in a submenu ?  I guess you
> have probably tested this but I thought I should ask...
> 
Yes, this minor change just get 'parsemenu' subroutine capability of recognizing 'submenu'.
The outer layer logic isn't affected.
Actually, the Xen boot menuentry we choose, is inside a submenu. It works and /etc/default/grub
is assigned properly.
> Can you please not adjust the whitespace ?  osstest in general doesn't
> have a requirement for any particular whitespace use, and certainly if
> there are to be any whitespace changes they ought to be in a separate
> patch.
I adjust those because some one in last version's reply told us that
osstest prefers white space substitution to tab, and traditionally 4
white space of 1 tab. (This align with my previous coding experience as well)
And I indeed find that this hunk of code doesn't looks well in my editor.
Its unalignment increases difficulty of reading.
I would suggest to adjust the this hunk's indentation and use white space
substitution to tab to have best suitability across different editors.
> 
> Thanks,
> Ian.

Re: [PATCH OSSTEST 01/12] Add support of parsing grub which has 'submenu' primitive

[Ian Campbell]

On Wed, 2015-02-11 at 14:44 +0000, Ian Jackson wrote:
> Robert Ho writes ("[PATCH OSSTEST 01/12] Add support of parsing grub which has 'submenu' primitive"):
> >  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. Also, this patch adjust
> >  some indent alignments.
> 
> Thanks for this submission.  Dealing with submenus is definitely
> something we want to do.
> 
> I haven't looked at the code in detail yet but I have a general
> question: we currently count menu entries and eventually write
> GRUB_DEFAULT=<some number>  into /etc/default/grub.

FWIW at some point (possibly coinciding with the addition of submenus,
I'm not sure) grub gained the ability to specify the title of the menu
item you wish to boot as the default, which would simplify things if it
could be used universally (i.e. was supported in Wheezy already).

Re: [PATCH OSSTEST 01/12] Add support of parsing grub which has 'submenu' primitive

[Wei Liu]

On Thu, Feb 12, 2015 at 02:01:59AM +0000, Hu, Robert wrote:
> 
> > -----Original Message-----
> > From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> > Sent: Wednesday, February 11, 2015 10:44 PM
> > To: Hu, Robert
> > Cc: xen-devel@lists.xen.org; jfehlig@suse.com; wei.liu2@citrix.com;
> > ian.campbell@citrix.com; Pang, LongtaoX
> > Subject: Re: [PATCH OSSTEST 01/12] Add support of parsing grub which has
> > 'submenu' primitive
> > 
> > Robert Ho writes ("[PATCH OSSTEST 01/12] Add support of parsing grub which
> > has 'submenu' primitive"):
> > >  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. Also, this patch adjust
> > >  some indent alignments.
> > 
> > Thanks for this submission.  Dealing with submenus is definitely
> > something we want to do.
> > 
> > I haven't looked at the code in detail yet but I have a general
> > question: we currently count menu entries and eventually write
> > GRUB_DEFAULT=<some number>  into /etc/default/grub.
> > 
> > Does this work properly if the entry is in a submenu ?  I guess you
> > have probably tested this but I thought I should ask...
> > 
> Yes, this minor change just get 'parsemenu' subroutine capability of recognizing 'submenu'.
> The outer layer logic isn't affected.
> Actually, the Xen boot menuentry we choose, is inside a submenu. It works and /etc/default/grub
> is assigned properly.

In any case this is a very useful improvement.

Out of interest what Linux are you running?  If you're running Debian
and the overlay 20_linux_xen (inside $OSSTEST/overlay/etc/etc/grub.d) is
copied to your test host, there shouldn't be any submenu entries in your
grub.cfg, I think.

Wei.

> > Can you please not adjust the whitespace ?  osstest in general doesn't
> > have a requirement for any particular whitespace use, and certainly if
> > there are to be any whitespace changes they ought to be in a separate
> > patch.
> I adjust those because some one in last version's reply told us that
> osstest prefers white space substitution to tab, and traditionally 4
> white space of 1 tab. (This align with my previous coding experience as well)
> And I indeed find that this hunk of code doesn't looks well in my editor.
> Its unalignment increases difficulty of reading.
> I would suggest to adjust the this hunk's indentation and use white space
> substitution to tab to have best suitability across different editors.
> > 
> > Thanks,
> > Ian.

Re: [PATCH OSSTEST 11/12] Changes on test step of debain hvm guest install

[Ian Jackson]

Robert Ho writes ("[PATCH OSSTEST 11/12] Changes on test step of debain hvm guest install"):
>  This patch is to make ts-debian-hvm-install accomodate

Ah yes here is the meat.

Firstly, can you please reformat your commit message so that the
individual points are separated out into paragraphs.  But I think
actually that probably some of this wants to go into different commits
(and perhaps different places).

> diff --git a/ts-debian-hvm-install b/ts-debian-hvm-install
> index 37eade2..e905698 100755
> --- a/ts-debian-hvm-install
> +++ b/ts-debian-hvm-install
> @@ -28,22 +28,30 @@ if (@ARGV && $ARGV[0] =~ m/^--stage(\d+)$/) { $stage=$1; shift @ARGV; }
...
> +if ($nested eq 'nested_L1') {
> +    $gn ||= 'nested';
> +    $guesthost ||= "$gn.l1.osstest";
> +} elsif ($nested eq 'nested_L2') {
> +    $whhost = 'L1_host';
> +    $gn ||= 'nested2';
> +    $guesthost ||= "$gn.l2.osstest";
> +} else {
> +    $gn ||= 'debianhvm';
> +    $guesthost= "$gn.guest.osstest";
> +}

I don't think this is the right way to control the nestedness.
Also your test recipe seems wrong.  You write:

+    run-ts . = ts-debian-hvm-install + host + nested + nested_L1
+    run-ts . = ts-xen-install + host + nested + nested_build
+    run-ts . = ts-debian-hvm-install + host + nested2 + nested_L2
+    run-ts . = ts-guest-destroy + host + nested

I think this should look more like:

+    run-ts . = ts-debian-hvm-install + host + nested
+    run-ts . = ts-nested-setup + host + nested
+    run-ts . = ts-xen-install nested
+    run-ts . = ts-host-reboot nested
+    run-ts . = ts-debian-hvm-install nested nested2

ts-nested-setup would turn on nested HVM support in the domain config,
figures out the hostname etc. and makes some appropriate runvars which
selecthost would recognise.

I don't know why you need to use a dedicated VG for your nested
guests's L2 guests - please explain - but if you do, probably
ts-nested-setup could set it up.

> @@ -63,7 +71,7 @@ d-i partman-auto/expert_recipe string \\
>                          use_filesystem{ } filesystem{ vfat } \\
>                          mountpoint{ /boot/efi } \\
>                  . \\
> -                5000 50 5000 ext4 \\
> +                10000 50 10000 ext4 \\

I think this needs an explanation.  You mentioned it in your commit
message but didn't give reasons.  I think this should perhaps be done
in a different way.

> +if ($nested eq 'nested_L2') {
> +    my $L2_disk_mb = 20000;
> +    my $L0= selecthost($r{'L0_Ident'});

As a style matter, runvars and perl local variable generally have
all-lowercase names.

> +if ($nested eq 'nested_L2') {
> +    target_cmd_root($gho, "init 0");
> +    target_await_down($gho,60);
> +    target_ping_check_down($gho);
> +}
> +if ($nested eq 'nested_L1') {
> +    store_runvar("L1_host", $gn);
> +    store_runvar("L1_IP", $gho->{Ip});
> +    store_runvar("L0_Ident", $whhost);
> +    target_cmd_root($gho, "mkdir -p /home/osstest/.ssh && cp /root/.ssh/authorized_keys /home/osstest/.ssh/");
> +}

I don't understand the purpose behind these special cases.

Thanks,
Ian.

Re: [PATCH OSSTEST 12/12] Changes to test step of xen install

[Ian Jackson]

Robert Ho writes ("[PATCH OSSTEST 12/12] Changes to test step of xen install"):
>  This patch accomodates ts-xen-install to nested L1 xen installation
>  usage. Its change is relatively simpler than
>  ts-debain-hvm-install. We simply alter '$ho' usage to 'w_ho', which
>  is assigned to '$ho' in original L0 installation context, while
>  assigned to '$gho' in L1 Xen installation context.

Certainly I think we should use ts-xen-install for installing Xen on
the L1.  But I think that ts-xen-install should probably think almost
entirely about the L1 and $ho should be the L1.

I think if you followed the suggestion for the structure that I made
in my previous patch, very little of the changes here would be
necessary.

It's not clear to me that _anything_ would need to change in
ts-xen-install, in fact.  (Although I may be wrong.)

Thanks,
Ian.

Re: [PATCH OSSTEST 01/12] Add support of parsing grub which has 'submenu' primitive

[Ian Jackson]

Wei Liu writes ("Re: [PATCH OSSTEST 01/12] Add support of parsing grub which has 'submenu' primitive"):
> On Thu, Feb 12, 2015 at 02:01:59AM +0000, Hu, Robert wrote:
> > Yes, this minor change just get 'parsemenu' subroutine capability of recognizing 'submenu'.
> > The outer layer logic isn't affected.
> > Actually, the Xen boot menuentry we choose, is inside a submenu. It works and /etc/default/grub
> > is assigned properly.

Great.

> In any case this is a very useful improvement.

Yes, indeed!

> Out of interest what Linux are you running?  If you're running Debian
> and the overlay 20_linux_xen (inside $OSSTEST/overlay/etc/etc/grub.d) is
> copied to your test host, there shouldn't be any submenu entries in your
> grub.cfg, I think.

I consider that a workaround (and I think so do you).

So I think subject to the (rather daft) argument we are having over
whitespace this is a really useful patch.

> > > Can you please not adjust the whitespace ?  osstest in general doesn't
> > > have a requirement for any particular whitespace use, and certainly if
> > > there are to be any whitespace changes they ought to be in a separate
> > > patch.
> >
> > I adjust those because some one in last version's reply told us that
> > osstest prefers white space substitution to tab,

I'm sorry that we seem to be having a disagreement over this.  That's
not very helpful for you, I realise!

I hope that whoever made those comments would agree that whitespace
cleanups should at least be in a separate patch.  So please when you
resubmit can you split them out ?

I can't seem to find the email you refer to.  Do you happen to be able
to give me a reference ?

> > and traditionally 4 white space of 1 tab. (This align with my
> > previous coding experience as well)

4-character tabs are quite unusual in the Free Software world.  8 is
usual.

> > And I indeed find that this hunk of code doesn't looks well in my editor.
> > Its unalignment increases difficulty of reading.

Since evidently this is annoying to you I won't stand in the way of
your effort to clean this up, even though I don't much care about it.
So if you submit this as a separate patch I won't block it.

But maybe simply configuring your editor to use 8-character tabs will
fix the problem for you ?  That would be less work than preparing
whitespace adjustment patches.

Thanksw,
Ian.

Re: [PATCH OSSTEST 01/12] Add support of parsing grub which has 'submenu' primitive

[Hu, Robert]

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@citrix.com]
> Sent: Thursday, February 12, 2015 6:13 PM
> To: Hu, Robert
> Cc: Ian Jackson; xen-devel@lists.xen.org; jfehlig@suse.com;
> wei.liu2@citrix.com; ian.campbell@citrix.com; Pang, LongtaoX
> Subject: Re: [PATCH OSSTEST 01/12] Add support of parsing grub which has
> 'submenu' primitive
> 
> On Thu, Feb 12, 2015 at 02:01:59AM +0000, Hu, Robert wrote:
> >
> > > -----Original Message-----
> > > From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> > > Sent: Wednesday, February 11, 2015 10:44 PM
> > > To: Hu, Robert
> > > Cc: xen-devel@lists.xen.org; jfehlig@suse.com; wei.liu2@citrix.com;
> > > ian.campbell@citrix.com; Pang, LongtaoX
> > > Subject: Re: [PATCH OSSTEST 01/12] Add support of parsing grub which has
> > > 'submenu' primitive
> > >
> > > Robert Ho writes ("[PATCH OSSTEST 01/12] Add support of parsing grub
> which
> > > has 'submenu' primitive"):
> > > >  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. Also, this patch
> adjust
> > > >  some indent alignments.
> > >
> > > Thanks for this submission.  Dealing with submenus is definitely
> > > something we want to do.
> > >
> > > I haven't looked at the code in detail yet but I have a general
> > > question: we currently count menu entries and eventually write
> > > GRUB_DEFAULT=<some number>  into /etc/default/grub.
> > >
> > > Does this work properly if the entry is in a submenu ?  I guess you
> > > have probably tested this but I thought I should ask...
> > >
> > Yes, this minor change just get 'parsemenu' subroutine capability of
> recognizing 'submenu'.
> > The outer layer logic isn't affected.
> > Actually, the Xen boot menuentry we choose, is inside a submenu. It works
> and /etc/default/grub
> > is assigned properly.
> 
> In any case this is a very useful improvement.
> 
> Out of interest what Linux are you running?  If you're running Debian
> and the overlay 20_linux_xen (inside $OSSTEST/overlay/etc/etc/grub.d) is
> copied to your test host, there shouldn't be any submenu entries in your
> grub.cfg, I think.
> 
> Wei.
We use Debian + linux-stable kernel in the test.
Didn't look into details of the grub generating procedure, but my observation
is that it does have the submenu.
> 
> > > Can you please not adjust the whitespace ?  osstest in general doesn't
> > > have a requirement for any particular whitespace use, and certainly if
> > > there are to be any whitespace changes they ought to be in a separate
> > > patch.
> > I adjust those because some one in last version's reply told us that
> > osstest prefers white space substitution to tab, and traditionally 4
> > white space of 1 tab. (This align with my previous coding experience as well)
> > And I indeed find that this hunk of code doesn't looks well in my editor.
> > Its unalignment increases difficulty of reading.
> > I would suggest to adjust the this hunk's indentation and use white space
> > substitution to tab to have best suitability across different editors.
> > >
> > > Thanks,
> > > Ian.

Re: [PATCH OSSTEST 11/12] Changes on test step of debain hvm guest install

[Hu, Robert]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: Friday, February 13, 2015 2:17 AM
> To: Hu, Robert
> Cc: xen-devel@lists.xen.org; jfehlig@suse.com; wei.liu2@citrix.com;
> ian.campbell@citrix.com; Pang, LongtaoX
> Subject: Re: [PATCH OSSTEST 11/12] Changes on test step of debain hvm guest
> install
> 
> Robert Ho writes ("[PATCH OSSTEST 11/12] Changes on test step of debain hvm
> guest install"):
> >  This patch is to make ts-debian-hvm-install accomodate
> 
> Ah yes here is the meat.
> 
> Firstly, can you please reformat your commit message so that the
> individual points are separated out into paragraphs.  But I think
> actually that probably some of this wants to go into different commits
> (and perhaps different places).
You mean dividing this patch into more pieces/commits?
> 
> > diff --git a/ts-debian-hvm-install b/ts-debian-hvm-install
> > index 37eade2..e905698 100755
> > --- a/ts-debian-hvm-install
> > +++ b/ts-debian-hvm-install
> > @@ -28,22 +28,30 @@ if (@ARGV && $ARGV[0] =~ m/^--stage(\d+)$/)
> { $stage=$1; shift @ARGV; }
> ...
> > +if ($nested eq 'nested_L1') {
> > +    $gn ||= 'nested';
> > +    $guesthost ||= "$gn.l1.osstest";
> > +} elsif ($nested eq 'nested_L2') {
> > +    $whhost = 'L1_host';
> > +    $gn ||= 'nested2';
> > +    $guesthost ||= "$gn.l2.osstest";
> > +} else {
> > +    $gn ||= 'debianhvm';
> > +    $guesthost= "$gn.guest.osstest";
> > +}
> 
> I don't think this is the right way to control the nestedness.
> Also your test recipe seems wrong.  You write:
> 
> +    run-ts . = ts-debian-hvm-install + host + nested + nested_L1
> +    run-ts . = ts-xen-install + host + nested + nested_build
> +    run-ts . = ts-debian-hvm-install + host + nested2 + nested_L2
> +    run-ts . = ts-guest-destroy + host + nested
> 
> I think this should look more like:
> 
> +    run-ts . = ts-debian-hvm-install + host + nested
> +    run-ts . = ts-nested-setup + host + nested
> +    run-ts . = ts-xen-install nested
> +    run-ts . = ts-host-reboot nested
> +    run-ts . = ts-debian-hvm-install nested nested2
> 
OK. Since we could only try to learn your design arch/hierarchy of osstest,
through code reading, such as terms of test job, test step, recipe, etc., 
we inevitably made some misunderstanding or unawareness.
Fortunately getting closer and closer to your mind now.
Will follow your recipe composing above.
> ts-nested-setup would turn on nested HVM support in the domain config,
> figures out the hostname etc. and makes some appropriate runvars which
> selecthost would recognise.
> 
Thanks for this help.
> I don't know why you need to use a dedicated VG for your nested
> guests's L2 guests - please explain - but if you do, probably
> ts-nested-setup could set it up.
The existing ts-debian-hvm-install code presume host has vg set
for guest installation. To make minimal code change, we'd like
to imitate that presumption for L2's host. 
> 
> > @@ -63,7 +71,7 @@ d-i partman-auto/expert_recipe string \\
> >                          use_filesystem{ } filesystem{ vfat } \\
> >                          mountpoint{ /boot/efi } \\
> >                  . \\
> > -                5000 50 5000 ext4 \\
> > +                10000 50 10000 ext4 \\
> 
> I think this needs an explanation.  You mentioned it in your commit
> message but didn't give reasons.  I think this should perhaps be done
> in a different way.
You mean not increase the size uniformly, but conditionally only for
L1?
> 
> > +if ($nested eq 'nested_L2') {
> > +    my $L2_disk_mb = 20000;
> > +    my $L0= selecthost($r{'L0_Ident'});
> 
> As a style matter, runvars and perl local variable generally have
> all-lowercase names.
Sure, will follow the convention.
> 
> > +if ($nested eq 'nested_L2') {
> > +    target_cmd_root($gho, "init 0");
> > +    target_await_down($gho,60);
> > +    target_ping_check_down($gho);
> > +}
> > +if ($nested eq 'nested_L1') {
> > +    store_runvar("L1_host", $gn);
> > +    store_runvar("L1_IP", $gho->{Ip});
> > +    store_runvar("L0_Ident", $whhost);
> > +    target_cmd_root($gho, "mkdir -p /home/osstest/.ssh && cp
> /root/.ssh/authorized_keys /home/osstest/.ssh/");
> > +}
> 
> I don't understand the purpose behind these special cases.
The first block is shut down L2 guest after proving it boots up.
The second block is in L1 context, that store run vars to pass down
information to L2.
To follow your recipe, these parts shall be moved to other ts-*.
> 
> Thanks,
> Ian.

Re: [PATCH OSSTEST 12/12] Changes to test step of xen install

[Hu, Robert]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: Friday, February 13, 2015 2:21 AM
> To: Hu, Robert
> Cc: xen-devel@lists.xen.org; ian.jackson@eu.citrix.com; jfehlig@suse.com;
> wei.liu2@citrix.com; ian.campbell@citrix.com; Pang, LongtaoX
> Subject: Re: [PATCH OSSTEST 12/12] Changes to test step of xen install
> 
> Robert Ho writes ("[PATCH OSSTEST 12/12] Changes to test step of xen
> install"):
> >  This patch accomodates ts-xen-install to nested L1 xen installation
> >  usage. Its change is relatively simpler than
> >  ts-debain-hvm-install. We simply alter '$ho' usage to 'w_ho', which
> >  is assigned to '$ho' in original L0 installation context, while
> >  assigned to '$gho' in L1 Xen installation context.
> 
> Certainly I think we should use ts-xen-install for installing Xen on
> the L1.  But I think that ts-xen-install should probably think almost
> entirely about the L1 and $ho should be the L1.
> 
> I think if you followed the suggestion for the structure that I made
> in my previous patch, very little of the changes here would be
> necessary.
> 
> It's not clear to me that _anything_ would need to change in
> ts-xen-install, in fact.  (Although I may be wrong.)
Going to read selecthost() and its relative callers deeper, I think you
are right.
What needs to be change is the selecthost() not xen install.
> 
> Thanks,
> Ian.

Re: [PATCH OSSTEST 01/12] Add support of parsing grub which has 'submenu' primitive

[Hu, Robert]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: Friday, February 13, 2015 2:32 AM
> To: Wei Liu
> Cc: Hu, Robert; xen-devel@lists.xen.org; jfehlig@suse.com;
> ian.campbell@citrix.com; Pang, LongtaoX
> Subject: Re: [PATCH OSSTEST 01/12] Add support of parsing grub which has
> 'submenu' primitive
> 
> Wei Liu writes ("Re: [PATCH OSSTEST 01/12] Add support of parsing grub which
> has 'submenu' primitive"):
> > On Thu, Feb 12, 2015 at 02:01:59AM +0000, Hu, Robert wrote:
> > > Yes, this minor change just get 'parsemenu' subroutine capability of
> recognizing 'submenu'.
> > > The outer layer logic isn't affected.
> > > Actually, the Xen boot menuentry we choose, is inside a submenu. It works
> and /etc/default/grub
> > > is assigned properly.
> 
> Great.
> 
> > In any case this is a very useful improvement.
> 
> Yes, indeed!
> 
> > Out of interest what Linux are you running?  If you're running Debian
> > and the overlay 20_linux_xen (inside $OSSTEST/overlay/etc/etc/grub.d) is
> > copied to your test host, there shouldn't be any submenu entries in your
> > grub.cfg, I think.
> 
> I consider that a workaround (and I think so do you).
> 
> So I think subject to the (rather daft) argument we are having over
> whitespace this is a really useful patch.
> 
> > > > Can you please not adjust the whitespace ?  osstest in general doesn't
> > > > have a requirement for any particular whitespace use, and certainly if
> > > > there are to be any whitespace changes they ought to be in a separate
> > > > patch.
> > >
> > > I adjust those because some one in last version's reply told us that
> > > osstest prefers white space substitution to tab,
> 
> I'm sorry that we seem to be having a disagreement over this.  That's
> not very helpful for you, I realise!
> 
> I hope that whoever made those comments would agree that whitespace
> cleanups should at least be in a separate patch.  So please when you
> resubmit can you split them out ?
Sure, will separate white space change and indentation adjustments out.
> 
> I can't seem to find the email you refer to.  Do you happen to be able
> to give me a reference ?
> 
> > > and traditionally 4 white space of 1 tab. (This align with my
> > > previous coding experience as well)
> 
> 4-character tabs are quite unusual in the Free Software world.  8 is
> usual.
> 
> > > And I indeed find that this hunk of code doesn't looks well in my editor.
> > > Its unalignment increases difficulty of reading.
> 
> Since evidently this is annoying to you I won't stand in the way of
> your effort to clean this up, even though I don't much care about it.
> So if you submit this as a separate patch I won't block it.
Thanks for your understanding.
> 
> But maybe simply configuring your editor to use 8-character tabs will
> fix the problem for you ?  That would be less work than preparing
> whitespace adjustment patches.
OK, will have a try first. :)
> 
> Thanksw,
> Ian.

Re: [PATCH OSSTEST 10/12] Compose the main body of test-nested test job.

[Hu, Robert]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: Thursday, February 12, 2015 1:07 AM
> To: Hu, Robert
> Cc: xen-devel@lists.xen.org; jfehlig@suse.com; wei.liu2@citrix.com;
> ian.campbell@citrix.com; Pang, LongtaoX
> Subject: Re: [PATCH OSSTEST 10/12] Compose the main body of test-nested
> test job.
> 
> Robert Ho writes ("[PATCH OSSTEST 10/12] Compose the main body of
> test-nested test job."):
> >  Compose the main body of test-nested test job.
> 
> Ah, this is what I was missing earlier.  You really need to order this
> so that things come after things which depend on them.
> 
> Typically:
>  * cleanups
>  * define new TestSupport facilities
>  * define new ts-* scripts if any
>  * define new recipies
>  * updates to make-flight to define new jobs.
> 
OK, will follow this order.
> > +proc need-hosts/test-nested {} {return host}
> > +proc run-job/test-nested {} {
> > +    run-ts . = ts-debian-hvm-install + host + nested + nested_L1
> 
> ts-debian-hvm-install takes only two arguments.  You are passing 3.
> I guess this is in further patches...
sure:)
> 
> Ian.

Re: [PATCH OSSTEST 07/12] For hvm guest configuration, config console to 'hvc0'

[Hu, Robert]

Best Regards,
Robert Ho

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: Thursday, February 12, 2015 1:04 AM
> To: Hu, Robert
> Cc: xen-devel@lists.xen.org; jfehlig@suse.com; wei.liu2@citrix.com;
> ian.campbell@citrix.com; Pang, LongtaoX
> Subject: Re: [PATCH OSSTEST 07/12] For hvm guest configuration, config
> console to 'hvc0'
> 
> Robert Ho writes ("[PATCH OSSTEST 07/12] For hvm guest configuration, config
> console to 'hvc0'"):
> > ---
> >  Osstest/TestSupport.pm | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
> > index c23bbc7..864805e 100644
> > --- a/Osstest/TestSupport.pm
> > +++ b/Osstest/TestSupport.pm
> > @@ -1753,7 +1753,11 @@ sub target_kernkind_check ($) {
> >      if ($kernkind eq 'pvops') {
> >          store_runvar($pfx."rootdev", 'xvda') if $isguest;
> >          store_runvar($pfx."console", 'hvc0');
> > -    } elsif ($kernkind !~ m/2618/) {
> > +    }
> > +    elsif ($kernkind eq 'hvm'){
> > +        store_runvar($pfx."console", 'hvc0');       #nested hvm guest
> shall not append console=xvc0; I guess this applies to all hvm guests.
> > +    }
> > +    elsif ($kernkind !~ m/2618/) {
> 
> I don't understand why this is necessary.  Surely all the kernels here
> are pvops so the kernkind should be 'pvops' in all cases and the
> console will be set to hvc0 anyway ?
Oh, this is my mistake. Will revert.
> 
> Ian.

Re: [PATCH OSSTEST 02/12] Increase boot timer to accomodate to nest test

[Hu, Robert]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: Wednesday, February 11, 2015 10:47 PM
> To: Hu, Robert
> Cc: xen-devel@lists.xen.org; jfehlig@suse.com; wei.liu2@citrix.com;
> ian.campbell@citrix.com; Pang, LongtaoX
> Subject: Re: [PATCH OSSTEST 02/12] Increase boot timer to accomodate to nest
> test
> 
> Robert Ho writes ("[PATCH OSSTEST 02/12] Increase boot timer to accomodate
> to nest test"):
> > In nested test case, guest boot will take more time.
> >  Increase the timer to 200 seconds.
> 
> Can we make this conditional somehow ?
> 
> I think it should probably be picked up from a runvar.
> 
> We don't currently have timeouts directly in runvars and we probably
> don't want them in make-flight.  So perhaps we should have a runvar
> named after the host indicating that it is a nested virt host.
So how about reconsider this? I would suggest that just increase the
timer here, since, though it is increased, the timer gets return as long as
guest boots up, no impact to existing test cases.
> 
> Obviously that would mean this patch would have to come later in the
> series.  I'll have to look at the rest of the series to have a clearer
> idea what the right thing looks like.
> 
> Thanks,
> Ian.

Re: [PATCH OSSTEST 05/12] Add and expose some testsupport APIs

[Hu, Robert]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: Wednesday, February 11, 2015 10:54 PM
> To: Hu, Robert
> Cc: xen-devel@lists.xen.org; jfehlig@suse.com; wei.liu2@citrix.com;
> ian.campbell@citrix.com; Pang, LongtaoX
> Subject: Re: [PATCH OSSTEST 05/12] Add and expose some testsupport APIs
> 
> Robert Ho writes ("[PATCH OSSTEST 05/12] Add and expose some testsupport
> APIs"):
> > When install L2 guest, we will need to invoke
> >  'select_ether' to get guest MAC address. So here expose select_ether().
> 
> I'm not sure whether you actually need to do this.  I will look at the
> rest of your series to see why prepareguest() isn't suitable.  But
> this part of the patch is fine in principle.
> 
> > And
> 
> These seem like two indepenedent patches.  They should be split up.
> 
> >  also, we added another function 'guest_editconfig_cd' and expose it.
> >   This function bascically changes guest boot device sequence and
> >  alter its on_reboot behavior to restart.
> 
> I don't understand why guest_editconfig_nocd isn't sufficient.
Em..., seems guest_editconfig_nocd can achieve my purpose as well.
I would try to reuse it in next sending.
I guess, originally I composed another function was to avoid the risk
of breaking down existing functions, given the situation I know little about your
design mind. Now that we changed our mind to
reuse existing functions as much as possible, I should have eliminated
these old-time addings.
> 
> Ian.

Re: [PATCH OSSTEST 03/12] Designate vif device model to e1000

[Hu, Robert]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: Wednesday, February 11, 2015 10:50 PM
> To: Hu, Robert
> Cc: xen-devel@lists.xen.org; jfehlig@suse.com; wei.liu2@citrix.com;
> ian.campbell@citrix.com; Pang, LongtaoX
> Subject: Re: [PATCH OSSTEST 03/12] Designate vif device model to e1000
> 
> Robert Ho writes ("[PATCH OSSTEST 03/12] Designate vif device model to
> e1000"):
> > Designate vif model to 'e1000', otherwise, with default
> >  device model, the L1 eth0 interface disappear, hence xenbridge cannot
> work.
> >  Maybe this limitation can be removed later after some fix it. For now, we
> >  have to accomodate to it.
> 
> I don't understand this, I'm afraid.  Can you please explain the bug
> in more detail in the commit message ?
I didn't look into details. Just observed the phenomenon if not designate it as
e1000, in L1, we will not have eth0 visible, therefore bridge on it won't work.
As long as we designate vif to e1000, it works.
But this is not a problem in our test environment in
which we don't use Debian as nested Dom0. So I guess this is Debian-specific
bug.
> 
> It is definitely not acceptable to change the default network card for
> all guests in prepareguest_part_xencfg.  It would be OK to provide a
> guest-specific runvar to specify the guest network card, and it might
> be OK to set that in the nested-specific test job creation.
OK, will use this approach to walk around and for the consideration of
future test scope scaling.
> 
> Thanks,
> Ian.

Re: [PATCH OSSTEST 06/12] Manipulate $ho IP assignment for nest L2 situation

[Hu, Robert]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: Wednesday, February 11, 2015 10:59 PM
> To: Hu, Robert
> Cc: xen-devel@lists.xen.org; ian.jackson@eu.citrix.com; jfehlig@suse.com;
> wei.liu2@citrix.com; ian.campbell@citrix.com; Pang, LongtaoX
> Subject: Re: [PATCH OSSTEST 06/12] Manipulate $ho IP assignment for nest L2
> situation
> 
> Robert Ho writes ("[PATCH OSSTEST 06/12] Manipulate $ho IP assignment for
> nest L2 situation"):
> >  In L2 installation context, its host (L1) IP address is not queried
> > from DNS, but from previous step of L1 installation, in which, L1 IP
> > is stored in run var.
> 
> > -    $ho->{IpStatic} = get_host_property($ho,'ip-addr');
> > +    if ($name eq 'nested') {
> 
> This is definitely the wrong test.
> 
> It would be easier to read this series if you introduced the framework
> first, and then applied all the specific differences afterwards.
> 
> Instead of keying off $name I think you probably need to make a
> variant of selecthost that takes an existing guest ($gho) and converts
> it into a useable host ($ho).
> 
> It would probably be necessary to split out the bulk of the existing
> selecthost into a core function.
> 
> I think you also want a general way to specify how the L1's host
> properties are set.
Good point! This is indeed my missing point, will fix this.
I didn't get the idea of host property in previous code reading.
Where can I get the idea of what 'host property' is? how it is supposed to
use? Any text I can find introducing it? Or you can give a little
hint on it? thanks.
> 
> Ian.

Re: [PATCH OSSTEST 02/12] Increase boot timer to accomodate to nest test

[Ian Jackson]

Hu, Robert writes ("RE: [PATCH OSSTEST 02/12] Increase boot timer to accomodate to nest test"):
> > Robert Ho writes ("[PATCH OSSTEST 02/12] Increase boot timer to accomodate
> > to nest test"):
> > > In nested test case, guest boot will take more time.
> > >  Increase the timer to 200 seconds.
> > 
> > Can we make this conditional somehow ?
> > 
> > I think it should probably be picked up from a runvar.

Thinking about this some more, there could be a runvar which would
multiply all of the guest-related timeouts.  The runvar name would have
the host ident in it (and a missing runvar would mean 1.0).  Then
ts-nested-setup could set this runvar.

This would also allow us to adjust the timeouts for other
host-specific reasons in the future (for example, if we grow some
hosts which are unreasonably slow or supposedly very fast).

> So how about reconsider this? I would suggest that just increase the
> timer here, since, though it is increased, the timer gets return as long as
> guest boots up, no impact to existing test cases.

But we care about not eliminating existing genuine failures, too.  If
a guest does not start up quickly, then that is a regression.

Thanks,
Ian.

Re: [PATCH OSSTEST 11/12] Changes on test step of debain hvm guest install

[Ian Jackson]

Hu, Robert writes ("RE: [PATCH OSSTEST 11/12] Changes on test step of debain hvm guest install"):
> [Ian]
> > Firstly, can you please reformat your commit message so that the
> > individual points are separated out into paragraphs.  But I think
> > actually that probably some of this wants to go into different commits
> > (and perhaps different places).
>
> You mean dividing this patch into more pieces/commits?

Yes.  At least, that might be a good idea.  I'm finding it a bit
difficult to see the wood for the trees.

I can, however, offer some general guidelines on when to split/merge:

 * Changes must be in the same commit if they are mutually
   interdependent, so that the code only works before all of them or
   after all of them.

 * Each commit should be the smallest piece which is a coherent and
   comprehensible alteration.  (Although small changes might be put
   together in the same commit, especially if they are conceptually
   very similar and/or aren't textually interleaved.)

 * On ordering: it should not normally be necessary to read subsequent
   commits to understand earlier ones.  So generally that means that
   new interfaces should be introduced (with any necessary doc
   comments, etc.) earlier and used later.

 * It is a good idea to split out refactoring operations, and code
   motion, each into their own patch.  That makes it much easier to
   review both the refactoring/motion (because it's much easier to
   look for unintentional functional changes), and the other patches
   (because they don't contain non-functional `noise').  When you do
   this, the refactoring/motion should clearly say what if any
   functional change is introduced.  `No functional change' is the
   usual stock phrase for cases when it's true.

> > I think this should look more like:
> > 
> > +    run-ts . = ts-debian-hvm-install + host + nested
> > +    run-ts . = ts-nested-setup + host + nested
> > +    run-ts . = ts-xen-install nested
> > +    run-ts . = ts-host-reboot nested
> > +    run-ts . = ts-debian-hvm-install nested nested2
> > 
> OK. Since we could only try to learn your design arch/hierarchy of osstest,
> through code reading, such as terms of test job, test step, recipe, etc., 
> we inevitably made some misunderstanding or unawareness.

Indeed.  That's fine.

> Fortunately getting closer and closer to your mind now.

I think so, yes.  Of course if you think there is something in the
current design/architecture which is wrong or broken, then please do
say so.  We're aware it's not perfect.

Likewise, if something I suggest doesn't make sense or doesn't seem to
work well, please do feel free to contradict me.  We're relying on
your judgement as well as mine :-).

> Will follow your recipe composing above.

So, yes, please, if you agree that this is a good way to go.  I think
it will reduce the amount of nested-specific code elsewhere.

> > I don't know why you need to use a dedicated VG for your nested
> > guests's L2 guests - please explain - but if you do, probably
> > ts-nested-setup could set it up.
> 
> The existing ts-debian-hvm-install code presume host has vg set
> for guest installation. To make minimal code change, we'd like
> to imitate that presumption for L2's host. 

Strictly speaking, the existing ts-debian-hvm-install simply expects
that the host has a VG.  The existing ts-host-install code (in
Debian.pm) sets up the VG (via preseed_create).  So I think your
problem is that ts-debian-hvm-install doesn't set up the guest with
LVM for its filesystems ?

I think it would be better to unify the d-i partman-auto/expert_recipe
in Debian.pm with the one in ts-debian-hvm-install, and make all
Debian HVM installations use LVM.

Wei, do you agree ?  (TBH maybe I should have asked Wei to do that
when he introduced the new preseed setup in ts-debian-hvm-install.)

> > > @@ -63,7 +71,7 @@ d-i partman-auto/expert_recipe string \\
> > >                          use_filesystem{ } filesystem{ vfat } \\
> > >                          mountpoint{ /boot/efi } \\
> > >                  . \\
> > > -                5000 50 5000 ext4 \\
> > > +                10000 50 10000 ext4 \\
> > 
> > I think this needs an explanation.  You mentioned it in your commit
> > message but didn't give reasons.  I think this should perhaps be done
> > in a different way.
> 
> You mean not increase the size uniformly, but conditionally only for
> L1?

I don't know.  When you explain why this is necessary it will
hopefully become clearer to me what I think is the best way to address
the problem.

> > > +if ($nested eq 'nested_L2') {
> > > +    target_cmd_root($gho, "init 0");
> > > +    target_await_down($gho,60);
> > > +    target_ping_check_down($gho);
> > > +}
> > > +if ($nested eq 'nested_L1') {
> > > +    store_runvar("L1_host", $gn);
> > > +    store_runvar("L1_IP", $gho->{Ip});
> > > +    store_runvar("L0_Ident", $whhost);
> > > +    target_cmd_root($gho, "mkdir -p /home/osstest/.ssh && cp
> > /root/.ssh/authorized_keys /home/osstest/.ssh/");
> > > +}
> > 
> > I don't understand the purpose behind these special cases.
> 
> The first block is shut down L2 guest after proving it boots up.

This should be done as a separate test step.  I think you can use the
ts-guest-stop script.

If you want to check that the guest can shut itself off then that
would be a new kind of test step (which could perhaps profitably added
to a wider variety of recipes).


Thanks,
Ian.

Re: [PATCH OSSTEST 11/12] Changes on test step of debain hvm guest install

[Hu, Robert]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: Friday, February 13, 2015 8:03 PM
> To: Hu, Robert
> Cc: xen-devel@lists.xen.org; jfehlig@suse.com; wei.liu2@citrix.com;
> ian.campbell@citrix.com; Pang, LongtaoX
> Subject: RE: [PATCH OSSTEST 11/12] Changes on test step of debain hvm guest
> install
> 
> Hu, Robert writes ("RE: [PATCH OSSTEST 11/12] Changes on test step of debain
> hvm guest install"):
> > [Ian]
> > > Firstly, can you please reformat your commit message so that the
> > > individual points are separated out into paragraphs.  But I think
> > > actually that probably some of this wants to go into different commits
> > > (and perhaps different places).
> >
> > You mean dividing this patch into more pieces/commits?
> 
> Yes.  At least, that might be a good idea.  I'm finding it a bit
> difficult to see the wood for the trees.
> 
> I can, however, offer some general guidelines on when to split/merge:
> 
>  * Changes must be in the same commit if they are mutually
>    interdependent, so that the code only works before all of them or
>    after all of them.
> 
>  * Each commit should be the smallest piece which is a coherent and
>    comprehensible alteration.  (Although small changes might be put
>    together in the same commit, especially if they are conceptually
>    very similar and/or aren't textually interleaved.)
> 
>  * On ordering: it should not normally be necessary to read subsequent
>    commits to understand earlier ones.  So generally that means that
>    new interfaces should be introduced (with any necessary doc
>    comments, etc.) earlier and used later.
> 
>  * It is a good idea to split out refactoring operations, and code
>    motion, each into their own patch.  That makes it much easier to
>    review both the refactoring/motion (because it's much easier to
>    look for unintentional functional changes), and the other patches
>    (because they don't contain non-functional `noise').  When you do
>    this, the refactoring/motion should clearly say what if any
>    functional change is introduced.  `No functional change' is the
>    usual stock phrase for cases when it's true.
> 
> > > I think this should look more like:
> > >
> > > +    run-ts . = ts-debian-hvm-install + host + nested
> > > +    run-ts . = ts-nested-setup + host + nested
> > > +    run-ts . = ts-xen-install nested
> > > +    run-ts . = ts-host-reboot nested
> > > +    run-ts . = ts-debian-hvm-install nested nested2
> > >
> > OK. Since we could only try to learn your design arch/hierarchy of osstest,
> > through code reading, such as terms of test job, test step, recipe, etc.,
> > we inevitably made some misunderstanding or unawareness.
> 
For example, we don't understand the above why some (the first 2) 
passing params to ts-* with '+' and the latter 2 doesn't?
> Indeed.  That's fine.
> 
> > Fortunately getting closer and closer to your mind now.
> 
> I think so, yes.  Of course if you think there is something in the
> current design/architecture which is wrong or broken, then please do
> say so.  We're aware it's not perfect.
> 
> Likewise, if something I suggest doesn't make sense or doesn't seem to
> work well, please do feel free to contradict me.  We're relying on
> your judgement as well as mine :-).
> 
> > Will follow your recipe composing above.
> 
> So, yes, please, if you agree that this is a good way to go.  I think
> it will reduce the amount of nested-specific code elsewhere.
> 
> > > I don't know why you need to use a dedicated VG for your nested
> > > guests's L2 guests - please explain - but if you do, probably
> > > ts-nested-setup could set it up.
> >
> > The existing ts-debian-hvm-install code presume host has vg set
> > for guest installation. To make minimal code change, we'd like
> > to imitate that presumption for L2's host.
> 
> Strictly speaking, the existing ts-debian-hvm-install simply expects
> that the host has a VG.  The existing ts-host-install code (in
> Debian.pm) sets up the VG (via preseed_create).  So I think your
> problem is that ts-debian-hvm-install doesn't set up the guest with
> LVM for its filesystems ?
Yes
> 
> I think it would be better to unify the d-i partman-auto/expert_recipe
> in Debian.pm with the one in ts-debian-hvm-install, and make all
> Debian HVM installations use LVM.
> 
> Wei, do you agree ?  (TBH maybe I should have asked Wei to do that
> when he introduced the new preseed setup in ts-debian-hvm-install.)
Am I supposed to wait for Wei's patch or use my approach for a while and
revert to Wei's patch afterwards?
> 
> > > > @@ -63,7 +71,7 @@ d-i partman-auto/expert_recipe string \\
> > > >                          use_filesystem{ } filesystem{ vfat } \\
> > > >                          mountpoint{ /boot/efi } \\
> > > >                  . \\
> > > > -                5000 50 5000 ext4 \\
> > > > +                10000 50 10000 ext4 \\
> > >
> > > I think this needs an explanation.  You mentioned it in your commit
> > > message but didn't give reasons.  I think this should perhaps be done
> > > in a different way.
> >
> > You mean not increase the size uniformly, but conditionally only for
> > L1?
> 
> I don't know.  When you explain why this is necessary it will
> hopefully become clearer to me what I think is the best way to address
> the problem.
In L1, a 'Debain-xxx-.iso' image will be stored there, therefore needs more
 capacity to host that. 5G size is definitely not enough.
> 
> > > > +if ($nested eq 'nested_L2') {
> > > > +    target_cmd_root($gho, "init 0");
> > > > +    target_await_down($gho,60);
> > > > +    target_ping_check_down($gho);
> > > > +}
> > > > +if ($nested eq 'nested_L1') {
> > > > +    store_runvar("L1_host", $gn);
> > > > +    store_runvar("L1_IP", $gho->{Ip});
> > > > +    store_runvar("L0_Ident", $whhost);
> > > > +    target_cmd_root($gho, "mkdir -p /home/osstest/.ssh && cp
> > > /root/.ssh/authorized_keys /home/osstest/.ssh/");
> > > > +}
> > >
> > > I don't understand the purpose behind these special cases.
> >
> > The first block is shut down L2 guest after proving it boots up.
> 
> This should be done as a separate test step.  I think you can use the
> ts-guest-stop script.
OK
> 
> If you want to check that the guest can shut itself off then that
> would be a new kind of test step (which could perhaps profitably added
> to a wider variety of recipes).
> 
> 
> Thanks,
> Ian.

Re: [PATCH OSSTEST 11/12] Changes on test step of debain hvm guest install

[Wei Liu]

On Sun, Feb 15, 2015 at 09:43:33AM +0000, Hu, Robert wrote:
> > -----Original Message-----
> > From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> > Sent: Friday, February 13, 2015 8:03 PM
> > To: Hu, Robert
> > Cc: xen-devel@lists.xen.org; jfehlig@suse.com; wei.liu2@citrix.com;
> > ian.campbell@citrix.com; Pang, LongtaoX
> > Subject: RE: [PATCH OSSTEST 11/12] Changes on test step of debain hvm guest
> > install
> > 
> > Hu, Robert writes ("RE: [PATCH OSSTEST 11/12] Changes on test step of debain
> > hvm guest install"):
> > > [Ian]
> > > > Firstly, can you please reformat your commit message so that the
> > > > individual points are separated out into paragraphs.  But I think
> > > > actually that probably some of this wants to go into different commits
> > > > (and perhaps different places).
> > >
> > > You mean dividing this patch into more pieces/commits?
> > 
> > Yes.  At least, that might be a good idea.  I'm finding it a bit
> > difficult to see the wood for the trees.
> > 
> > I can, however, offer some general guidelines on when to split/merge:
> > 
> >  * Changes must be in the same commit if they are mutually
> >    interdependent, so that the code only works before all of them or
> >    after all of them.
> > 
> >  * Each commit should be the smallest piece which is a coherent and
> >    comprehensible alteration.  (Although small changes might be put
> >    together in the same commit, especially if they are conceptually
> >    very similar and/or aren't textually interleaved.)
> > 
> >  * On ordering: it should not normally be necessary to read subsequent
> >    commits to understand earlier ones.  So generally that means that
> >    new interfaces should be introduced (with any necessary doc
> >    comments, etc.) earlier and used later.
> > 
> >  * It is a good idea to split out refactoring operations, and code
> >    motion, each into their own patch.  That makes it much easier to
> >    review both the refactoring/motion (because it's much easier to
> >    look for unintentional functional changes), and the other patches
> >    (because they don't contain non-functional `noise').  When you do
> >    this, the refactoring/motion should clearly say what if any
> >    functional change is introduced.  `No functional change' is the
> >    usual stock phrase for cases when it's true.
> > 
> > > > I think this should look more like:
> > > >
> > > > +    run-ts . = ts-debian-hvm-install + host + nested
> > > > +    run-ts . = ts-nested-setup + host + nested
> > > > +    run-ts . = ts-xen-install nested
> > > > +    run-ts . = ts-host-reboot nested
> > > > +    run-ts . = ts-debian-hvm-install nested nested2
> > > >
> > > OK. Since we could only try to learn your design arch/hierarchy of osstest,
> > > through code reading, such as terms of test job, test step, recipe, etc.,
> > > we inevitably made some misunderstanding or unawareness.
> > 
> For example, we don't understand the above why some (the first 2) 
> passing params to ts-* with '+' and the latter 2 doesn't?

sg-run-job is written in tcl. I don't claim to be an expert on this
language. I spent ~1.5 hour to grasp the basic and was able to decrypt
this script. The manual and tutorial on tcl official website are quite
helpful.

All those parameters are passed to the ts-* script. The "+" sign is a
toggle to control if it should appear in the final report. Have a look
at spawn-ts, or more preciously, around L123 (it may vary depending on
your OSSTest tree changeset).

If you look at the actual output of you script, you will see all
parameters are passed to the script.

> > Indeed.  That's fine.
> > 
> > > Fortunately getting closer and closer to your mind now.
> > 
> > I think so, yes.  Of course if you think there is something in the
> > current design/architecture which is wrong or broken, then please do
> > say so.  We're aware it's not perfect.
> > 
> > Likewise, if something I suggest doesn't make sense or doesn't seem to
> > work well, please do feel free to contradict me.  We're relying on
> > your judgement as well as mine :-).
> > 
> > > Will follow your recipe composing above.
> > 
> > So, yes, please, if you agree that this is a good way to go.  I think
> > it will reduce the amount of nested-specific code elsewhere.
> > 
> > > > I don't know why you need to use a dedicated VG for your nested
> > > > guests's L2 guests - please explain - but if you do, probably
> > > > ts-nested-setup could set it up.
> > >
> > > The existing ts-debian-hvm-install code presume host has vg set
> > > for guest installation. To make minimal code change, we'd like
> > > to imitate that presumption for L2's host.
> > 
> > Strictly speaking, the existing ts-debian-hvm-install simply expects
> > that the host has a VG.  The existing ts-host-install code (in
> > Debian.pm) sets up the VG (via preseed_create).  So I think your
> > problem is that ts-debian-hvm-install doesn't set up the guest with
> > LVM for its filesystems ?
> Yes
> > 
> > I think it would be better to unify the d-i partman-auto/expert_recipe
> > in Debian.pm with the one in ts-debian-hvm-install, and make all
> > Debian HVM installations use LVM.
> > 
> > Wei, do you agree ?  (TBH maybe I should have asked Wei to do that
> > when he introduced the new preseed setup in ts-debian-hvm-install.)

Yes. I agree we should use LVM whenever possible. Sorry for not having
done that...

One thing to keep in mind though, EFI boot requires a vfat partition.

> Am I supposed to wait for Wei's patch or use my approach for a while and
> revert to Wei's patch afterwards?

What patch do you expect from me?

Wei.

Re: [PATCH OSSTEST 11/12] Changes on test step of debain hvm guest install

[Hu, Robert]

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@citrix.com]
> Sent: Monday, February 16, 2015 6:17 PM
> To: Hu, Robert
> Cc: Ian Jackson; xen-devel@lists.xen.org; jfehlig@suse.com;
> wei.liu2@citrix.com; ian.campbell@citrix.com; Pang, LongtaoX
> Subject: Re: [PATCH OSSTEST 11/12] Changes on test step of debain hvm guest
> install
> 
> On Sun, Feb 15, 2015 at 09:43:33AM +0000, Hu, Robert wrote:
> > > -----Original Message-----
> > > From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> > > Sent: Friday, February 13, 2015 8:03 PM
> > > To: Hu, Robert
> > > Cc: xen-devel@lists.xen.org; jfehlig@suse.com; wei.liu2@citrix.com;
> > > ian.campbell@citrix.com; Pang, LongtaoX
> > > Subject: RE: [PATCH OSSTEST 11/12] Changes on test step of debain hvm
> guest
> > > install
> > >
> > > Hu, Robert writes ("RE: [PATCH OSSTEST 11/12] Changes on test step of
> debain
> > > hvm guest install"):
> > > > [Ian]
> > > > > Firstly, can you please reformat your commit message so that the
> > > > > individual points are separated out into paragraphs.  But I think
> > > > > actually that probably some of this wants to go into different commits
> > > > > (and perhaps different places).
> > > >
> > > > You mean dividing this patch into more pieces/commits?
> > >
> > > Yes.  At least, that might be a good idea.  I'm finding it a bit
> > > difficult to see the wood for the trees.
> > >
> > > I can, however, offer some general guidelines on when to split/merge:
> > >
> > >  * Changes must be in the same commit if they are mutually
> > >    interdependent, so that the code only works before all of them or
> > >    after all of them.
> > >
> > >  * Each commit should be the smallest piece which is a coherent and
> > >    comprehensible alteration.  (Although small changes might be put
> > >    together in the same commit, especially if they are conceptually
> > >    very similar and/or aren't textually interleaved.)
> > >
> > >  * On ordering: it should not normally be necessary to read subsequent
> > >    commits to understand earlier ones.  So generally that means that
> > >    new interfaces should be introduced (with any necessary doc
> > >    comments, etc.) earlier and used later.
> > >
> > >  * It is a good idea to split out refactoring operations, and code
> > >    motion, each into their own patch.  That makes it much easier to
> > >    review both the refactoring/motion (because it's much easier to
> > >    look for unintentional functional changes), and the other patches
> > >    (because they don't contain non-functional `noise').  When you do
> > >    this, the refactoring/motion should clearly say what if any
> > >    functional change is introduced.  `No functional change' is the
> > >    usual stock phrase for cases when it's true.
> > >
> > > > > I think this should look more like:
> > > > >
> > > > > +    run-ts . = ts-debian-hvm-install + host + nested
> > > > > +    run-ts . = ts-nested-setup + host + nested
> > > > > +    run-ts . = ts-xen-install nested
> > > > > +    run-ts . = ts-host-reboot nested
> > > > > +    run-ts . = ts-debian-hvm-install nested nested2
> > > > >
> > > > OK. Since we could only try to learn your design arch/hierarchy of osstest,
> > > > through code reading, such as terms of test job, test step, recipe, etc.,
> > > > we inevitably made some misunderstanding or unawareness.
> > >
> > For example, we don't understand the above why some (the first 2)
> > passing params to ts-* with '+' and the latter 2 doesn't?
> 
> sg-run-job is written in tcl. I don't claim to be an expert on this
> language. I spent ~1.5 hour to grasp the basic and was able to decrypt
> this script. The manual and tutorial on tcl official website are quite
> helpful.
> 
> All those parameters are passed to the ts-* script. The "+" sign is a
> toggle to control if it should appear in the final report. Have a look
> at spawn-ts, or more preciously, around L123 (it may vary depending on
> your OSSTest tree changeset).
> 
> If you look at the actual output of you script, you will see all
> parameters are passed to the script.
Thanks for your help!
I have also spent days on the tcl basic study half years ago. Since didn't
find '+' as a tcl primitive, got confused.
> 
> > > Indeed.  That's fine.
> > >
> > > > Fortunately getting closer and closer to your mind now.
> > >
> > > I think so, yes.  Of course if you think there is something in the
> > > current design/architecture which is wrong or broken, then please do
> > > say so.  We're aware it's not perfect.
> > >
> > > Likewise, if something I suggest doesn't make sense or doesn't seem to
> > > work well, please do feel free to contradict me.  We're relying on
> > > your judgement as well as mine :-).
> > >
> > > > Will follow your recipe composing above.
> > >
> > > So, yes, please, if you agree that this is a good way to go.  I think
> > > it will reduce the amount of nested-specific code elsewhere.
> > >
> > > > > I don't know why you need to use a dedicated VG for your nested
> > > > > guests's L2 guests - please explain - but if you do, probably
> > > > > ts-nested-setup could set it up.
> > > >
> > > > The existing ts-debian-hvm-install code presume host has vg set
> > > > for guest installation. To make minimal code change, we'd like
> > > > to imitate that presumption for L2's host.
> > >
> > > Strictly speaking, the existing ts-debian-hvm-install simply expects
> > > that the host has a VG.  The existing ts-host-install code (in
> > > Debian.pm) sets up the VG (via preseed_create).  So I think your
> > > problem is that ts-debian-hvm-install doesn't set up the guest with
> > > LVM for its filesystems ?
> > Yes
> > >
> > > I think it would be better to unify the d-i partman-auto/expert_recipe
> > > in Debian.pm with the one in ts-debian-hvm-install, and make all
> > > Debian HVM installations use LVM.
> > >
> > > Wei, do you agree ?  (TBH maybe I should have asked Wei to do that
> > > when he introduced the new preseed setup in ts-debian-hvm-install.)
> 
> Yes. I agree we should use LVM whenever possible. Sorry for not having
> done that...
> 
> One thing to keep in mind though, EFI boot requires a vfat partition.
> 
> > Am I supposed to wait for Wei's patch or use my approach for a while and
> > revert to Wei's patch afterwards?
> 
> What patch do you expect from me?
That Ian mentioned above 
'unify the d-i partman-auto/expert_recipe
in Debian.pm with the one in ts-debian-hvm-install, and make all
Debian HVM installations use LVM.'
> 
> Wei.

Re: [PATCH OSSTEST 11/12] Changes on test step of debain hvm guest install

[Wei Liu]

On Tue, Feb 17, 2015 at 12:45:34AM +0000, Hu, Robert wrote:
[...]
> > 
> > > Am I supposed to wait for Wei's patch or use my approach for a while and
> > > revert to Wei's patch afterwards?
> > 
> > What patch do you expect from me?
> That Ian mentioned above 
> 'unify the d-i partman-auto/expert_recipe
> in Debian.pm with the one in ts-debian-hvm-install, and make all
> Debian HVM installations use LVM.'

I'm afraid I don't have time to do the refactoring and testing any time
soon.

I had a look at d-i's preseed documentation. And this is what I come up
with. Note it's untested patch, just a proof-of-concept what the final
recipe might look like.

A proper upstream patch will require factoring out the common bits first
(/boot, / and swap) and then append test case specific bits (in this
case, the EFI boot partition) later.

Wei.

diff --git a/ts-debian-hvm-install b/ts-debian-hvm-install
index 449b96c..e87a2c0 100755
--- a/ts-debian-hvm-install
+++ b/ts-debian-hvm-install
@@ -54,6 +54,12 @@ d-i partman-auto/method string  regular

 d-i partman-auto/expert_recipe string \\
         boot-root :: \\
+                100 50 100 ext4
+                       \$primary{ } \$bootable{ }                      \\
+                       method{ format } format{ }                      \\
+                       use_filesystem{ } filesystem{ ext3 }            \\
+                       mountpoint{ /boot }                             \\
+               .                                                       \\
                 512 50 512 vfat \\
                         \$primary{ } \$bootable{ } \\
                         method{ efi } format{ } \\
@@ -63,10 +69,10 @@ d-i partman-auto/expert_recipe string \\
                 5000 50 5000 ext4 \\
                         method{ format } format{ } \\
                         use_filesystem{ } filesystem{ ext4 } \\
-                        mountpoint{ / } \\
+                        mountpoint{ / } \$lvmok{ } \\
                 . \\
                 512 30 100% linux-swap \\
-                        method{ swap } format{ } \\
+                        method{ swap } format{ } \$lvmok{ } \\
                 .


> > Wei.

Re: [PATCH OSSTEST 11/12] Changes on test step of debain hvm guest install

[Wei Liu]

On Tue, Feb 17, 2015 at 10:37:39AM +0000, Wei Liu wrote:
> On Tue, Feb 17, 2015 at 12:45:34AM +0000, Hu, Robert wrote:
> [...]
> > > 
> > > > Am I supposed to wait for Wei's patch or use my approach for a while and
> > > > revert to Wei's patch afterwards?
> > > 
> > > What patch do you expect from me?
> > That Ian mentioned above 
> > 'unify the d-i partman-auto/expert_recipe
> > in Debian.pm with the one in ts-debian-hvm-install, and make all
> > Debian HVM installations use LVM.'
> 
> I'm afraid I don't have time to do the refactoring and testing any time
> soon.
> 
> I had a look at d-i's preseed documentation. And this is what I come up
> with. Note it's untested patch, just a proof-of-concept what the final
> recipe might look like.
> 
> A proper upstream patch will require factoring out the common bits first
> (/boot, / and swap) and then append test case specific bits (in this
> case, the EFI boot partition) later.
> 
> Wei.
> 
> diff --git a/ts-debian-hvm-install b/ts-debian-hvm-install
> index 449b96c..e87a2c0 100755
> --- a/ts-debian-hvm-install
> +++ b/ts-debian-hvm-install
> @@ -54,6 +54,12 @@ d-i partman-auto/method string  regular
> 
>  d-i partman-auto/expert_recipe string \\
>          boot-root :: \\
> +                100 50 100 ext4
> +                       \$primary{ } \$bootable{ }                      \\
> +                       method{ format } format{ }                      \\
> +                       use_filesystem{ } filesystem{ ext3 }            \\
                                                        ext4

Copy and paste error, sorry.

> +                       mountpoint{ /boot }                             \\
> +               .                                                       \\
>                  512 50 512 vfat \\
>                          \$primary{ } \$bootable{ } \\
                                          ^^^^^^^^
And you might want to get rid of this bootable flag.

The testing of this patch will require you to run at least
test-amd64-amd64-xl-qemuu-{debianhvm,ovmf}-amd64.

Wei.

Re: [PATCH OSSTEST 11/12] Changes on test step of debain hvm guest install

[Ian Jackson]

Wei Liu writes ("Re: [PATCH OSSTEST 11/12] Changes on test step of debain hvm guest install"):
> On Sun, Feb 15, 2015 at 09:43:33AM +0000, Hu, Robert wrote:
> > For example, we don't understand the above why some (the first 2) 
> > passing params to ts-* with '+' and the latter 2 doesn't?
> 
> sg-run-job is written in tcl. I don't claim to be an expert on this
> language. I spent ~1.5 hour to grasp the basic and was able to decrypt
> this script. The manual and tutorial on tcl official website are quite
> helpful.
> 
> All those parameters are passed to the ts-* script. The "+" sign is a
> toggle to control if it should appear in the final report. Have a look
> at spawn-ts, or more preciously, around L123 (it may vary depending on
> your OSSTest tree changeset).

More precisely the `+' toggles whether the parameter becomes part of
the `testid', which is the unique identifier (within a job) for a
particular test step.

> If you look at the actual output of you script, you will see all
> parameters are passed to the script.

All except the `+', of course.

I'm sorry that the arguments to run-ts, spawn-ts, et al, are not
properly documented.  Thanks to Wei for explaining.

Ian.

Re: [PATCH OSSTEST 05/12] Add and expose some testsupport APIs

[Pang, LongtaoX]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: Wednesday, February 11, 2015 10:54 PM
> To: Hu, Robert
> Cc: xen-devel@lists.xen.org; jfehlig@suse.com; wei.liu2@citrix.com;
> ian.campbell@citrix.com; Pang, LongtaoX
> Subject: Re: [PATCH OSSTEST 05/12] Add and expose some testsupport APIs
> 
> Robert Ho writes ("[PATCH OSSTEST 05/12] Add and expose some testsupport
> APIs"):
> > When install L2 guest, we will need to invoke  'select_ether' to get
> > guest MAC address. So here expose select_ether().
> 
> I'm not sure whether you actually need to do this.  I will look at the rest of
> your series to see why prepareguest() isn't suitable.  But this part of the patch
> is fine in principle.
> 
> > And
> 
> These seem like two indepenedent patches.  They should be split up.
> 
> >  also, we added another function 'guest_editconfig_cd' and expose it.
> >   This function bascically changes guest boot device sequence and
> > alter its on_reboot behavior to restart.
> 
> I don't understand why guest_editconfig_nocd isn't sufficient.
> 
After finishing L1 guest VM installation, we need to change L1 guest boot sequence from ISO image to hard disk, 
so we will edit the "boot=cd" in hvm configure file and we added 'guest_editconfig_cd'. But, guest_editconfig_nocd could not get
this point.
> Ian.

Re: [PATCH OSSTEST 11/12] Changes on test step of debain hvm guest install

[Pang, LongtaoX]

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@citrix.com]
> Sent: Tuesday, February 17, 2015 6:47 PM
> To: Hu, Robert
> Cc: Wei Liu; Ian Jackson; xen-devel@lists.xen.org; jfehlig@suse.com;
> ian.campbell@citrix.com; Pang, LongtaoX
> Subject: Re: [PATCH OSSTEST 11/12] Changes on test step of debain hvm guest
> install
> 
> On Tue, Feb 17, 2015 at 10:37:39AM +0000, Wei Liu wrote:
> > On Tue, Feb 17, 2015 at 12:45:34AM +0000, Hu, Robert wrote:
> > [...]
> > > >
> > > > > Am I supposed to wait for Wei's patch or use my approach for a
> > > > > while and revert to Wei's patch afterwards?
> > > >
> > > > What patch do you expect from me?
> > > That Ian mentioned above
> > > 'unify the d-i partman-auto/expert_recipe in Debian.pm with the one
> > > in ts-debian-hvm-install, and make all Debian HVM installations use
> > > LVM.'
> >
> > I'm afraid I don't have time to do the refactoring and testing any
> > time soon.
> >
> > I had a look at d-i's preseed documentation. And this is what I come
> > up with. Note it's untested patch, just a proof-of-concept what the
> > final recipe might look like.
> >
> > A proper upstream patch will require factoring out the common bits
> > first (/boot, / and swap) and then append test case specific bits (in
> > this case, the EFI boot partition) later.
> >
> > Wei.
> >
> > diff --git a/ts-debian-hvm-install b/ts-debian-hvm-install index
> > 449b96c..e87a2c0 100755
> > --- a/ts-debian-hvm-install
> > +++ b/ts-debian-hvm-install
> > @@ -54,6 +54,12 @@ d-i partman-auto/method string  regular
> >
> >  d-i partman-auto/expert_recipe string \\
> >          boot-root :: \\
> > +                100 50 100 ext4
> > +                       \$primary{ } \$bootable{ }
> \\
> > +                       method{ format } format{ }
> \\
> > +                       use_filesystem{ } filesystem{ ext3 }
> \\
>                                                         ext4
> 
> Copy and paste error, sorry.
> 
> > +                       mountpoint{ /boot }
> \\
> > +               .
> \\
> >                  512 50 512 vfat \\
> >                          \$primary{ } \$bootable{ } \\
>                                           ^^^^^^^^ And you might
> want to get rid of this bootable flag.
> 
> The testing of this patch will require you to run at least
> test-amd64-amd64-xl-qemuu-{debianhvm,ovmf}-amd64.
>
Since this is just a proof-of-concept patch, could you provide a workable one based on latest OSSTest master branch, 
and make all Debian HVM installations use LVM?
 
> Wei.