Skip to content

Commit

Permalink
Implement Persistent Workers as an execution path (buildfarm#1260)
Browse files Browse the repository at this point in the history
Followup to buildfarm#1195

Add a new execution pathway in worker/Executor.java to use persistent workers via PersistentExecutor, like DockerExecutor.

Mostly unchanged from the form we used to experiment back at Twitter, but now with tests.

Co-authored-by: Shane Delmore [email protected]
  • Loading branch information
wiwa authored Nov 20, 2023
1 parent 221eae9 commit 91587e7
Show file tree
Hide file tree
Showing 24 changed files with 2,043 additions and 8 deletions.
16 changes: 15 additions & 1 deletion src/main/java/build/buildfarm/common/ExecutionProperties.java
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,21 @@ public class ExecutionProperties {
/**
* @field WORKER
* @brief The exec_property to ensure that the action only runs on the worker name given.
* @details Useful for diagnosing woker issues by targeting builds to a specific worker.
* @details Useful for diagnosing worker issues by targeting builds to a specific worker.
*/
public static final String WORKER = "Worker";

/**
* @field PERSISTENT_WORKER_KEY
* @brief Hash of tool inputs from --experiemental_remote_mark_tool_inputs
* @details See https://github.com/bazelbuild/bazel/issues/10091
*/
public static final String PERSISTENT_WORKER_KEY = "persistentWorkerKey";

/**
* @field PERSISTENT_WORKER_COMMAND
* @brief Command string to start the persistent worker
* @details See https://github.com/bazelbuild/bazel/issues/10091
*/
public static final String PERSISTENT_WORKER_COMMAND = "persistentWorkerCommand";
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

@Data
public class DequeueMatchSettings {

@Getter(AccessLevel.NONE)
private boolean acceptEverything; // deprecated

Expand Down
4 changes: 4 additions & 0 deletions src/main/java/build/buildfarm/worker/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,16 @@ java_library(
plugins = ["//src/main/java/build/buildfarm/common:lombok"],
visibility = ["//visibility:public"],
deps = [
"//persistentworkers/src/main/java/persistent/bazel:bazel-persistent-workers",
"//persistentworkers/src/main/java/persistent/common:persistent-common",
"//src/main/java/build/buildfarm/cas",
"//src/main/java/build/buildfarm/common",
"//src/main/java/build/buildfarm/common/config",
"//src/main/java/build/buildfarm/instance",
"//src/main/java/build/buildfarm/instance/stub",
"//src/main/java/build/buildfarm/worker/persistent",
"//src/main/java/build/buildfarm/worker/resources",
"//src/main/java/build/buildfarm/worker/util",
"//src/main/protobuf:build_buildfarm_v1test_buildfarm_java_proto",
"@bazel//src/main/protobuf:execution_statistics_java_proto",
"@googleapis//:google_rpc_code_java_proto",
Expand Down
34 changes: 30 additions & 4 deletions src/main/java/build/buildfarm/worker/Executor.java
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,16 @@
import build.buildfarm.common.config.ExecutionPolicy;
import build.buildfarm.common.config.ExecutionWrapper;
import build.buildfarm.v1test.ExecutingOperationMetadata;
import build.buildfarm.v1test.Tree;
import build.buildfarm.worker.WorkerContext.IOResource;
import build.buildfarm.worker.persistent.PersistentExecutor;
import build.buildfarm.worker.persistent.WorkFilesContext;
import build.buildfarm.worker.resources.ResourceLimits;
import com.github.dockerjava.api.DockerClient;
import com.github.dockerjava.core.DockerClientBuilder;
import com.google.common.base.Stopwatch;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.shell.Protos.ExecutionStatistics;
import com.google.longrunning.Operation;
import com.google.protobuf.Any;
Expand Down Expand Up @@ -427,16 +431,38 @@ private Code executeCommand(
for (EnvironmentVariable environmentVariable : environmentVariables) {
environment.put(environmentVariable.getName(), environmentVariable.getValue());
}
for (Map.Entry<String, String> environmentVariable :
limits.extraEnvironmentVariables.entrySet()) {
environment.put(environmentVariable.getKey(), environmentVariable.getValue());
}
environment.putAll(limits.extraEnvironmentVariables);

// allow debugging before an execution
if (limits.debugBeforeExecution) {
return ExecutionDebugger.performBeforeExecutionDebug(processBuilder, limits, resultBuilder);
}

boolean usePersistentWorker =
!limits.persistentWorkerKey.isEmpty() && !limits.persistentWorkerCommand.isEmpty();

if (usePersistentWorker) {
log.fine(
"usePersistentWorker; got persistentWorkerCommand of : "
+ limits.persistentWorkerCommand);

Tree execTree = workerContext.getQueuedOperation(operationContext.queueEntry).getTree();

WorkFilesContext filesContext =
WorkFilesContext.fromContext(execDir, execTree, operationContext.command);

return PersistentExecutor.runOnPersistentWorker(
limits.persistentWorkerCommand,
filesContext,
operationName,
ImmutableList.copyOf(arguments),
ImmutableMap.copyOf(environment),
limits,
timeout,
PersistentExecutor.defaultWorkRootsDir,
resultBuilder);
}

// run the action under docker
if (limits.containerSettings.enabled) {
DockerClient dockerClient = DockerClientBuilder.getInstance().build();
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/build/buildfarm/worker/OperationContext.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import com.google.longrunning.Operation;
import java.nio.file.Path;

final class OperationContext {
public final class OperationContext {
final ExecuteResponse.Builder executeResponse;
final Operation operation;
final Poller poller;
Expand Down
31 changes: 31 additions & 0 deletions src/main/java/build/buildfarm/worker/persistent/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
java_library(
name = "persistent",
srcs = glob(["*.java"]),
plugins = ["//src/main/java/build/buildfarm/common:lombok"],
visibility = ["//visibility:public"],
deps = [
"//persistentworkers/src/main/java/persistent/bazel:bazel-persistent-workers",
"//persistentworkers/src/main/java/persistent/common:persistent-common",
"//persistentworkers/src/main/java/persistent/common/util",
"//persistentworkers/src/main/protobuf:worker_protocol_java_proto",
"//src/main/java/build/buildfarm/common",
"//src/main/java/build/buildfarm/worker/resources",
"//src/main/java/build/buildfarm/worker/util",
"//src/main/protobuf:build_buildfarm_v1test_buildfarm_java_proto",
"@maven//:com_google_api_grpc_proto_google_common_protos",
"@maven//:com_google_guava_guava",
"@maven//:com_google_protobuf_protobuf_java",
"@maven//:com_google_protobuf_protobuf_java_util",
"@maven//:commons_io_commons_io",
"@maven//:io_grpc_grpc_api",
"@maven//:io_grpc_grpc_context",
"@maven//:io_grpc_grpc_core",
"@maven//:io_grpc_grpc_netty",
"@maven//:io_grpc_grpc_protobuf",
"@maven//:io_grpc_grpc_stub",
"@maven//:io_prometheus_simpleclient",
"@maven//:org_apache_commons_commons_compress",
"@maven//:org_jetbrains_annotations",
"@maven//:org_projectlombok_lombok",
],
)
171 changes: 171 additions & 0 deletions src/main/java/build/buildfarm/worker/persistent/FileAccessUtils.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
// Copyright 2023 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 build.buildfarm.worker.persistent;

import static java.nio.file.StandardCopyOption.COPY_ATTRIBUTES;
import static java.nio.file.StandardCopyOption.REPLACE_EXISTING;

import com.google.common.collect.ImmutableSet;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.attribute.PosixFilePermission;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.function.Supplier;
import lombok.extern.java.Log;

/**
* Utility for concurrent move/copy of files Can be extended in the future to (sym)linking if we
* need performance
*/
@Log
public final class FileAccessUtils {
// singleton class with only static methods
private FileAccessUtils() {}

public static Path addPosixOwnerWrite(Path absPath) throws IOException {
Set<PosixFilePermission> perms = Files.getPosixFilePermissions(absPath);

ImmutableSet<PosixFilePermission> permsWithWrite =
ImmutableSet.<PosixFilePermission>builder()
.addAll(perms)
.add(PosixFilePermission.OWNER_WRITE)
.build();

return Files.setAttribute(absPath, "posix:permissions", permsWithWrite);
}

private static final ConcurrentHashMap<Path, PathLock> fileLocks = new ConcurrentHashMap<>();

// Used here as a simple lock for locking "files" (paths)
private static class PathLock {
// Not used elsewhere
private PathLock() {}
}

/**
* Copies a file, creating necessary directories, replacing existing files. The resulting file is
* set to be writeable, and we throw if we cannot set that. Thread-safe (within a process) against
* writes to the same path.
*
* @param from
* @param to
* @throws IOException
*/
public static void copyFile(Path from, Path to) throws IOException {
Path absTo = to.toAbsolutePath();
log.finer("copyFile: " + from + " to " + absTo);
if (!Files.exists(from)) {
throw new IOException("copyFile: source file doesn't exist: " + from);
}
IOException ioException =
writeFileSafe(
to,
() -> {
try {
Files.copy(from, absTo, REPLACE_EXISTING, COPY_ATTRIBUTES);
addPosixOwnerWrite(absTo);
return null;
} catch (IOException e) {
return new IOException("copyFile() could not set writeable: " + absTo, e);
}
});
if (ioException != null) {
throw ioException;
}
}

/**
* Moves a file, creating necessary directories, replacing existing files. The resulting file is
* set to be writeable, and we throw if we cannot set that. Thread-safe against writes to the same
* path.
*
* @param from
* @param to
* @throws IOException
*/
public static void moveFile(Path from, Path to) throws IOException {
Path absTo = to.toAbsolutePath();
log.finer("moveFile: " + from + " to " + absTo);
if (!Files.exists(from)) {
throw new IOException("moveFile: source file doesn't exist: " + from);
}
IOException ioException =
writeFileSafe(
absTo,
() -> {
try {
Files.move(from, absTo, REPLACE_EXISTING);
addPosixOwnerWrite(absTo);
return null;
} catch (IOException e) {
return new IOException("copyFile() could not set writeable: " + absTo, e);
}
});
if (ioException != null) {
throw ioException;
}
}

/**
* Deletes a file; Thread-safe against writes to the same path.
*
* @param toDelete
* @throws IOException
*/
public static void deleteFileIfExists(Path toDelete) throws IOException {
Path absTo = toDelete.toAbsolutePath();
PathLock toLock = fileLock(absTo);
synchronized (toLock) {
try {
Files.deleteIfExists(absTo);
} finally {
fileLocks.remove(absTo);
}
}
}

/**
* Thread-safe (not multi-process-safe) wrapper for locking paths before a write operation.
*
* <p>This method will create necessary parent directories.
*
* <p>It is up to the write operation to specify whether or not to overwrite existing files.
*/
@SuppressWarnings("PMD.UnnecessaryLocalBeforeReturn")
private static IOException writeFileSafe(Path absTo, Supplier<IOException> writeOp) {
PathLock toLock = fileLock(absTo);
synchronized (toLock) {
try {
// If 'absTo' is a symlink, checks if its target file exists
Files.createDirectories(absTo.getParent());
return writeOp.get();
} catch (IOException e) {
// PMD will complain about UnnecessaryLocalBeforeReturn
// In this case, it is necessary to catch the exception
return e;
} finally {
// Clean up to prevent too many locks.
fileLocks.remove(absTo);
}
}
}

// "Logical" file lock
private static PathLock fileLock(Path writeTo) {
return fileLocks.computeIfAbsent(writeTo, k -> new PathLock());
}
}
Loading

0 comments on commit 91587e7

Please sign in to comment.