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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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.

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