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: Tue, 17 Feb 2015 00:45:34 +0000 Message-ID: <9E79D1C9A97CFD4097BCE431828FDD31B1AB22@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> <9E79D1C9A97CFD4097BCE431828FDD31B18B38@SHSMSX103.ccr.corp.intel.com> <20150216101636.GA12471@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150216101636.GA12471@zion.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: Wei Liu Cc: "Pang, LongtaoX" , "jfehlig@suse.com" , Ian Jackson , "ian.campbell@citrix.com" , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org > -----Original Message----- > From: Wei Liu [mailto:wei.liu2@citrix.com] > Sent: Monday, February 16, 2015 6:17 PM > To: Hu, Robert > Cc: Ian Jackson; 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 > > On Sun, Feb 15, 2015 at 09:43:33AM +0000, Hu, Robert wrote: > > > -----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? > > sg-run-job is written in tcl. I don't claim to be an expert on this > language. I spent ~1.5 hour to grasp the basic and was able to decrypt > this script. The manual and tutorial on tcl official website are quite > helpful. > > All those parameters are passed to the ts-* script. The "+" sign is a > toggle to control if it should appear in the final report. Have a look > at spawn-ts, or more preciously, around L123 (it may vary depending on > your OSSTest tree changeset). > > If you look at the actual output of you script, you will see all > parameters are passed to the script. Thanks for your help! I have also spent days on the tcl basic study half years ago. Since didn't find '+' as a tcl primitive, got confused. > > > > 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.) > > Yes. I agree we should use LVM whenever possible. Sorry for not having > done that... > > One thing to keep in mind though, EFI boot requires a vfat partition. > > > Am I supposed to wait for Wei's patch or use my approach for a while and > > revert to Wei's patch afterwards? > > What patch do you expect from me? That Ian mentioned above '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.