From c89f8cfbc1d676cd90743aeadc4e5df6453738b4 Mon Sep 17 00:00:00 2001 From: Googler Date: Fri, 13 Dec 2024 07:14:32 -0800 Subject: [PATCH] Implement an idle task to garbage collect stale install bases. The `--experimental_install_base_gc_max_age` flag determines the criteria for considering an install base stale; zero disables garbage collection. The default will be replaced with a suitable nonzero value in a future CL. Progress on #2109. PiperOrigin-RevId: 705874241 Change-Id: I3c8526a4a0e20a12d52c08cb282f24e268cbc633 --- .../java/com/google/devtools/build/lib/BUILD | 1 + .../build/lib/runtime/BlazeRuntime.java | 6 ++ .../lib/runtime/CommonCommandOptions.java | 13 ++++ .../google/devtools/build/lib/server/BUILD | 7 ++- .../server/InstallBaseGarbageCollector.java | 19 +++++- .../InstallBaseGarbageCollectorIdleTask.java | 60 +++++++++++++++++++ .../google/devtools/build/lib/runtime/BUILD | 1 + .../build/lib/runtime/BlazeRuntimeTest.java | 32 +++++++++- src/test/shell/bazel/client_test.sh | 28 +++++++++ 9 files changed, 163 insertions(+), 4 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/server/InstallBaseGarbageCollectorIdleTask.java diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD index a4a88a87b0e4e0..794ef830285da4 100644 --- a/src/main/java/com/google/devtools/build/lib/BUILD +++ b/src/main/java/com/google/devtools/build/lib/BUILD @@ -411,6 +411,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/runtime/events", "//src/main/java/com/google/devtools/build/lib/server", "//src/main/java/com/google/devtools/build/lib/server:idle_task", + "//src/main/java/com/google/devtools/build/lib/server:install_base_garbage_collector", "//src/main/java/com/google/devtools/build/lib/server:pid_file_watcher", "//src/main/java/com/google/devtools/build/lib/server:rpc_server", "//src/main/java/com/google/devtools/build/lib/server:shutdown_hooks", diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java index d69290fbdc295a..c9c1e978bd6331 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java @@ -83,6 +83,7 @@ import com.google.devtools.build.lib.server.FailureDetails.Filesystem; import com.google.devtools.build.lib.server.GrpcServerImpl; import com.google.devtools.build.lib.server.IdleTask; +import com.google.devtools.build.lib.server.InstallBaseGarbageCollectorIdleTask; import com.google.devtools.build.lib.server.PidFileWatcher; import com.google.devtools.build.lib.server.RPCServer; import com.google.devtools.build.lib.server.ShutdownHooks; @@ -624,6 +625,11 @@ void beforeCommand(CommandEnvironment env, CommonCommandOptions options) { .handle(Event.error("Error while creating memory profile file: " + e.getMessage())); } } + if (!options.installBaseGcMaxAge.isZero()) { + env.addIdleTask( + InstallBaseGarbageCollectorIdleTask.create( + workspace.getDirectories().getInstallBase(), options.installBaseGcMaxAge)); + } } @Override diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java b/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java index b45f0fd92f5b3b..a58a6171dc9647 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java @@ -23,6 +23,7 @@ import com.google.devtools.common.options.Converter; import com.google.devtools.common.options.Converters; import com.google.devtools.common.options.Converters.AssignmentConverter; +import com.google.devtools.common.options.Converters.DurationConverter; import com.google.devtools.common.options.EnumConverter; import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; @@ -105,6 +106,18 @@ public class CommonCommandOptions extends OptionsBase { help = "Whether profiling slow operations is always turned on") public boolean alwaysProfileSlowOperations; + @Option( + name = "experimental_install_base_gc_max_age", + defaultValue = "0", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS}, + converter = DurationConverter.class, + help = + "How long an install base must go unused before it's eligible for garbage collection." + + " If nonzero, the server will attempt to garbage collect other install bases when" + + " idle.") + public Duration installBaseGcMaxAge; + /** Converter for UUID. Accepts values as specified by {@link UUID#fromString(String)}. */ public static class UUIDConverter extends Converter.Contextless { diff --git a/src/main/java/com/google/devtools/build/lib/server/BUILD b/src/main/java/com/google/devtools/build/lib/server/BUILD index b49dde0b686cf2..640ac73d61dd7e 100644 --- a/src/main/java/com/google/devtools/build/lib/server/BUILD +++ b/src/main/java/com/google/devtools/build/lib/server/BUILD @@ -104,10 +104,15 @@ java_library( java_library( name = "install_base_garbage_collector", - srcs = ["InstallBaseGarbageCollector.java"], + srcs = [ + "InstallBaseGarbageCollector.java", + "InstallBaseGarbageCollectorIdleTask.java", + ], deps = [ + ":idle_task", "//src/main/java/com/google/devtools/build/lib/util:file_system_lock", "//src/main/java/com/google/devtools/build/lib/vfs", + "//third_party:flogger", "//third_party:guava", ], ) diff --git a/src/main/java/com/google/devtools/build/lib/server/InstallBaseGarbageCollector.java b/src/main/java/com/google/devtools/build/lib/server/InstallBaseGarbageCollector.java index 53593586b5cfb8..6e3696122d34cb 100644 --- a/src/main/java/com/google/devtools/build/lib/server/InstallBaseGarbageCollector.java +++ b/src/main/java/com/google/devtools/build/lib/server/InstallBaseGarbageCollector.java @@ -49,13 +49,28 @@ public final class InstallBaseGarbageCollector { * @param ownInstallBase the current server's install base * @param maxAge how long an install base must remain unused before it's eligible for collection */ - public InstallBaseGarbageCollector(Path root, Path ownInstallBase, Duration maxAge) { + InstallBaseGarbageCollector(Path root, Path ownInstallBase, Duration maxAge) { this.root = root; this.ownInstallBase = ownInstallBase; this.maxAge = maxAge; } - public void run() throws IOException, InterruptedException { + @VisibleForTesting + public Path getRoot() { + return root; + } + + @VisibleForTesting + public Path getOwnInstallBase() { + return ownInstallBase; + } + + @VisibleForTesting + public Duration getMaxAge() { + return maxAge; + } + + void run() throws IOException, InterruptedException { for (Dirent dirent : root.readdir(Symlinks.FOLLOW)) { if (Thread.interrupted()) { throw new InterruptedException(); diff --git a/src/main/java/com/google/devtools/build/lib/server/InstallBaseGarbageCollectorIdleTask.java b/src/main/java/com/google/devtools/build/lib/server/InstallBaseGarbageCollectorIdleTask.java new file mode 100644 index 00000000000000..b0c23fc0dc5dba --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/server/InstallBaseGarbageCollectorIdleTask.java @@ -0,0 +1,60 @@ +// Copyright 2024 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.server; + +import com.google.common.annotations.VisibleForTesting; +import com.google.common.flogger.GoogleLogger; +import com.google.devtools.build.lib.vfs.Path; +import java.io.IOException; +import java.time.Duration; + +/** An {@link IdleTask} to run a {@link InstallBaseGarbageCollector}. */ +public final class InstallBaseGarbageCollectorIdleTask implements IdleTask { + private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); + + private final InstallBaseGarbageCollector gc; + + private InstallBaseGarbageCollectorIdleTask(InstallBaseGarbageCollector gc) { + this.gc = gc; + } + + /** + * Creates a new {@link InstallBaseGarbageCollectorIdleTask} according to the options. + * + * @param installBase the server install base + * @param maxAge how long an install base must remain unused to be considered stale + * @return the idle task + */ + public static InstallBaseGarbageCollectorIdleTask create(Path installBase, Duration maxAge) { + return new InstallBaseGarbageCollectorIdleTask( + new InstallBaseGarbageCollector(installBase.getParentDirectory(), installBase, maxAge)); + } + + @VisibleForTesting + public InstallBaseGarbageCollector getGarbageCollector() { + return gc; + } + + @Override + public void run() { + try { + logger.atInfo().log("Install base garbage collection started"); + gc.run(); + } catch (IOException e) { + logger.atInfo().withCause(e).log("Install base garbage collection failed"); + } catch (InterruptedException e) { + logger.atInfo().withCause(e).log("Install base garbage collection interrupted"); + } + } +} diff --git a/src/test/java/com/google/devtools/build/lib/runtime/BUILD b/src/test/java/com/google/devtools/build/lib/runtime/BUILD index 4a3b23bb8a258b..ee9ae63821b477 100644 --- a/src/test/java/com/google/devtools/build/lib/runtime/BUILD +++ b/src/test/java/com/google/devtools/build/lib/runtime/BUILD @@ -74,6 +74,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/remote:store", "//src/main/java/com/google/devtools/build/lib/runtime/commands", "//src/main/java/com/google/devtools/build/lib/server:idle_task", + "//src/main/java/com/google/devtools/build/lib/server:install_base_garbage_collector", "//src/main/java/com/google/devtools/build/lib/skyframe:analysis_progress_receiver", "//src/main/java/com/google/devtools/build/lib/skyframe:configuration_phase_started_event", "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_key", diff --git a/src/test/java/com/google/devtools/build/lib/runtime/BlazeRuntimeTest.java b/src/test/java/com/google/devtools/build/lib/runtime/BlazeRuntimeTest.java index 5712226c522da1..2c6e95c71995b4 100644 --- a/src/test/java/com/google/devtools/build/lib/runtime/BlazeRuntimeTest.java +++ b/src/test/java/com/google/devtools/build/lib/runtime/BlazeRuntimeTest.java @@ -29,6 +29,7 @@ import com.google.devtools.build.lib.server.FailureDetails.Crash.Code; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.server.IdleTask; +import com.google.devtools.build.lib.server.InstallBaseGarbageCollectorIdleTask; import com.google.devtools.build.lib.testutil.ManualClock; import com.google.devtools.build.lib.util.DetailedExitCode; import com.google.devtools.build.lib.vfs.DigestHashFunction; @@ -43,6 +44,7 @@ import com.google.protobuf.ByteString; import com.google.protobuf.BytesValue; import com.google.protobuf.StringValue; +import java.time.Duration; import java.util.Arrays; import java.util.concurrent.atomic.AtomicReference; import org.junit.Test; @@ -170,7 +172,35 @@ public void addsResponseExtensions() throws Exception { } @Test - public void addsIdleTasks() throws Exception { + public void doesNotAddInstallBaseGcIdleTaskWhenDisabled() throws Exception { + BlazeRuntime runtime = createRuntime(); + CommandEnvironment env = createCommandEnvironment(runtime); + CommonCommandOptions options = new CommonCommandOptions(); + options.installBaseGcMaxAge = Duration.ZERO; + runtime.beforeCommand(env, options); + assertThat(env.getIdleTasks()).isEmpty(); + } + + @Test + public void addsInstallBaseGcIdleTaskWhenEnabled() throws Exception { + BlazeRuntime runtime = createRuntime(); + CommandEnvironment env = createCommandEnvironment(runtime); + CommonCommandOptions options = new CommonCommandOptions(); + options.installBaseGcMaxAge = Duration.ofDays(365); + runtime.beforeCommand(env, options); + assertThat(env.getIdleTasks()).hasSize(1); + assertThat(env.getIdleTasks().get(0)).isInstanceOf(InstallBaseGarbageCollectorIdleTask.class); + var idleTask = (InstallBaseGarbageCollectorIdleTask) env.getIdleTasks().get(0); + assertThat(idleTask.delay()).isEqualTo(Duration.ZERO); + assertThat(idleTask.getGarbageCollector().getRoot()) + .isEqualTo(blazeDirectories.getInstallBase().getParentDirectory()); + assertThat(idleTask.getGarbageCollector().getOwnInstallBase()) + .isEqualTo(blazeDirectories.getInstallBase()); + assertThat(idleTask.getGarbageCollector().getMaxAge()).isEqualTo(Duration.ofDays(365)); + } + + @Test + public void addsIdleTasksFromModules() throws Exception { BlazeRuntime runtime = createRuntime(); CommandEnvironment env = createCommandEnvironment(runtime); IdleTask fooTask = () -> {}; diff --git a/src/test/shell/bazel/client_test.sh b/src/test/shell/bazel/client_test.sh index 70fcd728267e5f..26da0e615e99fc 100755 --- a/src/test/shell/bazel/client_test.sh +++ b/src/test/shell/bazel/client_test.sh @@ -65,4 +65,32 @@ function test_install_base_lock() { "$LOCK_HELPER" "${install_base}.lock" exclusive exit || fail "Expected success" } +function test_install_base_garbage_collection() { + local -r install_user_root="$TEST_TMPDIR/test_install_base_garbage_collection" + local -r install_base="${install_user_root}/abcdefabcdefabcdefabcdefabcdefab" + + local -r stale="${install_user_root}/12345678901234567890123456789012" + mkdir -p "${stale}" + touch "${stale}/A-server.jar" + touch -t 200102030405 "${stale}" + + local -r fresh="${install_user_root}/98765432109876543210987654321098" + mkdir -p "${fresh}" + touch "${fresh}/A-server.jar" + + bazel --install_base="${install_base}" info \ + --experimental_install_base_gc_max_age=1d \ + &> "$TEST_log" || fail "Expected success" + + sleep 1 + + if ! [[ -d "${fresh}" ]]; then + fail "Expected ${fresh} to still exist" + fi + + if [[ -d "${stale}" ]]; then + fail "Expected ${stale} to no longer exist" + fi +} + run_suite "client_test"