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

2015-06-11 Thread Pang, LongtaoX 


 -Original Message-
 From: Ian Jackson [mailto:ian.jack...@eu.citrix.com]
 Sent: Wednesday, June 10, 2015 9:41 PM
 To: Pang, LongtaoX
 Cc: xen-devel@lists.xen.org; ian.campb...@citrix.com; wei.l...@citrix.com; Hu,
 Robert
 Subject: Re: [OSSTEST Nested PATCH v11 4/7] Changes on test step of Debian hvm
 guest install
 
 longtao.pang writes ([OSSTEST Nested PATCH v11 4/7] Changes on test step of
 Debian hvm guest install):
 ...
  1. The default disk size for guest is '1M' which is not sufficient
  for nested HVM guest, using larger disk size for nested guest
  to accommodate to nested test requirement, the specific disk_size is
  defined by make-flight.
  2. In L1 installation context, assign more memory (defined in runvar) to
  it; Since it acts as a nested hypervisor anyway.
  3. Comment out CDROM entry in sources.list to make HTTP URL entry
  available for L1 hvm guest.
  4. Enable nestedhvm feature in 'ExtraConfig' for nested job.
 
 I think this should be 3 separate patches:
 
   - Allow runvars to specify guest disk and ram size (turning
 previous values into defaults)
 
   - Comment out CDROM entry.  This needs better motivation.  I think
 your motivation is that the CDROM is removed and that therefore
 this line does not work ?
 
   - Honour $xopts{ExtraConfig} and use it to enable nestedhvm.
 
OK, we will try it.
 
  +my $extra_config='';
  +$extra_config .=nestedhvm=1\n
  +if guest_var($gho,enable_nestedhvm,'false') eq 'true';
 
 Idiom elsewhere is   =~ m/true/   rather than  eq 'true'
 although maybe Wei will provide a guest_var_boolean (see my comments
 on his [PATCH OSSTEST v3] Stubdom test case.
 
Yes, we will try it.
  +# Use guest_var to get specific disk size, or will use default $disk_mb
  +$disk_mb= guest_var($gho,'disksize',$disk_mb);
 
 I would prefer this comment and the one about RAM to be expressed,
 instead, like this:
 
 sub more_prepareguest_hvm (;@) {
 my ($ho, $gho, $ram_mb, $disk_mb, %xopts) = @_;
   + # $ram_mb and $disk_mb are defaults, used if runvars don't say
 
I am sorry, do you mean that I should add another more comment you preferred 
in 'more_prepareguest_hvm (;@)' function inside TestSupport.pm?
 
 Thanks,
 Ian.

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

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

2015-06-11 Thread Ian Jackson 
Pang, LongtaoX writes (RE: [OSSTEST Nested PATCH v11 4/7] Changes on test step 
of Debian hvm guest install):
 [Ian Jackson:]
  longtao.pang writes ([OSSTEST Nested PATCH v11 4/7] Changes on test step of
   +# Use guest_var to get specific disk size, or will use default 
   $disk_mb
   +$disk_mb= guest_var($gho,'disksize',$disk_mb);
  
  I would prefer this comment and the one about RAM to be expressed,
  instead, like this:
  
  sub more_prepareguest_hvm (;@) {
  my ($ho, $gho, $ram_mb, $disk_mb, %xopts) = @_;
+ # $ram_mb and $disk_mb are defaults, used if runvars don't say
  
 I am sorry, do you mean that I should add another more comment you preferred 
 in 'more_prepareguest_hvm (;@)' function inside TestSupport.pm?

I mean that you should add this comment instead of the ones next to
the assignments to $disk_mb and $ram_mb.

Interface comments (which in this case document a non-obvious feature
of the function's behaviour) are much better than code comments (which
in this case would simply say the same thing as the code).

Ian.

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

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

2015-06-10 Thread Ian Jackson 
longtao.pang writes ([OSSTEST Nested PATCH v11 4/7] Changes on test step of 
Debian hvm guest install):
...
 1. The default disk size for guest is '1M' which is not sufficient
 for nested HVM guest, using larger disk size for nested guest
 to accommodate to nested test requirement, the specific disk_size is
 defined by make-flight.
 2. In L1 installation context, assign more memory (defined in runvar) to
 it; Since it acts as a nested hypervisor anyway.
 3. Comment out CDROM entry in sources.list to make HTTP URL entry
 available for L1 hvm guest.
 4. Enable nestedhvm feature in 'ExtraConfig' for nested job.

I think this should be 3 separate patches:

  - Allow runvars to specify guest disk and ram size (turning
previous values into defaults)

  - Comment out CDROM entry.  This needs better motivation.  I think
your motivation is that the CDROM is removed and that therefore
this line does not work ?

  - Honour $xopts{ExtraConfig} and use it to enable nestedhvm.


 +my $extra_config='';
 +$extra_config .=nestedhvm=1\n
 +if guest_var($gho,enable_nestedhvm,'false') eq 'true';

Idiom elsewhere is   =~ m/true/   rather than  eq 'true'
although maybe Wei will provide a guest_var_boolean (see my comments
on his [PATCH OSSTEST v3] Stubdom test case.

 +# Use guest_var to get specific disk size, or will use default $disk_mb
 +$disk_mb= guest_var($gho,'disksize',$disk_mb);

I would prefer this comment and the one about RAM to be expressed,
instead, like this:

sub more_prepareguest_hvm (;@) {
my ($ho, $gho, $ram_mb, $disk_mb, %xopts) = @_;
  + # $ram_mb and $disk_mb are defaults, used if runvars don't say


Thanks,
Ian.

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

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

2015-06-09 Thread Ian Campbell 
On Tue, 2015-06-09 at 05:29 +, Pang, LongtaoX wrote:
 
 
  -Original Message-
  From: Ian Campbell [mailto:ian.campb...@citrix.com]
  Sent: Monday, June 08, 2015 6:31 PM
  To: Pang, LongtaoX
  Cc: xen-devel@lists.xen.org; ian.jack...@eu.citrix.com; 
  wei.l...@citrix.com; Hu,
  Robert
  Subject: Re: [OSSTEST Nested PATCH v11 4/7] Changes on test step of Debian 
  hvm
  guest install
  
  On Tue, 2015-05-26 at 17:08 +0800, longtao.pang wrote:
   1. The default disk size for guest is '1M' which is not sufficient
   for nested HVM guest, using larger disk size for nested guest
   to accommodate to nested test requirement, the specific disk_size is
   defined by make-flight.
   2. In L1 installation context, assign more memory (defined in runvar) to
   it; Since it acts as a nested hypervisor anyway.
   3. Comment out CDROM entry in sources.list to make HTTP URL entry
   available for L1 hvm guest.
   4. Enable nestedhvm feature in 'ExtraConfig' for nested job.
  
   Signed-off-by: longtao.pang longtaox.p...@intel.com
  
  Acked-by: Ian Campbell ian.campb...@citrix.com
  
  One query:
  [...]
   @@ -174,13 +185,18 @@ sub prep () {
if ($host_freemem_mb  $ram_lots * 2 + $ram_minslop) {
$ram_mb = $ram_lots;
} else {
   -$ram_mb = 768;
   +# Use guest_var to get specific memsize, or will use default 
   '768'
   +$ram_mb= guest_var($gho,'memsize',768);
  
  I think this only happens if the host has less than $ram_lots * 2 +
  $ram_minslop (==10100M) free, otherwise you get $ram_lots (5000M),
  which might be less than the runvar asked for...
  
 For nested job, the specific 'memsize' for L1 guest is 3072M which is defined 
 by make-flight.
 If the $ram_lots equal to 5000M which is larger than 3072M, that's suitable 
 for nested L1 guest.
 The condition for nested L1 guest's memory size that should not be less than 
 3072M, that's why we 
 add the code of $ram_mb =guest_var($gho,'memsize',768) here.

I was talking about the general case, which is broken for any guest
configured to have RAM  $ram_lots.


  Perhaps what we really want (maybe in a followup patch is):
  
  $ram_mb = guest_var($gho,'memsize',undef);
  if (!$ram_mb) {
   if ($host_freemem_mb  $ram_lots * 2 + $ram_minslop) {
$ram_mb = $ram_lots;
   } else {
$ram_mb = 768;
   }
  }
  ?
  
 So, I think maybe it's no need to change that.
 Please correct me, if I am wrong.



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

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

2015-06-09 Thread Pang, LongtaoX 


 -Original Message-
 From: Ian Campbell [mailto:ian.campb...@citrix.com]
 Sent: Tuesday, June 09, 2015 4:08 PM
 To: Pang, LongtaoX
 Cc: xen-devel@lists.xen.org; ian.jack...@eu.citrix.com; wei.l...@citrix.com; 
 Hu,
 Robert
 Subject: Re: [OSSTEST Nested PATCH v11 4/7] Changes on test step of Debian hvm
 guest install
 
 On Tue, 2015-06-09 at 05:29 +, Pang, LongtaoX wrote:
 
 
   -Original Message-
   From: Ian Campbell [mailto:ian.campb...@citrix.com]
   Sent: Monday, June 08, 2015 6:31 PM
   To: Pang, LongtaoX
   Cc: xen-devel@lists.xen.org; ian.jack...@eu.citrix.com; 
   wei.l...@citrix.com;
 Hu,
   Robert
   Subject: Re: [OSSTEST Nested PATCH v11 4/7] Changes on test step of Debian
 hvm
   guest install
  
   On Tue, 2015-05-26 at 17:08 +0800, longtao.pang wrote:
1. The default disk size for guest is '1M' which is not sufficient
for nested HVM guest, using larger disk size for nested guest
to accommodate to nested test requirement, the specific disk_size is
defined by make-flight.
2. In L1 installation context, assign more memory (defined in runvar) to
it; Since it acts as a nested hypervisor anyway.
3. Comment out CDROM entry in sources.list to make HTTP URL entry
available for L1 hvm guest.
4. Enable nestedhvm feature in 'ExtraConfig' for nested job.
   
Signed-off-by: longtao.pang longtaox.p...@intel.com
  
   Acked-by: Ian Campbell ian.campb...@citrix.com
  
   One query:
   [...]
@@ -174,13 +185,18 @@ sub prep () {
 if ($host_freemem_mb  $ram_lots * 2 + $ram_minslop) {
 $ram_mb = $ram_lots;
 } else {
-$ram_mb = 768;
+# Use guest_var to get specific memsize, or will use default 
'768'
+$ram_mb= guest_var($gho,'memsize',768);
  
   I think this only happens if the host has less than $ram_lots * 2 +
   $ram_minslop (==10100M) free, otherwise you get $ram_lots (5000M),
   which might be less than the runvar asked for...
  
  For nested job, the specific 'memsize' for L1 guest is 3072M which is 
  defined by
 make-flight.
  If the $ram_lots equal to 5000M which is larger than 3072M, that's 
  suitable
 for nested L1 guest.
  The condition for nested L1 guest's memory size that should not be less than
 3072M, that's why we
  add the code of $ram_mb =guest_var($gho,'memsize',768) here.
 
 I was talking about the general case, which is broken for any guest
 configured to have RAM  $ram_lots.
 
Thanks Ian, I get your point now. 
 
   Perhaps what we really want (maybe in a followup patch is):
  
   $ram_mb = guest_var($gho,'memsize',undef);
   if (!$ram_mb) {
if ($host_freemem_mb  $ram_lots * 2 + $ram_minslop) {
 $ram_mb = $ram_lots;
} else {
 $ram_mb = 768;
}
   }
   ?
  
  So, I think maybe it's no need to change that.
  Please correct me, if I am wrong.
 
Yes, your above patch is more suitable here and it's necessary to change the 
code.
Could you please change this query in your branch, so that I will not summit 
another version of nested patch to you?
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-06-08 Thread Pang, LongtaoX 


 -Original Message-
 From: Ian Campbell [mailto:ian.campb...@citrix.com]
 Sent: Monday, June 08, 2015 6:31 PM
 To: Pang, LongtaoX
 Cc: xen-devel@lists.xen.org; ian.jack...@eu.citrix.com; wei.l...@citrix.com; 
 Hu,
 Robert
 Subject: Re: [OSSTEST Nested PATCH v11 4/7] Changes on test step of Debian hvm
 guest install
 
 On Tue, 2015-05-26 at 17:08 +0800, longtao.pang wrote:
  1. The default disk size for guest is '1M' which is not sufficient
  for nested HVM guest, using larger disk size for nested guest
  to accommodate to nested test requirement, the specific disk_size is
  defined by make-flight.
  2. In L1 installation context, assign more memory (defined in runvar) to
  it; Since it acts as a nested hypervisor anyway.
  3. Comment out CDROM entry in sources.list to make HTTP URL entry
  available for L1 hvm guest.
  4. Enable nestedhvm feature in 'ExtraConfig' for nested job.
 
  Signed-off-by: longtao.pang longtaox.p...@intel.com
 
 Acked-by: Ian Campbell ian.campb...@citrix.com
 
 One query:
 [...]
  @@ -174,13 +185,18 @@ sub prep () {
   if ($host_freemem_mb  $ram_lots * 2 + $ram_minslop) {
   $ram_mb = $ram_lots;
   } else {
  -$ram_mb = 768;
  +# Use guest_var to get specific memsize, or will use default '768'
  +$ram_mb= guest_var($gho,'memsize',768);
 
 I think this only happens if the host has less than $ram_lots * 2 +
 $ram_minslop (==10100M) free, otherwise you get $ram_lots (5000M),
 which might be less than the runvar asked for...
 
For nested job, the specific 'memsize' for L1 guest is 3072M which is defined 
by make-flight.
If the $ram_lots equal to 5000M which is larger than 3072M, that's suitable 
for nested L1 guest.
The condition for nested L1 guest's memory size that should not be less than 
3072M, that's why we 
add the code of $ram_mb =guest_var($gho,'memsize',768) here.
 Perhaps what we really want (maybe in a followup patch is):
 
 $ram_mb = guest_var($gho,'memsize',undef);
 if (!$ram_mb) {
  if ($host_freemem_mb  $ram_lots * 2 + $ram_minslop) {
   $ram_mb = $ram_lots;
  } else {
   $ram_mb = 768;
  }
 }
 ?
 
So, I think maybe it's no need to change that.
Please correct me, if I am wrong.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-06-08 Thread Ian Campbell 
On Tue, 2015-05-26 at 17:08 +0800, longtao.pang wrote:
 1. The default disk size for guest is '1M' which is not sufficient
 for nested HVM guest, using larger disk size for nested guest
 to accommodate to nested test requirement, the specific disk_size is
 defined by make-flight.
 2. In L1 installation context, assign more memory (defined in runvar) to
 it; Since it acts as a nested hypervisor anyway.
 3. Comment out CDROM entry in sources.list to make HTTP URL entry
 available for L1 hvm guest.
 4. Enable nestedhvm feature in 'ExtraConfig' for nested job.
 
 Signed-off-by: longtao.pang longtaox.p...@intel.com

Acked-by: Ian Campbell ian.campb...@citrix.com

One query:
[...]
 @@ -174,13 +185,18 @@ sub prep () {
  if ($host_freemem_mb  $ram_lots * 2 + $ram_minslop) {
  $ram_mb = $ram_lots;
  } else {
 -$ram_mb = 768;
 +# Use guest_var to get specific memsize, or will use default '768'
 +$ram_mb= guest_var($gho,'memsize',768);

I think this only happens if the host has less than $ram_lots * 2 +
$ram_minslop (==10100M) free, otherwise you get $ram_lots (5000M),
which might be less than the runvar asked for...

Perhaps what we really want (maybe in a followup patch is):

$ram_mb = guest_var($gho,'memsize',undef);
if (!$ram_mb) {
 if ($host_freemem_mb  $ram_lots * 2 + $ram_minslop) {
  $ram_mb = $ram_lots;
 } else {
  $ram_mb = 768;
 }
}
?



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

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

2015-05-26 Thread longtao.pang 
1. The default disk size for guest is '1M' which is not sufficient
for nested HVM guest, using larger disk size for nested guest
to accommodate to nested test requirement, the specific disk_size is
defined by make-flight.
2. In L1 installation context, assign more memory (defined in runvar) to
it; Since it acts as a nested hypervisor anyway.
3. Comment out CDROM entry in sources.list to make HTTP URL entry
available for L1 hvm guest.
4. Enable nestedhvm feature in 'ExtraConfig' for nested job.

Signed-off-by: longtao.pang longtaox.p...@intel.com
---
Changes in v11:
1. Since the size of debian-7.2.0-amd64-CD-1.iso is just 624M, 
it's no need to extend nested L1's rootfs size, keep the original
size as '5000M'.
2. Add another use of preseed_hook_command to add new command of 
in-target sed -i 's/^deb *cdrom/#/g' /etc/apt/sources.list.
---
 ts-debian-hvm-install |   18 +-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/ts-debian-hvm-install b/ts-debian-hvm-install
index ea2d1ad..66ba5b0 100755
--- a/ts-debian-hvm-install
+++ b/ts-debian-hvm-install
@@ -92,6 +92,13 @@ if [ -e \$b/debian/grubx64.efi ] ; then
 fi
 END
 
+preseed_hook_command($gho, 'late_command', '', END);
+#!/bin/sh
+set -ex
+
+in-target sed -i 's/^deb *cdrom/#/g' /etc/apt/sources.list
+END
+
 $preseed_file .= preseed_hook_cmds();
 
 return $preseed_file;
@@ -166,6 +173,10 @@ sub prep () {
 target_putfilecontents_root_stash($ho, 10, preseed(),
   $preseed_file_path);
 
+my $extra_config='';
+$extra_config .=nestedhvm=1\n
+if guest_var($gho,enable_nestedhvm,'false') eq 'true';
+
 # If host has 8G free memory, create a guest with 4G memory to catch
 # any error that triggers cross 4G boundary
 my $host_freemem_mb = host_get_free_memory($ho);
@@ -174,13 +185,18 @@ sub prep () {
 if ($host_freemem_mb  $ram_lots * 2 + $ram_minslop) {
 $ram_mb = $ram_lots;
 } else {
-$ram_mb = 768;
+# Use guest_var to get specific memsize, or will use default '768'
+$ram_mb= guest_var($gho,'memsize',768);
 }
 logm(Host has $host_freemem_mb MB free memory, setting guest memory size 
to $ram_mb MB);
 
+# Use guest_var to get specific disk size, or will use default $disk_mb
+$disk_mb= guest_var($gho,'disksize',$disk_mb);
+
 more_prepareguest_hvm($ho,$gho, $ram_mb, $disk_mb,
   OnReboot = 'preserve',
   Bios = $r{bios},
+  ExtraConfig = $extra_config,
   PostImageHook = sub {
 my $cmds = iso_copy_content_from_image($gho, $newiso);
 $cmds .= prepare_initrd($initrddir,$newiso,$preseed_file_path);
-- 
1.7.10.4


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