Skip to content

Commit

Permalink
Change short output of worker type to have the same logic as the work…
Browse files Browse the repository at this point in the history
…er creation for sandboxing vs. multiplex.

This also clears up some messy handling of the multiplex/dynamic handling.

RELNOTES: None.
PiperOrigin-RevId: 361506276
  • Loading branch information
larsrc-google authored and copybara-github committed Mar 8, 2021
1 parent 7242b0e commit b5d6c38
Show file tree
Hide file tree
Showing 7 changed files with 84 additions and 26 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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");

Expand Down Expand Up @@ -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);
}

Expand All @@ -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();
}
Expand All @@ -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 =
Expand All @@ -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));
}
}
Expand All @@ -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());
Expand Down
13 changes: 11 additions & 2 deletions src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(""));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()),
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit b5d6c38

Please sign in to comment.