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

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

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

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