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. >> > > > > > > >> > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > >> > >
