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

[Ian Jackson <Ian.Jackson@eu.citrix.com>]

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.

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 ?

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

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

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

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.