Re: [Xen-devel] [OSSTEST Nested PATCH v8 4/7] Changes on test step of Debian hvm guest install

["Hu, Robert" <robert.hu@xxxxxxxxx>]

> -----Original Message-----
> From: Ian Campbell [mailto:ian.campbell@xxxxxxxxxx]
> Sent: Thursday, April 23, 2015 2:53 PM
> To: Hu, Robert
> Cc: Pang, LongtaoX; xen-devel@xxxxxxxxxxxxx; Ian.Jackson@xxxxxxxxxxxxx;
> wei.liu2@xxxxxxxxxx
> Subject: Re: [OSSTEST Nested PATCH v8 4/7] Changes on test step of Debian
> hvm guest install
> 
> On Thu, 2015-04-23 at 13:59 +0800, Robert Hu wrote:
> > On Tue, 2015-04-21 at 11:28 +0100, Ian Campbell wrote:
> > > On Mon, 2015-04-13 at 17:19 -0400, longtao.pang wrote:
> > > > 1. Increase disk size to accommodate to nested test requirement.
> > > > 2. Since 'Debain-xxx-.iso' image will be stored in rootfs of L1 guest,
> > > > therefore needs more disk capacity, increase root partition size in
> > > > preseed generation.
> > > > 3. In L1 installation context, assign more memory to it; Since it
> > > > acts as a nested hypervisor anyway.
> > > > 4. Comment out CDROM entry in sources.list to make HTTP URL entry
> > > > available for L1 hvm guest.
> > > > 5. Enable nestedhvm feature in ExtraConfig for nested job.
> > > >
> > > > Signed-off-by: longtao.pang <longtaox.pang@xxxxxxxxx>
> > > > ---
> > > > Changes in v8:
> > > > 1. Update a conventional way to comment out CDROM entry in
> sources.list.
> > > > 2. Enable nestedhvm feature only for the nested job.
> > > > 3. Set nested disk and memroy size to be driven from runvars in
> > > > 'ts-debian-hvm-install'.
> > > > ---
> > > >  ts-debian-hvm-install |    9 ++++++---
> > > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/ts-debian-hvm-install b/ts-debian-hvm-install
> > > > index cfd5144..13009d0 100755
> > > > --- a/ts-debian-hvm-install
> > > > +++ b/ts-debian-hvm-install
> > > > @@ -36,7 +36,7 @@ our $ho= selecthost($whhost);
> > > >
> > > >  # guest memory size will be set based on host free memory, see below
> > > >  our $ram_mb;
> > > > -our $disk_mb= 10000;
> > > > +our $disk_mb= $r{'nested_disk'} ? $r{'nested_disk'} : 10000;
> > >
> > > This shouldn't hardcode 'nested' here, but use the guest ident. (nested
> > > is the wrong name now anyway I think, since it is nestedl1 in this
> > > iteration, which is why we avoid such hardcoding)
> > Right, we shall store this runvar with name of ${l1_ident}_disksize in
> > ts-nested-setup.
> 
> This is the sort of thing which needs to be done from make-flight, not
> ts-*.
> 
Agree.
> > > So you should arrange to do this somewhere that $gho is in scope and use
> > > guest_var($gho, 'disk', 10000). $gho is in scope in $prep where $disk_mb
> > > is used and AFAICT it is only used in that function, so I think you can
> > > move this to a local variable
> > yes, it is supposed to be local of prep(). However, look at other ts-*
> > (ts-debian-install, ts-freebsd-install, etc.), $disk_mb is defined
> > 'our'. They seems to be in alliance, are we going to it specifically in
> > ts-debian-hvm-install? or in some future day, we change these
> > ts-*-install in another patch series?
> 
> If/when the other ts-*-install need to support sizing the disk based on
> a runvar they would need similar changes. If you want to make those now
> for consistency then that would be great, but I don't think it needs to
> be mandatory.
I would prefer to leaving $disk_mb 'our' at present; to be consistent with other
ts-*-installs. 
Meanwhile do the overriding by runvar value in prepareguest().
Are you OK with this?
> 
> > To use guest_var(), it needs $gho in scope as you said. But in prep() we
> > can see until prepareguest() returns, we don't have $gho in scope, while
> > prepareguest() needs $disk_mb as input. So here we get into a loop.
> > I think we shall in prepareguest(), check if $r{${guest_ident}_disksize}
> > is defined, then override passed in $mb value. How would you like this?
> 
> Hrm, perhaps this behaviour ought to be pushed down into prepareguest
> itself, IOW the existing $mb argument to that function would become the
> default, but the runvar would take precedence f set? That would mean
> that all ts-*-install would gain support the runvar.
> 
> Ian, what do you think of this?
> 
> The alternative would be to use $r{${gn}_disksize}.
> 
> > >
> > > >      more_prepareguest_hvm($ho,$gho, $ram_mb, $disk_mb,
> > > >                            OnReboot => 'preserve',
> > > >                            Bios => $r{bios},
> > > > +                          ExtraConfig => $nestedhvm_feature,
> > > >                            PostImageHook => sub {
> > > >          my $cmds = iso_copy_content_from_image($gho, $newiso);
> > > >          $cmds .=
> prepare_initrd($initrddir,$newiso,$preseed_file_path);
> > > > @@ -174,7 +177,7 @@ my $ram_lots = 5000;
> > > >  if ($host_freemem_mb > $ram_lots * 2 + $ram_minslop) {
> > > >      $ram_mb = $ram_lots;
> > > >  } else {
> > > > -    $ram_mb = 768;
> > > > +    $ram_mb = $r{"${gn}_memory"} ? $r{"${gn}_memory"} : 768;
> > >
> > > guest_var again and I think this can be local to prep() too..
> > Also, agree to use guest_var() while may be still be global here as
> > other similar ts-*-install does, change them together in the future.
> 
> This should be handled the same way disk is, however that ends up being.
> 
> Ian.

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