Hi Keith,
On Mon, 7 Mar 2011, Keith Mitchell wrote:
Hi Alok,
As promised, here's my review of section (a), as well as one comment about
the shadow_xxx files
Thanks so much for your timely review! I'll respond to
a few of your comments, Drew will pick up the rest.
- Keith
General:
Shadow* files comment:
The files are placed in:
.../vendor-packages/solaris_install/target/shadow/shadow_<something>.py
I think that should be
.../vendor-packages/solaris_install/target/shadow/<something>.py
The extra "shadow" component is redundant, especially when importing.
Agreed.
Nit: Are we using triple single-quotes or triple double-quotes for
docstrings? I think most of our code leans toward the former - consistency
would be nice (though I don't think we've had a 'formal' decision)
td.py:
Is there any reason not to name this something less terse? Perhaps
"discovery.py" (so the import path becomes
"solaris_install.target.discovery").
Agreed, similarly 'ti' can more descriptive if named
'instantiation'.
84: Since TD is one of the longer running checkpoints, I'm hoping this method
will get fleshed out.
This is actually a non-trivial problem to solve especially
in the case of TD. Given that discovery has no idea of how
many devices are connected to a given system, it's not obvious
to me yet how to either come up with a hard coded value or
a dynamically computed progress estimate.
99-100: Could be combined with the block that starts at 118
118: May be worth moving this whole block to a separate function. May not be.
Up to you.
202-216: You have no idea how happy this makes me.
I share your happiness, we lucked out a bit in that we don't
have to deal with upgrades from previous releases. The removal
of that constraint made this easier.
251: search_name appears to be unused
267: Popen call looks out of date - should be Popen.check_call()
I see a few other places in need of updating as well. (Apologies for changing
this syntax out from under you guys, but I promise it was for the best,
long-term!)
270: Could use "p.stdout.splitlines()"
271-272: I'm surprised this is necessary, though hopefully it doesn't come up
enough to be an issue.
276: Change to:
self.logger.debug("Populating DOC for zpool: %s", zpool_name) # Comma, not %
291: You could (outside the 'for' loop) run:
# zpool list -H -o name,bootfs
And then store the output for use in when looking at each zpool. Or you can
give it a pool name:
# zpool list -H -o bootfs <pool>
And then you just have the value - that route might be more stable.
300: Can you clarify what this value is used for? I'm concerned that, since
data pools can set this to arbitrary values, if we're using it as anything
more than information for the user we could end up with problems.
308: At this point in the code, we start getting pretty deep indentation
levels. Please break some of this out into separate function calls for
readability.
318: some_str.rpartition() will always return a tuple of length 3 with the
leading "/dev/dsk" removed (note that's Rpartition, not partition), so 317
isn't needed. This is particularly valuable given that line 321 assumes that
"entry" has been set to something...
321, 328: You should probably use rpartition for these (you likely want to
ensure that we're partitioning this string from the right, not the left).
337: It's probably worth adding a "break" after you've finished processing
the match
376: Please add a comment here explaining why we want to skip the "root"
vdev_type.
384-385: The first dataset is the root dataset, not the pool. It just happens
to require the same name as the pool, but it can be mounted, used and have
properties set just like any other dataset, as far as I understand. Should we
really skip it? If it does need to be skipped, should you use the "-s" (sort)
flag to the zfs list call to ensure that the sorting is exactly how we want
it?
397-398: Comment could be clearer: Explain that the output from zfs list
returns values like "37G" and "3.2M" and that Size() expects "37GB" and
"3.2MB"
426: Nit: The first portion of the comparison ("if zpool_name") is redundant.
The second comparison will end up failing properly whether zpool_name is
None, an empty string, or a different pool name.
455: Is setup_iscsi() only ever going to be needed by td.py? Or would it be
worth having in a separate location?
I don't see a use yet for this method outside of td.py.
520: If TD is run twice (or two separate TD checkpoints are run), will
setup_iscsi() behave appropriately?
We'll just end up re-running the iscsi setup commands twice,
it should just work (I've tried on the command line and it
infact does work).
550: Is there any concern that, e.g., self.search_type might be
"some_random_string"?
test_td.py:
33: Nit: Import from solaris_install.
38-39: "from x import *" is generally frowned upon. Preferable to use, for
example, import solaris_install.target.logical as logical; logical.my_func().
151-169: Enclose in a "def main():", and then add:
if __name__ == '__main__':
main()
vdevs.py:
General: What changes in the ZFS library do we need to be aware of to prevent
incompatibility in this module?
126: retrival -> retrieval
141: I'm confused by this comment. The iteration is over nvlists, not the
pairs within those lists, right? If the nvlist names are non-unique, doesn't
the assignment on 159 override any previous assignments from nvlists with the
same name?
size.py:
General: The "if/elif/elif/..." blocks could be condensed with a dictionary
that maps unit strings to unit values. e.g.:
units = { "kb" : 2 ** 10,
2 ** 10 : "kb",
"mb" : 2 ** 20,
2 ** 20 : "mb"}
# Mappings in both directions was intentional
self.byte_value = self.value * units[self.suffix.lower()]
Sectors would still need to be handled specially though (since you'd want
probably want the units dict to be at the class level, and sectors is an
instance variable)
60, 63: I don't see much value in storing the value/suffix parts as
attributes - would recommend using local vars and ensuring everything uses
self.byte_value.
65: Yes, we care. Would rather raise a ValueError here than have an odd
AttributeError on line 69 when we try to access self.suffix.
86-93: Should this really be settable? (If so, then it doesn't need to be a
property yet)
117: __repr__ should return a string that could be pasted into a Python
interpreter to recreate the object, e.g.:
repr(my_size)
Size("456b")
For precision, it should probably also always use the "bytes" value (which
shouldn't be cast to a float...)
However, the current __repr__ function would make a fine __str__ function
(although both functions should use units consistent with what it accepts,
i.e., "TB" instead of "T")
The get() method should be case-insensitive.
Lots of room for enhancements to this class: subclass long instead of object,
include methods for __add__, __sub__, etc. (I'm sure none of those are
strictly necessary, but at the time I'm writing this, I'm just kicking things
around on a Friday afternoon)
test_zpool_vdevs.py:
27, 46: :comments:
setUp/tearDown: See usr/src/lib/install_engine/test/engine_test_utils.py for
helper functions for tests that need to create/destroy/reset the
InstallEngine singleton. (Drew may have already addressed this)
122, 123: Use self.assertEquals(). Also, a subtle "nit" - reverse the order.
(When the test fails, it's more convenient to see "Test failure:
<vdev_map_root object> != <f1 object>")
This comment also applies to 165-166, 217-218, and 224-229
On 03/ 4/11 01:11 PM, Alok Aggarwal wrote:
Just a reminder to those reviewing that the comments
are due by March 10. If you need more time, please let
me know.
Thanks,
Alok
On Thu, 24 Feb 2011, Alok Aggarwal wrote:
We would like volunteers to review the TI/TD project. The
review is being broken down into the following chunks to
aid the review process.
a) Target discovery
b) Target Instantiation
c) Target controller API and target validation
d) ctypes interfaces
e) Other
Webrev location:
http://cr.opensolaris.org/~aalok/cud_ti/
Please let us know which of the above sections you'd like to
sign up for, an approximate list of files corresponding to each of the
review sections is included below for your convenience.
We would like your comments back by March 10.
Thanks,
Drew, Alok (and Jean)
(a) usr/src/lib/install_target/td.py
usr/src/lib/install_target/test_td.py
usr/src/lib/install_target/vdevs.py
usr/src/lib/install_target/size.py
usr/src/lib/install_target/test/test_zpool_vdevs.py
b) usr/src/lib/install_target/ti.py
usr/src/lib/install_target/physical.py
usr/src/lib/install_target/logical.py
usr/src/lib/install_target/test/test_target_instantiation.py
usr/src/lib/install_target/test/ti_full.py
c) usr/src/lib/install_target/shadow/*
usr/src/lib/install_target/test/test_shadow_list.py
d) usr/src/lib/install_target/libadm/*
usr/src/lib/install_target/libbe/*
usr/src/lib/install_target/libdevinfo/*
usr/src/lib/install_target/libdiskmgt/*
usr/src/lib/install_target/libnvpair/*
e) Makefiles, DC changes, MP test changes, engine and
errorsvc changes, packaging
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
