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

Add client option to disable updating the mtime of the install_base #20373

Conversation

rrbutani
Copy link
Contributor

@rrbutani rrbutani commented Nov 30, 2023

Currently if the --install_base path passed is not writeable by the user invoking Bazel, the Bazel client crashes1, even if the path already contains a suitable extracted Bazel installation:

bazel --install_base=/some/read/only/path/with/a/pre-existing/bazel/install version
FATAL: failed to set timestamp on '/some/read/only/path/with/a/pre-existing/bazel/install': (error: 30): Read-only file system

This happens because the Bazel client (unconditionally) attempts to update the mtime of this path:

bazel/src/main/cpp/blaze.cc

Lines 1010 to 1021 in a3c677d

// 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<blaze_util::IFileMtime> 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;
}
}
}

This PR gates this mtime update on a new client startup option: --update_install_base_mtime.


This commit:

  • adds the update_install_base_mtime option to startup_options.cc and BlazeServerStartupOptions.java
  • adds logic to src/main/cpp/blaze.cc to ensure that changes to this option do not cause server restarts
  • updates bazel_startup_options_test.cc
  • 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:

Footnotes

  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.

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
@github-actions github-actions bot added team-Rules-CPP Issues for C++ rules awaiting-review PR is awaiting review from an assigned reviewer labels Nov 30, 2023
@tjgq
Copy link
Contributor

tjgq commented Nov 30, 2023

Do we really need a flag for this? Would it suffice to have the mtime call ignore EROFS (and possibly also EPERM, for good measure)? This seems to only matter for cleanup, so there's no build correctness at stake.

@rrbutani
Copy link
Contributor Author

Do we really need a flag for this? Would it suffice to have the mtime call ignore EROFS (and possibly also EPERM, for good measure)? This seems to only matter for cleanup, so there's no build correctness at stake.

Sure, ignoring the error works just as well for my use case.

I'm not clear on what cleanup tooling the comment is referring to but regardless it does seem fairly unlikely that not erroring would break people's flows in a significant way.


As a counterpoint though: would we still want to emit a warning when the mtime update fails? If so, it'd be nice to have a flag to silence the warning...

(though, this — changes to the pair of install base, output base paths — happens infrequently enough that I'm willing to compromise on ^, especially if adding an extra startup flag will require an involved internal process or create extra work for the core team)

@rrbutani
Copy link
Contributor Author

I also wasn't able to come up with a way to produce a read-only install base for use with the integration tests without privileges (whereas testing whether the mtime is updated is straightforward) but that matters less I suppose; if we switch to ignoring the error I assume we'd be okay not having a test for this particular thing.

@tjgq
Copy link
Contributor

tjgq commented Dec 4, 2023

I wouldn't insist on having a test for the read-only case, and a warning seems unwarranted as well. This only affects the ability of external tooling to garbage collect old install bases, but if your install base is read-only, then it can't be garbage collected pretty much by definition :)

Would you like to either modify this PR, or send a separate one with the simpler change?

@rrbutani
Copy link
Contributor Author

rrbutani commented Dec 4, 2023

I wouldn't insist on having a test for the read-only case, and a warning seems unwarranted as well. This only affects the ability of external tooling to garbage collect old install bases, but if your install base is read-only, then it can't be garbage collected pretty much by definition :)

👍 Sounds good to me.

Would you like to either modify this PR, or send a separate one with the simpler change?

I'll open a separate PR later today.

rrbutani added a commit to rrbutani/bazel that referenced this pull request Dec 5, 2023
Currently if the `--install_base` path passed is not writable 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 commit updates the client to ignore such errors. See bazelbuild#20373 for
context.
@rrbutani
Copy link
Contributor Author

rrbutani commented Dec 5, 2023

Superseded by #20442.

@rrbutani rrbutani closed this Dec 5, 2023
@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Dec 5, 2023
rrbutani added a commit to rrbutani/bazel that referenced this pull request Dec 12, 2023
Currently if the `--install_base` path passed is not writable 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#L1010-L1021

[^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#L983-L993
[created]: https://github.com/bazelbuild/bazel/blob/a3c677dfea2de636a719d50345a5a97af96fae60/src/main/cpp/blaze.cc#L1003

This commit updates the client to ignore such errors. See bazelbuild#20373 for
context.
copybara-service bot pushed a commit that referenced this pull request Dec 15, 2023
Currently if the `--install_base` path passed is not writable by the user invoking Bazel, the Bazel client crashes:
```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#L1010-L1021

This commit updates the client to ignore such errors. See #20373 for context.

Closes #20442.

PiperOrigin-RevId: 591266054
Change-Id: If53e7cad48cb62406f7883f72b413e4b5a0bb8e2
bazel-io pushed a commit to bazel-io/bazel that referenced this pull request Dec 15, 2023
Currently if the `--install_base` path passed is not writable by the user invoking Bazel, the Bazel client crashes:
```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#L1010-L1021

This commit updates the client to ignore such errors. See bazelbuild#20373 for context.

Closes bazelbuild#20442.

PiperOrigin-RevId: 591266054
Change-Id: If53e7cad48cb62406f7883f72b413e4b5a0bb8e2
bazel-io pushed a commit to bazel-io/bazel that referenced this pull request Dec 21, 2023
Currently if the `--install_base` path passed is not writable by the user invoking Bazel, the Bazel client crashes:
```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#L1010-L1021

This commit updates the client to ignore such errors. See bazelbuild#20373 for context.

Closes bazelbuild#20442.

PiperOrigin-RevId: 591266054
Change-Id: If53e7cad48cb62406f7883f72b413e4b5a0bb8e2
github-merge-queue bot pushed a commit that referenced this pull request Jan 9, 2024
…tall_base` (#20568)

Currently if the `--install_base` path passed is not writable by the
user invoking Bazel, the Bazel client crashes:
```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#L1010-L1021

This commit updates the client to ignore such errors. See #20373 for
context.

Closes #20442.

Commit
7f782e3

PiperOrigin-RevId: 591266054
Change-Id: If53e7cad48cb62406f7883f72b413e4b5a0bb8e2

Co-authored-by: Rahul Butani <rrbutani@users.noreply.github.com>
Co-authored-by: Ian (Hee) Cha <heec@google.com>
github-merge-queue bot pushed a commit that referenced this pull request Jan 11, 2024
…tall_base` (#20648)

Currently if the `--install_base` path passed is not writable by the
user invoking Bazel, the Bazel client crashes:
```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#L1010-L1021

This commit updates the client to ignore such errors. See #20373 for
context.

Closes #20442.

Commit
7f782e3

PiperOrigin-RevId: 591266054
Change-Id: If53e7cad48cb62406f7883f72b413e4b5a0bb8e2

Co-authored-by: Rahul Butani <rrbutani@users.noreply.github.com>
Co-authored-by: Ian (Hee) Cha <heec@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants