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