Re: [OSSTEST Nested PATCH v8 5/7] Add new script to customize nested test configuration

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

On Wed, 2015-04-22 at 10:56 +0100, Ian Campbell wrote:
> On Wed, 2015-04-22 at 08:35 +0000, Pang, LongtaoX wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Ian Campbell [mailto:ian.campbell@citrix.com]
> > > Sent: Tuesday, April 21, 2015 6:40 PM
> > > To: Pang, LongtaoX
> > > Cc: xen-devel@lists.xen.org; Ian.Jackson@eu.citrix.com; wei.liu2@citrix.com; Hu,
> > > Robert
> > > Subject: Re: [OSSTEST Nested PATCH v8 5/7] Add new script to customize nested
> > > test configuration
> > > 
> > > On Mon, 2015-04-13 at 17:19 -0400, longtao.pang wrote:
> > > > 1. In this script, make some appropriate runvars which selecthost would
> > > > recognise.
> > > > 2. Prepare the configurations for installing L2 guest VM.
> > > > 3. Create a lv disk in L0 and hot-attach it to L1, need to restart L1 to
> > > > make the block disk to be recognized by L1 system, then using this disk
> > > > to create a VG that used for installing L2.
> > > >
> > > > Signed-off-by: longtao.pang <longtaox.pang@intel.com>
> > > > ---
> > > > Changes in v8:
> > > > 1. Replace '$nested_host' by '$l1->{Guest}'.
> > > > ---
> > > >  ts-nested-setup |   52
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 52 insertions(+)
> > > >  create mode 100755 ts-nested-setup
> > > >
> > > > diff --git a/ts-nested-setup b/ts-nested-setup
> > > > new file mode 100755
> > > > index 0000000..41d5e80
> > > > --- /dev/null
> > > > +++ b/ts-nested-setup
> > > > @@ -0,0 +1,52 @@
> > > > +use strict qw(vars);
> > > > +use DBI;
> > > > +use Osstest;
> > > > +use Osstest::Debian;
> > > > +use Osstest::TestSupport;
> > > > +
> > > > +tsreadconfig();
> > > > +our ($l0,$l1) = ts_get_host_guest(@ARGV);
> > > > +
> > > > +guest_check_ip($l1);
> > > > +target_cmd_root($l1, "mkdir -p /home/osstest/.ssh && cp
> > > /root/.ssh/authorized_keys /home/osstest/.ssh/");
> > > > +my $url =
> > > $c{WebspaceUrl}.$c{WebspaceCommon}.$l0->{Name}."_".'overlay.tar';
> > > > +target_cmd_root($l1, <<END);
> > > > +    wget -O overlay.tar $url
> > > > +    tar -xf overlay.tar -C /
> > > > +    rm overlay.tar -f
> > > > +    update-rc.d osstest-confirm-booted start 99 2 .
> > > > +END
> > > 
> > > I cc'd you on some patches which I think should help avoid this
> > > duplication.
> > > 
> > I am trying that.
> > > > +
> > > > +store_runvar('nested_l1',$l1->{Guest});
> > > 
> > > I'm not sure what this is for and it would normally be wrong to hardcode
> > > nested_l1 like that, where is it used?
> > > 
> > As mentioned in [0000 Patch], 'nested_l1' is ident for L1 guest VM,
> > 'nestedl1' is hostname for L1 guest VM.
> > ' store_runvar('nested_l1',$l1->{Guest});' is used to store L1's ident
> > and L1's hostname to database, that will be used for 'selecthost()'
> > function in later script.
> 
> Having a runvar with the bare ident as a name doesn't seem right.
> Perhaps you want to be writing to $r{"${ident}_hostname"} or some such?
> 
> What do you actually need the hostname for anyway?
Look at selecthost()
sub selecthost ($) {
    my ($ident) = @_;
    # must be run outside transaction
    my $name;
    if ($ident =~ m/=/) {
        $ident= $`;
        $name= $'; #'
        $r{$ident}= $name;
    } else {
        $name= $r{$ident};
        die "no specified $ident" unless defined $name;
    }
...

When in L1 iteration invocation of ts-debian-hvm-install(), the ident
passed in is L1 ident ("nested_l1"). Here the 'else' branch will need
$r{$ident}, whose value shall be L1's name. Therefore we prepare this
runvar in advance.
> 
> > > Most places you seem to use nestedl1, not nested_l1. I think you ended
> > > up with this due to not using guest_var and the various hardcoding
> > > you've done elsewhere. I suspect with all that fixed and make-flight
> > > updated accordingly you won't need this var any more.
> > > 
> > I think I should define L1 ident with " my $l1_ident = 'nested_l1' in 'ts-nested-setup'
> 
> Hardcoding anything like that in ts-nested-setup seems wrong to me.
> 
> The ident should come from the script's parameters (i.e. be part of the
> sg-run-job invocation of the script).
> 
> Imagine a hypothetical nested-virt migration test, in that scenario you
> would want to run ts-nested-setup on two hosts, both nestedl1src and
> nestedl2dst in order to configure things. That's why we don't hardcode
> such things.
> 
so to summarize, ts-nested-setup will be invoked with parameters of
$l0_ident, $l1_ident and $l1_name?
or only parameters of $l0_ident and $l1_ident, if we have setup
$l1_ident->$l1_name mapping in runvar when in l0's iteration of
ts-debian-hvm-install.
which would you prefer?
> > 
> > > > +store_runvar("$l1->{Guest}_ip",$l1->{Ip});
> > > > +
> > > > +my $l2_disk_mb = 20000;
> > > > +my $l2_lv_name = 'nestedl2-disk';
> > > > +my $vgname = $l0->{Name};
> > > > +$vgname .= ".$c{TestHostDomain}" if ($l0->{Suite} =~ m/lenny/);
> > > > +target_cmd_root($l0, <<END);
> > > > +    lvremove -f /dev/${vgname}/${l2_lv_name} ||:
> > > > +    lvcreate -L ${l2_disk_mb}M -n $l2_lv_name $vgname
> > > > +    dd if=/dev/zero of=/dev/${vgname}/${l2_lv_name} count=10
> > > 
> > > I think you can do all of this by passing a suitable l2 $gho to
> > > prepareguest_part_lvmdisk, can't you?
> > > 
> > > I think you can get that by my $l2 = selectguest($ARGV[????], $l1).
> > > 
> > I think I could try it, that means I will add more parameters for
> > 'ts-nested-setup' script, not just 'host' and 'nestedl1'.
I think we can pass in 3 parameters: $l0_ident, $l1_ident, and
$l2_ident, as long as we in advance setup their $ident-->$name mappings
in runvar. Here we have 2 options:
1. setup such mapping in first iteration (l0 interation) of
ts-debian-hvm-install
2. setup this in make flight
I prefer 2. since such mapping can be fixed from beginning and shall not
be changed in run time.
> 
> I think so, that seems to make sense since the script is in effect
> acting on three entities.
> 
If we use selectguest($l2_name,$l1_host) in scope of ts-nested-setup,
then we probably need to change 
sub dhcp_watch_setup ($$) {
    my ($ho,$gho) = @_;

    my $meth = get_host_property($ho,'dhcp-watch-method',undef);
--> my $meth = get_target_property($ho,'dhcp-watch-method',undef);
    $gho->{DhcpWatch} = get_host_method_object($ho, 'DhcpWatch', $meth);
}
since here $l1_host is actually a Guest struct rather than Host, it
doesn't have 'dhcp-watch-method'.
Are you OK with this change? I think get_target_property() suites both
situations.
 
> > > Where ARGV[????] is a new parameter passed by sg-run-job i.e. nestedl2,
> > > i.e. the one after whatever ts_get_host_guest consumes at the top of the
> > > file (so ARGV[2] perhaps?).
> > > 
> > Also, I think if use ' prepareguest_part_lvmdisk', I need set
> > 'nestedl2_vg' and 'nestedl2 _disk_lv' by make-flight.
> 
> I think you need to have called guest_find_lv at some point so these are
> set.
> 
> > But, when reuse 'ts-debian-hvm-install' to install L2, the '
> > prepareguest_part_lvmdisk' will be executed again(in
> > 'more_prepareguest_hvm' function), then there will be multi runvars
> > for 'nestedl2_vg' and 'nestedl2_disk_lv' in standalone.db, is that OK?
> > Please correct me if I am wrong.
> 
> You can't end up with multiple runvars, I think the subsequent
> invocations should just be reusing things.
I think not reusing, but override, with same values. So I think it's
fine.
> 
> One wrinkle is whether the lv is in the l0 or l1 "namespace". I'm not
> quite sure how that will fit together.
yeah, you hit it. Theoretically, it shall be in l1 namespace, i.e. using
l1's LV, but since currently L1 doesn't use LV formatting but raw
partition (you mentioned before Wei is doing the uniformity), so here it
is actually in l0's namespace.
In sub guest_find_lv ($) {
    my ($gho) = @_;
    my $gn= $gho->{Guest};
    $gho->{Vg}= $r{"${gn}_vg"};
    $gho->{Lv}= $r{"${gn}_disk_lv"};
    $gho->{Lvdev}= (defined $gho->{Vg} && defined $gho->{Lv})
        ? '/dev/'.$gho->{Vg}.'/'.$gho->{Lv} : undef;
}
you can see l2's Lvdev has different path from l1's, while both of them
provided by l0's LV layer.
> 
> Ian.
>