|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [OSSTEST PATCH 1/4] Add nested testcase of preparing and installing L1 guest VM
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@xxxxxxxxx>
> >
> > 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |