Repository navigation
[SSH] Report port forwarding refusals as their own telemetry categories - #7064
Conversation
Integration test reportCommit: b38d473
Top 6 slowest tests (at least 2 minutes):
|
mihaimitrea-db
left a comment
mihaimitrea-db
left a comment
There was a problem hiding this comment.
Not sure why my approval is needed? Could this signal something weird into our ownership system?
@anton-107 change LGTM I can approve if this was intentional.
1ea406f to
a84b403
Compare
rugpanov
left a comment
rugpanov
left a comment
There was a problem hiding this comment.
LGTM. Before you merge, rebase this branch onto the updated base. It still contains the old commits from #7057 and #7063. Because of this, GitHub shows 18 files instead of 6 and reports a merge conflict. Run git rebase --onto origin/ssh-preflight-port-forwarding-setting a441f86cf. Your commit applies to a84b40362 without conflicts, and the SSH unit tests and the two acceptance tests pass there.
| // (port_fwd_serverless_enabled), read from the setting before any compute was started. | ||
| SshTunnelErrorCategoryPortForwardingDisabled SshTunnelErrorCategory = "PORT_FORWARDING_DISABLED" | ||
|
|
||
| // The driver proxy refused the SSH server with a 403 after its run was submitted. Usually |
There was a problem hiding this comment.
[nice to have] This comment says that the 403 comes "after its run was submitted". But ensureSSHServerIsRunning also returns errDriverProxyForbidden from its first getServerMetadata call, before it submits a run. This occurs when a metadata.json file from an earlier connection exists. Thus a person who reads the data can think that each DRIVER_PROXY_FORBIDDEN row started compute. Change the text to "refused a request to the SSH server with a 403, for a server that was already running or for one that this connection started". Make the same change in the enum.proto comment in databricks-eng/universe#2802257 and in the table in the PR description.
There was a problem hiding this comment.
Done in b38d473. You're right that the first getServerMetadata call returns the 403 before any submit when an earlier connection's metadata.json exists. I updated the comment and the PR description table. I'll update the enum.proto comment in databricks-eng/universe#2802257 separately.
Both refusals from the previous changes were reported as SERVER_START_TIMEOUT (the driver proxy's 403) or UNKNOWN (the up-front setting check), so they looked like slow server starts or CLI bugs. They now report PORT_FORWARDING_DISABLED when the setting check stops the connection before any compute is started, and DRIVER_PROXY_FORBIDDEN when the driver proxy refuses the server after its run was submitted. Keeping them apart shows how often the setting can't be read and the proxy has to catch it. The values need the matching universe enum change to be stored. DECO-28889 Co-authored-by: Isaac <no-reply@databricks.com>
…erver Co-authored-by: Isaac <no-reply@databricks.com>
3451727 to
b38d473
Compare
4 participants
Changes
Two new
SshTunnelErrorCategoryvalues for the port forwarding refusals added in #7057 and #7063:PORT_FORWARDING_DISABLEDport_fwd_serverless_enabledreturnsfalse, before any compute is startedUNKNOWNDRIVER_PROXY_FORBIDDENSERVER_START_TIMEOUTBoth acceptance tests now also check the
error_categoryin the uploaded telemetry event.Why
PR 3 of DECO-28889, after #7057 and #7063. Without these, a disabled setting shows up as slow server starts or as CLI bugs. Keeping the two apart shows how often the setting can't be read (production today) and the proxy's 403 has to catch it.
The backend enum needs the matching values: https://github.com/databricks-eng/universe/pull/2802257. Either PR can land first; until the universe one does, ingestion drops these values.
Tests
acceptance/ssh/connect-serverless-port-forwarding-disabledreportsPORT_FORWARDING_DISABLED.acceptance/ssh/connect-serverless-feature-disabledreportsDRIVER_PROXY_FORBIDDEN.acceptance/sshtests andexperimental/ssh/...unit tests pass.This pull request and its description were written by Isaac.