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

["Hu, Robert" <robert.hu@xxxxxxxxx>]

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@xxxxxxxxxxxxx]
> Sent: Friday, February 13, 2015 2:17 AM
> To: Hu, Robert
> Cc: xen-devel@xxxxxxxxxxxxx; jfehlig@xxxxxxxx; wei.liu2@xxxxxxxxxx;
> ian.campbell@xxxxxxxxxx; 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.

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