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