Merge "Allow batch-fetch endpoint to delete refs"
diff --git a/BUILD b/BUILD index 5825e1d..4070d7c 100644 --- a/BUILD +++ b/BUILD
@@ -16,11 +16,11 @@ ], resources = glob(["src/main/resources/**/*"]), deps = [ + ":delete-project-neverlink", ":events-broker-neverlink", ":healthcheck-neverlink", + ":replication-neverlink", "//lib/commons:io", - "//plugins/delete-project", - "//plugins/replication", "@commons-lang3//jar", ], ) @@ -38,6 +38,7 @@ ":pull_replication_util", "//plugins/delete-project", "//plugins/events-broker", + "//plugins/healthcheck", "//plugins/replication", ], ) @@ -48,6 +49,7 @@ tags = ["pull-replication"], visibility = ["//visibility:public"], deps = PLUGIN_TEST_DEPS + PLUGIN_DEPS + [ + ":healthcheck-neverlink", ":pull-replication__plugin", ":pull_replication_util", "//plugins/replication", @@ -67,6 +69,7 @@ deps = PLUGIN_TEST_DEPS + PLUGIN_DEPS + [ ":pull-replication__plugin", "//plugins/delete-project", + "//plugins/healthcheck", "//plugins/replication", ], ) @@ -92,3 +95,15 @@ neverlink = 1, exports = ["//plugins/healthcheck"], ) + +java_library( + name = "replication-neverlink", + neverlink = 1, + exports = ["//plugins/replication"], +) + +java_library( + name = "delete-project-neverlink", + neverlink = 1, + exports = ["//plugins/delete-project"], +)
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/Source.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/Source.java index 0a55043..9283033 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/Source.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/Source.java
@@ -934,14 +934,22 @@ } } - long inflightTasksCount() { + public long inflightTasksCount() { return inFlight.size(); } - long pendingTasksCount() { + public long pendingTasksCount() { return pending.size(); } + public boolean zeroPendingTasksForRepo(Project.NameKey project) { + return pending.values().stream().noneMatch(fetch -> fetch.getProjectNameKey().equals(project)); + } + + public boolean zeroInflightTasksForRepo(Project.NameKey project) { + return inFlight.values().stream().noneMatch(fetch -> fetch.getProjectNameKey().equals(project)); + } + private static boolean matches(URIish uri, String urlMatch) { if (urlMatch == null || urlMatch.equals("") || urlMatch.equals("*")) { return true;
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/health/PullReplicationTasksHealthCheck.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/health/PullReplicationTasksHealthCheck.java index ea6fdb5..778a6cc 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/health/PullReplicationTasksHealthCheck.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/health/PullReplicationTasksHealthCheck.java
@@ -14,28 +14,107 @@ package com.googlesource.gerrit.plugins.replication.pull.health; +import com.google.common.base.Ticker; +import com.google.common.collect.ImmutableSet; import com.google.common.util.concurrent.ListeningExecutorService; +import com.google.gerrit.entities.Project; import com.google.gerrit.extensions.annotations.PluginName; import com.google.gerrit.metrics.MetricMaker; +import com.google.gerrit.server.config.ConfigUtil; import com.google.inject.Inject; import com.google.inject.Singleton; import com.googlesource.gerrit.plugins.healthcheck.HealthCheckConfig; import com.googlesource.gerrit.plugins.healthcheck.check.AbstractHealthCheck; +import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck; +import com.googlesource.gerrit.plugins.replication.MergedConfigResource; +import com.googlesource.gerrit.plugins.replication.pull.Source; +import com.googlesource.gerrit.plugins.replication.pull.SourcesCollection; +import java.util.List; +import java.util.Optional; +import java.util.Set; +import java.util.concurrent.TimeUnit; +import org.eclipse.jgit.lib.Config; @Singleton public class PullReplicationTasksHealthCheck extends AbstractHealthCheck { + private static final long DEFAULT_PERIOD_OF_TIME_SECS = 10L; + public static final String HEALTHCHECK_NAME_SUFFIX = "-outstanding-tasks"; + public static final String PROJECTS_FILTER_FIELD = "projects"; + public static final String PERIOD_OF_TIME_FIELD = "periodOfTime"; + private final Set<String> projects; + private final long periodOfTimeNanos; + private final SourcesCollection sourcesCollection; + private final Ticker ticker; + private Optional<Long> successfulSince = Optional.empty(); + private boolean isCaughtUp; @Inject public PullReplicationTasksHealthCheck( ListeningExecutorService executor, - HealthCheckConfig config, + HealthCheckConfig healthCheckConfig, + MergedConfigResource configResource, @PluginName String name, - MetricMaker metricMaker) { - super(executor, config, name + "-tasks", metricMaker); + MetricMaker metricMaker, + SourcesCollection sourcesCollection, + Ticker ticker) { + super(executor, healthCheckConfig, name + HEALTHCHECK_NAME_SUFFIX, metricMaker); + String healthCheckName = name + HEALTHCHECK_NAME_SUFFIX; + + Config replicationConfig = configResource.getConfig(); + this.projects = + Set.of( + replicationConfig.getStringList( + HealthCheckConfig.HEALTHCHECK, healthCheckName, PROJECTS_FILTER_FIELD)); + this.periodOfTimeNanos = + ConfigUtil.getTimeUnit( + replicationConfig, + HealthCheckConfig.HEALTHCHECK, + healthCheckName, + PERIOD_OF_TIME_FIELD, + DEFAULT_PERIOD_OF_TIME_SECS, + TimeUnit.NANOSECONDS); + this.sourcesCollection = sourcesCollection; + this.ticker = ticker; + } + + public long getPeriodOfTimeNanos() { + return periodOfTimeNanos; + } + + public Set<String> getProjects() { + return ImmutableSet.copyOf(projects); } @Override protected Result doCheck() throws Exception { - return Result.PASSED; + if (isCaughtUp) { + return Result.PASSED; + } + long checkTime = ticker.read(); + List<Source> sources = sourcesCollection.getAll(); + boolean hasNoOutstandingTasks = + sources.stream() + .allMatch( + source -> { + if (projects.isEmpty()) { + return source.pendingTasksCount() == 0 && source.inflightTasksCount() == 0; + } else { + return projects.stream() + .allMatch( + project -> + source.zeroPendingTasksForRepo(Project.nameKey(project)) + && source.zeroInflightTasksForRepo(Project.nameKey(project))); + } + }); + successfulSince = + hasNoOutstandingTasks ? successfulSince.or(() -> Optional.of(checkTime)) : Optional.empty(); + return reportResult(checkTime); + } + + private HealthCheck.Result reportResult(long checkTime) { + Optional<Result> maybePassed = + successfulSince.filter(ss -> checkTime >= ss + periodOfTimeNanos).map(ss -> Result.PASSED); + isCaughtUp = maybePassed.isPresent(); + return maybePassed.orElse(Result.FAILED); } }
diff --git a/src/main/resources/Documentation/about.md b/src/main/resources/Documentation/about.md index 22b52e7..bfc2974 100644 --- a/src/main/resources/Documentation/about.md +++ b/src/main/resources/Documentation/about.md
@@ -1,5 +1,8 @@ This plugin can automatically mirror repositories from other systems. +Overview +-------- + Typically replication should be done over SSH, with a passwordless public/private key pair. On a trusted network it is also possible to use replication over the insecure (but much faster due to no @@ -9,6 +12,13 @@ local path as replication source. This makes e.g. sense if a network share is mounted to which the repositories should be replicated from. +Installation +------------ + +This plugin depends on the replication and delete-project plugins, +therefore requires them to be installed as well into the +`$GERRIT_SITE/plugins` directory. + Access ------
diff --git a/src/main/resources/Documentation/healthcheck.md b/src/main/resources/Documentation/healthcheck.md new file mode 100644 index 0000000..3a42e6a --- /dev/null +++ b/src/main/resources/Documentation/healthcheck.md
@@ -0,0 +1,73 @@ +@PLUGIN@ health checks +============== + +The @PLUGIN@ plugin registers the `pull-replication-outstanding-tasks` +healthcheck. This check will mark a gerrit instance as healthy upon +startup only when the node has caught up with all the outstanding +pull-replication tasks. The goal is to mark the node as healthy when it +is ready to receive write traffic. "Caught up" means: + +- All pending & in-flight replication tasks across all sources (or +across a configurable set of repos) have completed +- There are no queued replication tasks pending and the above condition +lasts for at least N seconds (configurable) + +See [Healthcheck based on replication tasks](https://issues.gerritcodereview.com/issues/312895374) for more details. + +**It is worth noting that once the healthcheck eventually succeeds and +the instance is marked healthy, the check is then skipped (ie any +subsequent invocations will always mark the instance as healthy +irrespective of any pending or inflight tasks being present).** + +Health check configuration +-------------------------- + +The configuration of the health check is split across two files. +- The "standard" properties commonly available to all other checks +of the `healthcheck` plugin. These are set in the `healthcheck` plugin's +[config file](https://gerrit.googlesource.com/plugins/healthcheck/+/refs/heads/master/src/main/resources/Documentation/config.md#settings). +- Settings specific to the check are set in the plugin's [config file](./config.md#file-pluginconfig). + + +The health check can be configured as follows: +- `healthcheck.@PLUGIN@-outstanding-tasks.projects`: The repo(s) that +the health check will track outstanding replication tasks against. +Multiple entries are supported. If not specified, all the outstanding +replication tasks are tracked. +- `healthcheck.@PLUGIN@-outstanding-tasks.periodOfTime`: The time for +which the check needs to be successful, in order for the instance to be +marked healthy. If the time unit is omitted it defaults to milliseconds. +Values should use common unit suffixes to express their setting: + +* ms, milliseconds +* s, sec, second, seconds +* m, min, minute, minutes +* h, hr, hour, hours + +Default is 10s. + +This example config will report the node healthy when there are no +pending tasks for the `foo` and `bar/baz` repos continuously for a +period of 5 seconds after the plugin startup. +``` +[healthcheck "pull-replication-tasks"] + projects = foo + projects = bar/baz + periodOfTime = 5 sec +``` + +Useful information +------------------ + +- The health check is registered only when the [healthcheck](https://gerrit.googlesource.com/plugins/healthcheck) plugin +is installed. If the `healthcheck` plugin is not installed, then the +check registration is skipped during load of the pull-replication +plugin. + +> **Note** when the @@PLUGIN@@ plugin is installed as a lib (see [extension-point.md](extension-point.md)), +> then the healthcheck plugin jar should also be present in the `lib` directory, +> for the check registration to work. + +- Because the pull-replication healthcheck depends on the `healthcheck` plugin, renaming/removing the `healthcheck` +jar file is not supported during runtime. Doing so can lead to unpredictable behaviour of your gerrit instance. +
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/PullReplicationHealthCheckIT.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/PullReplicationHealthCheckIT.java new file mode 100644 index 0000000..346b7fa --- /dev/null +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/PullReplicationHealthCheckIT.java
@@ -0,0 +1,242 @@ +// Copyright (C) 2024 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.replication.pull; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assertWithMessage; +import static com.googlesource.gerrit.plugins.replication.pull.health.PullReplicationTasksHealthCheck.HEALTHCHECK_NAME_SUFFIX; +import static com.googlesource.gerrit.plugins.replication.pull.health.PullReplicationTasksHealthCheck.PERIOD_OF_TIME_FIELD; + +import com.google.common.base.Stopwatch; +import com.google.gerrit.acceptance.SkipProjectClone; +import com.google.gerrit.acceptance.TestPlugin; +import com.google.gerrit.acceptance.UseLocalDisk; +import com.google.gerrit.acceptance.config.GerritConfig; +import com.google.gerrit.extensions.registration.DynamicSet; +import com.google.gerrit.server.config.SitePaths; +import com.google.gerrit.server.events.Event; +import com.google.gerrit.server.events.EventListener; +import com.google.gerrit.server.events.ProjectEvent; +import com.google.inject.AbstractModule; +import com.google.inject.Inject; +import com.google.inject.Singleton; +import com.googlesource.gerrit.plugins.healthcheck.HealthCheckConfig; +import com.googlesource.gerrit.plugins.healthcheck.HealthCheckExtensionApiModule; +import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck; +import com.googlesource.gerrit.plugins.replication.ApiModule; +import com.googlesource.gerrit.plugins.replication.ReplicationConfigModule; +import com.googlesource.gerrit.plugins.replication.pull.health.PullReplicationTasksHealthCheck; +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; +import java.util.concurrent.TimeUnit; +import java.util.stream.Stream; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Ref; +import org.eclipse.jgit.storage.file.FileBasedConfig; +import org.eclipse.jgit.util.FS; +import org.junit.Test; + +@SkipProjectClone +@UseLocalDisk +@TestPlugin( + name = "pull-replication", + sysModule = + "com.googlesource.gerrit.plugins.replication.pull.PullReplicationHealthCheckIT$PullReplicationHealthCheckTestModule", + httpModule = "com.googlesource.gerrit.plugins.replication.pull.api.HttpModule") +public class PullReplicationHealthCheckIT extends PullReplicationSetupBase { + private static final int TEST_HEALTHCHECK_PERIOD_OF_TIME_SEC = 5; + private static final String TEST_PLUGIN_NAME = "pull-replication"; + + public static class PullReplicationHealthCheckTestModule extends AbstractModule { + private final PullReplicationModule pullReplicationModule; + + @Inject + PullReplicationHealthCheckTestModule( + ReplicationConfigModule configModule, InMemoryMetricMaker memMetric) { + this.pullReplicationModule = new PullReplicationModule(configModule, memMetric); + } + + @Override + protected void configure() { + install(new ApiModule()); + install(new HealthCheckExtensionApiModule()); + install(pullReplicationModule); + + DynamicSet.bind(binder(), EventListener.class) + .to(PullReplicationHealthCheckIT.BufferedEventListener.class) + .asEagerSingleton(); + } + } + + @Inject private SitePaths sitePaths; + private PullReplicationHealthCheckIT.BufferedEventListener eventListener; + private final int periodOfTimeSecs = 1; + + @Singleton + public static class BufferedEventListener implements EventListener { + + private final List<Event> eventsReceived; + private String eventTypeFilter; + + @Inject + public BufferedEventListener() { + eventsReceived = new ArrayList<>(); + } + + @Override + public void onEvent(Event event) { + if (event.getType().equals(eventTypeFilter)) { + eventsReceived.add(event); + } + } + + public void clearFilter(String expectedEventType) { + eventsReceived.clear(); + eventTypeFilter = expectedEventType; + } + + public int numEventsReceived() { + return eventsReceived.size(); + } + + @SuppressWarnings("unchecked") + public <T extends Event> Stream<T> eventsStream() { + return (Stream<T>) eventsReceived.stream(); + } + } + + @Override + public void setUpTestPlugin() throws Exception { + + FileBasedConfig config = + new FileBasedConfig(sitePaths.etc_dir.resolve("replication.config").toFile(), FS.DETECTED); + config.setString("replication", null, "syncRefs", "ALL REFS ASYNC"); + config.setString( + HealthCheckConfig.HEALTHCHECK, + "pull-replication" + HEALTHCHECK_NAME_SUFFIX, + PERIOD_OF_TIME_FIELD, + periodOfTimeSecs + " sec"); + config.save(); + + super.setUpTestPlugin(true); + eventListener = getInstance(PullReplicationHealthCheckIT.BufferedEventListener.class); + } + + @Override + protected boolean useBatchRefUpdateEvent() { + return false; + } + + @Override + protected void setReplicationSource( + String remoteName, List<String> replicaSuffixes, Optional<String> project) + throws IOException { + List<String> fetchUrls = + buildReplicaURLs(replicaSuffixes, s -> gitPath.resolve("${name}" + s + ".git").toString()); + config.setStringList("remote", remoteName, "url", fetchUrls); + config.setString("remote", remoteName, "apiUrl", adminRestSession.url()); + config.setString("remote", remoteName, "fetch", "+refs/*:refs/*"); + config.setInt("remote", remoteName, "timeout", 600); + config.setInt("remote", remoteName, "replicationDelay", TEST_REPLICATION_DELAY); + project.ifPresent(prj -> config.setString("remote", remoteName, "projects", prj)); + config.setBoolean("gerrit", null, "autoReload", true); + config.setInt("replication", null, "maxApiPayloadSize", 1); + config.setString( + HealthCheckConfig.HEALTHCHECK, + TEST_PLUGIN_NAME + HEALTHCHECK_NAME_SUFFIX, + PERIOD_OF_TIME_FIELD, + TEST_HEALTHCHECK_PERIOD_OF_TIME_SEC + " sec"); + config.save(); + } + + @Test + @GerritConfig(name = "gerrit.instanceId", value = TEST_REPLICATION_REMOTE) + public void shouldReportInstanceHealthyWhenThereAreNoOutstandingReplicationTasks() + throws Exception { + Stopwatch testStartStopwatch = Stopwatch.createUnstarted(); + testRepo = cloneProject(createTestProject(project + TEST_REPLICATION_SUFFIX)); + PullReplicationTasksHealthCheck healthcheck = + getInstance(PullReplicationTasksHealthCheck.class); + + Ref newRef = createNewRef(); + + ReplicationQueue pullReplicationQueue = getInstance(ReplicationQueue.class); + ProjectEvent event = + generateUpdateEvent( + project, + newRef.getName(), + ObjectId.zeroId().getName(), + newRef.getObjectId().getName(), + TEST_REPLICATION_REMOTE); + + eventListener.clearFilter(FetchRefReplicatedEvent.TYPE); + pullReplicationQueue.onEvent(event); + // If replication hasn't started yet, the healthcheck returns PASSED + // but hasReplicationFinished() would be false, ending up in producing + // flaky failures. + waitUntil(() -> hasReplicationBeenScheduledOrStarted()); + + waitUntil( + () -> { + boolean healthCheckPassed = healthcheck.run().result == HealthCheck.Result.PASSED; + boolean replicationFinished = hasReplicationFinished(); + if (!replicationFinished) { + assertWithMessage("Instance reported healthy while waiting for replication to finish") + // Racy condition: we need to make sure that this isn't a false alarm + // and accept the case when the replication finished between the + // if(!replicationFinished) check and the assertion here + .that(!healthCheckPassed || hasReplicationFinished()) + .isTrue(); + } else { + if (!testStartStopwatch.isRunning()) { + testStartStopwatch.start(); + } + } + return replicationFinished; + }); + + if (testStartStopwatch.elapsed(TimeUnit.SECONDS) < TEST_HEALTHCHECK_PERIOD_OF_TIME_SEC) { + assertThat(healthcheck.run().result).isEqualTo(HealthCheck.Result.FAILED); + } + + waitUntil( + () -> testStartStopwatch.elapsed(TimeUnit.SECONDS) > TEST_HEALTHCHECK_PERIOD_OF_TIME_SEC); + assertThat(healthcheck.run().result).isEqualTo(HealthCheck.Result.PASSED); + } + + private boolean hasReplicationFinished() { + return inMemoryMetrics() + .counterValue("tasks/completed", TEST_REPLICATION_REMOTE) + .filter(counter -> counter > 0) + .isPresent(); + } + + private boolean hasReplicationBeenScheduledOrStarted() { + return inMemoryMetrics() + .counterValue("tasks/scheduled", TEST_REPLICATION_REMOTE) + .filter(counter -> counter > 0) + .isPresent() + || inMemoryMetrics() + .counterValue("tasks/started", TEST_REPLICATION_REMOTE) + .filter(counter -> counter > 0) + .isPresent(); + } + + private InMemoryMetricMaker inMemoryMetrics() { + return getInstance(InMemoryMetricMaker.class); + } +}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/PullReplicationITAbstract.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/PullReplicationITAbstract.java index c791770..5e44cb9 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/PullReplicationITAbstract.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/PullReplicationITAbstract.java
@@ -39,6 +39,7 @@ import com.google.gerrit.server.events.ProjectEvent; import com.google.inject.Inject; import com.google.inject.Singleton; +import com.googlesource.gerrit.plugins.healthcheck.HealthCheckExtensionApiModule; import com.googlesource.gerrit.plugins.replication.ApiModule; import com.googlesource.gerrit.plugins.replication.AutoReloadConfigDecorator; import com.googlesource.gerrit.plugins.replication.ReplicationConfigModule; @@ -80,6 +81,7 @@ protected void configure() { super.configure(); install(new ApiModule()); + install(new HealthCheckExtensionApiModule()); DynamicSet.bind(binder(), EventListener.class) .to(BufferedEventListener.class)
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/PullReplicationSetupBase.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/PullReplicationSetupBase.java index 9cb2dc9..ed71545 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/PullReplicationSetupBase.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/PullReplicationSetupBase.java
@@ -14,6 +14,8 @@ package com.googlesource.gerrit.plugins.replication.pull; +import static com.google.gerrit.acceptance.GitUtil.assertPushOk; +import static com.google.gerrit.acceptance.GitUtil.pushOne; import static java.util.stream.Collectors.toList; import com.google.common.base.Suppliers; @@ -38,10 +40,12 @@ import java.util.Arrays; import java.util.List; import java.util.Optional; +import java.util.UUID; import java.util.function.Function; import java.util.function.Supplier; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.storage.file.FileBasedConfig; import org.eclipse.jgit.util.FS; @@ -179,4 +183,12 @@ event.instanceId = instanceId; return event; } + + protected Ref createNewRef() throws Exception { + String newRef = "refs/heads/" + UUID.randomUUID(); + RevCommit newCommit = testRepo.branch("HEAD").commit().create(); + testRepo.branch(newRef).update(newCommit); + assertPushOk(pushOne(testRepo, newRef, newRef, false, false, List.of()), newRef); + return testRepo.getRepository().exactRef(newRef); + } }
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/TestPullReplicationModule.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/TestPullReplicationModule.java index a20f617..7bbd1e2 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/TestPullReplicationModule.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/TestPullReplicationModule.java
@@ -2,6 +2,7 @@ import com.google.inject.AbstractModule; import com.google.inject.Inject; +import com.googlesource.gerrit.plugins.healthcheck.HealthCheckExtensionApiModule; import com.googlesource.gerrit.plugins.replication.ApiModule; public class TestPullReplicationModule extends AbstractModule { @@ -16,6 +17,7 @@ @Override protected void configure() { install(new ApiModule()); + install(new HealthCheckExtensionApiModule()); install(pullReplicationModule); } }
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/health/PullReplicationTasksHealthCheckTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/health/PullReplicationTasksHealthCheckTest.java new file mode 100644 index 0000000..06fa996 --- /dev/null +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/health/PullReplicationTasksHealthCheckTest.java
@@ -0,0 +1,275 @@ +// Copyright (C) 2024 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.replication.pull.health; + +import static com.google.common.truth.Truth.assertThat; +import static com.googlesource.gerrit.plugins.replication.pull.health.PullReplicationTasksHealthCheck.HEALTHCHECK_NAME_SUFFIX; +import static com.googlesource.gerrit.plugins.replication.pull.health.PullReplicationTasksHealthCheck.PERIOD_OF_TIME_FIELD; +import static com.googlesource.gerrit.plugins.replication.pull.health.PullReplicationTasksHealthCheck.PROJECTS_FILTER_FIELD; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.when; + +import com.google.common.base.Ticker; +import com.google.common.testing.FakeTicker; +import com.google.gerrit.common.Nullable; +import com.google.gerrit.entities.Project; +import com.google.gerrit.extensions.annotations.PluginName; +import com.google.gerrit.metrics.DisabledMetricMaker; +import com.google.gerrit.metrics.MetricMaker; +import com.google.inject.AbstractModule; +import com.google.inject.Guice; +import com.google.inject.Injector; +import com.googlesource.gerrit.plugins.healthcheck.HealthCheckConfig; +import com.googlesource.gerrit.plugins.healthcheck.HealthCheckExtensionApiModule; +import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck; +import com.googlesource.gerrit.plugins.replication.ConfigResource; +import com.googlesource.gerrit.plugins.replication.MergedConfigResource; +import com.googlesource.gerrit.plugins.replication.pull.Source; +import com.googlesource.gerrit.plugins.replication.pull.SourcesCollection; +import java.time.Duration; +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; +import java.util.concurrent.TimeUnit; +import org.eclipse.jgit.lib.Config; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; + +@RunWith(MockitoJUnitRunner.class) +public class PullReplicationTasksHealthCheckTest { + private static final String PLUGIN_NAME = "pull-replication"; + private static final String SECTION_NAME = PLUGIN_NAME + HEALTHCHECK_NAME_SUFFIX; + private static final String ZERO_PERIOD_OF_TIME = "0 sec"; + private static final Optional<String> NO_PROJECT_FILTER = Optional.empty(); + + private final int periodOfTimeMillis = 10; + private final String periodOfTimeMillisStr = periodOfTimeMillis + " ms"; + private final FakeTicker fakeTicker = new FakeTicker(); + @Mock private SourcesCollection sourcesCollection; + + @Mock private Source source; + @Mock private Source anotherSource; + + @Before + public void setUp() { + when(sourcesCollection.getAll()).thenReturn(List.of(source)); + } + + @Test + public void shouldReadConfig() { + List<String> projectsToCheck = List.of("foo", "bar/baz"); + Injector injector = testInjector(new TestModule(projectsToCheck, periodOfTimeMillisStr)); + + PullReplicationTasksHealthCheck check = + injector.getInstance(PullReplicationTasksHealthCheck.class); + + assertThat(check.getProjects()).containsExactlyElementsIn(projectsToCheck); + assertThat(check.getPeriodOfTimeNanos()) + .isEqualTo(TimeUnit.MILLISECONDS.toNanos(periodOfTimeMillis)); + } + + @Test + public void shouldOnlyCheckTasksForReposThatMatchTheRepoFilter() { + int numIterations = 3; + String repo = "foo"; + when(source.zeroPendingTasksForRepo(Project.nameKey(repo))).thenReturn(false).thenReturn(true); + when(source.zeroInflightTasksForRepo(Project.nameKey(repo))).thenReturn(false).thenReturn(true); + lenient().when(source.zeroInflightTasksForRepo(Project.nameKey("ignored"))).thenReturn(false); + lenient().when(source.pendingTasksCount()).thenReturn(10L); + + PullReplicationTasksHealthCheck check = newPullReplicationTasksHealthCheck(Optional.of(repo)); + + List<HealthCheck.Result> checkResults = runNTimes(numIterations, check); + assertThat(checkResults) + .containsExactly( + HealthCheck.Result.FAILED, HealthCheck.Result.FAILED, HealthCheck.Result.PASSED); + } + + @Test + public void shouldCheckAllOutstandingTasksWhenRepoFilterIsNotConfigured() { + when(source.pendingTasksCount()).thenReturn(1L).thenReturn(0L); + when(source.inflightTasksCount()).thenReturn(1L).thenReturn(0L); + + PullReplicationTasksHealthCheck check = newPullReplicationTasksHealthCheck(NO_PROJECT_FILTER); + + List<HealthCheck.Result> checkResults = runNTimes(3, check); + assertThat(checkResults) + .containsExactly( + HealthCheck.Result.FAILED, HealthCheck.Result.FAILED, HealthCheck.Result.PASSED); + } + + @Test + public void shouldCheckTasksAcrossSources() { + when(sourcesCollection.getAll()).thenReturn(List.of(source, anotherSource)); + when(source.pendingTasksCount()).thenReturn(1L).thenReturn(0L); + when(source.inflightTasksCount()).thenReturn(1L).thenReturn(0L); + when(anotherSource.pendingTasksCount()).thenReturn(1L).thenReturn(0L); + when(anotherSource.inflightTasksCount()).thenReturn(1L).thenReturn(0L); + + PullReplicationTasksHealthCheck check = newPullReplicationTasksHealthCheck(NO_PROJECT_FILTER); + + List<HealthCheck.Result> checkResults = runNTimes(5, check); + assertThat(checkResults) + .containsExactly( + HealthCheck.Result.FAILED, + HealthCheck.Result.FAILED, + HealthCheck.Result.FAILED, + HealthCheck.Result.FAILED, + HealthCheck.Result.PASSED); + } + + @Test + public void shouldFailIfCheckDoesNotReportHealthyConsistentlyOverPeriodOfTime() { + long healthCheckPeriodOfTimeMsec = 50L; + String healthCheckPeriodOfTime = healthCheckPeriodOfTimeMsec + " ms"; + int checkInterations = 3; + Duration checkInterval = + Duration.ofMillis(healthCheckPeriodOfTimeMsec).dividedBy(checkInterations); + long fakeTimerStartNanos = fakeTicker.read(); + when(source.pendingTasksCount()).thenReturn(0L).thenReturn(1L).thenReturn(0L); + when(source.inflightTasksCount()).thenReturn(0L); + + Injector injector = testInjector(new TestModule(List.of(), healthCheckPeriodOfTime)); + PullReplicationTasksHealthCheck check = + injector.getInstance(PullReplicationTasksHealthCheck.class); + + List<HealthCheck.Result> checkResults = + runNTimes(checkInterations, check, () -> fakeTicker.advance(checkInterval)); + + assertThat(checkResults).doesNotContain(HealthCheck.Result.PASSED); + assertThat(fakeTicker.read()) + .isAtLeast(Duration.ofMillis(periodOfTimeMillis).plusNanos(fakeTimerStartNanos).toNanos()); + } + + @Test + public void shouldFailOnFirstInvocationEvenIfThereAreNoOutstandingTasksAndANonZeroPeriodOfTime() { + mockSourceWithNoOutstandingTasks(); + + Injector injector = testInjector(new TestModule(List.of(), periodOfTimeMillisStr)); + PullReplicationTasksHealthCheck check = + injector.getInstance(PullReplicationTasksHealthCheck.class); + + assertThat(check.run().result).isEqualTo(HealthCheck.Result.FAILED); + } + + @Test + public void + shouldReportHealthyOnFirstInvocationIfThereAreNoOutstandingTasksAndAZeroPeriodOfTime() { + mockSourceWithNoOutstandingTasks(); + + Injector injector = testInjector(new TestModule(List.of(), ZERO_PERIOD_OF_TIME)); + PullReplicationTasksHealthCheck check = + injector.getInstance(PullReplicationTasksHealthCheck.class); + + assertThat(check.run().result).isEqualTo(HealthCheck.Result.PASSED); + } + + @Test + public void shouldAlwaysReportHealthyAfterItHasCaughtUpWithOutstandingTasks() { + int numCheckIterations = 2; + when(source.pendingTasksCount()).thenReturn(0L, 0L, 1L); + when(source.inflightTasksCount()).thenReturn(0L); + + Injector injector = testInjector(new TestModule(List.of(), periodOfTimeMillisStr)); + PullReplicationTasksHealthCheck check = + injector.getInstance(PullReplicationTasksHealthCheck.class); + + assertThat( + runNTimes( + numCheckIterations, + check, + () -> fakeTicker.advance(Duration.ofMillis(periodOfTimeMillis)))) + .containsExactly(HealthCheck.Result.FAILED, HealthCheck.Result.PASSED); + + assertThat(check.run().result).isEqualTo(HealthCheck.Result.PASSED); + } + + private Injector testInjector(AbstractModule testModule) { + return Guice.createInjector(new HealthCheckExtensionApiModule(), testModule); + } + + private List<HealthCheck.Result> runNTimes(int nTimes, PullReplicationTasksHealthCheck check) { + return runNTimes(nTimes, check, null); + } + + private PullReplicationTasksHealthCheck newPullReplicationTasksHealthCheck( + Optional<String> projectNameToCheck) { + Injector injector = + testInjector(new TestModule(projectNameToCheck.stream().toList(), ZERO_PERIOD_OF_TIME)); + + PullReplicationTasksHealthCheck check = + injector.getInstance(PullReplicationTasksHealthCheck.class); + return check; + } + + private List<HealthCheck.Result> runNTimes( + int nTimes, PullReplicationTasksHealthCheck check, @Nullable Runnable postRunFunc) { + List<HealthCheck.Result> results = new ArrayList<>(); + for (int i = 0; i < nTimes; i++) { + results.add(check.run().result); + if (postRunFunc != null) { + postRunFunc.run(); + } + } + + return results; + } + + private void mockSourceWithNoOutstandingTasks() { + when(source.pendingTasksCount()).thenReturn(0L); + when(source.inflightTasksCount()).thenReturn(0L); + } + + private class TestModule extends AbstractModule { + Config config; + MergedConfigResource configResource; + private final HealthCheckConfig healthCheckConfig = HealthCheckConfig.DEFAULT_CONFIG; + + public TestModule(List<String> projects, String periodOfTime) { + this.config = new Config(); + config.setStringList( + HealthCheckConfig.HEALTHCHECK, SECTION_NAME, PROJECTS_FILTER_FIELD, projects); + config.setString( + HealthCheckConfig.HEALTHCHECK, SECTION_NAME, PERIOD_OF_TIME_FIELD, periodOfTime); + configResource = + MergedConfigResource.withBaseOnly( + new ConfigResource() { + @Override + public Config getConfig() { + return config; + } + + @Override + public String getVersion() { + return ""; + } + }); + } + + @Override + protected void configure() { + bind(Config.class).toInstance(config); + bind(MergedConfigResource.class).toInstance(configResource); + bind(MetricMaker.class).toInstance(new DisabledMetricMaker()); + bind(HealthCheckConfig.class).toInstance(healthCheckConfig); + bind(String.class).annotatedWith(PluginName.class).toInstance(PLUGIN_NAME); + bind(SourcesCollection.class).toInstance(sourcesCollection); + bind(Ticker.class).toInstance(fakeTicker); + } + } +}