Thanks ! Jan
On 03/10/09 09:23, Sundar Yamunachari wrote: > 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/e97bfc0e/attachment.html>
