Hi Dave, Thanks for the review.
On 27/04/2011 19:18, Dave Miner wrote: > On 04/22/11 01:25 PM, Darren Kenny wrote: >> Hi, >> >> I'd like to ask for help in reviewing the code for the moving of the Auto >> Installer client to the CUD architecture. >> >> Some light reading for the weekend ;) >> >> As Drew pointed out, we're adding less code than we're removing - that has >> to be >> positive, doesn't it?? >> >> I'm going to schedule a code review for 8am PDT/4pm IST on Tuesday 26th, I'll >> send out details closer to the date. >> >> The webrev is at: >> >> http://cr.opensolaris.org/~dkenny/cud_ai-to-slim/ >> > > > Overall looks quite good. Did not review tests, but let me know if you > need reviewers for those. > > Dave > > auto-install/__init__.py: start date on copyright should have stayed at 2008 > Fixed. > ai_get_manifest.py: > > 73,828,829: should really be /system/volatile now Fixed. > > ai_manifest.xml: > > 22: start date on copyright should have stayed at 2008 Fixed. > > 114: The old manifest was showing the combination of these two > properties applied together. The change doesn't seem the same. Does this read better? <!-- G3 --> <!-- <disk_keyword key="boot_disk"/> --> <!-- On X86 machines, Slices exist within partitions only --> <!-- Uncomment this to force AI to find an existing Solaris partition. --> <!-- <partition action="use_existing_solaris2"> <slice name="0"> <size val="20480mb"/> </slice> <slice name="4"> <size val="20480mb"/> </slice> </partition> or, use the following to create a Solaris partition --> <partition name="1" part_type="191"> <size start_sector="200" val="40960mb"/> <slice name="0"> <size val="20480mb"/> </slice> <slice name="4"> <size val="20480mb"/> </slice> </partition> <!-- Define some other partitions to create too --> <partition name="2" part_type="99"> <size start_sector="200" val="20480mb"/> </partition> <partition name="4" part_type="99"> <size start_sector="2000" val="20480mb"/> </partition> <!-- On SPARC systems, only specify the Slice layout. --> <!-- <slice name="0"> <size val="20480mb"/> </slice> <slice name="4"> <size val="20480mb"/> </slice> --> > > ai_sd.py, 161: s.var/run.system/volatile. Done. > > auto_install.py > > 150 and others [esp. determine_profile_type()]: Ethan's zones spec says > that -m is used for manifests, -p for SCI profiles. The usage here is > -p for manifests (the prior, and confusing usage/terminology). Perhaps > you can fix this now so that the QE tests converge sooner than later and > the terminology doesn't cause developers to create bugs based on a > misunderstanding of what profile means? We can change to using -m easy enough. Do you want us to add the -p option here? Right now SCI profiles are picked up from the location that manifest-locator downloads to. > > 272: It seems surprising that we have to do this special handling, as > well as the block at 395 in perform_autoinstall(). Would it make sense > to modify the derivation and parser checkpoints to deal with the fact > they may have nothing to do and let them just run in the normal > sequence? It would seem to simplify a lot of perform_autoinstall(), > though I realize you need to do something after parsing in order to > process the proxy and auto_reboot settings, but even that could > seemingly be a checkpoint. This would certainly appear do-able, we'll look into refactoring things. > > 315: If we're always going to send debug-level to the service log as > implied at 297, is there really a reason to have -v? If you're happy for us to always send debug information to the log, then we're happy to remove the -v option. If not, then we could easily toggle the level for the file based on the -v option between INFO and DEBUG. > > 437: I'm unclear how "creating Logical" is the right context in this > error message, or what that even means. Yep, I think that's a cut-n-paste error :-/ > > 480: It would be nice if we could set the dataset for the engine to take > snapshots of during the install process. This would seemingly be > helpful in debugging issues with all of the post-TI checkpoints. Do you mean to enable resuming of checkpoints? Or do you mean just to be able to access the snapshotted DOC at the time things broke? The engine does tend to snapshot to temporary files if there is no ZFS dataset, but it also tends to clean up after itself... > > 567: what's the use case for -d? Do we really need to bother? I'd prefer to fail if there was no manifest specified, similarly I think the -d option really isn't all that useful on it's own for a normal install. I would certainly prefer to remove it. > > 997: s/of/if/ 887? Done. > > 925ff: A lot of this seems like it might be more generally useful than > just for AI. Should we put factoring this out as a future work item? > parse_progress_msg seems like something that ought to come from > ProgressHandler. I'll log an RFE for this - I think that it would be useful to have most of this progress handler as part of the logging framework. It's not all that different to what DC does apart from the final output part. Although, one thing we've worked around too is to allow a progress server to also receive the other message levels - logging at the moment doesn't allow this... > > 1029: Receive Fixed. > > 1011: what's this magic number 524288? Ehm, don't really know, we inherited from $SRC/lib/install_logging_pymod/test/test_logger.py which was the only basis we had for a progress handler example. > > target_selection.py > > 24: nit, new file, should just be 2011 Fixed. > > 81: not sure what there is to do right here. Surprising that the > initial value isn't 0? We've changed this to use TargetController's minimum_target_size property now so we're consistent. > > 86: why not just use platform.processor()'s value directly? Agreed, changed. > > 251: In wording this error message and the many other similar ones that > occur in this module, I'd suggest a slight alteration to provide better > clarity (state the rule that's violated, rather than what was done in > the manifest), and I wouldn't use capitalization where that causes the > string to be different than the element or value used in the manifest > (this helps searching more complex manifests). So, maybe something like > "zpool '%s' must not be specified more than once", or (if you like some > caps at the beginning) "Must not specify more than one zpool named '%s'". We'll take a look at this. > > 501: The comment here is slightly incorrect, in that the noswap/nodump > specifications allow for there to be none of either one. Updated to be clearer. > > 547: "per root pool" Fixed. > > 685: I would think there are other cases where found_toplevel should be > set, but there isn't any other reference within the for loop that > started at 588. This if condition is run for every vdev in the zpool. I don't think there is any other place that we need to check it (admittedly it's not obvious unless you look closely at the indentation). > > 724: The "vdev redundancy" term seems a little obscure. It's not that obscure if you're looking at a manifest, where you would have <vdev name="vdev" redundancy="?????"/> it's possibly the toplevel term I would think is more obscure, but that's the terminology that ZFS uses to describe it. > > 795: "each" Fixed. > > 1088: "identifiable" Fixed. > > 1360: why not do this check against discovered_disk before copying? Fixed. > > 1671: Perhaps a better docstring Heh, yeah... Fixed. > > 1778: Does an error or warning get raised somewhere else in this case? The DTD validation should now fail if this occurs. I changed it so that it's not a loop anymore, but a simple if. > > usr/src/cmd/auto-install/svc/auto-installer: > > s./var/run./system/volatile.g999 Fixed. > > manifest-locator.xml, 42: If the only reason we had this dependency was > to get /tmp mounted, we should drop it, as /system/volatile is mounted > by the kernel before init even runs. Removed. > > utmpx.py > > 110: we definitely need to check this behavior when virtual consoles are > turned on, but my guess is that the right answer is to treat that as > nobody being logged in Agreed, so we only care if someone logs into the main console or VT/1, otherwise we can continue to log to the console. > > 120: For the present implementation it would seemingly be better to have > users_on_console() take an optional argument to turn on printing and > then you can move 127-131 and delete the rest Done. Thanks, Darren. _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
