Fix plugin component configuration.

api/src/org/apache/cloudstack/acl/APILimitChecker.java

@@ -16,6 +16,8 @@
 // under the License.
 package org.apache.cloudstack.acl;
 
+import org.apache.cloudstack.api.ServerApiException;
+
 import com.cloud.user.Account;
 import com.cloud.utils.component.Adapter;
 
@@ -24,5 +26,5 @@ import com.cloud.utils.component.Adapter;
  */
 public interface APILimitChecker extends Adapter {
     // Interface for checking if the account is over its api limit
-    boolean isUnderLimit(Account account);
+    void checkLimit(Account account) throws ServerApiException;
 }

client/pom.xml

@@ -30,6 +30,11 @@
       <artifactId>cloud-plugin-acl-static-role-based</artifactId>
       <version>${project.version}</version>
     </dependency>
+    <dependency>
+      <groupId>org.apache.cloudstack</groupId>
+      <artifactId>cloud-plugin-api-limit-account-based</artifactId>
+      <version>${project.version}</version>
+    </dependency>    
     <dependency>
       <groupId>org.apache.cloudstack</groupId>
       <artifactId>cloud-plugin-api-discovery</artifactId>

client/tomcatconf/components.xml.in

@@ -56,6 +56,9 @@ under the License.
         <adapters key="org.apache.cloudstack.acl.APIChecker">
             <adapter name="StaticRoleBasedAPIAccessChecker" class="org.apache.cloudstack.acl.StaticRoleBasedAPIAccessChecker"/>
         </adapters>
+        <adapters key="org.apache.cloudstack.acl.APILimitChecker">
+            <adapter name="AccountBasedAPIRateLimit" class="org.apache.cloudstack.ratelimit.ApiRateLimitServiceImpl" singleton="true"/>
+        </adapters>        
         <adapters key="com.cloud.agent.manager.allocator.HostAllocator">
             <adapter name="FirstFitRouting" class="com.cloud.agent.manager.allocator.impl.FirstFitRoutingAllocator"/>
             <!--adapter name="FirstFitRouting" class="com.cloud.agent.manager.allocator.impl.RecreateHostAllocator"/-->
@@ -180,6 +183,11 @@ under the License.
         <pluggableservice name="ApiDiscoveryService" key="org.apache.cloudstack.discovery.ApiDiscoveryService" class="org.apache.cloudstack.discovery.ApiDiscoveryServiceImpl"/>
         <pluggableservice name="VirtualRouterElementService" key="com.cloud.network.element.VirtualRouterElementService" class="com.cloud.network.element.VirtualRouterElement"/>
         <pluggableservice name="NiciraNvpElementService" key="com.cloud.network.element.NiciraNvpElementService" class="com.cloud.network.element.NiciraNvpElement"/>
+        <pluggableservice name="ApiRateLimitService" key="org.apache.cloudstack.api.ratelimit.ApiRateLimitService" class="org.apache.cloudstack.ratelimit.ApiRateLimitServiceImpl">
+             <param name="api.throttling.interval">1</param>
+             <param name="api.throttling.max">25</param>
+             <param name="api.throttling.cachesize">50000</param>
+        </pluggableservice>
         <dao name="OvsTunnelInterfaceDao" class="com.cloud.network.ovs.dao.OvsTunnelInterfaceDaoImpl" singleton="false"/>
         <dao name="OvsTunnelAccountDao" class="com.cloud.network.ovs.dao.OvsTunnelNetworkDaoImpl" singleton="false"/>
         <dao name="NiciraNvpDao" class="com.cloud.network.dao.NiciraNvpDaoImpl" singleton="false"/>

plugins/api/rate-limit/src/org/apache/cloudstack/api/command/admin/ratelimit/ResetApiLimitCmd.java

@@ -14,7 +14,7 @@
 // KIND, either express or implied.  See the License for the
 // specific language governing permissions and limitations
 // under the License.
-package org.apache.cloudstack.api.commands.admin.ratelimit;
+package org.apache.cloudstack.api.command.admin.ratelimit;
 
 import org.apache.cloudstack.api.*;
 import org.apache.cloudstack.api.response.AccountResponse;

plugins/api/rate-limit/src/org/apache/cloudstack/api/command/user/ratelimit/GetApiLimitCmd.java

@@ -27,7 +27,7 @@ import org.apache.cloudstack.api.Parameter;
 import org.apache.cloudstack.api.PlugService;
 import org.apache.cloudstack.api.ServerApiException;
 import org.apache.cloudstack.api.BaseCmd.CommandType;
-import org.apache.cloudstack.api.commands.admin.ratelimit.ResetApiLimitCmd;
+import org.apache.cloudstack.api.command.admin.ratelimit.ResetApiLimitCmd;
 import org.apache.cloudstack.api.response.AccountResponse;
 import org.apache.cloudstack.api.response.ApiLimitResponse;
 import org.apache.cloudstack.api.response.PhysicalNetworkResponse;

plugins/api/rate-limit/src/org/apache/cloudstack/ratelimit/ApiRateLimitService.java

@@ -16,8 +16,8 @@
 // under the License.
 package org.apache.cloudstack.ratelimit;
 
+import org.apache.cloudstack.api.command.admin.ratelimit.ResetApiLimitCmd;
 import org.apache.cloudstack.api.command.user.ratelimit.GetApiLimitCmd;
-import org.apache.cloudstack.api.commands.admin.ratelimit.ResetApiLimitCmd;
 import org.apache.cloudstack.api.response.ApiLimitResponse;
 import org.apache.cloudstack.api.response.ListResponse;
 

plugins/api/rate-limit/src/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java

@@ -25,19 +25,18 @@ import net.sf.ehcache.CacheManager;
 
 import org.apache.log4j.Logger;
 
-import com.cloud.configuration.Config;
-import com.cloud.configuration.dao.ConfigurationDao;
 import org.apache.cloudstack.acl.APILimitChecker;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.command.admin.ratelimit.ResetApiLimitCmd;
 import org.apache.cloudstack.api.command.user.ratelimit.GetApiLimitCmd;
-import org.apache.cloudstack.api.commands.admin.ratelimit.ResetApiLimitCmd;
 import org.apache.cloudstack.api.response.ApiLimitResponse;
-import com.cloud.network.element.NetworkElement;
 import com.cloud.user.Account;
 import com.cloud.user.UserContext;
+import com.cloud.utils.PropertiesUtil;
 import com.cloud.utils.component.AdapterBase;
-import com.cloud.utils.component.Inject;
 
-@Local(value = NetworkElement.class)
+@Local(value = APILimitChecker.class)
 public class ApiRateLimitServiceImpl extends AdapterBase implements APILimitChecker, ApiRateLimitService {
 	private static final Logger s_logger = Logger.getLogger(ApiRateLimitServiceImpl.class);
 
@@ -51,33 +50,40 @@ public class ApiRateLimitServiceImpl extends AdapterBase implements APILimitChec
 	 */
 	private int maxAllowed = 30;
 
-	@Inject
+	private LimitStore _store = null;
-	ConfigurationDao _configDao;
-
-	private LimitStore _store;
 
 
 	@Override
     public boolean configure(String name, Map<String, Object> params) throws ConfigurationException {
         super.configure(name, params);
+
+        if (_store == null) {
+            // not configured yet, note that since this class is both adapter
+            // and pluggableService, so this method
+            // may be invoked twice in ComponentLocator.
             // get global configured duration and max values
-        String duration = _configDao.getValue(Config.ApiLimitInterval.key());
+            Object duration = params.get("api.throttling.interval");
             if (duration != null) {
-            timeToLive = Integer.parseInt(duration);
+                timeToLive = Integer.parseInt((String) duration);
             }
-        String maxReqs = _configDao.getValue(Config.ApiLimitMax.key());
+            Object maxReqs = params.get("api.throttling.max");
             if (maxReqs != null) {
-            maxAllowed = Integer.parseInt(maxReqs);
+                maxAllowed = Integer.parseInt((String) maxReqs);
             }
             // create limit store
             EhcacheLimitStore cacheStore = new EhcacheLimitStore();
-        int maxElements = 10000;  //TODO: what should be the proper number here?
+            int maxElements = 10000;
+            Object cachesize = params.get("api.throttling.cachesize");
+            if ( cachesize != null ){
+                maxElements = Integer.parseInt((String)cachesize);
+            }
             CacheManager cm = CacheManager.create();
-        Cache cache = new Cache("api-limit-cache", maxElements, true, false, timeToLive, timeToLive);
+            Cache cache = new Cache("api-limit-cache", maxElements, false, false, timeToLive, timeToLive);
             cm.addCache(cache);
             s_logger.info("Limit Cache created: " + cache.toString());
             cacheStore.setCache(cache);
             _store = cacheStore;
+        }
 
         return true;
 
@@ -123,9 +129,8 @@ public class ApiRateLimitServiceImpl extends AdapterBase implements APILimitChec
     }
 
 
-
     @Override
-    public boolean isUnderLimit(Account account) {
+    public void checkLimit(Account account) throws ServerApiException {
 
         Long accountId = account.getId();
         StoreEntry entry = _store.get(accountId);
@@ -140,17 +145,21 @@ public class ApiRateLimitServiceImpl extends AdapterBase implements APILimitChec
         int current = entry.incrementAndGet();
 
         if (current <= maxAllowed) {
-            return true;
+            return;
         } else {
-            return false;
+            long expireAfter = entry.getExpireDuration();
+            s_logger.warn("The given user has reached his/her account api limit, please retry after " + expireAfter + " ms.");
+            throw new ServerApiException(BaseCmd.API_LIMIT_EXCEED, "The given user has reached his/her account api limit, please retry after " +
+                    expireAfter + " ms.");
         }
     }
 
 
 
     @Override
-    public String[] getPropertiesFiles() {
+    public Map<String, String> getProperties() {
-        return new String[] { "api-limit_commands.properties" };
+        return PropertiesUtil.processConfigFile(new String[]
+                { "api-limit_commands.properties" });
     }
 
 

plugins/api/rate-limit/src/org/apache/cloudstack/ratelimit/StoreEntryImpl.java

@@ -47,7 +47,7 @@ public class StoreEntryImpl implements StoreEntry {
         if ( isExpired() )
             return 0; // already expired
         else {
-            return (expiry - System.currentTimeMillis()) * 1000;
+            return expiry - System.currentTimeMillis();
         }
     }
 

plugins/api/rate-limit/test/org/apache/cloudstack/ratelimit/ApiRateLimitTest.java

@@ -23,8 +23,9 @@ import java.util.concurrent.Executors;
 
 import javax.naming.ConfigurationException;
 
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.command.admin.ratelimit.ResetApiLimitCmd;
 import org.apache.cloudstack.api.command.user.ratelimit.GetApiLimitCmd;
-import org.apache.cloudstack.api.commands.admin.ratelimit.ResetApiLimitCmd;
 import org.apache.cloudstack.api.response.ApiLimitResponse;
 import org.apache.cloudstack.ratelimit.ApiRateLimitServiceImpl;
 import org.junit.Before;
@@ -43,16 +44,10 @@ import static org.mockito.Mockito.*;
 public class ApiRateLimitTest {
 
 	static ApiRateLimitServiceImpl _limitService = new ApiRateLimitServiceImpl();
-	static ConfigurationDao _configDao = mock(ConfigurationDao.class);
 	private static long acctIdSeq = 0L;
 
 	@BeforeClass
 	public static void setUp() throws ConfigurationException {
-		_limitService._configDao = _configDao;
-
-		// No global configuration set, will set in each test case
-		when(_configDao.getValue(Config.ApiLimitInterval.key())).thenReturn(null);
-        when(_configDao.getValue(Config.ApiLimitMax.key())).thenReturn(null);
 
 		_limitService.configure("ApiRateLimitTest", Collections.<String, Object> emptyMap());
 	}
@@ -62,6 +57,16 @@ public class ApiRateLimitTest {
 	    return new AccountVO(acctIdSeq++);
 	}
 
+	private boolean isUnderLimit(Account key){
+	    try{
+	        _limitService.checkLimit(key);
+	        return true;
+	    }
+	    catch (ServerApiException ex){
+	        return false;
+	    }
+	}
+
     @Test
     public void sequentialApiAccess() {
         int allowedRequests = 1;
@@ -69,10 +74,10 @@ public class ApiRateLimitTest {
         _limitService.setTimeToLive(1);
 
         Account key = createFakeAccount();
-        assertTrue("Allow for the first request", _limitService.isUnderLimit(key));
+        assertTrue("Allow for the first request", isUnderLimit(key));
 
         assertFalse("Second request should be blocked, since we assume that the two api "
-                + " accesses take less than a second to perform", _limitService.isUnderLimit(key));
+                + " accesses take less than a second to perform", isUnderLimit(key));
     }
 
     @Test
@@ -84,11 +89,11 @@ public class ApiRateLimitTest {
         Account key = createFakeAccount();
 
         for (int i = 0; i < allowedRequests; i++) {
-            assertTrue("We should allow " + allowedRequests + " requests per second", _limitService.isUnderLimit(key));
+            assertTrue("We should allow " + allowedRequests + " requests per second", isUnderLimit(key));
         }
 
 
-        assertFalse("We should block >" + allowedRequests + " requests per second", _limitService.isUnderLimit(key));
+        assertFalse("We should block >" + allowedRequests + " requests per second", isUnderLimit(key));
     }
 
     @Test
@@ -121,7 +126,7 @@ public class ApiRateLimitTest {
                     try {
                         startGate.await();
 
-                        isUsable[j] = _limitService.isUnderLimit(key);
+                        isUsable[j] = isUnderLimit(key);
 
                     } catch (InterruptedException e) {
                         e.printStackTrace();
@@ -155,12 +160,12 @@ public class ApiRateLimitTest {
 
         Account key = this.createFakeAccount();
 
-        assertTrue("The first request should be allowed", _limitService.isUnderLimit(key));
+        assertTrue("The first request should be allowed", isUnderLimit(key));
 
         // Allow the token to expire
         Thread.sleep(1001);
 
-        assertTrue("Another request after interval should be allowed as well", _limitService.isUnderLimit(key));
+        assertTrue("Another request after interval should be allowed as well", isUnderLimit(key));
     }
 
     @Test
@@ -171,16 +176,16 @@ public class ApiRateLimitTest {
 
         Account key = this.createFakeAccount();
 
-        assertTrue("The first request should be allowed", _limitService.isUnderLimit(key));
+        assertTrue("The first request should be allowed", isUnderLimit(key));
 
-        assertFalse("Another request should be blocked", _limitService.isUnderLimit(key));
+        assertFalse("Another request should be blocked", isUnderLimit(key));
 
         ResetApiLimitCmd cmd = new ResetApiLimitCmd();
         cmd.setAccountId(key.getId());
 
         _limitService.resetApiLimit(cmd);
 
-        assertTrue("Another request should be allowed after reset counter", _limitService.isUnderLimit(key));
+        assertTrue("Another request should be allowed after reset counter", isUnderLimit(key));
     }
 
     /* Disable this since I cannot mock Static method UserContext.current()
@@ -193,7 +198,7 @@ public class ApiRateLimitTest {
         Account key = this.createFakeAccount();
 
         for ( int i = 0; i < 5; i++ ){
-            assertTrue("Issued 5 requests", _limitService.isUnderLimit(key));
+            assertTrue("Issued 5 requests", isUnderLimit(key));
         }
 
         GetApiLimitCmd cmd = new GetApiLimitCmd();

server/src/com/cloud/api/ApiServer.java

@@ -556,12 +556,9 @@ public class ApiServer implements HttpRequestHandler {
             if (userId != null) {
             	User user = ApiDBUtils.findUserById(userId);
             	if (apiThrottlingEnabled){
-            	    // go through each API limit checker
+            	    // go through each API limit checker, throw exception inside adapter implementation so that message
-            	    if (!isRequestAllowed(user)) {
+            	    // can contain some detailed information only known for each adapter implementation.
-            	        //FIXME: more detailed message regarding when he/she can retry
+            	    checkRequestLimit(user);
-                        s_logger.warn("The given user has reached his/her account api limit, please retry later");
-                        throw new ServerApiException(BaseCmd.API_LIMIT_EXCEED, "The given user has reached his/her account api limit");
-            	    }
             	}
                 if (!isCommandAvailable(user, commandName)) {
                     s_logger.debug("The given command:" + commandName + " does not exist or it is not available for user with id:" + userId);
@@ -803,20 +800,19 @@ public class ApiServer implements HttpRequestHandler {
     }
 
 
-    private boolean isRequestAllowed(User user) {
+    private void checkRequestLimit(User user) throws ServerApiException {
         Account account = ApiDBUtils.findAccountById(user.getAccountId());
         if ( _accountMgr.isRootAdmin(account.getType()) ){
             // no api throttling for root admin
-            return true;
+            return;
         }
         for (APILimitChecker apiChecker : _apiLimitCheckers) {
             // Fail the checking if any checker fails to verify
-            if (!apiChecker.isUnderLimit(account))
+            apiChecker.checkLimit(account);
-                return false;
          }
-        return true;
     }
 
+
     private boolean doesCommandExist(String apiName) {
         for (APIChecker apiChecker : _apiAccessCheckers) {
             // If any checker has api info on the command, return true

server/src/com/cloud/configuration/Config.java

@@ -361,9 +361,7 @@ public enum Config {
 	                null, "Limits number of snapshots that can be handled by the host concurrently; default is NULL - unlimited", null),
 
 	// API throttling
-    ApiLimitInterval("Advanced", ManagementServer.class, Long.class, "api.throttling.interval", "1", "The default time interval in seconds used to set account based api limit", null),
+    ApiLimitEnabled("Advanced", ManagementServer.class, Boolean.class, "api.throttling.enable", "true", "If true, api throttline feature is enabled", "true,false");
-    ApiLimitMax("Advanced", ManagementServer.class, Long.class, "api.throttling.max", "25", "The max number of API requests within api.throttling.interval duration", null),
-    ApiLimitEnabled("Advanced", ManagementServer.class, Boolean.class, "api.throttling.enabled", "true", "If true, api throttline feature is enabled", "true,false");
 
 	private final String _category;
 	private final Class<?> _componentClass;

server/test/com/cloud/api/ListPerfTest.java

@@ -17,6 +17,9 @@
 package com.cloud.api;
 
 import java.util.HashMap;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
 
 import org.junit.Before;
 import org.junit.Test;
@@ -163,6 +166,57 @@ public class ListPerfTest extends APITest {
 
     }
 
+    @Test
+    public void testMultiListAccounts() throws Exception {
+        // log in using normal user
+        login("demo", "password");
+        // issue list Accounts calls
+        final HashMap<String, String> params = new HashMap<String, String>();
+        params.put("response", "json");
+        params.put("listAll", "true");
+        params.put("sessionkey", sessionKey);
+        int clientCount = 6;
+        Runnable[] clients = new Runnable[clientCount];
+        final boolean[] isUsable = new boolean[clientCount];
 
+        final CountDownLatch startGate = new CountDownLatch(1);
+
+        final CountDownLatch endGate = new CountDownLatch(clientCount);
+
+
+        for (int i = 0; i < isUsable.length; ++i) {
+            final int j = i;
+            clients[j] = new Runnable() {
+
+                /**
+                 * {@inheritDoc}
+                 */
+                @Override
+                public void run() {
+                    try {
+                        startGate.await();
+
+                        System.out.println(sendRequest("listAccounts", params));
+
+                    } catch (InterruptedException e) {
+                        e.printStackTrace();
+                    } finally {
+                        endGate.countDown();
+                    }
+                }
+            };
+        }
+
+        ExecutorService executor = Executors.newFixedThreadPool(clientCount);
+
+        for (Runnable runnable : clients) {
+            executor.execute(runnable);
+        }
+
+        startGate.countDown();
+
+        endGate.await();
+
+    }
 
 }

setup/db/db/schema-40to410.sql

@@ -142,6 +142,8 @@ UPDATE `cloud`.`conditions` set uuid=id WHERE uuid is NULL;
 
 INSERT IGNORE INTO `cloud`.`configuration` VALUES ('Advanced', 'DEFAULT', 'management-server', '"detail.batch.query.size"', '2000', 'Default entity detail batch query size for listing');
 
+INSERT IGNORE INTO `cloud`.`configuration` VALUES ('Advanced', 'DEFAULT', 'management-server', 'api.throttling.enabled', 'true, 'enable api rate limiting');
+
 --- DB views for list api ---
 use cloud;