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

["Pang, LongtaoX" <longtaox.pang@xxxxxxxxx>]

> -----Original Message-----
> From: Ian Campbell [mailto:ian.campbell@xxxxxxxxxx]
> Sent: Thursday, April 23, 2015 7:30 PM
> To: Hu, Robert
> Cc: Pang, LongtaoX; xen-devel@xxxxxxxxxxxxx; Ian.Jackson@xxxxxxxxxxxxx;
> wei.liu2@xxxxxxxxxx
> 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?

> 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".
> 
Yes, I think so.

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

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

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