Merge branch 'stable-3.1' into stable-3.2 * stable-3.1: Make GlobalHealthCheck reentrant Change-Id: I165df05443af762402efdb668a613c971963c790
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckMetrics.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckMetrics.java index fb298fd..cea2530 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckMetrics.java +++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckMetrics.java
@@ -15,6 +15,7 @@ package com.googlesource.gerrit.plugins.healthcheck; import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.Sets; import com.google.gerrit.extensions.events.LifecycleListener; import com.google.gerrit.extensions.registration.DynamicSet; import com.google.gerrit.extensions.registration.RegistrationHandle; @@ -54,7 +55,10 @@ @Override public void start() { - for (HealthCheck healthCheck : healthChecks) { + Set<HealthCheck> allChecks = Sets.newHashSet(healthChecks); + allChecks.add(globalHealthCheck); + + for (HealthCheck healthCheck : allChecks) { String name = healthCheck.name(); Counter0 failureMetric = getFailureMetric(name); @@ -65,21 +69,6 @@ registeredMetrics.add(metricMaker.newTrigger(latencyMetric, metricCallBack)); triggers.add(metricCallBack); } - - Counter0 globalFailureMetric = getFailureMetric("global"); - CallbackMetric0<Long> globalLatencyMetric = getLatencyMetric("global"); - Runnable globalMetricCallBack = - () -> { - HealthCheck.StatusSummary status = globalHealthCheck.getGlobalStatusSummary(); - globalLatencyMetric.set(status.elapsed); - if (status.isFailure()) { - globalFailureMetric.increment(); - } - }; - - registeredMetrics.add(globalFailureMetric); - registeredMetrics.add(metricMaker.newTrigger(globalLatencyMetric, globalMetricCallBack)); - triggers.add(globalMetricCallBack); } private Runnable getCallbackMetric(
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/api/HealthCheckStatusEndpoint.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/api/HealthCheckStatusEndpoint.java index 9e0a607..2d5f7de 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/api/HealthCheckStatusEndpoint.java +++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/api/HealthCheckStatusEndpoint.java
@@ -20,6 +20,7 @@ import com.google.inject.Singleton; import com.googlesource.gerrit.plugins.healthcheck.check.GlobalHealthCheck; import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck; +import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck.Result; import java.util.Map; import javax.servlet.http.HttpServletResponse; @@ -36,15 +37,16 @@ @Override public Response<Map<String, Object>> apply(ConfigResource resource) throws AuthException, BadRequestException, ResourceConflictException, Exception { - Map<String, Object> result = healthChecks.run(); + HealthCheck.StatusSummary globalHealthCheckStatus = healthChecks.run(); - result.put("ts", healthChecks.getGlobalStatusSummary().ts); - result.put("elapsed", healthChecks.getGlobalStatusSummary().elapsed); - return Response.withStatusCode(getHTTPResultCode(result), result); + Map<String, Object> result = globalHealthCheckStatus.subChecks; + result.put("ts", globalHealthCheckStatus.ts); + result.put("elapsed", globalHealthCheckStatus.elapsed); + return Response.withStatusCode(getHTTPResultCode(globalHealthCheckStatus), result); } - private int getHTTPResultCode(Map<String, Object> result) { - return healthChecks.getResultStatus(result) == HealthCheck.Result.FAILED + private int getHTTPResultCode(HealthCheck.StatusSummary checkStatus) { + return checkStatus.result == Result.FAILED ? HttpServletResponse.SC_INTERNAL_SERVER_ERROR : HttpServletResponse.SC_OK; }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/AbstractHealthCheck.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/AbstractHealthCheck.java index a6eae79..d0181ab 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/AbstractHealthCheck.java +++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/AbstractHealthCheck.java
@@ -17,6 +17,7 @@ import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.ListeningExecutorService; import com.googlesource.gerrit.plugins.healthcheck.HealthCheckConfig; +import java.util.Collections; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; @@ -28,7 +29,7 @@ private final long timeout; private final String name; private final ListeningExecutorService executor; - protected StatusSummary latestStatus; + protected volatile StatusSummary latestStatus; protected HealthCheckConfig config; protected AbstractHealthCheck( @@ -47,6 +48,7 @@ @Override public StatusSummary run() { + StatusSummary checkStatusSummary; boolean enabled = config.healthCheckEnabled(name); final long ts = System.currentTimeMillis(); ListenableFuture<StatusSummary> resultFuture = @@ -59,18 +61,24 @@ log.warn("Check {} failed", name, e); healthy = Result.FAILED; } - return new StatusSummary(healthy, ts, System.currentTimeMillis() - ts); + return new StatusSummary( + healthy, ts, System.currentTimeMillis() - ts, Collections.emptyMap()); }); try { - latestStatus = resultFuture.get(timeout, TimeUnit.MILLISECONDS); + checkStatusSummary = resultFuture.get(timeout, TimeUnit.MILLISECONDS); } catch (TimeoutException e) { log.warn("Check {} timed out", name, e); - latestStatus = new StatusSummary(Result.TIMEOUT, ts, System.currentTimeMillis() - ts); + checkStatusSummary = + new StatusSummary( + Result.TIMEOUT, ts, System.currentTimeMillis() - ts, Collections.emptyMap()); } catch (InterruptedException | ExecutionException e) { log.warn("Check {} failed while waiting for its future result", name, e); - latestStatus = new StatusSummary(Result.FAILED, ts, System.currentTimeMillis() - ts); + checkStatusSummary = + new StatusSummary( + Result.FAILED, ts, System.currentTimeMillis() - ts, Collections.emptyMap()); } - return latestStatus; + latestStatus = checkStatusSummary.shallowCopy(); + return checkStatusSummary; } protected abstract Result doCheck() throws Exception;
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/GlobalHealthCheck.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/GlobalHealthCheck.java index c9066c5..c464a9e 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/GlobalHealthCheck.java +++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/GlobalHealthCheck.java
@@ -23,18 +23,18 @@ import java.util.stream.StreamSupport; @Singleton -public class GlobalHealthCheck { +public class GlobalHealthCheck implements HealthCheck { private final DynamicSet<HealthCheck> healthChecks; - private HealthCheck.StatusSummary globalStatusSummary; + private volatile StatusSummary latestStatus = StatusSummary.INITIAL_STATUS; @Inject public GlobalHealthCheck(DynamicSet<HealthCheck> healthChecks) { this.healthChecks = healthChecks; - this.globalStatusSummary = HealthCheck.StatusSummary.INITIAL_STATUS; } - public Map<String, Object> run() { + @Override + public HealthCheck.StatusSummary run() { Iterable<HealthCheck> iterable = () -> healthChecks.iterator(); long ts = System.currentTimeMillis(); Map<String, Object> checkToResults = @@ -42,25 +42,33 @@ .map(check -> Arrays.asList(check.name(), check.run())) .collect(Collectors.toMap(k -> (String) k.get(0), v -> v.get(1))); long elapsed = System.currentTimeMillis() - ts; - globalStatusSummary = - new HealthCheck.StatusSummary(getResultStatus(checkToResults), ts, elapsed); - return checkToResults; + StatusSummary globalStatus = + new HealthCheck.StatusSummary( + hasAnyFailureOnResults(checkToResults) ? Result.FAILED : Result.PASSED, + ts, + elapsed, + checkToResults); + latestStatus = globalStatus.shallowCopy(); + return globalStatus; } - public HealthCheck.StatusSummary getGlobalStatusSummary() { - return this.globalStatusSummary; - } - - public HealthCheck.Result getResultStatus(Map<String, Object> result) { - if (result.values().stream() + public static boolean hasAnyFailureOnResults(Map<String, Object> results) { + return results.values().stream() .filter( res -> res instanceof HealthCheck.StatusSummary && ((HealthCheck.StatusSummary) res).isFailure()) - .findFirst() - .isPresent()) { - return HealthCheck.Result.FAILED; - } - return HealthCheck.Result.PASSED; + .findAny() + .isPresent(); + } + + @Override + public String name() { + return "global"; + } + + @Override + public StatusSummary getLatestStatus() { + return latestStatus; } }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/HealthCheck.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/HealthCheck.java index de571d1..46acdde 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/HealthCheck.java +++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/HealthCheck.java
@@ -16,7 +16,9 @@ import com.google.gson.annotations.SerializedName; import java.util.Arrays; +import java.util.Collections; import java.util.HashSet; +import java.util.Map; import java.util.Set; public interface HealthCheck { @@ -34,22 +36,29 @@ public class StatusSummary { public static final StatusSummary INITIAL_STATUS = - new StatusSummary(Result.PASSED, System.currentTimeMillis(), 0L); + new StatusSummary(Result.PASSED, System.currentTimeMillis(), 0L, Collections.emptyMap()); public final Result result; public final long ts; public final long elapsed; + public final transient Map<String, Object> subChecks; + public static final Set<Result> failingResults = new HashSet<>(Arrays.asList(Result.FAILED, Result.TIMEOUT)); - public StatusSummary(Result result, long ts, long elapsed) { + public StatusSummary(Result result, long ts, long elapsed, Map<String, Object> subChecks) { this.result = result; this.ts = ts; this.elapsed = elapsed; + this.subChecks = subChecks; } public Boolean isFailure() { return failingResults.contains(this.result); } + + public StatusSummary shallowCopy() { + return new StatusSummary(result, ts, elapsed, Collections.emptyMap()); + } } StatusSummary run();
diff --git a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/GlobalHealthCheckTest.java b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/GlobalHealthCheckTest.java deleted file mode 100644 index a1c792e..0000000 --- a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/GlobalHealthCheckTest.java +++ /dev/null
@@ -1,55 +0,0 @@ -// Copyright (C) 2021 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.extensions.registration.DynamicSet; -import com.google.inject.Inject; -import com.googlesource.gerrit.plugins.healthcheck.check.GlobalHealthCheck; -import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck; -import java.util.HashMap; -import org.junit.Test; - -public class GlobalHealthCheckTest { - - @Inject private DynamicSet<HealthCheck> healthChecks; - - GlobalHealthCheck globalHealthCheck = new GlobalHealthCheck(healthChecks); - - @Test - public void shouldFailIfOneCheckFails() { - - HashMap<String, Object> checks = - new HashMap<String, Object>() { - { - put("failingCheck", new HealthCheck.StatusSummary(HealthCheck.Result.FAILED, 0L, 0L)); - put("passingCheck", new HealthCheck.StatusSummary(HealthCheck.Result.PASSED, 0L, 0L)); - } - }; - assertThat(globalHealthCheck.getResultStatus(checks)).isEqualTo(HealthCheck.Result.FAILED); - } - - @Test - public void shouldPassIfAllChecksPass() { - HashMap<String, Object> checks = - new HashMap<String, Object>() { - { - put("passingCheck", new HealthCheck.StatusSummary(HealthCheck.Result.PASSED, 0L, 0L)); - } - }; - assertThat(globalHealthCheck.getResultStatus(checks)).isEqualTo(HealthCheck.Result.PASSED); - } -}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckMetricsTest.java b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckMetricsTest.java index fb64048..1c01950 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckMetricsTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckMetricsTest.java
@@ -33,6 +33,7 @@ import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck; import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck.Result; import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheck.StatusSummary; +import java.util.Collections; import org.junit.Before; import org.junit.Test; @@ -69,7 +70,7 @@ @Test public void shouldSendCounterWhenStatusSummaryFailed() { Long elapsed = 100L; - setWithStatusSummary(new StatusSummary(Result.FAILED, 1L, elapsed)); + setWithStatusSummary(new StatusSummary(Result.FAILED, 1L, elapsed, Collections.emptyMap())); assertThat(testMetrics.getFailures()).isEqualTo(1); assertThat(testMetrics.getLatency()).isEqualTo(elapsed); @@ -78,7 +79,7 @@ @Test public void shouldSendCounterWhenStatusSummaryTimeout() { Long elapsed = 100L; - setWithStatusSummary(new StatusSummary(Result.TIMEOUT, 1L, elapsed)); + setWithStatusSummary(new StatusSummary(Result.TIMEOUT, 1L, elapsed, Collections.emptyMap())); assertThat(testMetrics.getFailures()).isEqualTo(1); assertThat(testMetrics.getLatency()).isEqualTo(elapsed); @@ -87,7 +88,7 @@ @Test public void shouldNOTSendCounterWhenStatusSummarySuccess() { Long elapsed = 100L; - setWithStatusSummary(new StatusSummary(Result.PASSED, 1L, elapsed)); + setWithStatusSummary(new StatusSummary(Result.PASSED, 1L, elapsed, Collections.emptyMap())); assertThat(testMetrics.failures).isEqualTo(0L); assertThat(testMetrics.getLatency()).isEqualTo(elapsed);