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

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

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@intel.com>
> > > ---
> > > 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-*.

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

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