Re: [Xen-devel] [PATCH OSSTEST 11/12] Changes on test step of debain hvm guest install

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

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@xxxxxxxxxx]
> Sent: Monday, February 16, 2015 6:17 PM
> To: Hu, Robert
> Cc: Ian Jackson; xen-devel@xxxxxxxxxxxxx; jfehlig@xxxxxxxx;
> wei.liu2@xxxxxxxxxx; ian.campbell@xxxxxxxxxx; 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@xxxxxxxxxxxxx]
> > > Sent: Friday, February 13, 2015 8:03 PM
> > > To: Hu, Robert
> > > Cc: xen-devel@xxxxxxxxxxxxx; jfehlig@xxxxxxxx; wei.liu2@xxxxxxxxxx;
> > > ian.campbell@xxxxxxxxxx; 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.

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