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

x/build: trybots should test subrepos against previous Go releases #17626

Closed
bradfitz opened this Issue Oct 27, 2016 · 26 comments

Comments

Projects
None yet
7 participants
@bradfitz
Member

bradfitz commented Oct 27, 2016

Subrepos trybots should test against Go tip, Go 1.N, Go 1.N-1.

@bradfitz bradfitz added the Builders label Oct 27, 2016

@bradfitz bradfitz added this to the Unreleased milestone Oct 27, 2016

@bradfitz bradfitz self-assigned this Oct 27, 2016

@dsnet

This comment has been minimized.

Member

dsnet commented Oct 27, 2016

What's the general policy for how many versions back we maintain support on a given subrepo?

@bradfitz

This comment has been minimized.

Member

bradfitz commented Oct 27, 2016

In general, the two most recent. (which means between 12-18 months for us)

@mxplusb

This comment has been minimized.

mxplusb commented Dec 1, 2016

I'm new to the language contribution process, but I'd like to help with this issue since it seems like something simple (relative). How can I help with this issue?

@bradfitz

This comment has been minimized.

Member

bradfitz commented Dec 1, 2016

This is a hard one to contribute to because the Go build system is large and has many moving parts, and it's difficult to run without access to our GCP project, and/or paying for it.

The code is in https://github.com/golang/build (mostly in cmd/coordinator) and you can run it in localhost machine. Kinda. But it might require some hackery for local testing. That code bitrots often and needs occasional love. I suspect it's partially broken.

@mxplusb

This comment has been minimized.

mxplusb commented Dec 2, 2016

Alright, I'll see if I can make some progress, and update here if I need help. Do I need a builder hash at all?

@bradfitz

This comment has been minimized.

Member

bradfitz commented Dec 2, 2016

Nope.

@mxplusb

This comment has been minimized.

mxplusb commented Dec 11, 2016

I'm getting this issue when trying to run it locally:

failed to get a token source: google: could not find default credentials. See https://developers.google.com/accounts/docs/application-default-credentials for more information.

Do those creds need to be on the Go build system or will any credentials work?

@bradfitz

This comment has been minimized.

Member

bradfitz commented Dec 12, 2016

You're starting to discover why this is difficult. You shouldn't need any credentials in localhost mode. But we also don't develop the build system in localhost mode, so you probably need to fix it up to work.

@mxplusb

This comment has been minimized.

mxplusb commented Dec 12, 2016

Fix the system to fix the system, can do! I'll create a sub-issue to allow it to work in localhost mode and tackle that first.

@bradfitz bradfitz changed the title from x/build: test subrepos against previous Go releases to x/build: trybots should test subrepos against previous Go releases Apr 24, 2017

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jun 16, 2017

@andybons, after the dev.golang.org migration (#20691), this would be a good second one.

@gopherbot

This comment has been minimized.

gopherbot commented Jul 28, 2017

Change https://golang.org/cl/51590 mentions this issue: maintner/maintnerd, cmd/coordinator: use maintnerd to find TryBot work

gopherbot pushed a commit to golang/build that referenced this issue Jul 28, 2017

maintner/maintnerd, cmd/coordinator: use maintnerd to find TryBot work
Instead of polling Gerrit every 60 seconds, poll maintnerd every
second. This should improve TryBot-requested to TryBot-running latency
by 30 seconds on average.

(Ideally it'd long poll for changes and not even have ~500ms delay,
but this is an improvement.)

Also, in the process this does most of the work for golang/go#17626.
And this cleans up the repo head tracking complication in the coordinator
and just asks maintner for it instead.

Running benchmarks in the coordinator has been disabled since
2017-06-23 but this CL disables it further, removing some code it'd
need to work. But considering that benchmarking would need code
repairs to get working again anyway, I consider this acceptable in the
short term. I left TODO notes.

Updates golang/go#17626
Updates golang/go#19871

Change-Id: Idab43f65a9cca861bffb37748b036a5c9baee864
Reviewed-on: https://go-review.googlesource.com/51590
Reviewed-by: Andrew Bonventre <andybons@golang.org>
@heschik

This comment has been minimized.

Contributor

heschik commented Oct 16, 2018

This would be really nice for x/tools/go/packages, which has very different behavior on 1.10.

@gopherbot

This comment has been minimized.

gopherbot commented Oct 19, 2018

Change https://golang.org/cl/143538 mentions this issue: cmd/coordinator: run subrepo trybots against recent Go release branches

gopherbot pushed a commit to golang/build that referenced this issue Oct 29, 2018

cmd/coordinator: run subrepo trybots against recent Go release branches
This is the coordinator half of the change to make the coordinator
test subrepos against two prior releases of Go.

The next change that's required is for the maintnerd API server to
return the name of the past two releases.

For now this change is a no-op. But once maintnerd starts returning
more data, then this change will start causing causing two more
linux-amd64 builders per subrepo trybot.

Updates golang/go#17626

Change-Id: I1cedbc2e4eee51fb003c8bcc8e072fd10717a833
Reviewed-on: https://go-review.googlesource.com/c/143538
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot

This comment has been minimized.

gopherbot commented Oct 30, 2018

Change https://golang.org/cl/146137 mentions this issue: maintner: add RPC endpoint for listing Go releases

gopherbot pushed a commit to golang/build that referenced this issue Oct 31, 2018

maintner: add RPC endpoint for listing Go releases
This change adds an RPC endpoint to maintnerd API server to list
the supported Go releases. This is needed and will be used by
cmd/coordinator to run subrepo trybots on previous Go release
(in addition to current).

It's implemented on top of the Gerrit project ref data that the
maintner corpus already tracks. A release is considered to be
exist for each git tag named "goX", "goX.Y", or "goX.Y.Z".

This functionality is also implemented in some other places using
data that is further away from the source of truth. Such places can
start to use this endpoint instead.

Add a subcommand to maintq to invoke the new list Go releases endpoint.
Its current output (against a local development maintnerd server):

	$ go run .../maintner/maintq -server=localhost:6344 list-releases
	major:1 minor:11 patch:1 tag_name:"go1.11.1" tag_commit:"26957168c4c0cdcc7ca4f0b19d0eb19474d224ac" branch_name:"release-branch.go1.11" branch_commit:"97781d2ed116d2cd9cb870d0b84fc0ec598c9abc"
	major:1 minor:10 patch:4 tag_name:"go1.10.4" tag_commit:"2191fce26a7fd1cd5b4975e7bd44ab44b1d9dd78" branch_name:"release-branch.go1.10" branch_commit:"e97b7d68f107ff60152f5bd5701e0286f221ee93"

Updates golang/go#17626

Change-Id: Ia9ea7f49d421ce0c7a9e85c423aba31572cea52b
Reviewed-on: https://go-review.googlesource.com/c/146137
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

gopherbot commented Nov 2, 2018

Change https://golang.org/cl/147137 mentions this issue: failing_test: test of new trybot feature

@gopherbot

This comment has been minimized.

gopherbot commented Nov 2, 2018

Change https://golang.org/cl/147198 mentions this issue: maintner/maintnerd/maintapi: test subrepos against previous Go releases

@gopherbot

This comment has been minimized.

gopherbot commented Nov 2, 2018

Change https://golang.org/cl/147200 mentions this issue: maintner/maintnerd: change definition of Go release to require release branch

gopherbot pushed a commit to golang/build that referenced this issue Nov 2, 2018

maintner/maintnerd: change definition of Go release to require releas…
…e branch

Previously, the list-Go-releases RPC endpoint was more flexible and
considered a Go release to exist as long as there's a git tag for it,
regardless of whether a corresponding release branch existed.

Such a situation is very unlikely to come up, and we don't want to cause
all users of this API to filter out releases without a release branch.
So just re-define a Go release to require a release branch, and
make that the API promise.

In practice, this should not have any effect because all go tags
have corresponding release branches.

Updates golang/go#17626

Change-Id: Ia7a8354000483c969e123f0f3605fd360846c40b
Reviewed-on: https://go-review.googlesource.com/c/147200
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

@dmitshur dmitshur removed the help wanted label Nov 2, 2018

@dmitshur

This comment has been minimized.

Member

dmitshur commented Nov 3, 2018

Reopening for deploy and verification.

@dmitshur dmitshur reopened this Nov 3, 2018

@dmitshur

This comment has been minimized.

Member

dmitshur commented Nov 3, 2018

Deployed.

According to https://go-review.googlesource.com/c/tools/+/147137#message-af8badf0a004b89d6130ff01bdf798052e2bebc8, it's not working yet. There are still 18 trybots for a subrepo run, that only include the latest Go tip version. /cc @bradfitz

@dmitshur

This comment has been minimized.

Member

dmitshur commented Nov 3, 2018

Whoops. I deployed cmd/coordinator, but for this to take effect I should be deploying maintnerd.

So, this isn't actually deployed yet. Working on that now.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 3, 2018

So it seems to work, but I missed something in the UI. It's using the builder name and not the new Name() string accessor here:

screen shot 2018-11-03 at 12 49 15 pm

Notice that (and the grid below) is missing the Go version in parens.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 3, 2018

But I do get the nice names in the final email:

screen shot 2018-11-03 at 12 50 52 pm

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 3, 2018

Also, (go branch release-branch.go1.10) is pretty verbose. I clearly didn't think that through.

We could make that accessor clean up the string a bit and just say (Go 1.10).

@bradfitz bradfitz assigned dmitshur and unassigned andybons Nov 3, 2018

@dmitshur

This comment has been minimized.

Member

dmitshur commented Nov 3, 2018

Indeed! From https://go-review.googlesource.com/c/tools/+/147137#message-d1a29aad64e636ed4ffcbaf1f2c3f324dc5d5d54:

7 of 20 TryBots failed:
...
Failed on linux-amd64 (go branch release-branch.go1.10): https://storage.googleapis.com/go-build-log/1ae73979/linux-amd64_c18e47da.log
Failed on linux-amd64 (go branch release-branch.go1.11): https://storage.googleapis.com/go-build-log/e8a95aeb/linux-amd64_1798cb41.log
Failed on linux-amd64: https://storage.googleapis.com/go-build-log/f2cd0fa7/linux-amd64_fbab8151.log
...

That's proof it's working as intended! 🎉

I'll consider this issue resolved. Let's open a new one for the UI improvements (edit: opened #28580).

@dmitshur dmitshur closed this Nov 3, 2018

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 3, 2018

I'm fine reusing this bug, but new one is also fine.

@gopherbot

This comment has been minimized.

gopherbot commented Nov 5, 2018

Change https://golang.org/cl/147497 mentions this issue: cmd/coordinator: fix another UI issue with trybots against old Go releases

gopherbot pushed a commit to golang/build that referenced this issue Nov 6, 2018

cmd/coordinator: fix another UI issue with trybots against old Go rel…
…eases

Updates golang/go#28580
Updates golang/go#17626

Change-Id: Ifa88e4ad23e823e5d1a25c46b76afb11b4e7907b
Reviewed-on: https://go-review.googlesource.com/c/147497
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment