Change short output of worker type to have the same logic as the worker creation for sandboxing vs. multiplex.

larsrc-google · copybara-github · commit b5d6c38535c7 · 2021-03-08T01:06:47.000-08:00
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
@@ -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<Worker> 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<Worker> 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<Worker> 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<Worker> 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<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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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)

Original file line number	Diff line number	Diff line change
`@@ -75,7 +75,7 @@ public static synchronized void removeInstance(WorkerKey key) throws UserExecExc`
`75`	`75`	`InstanceInfo instanceInfo = multiplexerInstance.get(key);`
`76`	`76`	`if (instanceInfo == null) {`
`77`	`77`	`throw createUserExecException(`
`78`		`- "Attempting to remove non-existent multiplexer instance.",`
	`78`	`+ String.format("Attempting to remove non-existent multiplexer instance for %s.", key),`
`79`	`79`	`Code.MULTIPLEXER_INSTANCE_REMOVAL_FAILURE);`
`80`	`80`	`}`
`81`	`81`	`instanceInfo.decreaseRefCount();`
Original file line number	Diff line number	Diff line change
`@@ -110,7 +110,7 @@ private static WorkerPoolConfig makeConfig(int max) {`
`110`	`110`	`}`
`111`	`111`
`112`	`112`	`private SimpleWorkerPool getPool(WorkerKey key) {`
`113`		`- if (key.getProxied()) {`
	`113`	`+ if (key.isMultiplex()) {`
`114`	`114`	`return multiplexPools.getOrDefault(key.getMnemonic(), multiplexPools.get(""));`
`115`	`115`	`} else {`
`116`	`116`	`return workerPools.getOrDefault(key.getMnemonic(), workerPools.get(""));`