Skip to content

Commit 1913c68

Browse files
ustcweizhouustcweizhou
authored
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.
1 parent 890e847 commit 1913c68

4 files changed

Lines changed: 69 additions & 39 deletions

File tree

‎engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/NetworkOrchestrationService.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ void prepare(VirtualMachineProfile profile, DeployDestination dest, ReservationC
129129

130130
void cleanupNics(VirtualMachineProfile vm);
131131

132-
void expungeNics(VirtualMachineProfile vm);
132+
void removeNics(VirtualMachineProfile vm);
133133

134134
List<NicProfile> getNicProfiles(VirtualMachine vm);
135135

‎engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2168,10 +2168,10 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
21682168
}
21692169

21702170
@Override
2171-
public void expungeNics(final VirtualMachineProfile vm) {
2172-
final List<NicVO> nics = _nicDao.listByVmIdIncludingRemoved(vm.getId());
2171+
public void removeNics(final VirtualMachineProfile vm) {
2172+
final List<NicVO> nics = _nicDao.listByVmId(vm.getId());
21732173
for (final NicVO nic : nics) {
2174-
_nicDao.expunge(nic.getId());
2174+
_nicDao.remove(nic.getId());
21752175
}
21762176
}
21772177

‎server/src/main/java/com/cloud/vm/UserVmManagerImpl.java‎

Lines changed: 63 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,8 @@
2424
import java.util.Arrays;
2525
import java.util.Date;
2626
import java.util.HashMap;
27-
import java.util.HashSet;
2827
import java.util.LinkedHashMap;
28+
import java.util.LinkedHashSet;
2929
import java.util.List;
3030
import java.util.Map;
3131
import java.util.Map.Entry;
@@ -6221,7 +6221,7 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
62216221
_securityGroupMgr.removeInstanceFromGroups(cmd.getVmId());
62226222
// cleanup the network for the oldOwner
62236223
_networkMgr.cleanupNics(vmOldProfile);
6224-
_networkMgr.expungeNics(vmOldProfile);
6224+
_networkMgr.removeNics(vmOldProfile);
62256225
// security groups will be recreated for the new account, when the
62266226
// VM is started
62276227
List<NetworkVO> networkList = new ArrayList<NetworkVO>();
@@ -6283,34 +6283,25 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
62836283

62846284
s_logger.debug("AssignVM: Basic zone, adding security groups no " + securityGroupIdList.size() + " to " + vm.getInstanceName());
62856285
} else {
6286+
Set<NetworkVO> applicableNetworks = new LinkedHashSet<>();
6287+
Map<Long, String> requestedIPv4ForNics = new HashMap<>();
6288+
Map<Long, String> requestedIPv6ForNics = new HashMap<>();
62866289
if (zone.isSecurityGroupEnabled()) { // advanced zone with security groups
62876290
// cleanup the old security groups
62886291
_securityGroupMgr.removeInstanceFromGroups(cmd.getVmId());
6289-
6290-
Set<NetworkVO> applicableNetworks = new HashSet<NetworkVO>();
6291-
String requestedIPv4ForDefaultNic = null;
6292-
String requestedIPv6ForDefaultNic = null;
62936292
// if networkIdList is null and the first network of vm is shared network, then keep it if possible
62946293
if (networkIdList == null || networkIdList.isEmpty()) {
62956294
NicVO defaultNicOld = _nicDao.findDefaultNicForVM(vm.getId());
62966295
if (defaultNicOld != null) {
62976296
NetworkVO defaultNetworkOld = _networkDao.findById(defaultNicOld.getNetworkId());
6298-
if (defaultNetworkOld != null && defaultNetworkOld.getGuestType() == Network.GuestType.Shared && defaultNetworkOld.getAclType() == ACLType.Domain) {
6299-
try {
6300-
_networkModel.checkNetworkPermissions(newAccount, defaultNetworkOld);
6301-
applicableNetworks.add(defaultNetworkOld);
6302-
requestedIPv4ForDefaultNic = defaultNicOld.getIPv4Address();
6303-
requestedIPv6ForDefaultNic = defaultNicOld.getIPv6Address();
6304-
s_logger.debug("AssignVM: use old shared network " + defaultNetworkOld.getName() + " with old ip " + requestedIPv4ForDefaultNic + " on default nic of vm:" + vm.getInstanceName());
6305-
} catch (PermissionDeniedException e) {
6306-
s_logger.debug("AssignVM: the shared network on old default nic can not be applied to new account");
6307-
}
6297+
if (canAccountUseNetwork(newAccount, defaultNetworkOld)) {
6298+
applicableNetworks.add(defaultNetworkOld);
6299+
requestedIPv4ForNics.put(defaultNetworkOld.getId(), defaultNicOld.getIPv4Address());
6300+
requestedIPv6ForNics.put(defaultNetworkOld.getId(), defaultNicOld.getIPv6Address());
6301+
s_logger.debug("AssignVM: use old shared network " + defaultNetworkOld.getName() + " with old ip " + defaultNicOld.getIPv4Address() + " on default nic of vm:" + vm.getInstanceName());
63086302
}
63096303
}
63106304
}
6311-
// cleanup the network for the oldOwner
6312-
_networkMgr.cleanupNics(vmOldProfile);
6313-
_networkMgr.expungeNics(vmOldProfile);
63146305

63156306
if (networkIdList != null && !networkIdList.isEmpty()) {
63166307
// add any additional networks
@@ -6333,10 +6324,24 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
63336324
ex.addProxyObject(network.getUuid(), "networkId");
63346325
throw ex;
63356326
}
6327+
6328+
if (network.getGuestType() == Network.GuestType.Shared && network.getAclType() == ACLType.Domain) {
6329+
NicVO nicOld = _nicDao.findByNtwkIdAndInstanceId(network.getId(), vm.getId());
6330+
if (nicOld != null) {
6331+
requestedIPv4ForNics.put(network.getId(), nicOld.getIPv4Address());
6332+
requestedIPv6ForNics.put(network.getId(), nicOld.getIPv6Address());
6333+
s_logger.debug("AssignVM: use old shared network " + network.getName() + " with old ip " + nicOld.getIPv4Address() + " on nic of vm:" + vm.getInstanceName());
6334+
}
6335+
}
6336+
s_logger.debug("AssignVM: Added network " + network.getName() + " to vm " + vm.getId());
63366337
applicableNetworks.add(network);
63376338
}
63386339
}
63396340

6341+
// cleanup the network for the oldOwner
6342+
_networkMgr.cleanupNics(vmOldProfile);
6343+
_networkMgr.removeNics(vmOldProfile);
6344+
63406345
// add the new nics
63416346
LinkedHashMap<Network, List<? extends NicProfile>> networks = new LinkedHashMap<Network, List<? extends NicProfile>>();
63426347
int toggle = 0;
@@ -6345,11 +6350,12 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
63456350
NicProfile defaultNic = new NicProfile();
63466351
if (toggle == 0) {
63476352
defaultNic.setDefaultNic(true);
6348-
defaultNic.setRequestedIPv4(requestedIPv4ForDefaultNic);
6349-
defaultNic.setRequestedIPv6(requestedIPv6ForDefaultNic);
63506353
defaultNetwork = appNet;
63516354
toggle++;
63526355
}
6356+
6357+
defaultNic.setRequestedIPv4(requestedIPv4ForNics.get(appNet.getId()));
6358+
defaultNic.setRequestedIPv6(requestedIPv6ForNics.get(appNet.getId()));
63536359
networks.put(appNet, new ArrayList<NicProfile>(Arrays.asList(defaultNic)));
63546360

63556361
}
@@ -6412,27 +6418,20 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
64126418
if (securityGroupIdList != null && !securityGroupIdList.isEmpty()) {
64136419
throw new InvalidParameterValueException("Can't move vm with security groups; security group feature is not enabled in this zone");
64146420
}
6415-
Set<NetworkVO> applicableNetworks = new HashSet<NetworkVO>();
64166421
// if networkIdList is null and the first network of vm is shared network, then keep it if possible
64176422
if (networkIdList == null || networkIdList.isEmpty()) {
64186423
NicVO defaultNicOld = _nicDao.findDefaultNicForVM(vm.getId());
64196424
if (defaultNicOld != null) {
64206425
NetworkVO defaultNetworkOld = _networkDao.findById(defaultNicOld.getNetworkId());
6421-
if (defaultNetworkOld != null && defaultNetworkOld.getGuestType() == Network.GuestType.Shared && defaultNetworkOld.getAclType() == ACLType.Domain) {
6422-
try {
6423-
_networkModel.checkNetworkPermissions(newAccount, defaultNetworkOld);
6424-
applicableNetworks.add(defaultNetworkOld);
6425-
} catch (PermissionDeniedException e) {
6426-
s_logger.debug("AssignVM: the shared network on old default nic can not be applied to new account");
6427-
}
6426+
if (canAccountUseNetwork(newAccount, defaultNetworkOld)) {
6427+
applicableNetworks.add(defaultNetworkOld);
6428+
requestedIPv4ForNics.put(defaultNetworkOld.getId(), defaultNicOld.getIPv4Address());
6429+
requestedIPv6ForNics.put(defaultNetworkOld.getId(), defaultNicOld.getIPv6Address());
6430+
s_logger.debug("AssignVM: use old shared network " + defaultNetworkOld.getName() + " with old ip " + defaultNicOld.getIPv4Address() + " on default nic of vm:" + vm.getInstanceName());
64286431
}
64296432
}
64306433
}
64316434

6432-
// cleanup the network for the oldOwner
6433-
_networkMgr.cleanupNics(vmOldProfile);
6434-
_networkMgr.expungeNics(vmOldProfile);
6435-
64366435
if (networkIdList != null && !networkIdList.isEmpty()) {
64376436
// add any additional networks
64386437
for (Long networkId : networkIdList) {
@@ -6452,6 +6451,16 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
64526451
ex.addProxyObject(network.getUuid(), "networkId");
64536452
throw ex;
64546453
}
6454+
6455+
if (network.getGuestType() == Network.GuestType.Shared && network.getAclType() == ACLType.Domain) {
6456+
NicVO nicOld = _nicDao.findByNtwkIdAndInstanceId(network.getId(), vm.getId());
6457+
if (nicOld != null) {
6458+
requestedIPv4ForNics.put(network.getId(), nicOld.getIPv4Address());
6459+
requestedIPv6ForNics.put(network.getId(), nicOld.getIPv6Address());
6460+
s_logger.debug("AssignVM: use old shared network " + network.getName() + " with old ip " + nicOld.getIPv4Address() + " on nic of vm:" + vm.getInstanceName());
6461+
}
6462+
}
6463+
s_logger.debug("AssignVM: Added network " + network.getName() + " to vm " + vm.getId());
64556464
applicableNetworks.add(network);
64566465
}
64576466
} else if (applicableNetworks.isEmpty()) {
@@ -6515,6 +6524,10 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
65156524
applicableNetworks.add(defaultNetwork);
65166525
}
65176526

6527+
// cleanup the network for the oldOwner
6528+
_networkMgr.cleanupNics(vmOldProfile);
6529+
_networkMgr.removeNics(vmOldProfile);
6530+
65186531
// add the new nics
65196532
LinkedHashMap<Network, List<? extends NicProfile>> networks = new LinkedHashMap<Network, List<? extends NicProfile>>();
65206533
int toggle = 0;
@@ -6524,6 +6537,8 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
65246537
defaultNic.setDefaultNic(true);
65256538
toggle++;
65266539
}
6540+
defaultNic.setRequestedIPv4(requestedIPv4ForNics.get(appNet.getId()));
6541+
defaultNic.setRequestedIPv6(requestedIPv6ForNics.get(appNet.getId()));
65276542
networks.put(appNet, new ArrayList<NicProfile>(Arrays.asList(defaultNic)));
65286543
}
65296544
VirtualMachine vmi = _itMgr.findById(vm.getId());
@@ -6536,6 +6551,21 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
65366551
return vm;
65376552
}
65386553

6554+
private boolean canAccountUseNetwork(Account newAccount, Network network) {
6555+
if (network != null && network.getAclType() == ACLType.Domain
6556+
&& (network.getGuestType() == Network.GuestType.Shared
6557+
|| network.getGuestType() == Network.GuestType.L2)) {
6558+
try {
6559+
_networkModel.checkNetworkPermissions(newAccount, network);
6560+
return true;
6561+
} catch (PermissionDeniedException e) {
6562+
s_logger.debug(String.format("AssignVM: %s network %s can not be used by new account %s", network.getGuestType(), network.getName(), newAccount.getAccountName()));
6563+
return false;
6564+
}
6565+
}
6566+
return false;
6567+
}
6568+
65396569
@Override
65406570
public UserVm restoreVM(RestoreVMCmd cmd) throws InsufficientCapacityException, ResourceUnavailableException {
65416571
// Input validation

‎server/src/test/java/com/cloud/vpc/MockNetworkManagerImpl.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -580,10 +580,10 @@ public void cleanupNics(VirtualMachineProfile vm) {
580580
}
581581

582582
/* (non-Javadoc)
583-
* @see com.cloud.network.NetworkManager#expungeNics(com.cloud.vm.VirtualMachineProfile)
583+
* @see com.cloud.network.NetworkManager#removeNics(com.cloud.vm.VirtualMachineProfile)
584584
*/
585585
@Override
586-
public void expungeNics(VirtualMachineProfile vm) {
586+
public void removeNics(VirtualMachineProfile vm) {
587587
// TODO Auto-generated method stub
588588

589589
}

0 commit comments

Comments
 (0)