Hi Niall,
Thanks for the review...
> Hi Darren,
>
> target_selection was tough going ;-) I didn't cover unit tests and focused
primarily on auto_install.py
> and target_selection.py.
No worries I think you got the main bones of it, thanks.
>
> Here are my comments/questions:
>
> ai_instance.py
> --------------
>
> 26: It appears that ParsingError is an unused import in this module
True, while we didn't change it, I don't see harm in removing it... done.
>
>
> ai.dtd
> ------
>
> 43: Should a boot_mods elemement be included as an optional element here to
> support customisation of the boot menu of clients that get installed using
> AI.?
>
> ie.:
> 26: <!ENTITY % boot_mods SYSTEM "boot_mods.dtd">
> %boot_mods;
> 43: <!ELEMENT ai_instance (boot_mods?, target?, software+, add_drivers?,
(configuration*), source*)>
>
I think this would certainly be useful, but currently not in the DTD
specifications. Maybe this is something we could discuss adding now.
Could you maybe start a specific thread on caiman-discuss about it?
> Not sure if this is an actual requirement.
>
> 43: Why is the configuration* attribute within brackets? Are they still
> necessary?
While we're not using it in AI right now, it is something that Ethan will be
using when he pushes the AI/Zones work - hence it's still there...
>
> ai_manifest.xml
> ---------------
> In tandem with ai.dtd, should there be a commented boot_mods section?
If we update the DTD to support it, certainly.
>
> auto_install.py
> ---------------
> 165 & 169: Should the TI acronomym be substituted with "target instantiation"?
Done.
>
> Consistency nits:
> Some user messages end with periods while others don't...
> With: 173, 193, 205, 209, 214,
> Without: 158, 161, 165, 169, 177, 201
Removed all the periods.
>
>
> 271: Nit
> Remove space character betewen "order" and ":"...
> print "Checkpoints will be run in following order :"
> ->
> print "Checkpoints will be run in following order:"
>
Done (also added a 'the' to be 'the following...')
> 343:
> Inconsistent docstring formatting (''' -> """)
> '''If BE exists, then transfer Log Files to New BE'''
> ->
> """
> If BE exists, then transfer Log Files to New BE
> """
I've made it a little more consistent...
But, I believe the first line should be on the same as the ''' starting
point for correct formatting...
>
> 420-421: Nit
> Make double blank lines single for PEP8 compliance
Done - thought I had most of these...
>
> 437, 452: See 271
Changed.
>
> 465: Question: if http_proxy is unreachable is it fatal in every case
> and would it be useful/timesaving to attempt to see if http_proxy is
> reachable and bail out at this early point of execution before going
> through the destructive processes of target instantiation etc.?
I think that's not for us to check, IPS will fail later if it's not working...
>
> 496, 675, 676: See 271
Done.
>
> target_selection.py
> -------------------
>
> General:
> There seems to be an underlying assumption here that there can be more than
> one
> logical section (I see examples of walking lists of logicals in the code).
> As I understand it, there can only be one, even if multiple zpools are
> specified,
> but maybe I'm just mistunderstanding the purpose of the code :)
>
Well, it used to be the case that there could be, it's only recent that it
changed to be enforced by the DTD that there is only 1.
The value returned from get_children is a list, so it's not a huge leap to
have a loop around it for easy processing, but possibly we should remove these
now alright.
> Nit: Inconsistent docstring formatting for methods
> 84: What's wrong with just using 'i386'?
Changed these with Dave's and Drew's review too.
>
> 375-376:
> How can it be possible that new_zpool is None here? new_zpool is already
> copied at line 256 using copy.copy(zpool). And if that failed, there would
> be no reason to believe trying the same copy.copy() operation again would
> yield
> a different result.
I agree, removed it.
>
>
>
> 401: Which gate are you getting your TargetController module spec from? I
> can't see TC_DEFAULT_VDEV_NAME defined anywhere except int cud_text gate.
>
It *was* there, it's been recently renamed, I'll be posting a new webrev once
I've got through all the code review comments...
>
>
> 501-503:
> As best I can tell from dumpadm(1M) there can only be one configured system
> dump device, but the docstring suggests that it will create a dump device per
> pool. I guess that's slightly different than creating the zvol dump device
> though.
That's a doc error, it's only created in the root pool, so there would only
ever be one automatically generated dump device since there is only one
possible root pool.
This comment was updated recently.
>
> 579-585: Again I may be missing some context info here but as best I can
> gather
> this seems to be validation of the Target.Desired tree.
Yes it is validation - mainly we are checking ourselves, but also since TI is
(as is often quoted) "dumb" we need to be sure...
> 2 questions...
> 1. Is it possible that the zpool could be an existing pool whose action
> attribute could be "preserve" or "use_existing"? It seems that the
> assumption is that the pool is going to be created. In the case of the
> action
> being "create" does TI not destroy the existing pool and it's associated
> mountpoint if it exists already?
You are correct, and we've recently changed the validation to be more aware of
the "preserve" and "use_existing" actions, checking for example that if it's
marked as "use_existing" that it must be a root pool.
> 2. Why is "rpool" given special treatment (and hardcoded). I know that has
> traditionally been the default root pool name but I don't think that's ever
> been a hard rule AFAIK.
It's not any more, we had that so that when we were running tests on a machine
that already had a rpool things wouldn't fail. But we've since changed things
so that an automatically generated root pool won't just be called rpool, but
possibly rpoolN should there already be an existing rpool.
>
> 588: 'vdev' seems a bit misleading as a variable name since it can represent
> any one of vdev, zvol, filesystem, be, pool_options or dataset_options
Agreed, changed it to be 'child'.
>
>
> 886: Nit: "idenfifiable" -> "identifiable"
Fixed.
>
>
> 1140: This conditional block can be moved up above the rpool_dev_id
> construction
> on 1137, since it is independent of it.
>
Done.
>
> 1494: Are there really multiple logicals?
> From boot.dtd:
> <!ELEMENT target (disk*, logical?)>
All of vdev, be, etc. are logicals (the are all in logical.py).
But since you're not the first to say this, I've changed it to
'__create_temp_logical_tree'...
>
> 1502: 1509: See 401
See comment on 401 ;)
>
> 1555-1560, 1591-1594: See 1494
No, but there used to be, will look into removing these loops where not
necessary.
>
>
Thanks,
Darren.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
