|
[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 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?
> );
> %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 |