cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "bharat kumar" <bharat.ku...@citrix.com>
Subject 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.
Date Wed, 07 Aug 2013 08:54:42 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message