After talking with Daan, we decided to revert the commit for now. After Bharat complete the rewrite of dnsmasqconfigurator in bash script. A new patch would be committed to fix CLOUDSTACK-3357 before the code freeze,
--Sheng On Sun, Jul 21, 2013 at 9:52 AM, Sheng Yang <sh...@yasker.org> wrote: > And it's also my fault on this issue because I didn't look at the patch > when it's in reviewboard. I was not that diligent at reviewboard before... > > To prevent the same thing happen again, we may want to do something like > xxx area's code need to be reviewed by xxx, xxx, xxx in the future? > > --Sheng > > > On Sun, Jul 21, 2013 at 9:28 AM, Sheng Yang <sh...@yasker.org> wrote: > >> Sorry, the integration test of VPC has been failed for quite some time, I >> just found this issue after fixing some other issues in the integration >> test... >> >> --Sheng >> >> >> >> On Sun, Jul 21, 2013 at 9:23 AM, Daan Hoogland >> <daan.hoogl...@gmail.com>wrote: >> >>> I am happy to redo the work with a different approach, but it did not >>> break >>> integration last week, did it? So a newer patch needs to be reverted, no? >>> The problem might be cumulative due to more then one commit, in which >>> case >>> a role forward might be the quickest way to go. >>> >>> >>> On Sun, Jul 21, 2013 at 6:15 PM, Sheng Yang <sh...@yasker.org> wrote: >>> >>> > In fact DnsMasqConfigurator has other issues since it would complete >>> > override the whole configuration file modified by bash scripts in the >>> VR. >>> > So Bharat is working on that and should send out patch soon, using bash >>> > scripts on the dnsmasq.conf instead. Also that's our preferred >>> approach to >>> > modify dnsmasq.conf, so I think we can fix the network domain issue >>> after >>> > Bharat's patch is checked in. I am sure at least you need a parameter >>> to >>> > indicate VPC router. >>> > >>> > For now, I think the first priority is unblock regression test and vpc >>> > function. >>> > >>> > --Sheng >>> > >>> > On Sun, Jul 21, 2013 at 9:07 AM, Daan Hoogland < >>> daan.hoogl...@gmail.com >>> > >wrote: >>> > >>> > > So if I understand your mail correctly, a VpcDnsMasqConfigurator >>> should >>> > be >>> > > created to be used in configDnsMasq in >>> VpcVirtualApplienceManagerImpl, >>> > > instead of the one used now? >>> > > >>> > > >>> > > On Sun, Jul 21, 2013 at 6:01 PM, Sheng Yang <sh...@yasker.org> >>> wrote: >>> > > >>> > > > Hi Daan, >>> > > > >>> > > > We're talking about vpc router. Your patch affect the guest vm >>> because >>> > > > guest VM cannot get ip address from VPC router(DHCP request fail). >>> > > > >>> > > > The VPC indeed using a different dnsmasq.conf. See >>> > > > /etc/vpcdnsmasq.conf(which is a template of VPC used dnsmasq). >>> > > > >>> > > > You're using DnsMasqConfigurator, which is only used for normal VR >>> > before >>> > > > your patch. It would generated dnsmasq.conf contained: >>> > > > >>> > > > <quote> >>> > > > except-interface=eth1 >>> > > > except-interface=eth2 >>> > > > except-interface=lo >>> > > > </quote> >>> > > > >>> > > > However, these lines by no chance can work in a VPC router, which >>> is >>> > > > introduced by your commit. Because for VPC router, eth2 is the >>> first >>> > > guest >>> > > > nic, it cannot be excepted from listening ports, otherwise any >>> guest >>> > vm's >>> > > > dhcp request in the first subnet would fail to get IP address >>> through >>> > > DHCP. >>> > > > >>> > > > --Sheng >>> > > > >>> > > > >>> > > > On Sun, Jul 21, 2013 at 3:49 AM, Daan Hoogland < >>> > daan.hoogl...@gmail.com >>> > > > >wrote: >>> > > > >>> > > > > H Sheng et al, >>> > > > > >>> > > > > I have no overview of what effect my patch has on guest vm's. It >>> is >>> > > > > supposed to only generate a dnsmasq.conf for vpc-router-vm's. not >>> > > > > guestnetwork-vm's. I am happy to look at it, but have no >>> overview of >>> > > the >>> > > > > issue yet. Should a sepperate dnsmasq.conf instance be created >>> for >>> > vpc >>> > > > > routers to deal with the different nics? >>> > > > > >>> > > > > >>> > > > > On Sat, Jul 20, 2013 at 3:32 AM, Sheng Yang <sh...@yasker.org> >>> > wrote: >>> > > > > >>> > > > > > Daan, I've checked the code, the issue is, original code Bharat >>> > > > > > wrote(generated dnsmasq.conf) cannot deal with VPC. e.g. the >>> VPC >>> > > router >>> > > > > > wouldn't listen on eth0 and eth1, but non-vpc router wouldn't >>> > listen >>> > > on >>> > > > > > eth1 and eth2. Also, some detail configurations of VPC are >>> written >>> > > into >>> > > > > > /etc/dnsmasq.d/cloud.conf, rather than dnsmasq.conf, because it >>> > need >>> > > to >>> > > > > > deal with multiple nics with different range. >>> > > > > > >>> > > > > > So using non-vpc's configuration just break whole VPC >>> function. No >>> > > > chance >>> > > > > > it can work because eth2(which is internal network interface in >>> > VPC) >>> > > is >>> > > > > > excluded from listening interface. >>> > > > > > >>> > > > > > We need to deal with it ASAP because it's blocking the >>> regression >>> > > test. >>> > > > > In >>> > > > > > fact I didn't see much solution for now except reverting it >>> since >>> > the >>> > > > > > dnsmasq config command didn't support VPC anyway. >>> > > > > > >>> > > > > > BTW, I don't agree that we need dnsmasq configuration file >>> > generated >>> > > by >>> > > > > > cloudstack, since it's possible that the scripts in the VR >>> would >>> > > modify >>> > > > > the >>> > > > > > file as well(e.g. in cloud-early-setup), then the modification >>> > would >>> > > be >>> > > > > > overrided by whole new file generated by management >>> server(which >>> > > > happened >>> > > > > > in this case). We have to gather them in one place. I've talked >>> > with >>> > > > > Bharat >>> > > > > > and he would provide a patch soon to revert the >>> > DnsMasqConfigurator, >>> > > > and >>> > > > > > use bash scripts to do the necessary substitute instead. I >>> think >>> > any >>> > > > > > potential change to dnsmasq config command need to wait for it. >>> > > > > > >>> > > > > > --Sheng >>> > > > > > >>> > > > > > >>> > > > > > On Fri, Jul 19, 2013 at 5:54 PM, Sheng Yang <sh...@yasker.org> >>> > > wrote: >>> > > > > > >>> > > > > > > Daan, >>> > > > > > > >>> > > > > > > Bharat thinks your fix caused a block bug >>> > > > > > > https://issues.apache.org/jira/browse/CLOUDSTACK-3589? >>> Could you >>> > > > check >>> > > > > > it? >>> > > > > > > >>> > > > > > > --Sheng >>> > > > > > > >>> > > > > > > On Thu, Jul 18, 2013 at 9:52 AM, Bharat Kumar < >>> > > > bharat.ku...@citrix.com >>> > > > > > >wrote: >>> > > > > > > >>> > > > > > >> Hi Dann, >>> > > > > > >> >>> > > > > > >> The bug fix >>> > > > > > >> >>> > > > > > >>> > > > > >>> > > > >>> > > >>> > >>> https://git-wip-us.apache.org/repos/asf?p=cloudstack.git;a=commitdiff;h=b903262df5e2ff5d174859ce28abae75c4689f0cis >>> > > > > > >> causing an null value in dnsmasq.config file while creating >>> VPC >>> > > > > network >>> > > > > > >> (bug-id Cloudstack-3589 ). >>> > > > > > >> >>> > > > > > >> Can you please take a look at this. >>> > > > > > >> >>> > > > > > >> Regards, >>> > > > > > >> Bharat. >>> > > > > > > >>> > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > >>> > > > >>> > > >>> > >>> >> >> >
