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. 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. 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? line 1761 Might be nice to set a constant for this above. "ISCSI_DEFAULT_PORT = 3260" or similar? line 1898 Same comment as previous 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. 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. 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? line 1970: Does whatever causes the slow-down scale? Meaning, if there are more LUNs, or more targets, is the 1s still sufficient? line 2005: It seems more natural to key this on LUN number, is there a reason to key on ctd? line 2045: Same comment about only these two discovery methods being used. Nice work, hope the comments help. /jb On Feb 15, 2012, at 1:30 PM, Drew Fisher wrote: > Good morning! > > Dermot and I would like to get a first-round code review for the following > PSARC/CRs: > > PSARC/2012/023 Interactive Installation to iSCSI > 6974246 Automated Install should provide mechanism for setting iSCSI > initiator-id-node > 7004719 Opensolaris LiveCD installation should give a GUI for installing > Solaris onto iSCSI LUN > 7004720 Opensolaris text installer should give a screen for installing > solaris onto iscsi lun > 7114789 unlabelled iSCSI drives and setting an iSCSI boot-device require > special handling on SPARC > 7121245 iscsi paths don't translate to bootable OBP paths/strings > 7132111 sample ai manifest file does not mention setting whole_disk in disk > target section > 7132457 Race condition in AI involving the target discovery and the check for > the new OS Device Name > 7145512 pulling iSCSI information from the DHCP server does not support iSCSI > boot > > https://cr.opensolaris.org/action/browse/caiman/drewfish/iSCSI/ > > Karen: Could I get you to look specifically at the text-installer changes? > Niall/John: Could I get you both to look specifically at the GUI changes? > Jesse: Can you look at code in $SRC/lib/install_target as that's where all > the calls to iscsiadm and libima.so exist > > Outstanding issues: > > - QE has not yet started a test cycle. I've been working with Angela Li on > this. I'm hoping to start a test cycle soon > - ai_manifest.4 is still being edited. I've emailed proposed changes to Alta > who will translate them into actual English > - The fix for 7132457 is slated to be tested by PIT soon for confirmation. > > ISO / USB access can be found at Hudson: > http://indiana-build.us.oracle.com/view/iSCSI/ Choose the job for the > installer you want to look at and you'll see links along the job's page > pointing to the latest ISO / USB image > > Screenshots of the GUI and Text-Installer (for those not interested in firing > up an entire ISO) can be found here: > http://mox.us.oracle.com/code/screenshots/gui/ > http://mox.us.oracle.com/code/screenshots/ti/ > > If people want to try out the ISOs, let me know and I can provide target > IP/LUN information to use. > > Thanks! > > -Drew and Dermot > _______________________________________________ > 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
