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/08/06 14:48:20 UTC
Review Request 13323: if a failure occurs while adding VM to another network
(this should be the first vm in the subnet). The ip alias created as a part
of this process is not removed.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13323/
-----------------------------------------------------------
Review request for cloudstack, Alena Prokharchyk and Sheng Yang.
Bugs: Cloudstack-4083
Repository: cloudstack-git
Description
-------
https://issues.apache.org/jira/browse/CLOUDSTACK-4083
if a failure occurs while adding VM to another network (this should be the first vm in the subnet). The ip alias created as a part of this process is not removed.
This occurred because we were not cleaning the alias ips in the event of a failure.
As a part of the fix.
1.) moved the function removeDhcpServiceInsubnet and listLastNicsInSubnet to NetworkManager (These were in VirtualMachineManagerImpl earlier.)
2.) add the call to clean ipAlias in the remove nic function of the networkManager.
3.) Modified the removeDhcpServiceInsubnet to take network ad an argument. This will help in removing only the ipAlias which belong to a particular network.
Diffs
-----
server/src/com/cloud/network/NetworkManager.java dab7a13
server/src/com/cloud/network/NetworkManagerImpl.java effee96
server/src/com/cloud/vm/VirtualMachineManagerImpl.java b33ee49
server/test/com/cloud/network/MockNetworkManagerImpl.java 7f34e55
server/test/com/cloud/vpc/MockNetworkManagerImpl.java 178ebba
Diff: https://reviews.apache.org/r/13323/diff/
Testing
-------
Tested on 4.2.
created two guest networks guest1 and guest2.
created VMS in both the networks.
Added a VM(with no PV drivers) from guest1 to guest2.
this failed and on failure ipAlias configured as part of nic creation was removed.
Deleting the vm causes all the removal of all ipAliases from all the subnets in which this is the lastvm.
Thanks,
bharat kumar
Re: Review Request 13323: if a failure occurs while adding VM to another
network (this should be the first vm in the subnet). The ip alias created
as a part of this process is not removed.
Posted by bharat kumar <bh...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13323/#review24718
-----------------------------------------------------------
server/src/com/cloud/vm/VirtualMachineManagerImpl.java
<https://reviews.apache.org/r/13323/#comment48823>
Moved this code to networkManagerImpl
server/src/com/cloud/vm/VirtualMachineManagerImpl.java
<https://reviews.apache.org/r/13323/#comment48821>
Moved this code to networkManagerimpl
server/src/com/cloud/vm/VirtualMachineManagerImpl.java
<https://reviews.apache.org/r/13323/#comment48822>
removed this and added it to removenic in networkManagerImpl.
- bharat kumar
On Aug. 6, 2013, 12:48 p.m., bharat kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13323/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2013, 12:48 p.m.)
>
>
> Review request for cloudstack, Alena Prokharchyk and Sheng Yang.
>
>
> Bugs: Cloudstack-4083
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/CLOUDSTACK-4083
> if a failure occurs while adding VM to another network (this should be the first vm in the subnet). The ip alias created as a part of this process is not removed.
>
> This occurred because we were not cleaning the alias ips in the event of a failure.
>
> As a part of the fix.
> 1.) moved the function removeDhcpServiceInsubnet and listLastNicsInSubnet to NetworkManager (These were in VirtualMachineManagerImpl earlier.)
> 2.) add the call to clean ipAlias in the remove nic function of the networkManager.
> 3.) Modified the removeDhcpServiceInsubnet to take network ad an argument. This will help in removing only the ipAlias which belong to a particular network.
>
>
> Diffs
> -----
>
> server/src/com/cloud/network/NetworkManager.java dab7a13
> server/src/com/cloud/network/NetworkManagerImpl.java effee96
> server/src/com/cloud/vm/VirtualMachineManagerImpl.java b33ee49
> server/test/com/cloud/network/MockNetworkManagerImpl.java 7f34e55
> server/test/com/cloud/vpc/MockNetworkManagerImpl.java 178ebba
>
> Diff: https://reviews.apache.org/r/13323/diff/
>
>
> Testing
> -------
>
> Tested on 4.2.
> created two guest networks guest1 and guest2.
> created VMS in both the networks.
> Added a VM(with no PV drivers) from guest1 to guest2.
> this failed and on failure ipAlias configured as part of nic creation was removed.
>
> Deleting the vm causes all the removal of all ipAliases from all the subnets in which this is the lastvm.
>
>
> Thanks,
>
> bharat kumar
>
>
Re: Review Request 13323: if a failure occurs while adding VM to another
network (this should be the first vm in the subnet). The ip alias created
as a part of this process is not removed.
Posted by Sheng Yang <sh...@yasker.org>.
> On Aug. 7, 2013, 8:46 p.m., Sheng Yang wrote:
> > Ship It!
>
> Sheng Yang wrote:
> Please uppercase the bug id(CLOUDSTACK-xxxx rather than Cloudstack-xxx) next time, otherwise jira won't recognize it. I also removed the unnecessary import and blank line.
Pushed to 4.2
Bharat, please send a patch for MASTER as well. It's not applied now.
- Sheng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13323/#review24826
-----------------------------------------------------------
On Aug. 7, 2013, 8:53 a.m., bharat kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13323/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2013, 8:53 a.m.)
>
>
> Review request for cloudstack, Alena Prokharchyk and Sheng Yang.
>
>
> Bugs: Cloudstack-4083
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/CLOUDSTACK-4083
> if a failure occurs while adding VM to another network (this should be the first vm in the subnet). The ip alias created as a part of this process is not removed.
>
> This occurred because we were not cleaning the alias ips in the event of a failure.
>
> As a part of the fix.
> 1.) moved the function removeDhcpServiceInsubnet and listLastNicsInSubnet to NetworkManager (These were in VirtualMachineManagerImpl earlier.)
> 2.) add the call to clean ipAlias in the remove nic function of the networkManager.
> 3.) Modified the removeDhcpServiceInsubnet to take network ad an argument. This will help in removing only the ipAlias which belong to a particular network.
>
>
> Diffs
> -----
>
> server/src/com/cloud/network/NetworkManager.java dab7a13
> server/src/com/cloud/network/NetworkManagerImpl.java ad79a2f
> server/src/com/cloud/vm/VirtualMachineManagerImpl.java b33ee49
> server/test/com/cloud/network/MockNetworkManagerImpl.java 7f34e55
> server/test/com/cloud/vpc/MockNetworkManagerImpl.java 178ebba
>
> Diff: https://reviews.apache.org/r/13323/diff/
>
>
> Testing
> -------
>
> Tested on 4.2.
> created two guest networks guest1 and guest2.
> created VMS in both the networks.
> Added a VM(with no PV drivers) from guest1 to guest2.
> this failed and on failure ipAlias configured as part of nic creation was removed.
>
> Deleting the vm causes all the removal of all ipAliases from all the subnets in which this is the lastvm.
>
>
> Thanks,
>
> bharat kumar
>
>
Re: Review Request 13323: if a failure occurs while adding VM to another
network (this should be the first vm in the subnet). The ip alias created
as a part of this process is not removed.
Posted by Sheng Yang <sh...@yasker.org>.
> On Aug. 7, 2013, 8:46 p.m., Sheng Yang wrote:
> > Ship It!
Please uppercase the bug id(CLOUDSTACK-xxxx rather than Cloudstack-xxx) next time, otherwise jira won't recognize it. I also removed the unnecessary import and blank line.
- Sheng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13323/#review24826
-----------------------------------------------------------
On Aug. 7, 2013, 8:53 a.m., bharat kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13323/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2013, 8:53 a.m.)
>
>
> Review request for cloudstack, Alena Prokharchyk and Sheng Yang.
>
>
> Bugs: Cloudstack-4083
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/CLOUDSTACK-4083
> if a failure occurs while adding VM to another network (this should be the first vm in the subnet). The ip alias created as a part of this process is not removed.
>
> This occurred because we were not cleaning the alias ips in the event of a failure.
>
> As a part of the fix.
> 1.) moved the function removeDhcpServiceInsubnet and listLastNicsInSubnet to NetworkManager (These were in VirtualMachineManagerImpl earlier.)
> 2.) add the call to clean ipAlias in the remove nic function of the networkManager.
> 3.) Modified the removeDhcpServiceInsubnet to take network ad an argument. This will help in removing only the ipAlias which belong to a particular network.
>
>
> Diffs
> -----
>
> server/src/com/cloud/network/NetworkManager.java dab7a13
> server/src/com/cloud/network/NetworkManagerImpl.java ad79a2f
> server/src/com/cloud/vm/VirtualMachineManagerImpl.java b33ee49
> server/test/com/cloud/network/MockNetworkManagerImpl.java 7f34e55
> server/test/com/cloud/vpc/MockNetworkManagerImpl.java 178ebba
>
> Diff: https://reviews.apache.org/r/13323/diff/
>
>
> Testing
> -------
>
> Tested on 4.2.
> created two guest networks guest1 and guest2.
> created VMS in both the networks.
> Added a VM(with no PV drivers) from guest1 to guest2.
> this failed and on failure ipAlias configured as part of nic creation was removed.
>
> Deleting the vm causes all the removal of all ipAliases from all the subnets in which this is the lastvm.
>
>
> Thanks,
>
> bharat kumar
>
>
Re: Review Request 13323: if a failure occurs while adding VM to another
network (this should be the first vm in the subnet). The ip alias created
as a part of this process is not removed.
Posted by Sheng Yang <sh...@yasker.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13323/#review24826
-----------------------------------------------------------
Ship it!
Ship It!
- Sheng Yang
On Aug. 7, 2013, 8:53 a.m., bharat kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13323/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2013, 8:53 a.m.)
>
>
> Review request for cloudstack, Alena Prokharchyk and Sheng Yang.
>
>
> Bugs: Cloudstack-4083
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/CLOUDSTACK-4083
> if a failure occurs while adding VM to another network (this should be the first vm in the subnet). The ip alias created as a part of this process is not removed.
>
> This occurred because we were not cleaning the alias ips in the event of a failure.
>
> As a part of the fix.
> 1.) moved the function removeDhcpServiceInsubnet and listLastNicsInSubnet to NetworkManager (These were in VirtualMachineManagerImpl earlier.)
> 2.) add the call to clean ipAlias in the remove nic function of the networkManager.
> 3.) Modified the removeDhcpServiceInsubnet to take network ad an argument. This will help in removing only the ipAlias which belong to a particular network.
>
>
> Diffs
> -----
>
> server/src/com/cloud/network/NetworkManager.java dab7a13
> server/src/com/cloud/network/NetworkManagerImpl.java ad79a2f
> server/src/com/cloud/vm/VirtualMachineManagerImpl.java b33ee49
> server/test/com/cloud/network/MockNetworkManagerImpl.java 7f34e55
> server/test/com/cloud/vpc/MockNetworkManagerImpl.java 178ebba
>
> Diff: https://reviews.apache.org/r/13323/diff/
>
>
> Testing
> -------
>
> Tested on 4.2.
> created two guest networks guest1 and guest2.
> created VMS in both the networks.
> Added a VM(with no PV drivers) from guest1 to guest2.
> this failed and on failure ipAlias configured as part of nic creation was removed.
>
> Deleting the vm causes all the removal of all ipAliases from all the subnets in which this is the lastvm.
>
>
> Thanks,
>
> bharat kumar
>
>
Re: Review Request 13323: if a failure occurs while adding VM to another
network (this should be the first vm in the subnet). The ip alias created
as a part of this process is not removed.
Posted by Sheng Yang <sh...@yasker.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13323/#review24894
-----------------------------------------------------------
Committed to MASTER.
- Sheng Yang
On Aug. 8, 2013, 6:03 a.m., bharat kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13323/
> -----------------------------------------------------------
>
> (Updated Aug. 8, 2013, 6:03 a.m.)
>
>
> Review request for cloudstack, Alena Prokharchyk and Sheng Yang.
>
>
> Bugs: Cloudstack-4083
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/CLOUDSTACK-4083
> if a failure occurs while adding VM to another network (this should be the first vm in the subnet). The ip alias created as a part of this process is not removed.
>
> This occurred because we were not cleaning the alias ips in the event of a failure.
>
> As a part of the fix.
> 1.) moved the function removeDhcpServiceInsubnet and listLastNicsInSubnet to NetworkManager (These were in VirtualMachineManagerImpl earlier.)
> 2.) add the call to clean ipAlias in the remove nic function of the networkManager.
> 3.) Modified the removeDhcpServiceInsubnet to take network ad an argument. This will help in removing only the ipAlias which belong to a particular network.
>
>
> Diffs
> -----
>
> server/src/com/cloud/network/NetworkManager.java f6dbb19
> server/src/com/cloud/network/NetworkManagerImpl.java 42e4093
> server/src/com/cloud/vm/VirtualMachineManagerImpl.java 15a9a82
> server/test/com/cloud/vpc/MockNetworkManagerImpl.java df552e4
>
> Diff: https://reviews.apache.org/r/13323/diff/
>
>
> Testing
> -------
>
> Tested on 4.2.
> created two guest networks guest1 and guest2.
> created VMS in both the networks.
> Added a VM(with no PV drivers) from guest1 to guest2.
> this failed and on failure ipAlias configured as part of nic creation was removed.
>
> Deleting the vm causes all the removal of all ipAliases from all the subnets in which this is the lastvm.
>
>
> Thanks,
>
> bharat kumar
>
>
Re: Review Request 13323: if a failure occurs while adding VM to another
network (this should be the first vm in the subnet). The ip alias created
as a part of this process is not removed.
Posted by bharat kumar <bh...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13323/
-----------------------------------------------------------
(Updated Aug. 8, 2013, 6:03 a.m.)
Review request for cloudstack, Alena Prokharchyk and Sheng Yang.
Changes
-------
Patch for master.
Bugs: Cloudstack-4083
Repository: cloudstack-git
Description
-------
https://issues.apache.org/jira/browse/CLOUDSTACK-4083
if a failure occurs while adding VM to another network (this should be the first vm in the subnet). The ip alias created as a part of this process is not removed.
This occurred because we were not cleaning the alias ips in the event of a failure.
As a part of the fix.
1.) moved the function removeDhcpServiceInsubnet and listLastNicsInSubnet to NetworkManager (These were in VirtualMachineManagerImpl earlier.)
2.) add the call to clean ipAlias in the remove nic function of the networkManager.
3.) Modified the removeDhcpServiceInsubnet to take network ad an argument. This will help in removing only the ipAlias which belong to a particular network.
Diffs (updated)
-----
server/src/com/cloud/network/NetworkManager.java f6dbb19
server/src/com/cloud/network/NetworkManagerImpl.java 42e4093
server/src/com/cloud/vm/VirtualMachineManagerImpl.java 15a9a82
server/test/com/cloud/vpc/MockNetworkManagerImpl.java df552e4
Diff: https://reviews.apache.org/r/13323/diff/
Testing
-------
Tested on 4.2.
created two guest networks guest1 and guest2.
created VMS in both the networks.
Added a VM(with no PV drivers) from guest1 to guest2.
this failed and on failure ipAlias configured as part of nic creation was removed.
Deleting the vm causes all the removal of all ipAliases from all the subnets in which this is the lastvm.
Thanks,
bharat kumar
Re: Review Request 13323: if a failure occurs while adding VM to another
network (this should be the first vm in the subnet). The ip alias created
as a part of this process is not removed.
Posted by bharat kumar <bh...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13323/
-----------------------------------------------------------
(Updated Aug. 7, 2013, 8:53 a.m.)
Review request for cloudstack, Alena Prokharchyk and Sheng Yang.
Bugs: Cloudstack-4083
Repository: cloudstack-git
Description
-------
https://issues.apache.org/jira/browse/CLOUDSTACK-4083
if a failure occurs while adding VM to another network (this should be the first vm in the subnet). The ip alias created as a part of this process is not removed.
This occurred because we were not cleaning the alias ips in the event of a failure.
As a part of the fix.
1.) moved the function removeDhcpServiceInsubnet and listLastNicsInSubnet to NetworkManager (These were in VirtualMachineManagerImpl earlier.)
2.) add the call to clean ipAlias in the remove nic function of the networkManager.
3.) Modified the removeDhcpServiceInsubnet to take network ad an argument. This will help in removing only the ipAlias which belong to a particular network.
Diffs (updated)
-----
server/src/com/cloud/network/NetworkManager.java dab7a13
server/src/com/cloud/network/NetworkManagerImpl.java ad79a2f
server/src/com/cloud/vm/VirtualMachineManagerImpl.java b33ee49
server/test/com/cloud/network/MockNetworkManagerImpl.java 7f34e55
server/test/com/cloud/vpc/MockNetworkManagerImpl.java 178ebba
Diff: https://reviews.apache.org/r/13323/diff/
Testing
-------
Tested on 4.2.
created two guest networks guest1 and guest2.
created VMS in both the networks.
Added a VM(with no PV drivers) from guest1 to guest2.
this failed and on failure ipAlias configured as part of nic creation was removed.
Deleting the vm causes all the removal of all ipAliases from all the subnets in which this is the lastvm.
Thanks,
bharat kumar
Re: Review Request 13323: if a failure occurs while adding VM to another
network (this should be the first vm in the subnet). The ip alias created
as a part of this process is not removed.
Posted by bharat kumar <bh...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13323/
-----------------------------------------------------------
(Updated Aug. 7, 2013, 8:22 a.m.)
Review request for cloudstack, Alena Prokharchyk and Sheng Yang.
Bugs: Cloudstack-4083
Repository: cloudstack-git
Description
-------
https://issues.apache.org/jira/browse/CLOUDSTACK-4083
if a failure occurs while adding VM to another network (this should be the first vm in the subnet). The ip alias created as a part of this process is not removed.
This occurred because we were not cleaning the alias ips in the event of a failure.
As a part of the fix.
1.) moved the function removeDhcpServiceInsubnet and listLastNicsInSubnet to NetworkManager (These were in VirtualMachineManagerImpl earlier.)
2.) add the call to clean ipAlias in the remove nic function of the networkManager.
3.) Modified the removeDhcpServiceInsubnet to take network ad an argument. This will help in removing only the ipAlias which belong to a particular network.
Diffs (updated)
-----
server/src/com/cloud/network/NetworkManager.java dab7a13
server/src/com/cloud/network/NetworkManagerImpl.java ad79a2f
server/src/com/cloud/vm/VirtualMachineManagerImpl.java b33ee49
server/test/com/cloud/network/MockNetworkManagerImpl.java 7f34e55
server/test/com/cloud/vpc/MockNetworkManagerImpl.java 178ebba
Diff: https://reviews.apache.org/r/13323/diff/
Testing
-------
Tested on 4.2.
created two guest networks guest1 and guest2.
created VMS in both the networks.
Added a VM(with no PV drivers) from guest1 to guest2.
this failed and on failure ipAlias configured as part of nic creation was removed.
Deleting the vm causes all the removal of all ipAliases from all the subnets in which this is the lastvm.
Thanks,
bharat kumar
Re: Review Request 13323: if a failure occurs while adding VM to another
network (this should be the first vm in the subnet). The ip alias created
as a part of this process is not removed.
Posted by bharat kumar <bh...@citrix.com>.
> On Aug. 7, 2013, 12:03 a.m., Sheng Yang wrote:
> > server/src/com/cloud/network/NetworkManager.java, line 22
> > <https://reviews.apache.org/r/13323/diff/1/?file=337775#file337775line22>
> >
> > Seems not neceesary.
Hi Sheng,
This was already there. The IDE might have changed something. will fix this.
> On Aug. 7, 2013, 12:03 a.m., Sheng Yang wrote:
> > server/src/com/cloud/network/NetworkManagerImpl.java, line 2433
> > <https://reviews.apache.org/r/13323/diff/1/?file=337776#file337776line2433>
> >
> > It's better to show the semantic of "if this is the last nic in the subnet" in code. Say, if (vm.getType() == Type.User && it's the last nic)
>
> bharat kumar wrote:
> Hi sheng,
> removeDhcpServiceInsubnet functon calls the function listLastNicsInSubnet to get the last nics in subnet and then it removes the DhcpService based on the networkId.
>
> So I mean we do not know if it is last nic in the subnet until we call the listLastNicsInsubnet so we cannot put this check before hand.
>
> Sheng Yang wrote:
> You can wrap it in more meaningful way, e.g. get the check in network manager. The code should be the best comment for itself. In this case, "only remove subnet after last nic unplugged" is not in the if statement, but wrapped in the function. Semantic can be improved.
now i have broken down the code into smaller pieces and improved the semantics.
> On Aug. 7, 2013, 12:03 a.m., Sheng Yang wrote:
> > server/src/com/cloud/vm/VirtualMachineManagerImpl.java, line 479
> > <https://reviews.apache.org/r/13323/diff/1/?file=337777#file337777line479>
> >
> > Why we still need this "if networkid is null then we remove all"? removeNic() for each network in network manager is not enough?
>
> bharat kumar wrote:
> Hi sheng,
>
> In cases when a VM is getting expunged we need to remove the Dhcpservice in all the networks in which this vm has a last nic. so we pass a null as networkId to do this.
>
> Sheng Yang wrote:
> I meant, when vm is expunged, the unplugged function above in network manager wouldn't be called? If it's called, then why you need to check the nics of vm again?
removed the if networkid is null part, as this is called for every nic anyway.
- bharat
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13323/#review24762
-----------------------------------------------------------
On Aug. 7, 2013, 8:53 a.m., bharat kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13323/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2013, 8:53 a.m.)
>
>
> Review request for cloudstack, Alena Prokharchyk and Sheng Yang.
>
>
> Bugs: Cloudstack-4083
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/CLOUDSTACK-4083
> if a failure occurs while adding VM to another network (this should be the first vm in the subnet). The ip alias created as a part of this process is not removed.
>
> This occurred because we were not cleaning the alias ips in the event of a failure.
>
> As a part of the fix.
> 1.) moved the function removeDhcpServiceInsubnet and listLastNicsInSubnet to NetworkManager (These were in VirtualMachineManagerImpl earlier.)
> 2.) add the call to clean ipAlias in the remove nic function of the networkManager.
> 3.) Modified the removeDhcpServiceInsubnet to take network ad an argument. This will help in removing only the ipAlias which belong to a particular network.
>
>
> Diffs
> -----
>
> server/src/com/cloud/network/NetworkManager.java dab7a13
> server/src/com/cloud/network/NetworkManagerImpl.java ad79a2f
> server/src/com/cloud/vm/VirtualMachineManagerImpl.java b33ee49
> server/test/com/cloud/network/MockNetworkManagerImpl.java 7f34e55
> server/test/com/cloud/vpc/MockNetworkManagerImpl.java 178ebba
>
> Diff: https://reviews.apache.org/r/13323/diff/
>
>
> Testing
> -------
>
> Tested on 4.2.
> created two guest networks guest1 and guest2.
> created VMS in both the networks.
> Added a VM(with no PV drivers) from guest1 to guest2.
> this failed and on failure ipAlias configured as part of nic creation was removed.
>
> Deleting the vm causes all the removal of all ipAliases from all the subnets in which this is the lastvm.
>
>
> Thanks,
>
> bharat kumar
>
>
Re: Review Request 13323: if a failure occurs while adding VM to another
network (this should be the first vm in the subnet). The ip alias created
as a part of this process is not removed.
Posted by bharat kumar <bh...@citrix.com>.
> On Aug. 7, 2013, 12:03 a.m., Sheng Yang wrote:
> > server/src/com/cloud/network/NetworkManagerImpl.java, line 2433
> > <https://reviews.apache.org/r/13323/diff/1/?file=337776#file337776line2433>
> >
> > It's better to show the semantic of "if this is the last nic in the subnet" in code. Say, if (vm.getType() == Type.User && it's the last nic)
Hi sheng,
removeDhcpServiceInsubnet functon calls the function listLastNicsInSubnet to get the last nics in subnet and then it removes the DhcpService based on the networkId.
So I mean we do not know if it is last nic in the subnet until we call the listLastNicsInsubnet so we cannot put this check before hand.
> On Aug. 7, 2013, 12:03 a.m., Sheng Yang wrote:
> > server/src/com/cloud/vm/VirtualMachineManagerImpl.java, line 479
> > <https://reviews.apache.org/r/13323/diff/1/?file=337777#file337777line479>
> >
> > Why we still need this "if networkid is null then we remove all"? removeNic() for each network in network manager is not enough?
Hi sheng,
In cases when a VM is getting expunged we need to remove the Dhcpservice in all the networks in which this vm has a last nic. so we pass a null as networkId to do this.
- bharat
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13323/#review24762
-----------------------------------------------------------
On Aug. 6, 2013, 12:48 p.m., bharat kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13323/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2013, 12:48 p.m.)
>
>
> Review request for cloudstack, Alena Prokharchyk and Sheng Yang.
>
>
> Bugs: Cloudstack-4083
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/CLOUDSTACK-4083
> if a failure occurs while adding VM to another network (this should be the first vm in the subnet). The ip alias created as a part of this process is not removed.
>
> This occurred because we were not cleaning the alias ips in the event of a failure.
>
> As a part of the fix.
> 1.) moved the function removeDhcpServiceInsubnet and listLastNicsInSubnet to NetworkManager (These were in VirtualMachineManagerImpl earlier.)
> 2.) add the call to clean ipAlias in the remove nic function of the networkManager.
> 3.) Modified the removeDhcpServiceInsubnet to take network ad an argument. This will help in removing only the ipAlias which belong to a particular network.
>
>
> Diffs
> -----
>
> server/src/com/cloud/network/NetworkManager.java dab7a13
> server/src/com/cloud/network/NetworkManagerImpl.java effee96
> server/src/com/cloud/vm/VirtualMachineManagerImpl.java b33ee49
> server/test/com/cloud/network/MockNetworkManagerImpl.java 7f34e55
> server/test/com/cloud/vpc/MockNetworkManagerImpl.java 178ebba
>
> Diff: https://reviews.apache.org/r/13323/diff/
>
>
> Testing
> -------
>
> Tested on 4.2.
> created two guest networks guest1 and guest2.
> created VMS in both the networks.
> Added a VM(with no PV drivers) from guest1 to guest2.
> this failed and on failure ipAlias configured as part of nic creation was removed.
>
> Deleting the vm causes all the removal of all ipAliases from all the subnets in which this is the lastvm.
>
>
> Thanks,
>
> bharat kumar
>
>
Re: Review Request 13323: if a failure occurs while adding VM to another
network (this should be the first vm in the subnet). The ip alias created
as a part of this process is not removed.
Posted by Sheng Yang <sh...@yasker.org>.
> On Aug. 7, 2013, 12:03 a.m., Sheng Yang wrote:
> > server/src/com/cloud/network/NetworkManagerImpl.java, line 2433
> > <https://reviews.apache.org/r/13323/diff/1/?file=337776#file337776line2433>
> >
> > It's better to show the semantic of "if this is the last nic in the subnet" in code. Say, if (vm.getType() == Type.User && it's the last nic)
>
> bharat kumar wrote:
> Hi sheng,
> removeDhcpServiceInsubnet functon calls the function listLastNicsInSubnet to get the last nics in subnet and then it removes the DhcpService based on the networkId.
>
> So I mean we do not know if it is last nic in the subnet until we call the listLastNicsInsubnet so we cannot put this check before hand.
You can wrap it in more meaningful way, e.g. get the check in network manager. The code should be the best comment for itself. In this case, "only remove subnet after last nic unplugged" is not in the if statement, but wrapped in the function. Semantic can be improved.
> On Aug. 7, 2013, 12:03 a.m., Sheng Yang wrote:
> > server/src/com/cloud/vm/VirtualMachineManagerImpl.java, line 479
> > <https://reviews.apache.org/r/13323/diff/1/?file=337777#file337777line479>
> >
> > Why we still need this "if networkid is null then we remove all"? removeNic() for each network in network manager is not enough?
>
> bharat kumar wrote:
> Hi sheng,
>
> In cases when a VM is getting expunged we need to remove the Dhcpservice in all the networks in which this vm has a last nic. so we pass a null as networkId to do this.
I meant, when vm is expunged, the unplugged function above in network manager wouldn't be called? If it's called, then why you need to check the nics of vm again?
- Sheng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13323/#review24762
-----------------------------------------------------------
On Aug. 6, 2013, 12:48 p.m., bharat kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13323/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2013, 12:48 p.m.)
>
>
> Review request for cloudstack, Alena Prokharchyk and Sheng Yang.
>
>
> Bugs: Cloudstack-4083
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/CLOUDSTACK-4083
> if a failure occurs while adding VM to another network (this should be the first vm in the subnet). The ip alias created as a part of this process is not removed.
>
> This occurred because we were not cleaning the alias ips in the event of a failure.
>
> As a part of the fix.
> 1.) moved the function removeDhcpServiceInsubnet and listLastNicsInSubnet to NetworkManager (These were in VirtualMachineManagerImpl earlier.)
> 2.) add the call to clean ipAlias in the remove nic function of the networkManager.
> 3.) Modified the removeDhcpServiceInsubnet to take network ad an argument. This will help in removing only the ipAlias which belong to a particular network.
>
>
> Diffs
> -----
>
> server/src/com/cloud/network/NetworkManager.java dab7a13
> server/src/com/cloud/network/NetworkManagerImpl.java effee96
> server/src/com/cloud/vm/VirtualMachineManagerImpl.java b33ee49
> server/test/com/cloud/network/MockNetworkManagerImpl.java 7f34e55
> server/test/com/cloud/vpc/MockNetworkManagerImpl.java 178ebba
>
> Diff: https://reviews.apache.org/r/13323/diff/
>
>
> Testing
> -------
>
> Tested on 4.2.
> created two guest networks guest1 and guest2.
> created VMS in both the networks.
> Added a VM(with no PV drivers) from guest1 to guest2.
> this failed and on failure ipAlias configured as part of nic creation was removed.
>
> Deleting the vm causes all the removal of all ipAliases from all the subnets in which this is the lastvm.
>
>
> Thanks,
>
> bharat kumar
>
>
Re: Review Request 13323: if a failure occurs while adding VM to another
network (this should be the first vm in the subnet). The ip alias created
as a part of this process is not removed.
Posted by Sheng Yang <sh...@yasker.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13323/#review24762
-----------------------------------------------------------
server/src/com/cloud/network/NetworkManager.java
<https://reviews.apache.org/r/13323/#comment48882>
Seems not neceesary.
server/src/com/cloud/network/NetworkManagerImpl.java
<https://reviews.apache.org/r/13323/#comment48887>
It's better to show the semantic of "if this is the last nic in the subnet" in code. Say, if (vm.getType() == Type.User && it's the last nic)
server/src/com/cloud/network/NetworkManagerImpl.java
<https://reviews.apache.org/r/13323/#comment48885>
Uppercase "Subnet".
server/src/com/cloud/vm/VirtualMachineManagerImpl.java
<https://reviews.apache.org/r/13323/#comment48886>
Why we still need this "if networkid is null then we remove all"? removeNic() for each network in network manager is not enough?
- Sheng Yang
On Aug. 6, 2013, 12:48 p.m., bharat kumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13323/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2013, 12:48 p.m.)
>
>
> Review request for cloudstack, Alena Prokharchyk and Sheng Yang.
>
>
> Bugs: Cloudstack-4083
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/CLOUDSTACK-4083
> if a failure occurs while adding VM to another network (this should be the first vm in the subnet). The ip alias created as a part of this process is not removed.
>
> This occurred because we were not cleaning the alias ips in the event of a failure.
>
> As a part of the fix.
> 1.) moved the function removeDhcpServiceInsubnet and listLastNicsInSubnet to NetworkManager (These were in VirtualMachineManagerImpl earlier.)
> 2.) add the call to clean ipAlias in the remove nic function of the networkManager.
> 3.) Modified the removeDhcpServiceInsubnet to take network ad an argument. This will help in removing only the ipAlias which belong to a particular network.
>
>
> Diffs
> -----
>
> server/src/com/cloud/network/NetworkManager.java dab7a13
> server/src/com/cloud/network/NetworkManagerImpl.java effee96
> server/src/com/cloud/vm/VirtualMachineManagerImpl.java b33ee49
> server/test/com/cloud/network/MockNetworkManagerImpl.java 7f34e55
> server/test/com/cloud/vpc/MockNetworkManagerImpl.java 178ebba
>
> Diff: https://reviews.apache.org/r/13323/diff/
>
>
> Testing
> -------
>
> Tested on 4.2.
> created two guest networks guest1 and guest2.
> created VMS in both the networks.
> Added a VM(with no PV drivers) from guest1 to guest2.
> this failed and on failure ipAlias configured as part of nic creation was removed.
>
> Deleting the vm causes all the removal of all ipAliases from all the subnets in which this is the lastvm.
>
>
> Thanks,
>
> bharat kumar
>
>