Hi Darren,
target_selection was tough going ;-) I didn't cover unit tests and focused
primarily on auto_install.py
and target_selection.py.
Here are my comments/questions:
ai_instance.py
--------------
26: It appears that ParsingError is an unused import in this module
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*)>
Not sure if this is an actual requirement.
43: Why is the configuration* attribute within brackets? Are they still
necessary?
ai_manifest.xml
---------------
In tandem with ai.dtd, should there be a commented boot_mods section?
auto_install.py
---------------
165 & 169: Should the TI acronomym be substituted with "target instantiation"?
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
271: Nit
Remove space character betewen "order" and ":"...
print "Checkpoints will be run in following order :"
->
print "Checkpoints will be run in following order:"
343:
Inconsistent docstring formatting (''' -> """)
'''If BE exists, then transfer Log Files to New BE'''
->
"""
If BE exists, then transfer Log Files to New BE
"""
420-421: Nit
Make double blank lines single for PEP8 compliance
437, 452: See 271
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.?
496, 675, 676: See 271
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 :)
Nit: Inconsistent docstring formatting for methods
84: What's wrong with just using 'i386'?
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.
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.
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.
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.
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?
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.
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
886: Nit: "idenfifiable" -> "identifiable"
1140: This conditional block can be moved up above the rpool_dev_id construction
on 1137, since it is independent of it.
1494: Are there really multiple logicals?
>From boot.dtd:
<!ELEMENT target (disk*, logical?)>
1502: 1509: See 401
1555-1560, 1591-1594: See 1494
Cheers,
Niall
--
This message posted from opensolaris.org
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
