diff --git a/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java b/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java index 5303f863622deb..2640c2f055f603 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java @@ -157,27 +157,22 @@ public String parseIfMatches(String tag) throws ValidationException { /** How many extra resources an action requires for execution. */ public static final ParseableRequirement RESOURCES = ParseableRequirement.create( - "resources::", + "resources::", Pattern.compile("resources:(.+:.+)"), s -> { Preconditions.checkNotNull(s); int splitIndex = s.indexOf(":"); String resourceCount = s.substring(splitIndex+1); - int value; + float value; try { - value = Integer.parseInt(resourceCount); + value = Float.parseFloat(resourceCount); } catch (NumberFormatException e) { - return "can't be parsed as an integer"; - } - - // De-and-reserialize & compare to only allow canonical integer formats. - if (!Integer.toString(value).equals(resourceCount)) { - return "must be in canonical format (e.g. '4' instead of '+04')"; + return "can't be parsed as a float"; } - if (value < 1) { - return "can't be zero or negative"; + if (value < 0) { + return "can't be negative"; } return null; diff --git a/src/main/java/com/google/devtools/build/lib/actions/ResourceSet.java b/src/main/java/com/google/devtools/build/lib/actions/ResourceSet.java index 3453fb2d8c26ab..2e7b2ab3a5cd9c 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ResourceSet.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ResourceSet.java @@ -58,7 +58,8 @@ public class ResourceSet implements ResourceSetOrBuilder { /** The workerKey of used worker. Null if no worker is used. */ @Nullable private final WorkerKey workerKey; - private ResourceSet(double memoryMb, double cpuUsage, int localTestCount, @Nullable WorkerKey workerKey) { + private ResourceSet( + double memoryMb, double cpuUsage, int localTestCount, @Nullable WorkerKey workerKey) { this(memoryMb, cpuUsage, ImmutableMap.of(), localTestCount, workerKey); } @@ -102,7 +103,7 @@ public static ResourceSet createWithLocalTestCount(int localTestCount) { * represent available resources. */ public static ResourceSet create(double memoryMb, double cpuUsage, int localTestCount) { - return ResourceSet.create(memoryMb, cpuUsage, ImmutableMap.of(), localTestCount, /* wolkerKey= */ null); + return ResourceSet.createWithWorkerKey(memoryMb, cpuUsage, ImmutableMap.of(), localTestCount, /* wolkerKey= */ null); } /** @@ -112,7 +113,11 @@ public static ResourceSet create(double memoryMb, double cpuUsage, int localTest * ResourceSets that represent available resources. */ public static ResourceSet create(double memoryMb, double cpuUsage, ImmutableMap extraResourceUsage, int localTestCount) { - return createWithWorkerKey(memoryMb, cpuUsage, extraResourceUseage, localTestCount, /* workerKey= */ null); + return createWithWorkerKey(memoryMb, cpuUsage, extraResourceUsage, localTestCount, /* workerKey= */ null); + } + + public static ResourceSet createWithWorkerKey(double memoryMb, double cpuUsage, int localTestCount, WorkerKey workerKey) { + return ResourceSet.createWithWorkerKey(memoryMb, cpuUsage, /* extraResourceUsage= */ImmutableMap.of(), localTestCount, workerKey); } public static ResourceSet createWithWorkerKey( diff --git a/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java b/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java index a1cc160c17583c..2058b9fb49f5ed 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java +++ b/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java @@ -334,11 +334,11 @@ public boolean shouldMaterializeParamFiles() { help = "Set the number of extra resources available to Bazel. " + "Takes in a string-float pair. Can be used multiple times to specify multiple " - + "types of extra resources. Bazel will limit concurrently running test actions " - + "based on the available extra resources and the extra resources required " - + "by the test actions. Tests can declare the amount of extra resources they need " + + "types of extra resources. Bazel will limit concurrently running actions " + + "based on the available extra resources and the extra resources required. " + + "Tests can declare the amount of extra resources they need " + "by using a tag of the \"resources::\" format. " - + "Available CPU, RAM and test job resources cannot be set with this flag.", + + "Available CPU, RAM and resources cannot be set with this flag.", converter = Converters.StringToFloatAssignmentConverter.class) public List> localExtraResources; 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 index ac774ff26ff8bb..696b22b570e4be 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java @@ -407,6 +407,7 @@ WorkResponse execInWorkerWorkerAsResource( ResourceSet.createWithWorkerKey( spawn.getLocalResources().getMemoryMb(), spawn.getLocalResources().getCpuUsage(), + spawn.getLocalResources().getExtraResourceUsage(), spawn.getLocalResources().getLocalTestCount(), key); diff --git a/src/main/java/com/google/devtools/common/options/Converters.java b/src/main/java/com/google/devtools/common/options/Converters.java index 29c5f823fe250c..828d807a513651 100644 --- a/src/main/java/com/google/devtools/common/options/Converters.java +++ b/src/main/java/com/google/devtools/common/options/Converters.java @@ -475,7 +475,7 @@ public String getTypeDescription() { /** * A converter for for assignments from a string value to a float value. */ - public static class StringToFloatAssignmentConverter implements Converter> { + public static class StringToFloatAssignmentConverter extends Converter.Contextless> { private static final AssignmentConverter baseConverter = new AssignmentConverter(); @Override diff --git a/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java b/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java index b5242917a15119..aabf232e0d2f4e 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java @@ -114,21 +114,42 @@ private ResourceHandle acquire(double ram, double cpu, int tests) return acquire(ram, cpu, tests, ResourcePriority.LOCAL); } - private ResourceHandle acquire(double ram, double cpu, int tests, ImmutableMap extraResources, String mnemonic) + private ResourceHandle acquire(double ram, double cpu, int tests, String mnemonic) throws InterruptedException, IOException { return rm.acquireResources( resourceOwner, - ResourceSet.createWithWorkerKey(ram, cpu, tests, extraResources, createWorkerKey(mnemonic)), + ResourceSet.createWithWorkerKey(ram, cpu, tests, createWorkerKey(mnemonic)), + ResourcePriority.LOCAL); + } + + private ResourceHandle acquire(double ram, double cpu, ImmutableMap extraResources, int tests, ResourcePriority priority) + throws InterruptedException, IOException { + return rm.acquireResources(resourceOwner, ResourceSet.create(ram, cpu, extraResources, tests), priority); + } + + private ResourceHandle acquire(double ram, double cpu, ImmutableMap extraResources, int tests) + throws InterruptedException, IOException { + return acquire(ram, cpu, extraResources, tests, ResourcePriority.LOCAL); + } + + private ResourceHandle acquire(double ram, double cpu, ImmutableMap extraResources, int tests, String mnemonic) + throws InterruptedException, IOException { + + return rm.acquireResources( + resourceOwner, + ResourceSet.createWithWorkerKey(ram, cpu, extraResources, tests, createWorkerKey(mnemonic)), ResourcePriority.LOCAL); } private void release(double ram, double cpu, int tests) throws IOException, InterruptedException { - rm.releaseResources(resourceOwner, ResourceSet.create(ram, cpu, tests), /* worker=*/ null); + rm.releaseResources(resourceOwner, ResourceSet.create(ram, cpu, tests), /* worker= */ null); } - private void release(double ram, double cpu, int tests, ImmutableMap extraResources) { - rm.releaseResources(resourceOwner, ResourceSet.create(ram, cpu, extraResources, tests)); + private void release(double ram, double cpu, ImmutableMap extraResources, int tests) + throws InterruptedException, IOException { + rm.releaseResources(resourceOwner, ResourceSet.create(ram, cpu, extraResources, tests), + /* worker= */ null); } private void validate(int count) { @@ -181,9 +202,9 @@ public void testOverBudgetRequests() throws Exception { // Ditto, for extra resources (even if they don't exist in the available resource set): ImmutableMap bigExtraResources = ImmutableMap.of("gpu", 10.0f, "fancyresource", 10.0f, "nonexisting", 10.0f); - acquire(0, 0, 0, bigExtraResources); + acquire(0, 0, bigExtraResources, 0); assertThat(rm.inUse()).isTrue(); - release(0, 0, 0, bigExtraResources); + release(0, 0, bigExtraResources, 0); assertThat(rm.inUse()).isFalse(); } @@ -271,20 +292,21 @@ public void testThatExtraResourcesCannotBeOverallocated() throws Exception { assertThat(rm.inUse()).isFalse(); // Given a partially acquired extra resources: - acquire(0, 0, 1, ImmutableMap.of("gpu", 1.0f)); + acquire(0, 0, ImmutableMap.of("gpu", 1.0f), 1); // When a request for extra resources is made that would overallocate, // Then the request fails: - TestThread thread1 = new TestThread(() -> assertThat(acquireNonblocking(0, 0, 0, ImmutableMap.of("gpu", 1.1f))).isNull()); + TestThread thread1 = new TestThread(() -> acquire(0, 0, ImmutableMap.of("gpu", 1.1f), 0)); thread1.start(); - thread1.joinAndAssertState(10000); + AssertionError e = assertThrows(AssertionError.class, () -> thread1.joinAndAssertState(10000)); + assertThat(e).hasCauseThat().hasMessageThat().contains("is still alive"); } @Test public void testHasResources() throws Exception { assertThat(rm.inUse()).isFalse(); assertThat(rm.threadHasResources()).isFalse(); - acquire(1, 0.1, 1, ImmutableMap.of("gpu", 1.0f)); + acquire(1, 0.1, ImmutableMap.of("gpu", 1.0f), 1); assertThat(rm.threadHasResources()).isTrue(); // We have resources in this thread - make sure other threads @@ -305,15 +327,15 @@ public void testHasResources() throws Exception { assertThat(rm.threadHasResources()).isTrue(); release(0, 0, 1); assertThat(rm.threadHasResources()).isFalse(); - acquire(0, 0, 0, ImmutableMap.of("gpu", 1.0f)); + acquire(0, 0, ImmutableMap.of("gpu", 1.0f), 0); assertThat(rm.threadHasResources()).isTrue(); - release(0, 0, 0, ImmutableMap.of("gpu", 1.0f)); + release(0, 0, ImmutableMap.of("gpu", 1.0f), 0); assertThat(rm.threadHasResources()).isFalse(); }); thread1.start(); thread1.joinAndAssertState(10000); - release(1, 0.1, 1, ImmutableMap.of("gpu", 1.0f)); + release(1, 0.1, ImmutableMap.of("gpu", 1.0f), 1); assertThat(rm.threadHasResources()).isFalse(); assertThat(rm.inUse()).isFalse(); }