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

[Ian Campbell <ian.campbell@citrix.com>]

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?

> > 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.

> 
> > > +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 so, that seems to make sense since the script is in effect
acting on three entities.

> > 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.

One wrinkle is whether the lv is in the l0 or l1 "namespace". I'm not
quite sure how that will fit together.

Ian.