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

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

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.

Perhaps we could avoid this whole issue by avoiding any reference to a
specific l2 guest at this point completely, and just consider things as
"guest storage for use by l1 hypervisor".

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")

(for whichever info needs preserving for later)

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

I think the discussion above my have invalidate the need to do this, but
I think there wouldn't be much harm in having the $l1 Guest struct take
on some of the $l0 Host struct's properties, e.g. copy over some list of
which dhcp-watch-method would be one.

This is the same problem I mentioned above arising from $l1 doing double
duty as both host and guest. There are places in osstest which are
pretty fluid about this (if the object has the approrpaite props you can
use it as either), but it can be a bit confusing for the poor programmer
or reader...

Perhaps having a
   our $l1ho = make_nested_host($l0ho, $l1guest);
construct, which does copies $l1guest and propagates some properties as
above and then using $l1guest + $l1host in the appropriate places only
would help keep things straight?

(Best to wait for Ian's feedback on this before doing anything, he may
not like it)

Ian.