roachprod: better determination if scp -R flag can be used #109036
Conversation
|
This change is |
RaduBerinde
force-pushed
the
roachprod-scp-treedist-fix
branch
from
2bfc137 to
506552b
Compare
RaduBerinde
changed the title
| cmd := exec.Command("ssh", "-V") | ||
| out, err := cmd.CombinedOutput() | ||
| if err != nil { | ||
| panic(fmt.Sprintf("error running ssh -V: %v", err)) |
There was a problem hiding this comment.
This should be rare, think it's worth logging the output as well?
| if err != nil { | ||
| panic(fmt.Sprintf("error running ssh -V: %v", err)) | ||
| } | ||
| sshVersion3Internal.value = strings.Contains(string(out), "SSL 3.") |
There was a problem hiding this comment.
We could try parsing to version to future-proof this against version 4. But eh, I suppose that is next decades problem.
There was a problem hiding this comment.
Plus if history is any guide, next version might be 5. Or 9? :)
|
Neat! That would also match versions strings on Debian and Arch. |
RaduBerinde
force-pushed
the
roachprod-scp-treedist-fix
branch
from
506552b to
83cdf9c
Compare
There was a problem hiding this comment.
TFTR!
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @AlexTalks, @aliher1911, @herkolategan, @smg260, and @stevendanna)
pkg/roachprod/install/cluster_synced.go line 2454 at r1 (raw file):
Previously, stevendanna (Steven Danna) wrote…
This should be rare, think it's worth logging the output as well?
Done.
RaduBerinde
force-pushed
the
roachprod-scp-treedist-fix
branch
from
83cdf9c to
ae1f902
Compare
|
bors r+ |
| if c.UseTreeDist && sshVersion3() { | ||
| detail = " (dist)" |
There was a problem hiding this comment.
I'm confused -- this diff is changing the detail that is logged when transferring files, but not changing the actual logic of transferring these files. IIUC, this will change message logged on TC nightly builds (where agents run Ubuntu 20.04) but that's confusing since the transferring algorithm won't have changed.
Am I missing something?
|
bors r- |
|
Canceled. |
When uploading a file to a cluster, we use the "tree dist" algorithm by default. This uploads the file to a single node, then we copy the file from that node to the other nodes (up to 10). This only makes sense if the remote-to-remote transfers can happen directly, which only happens if we pass the `-R -A` flags to `scp`. Unfortunately older versions don't support these flags. Currently the flags are only passed if the OS is `darwin`. This commits improves the determination - we run `ssh -V` (once) and check if the `SSL` major version is three. For reference, some examples of what `ssh -V` returns: - recent MacOSX: `OpenSSH_9.0p1, LibreSSL 3.3.6` - Ubuntu 22.04: `OpenSSH_8.9p1 Ubuntu-3ubuntu0.3, OpenSSL 3.0.2 15 Mar 2022` In addition, if the version is not 3, we disable the use of "tree dist". Epic: none Release note: None
RaduBerinde
force-pushed
the
roachprod-scp-treedist-fix
branch
from
ae1f902 to
4240ac6
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/roachprod/install/cluster_synced.go line 1757 at r2 (raw file):
Previously, renatolabs (Renato Costa) wrote…
I'm confused -- this diff is changing the
detailthat is logged when transferring files, but not changing the actual logic of transferring these files. IIUC, this will change message logged on TC nightly builds (where agents run Ubuntu 20.04) but that's confusing since the transferring algorithm won't have changed.Am I missing something?
Ah.. oops, 😊 Fixed, PTAL
|
bors r+ |
|
Build succeeded: |
When uploading a file to a cluster, we use the "tree dist" algorithm by default. This uploads the file to a single node, then we copy the file from that node to the other nodes (up to 10).
This only makes sense if the remote-to-remote transfers can happen directly, which only happens if we pass the
-R -Aflags toscp. Unfortunately older versions don't support these flags. Currently the flags are only passed if the OS isdarwin.This commits improves the determination - we run
ssh -V(once) and check if theSSLmajor version is three. For reference, some examples of whatssh -Vreturns:OpenSSH_9.0p1, LibreSSL 3.3.6OpenSSH_8.9p1 Ubuntu-3ubuntu0.3, OpenSSL 3.0.2 15 Mar 2022In addition, if the version is not 3, we disable the use of "tree dist".
Epic: none
Release note: None