Re: [OSSTEST PATCH 1/4] Add nested testcase of preparing and installing L1 guest VM

[Robert Hu <robert.hu@vmm.sh.intel.com>]

On Thu, 2014-12-11 at 11:06 +0000, Wei Liu wrote:
> On Wed, Dec 10, 2014 at 04:07:37PM +0800, longtao.pang wrote:
> > From: "longtao.pang" <longtaox.pang@intel.com>
> > 
> > This patch is used for preparing and installing L1 guest VM inside L0 system
> > on testhost machine.
> > 
> > ---
> >  Osstest/Debian.pm      |   27 ++++++++++++++++++---------
> >  Osstest/TestSupport.pm |   31 ++++++++++++++++++++++++++-----
> >  sg-run-job             |    5 +++++
> >  ts-debian-hvm-install  |   34 ++++++++++++++++++++++++----------
> >  4 files changed, 73 insertions(+), 24 deletions(-)
> > 
> > diff --git a/Osstest/Debian.pm b/Osstest/Debian.pm
> > index c8db601..a671d20 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 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
> > @@ -34,6 +35,7 @@ BEGIN {
> >      @EXPORT      = qw(debian_boot_setup
> >                        %preseed_cmds
> >                        preseed_base
> > +                      setupboot_grub2
> 
> Why do you want to export this helper? I think debian_setup_boot will
> just choose the right one amongst uboot, grub1 and grub2.
> 
> >                        preseed_create
> >                        preseed_hook_command preseed_hook_installscript
> >                        di_installcmdline_core
> > @@ -286,15 +288,18 @@ sub setupboot_grub2 ($$$) {
> >      
> >          my $count= 0;
> >          my $entry;
> > +	my $submenu;
> >          while (<$f>) {
> >              next if m/^\s*\#/ || !m/\S/;
> >              if (m/^\s*\}\s*$/) {
> > -                die unless $entry;
> > +                die unless $entry || $submenu;
> > +	    	if(!defined $entry && defined $submenu){
> > +		  logm("Met end of a submenu starting from $submenu->{StartLine}.Our want kern is $want_kernver");
> > +		  $submenu=undef;
> > +		  next;
> > +		}
> >                  my (@missing) =
> > -                    grep { !defined $entry->{$_} } 
> > -		        (defined $xenhopt
> > -			 ? qw(Title Hv KernDom0 KernVer)
> > -			 : qw(Title Hv KernOnly KernVer));
> > +		grep { !defined $entry->{$_} }  (defined $xenhopt ? qw(Title Hv KernDom0 KernVer) : qw(Title Hv KernOnly KernVer));
> 
> Please don't make non-functional change like this.
> 
> >  		if (@missing) {
> >  		    logm("(skipping entry at $entry->{StartLine};".
> >  			 " no @missing)");
> > @@ -317,21 +322,25 @@ 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+)/) {
> > +#	    if (m/^\s*multiboot\s*(?:\/boot)*\/(xen\-[0-9][-+.0-9a-z]*\S+)/) {
> 
> And if this line is redundant, remove it. What's the reason of changing
> this regex? Are you using non-debian based distro?
> 
> >                  die unless $entry;
> >                  $entry->{Hv}= $1;
> >              }
> > -            if (m/^\s*multiboot\s*\/(vmlinu[xz]-(\S+))/) {
> > +	    if (m/^\s*multiboot\s*(?:\/boot)*\/(vmlinu[xz]-(\S+))/) {
> 
> And this?
> 
> >                  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;
> >              }
> >          }
> 
> As I said before, this hunk should be in its own patch.
> 
> Just FYI, there are multiple people (Dario, you and I) touching this
> piece of code. You might want to keep an eye on main OSSTest git tree
> and rebase before reposting (and so do Dario and I).
> 
> > diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
> > index a3b6936..1e47039 100644
> > --- a/Osstest/TestSupport.pm
> > +++ b/Osstest/TestSupport.pm
> > @@ -55,8 +55,9 @@ BEGIN {
> >                        target_putfilecontents_stash
> >  		      target_putfilecontents_root_stash
> >                        target_put_guest_image target_editfile
> > -                      target_editfile_root target_file_exists
> > -                      target_run_apt
> > +		      target_editfile_root target_file_exists 
> > +		      target_file_exists_root
> > +		      target_run_apt
> 
> Please don't just change white spaces. This makes patches hard to
> review.
> 
> >                        target_install_packages target_install_packages_norec
> >                        target_jobdir target_extract_jobdistpath_subdir
> >                        target_extract_jobdistpath target_guest_lv_name
> > @@ -67,7 +68,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 +110,7 @@ BEGIN {
> >                        iso_gen_flags_basic
> >                        iso_copy_content_from_image
> >                        guest_editconfig_nocd
> > +		      guest_editconfig_cd
> 
> Indentation. I think we mostly use space instead of hard tab. Ian?
So what's the convention in Xen code writing? use space instead of tab?
And how many space to substitute 1 tab? I used to use 4. 
> 
> >                        );
> >      %EXPORT_TAGS = ( );
> >  
> > @@ -481,6 +483,14 @@ sub target_file_exists ($$) {
> >      die "$rfile $out ?";
> >  }
> >  
> > +sub target_file_exists_root ($$) {
> > +    my ($ho,$rfile) = @_;
> > +    my $out= target_cmd_output_root($ho, "if test -e $rfile; then echo y; fi");
> > +    return 1 if $out =~ m/^y$/;
> > +    return 0 if $out !~ m/\S/;
> > +    die "$rfile $out ?";
> > +}
> > +
> >  sub teditfileex {
> >      my $user= shift @_;
> >      my $code= pop @_;
> > @@ -717,6 +727,7 @@ sub power_cycle_time ($) {
> >  sub power_cycle ($) {
> >      my ($ho) = @_;
> >      $mjobdb->host_check_allocated($ho);
> > +    $mjobdb->xen_check_installed($ho);
> 
> And this is? I don't see implementation in this patch set.
> 
> Also this routine is called by ts-host-install, which doesn't necessary
> require Xen to be installed.
> 
> >      die "refusing to set power state for host $ho->{Name}".
> >  	" possibly shared with other jobs\n"
> >  	if $ho->{SharedMaybeOthers};
> > @@ -937,7 +948,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 {
> 
> This should go into its own patch as well. I think it's probably nested
> virt is slower than real hardware so you need some more time?
> 
> >          my $output;
> >          if (!eval {
> >              $output= target_cmd_output($ho, <<END, 40);
> > @@ -1465,7 +1476,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}' ]
> 
> What is this needed? If it's necessary, please use a single commit and
> state the reason in commit log.
> 
> >  #
> >  on_poweroff = 'destroy'
> >  on_reboot   = '$onreboot'
> > @@ -2063,4 +2074,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/;
> 
> This pattern is different than the one used to match. This should also
> go into its own commit -- "Introduce guest_editconfig_cd".
> 
> > +        }
> > +        s/^on_reboot.*/on_reboot='restart'/;
> > +    });
> > +}
> > +
> >  1;
> > diff --git a/sg-run-job b/sg-run-job
> > index 2cf810a..8dcf7af 100755
> > --- a/sg-run-job
> > +++ b/sg-run-job
> > @@ -288,6 +288,11 @@ 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
> > +}
> > +
> >  proc test-guest-migr {g} {
> >      if {[catch { run-ts . = ts-migrate-support-check + host $g }]} return
> >  
> 
> This hunk should go into its own commit.
> 
> > diff --git a/ts-debian-hvm-install b/ts-debian-hvm-install
> > index 37eade2..6dcec3c 100755
> > --- a/ts-debian-hvm-install
> > +++ b/ts-debian-hvm-install
> 
> The modification of debian-hvm-install should also go into a single
> commit.
> 
> > @@ -28,22 +28,24 @@ 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_L1,$guesthost) = @ARGV;
> >  $whhost ||= 'host';
> > -$gn ||= 'debianhvm';
> > -
> > +$nested_L1 ||= '';
> > +if ($nested_L1 eq 'nested_L1') {
> > +    $gn ||= 'nested';
> > +    $guesthost ||= "$gn.l1.osstest";
> > +} else {
> > +    $gn ||= 'debianhvm';
> > +    $guesthost= "$gn.guest.osstest";
> > +}
> >  our $ho= selecthost($whhost);
> > -
> > +our $disk_mb= 50000;
> >  # 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};
> >  
> > -
> 
> Stray blank line change. Please avoid this kind of changes.
> 
> >  sub preseed () {
> >  
> >      my $preseed_file = preseed_base('wheezy','',());
> > @@ -63,7 +65,7 @@ d-i partman-auto/expert_recipe string \\
> >                          use_filesystem{ } filesystem{ vfat } \\
> >                          mountpoint{ /boot/efi } \\
> >                  . \\
> > -                5000 50 5000 ext4 \\
> > +                25000 50 25000 ext4 \\
> >                          method{ format } format{ } \\
> >                          use_filesystem{ } filesystem{ ext4 } \\
> >                          mountpoint{ / } \\
> > @@ -155,6 +157,8 @@ sub prep () {
> >      more_prepareguest_hvm($ho,$gho, $ram_mb, $disk_mb,
> >                            OnReboot => 'preserve',
> >                            Bios => $r{bios},
> > +                          DefVcpus => 4,
> 
> And where is this handled?
> 
> Wei.
> 
> > +                          ExtraConfig => '#nestedhvm=1',
> >                            PostImageHook => sub {
> >          my $cmds = iso_copy_content_from_image($gho, $newiso);
> >          $cmds .= prepare_initrd($initrddir,$newiso,$preseed_file_path);
> > @@ -176,6 +180,8 @@ my $ram_minslop = 100;
> >  my $ram_lots = 5000;
> >  if ($host_freemem_mb > $ram_lots * 2 + $ram_minslop) {
> >      $ram_mb = $ram_lots;
> > +} elsif ($nested_L1 eq 'nested_L1') {
> > +    $ram_mb = 2048;
> >  } else {
> >      $ram_mb = 768;
> >  }
> > @@ -192,7 +198,15 @@ if ($stage<2) {
> >      guest_destroy($ho,$gho);
> >  }
> >  
> > -guest_editconfig_nocd($gho,$emptyiso);
> > +if ($nested_L1 eq 'nested_L1') {
> > +    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_L1 eq 'nested_L1') {
> > +    target_cmd_root($gho, "mkdir -p /home/osstest/.ssh && cp /root/.ssh/authorized_keys /home/osstest/.ssh/");
> > +}
> > +
> > -- 
> > 1.7.10.4