Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[7.1.0] Fix stale trash dir not cleaned up on worker creation #21510

Merged
merged 1 commit into from
Feb 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -110,4 +110,8 @@ public void shutdown() {
service = null;
}
}

public Path getTrashBase() {
return trashBase;
}
}
3 changes: 1 addition & 2 deletions src/main/java/com/google/devtools/build/lib/worker/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,6 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/exec:execution_options",
"//src/main/java/com/google/devtools/build/lib/exec:runfiles_tree_updater",
"//src/main/java/com/google/devtools/build/lib/exec:spawn_strategy_registry",
"//src/main/java/com/google/devtools/build/lib/exec:tree_deleter",
"//src/main/java/com/google/devtools/build/lib/exec/local",
"//src/main/java/com/google/devtools/build/lib/runtime/commands/events",
"//src/main/java/com/google/devtools/build/lib/sandbox:linux_sandbox_util",
Expand Down Expand Up @@ -187,7 +186,7 @@ java_library(
":worker_events",
":worker_key",
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/com/google/devtools/build/lib/exec:tree_deleter",
"//src/main/java/com/google/devtools/build/lib/sandbox:tree_deleter",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//third_party:apache_commons_pool2",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
import com.google.common.io.BaseEncoding;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.Reporter;
import com.google.devtools.build.lib.exec.TreeDeleter;
import com.google.devtools.build.lib.sandbox.AsynchronousTreeDeleter;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.worker.SandboxedWorker.WorkerSandboxOptions;
Expand Down Expand Up @@ -47,7 +47,7 @@ public class WorkerFactory extends BaseKeyedPooledObjectFactory<WorkerKey, Worke
private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();

private final Path workerBaseDir;
private final TreeDeleter treeDeleter;
private final AsynchronousTreeDeleter treeDeleter;
private Reporter reporter;
private EventBus eventBus;

Expand All @@ -64,7 +64,7 @@ public WorkerFactory(Path workerBaseDir) {
public WorkerFactory(
Path workerBaseDir,
@Nullable WorkerSandboxOptions hardenedSandboxOptions,
@Nullable TreeDeleter treeDeleter) {
@Nullable AsynchronousTreeDeleter treeDeleter) {
this.workerBaseDir = workerBaseDir;
this.hardenedSandboxOptions = hardenedSandboxOptions;
this.treeDeleter = treeDeleter;
Expand All @@ -84,6 +84,10 @@ public Worker create(WorkerKey key) throws IOException {
String workTypeName = key.getWorkerTypeName();
if (!workerBaseDir.isDirectory()) {
workerBaseDir.createDirectoryAndParents();
Path deleterTrashBase = treeDeleter == null ? null : treeDeleter.getTrashBase();
if (deleterTrashBase != null) {
deleterTrashBase.createDirectory();
}
}
Path logFile =
workerBaseDir.getRelative(workTypeName + "-" + workerId + "-" + key.getMnemonic() + ".log");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import com.google.devtools.build.lib.exec.ExecutionOptions;
import com.google.devtools.build.lib.exec.RunfilesTreeUpdater;
import com.google.devtools.build.lib.exec.SpawnStrategyRegistry;
import com.google.devtools.build.lib.exec.TreeDeleter;
import com.google.devtools.build.lib.exec.local.LocalEnvProvider;
import com.google.devtools.build.lib.runtime.BlazeModule;
import com.google.devtools.build.lib.runtime.BlazeWorkspace;
Expand All @@ -45,10 +44,12 @@

/** A module that adds the WorkerActionContextProvider to the available action context providers. */
public class WorkerModule extends BlazeModule {

private static final String STALE_TRASH = "_stale_trash";
private CommandEnvironment env;

private WorkerFactory workerFactory;
private TreeDeleter treeDeleter;
private AsynchronousTreeDeleter treeDeleter;
@VisibleForTesting WorkerPoolImpl workerPool;
@Nullable private WorkerLifecycleManager workerLifecycleManager;

Expand Down Expand Up @@ -114,6 +115,9 @@ public void buildStarting(BuildStartingEvent event) {
Path trashBase = workerDir.getRelative(AsynchronousTreeDeleter.MOVED_TRASH_DIR);
if (treeDeleter == null) {
treeDeleter = new AsynchronousTreeDeleter(trashBase);
if (trashBase.exists()) {
removeStaleTrash(workerDir, trashBase);
}
}
WorkerFactory newWorkerFactory =
new WorkerFactory(workerDir, workerSandboxOptions, treeDeleter);
Expand Down Expand Up @@ -184,6 +188,28 @@ public void buildStarting(BuildStartingEvent event) {
workerPool.clearDoomedWorkers();
}

private void removeStaleTrash(Path workerDir, Path trashBase) {
try {
// The AsynchronousTreeDeleter relies on a counter for naming directories that will be
// moved out of the way before being deleted asynchronously.
// If there is trash on disk from a previous bazel server instance, the dirs will have
// names not synced with the counter, therefore we may run the risk of moving a directory
// in this server instance to a path of an existing directory. To solve this we rename
// the trash directory that was on disk, create a new empty trash directory and delete
// the old trash via the AsynchronousTreeDeleter. Before deletion the stale trash will be
// moved to a directory named `0` under MOVED_TRASH_DIR.
Path staleTrash = trashBase.getParentDirectory().getChild(STALE_TRASH);
trashBase.renameTo(staleTrash);
trashBase.createDirectory();
treeDeleter.deleteTree(staleTrash);
} catch (IOException e) {
env.getReporter()
.handle(
Event.error(
String.format("Could not trash dir in '%s': %s", workerDir, e.getMessage())));
}
}

@Override
public void registerSpawnStrategies(
SpawnStrategyRegistry.Builder registryBuilder, CommandEnvironment env) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import com.google.devtools.build.lib.runtime.BlazeRuntime;
import com.google.devtools.build.lib.runtime.BlazeWorkspace;
import com.google.devtools.build.lib.runtime.CommandEnvironment;
import com.google.devtools.build.lib.sandbox.AsynchronousTreeDeleter;
import com.google.devtools.build.lib.util.AbruptExitException;
import com.google.devtools.build.lib.vfs.DigestHashFunction;
import com.google.devtools.build.lib.vfs.FileSystem;
Expand Down Expand Up @@ -232,6 +233,32 @@ public void buildStarting_survivesNoWorkerDir() throws Exception {
assertThrows(IOException.class, () -> module.workerPool.borrowObject(key));
}

@Test
public void buildStarting_cleansStaleTrashDirCleanedOnFirstBuild() throws Exception {
WorkerModule module = new WorkerModule();
WorkerOptions options = WorkerOptions.DEFAULTS;

when(request.getOptions(WorkerOptions.class)).thenReturn(options);
setupEnvironment("/outputRoot");

module.beforeCommand(env);
Path workerDir = fs.getPath("/outputRoot/outputBase/bazel-workers");
Path trashBase = workerDir.getChild(AsynchronousTreeDeleter.MOVED_TRASH_DIR);
trashBase.createDirectoryAndParents();

Path staleTrash = trashBase.getChild("random-trash");

staleTrash.createDirectoryAndParents();
module.buildStarting(BuildStartingEvent.create(env, request));
// Trash is cleaned upon first build.
assertThat(staleTrash.exists()).isFalse();

staleTrash.createDirectoryAndParents();
module.buildStarting(BuildStartingEvent.create(env, request));
// Trash is not cleaned upon subsequent builds.
assertThat(staleTrash.exists()).isTrue();
}

private void setupEnvironment(String rootDir) throws IOException, AbruptExitException {
storedEventHandler = new StoredEventHandler();
Path root = fs.getPath(rootDir);
Expand Down
Loading