Use local_termination_grace_seconds when testing LinuxSandbox availability

[tsawada]

A 1s timeout was introduced in checking whether LinuxSandbox is available, to prevent a complete hangup on broken systems. However, it turned out that it occasionally results in misjudging that linux-sandbox being not available.
local_termination_grace_seconds defaults to 15s, which hopefully gives more headroom and configurability in various setups.

Fixes #18071

[tsawada]

CC @meisterT

[meisterT]

cc @larsrc-google

This whole situation sounds like a dilemma to me. Perhaps we need to take a step back and think about other ways to detect whether sandboxing is available?

[tsawada]

IMHO, some form of timeout is necessary to prevent complete freezes even when we use other ways to detect sandboxes, but simply it should have large enough headroom so that it'd never affect the normal execution paths on properly setup environments.

[meisterT]

If we keep the check as is, then yes we need a timeout and your case shows that it should be higher.

However, I was wondering if there is any other way to check whether this check makes sense...

[tsawada]

I agree there could be a way and that'd be cleaner, but this 1s timeout is currently blocking us from upgrading to v6.0+, and I believe this is affecting more users (by occasionally falling back to weaker sandboxes).
Would it be possible to merge this for now? If you think 30s is excessive, maybe even 10s would make the situation better for us.

[larsrc-google]

Actually, let's just do the 30s and see if it saves you. It's pretty rare that the check actually hangs and needs the timeout (#15373 is the only case we know of). That said, @sluongng's advice on the bug is valid - your system is in a bad shape if running a no-op command takes >1s. One could argue that the 1s timeout works as a warning that something else needs to be fixed, it's just a pretty obscure warning.

[sluongng]

This might cause Bazel to start up taking up to 30 seconds to complete though.

At the very least, switch to use local_termination_grace_seconds value would make it configurable.

[sluongng]

My opinion is that this is one of those "user is holding it wrong (because the tool is hard to hold)" kinda case.

I object to merging the change as is and I would urge the author to follow my comments at #18071.

For this change to merge, I would prefer if we (a) allow the value to be configurable and (b) leave the default value as-is.
The flag key should be either local_termination_grace_seconds or a new key that's close to it as the functionality of the 2 flags is somewhat similar: wait for timeout and kill the sandbox.

[larsrc-google]

local_termination_grace_seconds is not unreasonable here. The difference between its default and this PR is relatively small. I'll send in a change for that.

[tsawada]

@larsrc-google PTAL
Edited the PR title and description. Lmk if this should be another PR.

[tsawada]

@larsrc-google ping 😃

[tsawada]

@larsrc-google @sgowroji Thanks! It'd be great if this is taken / backported into 6.x releases. Please let me know if there's anything else I should do.

[larsrc-google]

Yes, please import it, and if possible backport to the latest 6.X release.

[iancha1992]

@bazel-io fork 6.3.0

[iancha1992]

@larsrc-google @sluongng @tsawada
There is a merge conflict when the cherry-pick to release-6.3.0 was attempted (from src/main/java/com/google/devtools/build/lib/sandbox/BUILD). Could you please submit a PR or let us know what commits to include?

[tsawada]

@iancha1992 For release-6.3.0 we only need to cherry-pick 5cac33f . Please skip 39b7d6d