You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by bharat kumar <bh...@citrix.com> on 2013/07/03 17:50:15 UTC
Review Request 12251: Incorporating the review comments given by Alena and
Sheng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12251/
-----------------------------------------------------------
Review request for cloudstack, Alena Prokharchyk, Abhinandan Prateek, and Sheng Yang.
Repository: cloudstack-git
Description
-------
Incorporating the review comments given by Alena and Sheng.
Diffs
-----
api/src/com/cloud/agent/api/to/DnsmasqTO.java c7be04d
api/src/com/cloud/network/element/DhcpServiceProvider.java 83008ca
core/src/com/cloud/agent/api/routing/DnsMasqConfigCommand.java a52af90
core/src/com/cloud/network/DnsMasqConfigurator.java dd34926
engine/schema/src/com/cloud/network/dao/IPAddressDao.java 3eba6d8
engine/schema/src/com/cloud/network/dao/IPAddressDaoImpl.java 1051b69
patches/systemvm/debian/config/root/createIpAlias.sh 1db210b
patches/systemvm/debian/config/root/deleteIpAlias.sh cf6d4de
patches/systemvm/debian/config/root/dnsmasq.sh 656fd3c
scripts/network/domr/call_dnsmasq.sh 097e185
scripts/vm/hypervisor/xenserver/createipAlias.sh c35658e
scripts/vm/hypervisor/xenserver/deleteipAlias.sh 6816edd
server/src/com/cloud/configuration/ConfigurationManagerImpl.java 27c1a51
server/src/com/cloud/network/NetworkServiceImpl.java 8cdcfde
server/src/com/cloud/network/element/VirtualRouterElement.java 1916678
server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java e863af4
utils/src/com/cloud/utils/net/NetUtils.java 5c13454
Diff: https://reviews.apache.org/r/12251/diff/
Testing
-------
Thanks,
bharat kumar
Re: Review Request 12251: Incorporating the review comments given by Alena
and Sheng
Posted by bharat kumar <bh...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12251/#review22839
-----------------------------------------------------------
engine/schema/src/com/cloud/network/dao/IPAddressDaoImpl.java
<https://reviews.apache.org/r/12251/#comment46547>
removed the raw sql code.
server/src/com/cloud/configuration/ConfigurationManagerImpl.java
<https://reviews.apache.org/r/12251/#comment46549>
Not catching the SQL exception any more.
- bharat kumar
On July 8, 2013, 5:47 p.m., bharat kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12251/
> -----------------------------------------------------------
>
> (Updated July 8, 2013, 5:47 p.m.)
>
>
> Review request for cloudstack, Alena Prokharchyk, Abhinandan Prateek, and Sheng Yang.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Incorporating the review comments given by Alena and Sheng.
>
>
> Diffs
> -----
>
> engine/schema/src/com/cloud/network/dao/IPAddressDao.java 3eba6d8
> engine/schema/src/com/cloud/network/dao/IPAddressDaoImpl.java 1051b69
> server/src/com/cloud/configuration/ConfigurationManagerImpl.java 27c1a51
>
> Diff: https://reviews.apache.org/r/12251/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> bharat kumar
>
>
Re: Review Request 12251: Incorporating the review comments given by Alena
and Sheng
Posted by Abhinandan Prateek <ap...@apache.org>.
> On July 15, 2013, 6:33 p.m., Sheng Yang wrote:
> > I think we can split the patch by different fixes(ensure each one of them won't break compile or any functionality), then we can commit it gradually. Do all of them in one batch is too big and hard to review.
> >
> > Besides, I didn't see any change to dnsmasq config file update mechanism. Regenerated one file is unacceptable for now since cloud-early-config would modify the file as well.
Yes I agree, in that we break this into a set of manageable pieces that can be reviewed and independently committed.
- Abhinandan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12251/#review23165
-----------------------------------------------------------
On July 13, 2013, 2:33 p.m., bharat kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12251/
> -----------------------------------------------------------
>
> (Updated July 13, 2013, 2:33 p.m.)
>
>
> Review request for cloudstack, Alena Prokharchyk, Abhinandan Prateek, and Sheng Yang.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Incorporating the review comments given by Alena and Sheng.
>
>
> Diffs
> -----
>
> api/src/com/cloud/agent/api/to/DhcpTO.java PRE-CREATION
> api/src/com/cloud/agent/api/to/DnsmasqTO.java c7be04d
> api/src/com/cloud/network/Network.java a06208b
> api/src/com/cloud/network/NetworkService.java 405cecd
> api/src/com/cloud/network/element/DhcpServiceProvider.java 83008ca
> core/src/com/cloud/agent/api/routing/DnsMasqConfigCommand.java a52af90
> core/src/com/cloud/network/DnsMasqConfigurator.java dd34926
> engine/schema/src/com/cloud/network/dao/IPAddressDao.java 3eba6d8
> engine/schema/src/com/cloud/network/dao/IPAddressDaoImpl.java 1051b69
> patches/systemvm/debian/config/root/createIpAlias.sh 1db210b
> patches/systemvm/debian/config/root/deleteIpAlias.sh cf6d4de
> scripts/network/domr/call_dnsmasq.sh 097e185
> scripts/vm/hypervisor/xenserver/createipAlias.sh c35658e
> scripts/vm/hypervisor/xenserver/deleteipAlias.sh 6816edd
> server/src/com/cloud/configuration/ConfigurationManagerImpl.java 27c1a51
> server/src/com/cloud/network/NetworkManagerImpl.java 708c03d
> server/src/com/cloud/network/NetworkServiceImpl.java 8cdcfde
> server/src/com/cloud/network/element/VirtualRouterElement.java 1916678
> server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java e863af4
> server/src/com/cloud/vm/VirtualMachineManagerImpl.java de0368a
> server/test/com/cloud/network/MockNetworkManagerImpl.java 077395f
> server/test/com/cloud/vpc/MockNetworkManagerImpl.java b609022
> utils/src/com/cloud/utils/net/NetUtils.java 5c13454
>
> Diff: https://reviews.apache.org/r/12251/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> bharat kumar
>
>
Re: Review Request 12251: Incorporating the review comments given by Alena
and Sheng
Posted by Sheng Yang <sh...@yasker.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12251/#review23165
-----------------------------------------------------------
I think we can split the patch by different fixes(ensure each one of them won't break compile or any functionality), then we can commit it gradually. Do all of them in one batch is too big and hard to review.
Besides, I didn't see any change to dnsmasq config file update mechanism. Regenerated one file is unacceptable for now since cloud-early-config would modify the file as well.
- Sheng Yang
On July 13, 2013, 2:33 p.m., bharat kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12251/
> -----------------------------------------------------------
>
> (Updated July 13, 2013, 2:33 p.m.)
>
>
> Review request for cloudstack, Alena Prokharchyk, Abhinandan Prateek, and Sheng Yang.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Incorporating the review comments given by Alena and Sheng.
>
>
> Diffs
> -----
>
> api/src/com/cloud/agent/api/to/DhcpTO.java PRE-CREATION
> api/src/com/cloud/agent/api/to/DnsmasqTO.java c7be04d
> api/src/com/cloud/network/Network.java a06208b
> api/src/com/cloud/network/NetworkService.java 405cecd
> api/src/com/cloud/network/element/DhcpServiceProvider.java 83008ca
> core/src/com/cloud/agent/api/routing/DnsMasqConfigCommand.java a52af90
> core/src/com/cloud/network/DnsMasqConfigurator.java dd34926
> engine/schema/src/com/cloud/network/dao/IPAddressDao.java 3eba6d8
> engine/schema/src/com/cloud/network/dao/IPAddressDaoImpl.java 1051b69
> patches/systemvm/debian/config/root/createIpAlias.sh 1db210b
> patches/systemvm/debian/config/root/deleteIpAlias.sh cf6d4de
> scripts/network/domr/call_dnsmasq.sh 097e185
> scripts/vm/hypervisor/xenserver/createipAlias.sh c35658e
> scripts/vm/hypervisor/xenserver/deleteipAlias.sh 6816edd
> server/src/com/cloud/configuration/ConfigurationManagerImpl.java 27c1a51
> server/src/com/cloud/network/NetworkManagerImpl.java 708c03d
> server/src/com/cloud/network/NetworkServiceImpl.java 8cdcfde
> server/src/com/cloud/network/element/VirtualRouterElement.java 1916678
> server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java e863af4
> server/src/com/cloud/vm/VirtualMachineManagerImpl.java de0368a
> server/test/com/cloud/network/MockNetworkManagerImpl.java 077395f
> server/test/com/cloud/vpc/MockNetworkManagerImpl.java b609022
> utils/src/com/cloud/utils/net/NetUtils.java 5c13454
>
> Diff: https://reviews.apache.org/r/12251/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> bharat kumar
>
>
Re: Review Request 12251: Incorporating the review comments given by Alena
and Sheng
Posted by Abhinandan Prateek <ap...@apache.org>.
> On July 17, 2013, 7:01 a.m., Abhinandan Prateek wrote:
> > Ship It!
This patch does not contain the dnsmasq changes. Bharat is going to create another review request for the dnsmasq changes so that it does not overwrite the cloud-early-config dns conf.
- Abhinandan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12251/#review23246
-----------------------------------------------------------
On July 17, 2013, 5:52 a.m., bharat kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12251/
> -----------------------------------------------------------
>
> (Updated July 17, 2013, 5:52 a.m.)
>
>
> Review request for cloudstack, Alena Prokharchyk, Abhinandan Prateek, and Sheng Yang.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Incorporating the review comments given by Alena and Sheng.
>
>
> Diffs
> -----
>
> api/src/com/cloud/agent/api/to/DhcpTO.java PRE-CREATION
> api/src/com/cloud/agent/api/to/DnsmasqTO.java c7be04d
> api/src/com/cloud/network/Network.java a06208b
> api/src/com/cloud/network/NetworkService.java 405cecd
> api/src/com/cloud/network/element/DhcpServiceProvider.java 83008ca
> core/src/com/cloud/agent/api/routing/DnsMasqConfigCommand.java a52af90
> core/src/com/cloud/network/DnsMasqConfigurator.java dd34926
> engine/schema/src/com/cloud/capacity/dao/CapacityDaoImpl.java cecc145
> engine/schema/src/com/cloud/network/dao/IPAddressDao.java 3eba6d8
> engine/schema/src/com/cloud/network/dao/IPAddressDaoImpl.java 1051b69
> engine/schema/src/com/cloud/vm/dao/NicDao.java 83f280e
> engine/schema/src/com/cloud/vm/dao/NicDaoImpl.java 420643f
> patches/systemvm/debian/config/root/createIpAlias.sh 1db210b
> patches/systemvm/debian/config/root/deleteIpAlias.sh cf6d4de
> patches/systemvm/debian/config/root/dnsmasq.sh 656fd3c
> scripts/network/domr/call_dnsmasq.sh 097e185
> scripts/vm/hypervisor/xenserver/createipAlias.sh c35658e
> scripts/vm/hypervisor/xenserver/deleteipAlias.sh 6816edd
> server/src/com/cloud/configuration/ConfigurationManagerImpl.java 801028d
> server/src/com/cloud/network/NetworkManagerImpl.java db375c3
> server/src/com/cloud/network/NetworkServiceImpl.java ccd23bf
> server/src/com/cloud/network/element/VirtualRouterElement.java 1916678
> server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java e437af6
> server/src/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java 39d0895
> server/src/com/cloud/vm/VirtualMachineManagerImpl.java afdc85d
> server/test/com/cloud/network/MockNetworkManagerImpl.java 9c7d092
> server/test/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImplTest.java 04a3601
> server/test/com/cloud/vpc/MockNetworkManagerImpl.java 523dfb8
> utils/conf/db.properties e1b5fe9
> utils/src/com/cloud/utils/net/NetUtils.java 68f4965
>
> Diff: https://reviews.apache.org/r/12251/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> bharat kumar
>
>
Re: Review Request 12251: Incorporating the review comments given by Alena
and Sheng
Posted by Abhinandan Prateek <ap...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12251/#review23246
-----------------------------------------------------------
Ship it!
Ship It!
- Abhinandan Prateek
On July 17, 2013, 5:52 a.m., bharat kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12251/
> -----------------------------------------------------------
>
> (Updated July 17, 2013, 5:52 a.m.)
>
>
> Review request for cloudstack, Alena Prokharchyk, Abhinandan Prateek, and Sheng Yang.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Incorporating the review comments given by Alena and Sheng.
>
>
> Diffs
> -----
>
> api/src/com/cloud/agent/api/to/DhcpTO.java PRE-CREATION
> api/src/com/cloud/agent/api/to/DnsmasqTO.java c7be04d
> api/src/com/cloud/network/Network.java a06208b
> api/src/com/cloud/network/NetworkService.java 405cecd
> api/src/com/cloud/network/element/DhcpServiceProvider.java 83008ca
> core/src/com/cloud/agent/api/routing/DnsMasqConfigCommand.java a52af90
> core/src/com/cloud/network/DnsMasqConfigurator.java dd34926
> engine/schema/src/com/cloud/capacity/dao/CapacityDaoImpl.java cecc145
> engine/schema/src/com/cloud/network/dao/IPAddressDao.java 3eba6d8
> engine/schema/src/com/cloud/network/dao/IPAddressDaoImpl.java 1051b69
> engine/schema/src/com/cloud/vm/dao/NicDao.java 83f280e
> engine/schema/src/com/cloud/vm/dao/NicDaoImpl.java 420643f
> patches/systemvm/debian/config/root/createIpAlias.sh 1db210b
> patches/systemvm/debian/config/root/deleteIpAlias.sh cf6d4de
> patches/systemvm/debian/config/root/dnsmasq.sh 656fd3c
> scripts/network/domr/call_dnsmasq.sh 097e185
> scripts/vm/hypervisor/xenserver/createipAlias.sh c35658e
> scripts/vm/hypervisor/xenserver/deleteipAlias.sh 6816edd
> server/src/com/cloud/configuration/ConfigurationManagerImpl.java 801028d
> server/src/com/cloud/network/NetworkManagerImpl.java db375c3
> server/src/com/cloud/network/NetworkServiceImpl.java ccd23bf
> server/src/com/cloud/network/element/VirtualRouterElement.java 1916678
> server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java e437af6
> server/src/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java 39d0895
> server/src/com/cloud/vm/VirtualMachineManagerImpl.java afdc85d
> server/test/com/cloud/network/MockNetworkManagerImpl.java 9c7d092
> server/test/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImplTest.java 04a3601
> server/test/com/cloud/vpc/MockNetworkManagerImpl.java 523dfb8
> utils/conf/db.properties e1b5fe9
> utils/src/com/cloud/utils/net/NetUtils.java 68f4965
>
> Diff: https://reviews.apache.org/r/12251/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> bharat kumar
>
>
Re: Review Request 12251: Incorporating the review comments given by Alena
and Sheng
Posted by bharat kumar <bh...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12251/
-----------------------------------------------------------
(Updated July 17, 2013, 5:52 a.m.)
Review request for cloudstack, Alena Prokharchyk, Abhinandan Prateek, and Sheng Yang.
Changes
-------
rebased with master.
Repository: cloudstack-git
Description
-------
Incorporating the review comments given by Alena and Sheng.
Diffs (updated)
-----
api/src/com/cloud/agent/api/to/DhcpTO.java PRE-CREATION
api/src/com/cloud/agent/api/to/DnsmasqTO.java c7be04d
api/src/com/cloud/network/Network.java a06208b
api/src/com/cloud/network/NetworkService.java 405cecd
api/src/com/cloud/network/element/DhcpServiceProvider.java 83008ca
core/src/com/cloud/agent/api/routing/DnsMasqConfigCommand.java a52af90
core/src/com/cloud/network/DnsMasqConfigurator.java dd34926
engine/schema/src/com/cloud/capacity/dao/CapacityDaoImpl.java cecc145
engine/schema/src/com/cloud/network/dao/IPAddressDao.java 3eba6d8
engine/schema/src/com/cloud/network/dao/IPAddressDaoImpl.java 1051b69
engine/schema/src/com/cloud/vm/dao/NicDao.java 83f280e
engine/schema/src/com/cloud/vm/dao/NicDaoImpl.java 420643f
patches/systemvm/debian/config/root/createIpAlias.sh 1db210b
patches/systemvm/debian/config/root/deleteIpAlias.sh cf6d4de
patches/systemvm/debian/config/root/dnsmasq.sh 656fd3c
scripts/network/domr/call_dnsmasq.sh 097e185
scripts/vm/hypervisor/xenserver/createipAlias.sh c35658e
scripts/vm/hypervisor/xenserver/deleteipAlias.sh 6816edd
server/src/com/cloud/configuration/ConfigurationManagerImpl.java 801028d
server/src/com/cloud/network/NetworkManagerImpl.java db375c3
server/src/com/cloud/network/NetworkServiceImpl.java ccd23bf
server/src/com/cloud/network/element/VirtualRouterElement.java 1916678
server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java e437af6
server/src/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java 39d0895
server/src/com/cloud/vm/VirtualMachineManagerImpl.java afdc85d
server/test/com/cloud/network/MockNetworkManagerImpl.java 9c7d092
server/test/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImplTest.java 04a3601
server/test/com/cloud/vpc/MockNetworkManagerImpl.java 523dfb8
utils/conf/db.properties e1b5fe9
utils/src/com/cloud/utils/net/NetUtils.java 68f4965
Diff: https://reviews.apache.org/r/12251/diff/
Testing
-------
Thanks,
bharat kumar
Re: Review Request 12251: Incorporating the review comments given by Alena
and Sheng
Posted by bharat kumar <bh...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12251/
-----------------------------------------------------------
(Updated July 13, 2013, 2:33 p.m.)
Review request for cloudstack, Alena Prokharchyk, Abhinandan Prateek, and Sheng Yang.
Changes
-------
incorporating the comments given by Murali. Revoking the alias ip when the last vm from the subnet is expunged.
Repository: cloudstack-git
Description
-------
Incorporating the review comments given by Alena and Sheng.
Diffs (updated)
-----
api/src/com/cloud/agent/api/to/DhcpTO.java PRE-CREATION
api/src/com/cloud/agent/api/to/DnsmasqTO.java c7be04d
api/src/com/cloud/network/Network.java a06208b
api/src/com/cloud/network/NetworkService.java 405cecd
api/src/com/cloud/network/element/DhcpServiceProvider.java 83008ca
core/src/com/cloud/agent/api/routing/DnsMasqConfigCommand.java a52af90
core/src/com/cloud/network/DnsMasqConfigurator.java dd34926
engine/schema/src/com/cloud/network/dao/IPAddressDao.java 3eba6d8
engine/schema/src/com/cloud/network/dao/IPAddressDaoImpl.java 1051b69
patches/systemvm/debian/config/root/createIpAlias.sh 1db210b
patches/systemvm/debian/config/root/deleteIpAlias.sh cf6d4de
scripts/network/domr/call_dnsmasq.sh 097e185
scripts/vm/hypervisor/xenserver/createipAlias.sh c35658e
scripts/vm/hypervisor/xenserver/deleteipAlias.sh 6816edd
server/src/com/cloud/configuration/ConfigurationManagerImpl.java 27c1a51
server/src/com/cloud/network/NetworkManagerImpl.java 708c03d
server/src/com/cloud/network/NetworkServiceImpl.java 8cdcfde
server/src/com/cloud/network/element/VirtualRouterElement.java 1916678
server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java e863af4
server/src/com/cloud/vm/VirtualMachineManagerImpl.java de0368a
server/test/com/cloud/network/MockNetworkManagerImpl.java 077395f
server/test/com/cloud/vpc/MockNetworkManagerImpl.java b609022
utils/src/com/cloud/utils/net/NetUtils.java 5c13454
Diff: https://reviews.apache.org/r/12251/diff/
Testing
-------
Thanks,
bharat kumar
Re: Review Request 12251: Incorporating the review comments given by Alena
and Sheng
Posted by Abhinandan Prateek <ap...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12251/#review22900
-----------------------------------------------------------
server/src/com/cloud/configuration/ConfigurationManagerImpl.java
<https://reviews.apache.org/r/12251/#comment46649>
You can remove the catch as you jurt rethrow the caught exception.
- Abhinandan Prateek
On July 8, 2013, 5:47 p.m., bharat kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12251/
> -----------------------------------------------------------
>
> (Updated July 8, 2013, 5:47 p.m.)
>
>
> Review request for cloudstack, Alena Prokharchyk, Abhinandan Prateek, and Sheng Yang.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Incorporating the review comments given by Alena and Sheng.
>
>
> Diffs
> -----
>
> engine/schema/src/com/cloud/network/dao/IPAddressDao.java 3eba6d8
> engine/schema/src/com/cloud/network/dao/IPAddressDaoImpl.java 1051b69
> server/src/com/cloud/configuration/ConfigurationManagerImpl.java 27c1a51
>
> Diff: https://reviews.apache.org/r/12251/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> bharat kumar
>
>
Re: Review Request 12251: Incorporating the review comments given by Alena
and Sheng
Posted by Abhinandan Prateek <ap...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12251/#review22901
-----------------------------------------------------------
Can you create a new patch that consolidates all these changes and resubmit.
- Abhinandan Prateek
On July 8, 2013, 5:47 p.m., bharat kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12251/
> -----------------------------------------------------------
>
> (Updated July 8, 2013, 5:47 p.m.)
>
>
> Review request for cloudstack, Alena Prokharchyk, Abhinandan Prateek, and Sheng Yang.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Incorporating the review comments given by Alena and Sheng.
>
>
> Diffs
> -----
>
> engine/schema/src/com/cloud/network/dao/IPAddressDao.java 3eba6d8
> engine/schema/src/com/cloud/network/dao/IPAddressDaoImpl.java 1051b69
> server/src/com/cloud/configuration/ConfigurationManagerImpl.java 27c1a51
>
> Diff: https://reviews.apache.org/r/12251/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> bharat kumar
>
>
Re: Review Request 12251: Incorporating the review comments given by Alena
and Sheng
Posted by bharat kumar <bh...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12251/
-----------------------------------------------------------
(Updated July 8, 2013, 5:47 p.m.)
Review request for cloudstack, Alena Prokharchyk, Abhinandan Prateek, and Sheng Yang.
Repository: cloudstack-git
Description
-------
Incorporating the review comments given by Alena and Sheng.
Diffs (updated)
-----
engine/schema/src/com/cloud/network/dao/IPAddressDao.java 3eba6d8
engine/schema/src/com/cloud/network/dao/IPAddressDaoImpl.java 1051b69
server/src/com/cloud/configuration/ConfigurationManagerImpl.java 27c1a51
Diff: https://reviews.apache.org/r/12251/diff/
Testing
-------
Thanks,
bharat kumar
Re: Review Request 12251: Incorporating the review comments given by Alena
and Sheng
Posted by bharat kumar <bh...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12251/#review22836
-----------------------------------------------------------
server/src/com/cloud/configuration/ConfigurationManagerImpl.java
<https://reviews.apache.org/r/12251/#comment46541>
Not calling the Transaction start here. i broke them into smaller transactions. Also not holing the transaction while sending the command to the backend.
- bharat kumar
On July 4, 2013, 2:09 a.m., bharat kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12251/
> -----------------------------------------------------------
>
> (Updated July 4, 2013, 2:09 a.m.)
>
>
> Review request for cloudstack, Alena Prokharchyk, Abhinandan Prateek, and Sheng Yang.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Incorporating the review comments given by Alena and Sheng.
>
>
> Diffs
> -----
>
> api/src/com/cloud/network/Network.java a06208b
> api/src/com/cloud/network/NetworkService.java 405cecd
> server/src/com/cloud/configuration/ConfigurationManagerImpl.java 27c1a51
> server/src/com/cloud/network/NetworkManagerImpl.java 708c03d
> server/src/com/cloud/network/NetworkServiceImpl.java 8cdcfde
> server/src/com/cloud/network/element/VirtualRouterElement.java 1916678
> server/test/com/cloud/network/MockNetworkManagerImpl.java 077395f
> server/test/com/cloud/vpc/MockNetworkManagerImpl.java b609022
>
> Diff: https://reviews.apache.org/r/12251/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> bharat kumar
>
>
Re: Review Request 12251: Incorporating the review comments given by Alena
and Sheng
Posted by bharat kumar <bh...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12251/
-----------------------------------------------------------
(Updated July 4, 2013, 2:09 a.m.)
Review request for cloudstack, Alena Prokharchyk, Abhinandan Prateek, and Sheng Yang.
Changes
-------
Sheng commented on the issue that we are calling the function configDhcpSupportForSubnet for all dhcp service providers even if they do not support multiple subnets. This will cause the dhcp config to fail in cases which do not support this
feature. I fixed this by adding the dhcpservice across multiple subnets as a capability of the virtualrouter's dhcp service, introduced a check before adding a new subnet and also before calling the configDhcpSupportForSubnet.
Repository: cloudstack-git
Description
-------
Incorporating the review comments given by Alena and Sheng.
Diffs (updated)
-----
api/src/com/cloud/network/Network.java a06208b
api/src/com/cloud/network/NetworkService.java 405cecd
server/src/com/cloud/configuration/ConfigurationManagerImpl.java 27c1a51
server/src/com/cloud/network/NetworkManagerImpl.java 708c03d
server/src/com/cloud/network/NetworkServiceImpl.java 8cdcfde
server/src/com/cloud/network/element/VirtualRouterElement.java 1916678
server/test/com/cloud/network/MockNetworkManagerImpl.java 077395f
server/test/com/cloud/vpc/MockNetworkManagerImpl.java b609022
Diff: https://reviews.apache.org/r/12251/diff/
Testing
-------
Thanks,
bharat kumar
Re: Review Request 12251: Incorporating the review comments given by Alena
and Sheng
Posted by Alena Prokharchyk <al...@citrix.com>.
> On July 3, 2013, 3:58 p.m., bharat kumar wrote:
> > server/src/com/cloud/configuration/ConfigurationManagerImpl.java, line 3054
> > <https://reviews.apache.org/r/12251/diff/1/?file=318002#file318002line3054>
> >
> > removed the sql exception
Bharat, I still see SQLException being caught:
} catch (SQLException e) {
3090
throw new CloudRuntimeException(e.getMessage());
3104
}
3091
}
Can you please remove this try/catch as you are not throwing the exception any longer?
- Alena
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12251/#review22722
-----------------------------------------------------------
On July 3, 2013, 3:50 p.m., bharat kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12251/
> -----------------------------------------------------------
>
> (Updated July 3, 2013, 3:50 p.m.)
>
>
> Review request for cloudstack, Alena Prokharchyk, Abhinandan Prateek, and Sheng Yang.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Incorporating the review comments given by Alena and Sheng.
>
>
> Diffs
> -----
>
> api/src/com/cloud/agent/api/to/DnsmasqTO.java c7be04d
> api/src/com/cloud/network/element/DhcpServiceProvider.java 83008ca
> core/src/com/cloud/agent/api/routing/DnsMasqConfigCommand.java a52af90
> core/src/com/cloud/network/DnsMasqConfigurator.java dd34926
> engine/schema/src/com/cloud/network/dao/IPAddressDao.java 3eba6d8
> engine/schema/src/com/cloud/network/dao/IPAddressDaoImpl.java 1051b69
> patches/systemvm/debian/config/root/createIpAlias.sh 1db210b
> patches/systemvm/debian/config/root/deleteIpAlias.sh cf6d4de
> patches/systemvm/debian/config/root/dnsmasq.sh 656fd3c
> scripts/network/domr/call_dnsmasq.sh 097e185
> scripts/vm/hypervisor/xenserver/createipAlias.sh c35658e
> scripts/vm/hypervisor/xenserver/deleteipAlias.sh 6816edd
> server/src/com/cloud/configuration/ConfigurationManagerImpl.java 27c1a51
> server/src/com/cloud/network/NetworkServiceImpl.java 8cdcfde
> server/src/com/cloud/network/element/VirtualRouterElement.java 1916678
> server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java e863af4
> utils/src/com/cloud/utils/net/NetUtils.java 5c13454
>
> Diff: https://reviews.apache.org/r/12251/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> bharat kumar
>
>
Re: Review Request 12251: Incorporating the review comments given by Alena
and Sheng
Posted by bharat kumar <bh...@citrix.com>.
> On July 3, 2013, 3:58 p.m., bharat kumar wrote:
> > server/src/com/cloud/configuration/ConfigurationManagerImpl.java, line 3054
> > <https://reviews.apache.org/r/12251/diff/1/?file=318002#file318002line3054>
> >
> > removed the sql exception
>
> Alena Prokharchyk wrote:
> Bharat, I still see SQLException being caught:
>
> } catch (SQLException e) {
> 3090
> throw new CloudRuntimeException(e.getMessage());
> 3104
> }
> 3091
> }
>
> Can you please remove this try/catch as you are not throwing the exception any longer?
The function deletePublicIPRangeExceptAliasIP used in handleIpAliasDeletion throws the sql exception.
- bharat
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12251/#review22722
-----------------------------------------------------------
On July 3, 2013, 3:50 p.m., bharat kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12251/
> -----------------------------------------------------------
>
> (Updated July 3, 2013, 3:50 p.m.)
>
>
> Review request for cloudstack, Alena Prokharchyk, Abhinandan Prateek, and Sheng Yang.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Incorporating the review comments given by Alena and Sheng.
>
>
> Diffs
> -----
>
> api/src/com/cloud/agent/api/to/DnsmasqTO.java c7be04d
> api/src/com/cloud/network/element/DhcpServiceProvider.java 83008ca
> core/src/com/cloud/agent/api/routing/DnsMasqConfigCommand.java a52af90
> core/src/com/cloud/network/DnsMasqConfigurator.java dd34926
> engine/schema/src/com/cloud/network/dao/IPAddressDao.java 3eba6d8
> engine/schema/src/com/cloud/network/dao/IPAddressDaoImpl.java 1051b69
> patches/systemvm/debian/config/root/createIpAlias.sh 1db210b
> patches/systemvm/debian/config/root/deleteIpAlias.sh cf6d4de
> patches/systemvm/debian/config/root/dnsmasq.sh 656fd3c
> scripts/network/domr/call_dnsmasq.sh 097e185
> scripts/vm/hypervisor/xenserver/createipAlias.sh c35658e
> scripts/vm/hypervisor/xenserver/deleteipAlias.sh 6816edd
> server/src/com/cloud/configuration/ConfigurationManagerImpl.java 27c1a51
> server/src/com/cloud/network/NetworkServiceImpl.java 8cdcfde
> server/src/com/cloud/network/element/VirtualRouterElement.java 1916678
> server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java e863af4
> utils/src/com/cloud/utils/net/NetUtils.java 5c13454
>
> Diff: https://reviews.apache.org/r/12251/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> bharat kumar
>
>
Re: Review Request 12251: Incorporating the review comments given by Alena
and Sheng
Posted by Alena Prokharchyk <al...@citrix.com>.
> On July 3, 2013, 3:58 p.m., bharat kumar wrote:
> > server/src/com/cloud/configuration/ConfigurationManagerImpl.java, line 3054
> > <https://reviews.apache.org/r/12251/diff/1/?file=318002#file318002line3054>
> >
> > removed the sql exception
>
> Alena Prokharchyk wrote:
> Bharat, I still see SQLException being caught:
>
> } catch (SQLException e) {
> 3090
> throw new CloudRuntimeException(e.getMessage());
> 3104
> }
> 3091
> }
>
> Can you please remove this try/catch as you are not throwing the exception any longer?
>
> bharat kumar wrote:
> The function deletePublicIPRangeExceptAliasIP used in handleIpAliasDeletion throws the sql exception.
Well, you should remove it. This code:
@Override
public boolean deletePublicIPRangeExceptAliasIP(long vlanDbId, String aliasIp) throws SQLException {
Transaction txn = Transaction.currentTxn();
String deleteSql = "DELETE FROM `cloud`.`user_ip_address` WHERE vlan_db_id = ? and public_ip_address!=?";
txn.start();
PreparedStatement stmt = txn.prepareAutoCloseStatement(deleteSql);
stmt.setLong(1, vlanDbId);
stmt.setString(2, aliasIp);
stmt.executeUpdate();
txn.commit();
return true;
}
uses raw mysql update. We should never do that. What if vlan_db_id/public_ip_address are renamed or gone in the future? It won't be caught during compilation time. Please rewrite this code the following way:
* use SearchBuilder or SearchCriteria to identify the rows you need to remove. Remove the raw mysql statement.
* remove PreparedStatement update.
* remove SQLException from the throws clause and from the underlying try/catch blocks.
- Alena
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12251/#review22722
-----------------------------------------------------------
On July 4, 2013, 2:09 a.m., bharat kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12251/
> -----------------------------------------------------------
>
> (Updated July 4, 2013, 2:09 a.m.)
>
>
> Review request for cloudstack, Alena Prokharchyk, Abhinandan Prateek, and Sheng Yang.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Incorporating the review comments given by Alena and Sheng.
>
>
> Diffs
> -----
>
> api/src/com/cloud/network/Network.java a06208b
> api/src/com/cloud/network/NetworkService.java 405cecd
> server/src/com/cloud/configuration/ConfigurationManagerImpl.java 27c1a51
> server/src/com/cloud/network/NetworkManagerImpl.java 708c03d
> server/src/com/cloud/network/NetworkServiceImpl.java 8cdcfde
> server/src/com/cloud/network/element/VirtualRouterElement.java 1916678
> server/test/com/cloud/network/MockNetworkManagerImpl.java 077395f
> server/test/com/cloud/vpc/MockNetworkManagerImpl.java b609022
>
> Diff: https://reviews.apache.org/r/12251/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> bharat kumar
>
>
Re: Review Request 12251: Incorporating the review comments given by Alena
and Sheng
Posted by bharat kumar <bh...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12251/#review22722
-----------------------------------------------------------
core/src/com/cloud/agent/api/routing/DnsMasqConfigCommand.java
<https://reviews.apache.org/r/12251/#comment46413>
rename the dnsmasqto to dhcpto
engine/schema/src/com/cloud/network/dao/IPAddressDao.java
<https://reviews.apache.org/r/12251/#comment46414>
Removed the sqlexception.
engine/schema/src/com/cloud/network/dao/IPAddressDaoImpl.java
<https://reviews.apache.org/r/12251/#comment46415>
removed the sqlexception
server/src/com/cloud/configuration/ConfigurationManagerImpl.java
<https://reviews.apache.org/r/12251/#comment46416>
added the enum
server/src/com/cloud/configuration/ConfigurationManagerImpl.java
<https://reviews.apache.org/r/12251/#comment46417>
removed the sql exception
server/src/com/cloud/configuration/ConfigurationManagerImpl.java
<https://reviews.apache.org/r/12251/#comment46418>
not using rollback explicitly
server/src/com/cloud/network/element/VirtualRouterElement.java
<https://reviews.apache.org/r/12251/#comment46419>
not catching the exception any more
server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java
<https://reviews.apache.org/r/12251/#comment46420>
renamed dnsmasq to dhcpto
- bharat kumar
On July 3, 2013, 3:50 p.m., bharat kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12251/
> -----------------------------------------------------------
>
> (Updated July 3, 2013, 3:50 p.m.)
>
>
> Review request for cloudstack, Alena Prokharchyk, Abhinandan Prateek, and Sheng Yang.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Incorporating the review comments given by Alena and Sheng.
>
>
> Diffs
> -----
>
> api/src/com/cloud/agent/api/to/DnsmasqTO.java c7be04d
> api/src/com/cloud/network/element/DhcpServiceProvider.java 83008ca
> core/src/com/cloud/agent/api/routing/DnsMasqConfigCommand.java a52af90
> core/src/com/cloud/network/DnsMasqConfigurator.java dd34926
> engine/schema/src/com/cloud/network/dao/IPAddressDao.java 3eba6d8
> engine/schema/src/com/cloud/network/dao/IPAddressDaoImpl.java 1051b69
> patches/systemvm/debian/config/root/createIpAlias.sh 1db210b
> patches/systemvm/debian/config/root/deleteIpAlias.sh cf6d4de
> patches/systemvm/debian/config/root/dnsmasq.sh 656fd3c
> scripts/network/domr/call_dnsmasq.sh 097e185
> scripts/vm/hypervisor/xenserver/createipAlias.sh c35658e
> scripts/vm/hypervisor/xenserver/deleteipAlias.sh 6816edd
> server/src/com/cloud/configuration/ConfigurationManagerImpl.java 27c1a51
> server/src/com/cloud/network/NetworkServiceImpl.java 8cdcfde
> server/src/com/cloud/network/element/VirtualRouterElement.java 1916678
> server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java e863af4
> utils/src/com/cloud/utils/net/NetUtils.java 5c13454
>
> Diff: https://reviews.apache.org/r/12251/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> bharat kumar
>
>