Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,13 @@
import java.util.Optional;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;

import org.apache.http.HttpResponse;
import org.apache.http.client.HttpClient;
import org.apache.http.client.config.RequestConfig;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.impl.client.HttpClientBuilder;
import org.apache.http.util.EntityUtils;
Expand Down Expand Up @@ -61,6 +64,20 @@ public String collectHealthReport(boolean isRemoteBuildEnabled, Map<String, Remo
JSONObject result = new JSONObject();
Map<String, CompletableFuture<Boolean>> reportResults = new HashMap<>(remoteBuilderPlatformMappings.size());
List<HttpGet> runningRequests = new ArrayList<>();
// Probe each builder on its own thread from a dedicated pool: a slow or
// unresponsive builder must not starve the probes of the healthy ones,
// which is what happens on the shared common ForkJoinPool under load.
// Size to the exact builder count so no probe ever waits in the queue --
// completeOnTimeout starts ticking at submission, so a queued probe could
// time out to false before its request ever runs.
ExecutorService healthCheckPool = Executors.newFixedThreadPool(
Math.max(1, remoteBuilderPlatformMappings.size()),
runnable -> {
Thread thread = new Thread(runnable, "health-check");
thread.setDaemon(true);
return thread;
});
try {
for (Map.Entry<String, RemoteInstanceConfig> entry : remoteBuilderPlatformMappings.entrySet()) {
String instanceId = entry.getValue().getInstanceId();
String platform = getPlatform(entry.getKey());
Expand All @@ -80,6 +97,13 @@ public String collectHealthReport(boolean isRemoteBuildEnabled, Map<String, Remo
// if instance is not located in GCP - make http request to it asynchronously
final String healthUrl = String.format("%s/health_report", entry.getValue().getUrl());
final HttpGet request = new HttpGet(healthUrl);
// Bound the blocking execute() so a stuck builder releases its worker
// near the timeout instead of holding it for the whole socket lifetime.
request.setConfig(RequestConfig.custom()
.setConnectTimeout(this.connectionTimeout)
.setSocketTimeout(this.connectionTimeout)
.setConnectionRequestTimeout(this.connectionTimeout)
.build());
runningRequests.add(request);
CompletableFuture<Boolean> innerRequest = CompletableFuture.supplyAsync(() -> {
JSONParser parser = new JSONParser();
Expand All @@ -100,7 +124,7 @@ public String collectHealthReport(boolean isRemoteBuildEnabled, Map<String, Remo
} catch(Exception exc) {
return Boolean.FALSE;
}
}).completeOnTimeout(Boolean.FALSE, this.connectionTimeout, TimeUnit.MILLISECONDS);
}, healthCheckPool).completeOnTimeout(Boolean.FALSE, this.connectionTimeout, TimeUnit.MILLISECONDS);
reportResults.put(entry.getKey(), innerRequest);
}
for (Map.Entry<String, CompletableFuture<Boolean>> status : reportResults.entrySet()) {
Expand Down Expand Up @@ -128,6 +152,9 @@ public String collectHealthReport(boolean isRemoteBuildEnabled, Map<String, Remo
result.put(entry.getKey(), entry.getValue().toString());
}
return result.toJSONString();
} finally {
healthCheckPool.shutdownNow();
}
} else {
return JSONObject.toJSONString(Collections.singletonMap("status", OperationalStatus.Operational.toString()));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,4 +175,23 @@ public void testRemoteNodesNotFullyOperational() {
String result = service.collectHealthReport(true, conf);
assertEquals(JSONObject.toJSONString(expected), result);
}

@Test
public void testHealthyNodeNotStarvedBySlowPeer() throws ParseException {
// Regression: a slow builder (9681, fixed 15s delay) must not starve the
// probe of a healthy builder (9678). Each probe runs on its own thread, so
// the fast node reports Operational within the same pass in which the slow
// node times out to Unreachable. On the previous shared-pool implementation
// the healthy probe could be starved and mis-reported as Unreachable.
Map<String, RemoteInstanceConfig> conf = Map.of(
"linux-latest", new RemoteInstanceConfig("http://localhost:9678", "linux-latest", true),
"windows-latest", new RemoteInstanceConfig("http://localhost:9681", "windows-latest", true)
);
JSONObject expected = new JSONObject(Map.of(
"linux", "Operational",
"windows", "Unreachable"
));
JSONObject result = (JSONObject) new JSONParser().parse(service.collectHealthReport(true, conf));
assertEquals(expected, result);
}
}