From 7b84c62d8ef3b920b11499fa5e18f0924d7da7e7 Mon Sep 17 00:00:00 2001 From: Rahul Butani Date: Wed, 29 Nov 2023 21:40:52 -0800 Subject: [PATCH] Add client option to disable updating the `mtime` of the `install_base` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently if the `--install_base` path passed is not writeable by the user invoking Bazel, the Bazel client crashes[^1]: ```console ❯ bazel --install_base=/some/read/only/path version FATAL: failed to set timestamp on '/some/read/only/path': (error: 30): Read-only file system ``` This happens because the Bazel client (unconditionally) attempts to update the `mtime` of this path: https://github.com/bazelbuild/bazel/blob/a3c677dfea2de636a719d50345a5a97af96fae60/src/main/cpp/blaze.cc#L1018-L1029 This PR gates this `mtime` update on a new client startup option: `--update_install_base_mtime`. [^1]: Note that if you invoke Bazel again after this happens, it will successfully start up because the `install` symlink in the output base [that inhibits the `mtime` update/install base check] has already been [created]. [that inhibits the `mtime` update/install base check]: https://github.com/bazelbuild/bazel/blob/a3c677dfea2de636a719d50345a5a97af96fae60/src/main/cpp/blaze.cc#L991-L1001 [created]: https://github.com/bazelbuild/bazel/blob/a3c677dfea2de636a719d50345a5a97af96fae60/src/main/cpp/blaze.cc#L1011 --- This commit: - [x] adds the `update_install_base_mtime` option to `startup_options.cc` and `BlazeServerStartupOptions.java` - [x] adds logic to `src/main/cpp/blaze.cc` to ensure that changes to this option do not cause server restarts - [x] updates `bazel_startup_options_test.cc` - [x] adds tests to `client_test.sh` that test that: * the install base `mtime` **is** updated when `--update_install_base_mtime` is passed (the default) * the install base `mtime` is **not** updated when `--noupdate_install_base_mtime` is passed * changes to `--install_base_mtime` do not cause server restarts TODO: - [ ] Both [`bazel_startup_options_test.cc`](https://github.com/bazelbuild/bazel/blob/a3c677dfea2de636a719d50345a5a97af96fae60/src/test/cpp/bazel_startup_options_test.cc#L92-L94) and [`startup_options.cc`](https://github.com/bazelbuild/bazel/blob/a3c677dfea2de636a719d50345a5a97af96fae60/src/main/cpp/startup_options.cc#L127-L129) describe a special procedure for updating startup flags: https://github.com/bazelbuild/bazel/blob/a3c677dfea2de636a719d50345a5a97af96fae60/src/test/cpp/bazel_startup_options_test.cc#L92-L94 https://github.com/bazelbuild/bazel/blob/a3c677dfea2de636a719d50345a5a97af96fae60/src/main/cpp/startup_options.cc#L127-L129 --- src/main/cpp/blaze.cc | 24 ++++++--- src/main/cpp/startup_options.cc | 2 + src/main/cpp/startup_options.h | 4 ++ .../runtime/BlazeServerStartupOptions.java | 10 ++++ src/test/cpp/bazel_startup_options_test.cc | 1 + src/test/shell/integration/client_test.sh | 51 +++++++++++++++++++ 6 files changed, 85 insertions(+), 7 deletions(-) diff --git a/src/main/cpp/blaze.cc b/src/main/cpp/blaze.cc index 6a764b15a52082..0dd20ddfeae4c2 100644 --- a/src/main/cpp/blaze.cc +++ b/src/main/cpp/blaze.cc @@ -476,6 +476,13 @@ static vector GetServerExeArgs(const blaze_util::Path &jvm_path, } else { result.push_back("--client_debug=false"); } + // Same as above; this option is only used by the client and doesn't + // necessitate a server restart: + if (startup_options.update_install_base_mtime) { + result.push_back("--update_install_base_mtime=true"); + } else { + result.push_back("--update_install_base_mtime=false"); + } // These flags are passed to the java process only for Blaze reporting // purposes; the real interpretation of the jvm flags occurs when we set up @@ -858,6 +865,7 @@ static bool IsVolatileArg(const string &arg) { static const std::set volatile_startup_options = { "--option_sources=", "--max_idle_secs=", "--connect_timeout_secs=", "--local_startup_timeout_secs=", "--client_debug=", "--preemptible=", + "--update_install_base_mtime=", // Internally, -XX:HeapDumpPath is set automatically via the user's TMPDIR // environment variable. Since that can change based on the shell, we // tolerate changes to it. Note that an explicit setting of @@ -1009,13 +1017,15 @@ static void EnsureCorrectRunningVersion(const StartupOptions &startup_options, // Update the mtime of the install base so that cleanup tools can // find install bases that haven't been used for a long time - std::unique_ptr mtime( - blaze_util::CreateFileMtime()); - if (!mtime->SetToNow(blaze_util::Path(startup_options.install_base))) { - string err = GetLastErrorString(); - BAZEL_DIE(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR) - << "failed to set timestamp on '" << startup_options.install_base - << "': " << err; + if (startup_options.update_install_base_mtime) { + std::unique_ptr mtime( + blaze_util::CreateFileMtime()); + if (!mtime->SetToNow(blaze_util::Path(startup_options.install_base))) { + string err = GetLastErrorString(); + BAZEL_DIE(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR) + << "failed to set timestamp on '" << startup_options.install_base + << "': " << err; + } } } } diff --git a/src/main/cpp/startup_options.cc b/src/main/cpp/startup_options.cc index d1e2f08e718d0f..58f4936185a24a 100644 --- a/src/main/cpp/startup_options.cc +++ b/src/main/cpp/startup_options.cc @@ -92,6 +92,7 @@ StartupOptions::StartupOptions(const string &product_name, expand_configs_in_place(true), digest_function(), idle_server_tasks(true), + update_install_base_mtime(true), original_startup_options_(std::vector()), #if defined(__APPLE__) macos_qos_class(QOS_CLASS_UNSPECIFIED), @@ -143,6 +144,7 @@ StartupOptions::StartupOptions(const string &product_name, RegisterNullaryStartupFlag("shutdown_on_low_sys_mem", &shutdown_on_low_sys_mem); RegisterNullaryStartupFlagNoRc("ignore_all_rc_files", &ignore_all_rc_files); + RegisterNullaryStartupFlag("update_install_base_mtime", &update_install_base_mtime); RegisterNullaryStartupFlag("unlimit_coredumps", &unlimit_coredumps); RegisterNullaryStartupFlag("watchfs", &watchfs); RegisterNullaryStartupFlag("write_command_log", &write_command_log); diff --git a/src/main/cpp/startup_options.h b/src/main/cpp/startup_options.h index 208e4bc6a96af4..496ad1e7af2a31 100644 --- a/src/main/cpp/startup_options.h +++ b/src/main/cpp/startup_options.h @@ -146,6 +146,10 @@ class StartupOptions { // Installation base for a specific release installation. std::string install_base; + // Whether to attempt to update the mtime of the provided `install_base` + // directory. + bool update_install_base_mtime; + // The toplevel directory containing Blaze's output. When Blaze is // run by a test, we use TEST_TMPDIR, simplifying the correct // hermetic invocation of Blaze from tests. diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java index 38cbff6319f02b..884bd61e274d4e 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java @@ -92,6 +92,16 @@ public String getTypeDescription() { help = "This launcher option is intended for use only by tests.") public PathFragment installBase; + @Option( + name = "update_install_base_mtime", + defaultValue = "true", // NOTE: only for documentation, value is set and used by the client. + documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS, + effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION}, + help = + "If set, the client will attempt to update the `mtime` of the `install_base` directory. " + + "Changing this option will not cause the server to restart.") + public boolean updateInstallBaseMtime; + /* * The installation MD5 - a content hash of the blaze binary (includes the Blaze deploy JAR and * any other embedded binaries - anything that ends up in the install_base). diff --git a/src/test/cpp/bazel_startup_options_test.cc b/src/test/cpp/bazel_startup_options_test.cc index 33ae9c2e8f30d2..a458478ff9f520 100644 --- a/src/test/cpp/bazel_startup_options_test.cc +++ b/src/test/cpp/bazel_startup_options_test.cc @@ -104,6 +104,7 @@ TEST_F(BazelStartupOptionsTest, ValidStartupFlags) { ExpectValidNullaryOption(options, "ignore_all_rc_files"); ExpectValidNullaryOption(options, "shutdown_on_low_sys_mem"); ExpectValidNullaryOption(options, "system_rc"); + ExpectValidNullaryOption(options, "update_install_base_mtime"); ExpectValidNullaryOption(options, "watchfs"); ExpectValidNullaryOption(options, "workspace_rc"); ExpectValidNullaryOption(options, "write_command_log"); diff --git a/src/test/shell/integration/client_test.sh b/src/test/shell/integration/client_test.sh index 19f23a7b7c83cf..0a5739cbfaac6c 100755 --- a/src/test/shell/integration/client_test.sh +++ b/src/test/shell/integration/client_test.sh @@ -49,6 +49,57 @@ function test_client_debug_change_does_not_restart_server() { expect_not_log "WARNING.* Running B\\(azel\\|laze\\) server needs to be killed" } +# Note: the install base check is elided if the `install` symlink in the output +# base matches the given install base: +# https://github.com/bazelbuild/bazel/blob/a3c677dfea2de636a719d50345a5a97af96fae60/src/main/cpp/blaze.cc#L991-L1001 +# +# To force this check to re-run, we pass a new `output_base` path on every +# invocation. +function test_update_install_base_mtime_updates_mtime() { + local install_base="$TEST_TMPDIR/install" + local args=( + --batch --update_install_base_mtime + --install_base="${install_base}" + ) + + bazel "${args[@]}" --output_base="$TEST_TMPDIR/out/a" version >&$TEST_log || fail "'bazel version' failed" + local original_mtime=$(stat -c %Y "${install_base}") + + sleep 1 + bazel "${args[@]}" --output_base="$TEST_TMPDIR/out/b" version >&$TEST_log || fail "'bazel version' failed" + local new_mtime=$(stat -c %Y "${install_base}") + + assert_not_equals "$original_mtime" "$new_mtime" +} + +# Really we'd like to test that `bazel --noupdate_install_base_mtime ...` is +# happy with a read-only install base but unfortunately it's tricky to make an +# install base that's read-only without `root` (`chattr`, `chown`, `mount`, etc. +# all need privileges). +function test_no_update_install_base_mtime_does_not_update_mtime() { + local install_base="$TEST_TMPDIR/install" + local args=( + --batch --noupdate_install_base_mtime + --install_base="${install_base}" + ) + + bazel "${args[@]}" --output_base="$TEST_TMPDIR/out/c" version >&$TEST_log || fail "'bazel version' failed" + local original_mtime=$(stat -c %Y "${install_base}") + + sleep 1 + bazel "${args[@]}" --output_base="$TEST_TMPDIR/out/d" version >&$TEST_log || fail "'bazel version' failed" + local new_mtime=$(stat -c %Y "${install_base}") + + assert_equals "$original_mtime" "$new_mtime" +} + +function test_update_install_base_mtime_change_does_not_restart_server() { + local server_pid1=$(bazel --update_install_base_mtime info server_pid 2>$TEST_log) + local server_pid2=$(bazel --noupdate_install_base_mtime info server_pid 2>$TEST_log) + assert_equals "$server_pid1" "$server_pid2" + expect_not_log "WARNING.* Running B\\(azel\\|laze\\) server needs to be killed" +} + function test_server_restart_due_to_startup_options() { local server_pid1=$(bazel --write_command_log info server_pid 2>$TEST_log) local server_pid2=$(bazel --nowrite_command_log info server_pid 2>$TEST_log)