Skip to content

Commit

Permalink
[7.4.0] Do no crash if a requested resource is not available (#23911)
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

Closes #23541.

PiperOrigin-RevId: 682282327
Change-Id: Ifdff5f85de9e45ac119c2a2cfd161c054a722546 
(cherry picked from commit e5ab94b)

Closes #23868

Co-authored-by: Alessandro Patti <ale812@yahoo.it>
  • Loading branch information
fmeum and AlessandroPatti authored Oct 9, 2024
1 parent fadee7a commit c2c4a39
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 14 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 @@ -488,6 +488,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/worker",
"//src/main/java/com/google/devtools/build/lib/worker:worker_key",
"//src/main/java/com/google/devtools/build/lib/worker:worker_pool",
"//src/main/protobuf:failure_details_java_proto",
"//third_party:guava",
"//third_party:jsr305",
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.profiler.AutoProfiler;
import com.google.devtools.build.lib.profiler.ProfilerTask;
import com.google.devtools.build.lib.server.FailureDetails;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.worker.Worker;
import com.google.devtools.build.lib.worker.WorkerKey;
Expand Down Expand Up @@ -236,14 +237,17 @@ public void setWorkerPool(WorkerPool workerPool) {
*/
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);

LatchWithWorker latchWithWorker = null;

// Validate requested resources exist before attempting to acquire.
assertResourcesTracked(resources);

AutoProfiler p =
profiled("Acquiring resources for: " + owner.describe(), ProfilerTask.ACTION_LOCK);
try {
Expand Down Expand Up @@ -468,12 +472,24 @@ private synchronized void processWaitingThreads(Deque<Pair<ResourceSet, LatchWit
}

/** 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 @@ -507,10 +523,6 @@ boolean areResourcesAvailable(ResourceSet resources) throws NoSuchElementExcepti
}
}

// 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 @@ -718,6 +718,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 @@ -99,17 +99,17 @@ public boolean validateObject(WorkerKey key, PooledObject<Worker> p) {
}

private ResourceHandle acquire(double ram, double cpu, int tests, ResourcePriority priority)
throws InterruptedException, IOException {
throws InterruptedException, IOException, ExecException {
return rm.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 rm.acquireResources(
resourceOwner,
Expand All @@ -127,7 +127,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 rm.acquireResources(
Expand All @@ -137,7 +137,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 @@ -676,7 +676,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 Down

0 comments on commit c2c4a39

Please sign in to comment.