Preserve SSH descendant filesystem access after bootstrap exit - #6645
Conversation
Integration test reportCommit: b9e2b42
Top 7 slowest tests (at least 2 minutes):
|
| } | ||
|
|
||
| func registeredPID() (int, error) { | ||
| content, err := os.ReadFile("/Workspace/.proc/self/metadata/pid") |
There was a problem hiding this comment.
[nice-to-fix] registeredPID() reads /Workspace/.proc/self/metadata/pid with no timeout, and it runs on every tick of the refresh loop. If WSFS is wedged in a way that makes the read hang rather than return an error, this goroutine blocks indefinitely — it stops refreshing the volumes registration and stops observing ctx.Done(). (The error case is fine and already tested: err != nil just forces re-registration.)
The coupling is the nasty part: a hung workspace-files mount would also freeze the volumes keepalive, which is otherwise independent and would keep recovering across a UC-FUSE restart.
Suggest bounding the read so a stuck FUSE mount can't pin the loop — e.g. do the read under a short context.WithTimeout (small goroutine + select on ctx.Done()), and treat a timeout the same as the existing error path (force = true). Low-probability trigger, but cheap insurance for a background loop that's meant to be self-healing.
| "github.com/databricks/cli/libs/log" | ||
| ) | ||
|
|
||
| const refreshInterval = time.Minute |
There was a problem hiding this comment.
[question] The PR description says registration is checked "every ten minutes," but the code (and the README / FAILURE_MODES docs) use 60 seconds. The code and docs agree — it's the description that's out of step — so this is just a confirmation:
Is 60s the intended cadence? Since volumes register with refreshUnchanged: true, every tick issues an unconditional PUT, i.e. ~60 volume registrations/hour per SSH server for the lifetime of the tunnel. If that load is fine with the UC-FUSE daemon owners, all good — just update the PR description. If ten minutes was the intent, the constant needs to change.
There was a problem hiding this comment.
yes, it is set to 60 seconds now. updated the PR description
Integration test reportCommit: 3e43be0
44 interesting tests: 25 flaky, 19 FAIL
Top 50 slowest tests (at least 2 minutes):
|
## Release v1.18.0 ### CLI * The AI Runtime commands have moved to `databricks air`. The previous `databricks experimental air` path now directs users to the new command. ([#6722](#6722)) * Write local state, cache, and config files atomically so an interrupted or concurrent write cannot corrupt them. ([#6708](#6708)) * Deprecate `--region` in `databricks auth docker configure` ahead of its removal in the next release, infer the Artifact Registry region when it is omitted, and add `databricks auth docker host --profile <name>` to show the profile's registry host and credential-helper status. ([#6782](#6782)) * Return `UNAUTHENTICATED` instead of `INVALID_REFRESH_TOKEN` when `databricks auth token --output json` cannot refresh a cached U2M token. ([#6731](#6731)) * Retry the current-user (SCIM `Me`) lookup on transient HTTP 500 responses so a temporarily-unavailable backend no longer fails bundle commands outright. ([#6766](#6766)) * Preserve workspace-file and volume access for SSH server descendants when the bootstrap notebook exits and the server survives. ([#6645](#6645)) * `ssh connect` and `ssh setup` now accept a `--keep-detached-processes` flag to keep processes detached from the SSH session (`tmux`, `setsid`, `nohup`) running after the tunnel shuts down. Teardown then terminates only the tunnel's own process group, and the bootstrap job run is held open while any detached process is still running, so the survivors keep their `/Workspace` and `/Volumes` access. A held-open run also suppresses cluster autotermination, so the flag is off by default, is bounded by `--server-timeout`, and is dedicated-cluster only. Without it, the server now logs a warning naming the detached processes it is about to destroy, instead of sweeping them silently. ([#6387](#6387)) ### Bundles * direct: Allow clearing a catalog's or schema's `custom_max_retention_hours` by removing it from configuration. ([#6792](#6792)) * direct: Allow clearing a genie space's `description` and a secret's `comment` by removing them from configuration. ([#6789](#6789)) * Fix direct-engine deploy recreating an MLflow experiment on every deploy when its `trace_location` was set out-of-band. ([#6787](#6787)) * direct: Store a Genie space's `serialized_space` in state as a content hash instead of its full contents. ([#6707](#6707)) * Fix `bundle deploy` failing with "Invalid python file reference" for jobs that use `git_source` with a `spark_python_task` on the direct engine. ([#6751](#6751)) * Fixed the direct engine mishandling UC grants that combine `ALL_PRIVILEGES` with a privilege it does not imply (`MANAGE`, `READ_METADATA`, `EXTERNAL_USE_SCHEMA`, `EXTERNAL_USE_LOCATION`): such privileges were dropped when granted and left behind when revoked, so the deployment never converged. ([#6733](#6733), [#6743](#6743)) * Don't fail migration if clean up actions fail. ([#6772](#6772)) * Ignore the backend-provided `spark.sql.ansi.enabled: "true"` pipeline configuration default when detecting direct-engine drift. ([#6816](#6816)) * Fix recreating a postgres synced table sometimes failing with a 409 ALREADY_EXISTS error while the previous table is still being deleted. ([#6728](#6728)) * Direct engine no longer recreates a resource when an immutable field the config omits was populated by the backend. ([#6790](#6790)) ### Dependency Updates * Bump dependencies with known vulnerabilities. ([#6723](#6723)) * Bump `github.com/databricks/databricks-sdk-go` from v0.178.0 to v0.182.0. ([#6817](#6817)) * Bump the Databricks Terraform provider from v1.132.0 to v1.134.0. ([#6818](#6818))
## Changes
On serverless, the SSH tunnel server no longer tries to register itself
with the WSFS and UC-FUSE daemons. It logs one Info line at startup and
skips the self-lookup, the current-user lookup, both daemon PUTs, and
the one-minute refresh loop:
```
Skipping SSH filesystem registration on serverless; /Workspace and /Volumes access depends on the bootstrap notebook
```
- New helper `startFuseRegistration(ctx, client, serverless)` in
`internal/server/fuse.go`. The existing registration block and its
warning moved out of `Run` without changes. `Run` now calls the helper
with `opts.Serverless`.
- The `ssh server --serverless` help text now says "Whether the server
runs on serverless compute".
- README ("Filesystem access") and FAILURE_MODES.md ("Filesystem access
after the bootstrap notebook exits") now describe the serverless
behaviour.
- Dedicated clusters behave exactly as before. The `fuse` package is
unchanged.
How `Run` changes at startup:
```diff
server.Run
- registerFuseCredentials # always ran, warned on failure
+ startFuseRegistration(opts.Serverless)
+ if serverless
+ log.Info "Skipping SSH filesystem registration on serverless; ..."
+ return
+ registerFuseCredentials # dedicated only, unchanged
+ fuse.Self # read own PID + start time from /proc
+ fuseUserInfo # CurrentUser.Me, 2s timeout
+ fuse.KeepRegistered # PUT to WSFS :1021 and UC-FUSE :1015, refresh every 60s
findAvailablePort
SaveWorkspaceMetadata
```
Where filesystem access comes from, by compute type:
```mermaid
flowchart LR
subgraph Dedicated
R1[bootstrap REPL<br/>registers itself] --> S1[tunnel server]
S1 -- registers own PID<br/>every 60s --> D1[WSFS / UC-FUSE]
S1 --> SH1[SSH sessions<br/>keep access after REPL exits]
end
subgraph Serverless
R2[bootstrap REPL<br/>registers as root] --> D2[WSFS / UC-FUSE]
R2 --> S2[tunnel server<br/>skips registration]
S2 --> SH2[SSH sessions<br/>covered as REPL descendants]
S2 -. non-root UID refused<br/>SDR-3124 .-x D2
end
```
## Why
Skipping costs nothing. The bootstrap REPL's own registration runs as
root and already covers the tunnel server and its descendants, and the
container is torn down 35–60s after the REPL exits. The skip depends on
the compute type the client already passes to the server, not on the
error, so re-enabling registration later is one deliberate change. The
regression came from databricks#6645.
No changelog fragment: the databricks#6645 entry ships as written.
## Tests
- New unit test `TestStartFuseRegistrationSkipsServerless`. Any HTTP
request fails it, which proves the current-user lookup is skipped. It
checks that the skip line is logged and nothing at WARN or above. It
touches neither `/proc` nor the network, so it runs on Linux, macOS and
Windows.
- `go test ./experimental/ssh/...` passes, except
`TestSSHServerBootstrap`, which fails locally on Python 3.6. That
failure existed before this change.
- `./task lint-q` and `./task ws` are clean.
- Not yet run on real compute: the dogfood checks from the spec
(serverless CPU, serverless GPU_1xA10, dedicated control).
This pull request and its description were written by Isaac.
Changes
Register the SSH server process with the workspace-file and volume daemons. Check credentials and registration every minute, retry failures, and avoid updates when nothing changed. Bound daemon requests and keep SSH startup available when registration fails.
Why
SSH descendants need filesystem access when the bootstrap notebook exits while the server survives. Registration does not extend the server lifetime, preserve detached processes, or renew a fixed bootstrap credential. Document these limits and troubleshooting steps.
Tests
This PR was written with Codex.