From 95faeb620e19adaec45cf89d00c2affe1ff31a8a Mon Sep 17 00:00:00 2001 From: alena Date: Tue, 15 Mar 2011 18:52:28 -0700 Subject: [PATCH] bug 8863: generate usage event when PF/LB/StaticNat rule is set with Revoke status, not when it's actually removed on the backend. status 8863: resolved fixed --- .../src/com/cloud/network/NetworkManager.java | 2 - .../com/cloud/network/NetworkManagerImpl.java | 155 +++++++----------- .../cloud/network/guru/DirectNetworkGuru.java | 2 +- .../cloud/network/guru/PublicNetworkGuru.java | 2 +- .../lb/LoadBalancingRulesManagerImpl.java | 28 +++- .../cloud/network/rules/RulesManagerImpl.java | 20 ++- 6 files changed, 92 insertions(+), 117 deletions(-) diff --git a/server/src/com/cloud/network/NetworkManager.java b/server/src/com/cloud/network/NetworkManager.java index 3de177d0a42..ddc04990d40 100644 --- a/server/src/com/cloud/network/NetworkManager.java +++ b/server/src/com/cloud/network/NetworkManager.java @@ -172,8 +172,6 @@ public interface NetworkManager extends NetworkService { boolean zoneIsConfiguredForExternalNetworking(long zoneId); - void unassignPublicIpAddress(IPAddressVO addr); - Map getServiceCapability(long zoneId, Service service); boolean applyIpAssociations(Network network, boolean continueOnError) throws ResourceUnavailableException; diff --git a/server/src/com/cloud/network/NetworkManagerImpl.java b/server/src/com/cloud/network/NetworkManagerImpl.java index 02ad16eb5bd..63ea29c624c 100755 --- a/server/src/com/cloud/network/NetworkManagerImpl.java +++ b/server/src/com/cloud/network/NetworkManagerImpl.java @@ -306,25 +306,6 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag txn.commit(); } - - @Override @DB - public void unassignPublicIpAddress(IPAddressVO addr) { - Transaction txn = Transaction.currentTxn(); - Account owner = _accountMgr.getAccount(addr.getAccountId()); - long isSourceNat = (addr.isSourceNat()) ? 1 : 0; - - txn.start(); - - _ipAddressDao.unassignIpAddress(addr.getId()); - if (owner.getAccountId() != Account.ACCOUNT_ID_SYSTEM) { - UsageEventVO usageEvent = new UsageEventVO(EventTypes.EVENT_NET_IP_RELEASE, owner.getId(), addr.getDataCenterId(), addr.getId(), addr.getAddress().toString(), isSourceNat); - _usageEventDao.persist(usageEvent); - } - - _accountMgr.decrementResourceCount(owner.getId(), ResourceType.public_ip); - - txn.commit(); - } @Override @DB @@ -472,7 +453,7 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag } else if (addr.getState() == IpAddress.State.Releasing) { //Cleanup all the resources for ip address if there are any, and only then unassign ip in the system if (cleanupIpResources(addr.getId(), Account.ACCOUNT_ID_SYSTEM, _accountMgr.getSystemAccount())) { - unassignPublicIpAddress(addr); + _ipAddressDao.unassignIpAddress(addr.getId()); } else { success = false; s_logger.warn("Failed to release resources for ip address id=" + addr.getId()); @@ -614,11 +595,11 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag } } - @Override + @Override @DB public boolean releasePublicIpAddress(long addrId, long userId, Account caller) { - //mark ip address as Releasing - IPAddressVO ip = _ipAddressDao.markAsUnavailable(addrId); + IPAddressVO ip = markIpAsUnavailable(addrId); + assert (ip != null) : "Unable to mark the ip address id=" + addrId + " as unavailable."; if (ip == null) { return true; @@ -631,9 +612,7 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag boolean success = true; //Cleanup all ip address resources - PF/LB/Static nat rules - if (cleanupIpResources(addrId, userId, caller)) { - unassignPublicIpAddress(ip); - } else { + if (!cleanupIpResources(addrId, userId, caller)) { success = false; s_logger.warn("Failed to release resources for ip address id=" + addrId); } @@ -1278,14 +1257,6 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag return _nicDao.listByVmIdIncludingRemoved(vm.getId()); } - private Account findAccountByIpAddress(Long ipAddressId) { - IPAddressVO address = _ipAddressDao.findById(ipAddressId); - if ((address != null) && (address.getAllocatedToAccountId() != null)) { - return _accountMgr.getActiveAccount(address.getAllocatedToAccountId()); - } - return null; - } - @Override public List getNicProfiles(VirtualMachine vm) { List nics = _nicDao.listByVmId(vm.getId()); @@ -1314,75 +1285,35 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag Long ipAddressId = cmd.getIpAddressId(); // Verify input parameters - Account accountByIp = findAccountByIpAddress(ipAddressId); - if (accountByIp == null) { - throw new InvalidParameterValueException("Unable to find account owner for ip " + ipAddressId); + IPAddressVO ipVO = _ipAddressDao.findById(ipAddressId); + if (ipVO == null) { + throw new InvalidParameterValueException("Unable to find ip address by id " + ipAddressId); + } + + if (ipVO.getAllocatedTime() == null) { + s_logger.debug("Ip Address id= " + ipAddressId + " is not allocated, so do nothing."); + return true; } - Long accountId = accountByIp.getId(); - if (!_accountMgr.isAdmin(caller.getType())) { - if (caller.getId() != accountId.longValue()) { - throw new PermissionDeniedException("account " + caller.getAccountName() + " doesn't own ip address id=" + ipAddressId); - } - } else { - Domain domain = _domainDao.findById(accountByIp.getDomainId()); - _accountMgr.checkAccess(caller, domain); + if (ipVO.getAllocatedToAccountId() != null) { + _accountMgr.checkAccess(caller, ipVO); } - try { - IPAddressVO ipVO = _ipAddressDao.findById(ipAddressId); - if (ipVO == null) { - return false; - } - - if (ipVO.getAllocatedTime() == null) { - return true; - } - - Account account = _accountMgr.getAccount(accountId); - if (account == null) { - return false; - } - - if ((ipVO.getAllocatedToAccountId() == null) || (ipVO.getAllocatedToAccountId().longValue() != accountId)) { - // FIXME: is the user visible in the admin account's domain???? - if (!BaseCmd.isAdmin(account.getType())) { - if (s_logger.isDebugEnabled()) { - s_logger.debug("permission denied disassociating IP address id=" + ipAddressId + "; acct: " + accountId + "; ip (acct / dc / dom / alloc): " + ipVO.getAllocatedToAccountId() + " / " + ipVO.getDataCenterId() + " / " - + ipVO.getAllocatedInDomainId() + " / " + ipVO.getAllocatedTime()); - } - throw new PermissionDeniedException("User/account does not own supplied address"); - } - } - - if (ipVO.getAllocatedTime() == null) { - return true; - } - - if (ipVO.isSourceNat()) { - throw new IllegalArgumentException("ip address is used for source nat purposes and can not be disassociated."); - } - - VlanVO vlan = _vlanDao.findById(ipVO.getVlanId()); - if (!vlan.getVlanType().equals(VlanType.VirtualNetwork)) { - throw new IllegalArgumentException("only ip addresses that belong to a virtual network may be disassociated."); - } - - // Check for account wide pool. It will have an entry for account_vlan_map. - if (_accountVlanMapDao.findAccountVlanMap(accountId, ipVO.getVlanId()) != null) { - throw new PermissionDeniedException("Ip address id=" + ipAddressId + " belongs to Account wide IP pool and cannot be disassociated"); - } - - return releasePublicIpAddress(ipAddressId, userId, caller); - - } catch (PermissionDeniedException pde) { - throw pde; - } catch (IllegalArgumentException iae) { - throw iae; - } catch (Throwable t) { - s_logger.error("Disassociate IP address threw an exception.", t); - throw new IllegalArgumentException("Disassociate IP address threw an exception"); + if (ipVO.isSourceNat()) { + throw new IllegalArgumentException("ip address is used for source nat purposes and can not be disassociated."); } + + VlanVO vlan = _vlanDao.findById(ipVO.getVlanId()); + if (!vlan.getVlanType().equals(VlanType.VirtualNetwork)) { + throw new IllegalArgumentException("only ip addresses that belong to a virtual network may be disassociated."); + } + + // Check for account wide pool. It will have an entry for account_vlan_map. + if (_accountVlanMapDao.findAccountVlanMap(ipVO.getAccountId(), ipVO.getVlanId()) != null) { + throw new InvalidParameterValueException("Ip address id=" + ipAddressId + " belongs to Account wide IP pool and cannot be disassociated"); + } + + return releasePublicIpAddress(ipAddressId, userId, caller); } @Override @@ -2041,7 +1972,7 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag //release all ip addresses List ipsToRelease = _ipAddressDao.listByAssociatedNetwork(networkId, null); for (IPAddressVO ipToRelease : ipsToRelease) { - IPAddressVO ip = _ipAddressDao.markAsUnavailable(ipToRelease.getId()); + IPAddressVO ip = markIpAsUnavailable(ipToRelease.getId()); assert (ip != null) : "Unable to mark the ip address id=" + ipToRelease.getId() + " as unavailable."; } @@ -2660,4 +2591,30 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag } return accountNetworks; } + + @DB + private IPAddressVO markIpAsUnavailable(long addrId) { + Transaction txn = Transaction.currentTxn(); + + IPAddressVO ip = _ipAddressDao.findById(addrId); + + if (ip.getState() != State.Releasing) { + txn.start(); + + ip = _ipAddressDao.markAsUnavailable(addrId); + + _accountMgr.decrementResourceCount(_ipAddressDao.findById(addrId).getAccountId(), ResourceType.public_ip); + + long isSourceNat = (ip.isSourceNat()) ? 1 : 0; + + if (ip.getAccountId() != Account.ACCOUNT_ID_SYSTEM) { + UsageEventVO usageEvent = new UsageEventVO(EventTypes.EVENT_NET_IP_RELEASE, ip.getAccountId(), ip.getDataCenterId(), addrId, ip.getAddress().addr(), isSourceNat); + _usageEventDao.persist(usageEvent); + } + + txn.commit(); + } + + return ip; + } } diff --git a/server/src/com/cloud/network/guru/DirectNetworkGuru.java b/server/src/com/cloud/network/guru/DirectNetworkGuru.java index 18c8b07cf8c..5ae3b62d891 100644 --- a/server/src/com/cloud/network/guru/DirectNetworkGuru.java +++ b/server/src/com/cloud/network/guru/DirectNetworkGuru.java @@ -208,7 +208,7 @@ public class DirectNetworkGuru extends AdapterBase implements NetworkGuru { public void deallocate(Network network, NicProfile nic, VirtualMachineProfile vm) { IPAddressVO ip = _ipAddressDao.findByAccountAndIp(vm.getVirtualMachine().getAccountId(), nic.getIp4Address()); if (ip != null) { - _networkMgr.unassignPublicIpAddress(ip); + _ipAddressDao.unassignIpAddress(ip.getId()); } nic.deallocate(); } diff --git a/server/src/com/cloud/network/guru/PublicNetworkGuru.java b/server/src/com/cloud/network/guru/PublicNetworkGuru.java index 3ab3b6103f8..0dc57239dc1 100644 --- a/server/src/com/cloud/network/guru/PublicNetworkGuru.java +++ b/server/src/com/cloud/network/guru/PublicNetworkGuru.java @@ -156,7 +156,7 @@ public class PublicNetworkGuru extends AdapterBase implements NetworkGuru { public void deallocate(Network network, NicProfile nic, VirtualMachineProfile vm) { IPAddressVO ip = _ipAddressDao.findByAccountAndIp(vm.getVirtualMachine().getAccountId(), nic.getIp4Address()); if (ip != null) { - _networkMgr.unassignPublicIpAddress(ip); + _ipAddressDao.unassignIpAddress(ip.getId()); } nic.deallocate(); } diff --git a/server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java b/server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java index efb75e85628..b2bcad758ea 100644 --- a/server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java +++ b/server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java @@ -285,11 +285,23 @@ public class LoadBalancingRulesManagerImpl implements LoadBalancingRulesManager, } + @DB private boolean deleteLoadBalancerRule(long loadBalancerId, boolean apply, Account caller, long callerUserId) { LoadBalancerVO lb = _lbDao.findById(loadBalancerId); + Transaction txn = Transaction.currentTxn(); + boolean generateUsageEvent = false; - lb.setState(FirewallRule.State.Revoke); - _lbDao.persist(lb); + txn.start(); + if (lb.getState() == FirewallRule.State.Staged) { + if (s_logger.isDebugEnabled()) { + s_logger.debug("Found a rule that is still in stage state so just removing it: " + lb); + } + generateUsageEvent = true; + } else if (lb.getState() == FirewallRule.State.Add || lb.getState() == FirewallRule.State.Active) { + lb.setState(FirewallRule.State.Revoke); + _lbDao.persist(lb); + generateUsageEvent = true; + } List maps = _lb2VmMapDao.listByLoadBalancerId(loadBalancerId); if (maps != null) { @@ -300,6 +312,14 @@ public class LoadBalancingRulesManagerImpl implements LoadBalancingRulesManager, } } + if (generateUsageEvent) { + //Generate usage event right after all rules were marked for revoke + UsageEventVO usageEvent = new UsageEventVO(EventTypes.EVENT_LOAD_BALANCER_DELETE, lb.getAccountId(), 0 , lb.getId(), null); + _usageEventDao.persist(usageEvent); + } + + txn.commit(); + if (apply) { try { if (!applyLoadBalancerConfig(loadBalancerId)) { @@ -312,9 +332,7 @@ public class LoadBalancingRulesManagerImpl implements LoadBalancingRulesManager, } } - _rulesDao.remove(lb.getId()); - UsageEventVO usageEvent = new UsageEventVO(EventTypes.EVENT_LOAD_BALANCER_DELETE, lb.getAccountId(), 0 , lb.getId(), null); - _usageEventDao.persist(usageEvent); + _rulesDao.remove(lb.getId()); s_logger.debug("Load balancer with id " + lb.getId() + " is removed successfully"); return true; } diff --git a/server/src/com/cloud/network/rules/RulesManagerImpl.java b/server/src/com/cloud/network/rules/RulesManagerImpl.java index d37e24050c7..48bf9a59a33 100644 --- a/server/src/com/cloud/network/rules/RulesManagerImpl.java +++ b/server/src/com/cloud/network/rules/RulesManagerImpl.java @@ -440,15 +440,24 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager { } Transaction txn = Transaction.currentTxn(); + boolean generateUsageEvent = false; + txn.start(); if (rule.getState() == State.Staged) { if (s_logger.isDebugEnabled()) { s_logger.debug("Found a rule that is still in stage state so just removing it: " + rule); } _firewallDao.remove(rule.getId()); + generateUsageEvent = true; } else if (rule.getState() == State.Add || rule.getState() == State.Active) { rule.setState(State.Revoke); _firewallDao.update(rule.getId(), rule); + generateUsageEvent = true; + } + + if (generateUsageEvent) { + UsageEventVO usageEvent = new UsageEventVO(EventTypes.EVENT_NET_RULE_DELETE, rule.getAccountId(), 0, rule.getId(), null); + _usageEventDao.persist(usageEvent); } // Save and create the event @@ -459,7 +468,6 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager { description.append("->[").append(pfRule.getDestinationIpAddress()).append(":").append(pfRule.getDestinationPortStart()).append("-").append(pfRule.getDestinationPortEnd()).append("]"); } description.append(" ").append(rule.getProtocol()); - txn.commit(); } @@ -490,10 +498,7 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager { } else { success = true; } - if(success){ - UsageEventVO usageEvent = new UsageEventVO(EventTypes.EVENT_NET_RULE_DELETE, rule.getAccountId(), 0, ruleId, null); - _usageEventDao.persist(usageEvent); - } + return success; } @@ -525,10 +530,7 @@ public class RulesManagerImpl implements RulesManager, RulesService, Manager { } else { success = true; } - if(success){ - UsageEventVO usageEvent = new UsageEventVO(EventTypes.EVENT_NET_RULE_DELETE, rule.getAccountId(), 0, ruleId, null); - _usageEventDao.persist(usageEvent); - } + return success; }