Hi Alok,
Alok Aggarwal wrote:
> Hi Jan,
>
> On Wed, 16 Sep 2009, Jan Damborsky wrote:
>
>> Hey Alok,
>>
>> please see my comments below.
>> I reviewed all files, only those commented are mentioned.
>
> Thanks for doing the review.
You are welcome !
[...]
>> cmd/auto-install/svc/auto-installer
>> -----------------------------------
>> 75 - I think we could take advantage of devprop(1M) instead
>> of parsing prtconf(1M) output.
>
> I actually can't get devprop(1M) to give me any output
> on sparc or x86, I'll play with it some more since it
> seems much cleaner.
It seems to work correctly for me on x86 - e.g.
$ prtconf -vp | grep boot
bios-boot-device: '80'
$ devprop bios-boot-device
80
But it doesn't seem to work on Sparc:
# prtconf -vp | grep bootargs
bootargs: 00
# devprop bootargs
#
It seems like a bug, since man page doesn't mention that
devprop(1M) is supposed to work only on some platforms.
>
>> Speaking about implementation of 'shutdown' feature, why was different
>> approach taken comparing to 'auto-reboot' which is provided via AI
>> manifest ?
>> I think it might be confusing and it seems that is works only for x86 -
>> why is it envisioned to work in Sparc case ?
>
> The 'shutdown' feature was implemented like this for the
> VMC project. It turns out that if 'shutdown' is implemented
> as part of the AI manifest instead, the VMC project would have to
> modify the manifest contained within the bootable ai image.
> And, in doing that they would have to pick apart solaris.zlib
> and reassemble it.
>
> This wasn't acceptable to that project and that's why specifying
> it on the grub line was implemented.
Understood. Even if we don't want to deal with decomposing the solaris.zlib,
I can still see the solution. If my understanding is correct, we can specify
manifest location in GRUB ('aimanifest' option is being introduced).
Since VMC is going to decompose the image anyway to enable shutdown in GRUB
menu, it could change 'aimanifest' instead to point to VMC manifest
which would
be already part of the image - it might look like default with shutdown
enabled. I am actually not proposing to do any real changes at this
point, just
trying to understand the problem :-)
How is it envisioned to work for Sparc ? I understand LDOMs are out of scope
right now, but I assume they are to be considered later ?
[...]
>
>> cmd/auto-install/svc/manifest-locator
>> -------------------------------------
>>
>> 62 - we enforce to provide 'install' boot parameter if installation
>> is to be performed from media. If not specified, installation process
>> is not kicked off.
>> How is it going to work in 'net boot' case ? Don't we also want user
>> to explicitly confirm the installation by specifying
>> 'boot net:dhcp - install' ?
>
> We do also want the users to specify 'boot net:dhcp - install'
> to explicitly confirm the installation. And, we want something
> similar for x86 as well. This is what we've been referring to as
> the "accidental reboot" issue in AI.
>
> Fixing the "accidental reboot" in AI wasn't a requirement for
> the Bootable AI project however and that is why you don't see
> any changes to address that issue here.
I see - thank you for clarifying this.
[...]
>> lib/libtransfer/transfer_mod.py
>> -------------------------------
>>
>> why is timeout feature implemented for 'pkg image-create', but
>> not for other pkg operations, e.g. 'pkg install' ?
>
> The problem that the timeout feature was intending to solve
> was one of network configuration not being complete before
> the 'pkg image-create' call. And, thus, initial contact with
> the IPS repo would fail as well.
To tell the truth, I am not quite convinced about this solution,
as 'pkg image-create' might not be the first place where network
communication takes place - e.g. if manifest is to be obtained
from network. How is it guaranteed that things work correctly in
that case ?
I think the better approach would be to wait until network
is brought up. It seems nwam team might provide some advice
what and how to check for network state we are interested in -
we could pick up one from list of states they defined:
http://www.opensolaris.org/os/project/nwam/architecture/#state-machine
But since both are just interim solutions until nwam team delivers stable
API we could consume, I am fine with not changing the code at this point.
However, I might recommend to put comment there explaining what
the purpose of implementing timeout for 'pkg image-create' is
and what the plans are as far as final solution is concerned.
>
> I can see the argument for extending the timeout for all
> the other 'pkg' operations as well, it would probably be
> useful in cases where there are intermittent connectivity
> failures.
>
> I'm ambivalent about making this change at this point but
> if you or others feel strongly, I could make the change.
I think it might be better to let IPS layer take care
of this and tune its parameters to fit AI needs instead of
implementing another IPS timeout mechanism in AI.
Based on this, I am not suggesting to change other IPS commands
at this point, I am fine with RFE addressing this problem later
and maybe in different way.
Thank you,
Jan
