From 1913c6854e357842db79ac74a62c2effc4f0f9f4 Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Mon, 1 Feb 2021 09:44:20 +0100 Subject: [PATCH] server: keep networks order and ips while move a vm with multiple networks (#4602) This PR fixes an issue when move a vm from an account to another account. Steps to reproduce the issue (1) create a vm with multiple shared networks (in advanced zone, or advanced zone with security groups) (2) create another account (in same domain who can also access the shared networks) (3) move vm to new account, with a list of networkid expected result: the vm has nics on the networks in same order as specified in API request, and nics have the same ips as before actual result: network order is not same as specified, ips are changed. --- .../service/NetworkOrchestrationService.java | 2 +- .../orchestration/NetworkOrchestrator.java | 6 +- .../java/com/cloud/vm/UserVmManagerImpl.java | 96 ++++++++++++------- .../com/cloud/vpc/MockNetworkManagerImpl.java | 4 +- 4 files changed, 69 insertions(+), 39 deletions(-) diff --git a/engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/NetworkOrchestrationService.java b/engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/NetworkOrchestrationService.java index 1e8135460ab..183606d5cc6 100644 --- a/engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/NetworkOrchestrationService.java +++ b/engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/NetworkOrchestrationService.java @@ -129,7 +129,7 @@ public interface NetworkOrchestrationService { void cleanupNics(VirtualMachineProfile vm); - void expungeNics(VirtualMachineProfile vm); + void removeNics(VirtualMachineProfile vm); List getNicProfiles(VirtualMachine vm); diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java index 5cf195ea0b9..363d38eebd0 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java @@ -2168,10 +2168,10 @@ public class NetworkOrchestrator extends ManagerBase implements NetworkOrchestra } @Override - public void expungeNics(final VirtualMachineProfile vm) { - final List nics = _nicDao.listByVmIdIncludingRemoved(vm.getId()); + public void removeNics(final VirtualMachineProfile vm) { + final List nics = _nicDao.listByVmId(vm.getId()); for (final NicVO nic : nics) { - _nicDao.expunge(nic.getId()); + _nicDao.remove(nic.getId()); } } diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 45782d9d4cc..adf00ba1935 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -24,8 +24,8 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Date; import java.util.HashMap; -import java.util.HashSet; import java.util.LinkedHashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -6221,7 +6221,7 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir _securityGroupMgr.removeInstanceFromGroups(cmd.getVmId()); // cleanup the network for the oldOwner _networkMgr.cleanupNics(vmOldProfile); - _networkMgr.expungeNics(vmOldProfile); + _networkMgr.removeNics(vmOldProfile); // security groups will be recreated for the new account, when the // VM is started List networkList = new ArrayList(); @@ -6283,34 +6283,25 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir s_logger.debug("AssignVM: Basic zone, adding security groups no " + securityGroupIdList.size() + " to " + vm.getInstanceName()); } else { + Set applicableNetworks = new LinkedHashSet<>(); + Map requestedIPv4ForNics = new HashMap<>(); + Map requestedIPv6ForNics = new HashMap<>(); if (zone.isSecurityGroupEnabled()) { // advanced zone with security groups // cleanup the old security groups _securityGroupMgr.removeInstanceFromGroups(cmd.getVmId()); - - Set applicableNetworks = new HashSet(); - String requestedIPv4ForDefaultNic = null; - String requestedIPv6ForDefaultNic = null; // if networkIdList is null and the first network of vm is shared network, then keep it if possible if (networkIdList == null || networkIdList.isEmpty()) { NicVO defaultNicOld = _nicDao.findDefaultNicForVM(vm.getId()); if (defaultNicOld != null) { NetworkVO defaultNetworkOld = _networkDao.findById(defaultNicOld.getNetworkId()); - if (defaultNetworkOld != null && defaultNetworkOld.getGuestType() == Network.GuestType.Shared && defaultNetworkOld.getAclType() == ACLType.Domain) { - try { - _networkModel.checkNetworkPermissions(newAccount, defaultNetworkOld); - applicableNetworks.add(defaultNetworkOld); - requestedIPv4ForDefaultNic = defaultNicOld.getIPv4Address(); - requestedIPv6ForDefaultNic = defaultNicOld.getIPv6Address(); - s_logger.debug("AssignVM: use old shared network " + defaultNetworkOld.getName() + " with old ip " + requestedIPv4ForDefaultNic + " on default nic of vm:" + vm.getInstanceName()); - } catch (PermissionDeniedException e) { - s_logger.debug("AssignVM: the shared network on old default nic can not be applied to new account"); - } + if (canAccountUseNetwork(newAccount, defaultNetworkOld)) { + applicableNetworks.add(defaultNetworkOld); + requestedIPv4ForNics.put(defaultNetworkOld.getId(), defaultNicOld.getIPv4Address()); + requestedIPv6ForNics.put(defaultNetworkOld.getId(), defaultNicOld.getIPv6Address()); + s_logger.debug("AssignVM: use old shared network " + defaultNetworkOld.getName() + " with old ip " + defaultNicOld.getIPv4Address() + " on default nic of vm:" + vm.getInstanceName()); } } } - // cleanup the network for the oldOwner - _networkMgr.cleanupNics(vmOldProfile); - _networkMgr.expungeNics(vmOldProfile); if (networkIdList != null && !networkIdList.isEmpty()) { // add any additional networks @@ -6333,10 +6324,24 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir ex.addProxyObject(network.getUuid(), "networkId"); throw ex; } + + if (network.getGuestType() == Network.GuestType.Shared && network.getAclType() == ACLType.Domain) { + NicVO nicOld = _nicDao.findByNtwkIdAndInstanceId(network.getId(), vm.getId()); + if (nicOld != null) { + requestedIPv4ForNics.put(network.getId(), nicOld.getIPv4Address()); + requestedIPv6ForNics.put(network.getId(), nicOld.getIPv6Address()); + s_logger.debug("AssignVM: use old shared network " + network.getName() + " with old ip " + nicOld.getIPv4Address() + " on nic of vm:" + vm.getInstanceName()); + } + } + s_logger.debug("AssignVM: Added network " + network.getName() + " to vm " + vm.getId()); applicableNetworks.add(network); } } + // cleanup the network for the oldOwner + _networkMgr.cleanupNics(vmOldProfile); + _networkMgr.removeNics(vmOldProfile); + // add the new nics LinkedHashMap> networks = new LinkedHashMap>(); int toggle = 0; @@ -6345,11 +6350,12 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir NicProfile defaultNic = new NicProfile(); if (toggle == 0) { defaultNic.setDefaultNic(true); - defaultNic.setRequestedIPv4(requestedIPv4ForDefaultNic); - defaultNic.setRequestedIPv6(requestedIPv6ForDefaultNic); defaultNetwork = appNet; toggle++; } + + defaultNic.setRequestedIPv4(requestedIPv4ForNics.get(appNet.getId())); + defaultNic.setRequestedIPv6(requestedIPv6ForNics.get(appNet.getId())); networks.put(appNet, new ArrayList(Arrays.asList(defaultNic))); } @@ -6412,27 +6418,20 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir if (securityGroupIdList != null && !securityGroupIdList.isEmpty()) { throw new InvalidParameterValueException("Can't move vm with security groups; security group feature is not enabled in this zone"); } - Set applicableNetworks = new HashSet(); // if networkIdList is null and the first network of vm is shared network, then keep it if possible if (networkIdList == null || networkIdList.isEmpty()) { NicVO defaultNicOld = _nicDao.findDefaultNicForVM(vm.getId()); if (defaultNicOld != null) { NetworkVO defaultNetworkOld = _networkDao.findById(defaultNicOld.getNetworkId()); - if (defaultNetworkOld != null && defaultNetworkOld.getGuestType() == Network.GuestType.Shared && defaultNetworkOld.getAclType() == ACLType.Domain) { - try { - _networkModel.checkNetworkPermissions(newAccount, defaultNetworkOld); - applicableNetworks.add(defaultNetworkOld); - } catch (PermissionDeniedException e) { - s_logger.debug("AssignVM: the shared network on old default nic can not be applied to new account"); - } + if (canAccountUseNetwork(newAccount, defaultNetworkOld)) { + applicableNetworks.add(defaultNetworkOld); + requestedIPv4ForNics.put(defaultNetworkOld.getId(), defaultNicOld.getIPv4Address()); + requestedIPv6ForNics.put(defaultNetworkOld.getId(), defaultNicOld.getIPv6Address()); + s_logger.debug("AssignVM: use old shared network " + defaultNetworkOld.getName() + " with old ip " + defaultNicOld.getIPv4Address() + " on default nic of vm:" + vm.getInstanceName()); } } } - // cleanup the network for the oldOwner - _networkMgr.cleanupNics(vmOldProfile); - _networkMgr.expungeNics(vmOldProfile); - if (networkIdList != null && !networkIdList.isEmpty()) { // add any additional networks for (Long networkId : networkIdList) { @@ -6452,6 +6451,16 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir ex.addProxyObject(network.getUuid(), "networkId"); throw ex; } + + if (network.getGuestType() == Network.GuestType.Shared && network.getAclType() == ACLType.Domain) { + NicVO nicOld = _nicDao.findByNtwkIdAndInstanceId(network.getId(), vm.getId()); + if (nicOld != null) { + requestedIPv4ForNics.put(network.getId(), nicOld.getIPv4Address()); + requestedIPv6ForNics.put(network.getId(), nicOld.getIPv6Address()); + s_logger.debug("AssignVM: use old shared network " + network.getName() + " with old ip " + nicOld.getIPv4Address() + " on nic of vm:" + vm.getInstanceName()); + } + } + s_logger.debug("AssignVM: Added network " + network.getName() + " to vm " + vm.getId()); applicableNetworks.add(network); } } else if (applicableNetworks.isEmpty()) { @@ -6515,6 +6524,10 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir applicableNetworks.add(defaultNetwork); } + // cleanup the network for the oldOwner + _networkMgr.cleanupNics(vmOldProfile); + _networkMgr.removeNics(vmOldProfile); + // add the new nics LinkedHashMap> networks = new LinkedHashMap>(); int toggle = 0; @@ -6524,6 +6537,8 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir defaultNic.setDefaultNic(true); toggle++; } + defaultNic.setRequestedIPv4(requestedIPv4ForNics.get(appNet.getId())); + defaultNic.setRequestedIPv6(requestedIPv6ForNics.get(appNet.getId())); networks.put(appNet, new ArrayList(Arrays.asList(defaultNic))); } VirtualMachine vmi = _itMgr.findById(vm.getId()); @@ -6536,6 +6551,21 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir return vm; } + private boolean canAccountUseNetwork(Account newAccount, Network network) { + if (network != null && network.getAclType() == ACLType.Domain + && (network.getGuestType() == Network.GuestType.Shared + || network.getGuestType() == Network.GuestType.L2)) { + try { + _networkModel.checkNetworkPermissions(newAccount, network); + return true; + } catch (PermissionDeniedException e) { + s_logger.debug(String.format("AssignVM: %s network %s can not be used by new account %s", network.getGuestType(), network.getName(), newAccount.getAccountName())); + return false; + } + } + return false; + } + @Override public UserVm restoreVM(RestoreVMCmd cmd) throws InsufficientCapacityException, ResourceUnavailableException { // Input validation diff --git a/server/src/test/java/com/cloud/vpc/MockNetworkManagerImpl.java b/server/src/test/java/com/cloud/vpc/MockNetworkManagerImpl.java index c962d90d2dd..0cc10ea7bb5 100644 --- a/server/src/test/java/com/cloud/vpc/MockNetworkManagerImpl.java +++ b/server/src/test/java/com/cloud/vpc/MockNetworkManagerImpl.java @@ -580,10 +580,10 @@ public class MockNetworkManagerImpl extends ManagerBase implements NetworkOrches } /* (non-Javadoc) - * @see com.cloud.network.NetworkManager#expungeNics(com.cloud.vm.VirtualMachineProfile) + * @see com.cloud.network.NetworkManager#removeNics(com.cloud.vm.VirtualMachineProfile) */ @Override - public void expungeNics(VirtualMachineProfile vm) { + public void removeNics(VirtualMachineProfile vm) { // TODO Auto-generated method stub }