From 9dc95af4c7ef10979f21173260f5433006116096 Mon Sep 17 00:00:00 2001 From: larsrc Date: Tue, 18 May 2021 01:22:54 -0700 Subject: [PATCH] Make workers restart on flags that affect their creation/behaviour. Also refactors the related code to being cleaner. RELNOTES: None. PiperOrigin-RevId: 374365649 --- .../google/devtools/build/lib/worker/BUILD | 1 - .../build/lib/worker/SimpleWorkerPool.java | 97 ++++++- .../build/lib/worker/WorkerFactory.java | 42 ++- .../build/lib/worker/WorkerModule.java | 105 +++---- .../devtools/build/lib/worker/WorkerPool.java | 149 +++++++--- .../build/lib/worker/WorkerPoolConfig.java | 79 ----- .../google/devtools/build/lib/worker/BUILD | 4 + .../build/lib/worker/ExampleWorker.java | 1 + ...t.java => SimpleWorkerPoolConfigTest.java} | 16 +- .../build/lib/worker/WorkerFactoryTest.java | 9 +- .../build/lib/worker/WorkerModuleTest.java | 269 ++++++++++++++++++ .../build/lib/worker/WorkerPoolTest.java | 87 +++--- .../lib/worker/WorkerSpawnRunnerTest.java | 28 +- 13 files changed, 623 insertions(+), 264 deletions(-) delete mode 100644 src/main/java/com/google/devtools/build/lib/worker/WorkerPoolConfig.java rename src/test/java/com/google/devtools/build/lib/worker/{WorkerPoolConfigTest.java => SimpleWorkerPoolConfigTest.java} (85%) create mode 100644 src/test/java/com/google/devtools/build/lib/worker/WorkerModuleTest.java diff --git a/src/main/java/com/google/devtools/build/lib/worker/BUILD b/src/main/java/com/google/devtools/build/lib/worker/BUILD index 1080013342f595..1008d0f30ecda2 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/BUILD +++ b/src/main/java/com/google/devtools/build/lib/worker/BUILD @@ -26,7 +26,6 @@ java_library( "//src/main/java/com/google/devtools/build/lib/exec:spawn_runner", "//src/main/java/com/google/devtools/build/lib/exec:spawn_strategy_registry", "//src/main/java/com/google/devtools/build/lib/exec/local", - "//src/main/java/com/google/devtools/build/lib/exec/local:options", "//src/main/java/com/google/devtools/build/lib/runtime/commands/events", "//src/main/java/com/google/devtools/build/lib/sandbox", "//src/main/java/com/google/devtools/build/lib/shell", diff --git a/src/main/java/com/google/devtools/build/lib/worker/SimpleWorkerPool.java b/src/main/java/com/google/devtools/build/lib/worker/SimpleWorkerPool.java index 02249e16256c20..4c67418d1e318a 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/SimpleWorkerPool.java +++ b/src/main/java/com/google/devtools/build/lib/worker/SimpleWorkerPool.java @@ -15,6 +15,7 @@ import com.google.common.base.Throwables; import java.io.IOException; +import java.util.Objects; import javax.annotation.concurrent.ThreadSafe; import org.apache.commons.pool2.impl.GenericKeyedObjectPool; import org.apache.commons.pool2.impl.GenericKeyedObjectPoolConfig; @@ -28,8 +29,38 @@ @ThreadSafe final class SimpleWorkerPool extends GenericKeyedObjectPool { - public SimpleWorkerPool(WorkerFactory factory, GenericKeyedObjectPoolConfig config) { - super(factory, config); + public SimpleWorkerPool(WorkerFactory factory, int max) { + super(factory, makeConfig(max)); + } + + static SimpleWorkerPoolConfig makeConfig(int max) { + SimpleWorkerPoolConfig config = new SimpleWorkerPoolConfig(); + + // It's better to re-use a worker as often as possible and keep it hot, in order to profit + // from JIT optimizations as much as possible. + config.setLifo(true); + + // Keep a fixed number of workers running per key. + config.setMaxIdlePerKey(max); + config.setMaxTotalPerKey(max); + config.setMinIdlePerKey(max); + + // Don't limit the total number of worker processes, as otherwise the pool might be full of + // workers for one WorkerKey and can't accommodate a worker for another WorkerKey. + config.setMaxTotal(-1); + + // Wait for a worker to become ready when a thread needs one. + config.setBlockWhenExhausted(true); + + // Always test the liveliness of worker processes. + config.setTestOnBorrow(true); + config.setTestOnCreate(true); + config.setTestOnReturn(true); + + // No eviction of idle workers. + config.setTimeBetweenEvictionRunsMillis(-1); + + return config; } @Override @@ -51,4 +82,66 @@ public void invalidateObject(WorkerKey key, Worker obj) throws IOException, Inte throw new RuntimeException("unexpected", t); } } + + /** + * Our own configuration class for the {@code SimpleWorkerPool} that correctly implements {@code + * equals()} and {@code hashCode()}. + */ + static final class SimpleWorkerPoolConfig extends GenericKeyedObjectPoolConfig { + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + SimpleWorkerPoolConfig that = (SimpleWorkerPoolConfig) o; + return getBlockWhenExhausted() == that.getBlockWhenExhausted() + && getFairness() == that.getFairness() + && getJmxEnabled() == that.getJmxEnabled() + && getLifo() == that.getLifo() + && getMaxWaitMillis() == that.getMaxWaitMillis() + && getMinEvictableIdleTimeMillis() == that.getMinEvictableIdleTimeMillis() + && getNumTestsPerEvictionRun() == that.getNumTestsPerEvictionRun() + && getSoftMinEvictableIdleTimeMillis() == that.getSoftMinEvictableIdleTimeMillis() + && getTestOnBorrow() == that.getTestOnBorrow() + && getTestOnCreate() == that.getTestOnCreate() + && getTestOnReturn() == that.getTestOnReturn() + && getTestWhileIdle() == that.getTestWhileIdle() + && getTimeBetweenEvictionRunsMillis() == that.getTimeBetweenEvictionRunsMillis() + && getMaxIdlePerKey() == that.getMaxIdlePerKey() + && getMaxTotal() == that.getMaxTotal() + && getMaxTotalPerKey() == that.getMaxTotalPerKey() + && getMinIdlePerKey() == that.getMinIdlePerKey() + && Objects.equals(getEvictionPolicyClassName(), that.getEvictionPolicyClassName()) + && Objects.equals(getJmxNameBase(), that.getJmxNameBase()) + && Objects.equals(getJmxNamePrefix(), that.getJmxNamePrefix()); + } + + @Override + public int hashCode() { + return Objects.hash( + getBlockWhenExhausted(), + getFairness(), + getJmxEnabled(), + getLifo(), + getMaxWaitMillis(), + getMinEvictableIdleTimeMillis(), + getNumTestsPerEvictionRun(), + getSoftMinEvictableIdleTimeMillis(), + getTestOnBorrow(), + getTestOnCreate(), + getTestOnReturn(), + getTestWhileIdle(), + getTimeBetweenEvictionRunsMillis(), + getMaxIdlePerKey(), + getMaxTotal(), + getMaxTotalPerKey(), + getMinIdlePerKey(), + getEvictionPolicyClassName(), + getJmxNameBase(), + getJmxNamePrefix()); + } + } } diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java index 1b3d25ab3767cf..b6a189e98d24ee 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java @@ -18,6 +18,7 @@ import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; +import java.util.Objects; import java.util.Optional; import java.util.TreeSet; import java.util.concurrent.atomic.AtomicInteger; @@ -35,23 +36,19 @@ class WorkerFactory extends BaseKeyedPooledObjectFactory { // request_id (which is indistinguishable from 0 in proto3). private static final AtomicInteger pidCounter = new AtomicInteger(1); - private WorkerOptions workerOptions; private final Path workerBaseDir; private Reporter reporter; + private final boolean workerSandboxing; - public WorkerFactory(WorkerOptions workerOptions, Path workerBaseDir) { - this.workerOptions = workerOptions; + public WorkerFactory(Path workerBaseDir, boolean workerSandboxing) { this.workerBaseDir = workerBaseDir; + this.workerSandboxing = workerSandboxing; } public void setReporter(Reporter reporter) { this.reporter = reporter; } - public void setOptions(WorkerOptions workerOptions) { - this.workerOptions = workerOptions; - } - @Override public Worker create(WorkerKey key) { int workerId = pidCounter.getAndIncrement(); @@ -60,7 +57,7 @@ public Worker create(WorkerKey key) { workerBaseDir.getRelative(workTypeName + "-" + workerId + "-" + key.getMnemonic() + ".log"); Worker worker; - boolean sandboxed = workerOptions.workerSandboxing || key.isSpeculative(); + boolean sandboxed = workerSandboxing || key.isSpeculative(); if (sandboxed) { Path workDir = getSandboxedWorkerPath(key, workerId); worker = new SandboxedWorker(key, workerId, workDir, logFile); @@ -70,7 +67,7 @@ public Worker create(WorkerKey key) { } else { worker = new SingleplexWorker(key, workerId, key.getExecRoot(), logFile); } - if (workerOptions.workerVerbose) { + if (reporter != null) { reporter.handle( Event.info( String.format( @@ -91,9 +88,7 @@ Path getSandboxedWorkerPath(WorkerKey key, int workerId) { .getRelative(workspaceName); } - /** - * Use the DefaultPooledObject implementation. - */ + /** Use the DefaultPooledObject implementation. */ @Override public PooledObject wrap(Worker worker) { return new DefaultPooledObject<>(worker); @@ -102,7 +97,7 @@ public PooledObject wrap(Worker worker) { /** When a worker process is discarded, destroy its process, too. */ @Override public void destroyObject(WorkerKey key, PooledObject p) { - if (workerOptions.workerVerbose) { + if (reporter != null) { int workerId = p.getObject().getWorkerId(); reporter.handle( Event.info( @@ -122,7 +117,7 @@ public boolean validateObject(WorkerKey key, PooledObject p) { Worker worker = p.getObject(); Optional exitValue = worker.getExitValue(); if (exitValue.isPresent()) { - if (workerOptions.workerVerbose && worker.diedUnexpectedly()) { + if (reporter != null && worker.diedUnexpectedly()) { String msg = String.format( "%s %s (id %d) has unexpectedly died with exit code %d.", @@ -140,7 +135,7 @@ public boolean validateObject(WorkerKey key, PooledObject p) { boolean filesChanged = !key.getWorkerFilesCombinedHash().equals(worker.getWorkerFilesCombinedHash()); - if (workerOptions.workerVerbose && reporter != null && filesChanged) { + if (reporter != null && filesChanged) { StringBuilder msg = new StringBuilder(); msg.append( String.format( @@ -167,4 +162,21 @@ public boolean validateObject(WorkerKey key, PooledObject p) { return !filesChanged; } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (!(o instanceof WorkerFactory)) { + return false; + } + WorkerFactory that = (WorkerFactory) o; + return workerSandboxing == that.workerSandboxing && workerBaseDir.equals(that.workerBaseDir); + } + + @Override + public int hashCode() { + return Objects.hash(workerBaseDir, workerSandboxing); + } } diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java index a34c9bd0fb8445..4ce54658235bc5 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java @@ -15,9 +15,9 @@ import static com.google.common.base.Preconditions.checkNotNull; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import com.google.common.eventbus.Subscribe; import com.google.devtools.build.lib.buildtool.buildevent.BuildCompleteEvent; import com.google.devtools.build.lib.buildtool.buildevent.BuildStartingEvent; @@ -33,23 +33,16 @@ import com.google.devtools.build.lib.sandbox.SandboxHelpers; import com.google.devtools.build.lib.sandbox.SandboxOptions; import com.google.devtools.build.lib.vfs.Path; -import com.google.devtools.build.lib.worker.WorkerOptions.MultiResourceConverter; +import com.google.devtools.build.lib.worker.WorkerPool.WorkerPoolConfig; import com.google.devtools.common.options.OptionsBase; import java.io.IOException; -import java.util.LinkedHashMap; -import java.util.List; -import java.util.Map; -import javax.annotation.Nonnull; /** A module that adds the WorkerActionContextProvider to the available action context providers. */ public class WorkerModule extends BlazeModule { private CommandEnvironment env; private WorkerFactory workerFactory; - private WorkerPool workerPool; - private WorkerOptions options; - private ImmutableMap workerPoolConfig; - private ImmutableMap multiplexPoolConfig; + @VisibleForTesting WorkerPool workerPool; @Override public Iterable> getCommandOptions(Command command) { @@ -68,21 +61,31 @@ public void beforeCommand(CommandEnvironment env) { @Subscribe public void cleanStarting(CleanStartingEvent event) { if (workerPool != null) { - this.options = event.getOptionsProvider().getOptions(WorkerOptions.class); - workerFactory.setReporter(env.getReporter()); - workerFactory.setOptions(options); + WorkerOptions options = event.getOptionsProvider().getOptions(WorkerOptions.class); + workerFactory.setReporter(options.workerVerbose ? env.getReporter() : null); shutdownPool( - "Clean command is running, shutting down worker pool...", /* alwaysLog= */ false); + "Clean command is running, shutting down worker pool...", + /* alwaysLog= */ false, + options.workerVerbose); } } + /** + * Handles updating worker factories and pools when a build starts. If either the workerDir or the + * sandboxing flag has changed, we need to recreate the factory, and we clear out logs at the same + * time. If options affecting the pools have changed, we just change the pools. + */ @Subscribe public void buildStarting(BuildStartingEvent event) { - options = event.getRequest().getOptions(WorkerOptions.class); + WorkerOptions options = event.getRequest().getOptions(WorkerOptions.class); + if (workerFactory != null) { + workerFactory.setReporter(options.workerVerbose ? env.getReporter() : null); + } + Path workerDir = + env.getOutputBase().getRelative(env.getRuntime().getProductName() + "-workers"); - if (workerFactory == null) { - Path workerDir = - env.getOutputBase().getRelative(env.getRuntime().getProductName() + "-workers"); + WorkerFactory newWorkerFactory = new WorkerFactory(workerDir, options.workerSandboxing); + if (!newWorkerFactory.equals(workerFactory)) { try { if (!workerDir.createDirectory()) { // Clean out old log files. @@ -102,51 +105,32 @@ public void buildStarting(BuildStartingEvent event) { .handle(Event.error("Could not create base directory for workers: " + workerDir)); } - workerFactory = new WorkerFactory(options, workerDir); + shutdownPool( + "Worker factory configuration has changed, restarting worker pool...", + /* alwaysLog= */ true, + options.workerVerbose); + workerFactory = newWorkerFactory; + workerFactory.setReporter(options.workerVerbose ? env.getReporter() : null); } - workerFactory.setReporter(env.getReporter()); - workerFactory.setOptions(options); - - ImmutableMap newConfig = createConfigFromOptions(options.workerMaxInstances); - ImmutableMap newMultiplexConfig = - createConfigFromOptions(options.workerMaxMultiplexInstances); + WorkerPoolConfig newConfig = + new WorkerPoolConfig( + workerFactory, + options.workerMaxInstances, + options.workerMaxMultiplexInstances, + options.highPriorityWorkers); // If the config changed compared to the last run, we have to create a new pool. - if ((workerPoolConfig != null && !workerPoolConfig.equals(newConfig)) - || (multiplexPoolConfig != null && !multiplexPoolConfig.equals(newMultiplexConfig))) { + if (workerPool == null || !newConfig.equals(workerPool.getWorkerPoolConfig())) { shutdownPool( - "Worker configuration has changed, restarting worker pool...", /* alwaysLog= */ true); + "Worker pool configuration has changed, restarting worker pool...", + /* alwaysLog= */ true, + options.workerVerbose); } if (workerPool == null) { - workerPoolConfig = newConfig; - multiplexPoolConfig = newMultiplexConfig; - workerPool = - new WorkerPool( - workerFactory, workerPoolConfig, multiplexPoolConfig, options.highPriorityWorkers); - } - } - - /** - * Creates a configuration for a worker pool from the options given. If the same mnemonic occurs - * more than once in the options, the last value passed wins. - */ - @Nonnull - private static ImmutableMap createConfigFromOptions( - List> options) { - LinkedHashMap newConfigBuilder = new LinkedHashMap<>(); - for (Map.Entry entry : options) { - newConfigBuilder.put(entry.getKey(), entry.getValue()); + workerPool = new WorkerPool(newConfig); } - - if (!newConfigBuilder.containsKey("")) { - // Empty string gives the number of workers for any type of worker not explicitly specified. - // If no value is given, use the default, 2. - newConfigBuilder.put("", MultiResourceConverter.DEFAULT_VALUE); - } - - return ImmutableMap.copyOf(newConfigBuilder); } @Override @@ -154,7 +138,7 @@ public void registerSpawnStrategies( SpawnStrategyRegistry.Builder registryBuilder, CommandEnvironment env) { checkNotNull(workerPool); SandboxOptions sandboxOptions = env.getOptions().getOptions(SandboxOptions.class); - options = env.getOptions().getOptions(WorkerOptions.class); + WorkerOptions options = env.getOptions().getOptions(WorkerOptions.class); LocalEnvProvider localEnvProvider = LocalEnvProvider.forCurrentOs(env.getClientEnv()); WorkerSpawnRunner spawnRunner = new WorkerSpawnRunner( @@ -178,17 +162,21 @@ public void registerSpawnStrategies( @Subscribe public void buildComplete(BuildCompleteEvent event) { + WorkerOptions options = env.getOptions().getOptions(WorkerOptions.class); if (options != null && options.workerQuitAfterBuild) { - shutdownPool("Build completed, shutting down worker pool...", /* alwaysLog= */ false); + shutdownPool( + "Build completed, shutting down worker pool...", + /* alwaysLog= */ false, + options.workerVerbose); } } /** Shuts down the worker pool and sets {#code workerPool} to null. */ - private void shutdownPool(String reason, boolean alwaysLog) { + private void shutdownPool(String reason, boolean alwaysLog, boolean workerVerbose) { Preconditions.checkArgument(!reason.isEmpty()); if (workerPool != null) { - if ((options != null && options.workerVerbose) || alwaysLog) { + if (workerVerbose || alwaysLog) { env.getReporter().handle(Event.info(reason)); } workerPool.close(); @@ -199,7 +187,6 @@ private void shutdownPool(String reason, boolean alwaysLog) { @Override public void afterCommand() { this.env = null; - this.options = null; if (this.workerFactory != null) { this.workerFactory.setReporter(null); diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerPool.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerPool.java index ba630a52578da9..7bb8f6475e5dbd 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerPool.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerPool.java @@ -16,9 +16,15 @@ import com.google.common.base.Throwables; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.worker.WorkerOptions.MultiResourceConverter; import java.io.IOException; +import java.util.LinkedHashMap; +import java.util.List; import java.util.Map; +import java.util.Map.Entry; +import java.util.Objects; import java.util.concurrent.atomic.AtomicInteger; +import javax.annotation.Nonnull; import javax.annotation.concurrent.ThreadSafe; import org.apache.commons.pool2.impl.GenericKeyedObjectPool; @@ -45,70 +51,66 @@ final class WorkerPool { private final AtomicInteger highPriorityWorkersInUse = new AtomicInteger(0); /** Which mnemonics create high-priority workers. */ private final ImmutableSet highPriorityWorkerMnemonics; + + private final WorkerPoolConfig workerPoolConfig; /** Map of singleplex worker pools, one per mnemonic. */ private final ImmutableMap workerPools; /** Map of multiplex worker pools, one per mnemonic. */ private final ImmutableMap multiplexPools; + public WorkerPool(WorkerPoolConfig workerPoolConfig) { + this.workerPoolConfig = workerPoolConfig; + + highPriorityWorkerMnemonics = + ImmutableSet.copyOf((Iterable) workerPoolConfig.getHighPriorityWorkers()); + + Map config = createConfigFromOptions(workerPoolConfig.getWorkerMaxInstances()); + Map multiplexConfig = + createConfigFromOptions(workerPoolConfig.getWorkerMaxMultiplexInstances()); + + workerPools = + createWorkerPools(workerPoolConfig.getWorkerFactory(), config, DEFAULT_MAX_WORKERS); + multiplexPools = + createWorkerPools( + workerPoolConfig.getWorkerFactory(), multiplexConfig, DEFAULT_MAX_MULTIPLEX_WORKERS); + } + + public WorkerPoolConfig getWorkerPoolConfig() { + return workerPoolConfig; + } + /** - * @param factory worker factory - * @param config pool configuration; max number of workers per WorkerKey for each mnemonic; the - * empty string key specifies the default maximum - * @param multiplexConfig like {@code config}, but for multiplex workers - * @param highPriorityWorkers mnemonics of high priority workers + * Creates a configuration for a worker pool from the options given. If the same mnemonic occurs + * more than once in the options, the last value passed wins. */ - public WorkerPool( - WorkerFactory factory, - Map config, - Map multiplexConfig, - Iterable highPriorityWorkers) { - highPriorityWorkerMnemonics = ImmutableSet.copyOf(highPriorityWorkers); - workerPools = createWorkerPools(factory, config, DEFAULT_MAX_WORKERS); - multiplexPools = createWorkerPools(factory, multiplexConfig, DEFAULT_MAX_MULTIPLEX_WORKERS); + @Nonnull + private static ImmutableMap createConfigFromOptions( + List> options) { + LinkedHashMap newConfigBuilder = new LinkedHashMap<>(); + for (Map.Entry entry : options) { + newConfigBuilder.put(entry.getKey(), entry.getValue()); + } + + if (!newConfigBuilder.containsKey("")) { + // Empty string gives the number of workers for any type of worker not explicitly specified. + // If no value is given, use the default, 2. + newConfigBuilder.put("", MultiResourceConverter.DEFAULT_VALUE); + } + + return ImmutableMap.copyOf(newConfigBuilder); } private static ImmutableMap createWorkerPools( WorkerFactory factory, Map config, int defaultMaxWorkers) { ImmutableMap.Builder workerPoolsBuilder = ImmutableMap.builder(); config.forEach( - (key, value) -> - workerPoolsBuilder.put(key, new SimpleWorkerPool(factory, makeConfig(value)))); + (key, value) -> workerPoolsBuilder.put(key, new SimpleWorkerPool(factory, value))); if (!config.containsKey("")) { - workerPoolsBuilder.put("", new SimpleWorkerPool(factory, makeConfig(defaultMaxWorkers))); + workerPoolsBuilder.put("", new SimpleWorkerPool(factory, defaultMaxWorkers)); } return workerPoolsBuilder.build(); } - private static WorkerPoolConfig makeConfig(int max) { - WorkerPoolConfig config = new WorkerPoolConfig(); - - // It's better to re-use a worker as often as possible and keep it hot, in order to profit - // from JIT optimizations as much as possible. - config.setLifo(true); - - // Keep a fixed number of workers running per key. - config.setMaxIdlePerKey(max); - config.setMaxTotalPerKey(max); - config.setMinIdlePerKey(max); - - // Don't limit the total number of worker processes, as otherwise the pool might be full of - // workers for one WorkerKey and can't accommodate a worker for another WorkerKey. - config.setMaxTotal(-1); - - // Wait for a worker to become ready when a thread needs one. - config.setBlockWhenExhausted(true); - - // Always test the liveliness of worker processes. - config.setTestOnBorrow(true); - config.setTestOnCreate(true); - config.setTestOnReturn(true); - - // No eviction of idle workers. - config.setTimeBetweenEvictionRunsMillis(-1); - - return config; - } - private SimpleWorkerPool getPool(WorkerKey key) { if (key.isMultiplex()) { return multiplexPools.getOrDefault(key.getMnemonic(), multiplexPools.get("")); @@ -197,4 +199,59 @@ public void close() { workerPools.values().forEach(GenericKeyedObjectPool::close); multiplexPools.values().forEach(GenericKeyedObjectPool::close); } + + static class WorkerPoolConfig { + private final WorkerFactory workerFactory; + private final List> workerMaxInstances; + private final List> workerMaxMultiplexInstances; + private final List highPriorityWorkers; + + WorkerPoolConfig( + WorkerFactory workerFactory, + List> workerMaxInstances, + List> workerMaxMultiplexInstances, + List highPriorityWorkers) { + this.workerFactory = workerFactory; + this.workerMaxInstances = workerMaxInstances; + this.workerMaxMultiplexInstances = workerMaxMultiplexInstances; + this.highPriorityWorkers = highPriorityWorkers; + } + + public WorkerFactory getWorkerFactory() { + return workerFactory; + } + + public List> getWorkerMaxInstances() { + return workerMaxInstances; + } + + public List> getWorkerMaxMultiplexInstances() { + return workerMaxMultiplexInstances; + } + + public List getHighPriorityWorkers() { + return highPriorityWorkers; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (!(o instanceof WorkerPoolConfig)) { + return false; + } + WorkerPoolConfig that = (WorkerPoolConfig) o; + return workerFactory.equals(that.workerFactory) + && workerMaxInstances.equals(that.workerMaxInstances) + && workerMaxMultiplexInstances.equals(that.workerMaxMultiplexInstances) + && highPriorityWorkers.equals(that.highPriorityWorkers); + } + + @Override + public int hashCode() { + return Objects.hash( + workerFactory, workerMaxInstances, workerMaxMultiplexInstances, highPriorityWorkers); + } + } } diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerPoolConfig.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerPoolConfig.java deleted file mode 100644 index 20d40dfcbca736..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerPoolConfig.java +++ /dev/null @@ -1,79 +0,0 @@ -// Copyright 2016 The Bazel Authors. All rights reserved. -// -// 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.google.devtools.build.lib.worker; - -import java.util.Objects; -import org.apache.commons.pool2.impl.GenericKeyedObjectPoolConfig; - -/** - * Our own configuration class for the {@code WorkerPool} that correctly implements {@code equals()} - * and {@code hashCode()}. - */ -final class WorkerPoolConfig extends GenericKeyedObjectPoolConfig { - @Override - public boolean equals(Object o) { - if (this == o) { - return true; - } - if (o == null || getClass() != o.getClass()) { - return false; - } - WorkerPoolConfig that = (WorkerPoolConfig) o; - return getBlockWhenExhausted() == that.getBlockWhenExhausted() - && getFairness() == that.getFairness() - && getJmxEnabled() == that.getJmxEnabled() - && getLifo() == that.getLifo() - && getMaxWaitMillis() == that.getMaxWaitMillis() - && getMinEvictableIdleTimeMillis() == that.getMinEvictableIdleTimeMillis() - && getNumTestsPerEvictionRun() == that.getNumTestsPerEvictionRun() - && getSoftMinEvictableIdleTimeMillis() == that.getSoftMinEvictableIdleTimeMillis() - && getTestOnBorrow() == that.getTestOnBorrow() - && getTestOnCreate() == that.getTestOnCreate() - && getTestOnReturn() == that.getTestOnReturn() - && getTestWhileIdle() == that.getTestWhileIdle() - && getTimeBetweenEvictionRunsMillis() == that.getTimeBetweenEvictionRunsMillis() - && getMaxIdlePerKey() == that.getMaxIdlePerKey() - && getMaxTotal() == that.getMaxTotal() - && getMaxTotalPerKey() == that.getMaxTotalPerKey() - && getMinIdlePerKey() == that.getMinIdlePerKey() - && Objects.equals(getEvictionPolicyClassName(), that.getEvictionPolicyClassName()) - && Objects.equals(getJmxNameBase(), that.getJmxNameBase()) - && Objects.equals(getJmxNamePrefix(), that.getJmxNamePrefix()); - } - - @Override - public int hashCode() { - return Objects.hash( - getBlockWhenExhausted(), - getFairness(), - getJmxEnabled(), - getLifo(), - getMaxWaitMillis(), - getMinEvictableIdleTimeMillis(), - getNumTestsPerEvictionRun(), - getSoftMinEvictableIdleTimeMillis(), - getTestOnBorrow(), - getTestOnCreate(), - getTestOnReturn(), - getTestWhileIdle(), - getTimeBetweenEvictionRunsMillis(), - getMaxIdlePerKey(), - getMaxTotal(), - getMaxTotalPerKey(), - getMinIdlePerKey(), - getEvictionPolicyClassName(), - getJmxNameBase(), - getJmxNamePrefix()); - } -} diff --git a/src/test/java/com/google/devtools/build/lib/worker/BUILD b/src/test/java/com/google/devtools/build/lib/worker/BUILD index 88afc7f3de68f5..83935fc31ecb24 100644 --- a/src/test/java/com/google/devtools/build/lib/worker/BUILD +++ b/src/test/java/com/google/devtools/build/lib/worker/BUILD @@ -61,14 +61,18 @@ java_library( srcs = glob(["*Test.java"]), deps = [ ":testutil", + "//src/main/java/com/google/devtools/build/lib:runtime", "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:execution_requirements", + "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories", + "//src/main/java/com/google/devtools/build/lib/analysis:server_directories", "//src/main/java/com/google/devtools/build/lib/clock", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/exec:spawn_runner", "//src/main/java/com/google/devtools/build/lib/exec/local", "//src/main/java/com/google/devtools/build/lib/sandbox", + "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception", "//src/main/java/com/google/devtools/build/lib/util:resource_converter", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", diff --git a/src/test/java/com/google/devtools/build/lib/worker/ExampleWorker.java b/src/test/java/com/google/devtools/build/lib/worker/ExampleWorker.java index 96e2e86b71548d..50439bd31cad2c 100644 --- a/src/test/java/com/google/devtools/build/lib/worker/ExampleWorker.java +++ b/src/test/java/com/google/devtools/build/lib/worker/ExampleWorker.java @@ -86,6 +86,7 @@ public void processRequests() throws IOException { if (request == null) { break; } + currentRequest = request; inputs.clear(); for (Input input : request.getInputsList()) { diff --git a/src/test/java/com/google/devtools/build/lib/worker/WorkerPoolConfigTest.java b/src/test/java/com/google/devtools/build/lib/worker/SimpleWorkerPoolConfigTest.java similarity index 85% rename from src/test/java/com/google/devtools/build/lib/worker/WorkerPoolConfigTest.java rename to src/test/java/com/google/devtools/build/lib/worker/SimpleWorkerPoolConfigTest.java index 27b7aac2e02a21..a314a9c5987579 100644 --- a/src/test/java/com/google/devtools/build/lib/worker/WorkerPoolConfigTest.java +++ b/src/test/java/com/google/devtools/build/lib/worker/SimpleWorkerPoolConfigTest.java @@ -14,20 +14,18 @@ package com.google.devtools.build.lib.worker; import com.google.common.testing.EqualsTester; - +import com.google.devtools.build.lib.worker.SimpleWorkerPool.SimpleWorkerPoolConfig; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -/** - * Test WorkerPoolConfig. - */ +/** Test SimpleWorkerPoolConfig. */ @RunWith(JUnit4.class) -public class WorkerPoolConfigTest { +public class SimpleWorkerPoolConfigTest { @Test public void testEquals() throws Exception { - WorkerPoolConfig config1a = new WorkerPoolConfig(); + SimpleWorkerPoolConfig config1a = new SimpleWorkerPoolConfig(); config1a.setLifo(true); config1a.setMaxIdlePerKey(4); config1a.setMaxTotalPerKey(4); @@ -39,7 +37,7 @@ public void testEquals() throws Exception { config1a.setTestOnReturn(true); config1a.setTimeBetweenEvictionRunsMillis(-1); - WorkerPoolConfig config1b = new WorkerPoolConfig(); + SimpleWorkerPoolConfig config1b = new SimpleWorkerPoolConfig(); config1b.setLifo(true); config1b.setMaxIdlePerKey(4); config1b.setMaxTotalPerKey(4); @@ -51,7 +49,7 @@ public void testEquals() throws Exception { config1b.setTestOnReturn(true); config1b.setTimeBetweenEvictionRunsMillis(-1); - WorkerPoolConfig config2a = new WorkerPoolConfig(); + SimpleWorkerPoolConfig config2a = new SimpleWorkerPoolConfig(); config2a.setLifo(true); config2a.setMaxIdlePerKey(1); config2a.setMaxTotalPerKey(1); @@ -63,7 +61,7 @@ public void testEquals() throws Exception { config2a.setTestOnReturn(true); config2a.setTimeBetweenEvictionRunsMillis(-1); - WorkerPoolConfig config2b = new WorkerPoolConfig(); + SimpleWorkerPoolConfig config2b = new SimpleWorkerPoolConfig(); config2b.setLifo(true); config2b.setMaxIdlePerKey(1); config2b.setMaxTotalPerKey(1); diff --git a/src/test/java/com/google/devtools/build/lib/worker/WorkerFactoryTest.java b/src/test/java/com/google/devtools/build/lib/worker/WorkerFactoryTest.java index fa373493818b4d..81671d0b9588c8 100644 --- a/src/test/java/com/google/devtools/build/lib/worker/WorkerFactoryTest.java +++ b/src/test/java/com/google/devtools/build/lib/worker/WorkerFactoryTest.java @@ -41,7 +41,8 @@ public class WorkerFactoryTest { @Test public void sandboxedWorkerPathEndsWithWorkspaceName() throws Exception { Path workerBaseDir = fs.getPath("/outputbase/bazel-workers"); - WorkerFactory workerFactory = new WorkerFactory(new WorkerOptions(), workerBaseDir); + final WorkerOptions workerOptions = new WorkerOptions(); + WorkerFactory workerFactory = new WorkerFactory(workerBaseDir, workerOptions.workerSandboxing); WorkerKey workerKey = createWorkerKey(/* mustBeSandboxed */ true, /* proxied */ false); Path sandboxedWorkerPath = workerFactory.getSandboxedWorkerPath(workerKey, 1); @@ -66,7 +67,8 @@ protected WorkerKey createWorkerKey(boolean mustBeSandboxed, boolean proxied, St @Test public void workerCreationTypeCheck() throws Exception { Path workerBaseDir = fs.getPath("/outputbase/bazel-workers"); - WorkerFactory workerFactory = new WorkerFactory(new WorkerOptions(), workerBaseDir); + final WorkerOptions workerOptions = new WorkerOptions(); + WorkerFactory workerFactory = new WorkerFactory(workerBaseDir, workerOptions.workerSandboxing); WorkerKey sandboxedWorkerKey = createWorkerKey(/* mustBeSandboxed */ true, /* proxied */ false); Worker sandboxedWorker = workerFactory.create(sandboxedWorkerKey); assertThat(sandboxedWorker.getClass()).isEqualTo(SandboxedWorker.class); @@ -88,7 +90,8 @@ public void workerCreationTypeCheck() throws Exception { @Test public void testMultiplexWorkersShareLogfiles() throws Exception { Path workerBaseDir = fs.getPath("/outputbase/bazel-workers"); - WorkerFactory workerFactory = new WorkerFactory(new WorkerOptions(), workerBaseDir); + final WorkerOptions workerOptions = new WorkerOptions(); + WorkerFactory workerFactory = new WorkerFactory(workerBaseDir, workerOptions.workerSandboxing); WorkerKey workerKey1 = createWorkerKey(/* mustBeSandboxed */ false, /* proxied */ true, "arg1"); Worker proxiedWorker1a = workerFactory.create(workerKey1); diff --git a/src/test/java/com/google/devtools/build/lib/worker/WorkerModuleTest.java b/src/test/java/com/google/devtools/build/lib/worker/WorkerModuleTest.java new file mode 100644 index 00000000000000..a33d1fb171826d --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/worker/WorkerModuleTest.java @@ -0,0 +1,269 @@ +// Copyright 2021 The Bazel Authors. All rights reserved. +// +// 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.google.devtools.build.lib.worker; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.devtools.build.lib.actions.ExecutionRequirements.WorkerProtocolFormat.JSON; +import static org.mockito.Mockito.when; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Lists; +import com.google.common.collect.Maps; +import com.google.common.eventbus.EventBus; +import com.google.devtools.build.lib.analysis.BlazeDirectories; +import com.google.devtools.build.lib.analysis.ServerDirectories; +import com.google.devtools.build.lib.buildtool.BuildRequest; +import com.google.devtools.build.lib.buildtool.buildevent.BuildStartingEvent; +import com.google.devtools.build.lib.clock.BlazeClock; +import com.google.devtools.build.lib.events.Reporter; +import com.google.devtools.build.lib.events.StoredEventHandler; +import com.google.devtools.build.lib.runtime.BlazeRuntime; +import com.google.devtools.build.lib.runtime.CommandEnvironment; +import com.google.devtools.build.lib.util.AbruptExitException; +import com.google.devtools.build.lib.vfs.DigestHashFunction; +import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; +import com.google.devtools.common.options.OptionsParsingResult; +import java.io.IOException; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnit; +import org.mockito.junit.MockitoRule; + +/** Tests for WorkerModule. I bet you didn't see that coming, eh? */ +@RunWith(JUnit4.class) +public class WorkerModuleTest { + @Rule public final MockitoRule mockito = MockitoJUnit.rule(); + @Mock CommandEnvironment env; + @Mock BuildRequest request; + @Mock OptionsParsingResult startupOptionsProvider; + + private FileSystem fs; + private StoredEventHandler storedEventHandler; + + @Before + public void setUp() { + fs = new InMemoryFileSystem(BlazeClock.instance(), DigestHashFunction.SHA256); + } + + @Test + public void buildStarting_createsPools() + throws AbruptExitException, IOException, InterruptedException { + WorkerModule module = new WorkerModule(); + WorkerOptions options = WorkerOptions.DEFAULTS; + when(request.getOptions(WorkerOptions.class)).thenReturn(options); + setupEnvironment("/outputRoot"); + + module.beforeCommand(env); + module.buildStarting(new BuildStartingEvent(env, request)); + assertThat(storedEventHandler.getEvents()).isEmpty(); + assertThat(fs.getPath("/outputRoot/outputBase/bazel-workers").exists()).isTrue(); + assertThat(module.workerPool).isNotNull(); + WorkerKey workerKey = TestUtils.createWorkerKey(JSON, fs); + Worker worker = module.workerPool.borrowObject(workerKey); + assertThat(worker.workerKey).isEqualTo(workerKey); + } + + @Test + public void buildStarting_restartsOnSandboxChanges() throws IOException, AbruptExitException { + WorkerModule module = new WorkerModule(); + WorkerOptions options = WorkerOptions.DEFAULTS; + when(request.getOptions(WorkerOptions.class)).thenReturn(options); + setupEnvironment("/outputRoot"); + + module.beforeCommand(env); + module.buildStarting(new BuildStartingEvent(env, request)); + assertThat(storedEventHandler.getEvents()).isEmpty(); + + Path workerDir = fs.getPath("/outputRoot/outputBase/bazel-workers"); + Path aLog = workerDir.getRelative("f.log"); + aLog.createSymbolicLink(PathFragment.EMPTY_FRAGMENT); + WorkerPool oldPool = module.workerPool; + options.workerSandboxing = !options.workerSandboxing; + module.beforeCommand(env); + module.buildStarting(new BuildStartingEvent(env, request)); + assertThat(storedEventHandler.getEvents()).hasSize(1); + assertThat(storedEventHandler.getEvents().get(0).getMessage()) + .contains("Worker factory configuration has changed"); + assertThat(module.workerPool).isNotSameInstanceAs(oldPool); + assertThat(aLog.exists()).isFalse(); + } + + @Test + public void buildStarting_workersDestroyedOnRestart() + throws IOException, AbruptExitException, InterruptedException { + WorkerModule module = new WorkerModule(); + WorkerOptions options = WorkerOptions.DEFAULTS; + options.workerVerbose = true; + when(request.getOptions(WorkerOptions.class)).thenReturn(options); + setupEnvironment("/outputRoot"); + + module.beforeCommand(env); + module.buildStarting(new BuildStartingEvent(env, request)); + WorkerKey workerKey = TestUtils.createWorkerKey(JSON, fs); + Worker worker = module.workerPool.borrowObject(workerKey); + assertThat(worker.workerKey).isEqualTo(workerKey); + assertThat(storedEventHandler.getEvents()).hasSize(1); + assertThat(storedEventHandler.getEvents().get(0).getMessage()) + .contains("Created new sandboxed dummy worker"); + storedEventHandler.clear(); + + Path workerDir = fs.getPath("/outputRoot/outputBase/bazel-workers"); + Path aLog = workerDir.getRelative("f.log"); + aLog.createSymbolicLink(PathFragment.EMPTY_FRAGMENT); + WorkerPool oldPool = module.workerPool; + options.workerSandboxing = !options.workerSandboxing; + module.beforeCommand(env); + module.buildStarting(new BuildStartingEvent(env, request)); + assertThat(storedEventHandler.getEvents()).hasSize(1); + assertThat(storedEventHandler.getEvents().get(0).getMessage()) + .contains("Worker factory configuration has changed"); + assertThat(module.workerPool).isNotSameInstanceAs(oldPool); + assertThat(aLog.exists()).isFalse(); + } + + @Test + public void buildStarting_restartsOnOutputbaseChanges() throws IOException, AbruptExitException { + WorkerModule module = new WorkerModule(); + WorkerOptions options = WorkerOptions.DEFAULTS; + when(request.getOptions(WorkerOptions.class)).thenReturn(options); + setupEnvironment("/outputRoot"); + + module.beforeCommand(env); + module.buildStarting(new BuildStartingEvent(env, request)); + assertThat(storedEventHandler.getEvents()).isEmpty(); + + // Log file from old root, doesn't get cleaned + Path workerDir = fs.getPath("/outputRoot/outputBase/bazel-workers"); + Path oldLog = workerDir.getRelative("f.log"); + oldLog.createSymbolicLink(PathFragment.EMPTY_FRAGMENT); + + WorkerPool oldPool = module.workerPool; + setupEnvironment("/otherRootDir"); + module.beforeCommand(env); + module.buildStarting(new BuildStartingEvent(env, request)); + assertThat(storedEventHandler.getEvents()).hasSize(1); + assertThat(storedEventHandler.getEvents().get(0).getMessage()) + .contains("Worker factory configuration has changed"); + assertThat(module.workerPool).isNotSameInstanceAs(oldPool); + assertThat(fs.getPath("/otherRootDir/outputBase/bazel-workers").exists()).isTrue(); + assertThat(oldLog.exists()).isTrue(); + } + + @Test + public void buildStarting_restartsOnHiPrioChanges() throws IOException, AbruptExitException { + WorkerModule module = new WorkerModule(); + WorkerOptions options = WorkerOptions.DEFAULTS; + when(request.getOptions(WorkerOptions.class)).thenReturn(options); + setupEnvironment("/outputRoot"); + + module.beforeCommand(env); + // Check that new pools/factories are made with default options + module.buildStarting(new BuildStartingEvent(env, request)); + assertThat(storedEventHandler.getEvents()).isEmpty(); + + // Logs are only cleared on factory reset, not on pool reset, so this file should survive + Path workerDir = fs.getPath("/outputRoot/outputBase/bazel-workers"); + Path oldLog = workerDir.getRelative("f.log"); + oldLog.createSymbolicLink(PathFragment.EMPTY_FRAGMENT); + + WorkerPool oldPool = module.workerPool; + options.highPriorityWorkers = ImmutableList.of("foo"); + module.beforeCommand(env); + module.buildStarting(new BuildStartingEvent(env, request)); + assertThat(storedEventHandler.getEvents()).hasSize(1); + assertThat(storedEventHandler.getEvents().get(0).getMessage()) + .contains("Worker pool configuration has changed"); + assertThat(module.workerPool).isNotSameInstanceAs(oldPool); + assertThat(oldLog.exists()).isTrue(); + } + + @Test + public void buildStarting_restartsOnNumMultiplexWorkersChanges() + throws IOException, AbruptExitException { + WorkerModule module = new WorkerModule(); + WorkerOptions options = WorkerOptions.DEFAULTS; + when(request.getOptions(WorkerOptions.class)).thenReturn(options); + setupEnvironment("/outputRoot"); + + module.beforeCommand(env); + // Check that new pools/factories are made with default options + module.buildStarting(new BuildStartingEvent(env, request)); + assertThat(storedEventHandler.getEvents()).isEmpty(); + + WorkerPool oldPool = module.workerPool; + options.workerMaxMultiplexInstances = Lists.newArrayList(Maps.immutableEntry("foo", 42)); + module.beforeCommand(env); + module.buildStarting(new BuildStartingEvent(env, request)); + assertThat(storedEventHandler.getEvents()).hasSize(1); + assertThat(storedEventHandler.getEvents().get(0).getMessage()) + .contains("Worker pool configuration has changed"); + assertThat(module.workerPool).isNotSameInstanceAs(oldPool); + } + + @Test + public void buildStarting_restartsOnNumWorkersChanges() throws IOException, AbruptExitException { + WorkerModule module = new WorkerModule(); + WorkerOptions options = WorkerOptions.DEFAULTS; + + when(request.getOptions(WorkerOptions.class)).thenReturn(options); + setupEnvironment("/outputRoot"); + + module.beforeCommand(env); + // Check that new pools/factories are made with default options + module.buildStarting(new BuildStartingEvent(env, request)); + assertThat(storedEventHandler.getEvents()).isEmpty(); + + WorkerPool oldPool = module.workerPool; + options.workerMaxInstances = Lists.newArrayList(Maps.immutableEntry("bar", 3)); + module.beforeCommand(env); + module.buildStarting(new BuildStartingEvent(env, request)); + assertThat(storedEventHandler.getEvents()).hasSize(1); + assertThat(storedEventHandler.getEvents().get(0).getMessage()) + .contains("Worker pool configuration has changed"); + assertThat(module.workerPool).isNotSameInstanceAs(oldPool); + } + + private void setupEnvironment(String rootDir) throws IOException, AbruptExitException { + storedEventHandler = new StoredEventHandler(); + Path root = fs.getPath(rootDir); + Path outputBase = root.getRelative("outputBase"); + outputBase.createDirectoryAndParents(); + when(env.getOutputBase()).thenReturn(outputBase); + Path workspace = fs.getPath("/workspace"); + when(env.getWorkingDirectory()).thenReturn(workspace); + ServerDirectories serverDirectories = + new ServerDirectories( + root.getRelative("userroot/install"), outputBase, root.getRelative("userroot")); + BlazeRuntime blazeRuntime = + new BlazeRuntime.Builder() + .setProductName("bazel") + .setServerDirectories(serverDirectories) + .setStartupOptionsProvider(startupOptionsProvider) + .build(); + when(env.getRuntime()).thenReturn(blazeRuntime); + when(env.getDirectories()) + .thenReturn(new BlazeDirectories(serverDirectories, null, null, "blaze")); + EventBus eventBus = new EventBus(); + when(env.getEventBus()).thenReturn(eventBus); + when(env.getReporter()).thenReturn(new Reporter(eventBus, storedEventHandler)); + } +} diff --git a/src/test/java/com/google/devtools/build/lib/worker/WorkerPoolTest.java b/src/test/java/com/google/devtools/build/lib/worker/WorkerPoolTest.java index 036ca7b11be835..20cb532c6484a7 100644 --- a/src/test/java/com/google/devtools/build/lib/worker/WorkerPoolTest.java +++ b/src/test/java/com/google/devtools/build/lib/worker/WorkerPoolTest.java @@ -23,15 +23,17 @@ import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; +import com.google.common.collect.Maps; import com.google.devtools.build.lib.clock.BlazeClock; import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; +import com.google.devtools.build.lib.worker.WorkerPool.WorkerPoolConfig; import java.io.IOException; import java.lang.Thread.State; +import java.util.Map.Entry; import org.apache.commons.pool2.impl.DefaultPooledObject; import org.junit.Before; import org.junit.Rule; @@ -60,14 +62,13 @@ private static class TestWorker extends SingleplexWorker { public void setUp() throws Exception { fileSystem = new InMemoryFileSystem(BlazeClock.instance(), DigestHashFunction.SHA256); doAnswer( - arg -> { - return new DefaultPooledObject<>( - new TestWorker( - arg.getArgument(0), - workerIds++, - fileSystem.getPath("/workDir"), - fileSystem.getPath("/logDir"))); - }) + arg -> + new DefaultPooledObject<>( + new TestWorker( + arg.getArgument(0), + workerIds++, + fileSystem.getPath("/workDir"), + fileSystem.getPath("/logDir")))) .when(factoryMock) .makeObject(any()); when(factoryMock.validateObject(any(), any())).thenReturn(true); @@ -77,10 +78,8 @@ public void setUp() throws Exception { public void testBorrow_createsWhenNeeded() throws Exception { WorkerPool workerPool = new WorkerPool( - factoryMock, - ImmutableMap.of("mnem", 2, "", 1), - ImmutableMap.of(), - Lists.newArrayList()); + new WorkerPoolConfig( + factoryMock, entryList("mnem", 2, "", 1), entryList(), Lists.newArrayList())); WorkerKey workerKey = createWorkerKey(fileSystem, "mnem", false); Worker worker1 = workerPool.borrowObject(workerKey); Worker worker2 = workerPool.borrowObject(workerKey); @@ -93,10 +92,8 @@ public void testBorrow_createsWhenNeeded() throws Exception { public void testBorrow_reusesWhenPossible() throws Exception { WorkerPool workerPool = new WorkerPool( - factoryMock, - ImmutableMap.of("mnem", 2, "", 1), - ImmutableMap.of(), - Lists.newArrayList()); + new WorkerPoolConfig( + factoryMock, entryList("mnem", 2, "", 1), entryList(), Lists.newArrayList())); WorkerKey workerKey = createWorkerKey(fileSystem, "mnem", false); Worker worker1 = workerPool.borrowObject(workerKey); workerPool.returnObject(workerKey, worker1); @@ -109,10 +106,8 @@ public void testBorrow_reusesWhenPossible() throws Exception { public void testBorrow_usesDefault() throws Exception { WorkerPool workerPool = new WorkerPool( - factoryMock, - ImmutableMap.of("mnem", 2, "", 1), - ImmutableMap.of(), - Lists.newArrayList()); + new WorkerPoolConfig( + factoryMock, entryList("mnem", 2, "", 1), entryList(), Lists.newArrayList())); WorkerKey workerKey1 = createWorkerKey(fileSystem, "mnem", false); Worker worker1 = workerPool.borrowObject(workerKey1); Worker worker1a = workerPool.borrowObject(workerKey1); @@ -129,10 +124,8 @@ public void testBorrow_usesDefault() throws Exception { public void testBorrow_pooledByKey() throws Exception { WorkerPool workerPool = new WorkerPool( - factoryMock, - ImmutableMap.of("mnem", 2, "", 1), - ImmutableMap.of(), - Lists.newArrayList()); + new WorkerPoolConfig( + factoryMock, entryList("mnem", 2, "", 1), entryList(), Lists.newArrayList())); WorkerKey workerKey1 = createWorkerKey(fileSystem, "mnem", false); Worker worker1 = workerPool.borrowObject(workerKey1); Worker worker1a = workerPool.borrowObject(workerKey1); @@ -149,10 +142,11 @@ public void testBorrow_pooledByKey() throws Exception { public void testBorrow_separateMultiplexWorkers() throws Exception { WorkerPool workerPool = new WorkerPool( - factoryMock, - ImmutableMap.of("mnem", 1, "", 1), - ImmutableMap.of("mnem", 2, "", 1), - Lists.newArrayList()); + new WorkerPoolConfig( + factoryMock, + entryList("mnem", 1, "", 1), + entryList("mnem", 2, "", 1), + Lists.newArrayList())); WorkerKey workerKey = createWorkerKey(fileSystem, "mnem", false); Worker worker1 = workerPool.borrowObject(workerKey); assertThat(worker1.getWorkerId()).isEqualTo(1); @@ -175,10 +169,11 @@ public void testBorrow_separateMultiplexWorkers() throws Exception { public void testBorrow_allowsOneHiPrio() throws Exception { WorkerPool workerPool = new WorkerPool( - factoryMock, - ImmutableMap.of("loprio", 2, "hiprio", 2, "", 1), - ImmutableMap.of(), - ImmutableList.of("hiprio")); + new WorkerPoolConfig( + factoryMock, + entryList("loprio", 2, "hiprio", 2, "", 1), + entryList(), + ImmutableList.of("hiprio"))); WorkerKey workerKey1 = createWorkerKey(fileSystem, "hiprio", false); Worker worker1 = workerPool.borrowObject(workerKey1); assertThat(worker1.getWorkerId()).isEqualTo(1); @@ -194,10 +189,11 @@ public void testBorrow_allowsOneHiPrio() throws Exception { public void testBorrow_twoHiPrioBlocks() throws Exception { WorkerPool workerPool = new WorkerPool( - factoryMock, - ImmutableMap.of("loprio", 2, "hiprio", 2, "", 1), - ImmutableMap.of(), - ImmutableList.of("hiprio")); + new WorkerPoolConfig( + factoryMock, + entryList("loprio", 2, "hiprio", 2, "", 1), + entryList(), + ImmutableList.of("hiprio"))); WorkerKey workerKey1 = createWorkerKey(fileSystem, "hiprio", false); Worker worker1 = workerPool.borrowObject(workerKey1); Worker worker1a = workerPool.borrowObject(workerKey1); @@ -238,4 +234,21 @@ public void testBorrow_twoHiPrioBlocks() throws Exception { verify(factoryMock, times(2)).makeObject(workerKey1); verify(factoryMock, times(1)).makeObject(workerKey2); } + + private static ImmutableList> entryList() { + return ImmutableList.of(); + } + + private static ImmutableList> entryList( + String key1, int value1, String key2, int value2) { + return ImmutableList.of(Maps.immutableEntry(key1, value1), Maps.immutableEntry(key2, value2)); + } + + private static ImmutableList> entryList( + String key1, int value1, String key2, int value2, String key3, int value3) { + return ImmutableList.of( + Maps.immutableEntry(key1, value1), + Maps.immutableEntry(key2, value2), + Maps.immutableEntry(key3, value3)); + } } diff --git a/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java index fcdfea1654e8b1..dbe71a4822ba0f 100644 --- a/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java @@ -47,6 +47,7 @@ import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; +import com.google.devtools.build.lib.worker.WorkerPool.WorkerPoolConfig; import com.google.devtools.build.lib.worker.WorkerProtocol.WorkRequest; import com.google.devtools.build.lib.worker.WorkerProtocol.WorkResponse; import java.io.IOException; @@ -85,20 +86,21 @@ public void setUp() { private WorkerPool createWorkerPool() { return new WorkerPool( - new WorkerFactory(options, fs.getPath("/workerBase")) { - @Override - public Worker create(WorkerKey key) { - return worker; - } + new WorkerPoolConfig( + new WorkerFactory(fs.getPath("/workerBase"), options.workerSandboxing) { + @Override + public Worker create(WorkerKey key) { + return worker; + } - @Override - public boolean validateObject(WorkerKey key, PooledObject p) { - return true; - } - }, - ImmutableMap.of(), - ImmutableMap.of(), - ImmutableList.of()); + @Override + public boolean validateObject(WorkerKey key, PooledObject p) { + return true; + } + }, + ImmutableList.of(), + ImmutableList.of(), + ImmutableList.of())); } @Test