cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sheng Yang" <sh...@yasker.org>
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 06:48:49 GMT


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


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