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/20090309/f763fee7/attachment.html>
