Skip to content

Commit

Permalink
Do no crash if a requested resource is not available
Browse files Browse the repository at this point in the history
Before:
```
Caused by: java.util.NoSuchElementException: Resource foo is not tracked in this resource set.
    at com.google.devtools.build.lib.actions.ResourceManager.assertExtraResourcesTracked(ResourceManager.java:499)
    at com.google.devtools.build.lib.actions.ResourceManager.areResourcesAvailable(ResourceManager.java:540)
```
After:
```
ERROR: src/test/java/com/google/devtools/build/lib/actions/BUILD:102:10: Testing //src/test/java/com/google/devtools/build/lib/actions:ActionsTests failed: Resource foo is not being tracked by the resource manager. Available resources are: cpu, memory
```

Fixes #19572
  • Loading branch information
AlessandroPatti committed Sep 6, 2024
1 parent 50b7dd2 commit 9cbe77b
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 15 deletions.
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/actions/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/worker:worker_key",
"//src/main/java/com/google/devtools/build/lib/worker:worker_pool",
"//src/main/java/com/google/devtools/build/lib/worker:worker_process_status",
"//src/main/protobuf:failure_details_java_proto",
"//third_party:auto_value",
"//third_party:error_prone_annotations",
"//third_party:flogger",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.google.devtools.build.lib.profiler.AutoProfiler;
import com.google.devtools.build.lib.profiler.Profiler;
import com.google.devtools.build.lib.profiler.ProfilerTask;
import com.google.devtools.build.lib.server.FailureDetails;
import com.google.devtools.build.lib.worker.Worker;
import com.google.devtools.build.lib.worker.WorkerKey;
import com.google.devtools.build.lib.worker.WorkerPool;
Expand Down Expand Up @@ -368,14 +369,16 @@ record ResourceRequest(
*/
public ResourceHandle acquireResources(
ActionExecutionMetadata owner, ResourceSet resources, ResourcePriority priority)
throws InterruptedException, IOException {
throws InterruptedException, IOException, ExecException {
Preconditions.checkNotNull(
resources, "acquireResources called with resources == NULL during %s", owner);
Preconditions.checkState(
!threadHasResources(), "acquireResources with existing resource lock during %s", owner);

ResourceLatch resourceLatch = null;

// Validate requested resources exist before creating a request.
assertResourcesTracked(resources);
ResourceRequest request =
new ResourceRequest(owner, resources, priority, requestIdGenerator.getAndIncrement());

Expand Down Expand Up @@ -590,12 +593,26 @@ private synchronized void processWaitingRequests(Deque<WaitingRequest> requests)
}

/** Throws an exception if requested extra resource isn't being tracked */
private void assertResourcesTracked(ResourceSet resources) throws NoSuchElementException {
private void assertResourcesTracked(ResourceSet resources) throws ExecException {
for (Map.Entry<String, Double> resource : resources.getResources().entrySet()) {
String key = resource.getKey();
if (!availableResources.getResources().containsKey(key)) {
throw new NoSuchElementException(
"Resource " + key + " is not tracked in this resource set.");
StringBuilder message = new StringBuilder();
message.append("Resource ");
message.append(key);
message.append(" is not being tracked by the resource manager.");
message.append(" Available resources are: ");
message.append(String.join(", ", availableResources.getResources().keySet()));
throw new UserExecException(
FailureDetails.FailureDetail.newBuilder()
.setMessage(message.toString())
.setLocalExecution(
FailureDetails.LocalExecution
.newBuilder()
.setCode(FailureDetails.LocalExecution.Code.UNTRACKED_RESOURCE)
.build())
.build()
);
}
}
}
Expand Down Expand Up @@ -625,10 +642,6 @@ synchronized boolean areResourcesAvailable(ResourceSet resources) {
return false;
}

// We test for tracking of extra resources whenever acquired and throw an
// exception before acquiring any untracked resource.
assertResourcesTracked(resources);

if (usedResources.isEmpty() && usedLocalTestCount == 0) {
return true;
}
Expand Down
1 change: 1 addition & 0 deletions src/main/protobuf/failure_details.proto
Original file line number Diff line number Diff line change
Expand Up @@ -750,6 +750,7 @@ message LocalExecution {
enum Code {
LOCAL_EXECUTION_UNKNOWN = 0 [(metadata) = { exit_code: 37 }];
LOCKFREE_OUTPUT_PREREQ_UNMET = 1 [(metadata) = { exit_code: 2 }];
UNTRACKED_RESOURCE = 2 [(metadata) = { exit_code: 1 }];
}

Code code = 1;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,17 +95,17 @@ public void configureResourceManager() throws Exception {
}

private ResourceHandle acquire(double ram, double cpu, int tests, ResourcePriority priority)
throws InterruptedException, IOException {
throws InterruptedException, IOException, ExecException {
return manager.acquireResources(resourceOwner, ResourceSet.create(ram, cpu, tests), priority);
}

private ResourceHandle acquire(double ram, double cpu, int tests)
throws InterruptedException, IOException {
throws InterruptedException, IOException, ExecException {
return acquire(ram, cpu, tests, ResourcePriority.LOCAL);
}

private ResourceHandle acquire(double ram, double cpu, int tests, String mnemonic)
throws InterruptedException, IOException {
throws InterruptedException, IOException, ExecException {

return manager.acquireResources(
resourceOwner,
Expand All @@ -123,7 +123,7 @@ private ResourceHandle acquire(
ImmutableMap<String, Double> extraResources,
int tests,
ResourcePriority priority)
throws InterruptedException, IOException, NoSuchElementException {
throws InterruptedException, IOException, NoSuchElementException, ExecException {
ImmutableMap.Builder<String, Double> resources = ImmutableMap.builder();
resources.putAll(extraResources).put(ResourceSet.MEMORY, ram).put(ResourceSet.CPU, cpu);
return manager.acquireResources(
Expand All @@ -133,7 +133,7 @@ private ResourceHandle acquire(
@CanIgnoreReturnValue
private ResourceHandle acquire(
double ram, double cpu, ImmutableMap<String, Double> extraResources, int tests)
throws InterruptedException, IOException, NoSuchElementException {
throws InterruptedException, IOException, NoSuchElementException, ExecException {
return acquire(ram, cpu, extraResources, tests, ResourcePriority.LOCAL);
}

Expand Down Expand Up @@ -665,7 +665,7 @@ public void testNonexistingResource() throws Exception {
new TestThread(
() ->
assertThrows(
NoSuchElementException.class,
UserExecException.class,
() -> acquire(0, 0, ImmutableMap.of("nonexisting", 1.0), 0)));
thread1.start();
thread1.joinAndAssertState(1000);
Expand All @@ -688,7 +688,7 @@ public void testAcquireWithWorker_acquireAndRelease() throws Exception {
}

@Test
public void testInvalidateAndClose() throws IOException, InterruptedException {
public void testInvalidateAndClose() throws IOException, InterruptedException, ExecException {
ResourceHandle handle;
verify(workerStatus, times(0)).maybeUpdateStatus(any());

Expand Down

0 comments on commit 9cbe77b

Please sign in to comment.