From b5d6c38535c7f6f1eab3fd4c8d3d2da91d0b0f8a Mon Sep 17 00:00:00 2001 From: larsrc Date: Mon, 8 Mar 2021 01:05:50 -0800 Subject: [PATCH] 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 --- .../build/lib/worker/WorkerFactory.java | 21 +++----- .../devtools/build/lib/worker/WorkerKey.java | 13 ++++- .../lib/worker/WorkerMultiplexerManager.java | 2 +- .../devtools/build/lib/worker/WorkerPool.java | 2 +- .../build/lib/worker/WorkerSpawnRunner.java | 7 +-- .../build/lib/worker/WorkerKeyTest.java | 16 ++++-- .../lib/worker/WorkerSpawnRunnerTest.java | 49 +++++++++++++++++++ 7 files changed, 84 insertions(+), 26 deletions(-) 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 337b18b84446ec..5088d50071c219 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 @@ public void setOptions(WorkerOptions workerOptions) { @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 @@ public Worker create(WorkerKey key) { 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 @@ public void destroyObject(WorkerKey key, PooledObject p) throws Exceptio 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 @@ public boolean validateObject(WorkerKey key, PooledObject p) { 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 @@ public boolean validateObject(WorkerKey key, PooledObject p) { 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 @@ public boolean validateObject(WorkerKey key, PooledObject p) { 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 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 5cdfee38078b03..c4741d6ed85f4a 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 @@ public boolean getProxied() { 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 0a58cd539f7f81..ed4353ed9ff88f 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 @@ public static synchronized void removeInstance(WorkerKey key) throws UserExecExc 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 9669af814a2dcd..abfa98c7ddf170 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 static WorkerPoolConfig makeConfig(int max) { } 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 1c43cbcab43ffe..d7f1cc99308726 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 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) 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 @@ private WorkRequest createWorkRequest( requestBuilder.addInputsBuilder().setPath(input.getExecPathString()).setDigest(digest); } - if (key.getProxied()) { + if (key.isMultiplex()) { requestBuilder.setRequestId(requestIdCounter.getAndIncrement()); } return requestBuilder.build(); @@ -420,7 +421,7 @@ WorkResponse execInWorker( // 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 656c4a59784a23..0a078b63a6fe16 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 @@ public class WorkerKeyTest { @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 d81f3a65ecab6e..adf995c45424af 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 @@ public void testExecInWorker_happyPath() throws ExecException, InterruptedExcept 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)