From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Hu, Robert" Subject: Re: [PATCH OSSTEST 11/12] Changes on test step of debain hvm guest install Date: Sun, 15 Feb 2015 09:43:33 +0000 Message-ID: <9E79D1C9A97CFD4097BCE431828FDD31B18B38@SHSMSX103.ccr.corp.intel.com> References: <1423648341-203755-1-git-send-email-robert.hu@intel.com> <1423648341-203755-12-git-send-email-robert.hu@intel.com> <21724.60955.207370.14337@mariner.uk.xensource.com> <9E79D1C9A97CFD4097BCE431828FDD31B15D9E@SHSMSX103.ccr.corp.intel.com> <21725.59370.336561.485823@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <21725.59370.336561.485823@mariner.uk.xensource.com> Content-Language: en-US List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: "Pang, LongtaoX" , "jfehlig@suse.com" , "wei.liu2@citrix.com" , "ian.campbell@citrix.com" , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org > -----Original Message----- > From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com] > Sent: Friday, February 13, 2015 8:03 PM > To: Hu, Robert > Cc: xen-devel@lists.xen.org; jfehlig@suse.com; wei.liu2@citrix.com; > ian.campbell@citrix.com; Pang, LongtaoX > Subject: RE: [PATCH OSSTEST 11/12] Changes on test step of debain hvm guest > install > > Hu, Robert writes ("RE: [PATCH OSSTEST 11/12] Changes on test step of debain > hvm guest install"): > > [Ian] > > > Firstly, can you please reformat your commit message so that the > > > individual points are separated out into paragraphs. But I think > > > actually that probably some of this wants to go into different commits > > > (and perhaps different places). > > > > You mean dividing this patch into more pieces/commits? > > Yes. At least, that might be a good idea. I'm finding it a bit > difficult to see the wood for the trees. > > I can, however, offer some general guidelines on when to split/merge: > > * Changes must be in the same commit if they are mutually > interdependent, so that the code only works before all of them or > after all of them. > > * Each commit should be the smallest piece which is a coherent and > comprehensible alteration. (Although small changes might be put > together in the same commit, especially if they are conceptually > very similar and/or aren't textually interleaved.) > > * On ordering: it should not normally be necessary to read subsequent > commits to understand earlier ones. So generally that means that > new interfaces should be introduced (with any necessary doc > comments, etc.) earlier and used later. > > * It is a good idea to split out refactoring operations, and code > motion, each into their own patch. That makes it much easier to > review both the refactoring/motion (because it's much easier to > look for unintentional functional changes), and the other patches > (because they don't contain non-functional `noise'). When you do > this, the refactoring/motion should clearly say what if any > functional change is introduced. `No functional change' is the > usual stock phrase for cases when it's true. > > > > I think this should look more like: > > > > > > + run-ts . = ts-debian-hvm-install + host + nested > > > + run-ts . = ts-nested-setup + host + nested > > > + run-ts . = ts-xen-install nested > > > + run-ts . = ts-host-reboot nested > > > + run-ts . = ts-debian-hvm-install nested nested2 > > > > > OK. Since we could only try to learn your design arch/hierarchy of osstest, > > through code reading, such as terms of test job, test step, recipe, etc., > > we inevitably made some misunderstanding or unawareness. > For example, we don't understand the above why some (the first 2) passing params to ts-* with '+' and the latter 2 doesn't? > Indeed. That's fine. > > > Fortunately getting closer and closer to your mind now. > > I think so, yes. Of course if you think there is something in the > current design/architecture which is wrong or broken, then please do > say so. We're aware it's not perfect. > > Likewise, if something I suggest doesn't make sense or doesn't seem to > work well, please do feel free to contradict me. We're relying on > your judgement as well as mine :-). > > > Will follow your recipe composing above. > > So, yes, please, if you agree that this is a good way to go. I think > it will reduce the amount of nested-specific code elsewhere. > > > > I don't know why you need to use a dedicated VG for your nested > > > guests's L2 guests - please explain - but if you do, probably > > > ts-nested-setup could set it up. > > > > The existing ts-debian-hvm-install code presume host has vg set > > for guest installation. To make minimal code change, we'd like > > to imitate that presumption for L2's host. > > Strictly speaking, the existing ts-debian-hvm-install simply expects > that the host has a VG. The existing ts-host-install code (in > Debian.pm) sets up the VG (via preseed_create). So I think your > problem is that ts-debian-hvm-install doesn't set up the guest with > LVM for its filesystems ? Yes > > I think it would be better to unify the d-i partman-auto/expert_recipe > in Debian.pm with the one in ts-debian-hvm-install, and make all > Debian HVM installations use LVM. > > Wei, do you agree ? (TBH maybe I should have asked Wei to do that > when he introduced the new preseed setup in ts-debian-hvm-install.) Am I supposed to wait for Wei's patch or use my approach for a while and revert to Wei's patch afterwards? > > > > > @@ -63,7 +71,7 @@ d-i partman-auto/expert_recipe string \\ > > > > use_filesystem{ } filesystem{ vfat } \\ > > > > mountpoint{ /boot/efi } \\ > > > > . \\ > > > > - 5000 50 5000 ext4 \\ > > > > + 10000 50 10000 ext4 \\ > > > > > > I think this needs an explanation. You mentioned it in your commit > > > message but didn't give reasons. I think this should perhaps be done > > > in a different way. > > > > You mean not increase the size uniformly, but conditionally only for > > L1? > > I don't know. When you explain why this is necessary it will > hopefully become clearer to me what I think is the best way to address > the problem. In L1, a 'Debain-xxx-.iso' image will be stored there, therefore needs more capacity to host that. 5G size is definitely not enough. > > > > > +if ($nested eq 'nested_L2') { > > > > + target_cmd_root($gho, "init 0"); > > > > + target_await_down($gho,60); > > > > + target_ping_check_down($gho); > > > > +} > > > > +if ($nested eq 'nested_L1') { > > > > + store_runvar("L1_host", $gn); > > > > + store_runvar("L1_IP", $gho->{Ip}); > > > > + store_runvar("L0_Ident", $whhost); > > > > + target_cmd_root($gho, "mkdir -p /home/osstest/.ssh && cp > > > /root/.ssh/authorized_keys /home/osstest/.ssh/"); > > > > +} > > > > > > I don't understand the purpose behind these special cases. > > > > The first block is shut down L2 guest after proving it boots up. > > This should be done as a separate test step. I think you can use the > ts-guest-stop script. OK > > If you want to check that the guest can shut itself off then that > would be a new kind of test step (which could perhaps profitably added > to a wider variety of recipes). > > > Thanks, > Ian.