Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

roachprod: ensure scp uses supported flags #99430

Merged
merged 1 commit into from
Apr 3, 2023

Conversation

AlexTalks
Copy link
Contributor

In #99283 the flags -R -A were added to the scp command invocation, in order to ensure that copies between two remote hosts are done without transferring to the localhost. These flags are only supported on MacOS ("darwin") and not on Linux, as this behavior is already the default on Linux. This change fixes that issue by only using these flags on MacOS, allowing the scp invocation (and by extension, roachprod) to work on Linux.

Epic: none

Release note: None

@AlexTalks AlexTalks requested a review from a team as a code owner March 23, 2023 20:36
@AlexTalks AlexTalks requested review from herkolategan and smg260 and removed request for a team March 23, 2023 20:36
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@renatolabs
Copy link
Contributor

We're already in the process of reverting that change (#99419) so tonight's run should be safe.

If we want to conditionally use it like proposed here, let's first make sure that:

  • @aliher1911 do you get good performance with this code? I don't see why not, but let me know.
  • I'll also run some roachtest runs on this branch to make sure everything is good. I think this should work fine with tree dist, but we should double check.

@aliher1911
Copy link
Contributor

I looked at

~/g/s/g/c/cockroach ❯❯❯ go tool dist list
aix/ppc64
android/386
android/amd64
android/arm
android/arm64
darwin/amd64
darwin/arm64
dragonfly/amd64
freebsd/386
freebsd/amd64
freebsd/arm
freebsd/arm64
illumos/amd64
ios/amd64
ios/arm64
js/wasm
linux/386
linux/amd64
linux/arm
linux/arm64
linux/loong64
linux/mips
linux/mips64
linux/mips64le
linux/mipsle
linux/ppc64
linux/ppc64le
linux/riscv64
linux/s390x
netbsd/386
netbsd/amd64
netbsd/arm
netbsd/arm64
openbsd/386
openbsd/amd64
openbsd/arm
openbsd/arm64
openbsd/mips64
plan9/386
plan9/amd64
plan9/arm
solaris/amd64
windows/386
windows/amd64
windows/arm
windows/arm64

and thinking if we need to include bsd family here? Or just keep "darwin" and be done with it?
I was testing it from linux box and scp works as expected and does direct host to host copy without those flags despite the same confusing man page.

@AlexTalks
Copy link
Contributor Author

Do we otherwise test and support roachprod on BSD? It feels like the common usage pattern is that it's used from MacOS and Linux. I realize that roachprod should be usable by any and all contributors, but given that we don't ship roachprod binaries, I'm not sure if we really need to do anything more that just checking runtime.GOOS == "darwin".

@aliher1911 aliher1911 self-requested a review March 24, 2023 12:35
Copy link
Contributor

@aliher1911 aliher1911 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing my sloppy engineering!

"-o", "StrictHostKeyChecking=no",
}
if runtime.GOOS == "darwin" {
args = append(args, []string{"-R", "-A"}...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just do

args = append(args, "-R", "-A")

@aliher1911
Copy link
Contributor

aliher1911 commented Mar 25, 2023

I actually dug a bit further by accident and figured that flags are supported by openssl 9 and not supported by openssl 7. Ubuntu/Debian that we use and I also have on one server have v7, while macos have 9 and I also have arch which is also using 9 and has all the flags. So it is not bsd specific. Ideally we should just check version, but I don't think there's an easy way beside doing something like ssh -vv localhost </dev/null and parsing version from stderr. Meh.

@renatolabs
Copy link
Contributor

I actually dug a bit further by accident and figured that flags are supported by openssl 9 and not supported by openssl 7. Ubuntu/Debian that we use and I also have on one server have v7, while macos have 9 and I also have arch which is also using 9 and has all the flags. So it is not bsd specific. Ideally we should just check version, but I don't think there's an easy way beside doing something like ssh -vv localhost </dev/null and parsing version from stderr. Meh.

Good catch! It failed on my gceworker and TeamCity and succeeded on OSX so I jumped to the wrong conclusion 🙂

It should be possible to parse the version relatively easily with ssh -V

@AlexTalks AlexTalks force-pushed the fix_linux_roachprod branch 2 times, most recently from 83a5603 to f151d17 Compare April 3, 2023 18:56
In cockroachdb#99283 the flags `-R -A` were added to the `scp` command invocation,
in order to ensure that copies between two remote hosts are done without
transferring to the localhost. These flags are only supported on MacOS
(`"darwin"`) and not on Linux, as this behavior is already the default
on Linux. This change fixes that issue by only using these flags on
MacOS, allowing the `scp` invocation (and by extension, roachprod) to
work on Linux.

Epic: none

Release note: None
@AlexTalks AlexTalks force-pushed the fix_linux_roachprod branch from f151d17 to 2785011 Compare April 3, 2023 19:04
@AlexTalks
Copy link
Contributor Author

bors r+

@AlexTalks
Copy link
Contributor Author

It should be possible to parse the version relatively easily with ssh -V

I added this as a TODO but wanted to merge this fix for now.

@craig
Copy link
Contributor

craig bot commented Apr 3, 2023

Build succeeded:

@craig craig bot merged commit 8e9273c into cockroachdb:master Apr 3, 2023
@AlexTalks AlexTalks deleted the fix_linux_roachprod branch April 3, 2023 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants