On 3/7/11 10:17 AM, Keith Mitchell wrote:
Hi Alok,
As promised, here's my review of section (a), as well as one comment
about the shadow_xxx files
- 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.
Fixed. The leading "shadow_" has been removed from all the shadow files.
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)
I don't honestly know. I tend to use double-quotes for docstrings.
I'll talk with Alok and try to standardize.
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").
Fixed. td.py has been renamed discovery.py and ti.py has been renamed
instantiation.py
84: Since TD is one of the longer running checkpoints, I'm hoping this
method will get fleshed out.
Alok and I are at a loss on this one. Setting the progress_estimate
varies wildly due to the complexity and architecture of the system being
discovered. If you've got any ideas on this one, we're both all ears.
99-100: Could be combined with the block that starts at 118
Fixed.
118: May be worth moving this whole block to a separate function. May
not be. Up to you.
Moving the drive_media parsing out of this section doesn't really buy us
anything.
202-216: You have no idea how happy this makes me.
We aim to please. :D
251: search_name appears to be unused
Fixed. Added a check for it.
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!)
*all* of the Popen calls have been modernized with the iterations and
bug fixes you and I have done.
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.
Fixed to use splitlines(). One day I'll remember that function.
271-272 is no longer needed once splitlines is used.
276: Change to:
self.logger.debug("Populating DOC for zpool: %s", zpool_name) #
Comma, not %
Done.
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.
Fixed with option #2.
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.
Yes, users can set this to whatever they'd like. We're simply querrying
the system for whatever the mountpoint of the pool is and storing for
information purposes. I do not believe anything is currently using the
mountpoint of the zpool from the Target.DISCOVERED tre.
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.
Fixed.
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...
Fixed. Didn't know about rpartition().
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).
Fixed.
337: It's probably worth adding a "break" after you've finished
processing the match
Fixed.
376: Please add a comment here explaining why we want to skip the
"root" vdev_type.
Fixed.
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?
I went ahead an removed the slice so we're walking the entire the pool.
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"
Fixed.
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.
Fixed.
455: Is setup_iscsi() only ever going to be needed by td.py? Or would
it be worth having in a separate location?
520: If TD is run twice (or two separate TD checkpoints are run), will
setup_iscsi() behave appropriately?
Alok already answered this.
550: Is there any concern that, e.g., self.search_type might be
"some_random_string"?
Maybe, but I'm not too worried about it. discovery runs fairly quickly
(even on a t2000 with 3150 attached to it), so if the user searches for
something not "disk" or "zpool", it'll just discover the entire system.
We can look into tightening this if needed.
test_td.py:
test_td.py was mostly incomplete when Alok spun the webrev. It's been
updated to use solaris_install.Popen and cleaned up a little bit.
33: Nit: Import from solaris_install.
Fixed.
38-39: "from x import *" is generally frowned upon. Preferable to use,
for example, import solaris_install.target.logical as logical;
logical.my_func().
Fixed. For the most part I totally agree. With test code though, I
don't really care so much.
151-169: Enclose in a "def main():", and then add:
if __name__ == '__main__':
main()
Done.
vdevs.py:
General: What changes in the ZFS library do we need to be aware of to
prevent incompatibility in this module?
We're not accessing *any* libzfs structures with this code so the only
thing we need to watch for is any changes to the global open/close of a
libzfs handle, the open/close of a zpool_handle and modification of a 3
line function (zpool_get_config()) which returns an nvlist.
126: retrival -> retrieval
I spel gud.
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?
This function has been completely removed. There's no need to search
for exported zpools because libdiskmgt already does that for us in
discovery.py
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()]
Fixed.
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)
sectors and bytes are their own special look up.
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.
Done. They're locals now.
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.
Done.
86-93: Should this really be settable? (If so, then it doesn't need to
be a property yet)
Removed the get/set entirely. I thought we were going to need something
like this, but a comment made during code walkthrough of discovery.py
(set the blocksize directly from libdiskmgt when Size() objects are
created), made this unnecessary.
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...)
Done.
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")
Done.
The get() method should be case-insensitive.
Done.
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)
Once I find that giant bucket of time, I'll look into this. Since we're
not currently performing basic math operations on Size() objects,
there's really no need for it. It's a good idea though and something we
should consider.
test_zpool_vdevs.py:
27, 46: :comments:
Fixed.
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)
Fixed.
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
Fixed.
Thank you so very much, Keith!
-Drew
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
