roachprod: add command to self update the roachprod binary #103307

smg260 · 2023-05-15T15:24:09Z

This PR contains 2 ways to get the latest roachprod.

update command as part of roachprod itself:

roachprod update will check and download the latest binary for the current platform, and optionally update the running roachprod.

This uses the TeamCity REST API with guest authentication to find the latest successful build (on master) from which to download the binary.

When proceeding with an update, the existing binary is renamed with a .bak prefix so that it may be reverted, either manually or via roachprod update --revert. Permissions are copied from the existing binary.
scripts/roachprod-get-latest.sh shell script

Downloads the latest roachprod binary from TeamCity to the specified (or default current) directory. Has basic checks for curl and confirming whether to overwrite any existing roachprod.

The builds used by both the binary, and the script are here

Note:

The linter prevents direct use of http.Get, so that meant creating proto files for the TeamCity rest API responses.
The existing httputil has no accommodations for unmarshalling a subset of fields in a json response, hence the addition of an IgnoreUnknownFields option.
This should work as long as our build remain public

Epic: none
Fixes: #97311

Release note: None

cockroach-teamcity · 2023-05-15T15:24:20Z

This change is

blathers-crl · 2023-05-15T21:22:53Z

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

`roachprod update` will check and download the latest binary for the current platform, and optionally update the running roachprod. This uses the TeamCity guestAuth API to find the latest successful build (currently just master) from which to download the binary. When proceeding with an update, the existing binary is renamed with a `.bak` prefix so that it may be reverted, either manually or via `roachprod update --revert`. Permissions are copied from the existing binary. Epic: none Fixes: cockroachdb#97311 Release note: None

herkolategan

Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @renatolabs, @smg260, and @srosenberg)

pkg/cmd/roachprod/upgrade/teamcity.go line 77 at r1 (raw file):

// downloadRoachprod downloads the roachprod binary from the build
// to the specified destination file.
func downloadRoachprod(buildId int32, destFile string) error {

Super nit: https://github.com/golang/go/wiki/CodeReviewComments#initialisms
buildId should probably be buildID but I've seen so many examples of zzzId throughout the codebase this falls into a super super nit category.

pkg/cmd/roachprod/upgrade/teamcity.go line 108 at r1 (raw file):

}

func roachprodDownloadUrl(buildId int32) string {

Nit: URL Same as above https://github.com/golang/go/wiki/CodeReviewComments#initialisms

pkg/cmd/roachprod/upgrade/util.go line 44 at r1 (raw file):

		}
		return err
	} else if destInfo.IsDir() {

Nit: above branch seems to always return, hence can probably drop the else if

smg260 · 2023-05-30T13:49:15Z

pkg/cmd/roachprod/upgrade/teamcity.go line 108 at r1 (raw file):

Previously, herkolategan (Herko Lategan) wrote…

Nit: URL Same as above https://github.com/golang/go/wiki/CodeReviewComments#initialisms

Right you are, the java-isms are hard to stamp out.

smg260 · 2023-05-30T13:49:38Z

pkg/cmd/roachprod/upgrade/util.go line 44 at r1 (raw file):

Previously, herkolategan (Herko Lategan) wrote…

Nit: above branch seems to always return, hence can probably drop the else if

Yet another one - good point

srosenberg

I like the new feature! However, it seems more effective to run the "latest" check automatically and warn the user if there happens to be a newer version. Given that we don't really version roachprod, you'd need to check git blame. The simplest hack that comes to mind is to parse out the latest-commit-details (html) div tag which you see in the upper-right corner when rendering [1]. Granted, it's really a convenience (UX) feature, we should probable keep it simple, and let the user upgrade if and when they run into problems with their current (roachprod) version.

[1] https://github.com/cockroachdb/cockroach/tree/master/pkg/cmd/roachprod

Reviewed 10 of 12 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @renatolabs and @smg260)

pkg/cmd/roachprod/upgrade/teamcity.go line 33 at r2 (raw file):

		"windows-amd64": "Cockroach_UnitTests_BazelBuildWindowsCross",
	}
	buildType = buildIDs[runtime.GOOS+"-"+runtime.GOARCH]

Mac builds don't seem to work because they're unsigned and need an explicit exclusion [1]. Methinks you'll need to run sudo xattr -cr roachprod to clear the quarantine bit.

[1] https://support.apple.com/guide/mac-help/apple-cant-check-app-for-malicious-software-mchleab3a043/mac

pkg/cmd/roachprod/upgrade/teamcity.go line 79 at r2 (raw file):

func downloadRoachprod(buildID int32, destFile string) error {
	if buildID <= 0 {
		return fmt.Errorf("invalid build id")

Nit: might be easier to troubleshoot if the buildID were included in the error message.

smg260

Yeah I had thought about versioning too, but it is a bit meh. I don't want to introduce a dependency on git (API, git root, etc). I see this more of a convenience too.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rail, @renatolabs, and @srosenberg)

pkg/cmd/roachprod/upgrade/teamcity.go line 33 at r2 (raw file):

Previously, srosenberg (Stan Rosenberg) wrote…

Mac builds don't seem to work because they're unsigned and need an explicit exclusion [1]. Methinks you'll need to run sudo xattr -cr roachprod to clear the quarantine bit.

[1] https://support.apple.com/guide/mac-help/apple-cant-check-app-for-malicious-software-mchleab3a043/mac

Interesting. I did not run into that problem from OSX, and I can't see any exceptions in my settings.

➜  cockroach git:(8978258e40c) ✗ roachprod -v
roachprod version details:
Build Tag:        v23.1.0-alpha.8-dev
Build Time:
Distribution:     OSS
Platform:         darwin arm64
Go Version:       go1.19.4
C Compiler:       Apple LLVM 14.0.3 (clang-1403.0.22.14.1)
Build Commit ID:
Build Type:
➜  cockroach git:(8978258e40c) ✗ roachprod update
Downloading latest roachprod
	from:	https://teamcity.cockroachdb.com/guestAuth/app/rest/builds/id:10348226/artifacts/content/bazel-bin/pkg/cmd/roachprod/roachprod_/roachprod
	to  :	/Users/miral/go/src/github.com/cockroachdb/cockroach/bin/roachprod.new
Download successful. Continue with update? Note: This will overwrite any existing roachprod.bak binary. y[default]/n:
Update successful: run `roachprod -v` to confirm.
➜  cockroach git:(8978258e40c) ✗ roachprod -v
roachprod version details:
Build Tag:        v23.2.0-alpha.00000000-dev-b24770ad4454c5a86e8eb9295832e76c6c966ab6
Build Time:       2023/06/01 13:37:20
Distribution:     OSS
Platform:         darwin arm64 (aarch64-apple-darwin21.2)
Go Version:       go1.19.4
C Compiler:       Clang 10.0.0
Build Commit ID:  b24770ad4454c5a86e8eb9295832e76c6c966ab6
Build Type:       development
➜  cockroach git:(8978258e40c) ✗

rail

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @renatolabs, @smg260, and @srosenberg)

pkg/cmd/roachprod/upgrade/teamcity.go line 33 at r2 (raw file):

Previously, smg260 (Miral Gadani) wrote…

Interesting. I did not run into that problem from OSX, and I can't see any exceptions in my settings.

➜  cockroach git:(8978258e40c) ✗ roachprod -v
roachprod version details:
Build Tag:        v23.1.0-alpha.8-dev
Build Time:
Distribution:     OSS
Platform:         darwin arm64
Go Version:       go1.19.4
C Compiler:       Apple LLVM 14.0.3 (clang-1403.0.22.14.1)
Build Commit ID:
Build Type:
➜  cockroach git:(8978258e40c) ✗ roachprod update
Downloading latest roachprod
	from:	https://teamcity.cockroachdb.com/guestAuth/app/rest/builds/id:10348226/artifacts/content/bazel-bin/pkg/cmd/roachprod/roachprod_/roachprod
	to  :	/Users/miral/go/src/github.com/cockroachdb/cockroach/bin/roachprod.new
Download successful. Continue with update? Note: This will overwrite any existing roachprod.bak binary. y[default]/n:
Update successful: run `roachprod -v` to confirm.
➜  cockroach git:(8978258e40c) ✗ roachprod -v
roachprod version details:
Build Tag:        v23.2.0-alpha.00000000-dev-b24770ad4454c5a86e8eb9295832e76c6c966ab6
Build Time:       2023/06/01 13:37:20
Distribution:     OSS
Platform:         darwin arm64 (aarch64-apple-darwin21.2)
Go Version:       go1.19.4
C Compiler:       Clang 10.0.0
Build Commit ID:  b24770ad4454c5a86e8eb9295832e76c6c966ab6
Build Type:       development
➜  cockroach git:(8978258e40c) ✗

The quarantine bit is explicitly set by the browser, but not by curl/wget/go. That is my understanding.

BTW, maybe we should be more paranoid about accessing a map entry here? Maybe check it by buildType, ok := buildIDs[runtime.GOOS+"-"+runtime.GOARCH]?

pkg/cmd/roachprod/upgrade/teamcity.go line 27 at r3 (raw file):

var (
	buildIDs = map[string]string{
		"linux-amd64":   "Cockroach_ScratchProjectPutTcExperimentsInHere_BazelBuild",

:)
Maybe we should reset this ID?

pkg/cmd/roachprod/upgrade/teamcity.go line 40 at r3 (raw file):

// current binary's directory. It returns the path to the downloaded binary.
// toFile is the path to the file to download to.
func DownloadLatestRoadprod(toFile string) error {

A nit. Maybe accept io.WriteCloser or something so it's easier to test?

srosenberg

Reviewed 1 of 1 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan, @rail, @renatolabs, and @smg260)

pkg/cmd/roachprod/upgrade/teamcity.go line 33 at r2 (raw file):

The quarantine bit is explicitly set by the browser, but not by curl/wget/go. That is my understanding.

Right. I checked the binary downloaded via chrome,

s -l@ ~/Downloads/roachprod
-rw-r--r--@ 1 srosenberg  staff  78397586 Jun  1 13:52 /Users/srosenberg/Downloads/roachprod
	com.apple.macl	      72
	com.apple.metadata:kMDItemWhereFroms	     319
	com.apple.quarantine	      21

The one downloaded via roachprod,

ls -l@ bin/roachprod.bak
-rwxr-xr-x  1 srosenberg  staff  77106354 Jun  1 13:49 bin/roachprod.bak

srosenberg

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @herkolategan, @rail, @renatolabs, and @smg260)

smg260

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @herkolategan, @rail, @renatolabs, and @srosenberg)

pkg/cmd/roachprod/upgrade/teamcity.go line 33 at r2 (raw file):
Yeah, that sounds right. I think users who end up downloading via the link could manage that issue themselves. @rail does the roachprod update work for you?

BTW, maybe we should be more paranoid about accessing a map entry here ..

Yeah that's fair; even though it's guarded by checking for "", better to be explicit. Updated

pkg/cmd/roachprod/upgrade/teamcity.go line 27 at r3 (raw file):

Previously, rail (Rail Aliiev) wrote…

:)
Maybe we should reset this ID?

😆 that would be nice

pkg/cmd/roachprod/upgrade/teamcity.go line 40 at r3 (raw file):

Previously, rail (Rail Aliiev) wrote…

A nit. Maybe accept io.WriteCloser or something so it's easier to test?

done

smg260 · 2023-06-01T18:05:09Z

pkg/cmd/roachprod/upgrade/teamcity.go line 40 at r3 (raw file):

Previously, smg260 (Miral Gadani) wrote…

done

rather, done in downloadRoachprod(..)

rail

Reviewed 1 of 2 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @renatolabs, @smg260, and @srosenberg)

pkg/cmd/roachprod/upgrade/teamcity.go line 27 at r3 (raw file):

Previously, smg260 (Miral Gadani) wrote…

😆 that would be nice

Here we go, Cockroach_ScratchProjectPutTcExperimentsInHere_BazelBuild is now Cockroach_Ci_Builds_BuildLinuxX8664!

smg260 · 2023-06-01T21:23:58Z

TFTR!

bors r=srosenberg,rail,herkolategan

craig · 2023-06-01T21:48:57Z

Build failed:

Bazel Essential CI (Cockroach)

smg260 · 2023-06-01T22:29:05Z

bors retry

craig · 2023-06-01T23:30:20Z

Build succeeded:

Bazel Essential CI (Cockroach)

renatolabs

Thanks for adding this command! Late review, just left some comments looking at possible future work, mostly thinking about the usability for non-engineering folks.

Couple of questions too:

Has this been communicated more broadly? I believe a number of people (TSEs for example) would be interested in this feature and glad they don't have to pull master and compile things themselves. We should also update this doc page: https://cockroachlabs.atlassian.net/wiki/spaces/TE/pages/2899247202/Update+your+Roachprod+Binary.
Is roachprod-get-latest used by any build at the moment, or is it just for our own use? If it's the latter, I'm afraid it will only be a matter of time before something changes and it stops working.

renatolabs · 2023-06-12T17:35:10Z

pkg/cmd/roachprod/main.go

@@ -1171,6 +1172,45 @@ func validateAndConfigure(cmd *cobra.Command, args []string) {
 	}
 }

+var updateCmd = &cobra.Command{
+	Use:   "update",
+	Short: "check TeamCity for a new roachprod binary and update if available",


This (and the Long description) could be made a little less implementation focused, which would probably make it more understandable for the main users of this update feature: non-engineering folks that rely on roachprod for experimentation.

renatolabs · 2023-06-12T17:44:17Z

pkg/cmd/roachprod/main.go

+			return err
+		}
+
+		if revertUpdate {


Do we have a concrete use case for this feature? To me it would be something that we add only if we know it's needed frequently. The main things I'm not a big fan of are:

it may be hard to know what the "previous" version is (what we are reverting to), especially in the case where the previous binary was built with dev (i.e., without stamping).

For folks that have roachprod in their $PATH, this will add roachprod.bak to the same directory, which can be confusing and/or inconvenient when doing roachp<tab>.

renatolabs · 2023-06-12T17:45:04Z

pkg/cmd/roachprod/main.go

+			return err
+		}
+
+		if upgrade.PromptYesNo("Continue with update? This will overwrite any existing roachprod.bak binary.") {


We should probably delete newBinary in the else clause.

renatolabs · 2023-06-12T17:46:02Z

pkg/cmd/roachprod/main.go

+				return errors.WithDetail(err, "unable to update binary")
+			}
+
+			fmt.Println("Update successful: run `roachprod -v` to confirm.")


For non-engineering folks, it would probably be helpful to run this automatically on their behalf. Also roachprod -v might not be a valid command if it's not in $PATH.

renatolabs · 2023-06-12T17:46:59Z

pkg/cmd/roachprod/main.go

+		}
+
+		if revertUpdate {
+			if upgrade.PromptYesNo("Revert to previous version? Note: this will replace the" +


It's a little unsatisfying that the package used by the update command is called upgrade. 🙂

smg260 force-pushed the 97311_roachprod_upgrading branch 7 times, most recently from bc2523d to e0f6e40 Compare May 15, 2023 21:22

smg260 requested review from srosenberg and renatolabs May 15, 2023 22:49

smg260 force-pushed the 97311_roachprod_upgrading branch 4 times, most recently from e6affcc to 2aba5d5 Compare May 17, 2023 13:00

smg260 force-pushed the 97311_roachprod_upgrading branch 2 times, most recently from a55459f to 8effb50 Compare May 22, 2023 15:31

smg260 marked this pull request as ready for review May 22, 2023 16:46

smg260 requested review from a team as code owners May 22, 2023 16:46

srosenberg requested review from herkolategan and removed request for a team May 24, 2023 07:30

herkolategan approved these changes May 30, 2023

View reviewed changes

smg260 force-pushed the 97311_roachprod_upgrading branch from 8effb50 to 8978258 Compare May 31, 2023 13:22

srosenberg reviewed May 31, 2023

View reviewed changes

srosenberg requested review from rail and removed request for a team May 31, 2023 22:59

smg260 commented Jun 1, 2023

View reviewed changes

smg260 force-pushed the 97311_roachprod_upgrading branch from 8978258 to f45aa4f Compare June 1, 2023 14:17

rail approved these changes Jun 1, 2023

View reviewed changes

smg260 force-pushed the 97311_roachprod_upgrading branch from f45aa4f to 206a376 Compare June 1, 2023 17:56

srosenberg reviewed Jun 1, 2023

View reviewed changes

srosenberg approved these changes Jun 1, 2023

View reviewed changes

smg260 commented Jun 1, 2023

View reviewed changes

rail reviewed Jun 1, 2023

View reviewed changes

Miral Gadani added 2 commits June 1, 2023 20:10

roachprod: httpclient with longer timeout for downloads

a892ee8

scripts: download latest roachprod binary

73bff11

smg260 force-pushed the 97311_roachprod_upgrading branch from 206a376 to 73bff11 Compare June 1, 2023 20:10

craig bot merged commit bf25b7b into cockroachdb:master Jun 1, 2023

renatolabs reviewed Jun 12, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

roachprod: add command to self update the roachprod binary #103307

roachprod: add command to self update the roachprod binary #103307

smg260 commented May 15, 2023 •

edited

Loading

cockroach-teamcity commented May 15, 2023

blathers-crl bot commented May 15, 2023

herkolategan left a comment

smg260 commented May 30, 2023

smg260 commented May 30, 2023

srosenberg left a comment

smg260 left a comment

rail left a comment

srosenberg left a comment

srosenberg left a comment

smg260 left a comment

smg260 commented Jun 1, 2023

rail left a comment

smg260 commented Jun 1, 2023

craig bot commented Jun 1, 2023

smg260 commented Jun 1, 2023

craig bot commented Jun 1, 2023

renatolabs left a comment

renatolabs Jun 12, 2023

renatolabs Jun 12, 2023

renatolabs Jun 12, 2023

renatolabs Jun 12, 2023

renatolabs Jun 12, 2023

roachprod: add command to self update the roachprod binary #103307

roachprod: add command to self update the roachprod binary #103307

Conversation

smg260 commented May 15, 2023 • edited Loading

cockroach-teamcity commented May 15, 2023

blathers-crl bot commented May 15, 2023

herkolategan left a comment

Choose a reason for hiding this comment

smg260 commented May 30, 2023

smg260 commented May 30, 2023

srosenberg left a comment

Choose a reason for hiding this comment

smg260 left a comment

Choose a reason for hiding this comment

rail left a comment

Choose a reason for hiding this comment

srosenberg left a comment

Choose a reason for hiding this comment

srosenberg left a comment

Choose a reason for hiding this comment

smg260 left a comment

Choose a reason for hiding this comment

smg260 commented Jun 1, 2023

rail left a comment

Choose a reason for hiding this comment

smg260 commented Jun 1, 2023

craig bot commented Jun 1, 2023

smg260 commented Jun 1, 2023

craig bot commented Jun 1, 2023

renatolabs left a comment

Choose a reason for hiding this comment

renatolabs Jun 12, 2023

Choose a reason for hiding this comment

renatolabs Jun 12, 2023

Choose a reason for hiding this comment

renatolabs Jun 12, 2023

Choose a reason for hiding this comment

renatolabs Jun 12, 2023

Choose a reason for hiding this comment

renatolabs Jun 12, 2023

Choose a reason for hiding this comment

smg260 commented May 15, 2023 •

edited

Loading