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

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

On Fri, 2015-04-24 at 08:45 +0000, Pang, LongtaoX wrote:
> 
> 
> > -----Original Message-----
> > From: Ian Campbell [mailto:ian.campbell@citrix.com]
> > Sent: Thursday, April 23, 2015 7:30 PM
> > To: Hu, Robert
> > Cc: Pang, LongtaoX; xen-devel@lists.xen.org; Ian.Jackson@eu.citrix.com;
> > wei.liu2@citrix.com
> > Subject: Re: [OSSTEST Nested PATCH v8 5/7] Add new script to customize nested
> > test configuration
> > 
> > On Thu, 2015-04-23 at 17:38 +0800, Robert Hu wrote:
> > > > > 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.
> > 
> > Ah I see, today usually the ident is "host" or "dst_host" or "src_host"
> > so I got confused by it just being "nested_l1" here.
> > 
> > In the normal case today the ident runvars are set by ts-hosts-allocate.
> > That can't apply to the nested case since it is allocating a guest and
> > not a host, so your ts-nested-setup seems like a reasonable enough place
> > to do it.
> > 
> > However, I don't think there is any reason for the indent, hostname and
> > guest name to differ, and I think perhaps it would be clearer to write
> > this as:
> >         our $l1_ident = $gho->{Guest};
> >         store_runvar($l1_ident, $gho->{Guest});
> > So that it is clear to the reader that this runvar is an ident. I
> > suppose a code comment would work as well (or in addition perhaps).
> > 
> > > >
> > > > > > 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?
> > 
> > $l1_ident and $l1_name should be the same, I think. Semantically the
> > script should be taking $l0_ident and $l1_ident, where $l0 here is a
> > host and $l1 here is a guest, so infact isn't $l1_nae just $l1->{Guest}?
> > 
> > (Things get confusing because $l1 can be a Guest or a Host depending on
> > which test script we are in)
> > 
> > > > > > > +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.
> > 
> > The issue we are coming across here is that we are trying to talk about
> > the l2 guest before it really exists, since that happens later when
> > prepareguest is called in the l1 context and here we are really in l0
> > context where the concept of a specific l2 guest doesn't really exist.
> > So my suggestion to use prepareguest_part_lvmdisk doesn't really hold
> > together.
> >
> So, is it means that will not use ' prepareguest_part_lvmdisk' to
> create lv storage disk inside L0 host, which used for installing L2
> guest, right?

Right. It might be nice to refactor some of prepareguest_part_lvmdisk
into a more generic helper which can be called to create an arbitrary
LVM LV though.

> > IOW the naming would be based on the l1 ident and some suffix, e.g.
> > "_guest_storage". So, given $l1_ident from above, you would do:
> > 
> > $guest_storage_lv_name = "${l1_ident}_guest_storage"
> > # make the LV
> > store_runvar("${l1_ident}_guest_storage_lv", $guest_storage_lv_name)
> > store_runvar("${l1_ident}_guest_storage_vdev", "xvde")
> > 
> Could you please make it clear, what's it used for that store above two runvars?

Those were just two random examples of properties of the second device
which I thought other code _might_ be interested in which I picked to
illustrate what I was talking about, they might not be needed in
reality.

Ian.