Configuration keys performance update

Configuration keys: cache inherited parent-scope value on scope miss to avoid repeated DB lookups.
Implemented caching inherited parent-scope value under "current" scope to avoid repeated configuration key hierarchy traversal.

Author: mprokopchuk

framework/config/src/main/java/org/apache/cloudstack/framework/config/ConfigKey.java

@@ -434,7 +434,15 @@ public T valueInScope(Scope scope, Long id) {
         }
         String value = s_depot != null ? s_depot.getConfigStringValue(_name, scope, id) : null;
         if (value == null) {
-            return valueInGlobalOrAvailableParentScope(scope, id);
+            T parentValue = valueInGlobalOrAvailableParentScope(scope, id);
+            // Cache the inherited value under this scope to avoid repeated hierarchy traversal.
+            // Skip this for keys with a multiplier: parentValue already has the multiplier applied,
+            // so caching its toString() and reading it back through valueOf() would apply the
+            // multiplier a second time (double scaling).
+            if (s_depot != null && parentValue != null && multiplier() == null) {
+                s_depot.cacheValue(_name, scope, id, parentValue.toString());
+            }
+            return parentValue;
         }
         logger.trace("Scope({}) value for config ({}): {}", scope, _name, _value);
 

framework/config/src/main/java/org/apache/cloudstack/framework/config/dao/ConfigurationDao.java

@@ -70,4 +70,6 @@ public interface ConfigurationDao extends GenericDao<ConfigurationVO, String> {
     void invalidateCache();
 
     List<ConfigurationVO> searchPartialConfigurations();
+
+    String getValueByKey(String key);
 }

framework/config/src/main/java/org/apache/cloudstack/framework/config/dao/ConfigurationDaoImpl.java

@@ -218,4 +218,10 @@ public List<ConfigurationVO> searchPartialConfigurations() {
         SearchCriteria<ConfigurationVO> sc = PartialSearch.create();
         return searchIncludingRemoved(sc, null, null, false);
     }
+
+    @Override
+    public String getValueByKey(String key) {
+        ConfigurationVO configVO = findByName(key);
+        return (configVO == null ? null : configVO.getValue());
+    }
 }

framework/config/src/main/java/org/apache/cloudstack/framework/config/impl/ConfigDepotImpl.java

@@ -24,11 +24,8 @@
 import java.util.List;
 import java.util.Set;
 
-import javax.annotation.PostConstruct;
 import javax.inject.Inject;
 
-import com.cloud.utils.db.Transaction;
-import com.cloud.utils.db.TransactionCallback;
 import org.apache.cloudstack.framework.config.ConfigDepot;
 import org.apache.cloudstack.framework.config.ConfigDepotAdmin;
 import org.apache.cloudstack.framework.config.ConfigKey;
@@ -38,13 +35,16 @@
 import org.apache.cloudstack.framework.config.dao.ConfigurationGroupDao;
 import org.apache.cloudstack.framework.config.dao.ConfigurationSubGroupDao;
 import org.apache.cloudstack.utils.cache.LazyCache;
+import org.apache.commons.beanutils.ConvertUtils;
 import org.apache.commons.lang.ObjectUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
 import com.cloud.utils.Pair;
 import com.cloud.utils.Ternary;
+import com.cloud.utils.db.Transaction;
+import com.cloud.utils.db.TransactionCallback;
 import com.cloud.utils.exception.CloudRuntimeException;
 
 /**
@@ -74,9 +74,18 @@
  *     when constructing a ConfigKey then configuration server should use the
  *     validation class to validate the value the admin input for the key.
  */
-public class ConfigDepotImpl implements ConfigDepot, ConfigDepotAdmin {
+public class ConfigDepotImpl implements ConfigDepot, ConfigDepotAdmin, Configurable {
     protected Logger logger = LogManager.getLogger(getClass());
+
     protected final static long CONFIG_CACHE_EXPIRE_SECONDS = 30;
+
+    protected final ConfigKey<Long> ConfigKeyCacheMaxSize = new ConfigKey<>("Advanced", Long.class, "config.key.cache.max.size", "512",
+            "Configuration keys cache max size", false);
+    protected final ConfigKey<Long> ConfigKeyCacheRefreshIntervalSeconds = new ConfigKey<>("Advanced", Long.class, "config.key.expire.seconds", String.valueOf(CONFIG_CACHE_EXPIRE_SECONDS),
+            "Configuration keys cache refresh interval in seconds", false);
+    protected final ConfigKey<Boolean> ConfigKeyCacheRefreshAfterWrite = new ConfigKey<>("Advanced", Boolean.class, "config.key.cache.refresh.after.write", "false",
+            "When true the configuration cache refreshes entries asynchronously and serves the stale value during reload (non-blocking); when false entries expire and the next read blocks to load a fresh value (stronger consistency across management servers)", false);
+
     @Inject
     ConfigurationDao _configDao;
     @Inject
@@ -87,15 +96,13 @@ public class ConfigDepotImpl implements ConfigDepot, ConfigDepotAdmin {
     List<ScopedConfigStorage> _scopedStorages;
     Set<Configurable> _configured = Collections.synchronizedSet(new HashSet<Configurable>());
     Set<String> newConfigs = Collections.synchronizedSet(new HashSet<>());
-    LazyCache<Ternary<String, ConfigKey.Scope, Long>, String> configCache;
+    volatile LazyCache<Ternary<String, ConfigKey.Scope, Long>, String> configCache;
 
     private HashMap<String, Pair<String, ConfigKey<?>>> _allKeys = new HashMap<String, Pair<String, ConfigKey<?>>>(1007);
 
     HashMap<ConfigKey.Scope, Set<ConfigKey<?>>> _scopeLevelConfigsMap = new HashMap<ConfigKey.Scope, Set<ConfigKey<?>>>();
 
     public ConfigDepotImpl() {
-        configCache = new LazyCache<>(512,
-                CONFIG_CACHE_EXPIRE_SECONDS, this::getConfigStringValueInternal);
         ConfigKey.init(this);
         createEmptyScopeLevelMappings();
     }
@@ -121,7 +128,63 @@ public ConfigKey<?> get(String key) {
         return value != null ? value.second() : null;
     }
 
-    @PostConstruct
+    @SuppressWarnings("unchecked")
+    private <T> T getConfigValue(ConfigKey<T> configKey) {
+        String valueString;
+        try {
+            valueString = _configDao.getValueByKey(configKey.key());
+            if (valueString == null) {
+                valueString = configKey.defaultValue();
+            }
+        } catch (CloudRuntimeException e) {
+            String msg = "Failed to retrieve configuration value for: " + configKey.key();
+            logger.error(msg, e);
+            throw e;
+        } catch (Exception e) {
+            String msg = "Failed to retrieve configuration value for: " + configKey.key();
+            logger.error(msg, e);
+            throw new CloudRuntimeException(msg, e);
+        }
+        return (T) ConvertUtils.convert(valueString, configKey.type());
+    }
+
+    /**
+     * Lazily initialize the config cache on first access. Reading the cache size and
+     * TTL here is safe because by the time any caller exercises the cache, Spring's
+     * refresh() has completed and DatabaseUpgradeChecker has run any pending schema
+     * migrations, so the configuration table is in its expected shape.
+     *
+     * populateConfiguration(this) is invoked here to guarantee that this bean's own
+     * ConfigKeys end up registered in _allKeys and persisted to the configuration
+     * table even when Spring's List<Configurable> autowiring excludes self. The call
+     * is idempotent: if ConfigurationServerImpl.populateConfigurations() already
+     * iterated over this bean, the _configured guard inside populateConfiguration
+     * makes it a no-op.
+     *
+     * The cache-tuning keys are read directly from the configuration table via
+     * getConfigValue(); they fall back to their defaults when no row is present. They are
+     * applied only here at initialization (the cache is built once), so changing them
+     * requires a restart.
+     */
+    private void ensureCacheInitialized() {
+        if (configCache == null) {
+            synchronized (this) {
+                if (configCache == null) {
+                    populateConfiguration(this);
+                    Long maxSize = getConfigValue(ConfigKeyCacheMaxSize);
+                    Long expirationSeconds = getConfigValue(ConfigKeyCacheRefreshIntervalSeconds);
+                    Boolean refreshAfterWrite = getConfigValue(ConfigKeyCacheRefreshAfterWrite);
+                    if (logger.isDebugEnabled()) {
+                        logger.debug("{} value: {}", ConfigKeyCacheMaxSize.key(), maxSize);
+                        logger.debug("{} value: {}", ConfigKeyCacheRefreshIntervalSeconds.key(), expirationSeconds);
+                        logger.debug("{} value: {}", ConfigKeyCacheRefreshAfterWrite.key(), refreshAfterWrite);
+                    }
+                    configCache = new LazyCache<>(maxSize, expirationSeconds, refreshAfterWrite, this::getConfigStringValueInternal);
+                }
+            }
+        }
+    }
+
     @Override
     public void populateConfigurations() {
         Date date = new Date();
@@ -282,6 +345,7 @@ protected String getConfigStringValueInternal(Ternary<String, ConfigKey.Scope, L
         final String key = cacheKey.first();
         final ConfigKey.Scope scope = cacheKey.second();
         final Long scopeId = cacheKey.third();
+        logger.debug("Fetching config key from DB: key={}, scope={}, scopeId={}", key, scope, scopeId);
         if (!ConfigKey.Scope.Global.equals(scope) && scopeId != null) {
             ScopedConfigStorage scopedConfigStorage = getScopedStorage(scope);
             if (scopedConfigStorage == null) {
@@ -290,11 +354,7 @@ protected String getConfigStringValueInternal(Ternary<String, ConfigKey.Scope, L
             final ScopedConfigStorage scopedConfigStorageFinal = scopedConfigStorage;
             return Transaction.execute((TransactionCallback<String>) status -> scopedConfigStorageFinal.getConfigValue(scopeId, key));
         }
-        ConfigurationVO configurationVO = _configDao.findById(key);
-        if (configurationVO != null) {
-            return configurationVO.getValue();
-        }
-        return null;
+        return  _configDao.getValueByKey(key);
     }
 
     protected Ternary<String, ConfigKey.Scope, Long> getConfigCacheKey(String key, ConfigKey.Scope scope, Long scopeId) {
@@ -303,11 +363,22 @@ protected Ternary<String, ConfigKey.Scope, Long> getConfigCacheKey(String key, C
 
     @Override
     public String getConfigStringValue(String key, ConfigKey.Scope scope, Long scopeId) {
+        ensureCacheInitialized();
         return configCache.get(getConfigCacheKey(key, scope, scopeId));
     }
 
+    /**
+     * Inserts a value directly into the config cache without persisting to DB.
+     * Used to cache inherited values (e.g. from a parent scope) under a more specific scope key.
+     */
+    public void cacheValue(String key, ConfigKey.Scope scope, Long scopeId, String value) {
+        ensureCacheInitialized();
+        configCache.put(getConfigCacheKey(key, scope, scopeId), value);
+    }
+
     @Override
     public void invalidateConfigCache(String key, ConfigKey.Scope scope, Long scopeId) {
+        ensureCacheInitialized();
         configCache.invalidate(getConfigCacheKey(key, scope, scopeId));
     }
 
@@ -397,4 +468,14 @@ public Pair<ConfigKey.Scope, Long> getParentScope(ConfigKey.Scope scope, Long id
         }
         return scopedConfigStorage.getParentScope(id);
     }
+
+    @Override
+    public String getConfigComponentName() {
+        return ConfigDepotImpl.class.getSimpleName();
+    }
+
+    @Override
+    public ConfigKey<?>[] getConfigKeys() {
+        return new ConfigKey[]{ConfigKeyCacheMaxSize, ConfigKeyCacheRefreshIntervalSeconds, ConfigKeyCacheRefreshAfterWrite};
+    }
 }

framework/config/src/test/java/org/apache/cloudstack/framework/config/impl/ConfigDepotAdminTest.java

@@ -88,9 +88,9 @@ public void setUp() throws Exception {
         _depotAdmin._configDao = _configDao;
         _depotAdmin._configGroupDao = _configGroupDao;
         _depotAdmin._configSubGroupDao = _configSubGroupDao;
-        _depotAdmin._configurables = new ArrayList<Configurable>();
+        _depotAdmin._configurables = new ArrayList<>();
         _depotAdmin._configurables.add(_configurable);
-        _depotAdmin._scopedStorages = new ArrayList<ScopedConfigStorage>();
+        _depotAdmin._scopedStorages = new ArrayList<>();
         _depotAdmin._scopedStorages.add(_scopedStorage);
     }
 

framework/config/src/test/java/org/apache/cloudstack/framework/config/impl/ConfigDepotImplTest.java

@@ -30,6 +30,7 @@
 import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
 import org.apache.cloudstack.framework.config.dao.ConfigurationSubGroupDao;
 import org.junit.Assert;
+import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.ArgumentCaptor;
@@ -41,6 +42,7 @@
 
 import com.cloud.utils.Pair;
 
+
 @RunWith(MockitoJUnitRunner.class)
 public class ConfigDepotImplTest {
 
@@ -55,6 +57,12 @@ public class ConfigDepotImplTest {
     @InjectMocks
     private ConfigDepotImpl configDepotImpl = new ConfigDepotImpl();
 
+    @Before
+    public void setUp() {
+        configDepotImpl.setConfigurables(Collections.emptyList());
+        configDepotImpl.populateConfigurations();
+    }
+
     @Test
     public void createConfigObjectPersistsSubGroupWithNameAndGroupId() {
         ConfigKey<?> key = Mockito.mock(ConfigKey.class);
@@ -102,9 +110,7 @@ public void testIsNewConfig() {
     }
 
     private void runTestGetConfigStringValue(String key, String value) {
-        ConfigurationVO configurationVO = Mockito.mock(ConfigurationVO.class);
-        Mockito.when(configurationVO.getValue()).thenReturn(value);
-        Mockito.when(_configDao.findById(key)).thenReturn(configurationVO);
+        Mockito.when(_configDao.getValueByKey(key)).thenReturn(value);
         String result = configDepotImpl.getConfigStringValue(key, ConfigKey.Scope.Global, null);
         Assert.assertEquals(value, result);
     }
@@ -131,7 +137,7 @@ private void runTestGetConfigStringValueExpiry(long wait, int configDBRetrieval)
         }
         String result = configDepotImpl.getConfigStringValue(key, ConfigKey.Scope.Global, null);
         Assert.assertEquals(value, result);
-        Mockito.verify(_configDao, Mockito.times(configDBRetrieval)).findById(key);
+        Mockito.verify(_configDao, Mockito.timeout(2000).times(configDBRetrieval)).getValueByKey(key);
     }
 
     @Test
@@ -199,6 +205,58 @@ public ConfigKey<?>[] getConfigKeys() {
         Mockito.verify(_configDao, Mockito.times(1)).persist(configurationVO);
     }
 
+    @Test
+    public void testParentScopeValueCachedUnderChildScopeOnMiss() {
+        String keyName = "test.key";
+        ConfigKey<String> key = new ConfigKey<>(ConfigKey.CATEGORY_ADVANCED, String.class,
+                keyName, "default-value", "test", true, List.of(ConfigKey.Scope.Cluster, ConfigKey.Scope.Zone));
+
+        Long clusterId = 1L;
+        Long zoneId = 2L;
+        String zoneValue = "zone-value";
+
+        ScopedConfigStorage clusterStorage = Mockito.mock(ScopedConfigStorage.class);
+        Mockito.when(clusterStorage.getScope()).thenReturn(ConfigKey.Scope.Cluster);
+        Mockito.when(clusterStorage.getConfigValue(clusterId, keyName)).thenReturn(null);
+        Mockito.when(clusterStorage.getParentScope(clusterId)).thenReturn(new Pair<>(ConfigKey.Scope.Zone, zoneId));
+
+        ScopedConfigStorage zoneStorage = Mockito.mock(ScopedConfigStorage.class);
+        Mockito.when(zoneStorage.getScope()).thenReturn(ConfigKey.Scope.Zone);
+        Mockito.when(zoneStorage.getConfigValue(zoneId, keyName)).thenReturn(zoneValue);
+
+        configDepotImpl.setScopedStorages(List.of(clusterStorage, zoneStorage));
+
+        // first call: no cluster value, traverses to zone, zone value is cached under cluster scope key
+        Assert.assertEquals(zoneValue, key.valueInScope(ConfigKey.Scope.Cluster, clusterId));
+
+        // second call: cache hit with zone value, cluster storage not queried again
+        Assert.assertEquals(zoneValue, key.valueInScope(ConfigKey.Scope.Cluster, clusterId));
+        Mockito.verify(clusterStorage, Mockito.times(1)).getConfigValue(clusterId, keyName);
+    }
+
+    @Test
+    public void testDefaultValueCachedUnderChildScopeOnMiss() {
+        String keyName = "test.key";
+        String keyValue = "default-value";
+        ConfigKey<String> key = new ConfigKey<>(ConfigKey.CATEGORY_ADVANCED, String.class,
+                keyName, keyValue, "test", true, ConfigKey.Scope.Cluster);
+
+        Long clusterId = 1L;
+
+        ScopedConfigStorage clusterStorage = Mockito.mock(ScopedConfigStorage.class);
+        Mockito.when(clusterStorage.getScope()).thenReturn(ConfigKey.Scope.Cluster);
+        Mockito.when(clusterStorage.getConfigValue(clusterId, keyName)).thenReturn(null);
+
+        configDepotImpl.setScopedStorages(List.of(clusterStorage));
+
+        // first call: no cluster value, traverses to default, default value is cached under cluster scope key
+        Assert.assertEquals(keyValue, key.valueInScope(ConfigKey.Scope.Cluster, clusterId));
+
+        // second call: cache hit with default value, cluster storage not queried again
+        Assert.assertEquals(keyValue, key.valueInScope(ConfigKey.Scope.Cluster, clusterId));
+        Mockito.verify(clusterStorage, Mockito.times(1)).getConfigValue(clusterId, keyName);
+    }
+
     @Test
     public void getParentScopeWithValidScope() {
         ConfigKey.Scope scope = ConfigKey.Scope.Cluster;
@@ -217,4 +275,36 @@ public void getParentScopeWithValidScope() {
         Assert.assertEquals(parentScope, result.first());
         Assert.assertEquals(parentId, result.second());
     }
+
+    @Test
+    public void testCacheNotInitializedBeforeFirstAccess() {
+        Assert.assertNull("configCache should be null until first access triggers lazy init",
+                configDepotImpl.configCache);
+    }
+
+    @Test
+    public void testCacheInitializedOnFirstAccess() {
+        Assert.assertNull(configDepotImpl.configCache);
+        configDepotImpl.getConfigStringValue("anyKey", ConfigKey.Scope.Global, null);
+        Assert.assertNotNull("configCache should be initialized after first access",
+                configDepotImpl.configCache);
+    }
+
+    @Test
+    public void testCacheInitializedOnlyOnce() {
+        configDepotImpl.getConfigStringValue("key1", ConfigKey.Scope.Global, null);
+        configDepotImpl.getConfigStringValue("key2", ConfigKey.Scope.Global, null);
+        configDepotImpl.getConfigStringValue("key3", ConfigKey.Scope.Global, null);
+        Mockito.verify(_configDao, Mockito.times(1)).getValueByKey("config.key.cache.max.size");
+        Mockito.verify(_configDao, Mockito.times(1)).getValueByKey("config.key.expire.seconds");
+    }
+
+    @Test
+    public void testCacheUsesDefaultsWhenConfigKeysAbsentInDB() {
+        Mockito.when(_configDao.getValueByKey("config.key.cache.max.size")).thenReturn(null);
+        Mockito.when(_configDao.getValueByKey("config.key.expire.seconds")).thenReturn(null);
+        configDepotImpl.getConfigStringValue("anyKey", ConfigKey.Scope.Global, null);
+        Assert.assertNotNull("Cache should initialize successfully with defaults when DB returns null",
+                configDepotImpl.configCache);
+    }
 }

server/src/test/java/com/cloud/vpc/dao/MockConfigurationDaoImpl.java

@@ -123,4 +123,9 @@ public void invalidateCache() {
     public List<ConfigurationVO> searchPartialConfigurations() {
         return List.of();
     }
+
+    @Override
+    public String getValueByKey(String key) {
+        return null;
+    }
 }