Implement changes indexes lock files check
Detect the following failures of both open and closed changes indexes
write.lock files:
* write.lock file is missing
* write.lock file is not writeable by Gerrit
Notes:
* so far only changes indexes are subject of check
* in order to test it `UseLocalDisk` annotation has to be applied (and
it is heavy operation) therefore `HealthCheckIT` was modified so that
`changesindex` healthcheck is disabled for all test cases (or
additionally disabled in case when some checks get disabled)
* `changesindex` healthcheck test cases were added to a dedicated
`ChangesIndexHealthCheckIT` file
* common functions from `HealthCheckIT` and `ChangesIndexHealthCheckIT`
were introduced in `AbstractHealthCheckIntegrationTest`
The write.lock ownership change detection was tested manually with the
following steps:
* docker run --rm -p 8080:8080 -p 29418:29418 -ti --name gerrit_3.9.1 gerritcodereview/gerrit:3.9.1
* docker cp bazel-bin/plugins/healthcheck/healthcheck.jar gerrit_3.9.1:/var/gerrit/plugins
* curl localhost:8080/config/server/healthcheck~status returned
"changesindex": {
"result": "passed",
"ts": 1703147883563,
"elapsed": 0
}
* docker exec -u root -it gerrit_3.9.1 /bin/bash
* chown root:root /var/gerrit/index/changes_0084/open/write.lock
* curl localhost:8080/config/server/healthcheck~status returned
"changesindex": {
"result": "failed",
"ts": 1703148022276,
"elapsed": 0
}
IOW it works as expected.
Bug: Issue 40015289
Change-Id: I66a4053483e2c10eb28c150782c33cfd10dc2b15
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/ChangesIndexHealthCheck.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/ChangesIndexHealthCheck.java
index 584787a..176cea9 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/ChangesIndexHealthCheck.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/ChangesIndexHealthCheck.java
@@ -20,32 +20,46 @@
import com.google.gerrit.index.IndexType;
import com.google.gerrit.metrics.MetricMaker;
import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.config.SitePaths;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import com.googlesource.gerrit.plugins.healthcheck.HealthCheckConfig;
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import java.util.Optional;
+import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.storage.file.FileBasedConfig;
+import org.eclipse.jgit.util.FS;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@Singleton
public class ChangesIndexHealthCheck extends AbstractHealthCheck {
private static final Logger log = LoggerFactory.getLogger(QueryChangesHealthCheck.class);
+ private static final String lockFilename = "write.lock";
private final boolean isLuceneIndex;
+ private final Optional<ChangesIndexLockFiles> changes;
@Inject
ChangesIndexHealthCheck(
@GerritServerConfig Config cfg,
ListeningExecutorService executor,
HealthCheckConfig config,
- MetricMaker metricMaker) {
+ MetricMaker metricMaker,
+ SitePaths sitePaths) {
super(executor, config, CHANGES_INDEX, metricMaker);
this.isLuceneIndex = isIndexTypeLucene(cfg);
+ this.changes = getChangesIndexLockFiles(sitePaths.index_dir);
}
@Override
protected Result doCheck() throws Exception {
- return isLuceneIndex ? Result.PASSED : Result.DISABLED;
+ return isLuceneIndex
+ ? changes.map(locks -> locks.valid() ? Result.PASSED : Result.FAILED).orElse(Result.FAILED)
+ : Result.DISABLED;
}
private static boolean isIndexTypeLucene(Config cfg) {
@@ -58,4 +72,52 @@
}
return isLucene;
}
+
+ private static Optional<ChangesIndexLockFiles> getChangesIndexLockFiles(Path indexDir) {
+ FileBasedConfig cfg =
+ new FileBasedConfig(indexDir.resolve("gerrit_index.config").toFile(), FS.detect());
+ try {
+ cfg.load();
+ return getActiveIndexVersion(cfg, "changes")
+ .map(version -> getChangesLockFiles(indexDir, version));
+ } catch (IOException | ConfigInvalidException e) {
+ log.error("Getting changes index version from configuration failed", e);
+ }
+ return Optional.empty();
+ }
+
+ private static Optional<String> getActiveIndexVersion(Config cfg, String indexPrefix) {
+ String section = "index";
+ return cfg.getSubsections(section).stream()
+ .filter(
+ subsection ->
+ subsection.startsWith(indexPrefix)
+ && cfg.getBoolean(section, subsection, "ready", false))
+ .findAny();
+ }
+
+ private static ChangesIndexLockFiles getChangesLockFiles(Path indexDir, String indexVersion) {
+ Path versionDir = indexDir.resolve(indexVersion);
+ return new ChangesIndexLockFiles(
+ versionDir.resolve("open").resolve(lockFilename).toFile(),
+ versionDir.resolve("closed").resolve(lockFilename).toFile());
+ }
+
+ private static class ChangesIndexLockFiles {
+ private final File openLock;
+ private final File closedLock;
+
+ private ChangesIndexLockFiles(File openLock, File closedLock) {
+ this.openLock = openLock;
+ this.closedLock = closedLock;
+ }
+
+ private boolean valid() {
+ return validLock(openLock) && validLock(closedLock);
+ }
+
+ private static boolean validLock(File writeLock) {
+ return writeLock.isFile() && writeLock.canWrite();
+ }
+ }
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/AbstractHealthCheckIntegrationTest.java b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/AbstractHealthCheckIntegrationTest.java
new file mode 100644
index 0000000..cd3862e
--- /dev/null
+++ b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/AbstractHealthCheckIntegrationTest.java
@@ -0,0 +1,87 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.plugins.healthcheck;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
+import com.google.gerrit.acceptance.RestResponse;
+import com.google.gerrit.extensions.annotations.PluginName;
+import com.google.gson.Gson;
+import com.google.gson.JsonObject;
+import com.google.inject.AbstractModule;
+import com.google.inject.Key;
+import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheckNames;
+import java.io.IOException;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.junit.Ignore;
+
+@Ignore
+public class AbstractHealthCheckIntegrationTest extends LightweightPluginDaemonTest {
+ public static class TestModule extends AbstractModule {
+
+ @Override
+ protected void configure() {
+ install(new HealthCheckExtensionApiModule());
+ install(new Module());
+ }
+ }
+
+ protected HealthCheckConfig config;
+ protected String healthCheckUriPath;
+
+ private final Gson gson = new Gson();
+
+ @Override
+ public void setUpTestPlugin() throws Exception {
+ super.setUpTestPlugin();
+
+ config = plugin.getSysInjector().getInstance(HealthCheckConfig.class);
+
+ int numChanges = config.getLimit(HealthCheckNames.QUERYCHANGES);
+ for (int i = 0; i < numChanges; i++) {
+ createChange("refs/for/master");
+ }
+ accountCreator.create(config.getUsername(HealthCheckNames.AUTH));
+
+ healthCheckUriPath =
+ String.format(
+ "/config/server/%s~status",
+ plugin.getSysInjector().getInstance(Key.get(String.class, PluginName.class)));
+ }
+
+ protected RestResponse getHealthCheckStatus() throws IOException {
+ return adminRestSession.get(healthCheckUriPath);
+ }
+
+ protected RestResponse getHealthCheckStatusAnonymously() throws IOException {
+ return anonymousRestSession.get(healthCheckUriPath);
+ }
+
+ protected void assertCheckResult(JsonObject respPayload, String checkName, String result) {
+ assertThat(respPayload.has(checkName)).isTrue();
+ JsonObject reviewDbStatus = respPayload.get(checkName).getAsJsonObject();
+ assertThat(reviewDbStatus.has("result")).isTrue();
+ assertThat(reviewDbStatus.get("result").getAsString()).isEqualTo(result);
+ }
+
+ protected void disableCheck(String check) throws ConfigInvalidException {
+ config.fromText(String.format("[healthcheck \"%s\"]\n" + "enabled = false", check));
+ }
+
+ protected JsonObject getResponseJson(RestResponse resp) throws IOException {
+ return gson.fromJson(resp.getReader(), JsonObject.class);
+ }
+}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/ChangesIndexHealthCheckIT.java b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/ChangesIndexHealthCheckIT.java
new file mode 100644
index 0000000..33d3277
--- /dev/null
+++ b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/ChangesIndexHealthCheckIT.java
@@ -0,0 +1,120 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.plugins.healthcheck;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.googlesource.gerrit.plugins.healthcheck.check.HealthCheckNames.CHANGES_INDEX;
+
+import com.google.gerrit.acceptance.RestResponse;
+import com.google.gerrit.acceptance.Sandboxed;
+import com.google.gerrit.acceptance.TestPlugin;
+import com.google.gerrit.acceptance.UseLocalDisk;
+import com.google.gerrit.acceptance.config.GerritConfig;
+import com.google.gerrit.server.config.SitePaths;
+import com.google.inject.Inject;
+import java.io.IOException;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Iterator;
+import javax.servlet.http.HttpServletResponse;
+import org.junit.Test;
+
+@TestPlugin(
+ name = "healthcheck-test",
+ sysModule =
+ "com.googlesource.gerrit.plugins.healthcheck.AbstractHealthCheckIntegrationTest$TestModule",
+ httpModule = "com.googlesource.gerrit.plugins.healthcheck.HttpModule")
+@Sandboxed
+public class ChangesIndexHealthCheckIT extends AbstractHealthCheckIntegrationTest {
+ @Inject SitePaths sitePaths;
+
+ @Test
+ @UseLocalDisk
+ @GerritConfig(name = "index.type", value = "lucene")
+ public void shouldReturnChangesIndexCheck() throws Exception {
+ RestResponse resp = getHealthCheckStatus();
+ resp.assertOK();
+
+ assertCheckResult(getResponseJson(resp), CHANGES_INDEX, "passed");
+ }
+
+ @Test
+ @UseLocalDisk
+ @GerritConfig(name = "index.type", value = "lucene")
+ public void shouldFailWhenOpenChangesIndexWriteLockIsMissing() throws Exception {
+ shouldFailWhenChangesIndexWriteLockIsMissing("open");
+ }
+
+ @Test
+ @UseLocalDisk
+ @GerritConfig(name = "index.type", value = "lucene")
+ public void shouldFailWhenClosedChangesIndexWriteLockIsMissing() throws Exception {
+ shouldFailWhenChangesIndexWriteLockIsMissing("closed");
+ }
+
+ private void shouldFailWhenChangesIndexWriteLockIsMissing(String indexType) throws Exception {
+ RestResponse resp = getHealthCheckStatus();
+ resp.assertOK();
+
+ assertCheckResult(getResponseJson(resp), CHANGES_INDEX, "passed");
+
+ Path openChangesIndexLockPath = getIndexLockFile(sitePaths.index_dir, indexType);
+ assertThat(
+ openChangesIndexLockPath
+ .toFile()
+ .renameTo(
+ openChangesIndexLockPath
+ .getParent()
+ .resolve(openChangesIndexLockPath.getFileName().toString() + ".backup")
+ .toFile()))
+ .isTrue();
+
+ resp = getHealthCheckStatus();
+ resp.assertStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+
+ assertCheckResult(getResponseJson(resp), CHANGES_INDEX, "failed");
+ }
+
+ @Test
+ @GerritConfig(name = "index.type", value = "lucene")
+ public void shouldReturnChangesIndexCheckAsDisabled() throws Exception {
+ disableCheck(CHANGES_INDEX);
+ RestResponse resp = getHealthCheckStatus();
+
+ resp.assertOK();
+ assertCheckResult(getResponseJson(resp), CHANGES_INDEX, "disabled");
+ }
+
+ @Test
+ @GerritConfig(name = "index.type", value = "fake")
+ public void shouldReturnChangesIndexCheckAsDisabledWhenIndexIsNotLucene() throws Exception {
+ RestResponse resp = getHealthCheckStatus();
+
+ resp.assertOK();
+ assertCheckResult(getResponseJson(resp), CHANGES_INDEX, "disabled");
+ }
+
+ private Path getIndexLockFile(Path indexDir, String indexType) throws IOException {
+ try (DirectoryStream<Path> dirStream = Files.newDirectoryStream(indexDir, "changes_*")) {
+ Iterator<Path> changesDir = dirStream.iterator();
+ if (changesDir.hasNext()) {
+ return changesDir.next().resolve(indexType).resolve("write.lock");
+ }
+ throw new RuntimeException(
+ String.format("there is no `changes_*` dir under [%s] index dir", indexDir));
+ }
+ }
+}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckIT.java b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckIT.java
index 1aa698c..a0c9885 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckIT.java
@@ -22,17 +22,11 @@
import static com.googlesource.gerrit.plugins.healthcheck.check.HealthCheckNames.JGIT;
import static com.googlesource.gerrit.plugins.healthcheck.check.HealthCheckNames.QUERYCHANGES;
-import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.acceptance.Sandboxed;
import com.google.gerrit.acceptance.TestPlugin;
import com.google.gerrit.acceptance.config.GerritConfig;
-import com.google.gerrit.extensions.annotations.PluginName;
-import com.google.gson.Gson;
import com.google.gson.JsonObject;
-import com.google.inject.AbstractModule;
-import com.google.inject.Key;
-import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheckNames;
import java.io.File;
import java.io.IOException;
import javax.servlet.http.HttpServletResponse;
@@ -42,43 +36,24 @@
@TestPlugin(
name = "healthcheck-test",
- sysModule = "com.googlesource.gerrit.plugins.healthcheck.HealthCheckIT$TestModule",
+ sysModule =
+ "com.googlesource.gerrit.plugins.healthcheck.AbstractHealthCheckIntegrationTest$TestModule",
httpModule = "com.googlesource.gerrit.plugins.healthcheck.HttpModule")
@Sandboxed
-public class HealthCheckIT extends LightweightPluginDaemonTest {
- Gson gson = new Gson();
- HealthCheckConfig config;
- String healthCheckUriPath;
- String failFilePath;
-
- public static class TestModule extends AbstractModule {
-
- @Override
- protected void configure() {
- install(new HealthCheckExtensionApiModule());
- install(new Module());
- }
- }
+public class HealthCheckIT extends AbstractHealthCheckIntegrationTest {
+ private String failFilePath = "/tmp/fail";
@Override
@Before
public void setUpTestPlugin() throws Exception {
super.setUpTestPlugin();
- config = plugin.getSysInjector().getInstance(HealthCheckConfig.class);
- failFilePath = "/tmp/fail";
new File(failFilePath).delete();
- int numChanges = config.getLimit(HealthCheckNames.QUERYCHANGES);
- for (int i = 0; i < numChanges; i++) {
- createChange("refs/for/master");
- }
- accountCreator.create(config.getUsername(HealthCheckNames.AUTH));
-
- healthCheckUriPath =
- String.format(
- "/config/server/%s~status",
- plugin.getSysInjector().getInstance(Key.get(String.class, PluginName.class)));
+ // disable `changesindex` check as it requires @UseLocalDisk annotation (so that all operations
+ // are persisted to local FS) to be applied and that would degrade this IT performance - see
+ // ChangesIndexHealthCheckIT for a changes index dedicated integration tests
+ super.disableCheck(CHANGES_INDEX);
}
@Test
@@ -225,30 +200,14 @@
assertThat(respBody.get("reason").getAsString()).isEqualTo("Fail Flag File exists");
}
- @Test
- public void shouldReturnChangesIndexCheck() throws Exception {
- RestResponse resp = getHealthCheckStatus();
- resp.assertOK();
-
- assertCheckResult(getResponseJson(resp), CHANGES_INDEX, "passed");
- }
-
- @Test
- public void shouldReturnChangesIndexCheckAsDisabled() throws Exception {
- disableCheck(CHANGES_INDEX);
- RestResponse resp = getHealthCheckStatus();
-
- resp.assertOK();
- assertCheckResult(getResponseJson(resp), CHANGES_INDEX, "disabled");
- }
-
- @Test
- @GerritConfig(name = "index.type", value = "fake")
- public void shouldReturnChangesIndexCheckAsDisabledWhenIndexIsNotLucene() throws Exception {
- RestResponse resp = getHealthCheckStatus();
-
- resp.assertOK();
- assertCheckResult(getResponseJson(resp), CHANGES_INDEX, "disabled");
+ @Override
+ protected void disableCheck(String check) throws ConfigInvalidException {
+ // additionally disable `changesindex` healthcheck as it requires @UseLocalDisk annotation to
+ // run properly
+ config.fromText(
+ String.format(
+ "[healthcheck \"%s\"]\n enabled = false\n[healthcheck \"%s\"]\n enabled = false",
+ check, CHANGES_INDEX));
}
private void createFailFileFlag(String path) throws IOException {
@@ -257,31 +216,7 @@
file.deleteOnExit();
}
- private RestResponse getHealthCheckStatus() throws IOException {
- return adminRestSession.get(healthCheckUriPath);
- }
-
- private RestResponse getHealthCheckStatusAnonymously() throws IOException {
- return anonymousRestSession.get(healthCheckUriPath);
- }
-
- private void assertCheckResult(JsonObject respPayload, String checkName, String result) {
- assertThat(respPayload.has(checkName)).isTrue();
- JsonObject reviewDbStatus = respPayload.get(checkName).getAsJsonObject();
- assertThat(reviewDbStatus.has("result")).isTrue();
- assertThat(reviewDbStatus.get("result").getAsString()).isEqualTo(result);
- }
-
- private void disableCheck(String check) throws ConfigInvalidException {
- config.fromText(String.format("[healthcheck \"%s\"]\n" + "enabled = false", check));
- }
-
private void setFailFlagFilePath(String path) throws ConfigInvalidException {
config.fromText(String.format("[healthcheck]\n" + "failFileFlagPath = %s", path));
}
-
- private JsonObject getResponseJson(RestResponse resp) throws IOException {
- JsonObject respPayload = gson.fromJson(resp.getReader(), JsonObject.class);
- return respPayload;
- }
}
