From 462ad5cfc2dc44ddd7b789f9208b227c8ae4f296 Mon Sep 17 00:00:00 2001 From: Abhinandan Prateek Date: Tue, 20 Dec 2011 18:31:37 +0530 Subject: [PATCH] bug 8962: network related improvements --- .../com/cloud/api/commands/AssignVMCmd.java | 13 ++ .../xen/resource/CitrixResourceBase.java | 30 +-- core/src/com/cloud/vm/UserVmVO.java | 4 + .../src/com/cloud/vm/UserVmManagerImpl.java | 174 +++++++++++++----- 4 files changed, 156 insertions(+), 65 deletions(-) diff --git a/api/src/com/cloud/api/commands/AssignVMCmd.java b/api/src/com/cloud/api/commands/AssignVMCmd.java index a89e95580b6..51b06cd3e2e 100644 --- a/api/src/com/cloud/api/commands/AssignVMCmd.java +++ b/api/src/com/cloud/api/commands/AssignVMCmd.java @@ -18,6 +18,9 @@ package com.cloud.api.commands; +import java.util.ArrayList; +import java.util.List; + import org.apache.log4j.Logger; import com.cloud.api.ApiConstants; @@ -32,6 +35,7 @@ import com.cloud.api.response.UserVmResponse; import com.cloud.api.response.ZoneResponse; import com.cloud.async.AsyncJob; import com.cloud.event.EventTypes; +import com.cloud.exception.InvalidParameterValueException; import com.cloud.user.Account; import com.cloud.uservm.UserVm; @@ -54,7 +58,12 @@ public class AssignVMCmd extends BaseCmd { @Parameter(name=ApiConstants.DOMAIN_ID, type=CommandType.LONG, description="an optional domainId for the vpn user. If the account parameter is used, domainId must also be used.") private Long domainId; + //Network information + @IdentityMapper(entityTableName="networks") + @Parameter(name=ApiConstants.NETWORK_IDS, type=CommandType.LIST, collectionType=CommandType.LONG, description="list of network ids that will be part of VM network after move") + private List networkIds; + ///////////////////////////////////////////////////// /////////////////// Accessors /////////////////////// ///////////////////////////////////////////////////// @@ -70,6 +79,10 @@ public class AssignVMCmd extends BaseCmd { public Long getDomainId() { return domainId; } + + public List getNetworkIds() { + return networkIds; + } ///////////////////////////////////////////////////// /////////////// API Implementation/////////////////// diff --git a/core/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java b/core/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java index 2dffda82720..15825ab8ff9 100755 --- a/core/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java +++ b/core/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java @@ -253,7 +253,7 @@ public abstract class CitrixResourceBase implements ServerResource, HypervisorRe protected long _dcId; protected String _pod; protected String _cluster; - protected static final XenServerPoolVms s_vms = new XenServerPoolVms(); + private static final XenServerPoolVms s_vms = new XenServerPoolVms(); protected String _privateNetworkName; protected String _linkLocalPrivateNetworkName; protected String _publicNetworkName; @@ -1081,7 +1081,7 @@ public abstract class CitrixResourceBase implements ServerResource, HypervisorRe } } } - s_logger.debug("The VM " + vmName + " is in Starting state."); + s_logger.debug("1. The VM " + vmName + " is in Starting state."); s_vms.put(_cluster, _name, vmName, State.Starting); Host host = Host.getByUuid(conn, _host.uuid); @@ -1164,7 +1164,7 @@ public abstract class CitrixResourceBase implements ServerResource, HypervisorRe } finally { synchronized (s_vms) { if (state != State.Stopped) { - s_logger.debug("The VM " + vmName + " is in " + state + " state."); + s_logger.debug("2. The VM " + vmName + " is in " + state + " state."); s_vms.put(_cluster, _name, vmName, state); } else { s_logger.debug("The VM is in stopped state, detected problem during startup : " + vmName); @@ -2168,7 +2168,7 @@ public abstract class CitrixResourceBase implements ServerResource, HypervisorRe Integer vncPort = null; if (state == State.Running) { synchronized (s_vms) { - s_logger.debug("The VM " + vmName + " is in " + State.Running + " state"); + s_logger.debug("3. The VM " + vmName + " is in " + State.Running + " state"); s_vms.put(_cluster, _name, vmName, State.Running); } } @@ -2191,7 +2191,7 @@ public abstract class CitrixResourceBase implements ServerResource, HypervisorRe for (NicTO nic : nics) { getNetwork(conn, nic); } - s_logger.debug("The VM " + vm.getName() + " is in " + State.Migrating + " state"); + s_logger.debug("4. The VM " + vm.getName() + " is in " + State.Migrating + " state"); s_vms.put(_cluster, _name, vm.getName(), State.Migrating); return new PrepareForMigrationAnswer(cmd); @@ -2428,7 +2428,7 @@ public abstract class CitrixResourceBase implements ServerResource, HypervisorRe state = s_vms.getState(_cluster, vmName); - s_logger.debug("The VM " + vmName + " is in " + State.Stopping + " state"); + s_logger.debug("5. The VM " + vmName + " is in " + State.Stopping + " state"); s_vms.put(_cluster, _name, vmName, State.Stopping); try { Set vms = VM.getByNameLabel(conn, vmName); @@ -2495,7 +2495,7 @@ public abstract class CitrixResourceBase implements ServerResource, HypervisorRe s_logger.warn(msg, e); return new MigrateAnswer(cmd, false, msg, null); } finally { - s_logger.debug("The VM " + vmName + " is in " + state + " state"); + s_logger.debug("6. The VM " + vmName + " is in " + state + " state"); s_vms.put(_cluster, _name, vmName, state); } @@ -2618,7 +2618,7 @@ public abstract class CitrixResourceBase implements ServerResource, HypervisorRe @Override public RebootAnswer execute(RebootCommand cmd) { Connection conn = getConnection(); - s_logger.debug("The VM " + cmd.getVmName() + " is in " + State.Starting + " state"); + s_logger.debug("7. The VM " + cmd.getVmName() + " is in " + State.Starting + " state"); s_vms.put(_cluster, _name, cmd.getVmName(), State.Starting); try { Set vms = null; @@ -2642,7 +2642,7 @@ public abstract class CitrixResourceBase implements ServerResource, HypervisorRe } return new RebootAnswer(cmd, "reboot succeeded", null, null); } finally { - s_logger.debug("The VM " + cmd.getVmName() + " is in " + State.Running + " state"); + s_logger.debug("8. The VM " + cmd.getVmName() + " is in " + State.Running + " state"); s_vms.put(_cluster, _name, cmd.getVmName(), State.Running); } } @@ -3136,7 +3136,7 @@ public abstract class CitrixResourceBase implements ServerResource, HypervisorRe State state = s_vms.getState(_cluster, vmName); - s_logger.debug("The VM " + vmName + " is in " + State.Stopping + " state"); + s_logger.debug("9. The VM " + vmName + " is in " + State.Stopping + " state"); s_vms.put(_cluster, _name, vmName, State.Stopping); try { @@ -3198,7 +3198,7 @@ public abstract class CitrixResourceBase implements ServerResource, HypervisorRe String msg = "VM destroy failed in Stop " + vmName + " Command due to " + e.getMessage(); s_logger.warn(msg, e); } finally { - s_logger.debug("The VM " + vmName + " is in " + state + " state"); + s_logger.debug("10. The VM " + vmName + " is in " + state + " state"); s_vms.put(_cluster, _name, vmName, state); } } @@ -6637,7 +6637,7 @@ public abstract class CitrixResourceBase implements ServerResource, HypervisorRe s_logger.warn("Detecting a change in host for " + vm); changes.put(vm, new Pair(host_uuid, newState)); - s_logger.debug("The VM " + vm + " is in " + newState + " state"); + s_logger.debug("11. The VM " + vm + " is in " + newState + " state"); s_vms.put(_cluster, host_uuid, vm, newState); continue; } @@ -6661,7 +6661,7 @@ public abstract class CitrixResourceBase implements ServerResource, HypervisorRe changes.put(vm, new Pair(host_uuid, newState)); } else if (oldState.second() == State.Starting) { if (newState == State.Running) { - s_logger.debug("The VM " + vm + " is in " + State.Running + " state"); + s_logger.debug("12. The VM " + vm + " is in " + State.Running + " state"); s_vms.put(_cluster, host_uuid, vm, newState); } else if (newState == State.Stopped) { s_logger.warn("Ignoring vm " + vm + " because of a lag in starting the vm."); @@ -6673,13 +6673,13 @@ public abstract class CitrixResourceBase implements ServerResource, HypervisorRe } } else if (oldState.second() == State.Stopping) { if (newState == State.Stopped) { - s_logger.debug("The VM " + vm + " is in " + State.Stopped + " state"); + s_logger.debug("13. The VM " + vm + " is in " + State.Stopped + " state"); s_vms.put(_cluster, host_uuid, vm, newState); } else if (newState == State.Running) { s_logger.warn("Ignoring vm " + vm + " because of a lag in stopping the vm. "); } } else if (oldState.second() != newState) { - s_logger.debug("The VM " + vm + " is in " + newState + " state was " + oldState.second()); + s_logger.debug("14. The VM " + vm + " is in " + newState + " state was " + oldState.second()); s_vms.put(_cluster, host_uuid, vm, newState); if (newState == State.Stopped) { /* diff --git a/core/src/com/cloud/vm/UserVmVO.java b/core/src/com/cloud/vm/UserVmVO.java index 8bb2c1fd4fd..3cd1bbf4cae 100755 --- a/core/src/com/cloud/vm/UserVmVO.java +++ b/core/src/com/cloud/vm/UserVmVO.java @@ -126,4 +126,8 @@ public class UserVmVO extends VMInstanceVO implements UserVm { public void setAccountId(long accountId){ this.accountId = accountId; } + + public void setDomainId(long domainId){ + this.domainId = domainId; + } } diff --git a/server/src/com/cloud/vm/UserVmManagerImpl.java b/server/src/com/cloud/vm/UserVmManagerImpl.java index 4d92fa865f0..50618f52dcc 100755 --- a/server/src/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/com/cloud/vm/UserVmManagerImpl.java @@ -18,6 +18,7 @@ package com.cloud.vm; import java.util.ArrayList; +import com.cloud.network.rules.FirewallRule; import java.util.Date; import java.util.HashMap; import java.util.List; @@ -135,10 +136,13 @@ import com.cloud.network.element.UserDataServiceProvider; import com.cloud.network.lb.LoadBalancingRulesManager; import com.cloud.network.router.VirtualNetworkApplianceManager; import com.cloud.network.rules.FirewallManager; +import com.cloud.network.rules.FirewallRuleVO; import com.cloud.network.rules.RulesManager; import com.cloud.network.security.SecurityGroup; import com.cloud.network.security.SecurityGroupManager; +import com.cloud.network.security.SecurityGroupVMMapVO; import com.cloud.network.security.dao.SecurityGroupDao; +import com.cloud.network.security.dao.SecurityGroupVMMapDao; import com.cloud.offering.NetworkOffering; import com.cloud.offering.NetworkOffering.Availability; import com.cloud.offering.ServiceOffering; @@ -359,6 +363,8 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager protected ResourceManager _resourceMgr; @Inject protected NetworkServiceMapDao _ntwkSrvcDao; + @Inject + SecurityGroupVMMapDao _securityGroupVMMapDao; protected ScheduledExecutorService _executor = null; protected int _expungeInterval; @@ -2194,7 +2200,7 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager // Verify that caller can perform actions in behalf of vm owner _accountMgr.checkAccess(caller, null, owner); - + if (networkIdList == null || networkIdList.isEmpty()) { NetworkVO defaultNetwork = null; @@ -3337,7 +3343,7 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager Account oldAccount = _accountService.getActiveAccountById(vm.getAccountId()); if (oldAccount == null) { - throw new InvalidParameterValueException("Invalid account for VM " + vm.getAccountId() + " in domain " + oldAccount.getDomainId()); + throw new InvalidParameterValueException("Invalid account for VM " + vm.getAccountId() + " in domain."); } //don't allow to move the vm from the project if (oldAccount.getType() == Account.ACCOUNT_TYPE_PROJECT) { @@ -3352,7 +3358,35 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager throw new InvalidParameterValueException("The new account owner " + cmd.getAccountName() + " is disabled."); } - //don't allow to move the vm if it's assigned to Isolated + // don't allow to move the vm if there are existing PF/LB/Static Nat rules, existing Security groups or vm is assigned to static Nat ip + IPAddressVO ip = _ipAddressDao.findByAssociatedVmId(cmd.getVmId()); + if (ip != null){ + List firewall_rules = _rulesDao.listByIpAndPurposeAndNotRevoked(ip.getId(), FirewallRule.Purpose.Firewall); + if (firewall_rules.size() > 0){ + throw new InvalidParameterValueException("Remove the Firewall rules for this VM before assigning to another user."); + } + List lb_rules = _rulesDao.listByIpAndPurposeAndNotRevoked(ip.getId(), FirewallRule.Purpose.LoadBalancing); + if (lb_rules.size() > 0){ + throw new InvalidParameterValueException("Remove the LoadBalancing rules for this VM before assigning to another user."); + } + List nat_rules = _rulesDao.listByIpAndPurposeAndNotRevoked(ip.getId(), FirewallRule.Purpose.StaticNat); + if (nat_rules.size() > 0){ + throw new InvalidParameterValueException("Remove the StaticNat rules for this VM before assigning to another user."); + } + List vpn_rules = _rulesDao.listByIpAndPurposeAndNotRevoked(ip.getId(), FirewallRule.Purpose.Vpn); + if (vpn_rules.size() > 0){ + throw new InvalidParameterValueException("Remove the Vpn rules for this VM before assigning to another user."); + } + List securityGroupsToVmMap = _securityGroupVMMapDao.listByInstanceId(cmd.getVmId()); + if (securityGroupsToVmMap.size() > 0){ + throw new InvalidParameterValueException("Remove the VM from security groups before assigning to another user."); + } + } + + DataCenterVO zone = _dcDao.findById(vm.getDataCenterIdToDeployIn()); + + //Remove vm from instance group + removeInstanceFromInstanceGroup(cmd.getVmId()); //VV 2: check if account/domain is with in resource limits to create a new vm _resourceLimitMgr.checkResourceLimit(newAccount, ResourceType.user_vm); @@ -3368,12 +3402,6 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager DomainVO domain = _domainDao.findById(cmd.getDomainId()); _accountMgr.checkAccess(newAccount, domain); - DataCenterVO zone = _dcDao.findById(vm.getDataCenterIdToDeployIn()); - - //check is zone networking is advanced - //if (zone.getNetworkType() != NetworkType.Advanced) { - // throw new InvalidParameterValueException("Assing virtual machine to another account is only available for advanced networking " + vm); - //} VMInstanceVO vmoi = _itMgr.findByIdAndType(vm.getType(), vm.getId()); VirtualMachineProfileImpl vmOldProfile = new VirtualMachineProfileImpl(vmoi); @@ -3388,14 +3416,17 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager // OWNERSHIP STEP 1: update the vm owner vm.setAccountId(newAccount.getAccountId()); + vm.setDomainId(cmd.getDomainId()); _vmDao.persist(vm); // OS 2: update volume List volumes = _volsDao.findByInstance(cmd.getVmId()); for (VolumeVO volume : volumes) { + _usageEventDao.persist(new UsageEventVO(EventTypes.EVENT_VOLUME_DELETE, volume.getAccountId(), volume.getDataCenterId(), volume.getId(), volume.getName())); _resourceLimitMgr.decrementResourceCount(oldAccount.getAccountId(), ResourceType.volume, Long.valueOf(volumes.size())); volume.setAccountId(newAccount.getAccountId()); _volsDao.persist(volume); _resourceLimitMgr.incrementResourceCount(newAccount.getAccountId(), ResourceType.volume, Long.valueOf(volumes.size())); + _usageEventDao.persist(new UsageEventVO(EventTypes.EVENT_VOLUME_CREATE, volume.getAccountId(), volume.getDataCenterId(), volume.getId(), volume.getName())); } _resourceLimitMgr.incrementResourceCount(newAccount.getAccountId(), ResourceType.user_vm); @@ -3406,57 +3437,100 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager txn.commit(); // OS 3: update the network - if (zone.getNetworkType() == NetworkType.Advanced) { - //cleanup the network for the oldOwner - _networkMgr.cleanupNics(vmOldProfile); - _networkMgr.expungeNics(vmOldProfile); + List networkIdList = cmd.getNetworkIds(); + if (zone.getNetworkType() == NetworkType.Basic) { + //security groups will be recreated for the new account, when the VM is started + } else { + if (zone.isSecurityGroupEnabled()) { + throw new InvalidParameterValueException("not yet tested for SecurityGroupEnabled advanced networks."); + } else { + //cleanup the network for the oldOwner + _networkMgr.cleanupNics(vmOldProfile); + _networkMgr.expungeNics(vmOldProfile); + + // add the new nics + List networkList = new ArrayList(); + NetworkVO defaultNetwork = null; - // add the new nics - List networkList = new ArrayList(); - NetworkVO defaultNetwork = null; - - List oldNetworks = new ArrayList(); - List zoneNetworks = _networkDao.listByZone(zone.getId()); - - for (NetworkVO network : zoneNetworks) { // get the default networks for the account - NetworkOfferingVO no = _networkOfferingDao.findById(network.getNetworkOfferingId()); - if (!no.isSystemOnly()) { - if (network.getGuestType() == Network.GuestType.Shared || !_networkDao.listBy(oldAccount.getId(), network.getId()).isEmpty()) { - oldNetworks.add(network); + List applicableNetworks = new ArrayList(); + // create the default network + List zoneNetworks = _networkDao.listByZone(zone.getId()); // get the default networks for the account + for (NetworkVO network : zoneNetworks) { + NetworkOfferingVO no = _networkOfferingDao.findById(network.getNetworkOfferingId()); + if (!no.isSystemOnly()) { + if (network.getGuestType() == Network.GuestType.Shared || !_networkDao.listBy(oldAccount.getId(), network.getId()).isEmpty()) { + applicableNetworks.add(network); + } } } - } - for (NetworkVO oldNet: oldNetworks){ - long networkOffering = oldNet.getNetworkOfferingId(); - PhysicalNetwork physicalNetwork = _networkMgr.translateZoneIdToPhysicalNetwork(zone.getId()); - List virtualNetworks = _networkMgr.listNetworksForAccount(newAccount.getId(), zone.getId(), Network.GuestType.Isolated); - if (virtualNetworks.isEmpty()) { - Network newNetwork = _networkMgr.createGuestNetwork(networkOffering, newAccount.getAccountName() + "-network", newAccount.getAccountName() + "-network", null, null, - null, null, newAccount, false, null, physicalNetwork, zone.getId(), ACLType.Account, null); - defaultNetwork = _networkDao.findById(newNetwork.getId()); - } else if (virtualNetworks.size() > 1) { - throw new InvalidParameterValueException("More than 1 default Virtaul networks are found for account " + newAccount + "; please specify networkIds"); - } else { - defaultNetwork = virtualNetworks.get(0); + if (networkIdList != null && !networkIdList.isEmpty()){ + // add any additional networks + for (Long networkId : networkIdList) { + NetworkVO network = _networkDao.findById(networkId); + if (network == null) { + throw new InvalidParameterValueException("Unable to find network by id " + networkIdList.get(0).longValue()); + } + + // Perform account permission check + if (network.getGuestType() != Network.GuestType.Shared) { + List networkMap = _networkDao.listBy(newAccount.getId(), network.getId()); + if (networkMap == null || networkMap.isEmpty()) { + throw new PermissionDeniedException("Unable to create a vm using network with id " + network.getId() + ", permission denied"); + } + } else { + if (!_networkMgr.isNetworkAvailableInDomain(networkId, newAccount.getDomainId())) { + throw new PermissionDeniedException("Shared network id=" + networkId + " is not available in domain id=" + newAccount.getDomainId()); + } + } + + //don't allow to use system networks + NetworkOffering networkOffering = _configMgr.getNetworkOffering(network.getNetworkOfferingId()); + if (networkOffering.isSystemOnly()) { + throw new InvalidParameterValueException("Network id=" + networkId + " is system only and can't be used for vm deployment"); + } + applicableNetworks.add(network); + } } - - networkList.add(defaultNetwork); - List> networks = new ArrayList>(); - for (NetworkVO network : networkList) { - networks.add(new Pair(network, null)); + for (NetworkVO appNet: applicableNetworks){ + long networkOffering = appNet.getNetworkOfferingId(); + PhysicalNetwork physicalNetwork = _networkMgr.translateZoneIdToPhysicalNetwork(zone.getId()); + List virtualNetworks = _networkMgr.listNetworksForAccount(newAccount.getId(), zone.getId(), Network.GuestType.Isolated); + if (virtualNetworks.isEmpty()) { + s_logger.debug("Creating network for account " + newAccount + " as a part of assignVM process"); + Network newNetwork = _networkMgr.createGuestNetwork(networkOffering, newAccount.getAccountName() + "-network", newAccount.getAccountName() + "-network", null, null, + null, null, newAccount, false, null, physicalNetwork, zone.getId(), ACLType.Account, null); + defaultNetwork = _networkDao.findById(newNetwork.getId()); + } else if (virtualNetworks.size() > 1) { + throw new InvalidParameterValueException("More than 1 default Virtaul networks are found for account " + newAccount + "; please specify networkIds"); + } else { + defaultNetwork = virtualNetworks.get(0); + } + + networkList.add(defaultNetwork); + + List> networks = new ArrayList>(); + int toggle=0; + for (NetworkVO network : networkList) { + NicProfile defaultNic = new NicProfile(); + if (toggle==0){ + defaultNic.setDefaultNic(true); + toggle++; + } + networks.add(new Pair(network, defaultNic)); + } + + VMInstanceVO vmi = _itMgr.findByIdAndType(vm.getType(), vm.getId()); + VirtualMachineProfileImpl vmProfile = new VirtualMachineProfileImpl(vmi); + _networkMgr.allocate(vmProfile, networks); } - - VMInstanceVO vmi = _itMgr.findByIdAndType(vm.getType(), vm.getId()); - VirtualMachineProfileImpl vmProfile = new VirtualMachineProfileImpl(vmi); - _networkMgr.allocate(vmProfile, networks); - } - } - + } //END IF NON SEC GRP ENABLED + } // END IF ADVANCED return vm; } + @Override public UserVm restoreVM(RestoreVMCmd cmd) { // Input validation