Skip to content

Commit b5d6c38

Browse files
larsrc-googlecopybara-github
larsrc-google
authored and
copybara-github
committed
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
1 parent 7242b0e commit b5d6c38

File tree

7 files changed

+84
-26
lines changed

7 files changed

+84
-26
lines changed

‎src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java‎

Lines changed: 6 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ public void setOptions(WorkerOptions workerOptions) {
5555
@Override
5656
public Worker create(WorkerKey key) {
5757
int workerId = pidCounter.getAndIncrement();
58-
String workTypeName = WorkerKey.makeWorkerTypeName(key.getProxied());
58+
String workTypeName = key.getWorkerTypeName();
5959
Path logFile =
6060
workerBaseDir.getRelative(workTypeName + "-" + workerId + "-" + key.getMnemonic() + ".log");
6161

@@ -87,12 +87,7 @@ public Worker create(WorkerKey key) {
8787
Path getSandboxedWorkerPath(WorkerKey key, int workerId) {
8888
String workspaceName = key.getExecRoot().getBaseName();
8989
return workerBaseDir
90-
.getRelative(
91-
WorkerKey.makeWorkerTypeName(key.getProxied())
92-
+ "-"
93-
+ workerId
94-
+ "-"
95-
+ key.getMnemonic())
90+
.getRelative(key.getWorkerTypeName() + "-" + workerId + "-" + key.getMnemonic())
9691
.getRelative(workspaceName);
9792
}
9893

@@ -115,7 +110,7 @@ public void destroyObject(WorkerKey key, PooledObject<Worker> p) throws Exceptio
115110
Event.info(
116111
String.format(
117112
"Destroying %s %s (id %d)",
118-
key.getMnemonic(), WorkerKey.makeWorkerTypeName(key.getProxied()), workerId)));
113+
key.getMnemonic(), key.getWorkerTypeName(), workerId)));
119114
}
120115
p.getObject().destroy();
121116
}
@@ -135,7 +130,7 @@ public boolean validateObject(WorkerKey key, PooledObject<Worker> p) {
135130
String.format(
136131
"%s %s (id %d) has unexpectedly died with exit code %d.",
137132
key.getMnemonic(),
138-
WorkerKey.makeWorkerTypeName(key.getProxied()),
133+
key.getWorkerTypeName(),
139134
worker.getWorkerId(),
140135
exitValue.get());
141136
ErrorMessage errorMessage =
@@ -150,9 +145,7 @@ public boolean validateObject(WorkerKey key, PooledObject<Worker> p) {
150145
String msg =
151146
String.format(
152147
"%s %s (id %d) was destroyed, but is still in the worker pool.",
153-
key.getMnemonic(),
154-
WorkerKey.makeWorkerTypeName(key.getProxied()),
155-
worker.getWorkerId());
148+
key.getMnemonic(), key.getWorkerTypeName(), worker.getWorkerId());
156149
reporter.handle(Event.info(msg));
157150
}
158151
}
@@ -166,9 +159,7 @@ public boolean validateObject(WorkerKey key, PooledObject<Worker> p) {
166159
msg.append(
167160
String.format(
168161
"%s %s (id %d) can no longer be used, because its files have changed on disk:",
169-
key.getMnemonic(),
170-
WorkerKey.makeWorkerTypeName(key.getProxied()),
171-
worker.getWorkerId()));
162+
key.getMnemonic(), key.getWorkerTypeName(), worker.getWorkerId()));
172163
TreeSet<PathFragment> files = new TreeSet<>();
173164
files.addAll(key.getWorkerFilesWithHashes().keySet());
174165
files.addAll(worker.getWorkerFilesWithHashes().keySet());

‎src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -127,20 +127,29 @@ public boolean getProxied() {
127127
return proxied;
128128
}
129129

130+
public boolean isMultiplex() {
131+
return getProxied() && !mustBeSandboxed;
132+
}
133+
130134
/** Returns the format of the worker protocol. */
131135
public WorkerProtocolFormat getProtocolFormat() {
132136
return protocolFormat;
133137
}
134138

135139
/** Returns a user-friendly name for this worker type. */
136-
public static String makeWorkerTypeName(boolean proxied) {
137-
if (proxied) {
140+
public static String makeWorkerTypeName(boolean proxied, boolean mustBeSandboxed) {
141+
if (proxied && !mustBeSandboxed) {
138142
return "multiplex-worker";
139143
} else {
140144
return "worker";
141145
}
142146
}
143147

148+
/** Returns a user-friendly name for this worker type. */
149+
public String getWorkerTypeName() {
150+
return makeWorkerTypeName(proxied, mustBeSandboxed);
151+
}
152+
144153
@Override
145154
public boolean equals(Object o) {
146155
if (this == o) {

‎src/main/java/com/google/devtools/build/lib/worker/WorkerMultiplexerManager.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ public static synchronized void removeInstance(WorkerKey key) throws UserExecExc
7575
InstanceInfo instanceInfo = multiplexerInstance.get(key);
7676
if (instanceInfo == null) {
7777
throw createUserExecException(
78-
"Attempting to remove non-existent multiplexer instance.",
78+
String.format("Attempting to remove non-existent multiplexer instance for %s.", key),
7979
Code.MULTIPLEXER_INSTANCE_REMOVAL_FAILURE);
8080
}
8181
instanceInfo.decreaseRefCount();

‎src/main/java/com/google/devtools/build/lib/worker/WorkerPool.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ private static WorkerPoolConfig makeConfig(int max) {
110110
}
111111

112112
private SimpleWorkerPool getPool(WorkerKey key) {
113-
if (key.getProxied()) {
113+
if (key.isMultiplex()) {
114114
return multiplexPools.getOrDefault(key.getMnemonic(), multiplexPools.get(""));
115115
} else {
116116
return workerPools.getOrDefault(key.getMnemonic(), workerPools.get(""));

‎src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,8 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
144144
throws ExecException, IOException, InterruptedException {
145145
context.report(
146146
ProgressStatus.SCHEDULING,
147-
WorkerKey.makeWorkerTypeName(Spawns.supportsMultiplexWorkers(spawn)));
147+
WorkerKey.makeWorkerTypeName(
148+
Spawns.supportsMultiplexWorkers(spawn), context.speculating()));
148149
if (spawn.getToolFiles().isEmpty()) {
149150
throw createUserExecException(
150151
String.format(ERROR_MESSAGE_PREFIX + REASON_NO_TOOLS, spawn.getMnemonic()),
@@ -301,7 +302,7 @@ private WorkRequest createWorkRequest(
301302

302303
requestBuilder.addInputsBuilder().setPath(input.getExecPathString()).setDigest(digest);
303304
}
304-
if (key.getProxied()) {
305+
if (key.isMultiplex()) {
305306
requestBuilder.setRequestId(requestIdCounter.getAndIncrement());
306307
}
307308
return requestBuilder.build();
@@ -420,7 +421,7 @@ WorkResponse execInWorker(
420421
// We acquired a worker and resources -- mark that as queuing time.
421422
spawnMetrics.setQueueTime(queueStopwatch.elapsed());
422423

423-
context.report(ProgressStatus.EXECUTING, WorkerKey.makeWorkerTypeName(key.getProxied()));
424+
context.report(ProgressStatus.EXECUTING, key.getWorkerTypeName());
424425
try {
425426
// We consider `prepareExecution` to be also part of setup.
426427
Stopwatch prepareExecutionStopwatch = Stopwatch.createStarted();

‎src/test/java/com/google/devtools/build/lib/worker/WorkerKeyTest.java‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -49,10 +49,18 @@ public class WorkerKeyTest {
4949

5050
@Test
5151
public void testWorkerKeyGetter() {
52-
assertThat(workerKey.mustBeSandboxed()).isEqualTo(true);
53-
assertThat(workerKey.getProxied()).isEqualTo(true);
54-
assertThat(WorkerKey.makeWorkerTypeName(false)).isEqualTo("worker");
55-
assertThat(WorkerKey.makeWorkerTypeName(true)).isEqualTo("multiplex-worker");
52+
assertThat(workerKey.mustBeSandboxed()).isTrue();
53+
assertThat(workerKey.getProxied()).isTrue();
54+
assertThat(workerKey.isMultiplex()).isFalse();
55+
assertThat(workerKey.getWorkerTypeName()).isEqualTo("worker");
56+
assertThat(WorkerKey.makeWorkerTypeName(/* proxied=*/ false, /* mustBeSandboxed=*/ false))
57+
.isEqualTo("worker");
58+
assertThat(WorkerKey.makeWorkerTypeName(/* proxied=*/ false, /* mustBeSandboxed=*/ true))
59+
.isEqualTo("worker");
60+
assertThat(WorkerKey.makeWorkerTypeName(/* proxied=*/ true, /* mustBeSandboxed=*/ false))
61+
.isEqualTo("multiplex-worker");
62+
assertThat(WorkerKey.makeWorkerTypeName(/* proxied=*/ true, /* mustBeSandboxed=*/ true))
63+
.isEqualTo("worker");
5664
// Hash code contains args, env, execRoot, proxied, and mnemonic.
5765
assertThat(workerKey.hashCode()).isEqualTo(1605714200);
5866
assertThat(workerKey.getProtocolFormat()).isEqualTo(WorkerProtocolFormat.PROTO);

‎src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,15 @@
1717
import static com.google.common.truth.Truth.assertThat;
1818
import static com.google.devtools.build.lib.worker.TestUtils.createWorkerKey;
1919
import static org.junit.Assert.assertThrows;
20+
import static org.mockito.Mockito.times;
21+
import static org.mockito.Mockito.verify;
2022
import static org.mockito.Mockito.when;
2123

2224
import com.google.common.collect.ImmutableList;
2325
import com.google.common.collect.ImmutableMap;
2426
import com.google.common.collect.ImmutableSet;
2527
import com.google.devtools.build.lib.actions.ExecException;
28+
import com.google.devtools.build.lib.actions.ExecutionRequirements.WorkerProtocolFormat;
2629
import com.google.devtools.build.lib.actions.MetadataProvider;
2730
import com.google.devtools.build.lib.actions.ResourceManager;
2831
import com.google.devtools.build.lib.actions.Spawn;
@@ -31,6 +34,7 @@
3134
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
3235
import com.google.devtools.build.lib.collect.nestedset.Order;
3336
import com.google.devtools.build.lib.events.ExtendedEventHandler;
37+
import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus;
3438
import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext;
3539
import com.google.devtools.build.lib.exec.local.LocalEnvProvider;
3640
import com.google.devtools.build.lib.sandbox.SandboxHelpers;
@@ -127,6 +131,51 @@ public void testExecInWorker_happyPath() throws ExecException, InterruptedExcept
127131
assertThat(response.getRequestId()).isEqualTo(0);
128132
assertThat(response.getOutput()).isEqualTo("out");
129133
assertThat(logFile.exists()).isFalse();
134+
verify(context, times(1)).report(ProgressStatus.EXECUTING, "worker");
135+
}
136+
137+
@Test
138+
public void testExecInWorker_noMultiplexWithDynamic()
139+
throws ExecException, InterruptedException, IOException {
140+
WorkerOptions workerOptions = new WorkerOptions();
141+
workerOptions.workerMultiplex = true;
142+
when(context.speculating()).thenReturn(true);
143+
WorkerSpawnRunner runner =
144+
new WorkerSpawnRunner(
145+
new SandboxHelpers(false),
146+
fs.getPath("/execRoot"),
147+
createWorkerPool(),
148+
/* multiplex */ true,
149+
reporter,
150+
localEnvProvider,
151+
/* binTools */ null,
152+
resourceManager,
153+
/* runfilestTreeUpdater */ null,
154+
workerOptions);
155+
// This worker key just so happens to be multiplex and require sandboxing.
156+
WorkerKey key = createWorkerKey(WorkerProtocolFormat.JSON, fs);
157+
Path logFile = fs.getPath("/worker.log");
158+
when(worker.getLogFile()).thenReturn(logFile);
159+
when(worker.getResponse(0))
160+
.thenReturn(
161+
WorkResponse.newBuilder().setExitCode(0).setRequestId(0).setOutput("out").build());
162+
WorkResponse response =
163+
runner.execInWorker(
164+
spawn,
165+
key,
166+
context,
167+
new SandboxInputs(ImmutableMap.of(), ImmutableSet.of(), ImmutableMap.of()),
168+
SandboxOutputs.create(ImmutableSet.of(), ImmutableSet.of()),
169+
ImmutableList.of(),
170+
inputFileCache,
171+
spawnMetrics);
172+
173+
assertThat(response).isNotNull();
174+
assertThat(response.getExitCode()).isEqualTo(0);
175+
assertThat(response.getRequestId()).isEqualTo(0);
176+
assertThat(response.getOutput()).isEqualTo("out");
177+
assertThat(logFile.exists()).isFalse();
178+
verify(context, times(1)).report(ProgressStatus.EXECUTING, "worker");
130179
}
131180

132181
private void assertRecordedResponsethrowsException(String recordedResponse, String exceptionText)

0 commit comments

Comments
 (0)