Change short output of worker type to have the same logic as the worker creation for sandboxing vs. multiplex.
This also clears up some messy handling of the multiplex/dynamic handling.
RELNOTES: None.
PiperOrigin-RevId: 361506276
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 337b18b..5088d50 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
@@ -55,7 +55,7 @@
@Override
public Worker create(WorkerKey key) {
int workerId = pidCounter.getAndIncrement();
- String workTypeName = WorkerKey.makeWorkerTypeName(key.getProxied());
+ String workTypeName = key.getWorkerTypeName();
Path logFile =
workerBaseDir.getRelative(workTypeName + "-" + workerId + "-" + key.getMnemonic() + ".log");
@@ -87,12 +87,7 @@
Path getSandboxedWorkerPath(WorkerKey key, int workerId) {
String workspaceName = key.getExecRoot().getBaseName();
return workerBaseDir
- .getRelative(
- WorkerKey.makeWorkerTypeName(key.getProxied())
- + "-"
- + workerId
- + "-"
- + key.getMnemonic())
+ .getRelative(key.getWorkerTypeName() + "-" + workerId + "-" + key.getMnemonic())
.getRelative(workspaceName);
}
@@ -115,7 +110,7 @@
Event.info(
String.format(
"Destroying %s %s (id %d)",
- key.getMnemonic(), WorkerKey.makeWorkerTypeName(key.getProxied()), workerId)));
+ key.getMnemonic(), key.getWorkerTypeName(), workerId)));
}
p.getObject().destroy();
}
@@ -135,7 +130,7 @@
String.format(
"%s %s (id %d) has unexpectedly died with exit code %d.",
key.getMnemonic(),
- WorkerKey.makeWorkerTypeName(key.getProxied()),
+ key.getWorkerTypeName(),
worker.getWorkerId(),
exitValue.get());
ErrorMessage errorMessage =
@@ -150,9 +145,7 @@
String msg =
String.format(
"%s %s (id %d) was destroyed, but is still in the worker pool.",
- key.getMnemonic(),
- WorkerKey.makeWorkerTypeName(key.getProxied()),
- worker.getWorkerId());
+ key.getMnemonic(), key.getWorkerTypeName(), worker.getWorkerId());
reporter.handle(Event.info(msg));
}
}
@@ -166,9 +159,7 @@
msg.append(
String.format(
"%s %s (id %d) can no longer be used, because its files have changed on disk:",
- key.getMnemonic(),
- WorkerKey.makeWorkerTypeName(key.getProxied()),
- worker.getWorkerId()));
+ key.getMnemonic(), key.getWorkerTypeName(), worker.getWorkerId()));
TreeSet<PathFragment> files = new TreeSet<>();
files.addAll(key.getWorkerFilesWithHashes().keySet());
files.addAll(worker.getWorkerFilesWithHashes().keySet());
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java
index 5cdfee3..c4741d6 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java
@@ -127,20 +127,29 @@
return proxied;
}
+ public boolean isMultiplex() {
+ return getProxied() && !mustBeSandboxed;
+ }
+
/** Returns the format of the worker protocol. */
public WorkerProtocolFormat getProtocolFormat() {
return protocolFormat;
}
/** Returns a user-friendly name for this worker type. */
- public static String makeWorkerTypeName(boolean proxied) {
- if (proxied) {
+ public static String makeWorkerTypeName(boolean proxied, boolean mustBeSandboxed) {
+ if (proxied && !mustBeSandboxed) {
return "multiplex-worker";
} else {
return "worker";
}
}
+ /** Returns a user-friendly name for this worker type. */
+ public String getWorkerTypeName() {
+ return makeWorkerTypeName(proxied, mustBeSandboxed);
+ }
+
@Override
public boolean equals(Object o) {
if (this == o) {
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerMultiplexerManager.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerMultiplexerManager.java
index 0a58cd5..ed4353e 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerMultiplexerManager.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerMultiplexerManager.java
@@ -75,7 +75,7 @@
InstanceInfo instanceInfo = multiplexerInstance.get(key);
if (instanceInfo == null) {
throw createUserExecException(
- "Attempting to remove non-existent multiplexer instance.",
+ String.format("Attempting to remove non-existent multiplexer instance for %s.", key),
Code.MULTIPLEXER_INSTANCE_REMOVAL_FAILURE);
}
instanceInfo.decreaseRefCount();
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 9669af8..abfa98c 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
@@ -110,7 +110,7 @@
}
private SimpleWorkerPool getPool(WorkerKey key) {
- if (key.getProxied()) {
+ if (key.isMultiplex()) {
return multiplexPools.getOrDefault(key.getMnemonic(), multiplexPools.get(""));
} else {
return workerPools.getOrDefault(key.getMnemonic(), workerPools.get(""));
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java
index 1c43cbc..d7f1cc9 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java
@@ -144,7 +144,8 @@
throws ExecException, IOException, InterruptedException {
context.report(
ProgressStatus.SCHEDULING,
- WorkerKey.makeWorkerTypeName(Spawns.supportsMultiplexWorkers(spawn)));
+ WorkerKey.makeWorkerTypeName(
+ Spawns.supportsMultiplexWorkers(spawn), context.speculating()));
if (spawn.getToolFiles().isEmpty()) {
throw createUserExecException(
String.format(ERROR_MESSAGE_PREFIX + REASON_NO_TOOLS, spawn.getMnemonic()),
@@ -301,7 +302,7 @@
requestBuilder.addInputsBuilder().setPath(input.getExecPathString()).setDigest(digest);
}
- if (key.getProxied()) {
+ if (key.isMultiplex()) {
requestBuilder.setRequestId(requestIdCounter.getAndIncrement());
}
return requestBuilder.build();
@@ -420,7 +421,7 @@
// We acquired a worker and resources -- mark that as queuing time.
spawnMetrics.setQueueTime(queueStopwatch.elapsed());
- context.report(ProgressStatus.EXECUTING, WorkerKey.makeWorkerTypeName(key.getProxied()));
+ context.report(ProgressStatus.EXECUTING, key.getWorkerTypeName());
try {
// We consider `prepareExecution` to be also part of setup.
Stopwatch prepareExecutionStopwatch = Stopwatch.createStarted();
diff --git a/src/test/java/com/google/devtools/build/lib/worker/WorkerKeyTest.java b/src/test/java/com/google/devtools/build/lib/worker/WorkerKeyTest.java
index 656c4a5..0a078b6 100644
--- a/src/test/java/com/google/devtools/build/lib/worker/WorkerKeyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/worker/WorkerKeyTest.java
@@ -49,10 +49,18 @@
@Test
public void testWorkerKeyGetter() {
- assertThat(workerKey.mustBeSandboxed()).isEqualTo(true);
- assertThat(workerKey.getProxied()).isEqualTo(true);
- assertThat(WorkerKey.makeWorkerTypeName(false)).isEqualTo("worker");
- assertThat(WorkerKey.makeWorkerTypeName(true)).isEqualTo("multiplex-worker");
+ assertThat(workerKey.mustBeSandboxed()).isTrue();
+ assertThat(workerKey.getProxied()).isTrue();
+ assertThat(workerKey.isMultiplex()).isFalse();
+ assertThat(workerKey.getWorkerTypeName()).isEqualTo("worker");
+ assertThat(WorkerKey.makeWorkerTypeName(/* proxied=*/ false, /* mustBeSandboxed=*/ false))
+ .isEqualTo("worker");
+ assertThat(WorkerKey.makeWorkerTypeName(/* proxied=*/ false, /* mustBeSandboxed=*/ true))
+ .isEqualTo("worker");
+ assertThat(WorkerKey.makeWorkerTypeName(/* proxied=*/ true, /* mustBeSandboxed=*/ false))
+ .isEqualTo("multiplex-worker");
+ assertThat(WorkerKey.makeWorkerTypeName(/* proxied=*/ true, /* mustBeSandboxed=*/ true))
+ .isEqualTo("worker");
// Hash code contains args, env, execRoot, proxied, and mnemonic.
assertThat(workerKey.hashCode()).isEqualTo(1605714200);
assertThat(workerKey.getProtocolFormat()).isEqualTo(WorkerProtocolFormat.PROTO);
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 d81f3a6..adf995c 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
@@ -17,12 +17,15 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.devtools.build.lib.worker.TestUtils.createWorkerKey;
import static org.junit.Assert.assertThrows;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.actions.ExecException;
+import com.google.devtools.build.lib.actions.ExecutionRequirements.WorkerProtocolFormat;
import com.google.devtools.build.lib.actions.MetadataProvider;
import com.google.devtools.build.lib.actions.ResourceManager;
import com.google.devtools.build.lib.actions.Spawn;
@@ -31,6 +34,7 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
+import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus;
import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext;
import com.google.devtools.build.lib.exec.local.LocalEnvProvider;
import com.google.devtools.build.lib.sandbox.SandboxHelpers;
@@ -127,6 +131,51 @@
assertThat(response.getRequestId()).isEqualTo(0);
assertThat(response.getOutput()).isEqualTo("out");
assertThat(logFile.exists()).isFalse();
+ verify(context, times(1)).report(ProgressStatus.EXECUTING, "worker");
+ }
+
+ @Test
+ public void testExecInWorker_noMultiplexWithDynamic()
+ throws ExecException, InterruptedException, IOException {
+ WorkerOptions workerOptions = new WorkerOptions();
+ workerOptions.workerMultiplex = true;
+ when(context.speculating()).thenReturn(true);
+ WorkerSpawnRunner runner =
+ new WorkerSpawnRunner(
+ new SandboxHelpers(false),
+ fs.getPath("/execRoot"),
+ createWorkerPool(),
+ /* multiplex */ true,
+ reporter,
+ localEnvProvider,
+ /* binTools */ null,
+ resourceManager,
+ /* runfilestTreeUpdater */ null,
+ workerOptions);
+ // This worker key just so happens to be multiplex and require sandboxing.
+ WorkerKey key = createWorkerKey(WorkerProtocolFormat.JSON, fs);
+ Path logFile = fs.getPath("/worker.log");
+ when(worker.getLogFile()).thenReturn(logFile);
+ when(worker.getResponse(0))
+ .thenReturn(
+ WorkResponse.newBuilder().setExitCode(0).setRequestId(0).setOutput("out").build());
+ WorkResponse response =
+ runner.execInWorker(
+ spawn,
+ key,
+ context,
+ new SandboxInputs(ImmutableMap.of(), ImmutableSet.of(), ImmutableMap.of()),
+ SandboxOutputs.create(ImmutableSet.of(), ImmutableSet.of()),
+ ImmutableList.of(),
+ inputFileCache,
+ spawnMetrics);
+
+ assertThat(response).isNotNull();
+ assertThat(response.getExitCode()).isEqualTo(0);
+ assertThat(response.getRequestId()).isEqualTo(0);
+ assertThat(response.getOutput()).isEqualTo("out");
+ assertThat(logFile.exists()).isFalse();
+ verify(context, times(1)).report(ProgressStatus.EXECUTING, "worker");
}
private void assertRecordedResponsethrowsException(String recordedResponse, String exceptionText)
