ssh: always re-upload dev builds of the tunnel binary - #6820
Conversation
uploadReleases skipped the upload whenever the binary already existed at the versioned workspace path. Dev and snapshot builds keep the same version string when rebuilt without a new commit, so the rebuilt binary was silently not uploaded and the cluster kept running the old one. Dev versions now always overwrite; release versions still skip. Co-authored-by: Isaac <no-reply@databricks.com>
Integration test reportCommit: 51ee98a
Top 4 slowest tests (at least 2 minutes):
|
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
Review (automated, 1 agent(s))
Verdict: Approved
0 Critical | 0 Major | 1 Gap (Nit) | 0 Nit | 0 Suggestion
See inline comments for details.
| func TestUploadReleasesWithExistingBinary(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| version string | ||
| wantUploaded []string | ||
| }{ | ||
| { | ||
| name: "release version skips the upload", | ||
| version: "1.12.0", | ||
| wantUploaded: nil, | ||
| }, | ||
| { | ||
| name: "dev version overwrites the binary", | ||
| version: "1.12.1-dev+abcdef123456", | ||
| wantUploaded: []string{"amd64", "arm64"}, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| ctx := t.Context() | ||
| workspaceFiler, err := filer.NewLocalClient(t.TempDir()) | ||
| require.NoError(t, err) | ||
| for _, arch := range []string{"amd64", "arm64"} { | ||
| remoteBinaryPath := strings.TrimSuffix(getReleaseName(arch, tt.version), ".zip") + "/databricks" | ||
| err := workspaceFiler.Write(ctx, remoteBinaryPath, strings.NewReader("old"), filer.CreateParentDirectories) | ||
| require.NoError(t, err) | ||
| } | ||
|
|
||
| var uploaded []string | ||
| getRelease := func(ctx context.Context, architecture, version, releasesDir string) (io.ReadCloser, error) { | ||
| uploaded = append(uploaded, architecture) | ||
| return io.NopCloser(strings.NewReader("new")), nil | ||
| } | ||
|
|
||
| err = uploadReleases(ctx, workspaceFiler, getRelease, tt.version, "") | ||
| require.NoError(t, err) | ||
| assert.Equal(t, tt.wantUploaded, uploaded) |
There was a problem hiding this comment.
[Gap (Nit)] Test asserts getRelease was called, not that overwrite landed
TestUploadReleasesWithExistingBinary does fail without the skip fix: both cases seed remoteBinaryPath, so Stat succeeds, and the pre-fix loop would continue for the dev version too. That part is good, and the release case is a useful guard against “always upload.” It does not, however, assert overwrite. After uploadReleases the test only compares the uploaded slice; it never Reads anything. Setup writes "old" to {release}/databricks (the Stat path). Production writes "new" to {release}/databricks.zip with filer.OverwriteIfExists. Those are different paths. Under LocalClient, the zip path does not exist yet, so Write would succeed even without OverwriteIfExists. The case named “dev version overwrites the binary” therefore does not show that new bytes replaced old ones. LocalClient also does not unzip, so it cannot honestly assert the databricks file content anyway.
Suggestion: After a successful upload, for the dev case Read {release}/databricks.zip and assert the body is "new"; for the release case assert that zip was not created and {release}/databricks is still "old". Stronger still: also pre-seed the zip path with "old" so dropping OverwriteIfExists fails locally. Do not assert {release}/databricks became "new" in this unit test — that only happens via workspace import-file unzip.
rclarey
left a comment
rclarey
left a comment
There was a problem hiding this comment.
Manually tested with --debug and works as expected ✅
4 participants
Changes
uploadReleasesno longer skips the upload for dev and snapshot versions (build.IsDevelopmentVersion) when the binary already exists at the versioned workspace path. Release versions still skip the upload.Also updates the rule in
experimental/ssh/CLAUDE.mdthat said to delete the versioned workspace directory before re-verifying. After this change, the remaining pitfall is thatconnectreuses a server that's already running the previous binary.Why
Dev and snapshot builds keep the same version string when rebuilt without a new commit. The rebuilt binary was silently not re-uploaded, and the cluster kept running the old one. Re-uploading dev builds every time costs only CLI developers.
Possible follow-up, out of scope here:
getReleaseNamestill detects dev versions withstrings.Contains(version, "dev")instead ofbuild.IsDevelopmentVersion.Tests
TestUploadReleasesWithExistingBinary. The dev version re-uploads both arches, and the release version skips. It fails without the fix.TestSSHServerBootstrapfails locally onmaintoo, because the local interpreter is Python 3.6. That failure is unrelated to this change.This pull request and its description were written by Isaac.