Alok, Thanks for the review. Comments below...
On 01/26/10 15:00, Alok Aggarwal wrote: > > On Tue, 26 Jan 2010, Ethan Quach wrote: > >> Can I get two reviews for this. >> >> These fixes are going to change the way we require booting >> Sparc and X86 over the network to instantiate an automated >> install. Flag days will be sent out. >> >> Webrev: >> ------------ >> http://cr.opensolaris.org/~equach/webrev.14082.13766.4069/ >> >> Defects: >> ------------ >> http://defect.opensolaris.org/bz/show_bug.cgi?id=14082 >> http://defect.opensolaris.org/bz/show_bug.cgi?id=13766 >> http://defect.opensolaris.org/bz/show_bug.cgi?id=4069 > > auto-installer: line 124: Where is SMF_FMRI defined? > (I don't think it is). Also, do you want it to be > AI_SMF_FMRI instead? SMF_FMRI is set as the running method's service fmri. > > manifest-locator: lines 207-239: So "install prompt" > aren't positional any more? That is, "boot cdrom - prompt > install" would also have the same effect. yes > > manifest-locator: line 243: Remove the use of $HEAD, yields > the same results. for cases where the same bootarg is given more than once, and we're not expecting it to be, the processing becomes faulty. For example, if there existed boot properties "...,foo=bar,...,...,foo=bar" the value for 'foo' ends up being "bar bar". We're not expecting multiple instances of any of our boot properties so I figured taking the first instance is a better compromise, over errorring out. > > manifest-locator: line 272-274: This may be good to document > in the formal documents. I agree, how to do this should probably be noted in the faq or troubleshooting section of the AI guide. I'll add it there. > > manifest-locator: line 302: I trust you've reworked this piece > so the correct check happens for x86? I've turned the check for the ramdisk boot block device to be Sparc specific, and added an x86 specific check to look for the 'install_media' boot property. If that exists, its assumed to be a net boot. (I'm trying to see if there's a better way to determine that we've booted from the network on x86, but there doesn't really seem to be.) > > dc_defs.py: Why bother setting the do_safe_default keyword in > DC and propagate it via the .image_info file? Why not just > always create a safe default GRUB menu entry especially since > we don't provide a user visible way to tweak DC to not be > the case? So that there's still that separation from what decides when to do it. For example, if we want an 2010.03 Ai server to properly support 2009.06 Ai images we need to do it this way -- embed the decision in the image. The same reasoning going forward if future images decide to change things in some way I guess. On the DC end of things, I wasn't sure I'd wanted to make it a tunable or if it should be a tunable just to make the impl cleaner, and not reliant on the build# of the build system. Thoughts? > > installadm-common.sh: line 520, 524: s/automated/automated install ok > > installadm.1m.txt: Do we also want to document here how to change > the default GRUB menu entry to do a hands free install? I don't know if that's a good idea. We're trying to prevent something bad here, so I'd rather not note that in the manpage. AIs been behaving this way up til now, but that doesn't mean it should have been. IMO if someone really wants to do this, they'll figure it out. thanks, -ethan
