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/273db760/attachment.html>
