cloudstack-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From muralire...@apache.org
Subject git commit: updated refs/heads/4.2 to fe568fe
Date Tue, 09 Jul 2013 08:25:34 GMT
Updated Branches:
  refs/heads/4.2 c7f26583a -> fe568fefd


CLOUDSTACK-234: create/delete firewa/lb/pf rule: send ip assoc command
only on first rule is created on the IP and last rule is revoked on the
IP

Current suboptima logic of IP Assoc

 - On associate IP to GuestNetwork there is an IPAssoc command sent to
   corresponding network service providers of the network
 - On every rule apply on IP associated with the network send IP assoc
   to the network service providers
 - On every rule deletion on IP associated with a network sernd IP assoc
   command to the network service providers

With this fix logic of IP assoc is changed as below which eliminates
executio of unnessary and expensive IpAssocCommand resource command

 - On associate IP to GuestNetwork, associate IP only to the network,
   Untill any service is associated with the IP dont send IP Assoc
 - On creation of first rule on the IP send IPAssoc to corresponding
   network service provider. Since IP is used for a service, IPAssoc
   need to be sent to correpondign service provider
 - On deletion of last rule on the IP send IPAssoc to corresponding
   network service provider. When last rule is deleted, IP has no
   service associated with it, so send IP assoc to service provider to
   remove the IP association


Project: http://git-wip-us.apache.org/repos/asf/cloudstack/repo
Commit: http://git-wip-us.apache.org/repos/asf/cloudstack/commit/fe568fef
Tree: http://git-wip-us.apache.org/repos/asf/cloudstack/tree/fe568fef
Diff: http://git-wip-us.apache.org/repos/asf/cloudstack/diff/fe568fef

Branch: refs/heads/4.2
Commit: fe568fefd3ba01c909ec4c1e9476053b43de34b5
Parents: c7f2658
Author: Murali Reddy <muralimmreddy@gmail.com>
Authored: Fri Jul 5 16:57:24 2013 +0530
Committer: Murali Reddy <muralimmreddy@gmail.com>
Committed: Tue Jul 9 13:31:39 2013 +0530

----------------------------------------------------------------------
 .../com/cloud/network/dao/FirewallRulesDao.java |   4 +-
 .../cloud/network/dao/FirewallRulesDaoImpl.java |  11 ++
 .../src/com/cloud/network/NetworkManager.java   |   2 +-
 .../com/cloud/network/NetworkManagerImpl.java   | 154 ++++++++++++++-----
 .../src/com/cloud/network/NetworkModelImpl.java |   4 +-
 .../cloud/network/rules/RulesManagerImpl.java   |   6 +-
 .../cloud/network/MockNetworkManagerImpl.java   |   2 +-
 .../com/cloud/vpc/MockNetworkManagerImpl.java   |   2 +-
 8 files changed, 136 insertions(+), 49 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/cloudstack/blob/fe568fef/engine/schema/src/com/cloud/network/dao/FirewallRulesDao.java
----------------------------------------------------------------------
diff --git a/engine/schema/src/com/cloud/network/dao/FirewallRulesDao.java b/engine/schema/src/com/cloud/network/dao/FirewallRulesDao.java
index 6b9b3bb..59987b2 100644
--- a/engine/schema/src/com/cloud/network/dao/FirewallRulesDao.java
+++ b/engine/schema/src/com/cloud/network/dao/FirewallRulesDao.java
@@ -54,7 +54,9 @@ public interface FirewallRulesDao extends GenericDao<FirewallRuleVO,
Long> {
     List<FirewallRuleVO> listByIpAndNotRevoked(long ipAddressId);
 
     long countRulesByIpId(long sourceIpId);
-    
+
+    long countRulesByIpIdAndState(long sourceIpId, FirewallRule.State state);
+
     List<FirewallRuleVO> listByNetworkPurposeTrafficTypeAndNotRevoked(long networkId,
FirewallRule.Purpose purpose, FirewallRule.TrafficType trafficType);
     List<FirewallRuleVO> listByNetworkPurposeTrafficType(long networkId, FirewallRule.Purpose
purpose, FirewallRule.TrafficType trafficType);
     

http://git-wip-us.apache.org/repos/asf/cloudstack/blob/fe568fef/engine/schema/src/com/cloud/network/dao/FirewallRulesDaoImpl.java
----------------------------------------------------------------------
diff --git a/engine/schema/src/com/cloud/network/dao/FirewallRulesDaoImpl.java b/engine/schema/src/com/cloud/network/dao/FirewallRulesDaoImpl.java
index 45a8068..41f911c 100644
--- a/engine/schema/src/com/cloud/network/dao/FirewallRulesDaoImpl.java
+++ b/engine/schema/src/com/cloud/network/dao/FirewallRulesDaoImpl.java
@@ -100,6 +100,7 @@ public class FirewallRulesDaoImpl extends GenericDaoBase<FirewallRuleVO,
Long> i
         RulesByIpCount = createSearchBuilder(Long.class);
         RulesByIpCount.select(null, Func.COUNT, RulesByIpCount.entity().getId());
         RulesByIpCount.and("ipAddressId", RulesByIpCount.entity().getSourceIpAddressId(),
Op.EQ);
+        RulesByIpCount.and("state", RulesByIpCount.entity().getState(), Op.EQ);
         RulesByIpCount.done();
     }
 
@@ -329,6 +330,16 @@ public class FirewallRulesDaoImpl extends GenericDaoBase<FirewallRuleVO,
Long> i
     }
 
     @Override
+    public long countRulesByIpIdAndState(long sourceIpId, FirewallRule.State state) {
+        SearchCriteria<Long> sc = RulesByIpCount.create();
+        sc.setParameters("ipAddressId", sourceIpId);
+        if (state != null) {
+            sc.setParameters("state", state);
+        }
+        return customSearch(sc, null).get(0);
+    }
+
+    @Override
     public List<FirewallRuleVO> listByIpAndPurposeWithState(Long ipId, Purpose purpose,
State state) {
         SearchCriteria<FirewallRuleVO> sc = AllFieldsSearch.create();
         sc.setParameters("ipId", ipId);

http://git-wip-us.apache.org/repos/asf/cloudstack/blob/fe568fef/server/src/com/cloud/network/NetworkManager.java
----------------------------------------------------------------------
diff --git a/server/src/com/cloud/network/NetworkManager.java b/server/src/com/cloud/network/NetworkManager.java
index 306169e..8dc7743 100755
--- a/server/src/com/cloud/network/NetworkManager.java
+++ b/server/src/com/cloud/network/NetworkManager.java
@@ -191,7 +191,7 @@ public interface NetworkManager  {
 
     public String acquireGuestIpAddress(Network network, String requestedIp);
 
-    boolean applyStaticNats(List<? extends StaticNat> staticNats, boolean continueOnError)
throws ResourceUnavailableException;
+    boolean applyStaticNats(List<? extends StaticNat> staticNats, boolean continueOnError,
boolean forRevoke) throws ResourceUnavailableException;
 
     boolean reallocate(VirtualMachineProfile<? extends VMInstanceVO> vm,
             DataCenterDeployment dest) throws InsufficientCapacityException, ConcurrentOperationException;

http://git-wip-us.apache.org/repos/asf/cloudstack/blob/fe568fef/server/src/com/cloud/network/NetworkManagerImpl.java
----------------------------------------------------------------------
diff --git a/server/src/com/cloud/network/NetworkManagerImpl.java b/server/src/com/cloud/network/NetworkManagerImpl.java
index dac6a3a..2b53565 100755
--- a/server/src/com/cloud/network/NetworkManagerImpl.java
+++ b/server/src/com/cloud/network/NetworkManagerImpl.java
@@ -627,30 +627,23 @@ public class NetworkManagerImpl extends ManagerBase implements NetworkManager,
L
     @Override
     public boolean applyIpAssociations(Network network, boolean continueOnError) throws ResourceUnavailableException
{
         List<IPAddressVO> userIps = _ipAddressDao.listByAssociatedNetwork(network.getId(),
null);
-        List<PublicIp> publicIps = new ArrayList<PublicIp>();
-        if (userIps != null && !userIps.isEmpty()) {
-            for (IPAddressVO userIp : userIps) {
-                PublicIp publicIp = PublicIp.createFromAddrAndVlan(userIp, _vlanDao.findById(userIp.getVlanId()));
-                publicIps.add(publicIp);
-            }
-        }
-
-        boolean success = applyIpAssociations(network, false, continueOnError, publicIps);
+        boolean success = true;
 
-        if (success) {
-            for (IPAddressVO addr : userIps) {
-                if (addr.getState() == IpAddress.State.Allocating) {
-                    markPublicIpAsAllocated(addr);
-                } else if (addr.getState() == IpAddress.State.Releasing) {
-                    // Cleanup all the resources for ip address if there are any, and only
then un-assign ip in the
-                    // system
-                    if (cleanupIpResources(addr.getId(), Account.ACCOUNT_ID_SYSTEM, _accountMgr.getSystemAccount()))
{
-                        s_logger.debug("Unassiging ip address " + addr);
-                        _ipAddressDao.unassignIpAddress(addr.getId());
-                    } else {
-                        success = false;
-                        s_logger.warn("Failed to release resources for ip address id=" +
addr.getId());
-                    }
+        // CloudStack will take a lazy approach to associate an acquired public IP to a network
service provider as
+        // it will not know what service an acquired IP will be used for. An IP is actually
associated with a provider when first
+        // rule is applied. Similarly when last rule on the acquired IP is revoked, IP is
not associated with any provider
+        // but still be associated with the account. At this point just mark IP as allocated
or released.
+        for (IPAddressVO addr : userIps) {
+            if (addr.getState() == IpAddress.State.Allocating) {
+                addr.setAssociatedWithNetworkId(network.getId());
+                markPublicIpAsAllocated(addr);
+            } else if (addr.getState() == IpAddress.State.Releasing) {
+                // Cleanup all the resources for ip address if there are any, and only then
un-assign ip in the system
+                if (cleanupIpResources(addr.getId(), Account.ACCOUNT_ID_SYSTEM, _accountMgr.getSystemAccount()))
{
+                    _ipAddressDao.unassignIpAddress(addr.getId());
+                } else {
+                    success = false;
+                    s_logger.warn("Failed to release resources for ip address id=" + addr.getId());
                 }
             }
         }
@@ -658,14 +651,17 @@ public class NetworkManagerImpl extends ManagerBase implements NetworkManager,
L
         return success;
     }
 
-   
 
+    // CloudStack will take a lazy approach to associate an acquired public IP to a network
service provider as
+    // it will not know what a acquired IP will be used for. An IP is actually associated
with a provider when first
+    // rule is applied. Similarly when last rule on the acquired IP is revoked, IP is not
associated with any provider
+    // but still be associated with the account. Its up to caller of this function to decide
when to invoke IPAssociation
     @Override
-    public boolean applyIpAssociations(Network network, boolean rulesRevoked, boolean continueOnError,

+    public boolean applyIpAssociations(Network network, boolean postApplyRules, boolean continueOnError,
             List<? extends PublicIpAddress> publicIps) throws ResourceUnavailableException
{
         boolean success = true;
 
-        Map<PublicIpAddress, Set<Service>> ipToServices = _networkModel.getIpToServices(publicIps,
rulesRevoked, true);
+        Map<PublicIpAddress, Set<Service>> ipToServices = _networkModel.getIpToServices(publicIps,
postApplyRules, true);
         Map<Provider, ArrayList<PublicIpAddress>> providerToIpList = _networkModel.getProviderToIpList(network,
ipToServices);
 
         for (Provider provider : providerToIpList.keySet()) {
@@ -2943,11 +2939,10 @@ public class NetworkManagerImpl extends ManagerBase implements NetworkManager,
L
                     publicIps.add(publicIp);
                 }
             }
-
-            // rules can not programmed unless IP is associated with network
-            // service provider, so run IP assoication for
-            // the network so as to ensure IP is associated before applying
-            // rules (in add state)
+        }
+        // rules can not programmed unless IP is associated with network service provider,
so run IP assoication for
+        // the network so as to ensure IP is associated before applying rules (in add state)
+        if (checkIfIpAssocRequired(network, false, publicIps)) {
             applyIpAssociations(network, false, continueOnError, publicIps);
         }
 
@@ -2961,15 +2956,65 @@ public class NetworkManagerImpl extends ManagerBase implements NetworkManager,
L
             success = false;
         }
 
-        if (!(rules.get(0).getPurpose() == FirewallRule.Purpose.Firewall && trafficType
== FirewallRule.TrafficType.Egress)) {
-            // if all the rules configured on public IP are revoked then
-            // dis-associate IP with network service provider
+        // if there are no active rules associated with a public IP, then public IP need
not be associated with a provider.
+        // This IPAssoc ensures, public IP is dis-associated after last active rule is revoked.
+        if (checkIfIpAssocRequired(network, true, publicIps)) {
             applyIpAssociations(network, true, continueOnError, publicIps);
         }
 
         return success;
     }
 
+    // An IP association is required in below cases
+    //  1.there is at least one public IP associated with the network on which first rule
(PF/static NAT/LB) is being applied.
+    //  2.last rule (PF/static NAT/LB) on the public IP has been revoked. So the public IP
should not be associated with any provider
+    boolean checkIfIpAssocRequired(Network network, boolean postApplyRules, List<PublicIp>
publicIps) {
+        for (PublicIp ip : publicIps) {
+            if (ip.isSourceNat()) {
+                continue;
+            } else if (ip.isOneToOneNat()) {
+                continue;
+            } else {
+                Long totalCount = null;
+                Long revokeCount = null;
+                Long activeCount = null;
+                Long addCount = null;
+
+                totalCount = _firewallDao.countRulesByIpId(ip.getId());
+                if (postApplyRules) {
+                    revokeCount = _firewallDao.countRulesByIpIdAndState(ip.getId(), FirewallRule.State.Revoke);
+                } else {
+                    activeCount = _firewallDao.countRulesByIpIdAndState(ip.getId(), FirewallRule.State.Active);
+                    addCount = _firewallDao.countRulesByIpIdAndState(ip.getId(), FirewallRule.State.Add);
+                }
+
+                if (totalCount == null || totalCount.longValue() == 0L) {
+                    continue;
+                }
+
+                if (postApplyRules) {
+
+                    if (revokeCount != null && revokeCount.longValue() == totalCount.longValue())
{
+                        s_logger.trace("All rules are in Revoke state, have to dis-assiciate
IP from the backend");
+                        return true;
+                    }
+                } else {
+                    if (activeCount != null && activeCount > 0) {
+                        continue;
+                    } else if (addCount != null && addCount.longValue() == totalCount.longValue())
{
+                        s_logger.trace("All rules are in Add state, have to assiciate IP
with the backend");
+                        return true;
+                    } else {
+                        continue;
+                    }
+                }
+            }
+        }
+
+        // there are no IP's corresponding to this network that need to be associated with
provider
+        return false;
+    }
+
     public class NetworkGarbageCollector implements Runnable {
 
         @Override
@@ -3535,7 +3580,8 @@ public class NetworkManagerImpl extends ManagerBase implements NetworkManager,
L
 
     
     @Override
-    public boolean applyStaticNats(List<? extends StaticNat> staticNats, boolean continueOnError)
throws ResourceUnavailableException {
+    public boolean applyStaticNats(List<? extends StaticNat> staticNats, boolean continueOnError,
boolean forRevoke)
+            throws ResourceUnavailableException {
         Network network = _networksDao.findById(staticNats.get(0).getNetworkId());
         boolean success = true;
 
@@ -3554,9 +3600,11 @@ public class NetworkManagerImpl extends ManagerBase implements NetworkManager,
L
             }
         }
 
-        // static NAT rules can not programmed unless IP is associated with network service
provider, so run IP
-        // association for the network so as to ensure IP is associated before applying rules
(in add state)
-        applyIpAssociations(network, false, continueOnError, publicIps);
+        // static NAT rules can not programmed unless IP is associated with source NAT service
provider, so run IP
+        // association for the network so as to ensure IP is associated before applying rules
+        if (checkStaticNatIPAssocRequired(network, false, forRevoke, publicIps)) {
+            applyIpAssociations(network, false, continueOnError, publicIps);
+        }
 
         // get provider
         StaticNatServiceProvider element = getStaticNatProviderForNetwork(network);
@@ -3587,12 +3635,38 @@ public class NetworkManagerImpl extends ManagerBase implements NetworkManager,
L
             }
         }
 
-        // if all the rules configured on public IP are revoked then, dis-associate IP with
network service provider
-        applyIpAssociations(network, true, continueOnError, publicIps);
+        // if the static NAT rules configured on public IP is revoked then, dis-associate
IP with static NAT service provider
+        if (checkStaticNatIPAssocRequired(network, true, forRevoke, publicIps)) {
+            applyIpAssociations(network, true, continueOnError, publicIps);
+        }
 
         return success;
     }
 
+    // checks if there are any public IP assigned to network, that are marked for one-to-one
NAT that
+    // needs to be associated/dis-associated with static-nat provider
+    boolean checkStaticNatIPAssocRequired(Network network, boolean postApplyRules, boolean
forRevoke, List<PublicIp> publicIps) {
+        for (PublicIp ip : publicIps) {
+            if (ip.isOneToOneNat()) {
+                Long activeFwCount = null;
+                activeFwCount =  _firewallDao.countRulesByIpIdAndState(ip.getId(), FirewallRule.State.Active);
+
+                if (!postApplyRules && !forRevoke) {
+                    if (activeFwCount > 0) {
+                        continue;
+                    } else {
+                        return true;
+                    }
+                } else if (postApplyRules && forRevoke) {
+                    return true;
+                }
+            } else {
+                continue;
+            }
+        }
+        return false;
+    }
+
     @DB
     @Override
     public boolean reallocate(VirtualMachineProfile<? extends VMInstanceVO> vm, DataCenterDeployment
dest) throws InsufficientCapacityException, ConcurrentOperationException {

http://git-wip-us.apache.org/repos/asf/cloudstack/blob/fe568fef/server/src/com/cloud/network/NetworkModelImpl.java
----------------------------------------------------------------------
diff --git a/server/src/com/cloud/network/NetworkModelImpl.java b/server/src/com/cloud/network/NetworkModelImpl.java
index 407bf70..d7ca639 100755
--- a/server/src/com/cloud/network/NetworkModelImpl.java
+++ b/server/src/com/cloud/network/NetworkModelImpl.java
@@ -273,7 +273,7 @@ public class NetworkModelImpl extends ManagerBase implements NetworkModel
{
     }
 
     @Override
-    public Map<PublicIpAddress, Set<Service>> getIpToServices(List<? extends
PublicIpAddress> publicIps, boolean rulesRevoked, boolean includingFirewall) {
+    public Map<PublicIpAddress, Set<Service>> getIpToServices(List<? extends
PublicIpAddress> publicIps, boolean postApplyRules, boolean includingFirewall) {
             Map<PublicIpAddress, Set<Service>> ipToServices = new HashMap<PublicIpAddress,
Set<Service>>();
     
             if (publicIps != null && !publicIps.isEmpty()) {
@@ -331,7 +331,7 @@ public class NetworkModelImpl extends ManagerBase implements NetworkModel
{
                             // IP is not being used for any purpose so skip IPAssoc to network
service provider
                             continue;
                         } else {
-                            if (rulesRevoked) {
+                            if (postApplyRules) {
                                 // no active rules/revoked rules are associated with this
public IP, so remove the
                                 // association with the provider
                                 if (ip.isSourceNat()) {

http://git-wip-us.apache.org/repos/asf/cloudstack/blob/fe568fef/server/src/com/cloud/network/rules/RulesManagerImpl.java
----------------------------------------------------------------------
diff --git a/server/src/com/cloud/network/rules/RulesManagerImpl.java b/server/src/com/cloud/network/rules/RulesManagerImpl.java
index 0f733fb..397ece8 100755
--- a/server/src/com/cloud/network/rules/RulesManagerImpl.java
+++ b/server/src/com/cloud/network/rules/RulesManagerImpl.java
@@ -964,7 +964,7 @@ public class RulesManagerImpl extends ManagerBase implements RulesManager,
Rules
         }
 
         try {
-            if (!_networkMgr.applyStaticNats(staticNats, continueOnError)) {
+            if (!_networkMgr.applyStaticNats(staticNats, continueOnError, false)) {
                 return false;
             }
         } catch (ResourceUnavailableException ex) {
@@ -1307,7 +1307,7 @@ public class RulesManagerImpl extends ManagerBase implements RulesManager,
Rules
 
         if (staticNats != null && !staticNats.isEmpty()) {
             try {
-                if (!_networkMgr.applyStaticNats(staticNats, continueOnError)) {
+                if (!_networkMgr.applyStaticNats(staticNats, continueOnError, forRevoke))
{
                     return false;
                 }
             } catch (ResourceUnavailableException ex) {
@@ -1334,7 +1334,7 @@ public class RulesManagerImpl extends ManagerBase implements RulesManager,
Rules
                 s_logger.debug("Found " + staticNats.size() + " static nats to disable for
network id " + networkId);
             }
             try {
-                if (!_networkMgr.applyStaticNats(staticNats, continueOnError)) {
+                if (!_networkMgr.applyStaticNats(staticNats, continueOnError, forRevoke))
{
                     return false;
                 }
             } catch (ResourceUnavailableException ex) {

http://git-wip-us.apache.org/repos/asf/cloudstack/blob/fe568fef/server/test/com/cloud/network/MockNetworkManagerImpl.java
----------------------------------------------------------------------
diff --git a/server/test/com/cloud/network/MockNetworkManagerImpl.java b/server/test/com/cloud/network/MockNetworkManagerImpl.java
index 077395f..9c7d092 100755
--- a/server/test/com/cloud/network/MockNetworkManagerImpl.java
+++ b/server/test/com/cloud/network/MockNetworkManagerImpl.java
@@ -323,7 +323,7 @@ public class MockNetworkManagerImpl extends ManagerBase implements NetworkManage
 
 
     @Override
-    public boolean applyStaticNats(List<? extends StaticNat> staticNats, boolean continueOnError)
throws ResourceUnavailableException {
+    public boolean applyStaticNats(List<? extends StaticNat> staticNats, boolean continueOnError,
boolean forRevoke) throws ResourceUnavailableException {
         // TODO Auto-generated method stub
         return false;
     }

http://git-wip-us.apache.org/repos/asf/cloudstack/blob/fe568fef/server/test/com/cloud/vpc/MockNetworkManagerImpl.java
----------------------------------------------------------------------
diff --git a/server/test/com/cloud/vpc/MockNetworkManagerImpl.java b/server/test/com/cloud/vpc/MockNetworkManagerImpl.java
index b609022..523dfb8 100644
--- a/server/test/com/cloud/vpc/MockNetworkManagerImpl.java
+++ b/server/test/com/cloud/vpc/MockNetworkManagerImpl.java
@@ -987,7 +987,7 @@ public class MockNetworkManagerImpl extends ManagerBase implements NetworkManage
      * @see com.cloud.network.NetworkManager#applyStaticNats(java.util.List, boolean)
      */
     @Override
-    public boolean applyStaticNats(List<? extends StaticNat> staticNats, boolean continueOnError)
+    public boolean applyStaticNats(List<? extends StaticNat> staticNats, boolean continueOnError,
boolean forRevoke)
             throws ResourceUnavailableException {
         // TODO Auto-generated method stub
         return false;


Mime
View raw message