Niall,
Response below.
On 02/21/12 00:59, Niall Power wrote:
Hi Dermot,
More responses below
On 20/02/2012 22:16, Dermot McCluskey wrote:
Hi Niall,
Thanks for reviewing. Responses below.
On 02/20/12 03:56, Niall Power wrote:
Hi Drew & Dermot,
Here are my comments for the GUI changes. I'll try to squeeze in
time to look at the target libraries also.
usr/src/cmd/gui-install/aux/INSTALL_DISK_DISCOVERY_PANEL.txt:
-------------------------------------------------------------
15: Ultra Nit: s/iSCSI standard/iSCSI protocol
ok
19: Nit: 'Local disks' is the quotation referenced, however the GUI
contains "Local Disks" d->D
ok
usr/src/cmd/gui-install/src/disk_screen.py:
-------------------------------------------
604: Will "disk.disk_prop.dev_type != iSCSI" guarantee that the disk
is local? How would this cope with a fibre channel disk?
Prior to this project, all discovered disks fall into what we
are calling "local" disks. Now Drew has modified discovery.py
to mark "COMSTAR" disks as dev_type="iSCSI", so that is
how we are distinguishing local vs non-local disks. If FCoE
happens, then we'll presumably need to some more support
in discovery.py and GUI will need to be aware of this.
Fair enough :)
601 & 606: It might be useful to state what disks are being filtered
*out* also. Users are likely to look for answers to why they can't
see the disk they want to install to.
eg.
599 disk.disk_prop.dev_type == "iSCSI":
600 filtered_disks.append(disk)
601 LOGGER.debug("Filter *in* disk: %s", disk.ctd)
continue
602 if profile.show_local and \
603 (disk.disk_prop is None or \
604 disk.disk_prop.dev_type != "iSCSI"):
605 filtered_disks.append(disk)
606 LOGGER.debug("Filter *in* disk: %s", disk.ctd)
continue
Logger.debug("Filter *out* disk: %s, disk.ctd)
Yes - that's good. Also, I think the term "filter" is
wrong here, so I've changed this block to:
matching_disks = list()
for disk in self._td_disks:
if profile.show_iscsi and \
disk.disk_prop is not None and \
disk.disk_prop.dev_type == "iSCSI":
matching_disks.append(disk)
LOGGER.debug("Including disk: %s", disk.ctd)
continue
if profile.show_local and \
(disk.disk_prop is None or \
disk.disk_prop.dev_type != "iSCSI"):
matching_disks.append(disk)
LOGGER.debug("Including disk: %s", disk.ctd)
continue
LOGGER.debug("Excluding disk: %s", disk.ctd)
self._td_disks = matching_disks
848 - 857: Why is it now hardcoding icons into the app instead of
looking them up from the icon theme as before?
This breaks both theming and accesibility. Icon changes or new
icons should go back into the Nimbus theme.
I'll pass this on on to Steve Wolf - he specifically wanted me
to include these new icons with GUI.
re: accessibility, though: there isn't a gnome-dev-harddisk icon
in either High Contrast or Large Print themes, so using a hardcoded
icon would not prevent those themes accessing a better icon,
as things currently stand.
Agreed. Although we should fulfill our end of the arrangement ;)
I've talked to Erwann (who wrote Nimbus) and Calum (Desktop
HCI guy, for those that don't know him) about this.
Erwann suggests it's not worth putting this in a theme, as it's
too specific to this app and there is little benefit in doing so.
Calum says HiColor theme, and possibly application-specific
HiColor, is, if anywhere, the correct theme to put this in, but
he wasn't very adamant about it, especially as we only have
1 size for the icons.
Accessibility would be the only beneficiary of this change, but
in fact, the a11y themes don't supply these icons, so there
would be no benefit in practice.
Also, this would create another Desktop patch to maintain,
as these changes are unlikely to go upstream.
So, would you still like me to move the icons into a theme?
If yes, how about, to allow the project to push now, I leave
code as-is for now; open an RFE to get the icons into GNOME;
and switch to using proper theme icon once that integrates?
- Dermot
usr/src/cmd/gui-install/src/gui_install_common.py:
--------------------------------------------------
292 & 293 Nit: s/Iscsi/iSCSI
This is a reference to the "Iscsi" class defined in physical.py,
so I think the capitalization is correct in that context.
My bad.
47 & 296: cache_update() It looks wrong to me for the GUI (or
other client) to be trying to manipulate libdiskmgt, which is just
an implementation detail of target_discovery. It sorta violates that
whole MVC thing :) Why can't target discovery take care of this
itself when doing iSCSI discovery?
Separately discussed with Drew.
356 & 400: Why does the checkpoint error no longer raise an
exception? It used to raise RunTimeError
I don't follow. Which previous raising of RuntimeError are you
referring to?
Ooops, sorry. I got confused with a logger.error() statement. Please
ignore me.
Thanks
Niall
- Dermot
Cheers!
Niall
On 16/02/2012 05:30, 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
<http://psarc.us.oracle.com/Archives/CaseLog/arc/PSARC/2012/023> Interactive
Installation to iSCSI
6974246 <http://monaco.us.oracle.com/detail.jsf?cr=6974246> Automated Install
should provide mechanism for setting iSCSI initiator-id-node
7004719 <http://monaco.us.oracle.com/detail.jsf?cr=7004719> Opensolaris
LiveCD installation should give a GUI for installing Solaris onto iSCSI LUN
7004720 <http://monaco.us.oracle.com/detail.jsf?cr=7004720> Opensolaris text
installer should give a screen for installing solaris onto iscsi lun
7114789 <http://monaco.us.oracle.com/detail.jsf?cr=7114789> unlabelled iSCSI
drives and setting an iSCSI boot-device require special handling on SPARC
7121245 <http://monaco.us.oracle.com/detail.jsf?cr=7121245> iscsi paths don't
translate to bootable OBP paths/strings
7132111 <http://monaco.us.oracle.com/detail.jsf?cr=7132111> sample ai
manifest file does not mention setting whole_disk in disk target section
7132457 <http://monaco.us.oracle.com/detail.jsf?cr=7132457> Race condition in
AI involving the target discovery and the check for the new OS Device Name
7145512 <http://monaco.us.oracle.com/detail.jsf?cr=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
