Jan,
I will review your code again after your Sue's code push.
jan damborsky wrote:
> Hi Sundar,
>
> thank you very much for your comments,
> please see my responses in line.
>
> Speaking about some logistic challenges, I was talking
> to Sue yesterday and it turned out that we will need
> to synchronize our pushes in some way as she is in the same
> code right now.
>
> The issue is that as part of my fix I implemented API
> in shell script part of installadm tools for looking up
> service database for information about service location.
>
> Sue yeasterday and you below pointed out that format of
> this database will be completely redesigned - thus this
> part of code will have to be reimplemented taking into
> account new format of database.
>
> I could push and let Sue merge with her changes, but if
> I understand correctly Jean depends on Sue's fix.
>
> As merging process and retesting would take some time,
> my push would mean additional delay for Jean's changes.
>
> Based on this, I might recommend I would wait with my
> push letting Sue integrate first and after that I would
> merge with her changes (it would basically mean to
> reimplement API for accessing service database), ask
> you for reviewing just that part (as you suggested below),
> retest and push.
>
> That could be done concurrently with what Jean is working on,
> since I think me and Jean don't depend on each other. Then I
> think delay should be minimized.
>
> Please let me know, what you think,
> Jan
>
>
>
> On 03/06/09 03:16, Sundar Yamunachari wrote:
>> Jan,
>> *
>> usr/src/cmd/auto-install/svc/auto-installer:*
>>
>> 73-87: Is it possible to have AI_SERVICE_ADDRESS with a valid value
>> and AI_SERVICE_NAME as NULL? If it is not valid, you can exit at 98.
>
> Thanks for pointing this out - this case is not valid - 'install_service'
> option is mandatory and should be always provided by AI server.
> I will exit at 98 with failure message and remove code at 104-109.
>
>> If it is possible, then 104-109 needs to be looked carefully. If the
>> AI_SERVICE_NAME is NULL, it will look for default service and it may
>> pick up a wrong service and will not look at AI_SERVICE_ADDRESS value.
>>
>> 120: Can you add the service name to the echo statement so that it
>> will tell what service we are looking for.
>
> Done.
>
>>
>> *usr/src/cmd/installadm/create-client.sh:*
>>
>> This code to find the txt-record will have to undergo changes because
>> service_data file will be replaced by either SMF property groups or
>> one file per service in the directory /var/installadm/services. I
>> will hold off reviewing this code till tomorrow.
>
> ok - please see my comment above.
>
>>
>> *usr/src/cmd/installadm/setup-sparc.sh:*
>>
>> 73: just a nit: Add a space before '='
>
> I think that space is not allowed in assignments, I have
> tried but it failed in that case.
>
>>
>> *usr/src/cmd/installadm/setup-tftp-links.sh*
>>
>> 46: You are setting SERVICE_ADDRESS here but it is not used in the
>> file. why is it needed?
>
> This mechanism is used for passing parameters to functions
> defined in installadm-common.sh - create_menu_lst_file
> in our particular case. I am not sure why this approach was
> originally taken, but seems like a good candidate for refactoring -
> it is confusing and error prone.
> Should I file bug for this or might there be desire for redesigning
> installadm tools and reimplement them in Python language - having thought
> about the challenges we will encounter in future with respect to discussed
> requirements (working in heterogeneous environment, need for abstracted
> API, being able to plug-in into different kind of implementations
> of consumed technologies), I would vote for this :-)
Please file a bug with this information. When we take up refactoring, we
will make sure that this gets fixed.
- Sundar
>
>>
>> Thanks,
>> Sundar
>>
>> jan damborsky wrote:
>>> Hi Sundar, Sue,
>>>
>>> could I please ask you for reviewing changes for following bug ?
>>>
>>> 6320 'No valid AI service found' error if AI client and server are
>>> on different subnets
>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=6320
>>>
>>> webrev:
>>> http://cr.opensolaris.org/~dambi/bug-6320/
>>>
>>> Other comments are also appreciated.
>>> Thank you very much,
>>> Jan
>>>
>>> modules affected:
>>> * AI image (/lib/svc/method/auto-installer)
>>> * installadm(1M) tools - no API changes, only internal ones
>>>
>>> Comparing to the short proposal sent for this fix, following
>>> modifications
>>> were introduced during the implementation:
>>>
>>> 'install_svc_address' option will be implemented instead of
>>> originally proposed 'install_service_address'
>>>
>>> The reason is that when old AI image was configured using new
>>> installadm(1M), AI client was originally confused by new option,
>>> since it was not able to distinguish between 'install_service'
>>> (used for service name) and 'install_service_address'.
>>> That would mean that installadm tools with fix for 6320 couldn't
>>> install older images and vice versa.
>>>
>>> While I was in that code, I also did additional changes:
>>>
>>> * clean up cstyle issues reported by 'hg pbchk'
>>>
>>> * check if the server hostname is resolved correctly
>>> (to something else than '127.0.0.1') when creating
>>> service . It serves as a stop gap before full check for
>>> correct network configuration is implemented.
>>> It saves time in situations when user creates service without
>>> verifying that resolver correctly resolves server hostname -
>>> user is informed and service is not created.
>>> It is helpful, since it was not straightforward to recover
>>> from that situation.
>>>
>>> Testing
>>> -------
>>> HW:
>>> * AI server - Laptop Lenovo X61, Virtual Box guest
>>> * router - Ultra 20
>>> * AI client -
>>> - x86: Ultra 20 (1GB RWM)
>>> - Sparc: T1000(8GB RWM)
>>> - Virtual Box guest
>>> - w2100z (2GB RWM) - connected to different subnet via router
>>>
>>> SW:
>>> * AI image created based on build 108 w/o the fix [img]
>>> * AI image created based on build 108 w/ the fix [img_6320]
>>> * installadm w/o the fix [ai_tools]
>>> * installadm w the fix [ai_tools_6320]
>>> * installation done from http://ipkg.sfbay/dev
>>>
>>> Test cases
>>> ----------
>>> [1] service created using [ai_tools], [img_6320] added
>>> # installadm create-service -s [img_6320] <path>
>>> - menu.lst didn't contain 'install_svc_address' option
>>> - AI client on the same subnet installed successfully
>>> - AI client on different subnet failed during service discovery
>>> - AI client set up as guest in Virtual Box failed during service
>>> discovery
>>>
>>> [2] service created using [ai_tools_6320], [img] added
>>> # installadm_6320 create-service -s [img] <path>
>>> - menu.lst contained 'install_svc_address' option
>>> - AI client on the same subnet installed successfully
>>> - AI client on different subnet failed during service discovery
>>> - AI client set up as guest in Virtual Box failed during service
>>> discovery
>>>
>>> [3] service created using [ai_tools_6320], [img_6320] added
>>> # installadm_6320 create-service -s [img_6320] <path>
>>> - AI client on the same subnet installed successfully
>>> - AI client on different subnet installed successfully
>>> - AI client set up as guest in Virtual Box installed successfully
>>>
>>>
>>> step [3] was also tested in following scenarios
>>> [A] service already existed, only image was added
>>> # installadm_6320 list
>>> _install_service_46501
>>> # installadm_6320 create-service -n _install_service_46501 -s
>>> [img_6320] <path>
>>>
>>> [B] client was added
>>> # installadm_6320 create-client -e <mac_address> -n
>>> _install_service_46501 -t <img_6320_path>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://mail.opensolaris.org/pipermail/caiman-discuss/attachments/20090309/5ace06fe/attachment.html>
