Jan,
It looks good now.
Thanks,
Sundar
jan damborsky wrote:
> Hi Sundar,
>
> thank you very much for your comments.
> Please see my response in-line.
>
> I have updated the webrev accordingly:
> http://cr.opensolaris.org/~dambi/bug-6320-merged/
>
> Jan
>
>
> On 03/10/09 07:31, Sundar Yamunachari wrote:
>> Jan:
>>
>> auto-installer:
>>
>> 74: nit - Can you change machine address to IP address to be clear.
>> Some people might assume that it is MAC address.
>
> Done.
>
>>
>> installadm-common.sh:
>>
>> 184-185: If I understand correctly "After 5091 is integrated, it is
>> not necessary to normalize the service_name. So you may want to
>> remove the line 187. Integration of 5091 will not break this code".
>> Is this correct assumption?
>
> This is correct - fix for 5091 will not break this code. Once 5091 is
> fixed,
> I plan to file bug to address necessary changes related to algorithm for
> normalization. This line as well as other code related to 'normalization'
> will have to be either modified or removed.
>
>>
>> The string "unknown" is used in many places. Can you make a variable,
>> assign the value "unknown" and use it?
>
> Done.
>
>>
>> installadm.h:
>>
>> 193: Can you change "service" to "install service"?
>
> Done.
>
>>
>> setup-sparc.sh:
>>
>> Here also the string "unknown" is used. You can define the string
>> "unknown" in installadm-common.sh and use it in setup-sparc and
>> create-client
>
> Done.
>
>>
>> Thanks,
>> Sundar
>>
>> jan damborsky wrote:
>>> Hi Sundar,
>>>
>>>
>>> On 03/09/09 08:41, Sundar Yamunachari wrote:
>>>> Jan,
>>>>
>>>> I will review your code again after your Sue's code push.
>>>
>>> Since Sue has already integrated, I have merged with her stuff,
>>> accommodated fix taking into account new format of service
>>> database, recreated test AI images and went through the same
>>> set of testing procedures I used initially.
>>>
>>> Could you please take a look at updated webrev ?
>>> http://cr.opensolaris.org/~dambi/bug-6320-merged/
>>>
>>> Thank you,
>>> Jan
>>>
>>>
>>>>
>>>> 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/20090310/734811dc/attachment.html>
