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

[Wei Liu <wei.liu2@citrix.com>]

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.

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

Wei.