Jesse,
Thanks for the review! Comments in-line...
On 2/20/12 6:04 PM, Jesse Butler wrote:
This looks really good, Drew. Some we've discussed offline, but I have
some comments for you regarding the library code (which is all I've
really dug into).
usr/src/lib/install_target/discovery.py
line 219
You are checking here for specifically COMSTAR LUNs, but I'm thinking
we probably intend to work with all Solaris-supported iSCSI targets.
This is presumably for the install applications to determine if a LUN
is iSCSI or not. If that's the case, you might want to instead walk
through all your disks once they are enumerated and compare to the
output of "iscsiadm list-target -v" or something, to then more
accurately set dev_type.
Fixed:
# check for any Iscsi object in the DOC. If found, change the
# dev_type from 'scsi' to 'iSCSI' to allow the installers finer
# granularity when choosing these types of LUNs
iscsi_check = self.doc.get_descendants(class_type=Iscsi)
if iscsi_check:
cmd = [ISCSIADM, "list", "target", "-S"]
p = run(cmd)
if new_disk.ctd in p.stdout:
new_disk.disk_prop.dev_type = "iSCSI"
else:
new_disk.disk_prop.dev_type = \
drive.controllers[0].attributes.type
Does that look ok to you?
usr/src/lib/install_target/libdiskmgt/cfunc.py
line 37
This could be dumb, but... I'm not seeing a dm_cache_update() in
libdiskmgt?
usr/src/lib/install_target/libdiskmgt/const.py
line 566
And now I see what are likely sysevent values, and this reminds me
that you're waiting on some work in libdiskmgt, right? So, probably
what's up with previous comment as well.
usr/src/lib/install_target/libdiskmgt/diskmgt.py
line 568
And now I see what looks like the rest of the context for last two
comments. I'm not sure this looks right...
**Ok - just caught up with you and got some confirmation of what this
looked like to me, and told you I'd put my response in here.
So, this mechanism allows the lower levels (libdiskmgt) to inform you
via sysevent that a new iSCSI LUN is in play, at which point you can
refresh the cache and requery. From the install code. This just feels
wrong.
Basically, this is all in place to ensure you see all the disks
possible at a given point in time. Two problems - libdiskmgt's cache
can go out-of-sync, and libdiskmgt has no polling capability so you
can determine if a sync is happening or if otherwise the library is in
flux. To me, both of these are issues with libdiskmgt, not its
consumers. When disk topology changes, the library should update its
own cache. Further, it should have a pollable value which indicates if
it's current state is valid or not.
That said, I know getting someone to own that and address it might be
difficult. But, I feel like I have to say it, at least.
I could say something here but a slow nod of the head is really all
that's needed.
usr/src/lib/install_target/physical.py
line 991
Not entirely sure what this is, but if it's just for internal
bookkeeping, why do we care if upper or lower?
This method is used by the GUI for the mouseover() calls for the disk
icons. I'm not sure on why it's upper()'d though. Dermot?
line 1761
Might be nice to set a constant for this above. "ISCSI_DEFAULT_PORT =
3260" or similar?
Fixed.
line 1898
Same comment as previous
Fixed.
line 1903
Why a timeout of 15? Seems also a bit long-ish. Guessing something
like 5 should be plenty to get a connection open.
Fixed.
line 1909
Mega-nit: s/Iscsi/iSCSI/
:)
line 1934:
This block sets up either static discovery or send targets. Do we
intend to support iSNS? Might be worth tossing a comment in here
saying we only support these two modes of discovery.
I'll add a comment as we're not supporting iSNS ... yet.
lines 1945 and 1959:
In the first list, the static-discovery case, you'll only list target
LUNs related to the particular iSCSI target described by your static
config. In the second list, the send targets case, you'll be listing
*all* targets, and all LUNs from each. This is probably pedantic,
since we're talking about a boot environment here where you've only
set up a single target for install, but it seems to me that you'll
either want all, or just the select target, and the decision shouldn't
be based upon discovery mode.
Possibly in the future, one may wish to have 2 Targets, a LUN from
each, to install to a mirrored pool. For example...
line 1962:
Just confirming - when called in this manner, this call will block, right?
Yes. devfsadm blocks.
line 1970:
Does whatever causes the slow-down scale? Meaning, if there are more
LUNs, or more targets, is the 1s still sufficient?
I've had PIT test this out and things seem to be working right. I have
no idea what causes the issue (The CR is 7132457 Race condition in AI
involving the target discovery and the check for the new OS Device Name)
line 2005:
It seems more natural to key this on LUN number, is there a reason to
key on ctd?
Since the parent object in the DOC is a disk object, we want to key on
CTD so we can set the parent Disk object's CTD attribute in the case of AI.
line 2045:
Same comment about only these two discovery methods being used.
I added a comment regarding iSNS.
Thanks again for the review!
-Drew
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
