Prevent Gerrit from starting when the cache dir is not writeable
When the cache directory was not accessible and writeable by the Gerrit
user, an error was logged but Gerrit was starting without persistent
cache anyway.
Do not hide cache directory issues but rather expose the error so that
the Gerrit server will not start inadvertently without persistent cache.
Bug: Issue 13415
Change-Id: I35191ba4d7bdfc6d12caeddec8ad733fa87f4cf1
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfig.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfig.java
index 782c216..7c593f2 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfig.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfig.java
@@ -65,7 +65,8 @@
@Assisted("ConfigKey") String configKey,
@Assisted("DiskLimit") long diskLimit,
@Nullable @Assisted("ExpireAfterWrite") Duration expireAfterWrite,
- @Nullable @Assisted("RefreshAfterWrite") Duration refreshAfterWrite) {
+ @Nullable @Assisted("RefreshAfterWrite") Duration refreshAfterWrite)
+ throws IOException {
final Path cacheDir = getCacheDir(site, cfg.getString("cache", null, "directory"));
this.persistedFile =
cacheDir != null ? cacheDir.resolve(String.format("%s.dat", name)).toFile() : null;
@@ -124,22 +125,16 @@
return maxBloatFactor;
}
- private static Path getCacheDir(SitePaths site, String name) {
+ private static Path getCacheDir(SitePaths site, String name) throws IOException {
if (name == null) {
return null;
}
Path loc = site.resolve(name);
if (!Files.exists(loc)) {
- try {
- Files.createDirectories(loc);
- } catch (IOException e) {
- logger.atWarning().log("Can't create disk cache: %s", loc.toAbsolutePath());
- return null;
- }
+ Files.createDirectories(loc);
}
if (!Files.isWritable(loc)) {
- logger.atWarning().log("Can't write to disk cache: %s", loc.toAbsolutePath());
- return null;
+ throw new IOException(String.format("Can't write to disk cache: %s", loc.toAbsolutePath()));
}
logger.atFine().log("Enabling disk cache %s", loc.toAbsolutePath());
return loc;
diff --git a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfigTest.java b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfigTest.java
index 5eaeabb..ee4d7c1 100644
--- a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfigTest.java
+++ b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfigTest.java
@@ -14,12 +14,14 @@
package com.googlesource.gerrit.modules.cache.chroniclemap;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static com.googlesource.gerrit.modules.cache.chroniclemap.ChronicleMapCacheConfig.DEFAULT_AVG_KEY_SIZE;
import static com.googlesource.gerrit.modules.cache.chroniclemap.ChronicleMapCacheConfig.DEFAULT_AVG_VALUE_SIZE;
import static com.googlesource.gerrit.modules.cache.chroniclemap.ChronicleMapCacheConfig.DEFAULT_MAX_BLOAT_FACTOR;
import static com.googlesource.gerrit.modules.cache.chroniclemap.ChronicleMapCacheConfig.DEFAULT_MAX_ENTRIES;
import com.google.gerrit.server.config.SitePaths;
+import java.io.IOException;
import java.nio.file.Files;
import java.time.Duration;
import org.eclipse.jgit.lib.StoredConfig;
@@ -70,7 +72,7 @@
}
@Test
- public void shouldNotProvidePersistedFileWhenCacheDirIsNotConfigured() {
+ public void shouldNotProvidePersistedFileWhenCacheDirIsNotConfigured() throws Exception {
assertThat(configUnderTest(gerritConfig).getPersistedFile()).isNull();
}
@@ -84,7 +86,7 @@
}
@Test
- public void shouldProvideDefinitionDiskLimitWhenNotConfigured() {
+ public void shouldProvideDefinitionDiskLimitWhenNotConfigured() throws Exception {
assertThat(configUnderTest(gerritConfig).getDiskLimit()).isEqualTo(definitionDiskLimit);
}
@@ -98,7 +100,7 @@
}
@Test
- public void shouldProvideDefaultMaxEntriesWhenNotConfigured() {
+ public void shouldProvideDefaultMaxEntriesWhenNotConfigured() throws Exception {
assertThat(configUnderTest(gerritConfig).getMaxEntries()).isEqualTo(DEFAULT_MAX_ENTRIES);
}
@@ -112,7 +114,7 @@
}
@Test
- public void shouldProvideDefaultAverageKeySizeWhenNotConfigured() {
+ public void shouldProvideDefaultAverageKeySizeWhenNotConfigured() throws Exception {
assertThat(configUnderTest(gerritConfig).getAverageKeySize()).isEqualTo(DEFAULT_AVG_KEY_SIZE);
}
@@ -126,13 +128,13 @@
}
@Test
- public void shouldProvideDefaultAverageValueSizeWhenNotConfigured() {
+ public void shouldProvideDefaultAverageValueSizeWhenNotConfigured() throws Exception {
assertThat(configUnderTest(gerritConfig).getAverageValueSize())
.isEqualTo(DEFAULT_AVG_VALUE_SIZE);
}
@Test
- public void shouldProvideMaxDefaultBloatFactorWhenNotConfigured() {
+ public void shouldProvideMaxDefaultBloatFactorWhenNotConfigured() throws Exception {
assertThat(configUnderTest(gerritConfig).getMaxBloatFactor())
.isEqualTo(DEFAULT_MAX_BLOAT_FACTOR);
}
@@ -157,7 +159,7 @@
}
@Test
- public void shouldProvideDefinitionExpireAfterWriteWhenNotConfigured() {
+ public void shouldProvideDefinitionExpireAfterWriteWhenNotConfigured() throws Exception {
assertThat(configUnderTest(gerritConfig).getExpireAfterWrite()).isEqualTo(expireAfterWrite);
}
@@ -172,11 +174,29 @@
}
@Test
- public void shouldProvideDefinitionRefreshAfterWriteWhenNotConfigured() {
+ public void shouldThrowExceptionWhenDirectoryDoesntExist() throws Exception {
+ gerritConfig.setString("cache", null, "directory", "/var/bar/foobar");
+ gerritConfig.save();
+
+ IOException thrown = assertThrows(IOException.class, () -> configUnderTest(gerritConfig));
+ assertThat(thrown).hasMessageThat().contains("Operation not permitted");
+ }
+
+ @Test
+ public void shouldThrowExceptionWhenDirectoryIsNotWriteable() throws Exception {
+ gerritConfig.setString("cache", null, "directory", "/var");
+ gerritConfig.save();
+
+ IOException thrown = assertThrows(IOException.class, () -> configUnderTest(gerritConfig));
+ assertThat(thrown).hasMessageThat().contains("Can't write to disk cache");
+ }
+
+ @Test
+ public void shouldProvideDefinitionRefreshAfterWriteWhenNotConfigured() throws Exception {
assertThat(configUnderTest(gerritConfig).getRefreshAfterWrite()).isEqualTo(refreshAfterWrite);
}
- private ChronicleMapCacheConfig configUnderTest(StoredConfig gerritConfig) {
+ private ChronicleMapCacheConfig configUnderTest(StoredConfig gerritConfig) throws IOException {
return new ChronicleMapCacheConfig(
gerritConfig,
sitePaths,
