Add active worker threads check Add check to monitor number of active worker threads to prevent situation when instance is out of worker threads but healthcheck is successful. Feature: Issue 11651 Change-Id: Id49057cf4e3be473a8a8262ffa6c230c1e1e2efe
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckConfig.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckConfig.java index 4e4fab1..a19c934 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckConfig.java +++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckConfig.java
@@ -40,6 +40,7 @@ private static final long HEALTHCHECK_TIMEOUT_DEFAULT = 500L; private static final String QUERY_DEFAULT = "status:open"; private static final int LIMIT_DEFAULT = 10; + private static final int ACTIVE_WORKERS_THRESHOLD_DEFAULT = 80; private static final String USERNAME_DEFAULT = "healthcheck"; private static final String PASSWORD_DEFAULT = ""; private static final boolean HEALTH_CHECK_ENABLED_DEFAULT = true; @@ -97,6 +98,14 @@ return config.getInt(HEALTHCHECK, healthCheckName, "limit", defaultLimit); } + public int getActiveWorkersThreshold(String healthCheckName) { + int defaultThreshold = + healthCheckName == null + ? ACTIVE_WORKERS_THRESHOLD_DEFAULT + : getActiveWorkersThreshold(null); + return config.getInt(HEALTHCHECK, healthCheckName, "threshold", defaultThreshold); + } + public Set<Project.NameKey> getJGITRepositories(String healthCheckName) { Set<Project.NameKey> repos = Stream.of(config.getStringList(HEALTHCHECK, healthCheckName, "project"))
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckSubsystemsModule.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckSubsystemsModule.java index 6426fa1..078010a 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckSubsystemsModule.java +++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckSubsystemsModule.java
@@ -17,6 +17,7 @@ import com.google.gerrit.extensions.events.LifecycleListener; import com.google.gerrit.extensions.registration.DynamicSet; import com.google.inject.AbstractModule; +import com.googlesource.gerrit.plugins.healthcheck.check.ActiveWorkersCheck; import com.googlesource.gerrit.plugins.healthcheck.check.AuthHealthCheck; import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck; import com.googlesource.gerrit.plugins.healthcheck.check.JGitHealthCheck; @@ -33,6 +34,7 @@ bindChecker(ProjectsListHealthCheck.class); bindChecker(QueryChangesHealthCheck.class); bindChecker(AuthHealthCheck.class); + bindChecker(ActiveWorkersCheck.class); bind(LifecycleListener.class).to(HealthCheckMetrics.class); }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/ActiveWorkersCheck.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/ActiveWorkersCheck.java new file mode 100644 index 0000000..f3afc99 --- /dev/null +++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/ActiveWorkersCheck.java
@@ -0,0 +1,83 @@ +// Copyright (C) 2019 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.check; + +import static com.googlesource.gerrit.plugins.healthcheck.check.HealthCheckNames.ACTIVEWORKERS; + +import com.codahale.metrics.MetricRegistry; +import com.google.common.util.concurrent.ListeningExecutorService; +import com.google.gerrit.server.config.GerritServerConfig; +import com.google.gerrit.server.config.ThreadSettingsConfig; +import com.google.inject.Inject; +import com.googlesource.gerrit.plugins.healthcheck.HealthCheckConfig; +import java.util.Optional; +import org.eclipse.jgit.lib.Config; + +public class ActiveWorkersCheck extends AbstractHealthCheck { + + public static final String ACTIVE_WORKERS_METRIC_NAME = + "queue/ssh_interactive_worker/active_threads"; + + private Integer threshold; + private Integer interactiveThreadsMaxPoolSize; + private MetricRegistry metricRegistry; + + @Inject + public ActiveWorkersCheck( + @GerritServerConfig Config gerritConfig, + ListeningExecutorService executor, + HealthCheckConfig healthCheckConfig, + ThreadSettingsConfig threadSettingsConfig, + MetricRegistry metricRegistry) { + super(executor, healthCheckConfig, ACTIVEWORKERS); + this.threshold = healthCheckConfig.getActiveWorkersThreshold(ACTIVEWORKERS); + this.metricRegistry = metricRegistry; + this.interactiveThreadsMaxPoolSize = + getInteractiveThreadsMaxPoolSize(threadSettingsConfig, gerritConfig); + } + + @Override + protected Result doCheck() throws Exception { + return Optional.ofNullable(metricRegistry.getGauges().get(ACTIVE_WORKERS_METRIC_NAME)) + .map( + metric -> { + float currentInteractiveThreadsPercentage = + ((long) metric.getValue() * 100) / interactiveThreadsMaxPoolSize; + return (currentInteractiveThreadsPercentage <= threshold) + ? Result.PASSED + : Result.FAILED; + }) + .orElse(Result.PASSED); + } + + /** + * This method is following logic from com.google.gerrit.sshd.CommandExecutorQueueProvider + * + * <p>We are not able to use "queue/ssh_interactive_worker/max_pool_size" metric because in + * current implementation it's always returning Integer.MAX_VALUE. + * + * @return max number of allowed threads in interactive work queue + */ + private Integer getInteractiveThreadsMaxPoolSize( + ThreadSettingsConfig threadSettingsConfig, Config gerritConfig) { + int poolSize = threadSettingsConfig.getSshdThreads(); + int batchThreads = + gerritConfig.getInt("sshd", "batchThreads", threadSettingsConfig.getSshdBatchTreads()); + if (batchThreads > poolSize) { + poolSize += batchThreads; + } + return Math.max(1, poolSize - batchThreads); + } +}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/HealthCheckNames.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/HealthCheckNames.java index b160fc4..9614484 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/HealthCheckNames.java +++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/HealthCheckNames.java
@@ -20,4 +20,5 @@ String PROJECTSLIST = "projectslist"; String QUERYCHANGES = "querychanges"; String AUTH = "auth"; + String ACTIVEWORKERS = "activeworkers"; }
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md index 2b7a224..739a43a 100644 --- a/src/main/resources/Documentation/config.md +++ b/src/main/resources/Documentation/config.md
@@ -48,6 +48,7 @@ - `jgit` : check connectivity to the filesystem and ability to open a JGit ref and object - `projectslist` : check the ability to list projects with their descriptions - `auth`: check the ability to authenticate with username and password +- `activeworkers`: check the number of active worker threads and the ability to create a new one Each check name can be disabled by setting the `enabled` parameter to **false**, by default this parameter is set to **true** @@ -81,4 +82,9 @@ - `healthcheck.auth.password` : Password to use for authentication - Default: no password \ No newline at end of file + Default: no password + + - `healthcheck.activeworkers.threshold` : Percent of queue occupancy above which queue is consider + as full. + + Default: 80 \ No newline at end of file
diff --git a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/ActiveWorkersCheckTest.java b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/ActiveWorkersCheckTest.java new file mode 100644 index 0000000..71bef43 --- /dev/null +++ b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/ActiveWorkersCheckTest.java
@@ -0,0 +1,161 @@ +// Copyright (C) 2019 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.HealthCheckConfig.DEFAULT_CONFIG; +import static com.googlesource.gerrit.plugins.healthcheck.check.HealthCheckNames.ACTIVEWORKERS; + +import com.codahale.metrics.Gauge; +import com.codahale.metrics.MetricRegistry; +import com.google.common.util.concurrent.ListeningExecutorService; +import com.google.gerrit.server.config.GerritServerConfig; +import com.google.gerrit.server.config.ThreadSettingsConfig; +import com.google.inject.AbstractModule; +import com.google.inject.Guice; +import com.google.inject.Injector; +import com.googlesource.gerrit.plugins.healthcheck.check.ActiveWorkersCheck; +import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck.Result; +import org.eclipse.jgit.lib.Config; +import org.junit.Test; + +public class ActiveWorkersCheckTest { + + @Test + public void shouldPassCheckWhenNoMetric() { + + Injector injector = testInjector(new TestModule(new Config(), new MetricRegistry())); + + ActiveWorkersCheck check = createCheck(injector); + assertThat(check.run().result).isEqualTo(Result.PASSED); + } + + @Test + public void shouldPassCheckWhenNoActiveWorkers() { + + MetricRegistry metricRegistry = createMetricRegistry(0L); + + Injector injector = testInjector(new TestModule(new Config(), metricRegistry)); + + ActiveWorkersCheck check = createCheck(injector); + assertThat(check.run().result).isEqualTo(Result.PASSED); + } + + @Test + public void shouldPassCheckWhenActiveWorkersLessThanDefaultThreshold() { + + MetricRegistry metricRegistry = createMetricRegistry(79L); + // By default threshold is 80% + Config gerritConfig = new Config(); + gerritConfig.setInt("sshd", null, "threads", 100); + Injector injector = testInjector(new TestModule(gerritConfig, metricRegistry)); + + ActiveWorkersCheck check = createCheck(injector); + assertThat(check.run().result).isEqualTo(Result.PASSED); + } + + @Test + public void shouldFailCheckWhenActiveWorkersMoreThanDefaultThreshold() { + + MetricRegistry metricRegistry = createMetricRegistry(90L); + // By default threshold is 80% + Config gerritConfig = new Config(); + gerritConfig.setInt("sshd", null, "threads", 100); + + Injector injector = testInjector(new TestModule(gerritConfig, metricRegistry)); + + ActiveWorkersCheck check = createCheck(injector); + assertThat(check.run().result).isEqualTo(Result.FAILED); + } + + @Test + public void shouldFailCheckWhenActiveWorkersMoreThanThreshold() { + + MetricRegistry metricRegistry = createMetricRegistry(6L); + + Config gerritConfig = new Config(); + gerritConfig.setInt("sshd", null, "threads", 12); + + Injector injector = testInjector(new TestModule(gerritConfig, metricRegistry)); + + HealthCheckConfig healthCheckConfig = + new HealthCheckConfig("[healthcheck \"" + ACTIVEWORKERS + "\"]\n" + " threshold = 50"); + ActiveWorkersCheck check = createCheck(injector, healthCheckConfig); + assertThat(check.run().result).isEqualTo(Result.FAILED); + } + + @Test + public void shouldPassCheckWhenActiveWorkersLessThanThreshold() { + + MetricRegistry metricRegistry = createMetricRegistry(5L); + + Config gerritConfig = new Config(); + gerritConfig.setInt("sshd", null, "threads", 12); + + Injector injector = testInjector(new TestModule(gerritConfig, metricRegistry)); + + HealthCheckConfig healthCheckConfig = + new HealthCheckConfig("[healthcheck \"" + ACTIVEWORKERS + "\"]\n" + " threshold = 50"); + ActiveWorkersCheck check = createCheck(injector, healthCheckConfig); + assertThat(check.run().result).isEqualTo(Result.PASSED); + } + + private Injector testInjector(AbstractModule testModule) { + return Guice.createInjector(new HealthCheckModule(), testModule); + } + + private MetricRegistry createMetricRegistry(Long value) { + MetricRegistry metricRegistry = new MetricRegistry(); + metricRegistry.register( + ActiveWorkersCheck.ACTIVE_WORKERS_METRIC_NAME, + new Gauge<Long>() { + @Override + public Long getValue() { + return value; + } + }); + return metricRegistry; + } + + private ActiveWorkersCheck createCheck(Injector injector) { + return createCheck(injector, DEFAULT_CONFIG); + } + + private ActiveWorkersCheck createCheck(Injector injector, HealthCheckConfig healtchCheckConfig) { + return new ActiveWorkersCheck( + new Config(), + injector.getInstance(ListeningExecutorService.class), + healtchCheckConfig, + injector.getInstance(ThreadSettingsConfig.class), + injector.getInstance(MetricRegistry.class)); + } + + private class TestModule extends AbstractModule { + Config gerritConfig; + MetricRegistry metricRegistry; + + public TestModule(Config gerritConfig, MetricRegistry metricRegistry) { + this.gerritConfig = gerritConfig; + this.metricRegistry = metricRegistry; + } + + @Override + protected void configure() { + bind(Config.class).annotatedWith(GerritServerConfig.class).toInstance(gerritConfig); + bind(ThreadSettingsConfig.class); + bind(MetricRegistry.class).toInstance(metricRegistry); + } + } +}
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 d23244b..87061f5 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckIT.java +++ b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckIT.java
@@ -16,6 +16,7 @@ import static com.google.common.net.HttpHeaders.CONTENT_TYPE; import static com.google.common.truth.Truth.assertThat; +import static com.googlesource.gerrit.plugins.healthcheck.check.HealthCheckNames.ACTIVEWORKERS; import static com.googlesource.gerrit.plugins.healthcheck.check.HealthCheckNames.AUTH; import static com.googlesource.gerrit.plugins.healthcheck.check.HealthCheckNames.JGIT; import static com.googlesource.gerrit.plugins.healthcheck.check.HealthCheckNames.QUERYCHANGES; @@ -141,6 +142,34 @@ assertCheckResult(getResponseJson(resp), QUERYCHANGES, "passed"); } + @Test + public void shouldReturnActiveWorkersCheck() throws Exception { + createChange("refs/for/master"); + RestResponse resp = getHealthCheckStatus(); + resp.assertOK(); + + assertCheckResult(getResponseJson(resp), ACTIVEWORKERS, "passed"); + } + + @Test + public void shouldReturnActiveWorkersCheckAsDisabled() throws Exception { + disableCheck(ACTIVEWORKERS); + RestResponse resp = getHealthCheckStatus(); + + resp.assertOK(); + assertCheckResult(getResponseJson(resp), ACTIVEWORKERS, "disabled"); + } + + @Test + public void shouldReturnActiveWorkersMultipleTimesCheck() throws Exception { + createChange("refs/for/master"); + getHealthCheckStatus(); + RestResponse resp = getHealthCheckStatus(); + resp.assertOK(); + + assertCheckResult(getResponseJson(resp), ACTIVEWORKERS, "passed"); + } + private RestResponse getHealthCheckStatus() throws IOException { return adminRestSession.get("/config/server/healthcheck~status"); }