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

x/build/{cmd/coordinator,app/appengine}: combine codebases #34744

Open
dmitshur opened this issue Oct 7, 2019 · 22 comments
Open

x/build/{cmd/coordinator,app/appengine}: combine codebases #34744

dmitshur opened this issue Oct 7, 2019 · 22 comments
Assignees
Milestone

Comments

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Oct 7, 2019

We've discussed and agreed wanting to merge app/appengine into cmd/coordinator. The motivation is to have a single codebase, share more code, fewer separate deployments, and tests with more inclusive coverage.

I didn't find an existing issue, so making this one to track the task.

/cc @bradfitz @andybons @toothrot

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Oct 7, 2019

I regret that I have but four emojis to give to this bug.

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 8, 2019

Change https://golang.org/cl/199800 mentions this issue: app/appengine: don't exclude release-branch-only builders

gopherbot pushed a commit to golang/build that referenced this issue Oct 8, 2019
The filtering of builders in commitBuilders was too strict,
causing post-submit builders that are configured to run on
release-branches only to not get included in the list of
builders.

This change makes the following builders no longer skipped:

	darwin-amd64-10_10
	freebsd-386-10_3
	freebsd-386-10_4
	freebsd-386-11_1
	freebsd-amd64-10_3
	freebsd-amd64-10_4
	freebsd-amd64-11_1
	nacl-386
	nacl-amd64p32

We may need to adjust the build.golang.org UI after to avoid
including unhelpful columns, but this is a first step to get
nacl builders to run on release branches again.

Updates golang/go#34738
Updates golang/go#34744

Change-Id: Iaf2b93aedd5f44b48b9a63b57f12549fe50b1637
Reviewed-on: https://go-review.googlesource.com/c/build/+/199800
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 8, 2019

Change https://golang.org/cl/199878 mentions this issue: app/appengine: hide release-branch-only builders when viewing master branch

gopherbot pushed a commit to golang/build that referenced this issue Oct 8, 2019
…branch

This is a follow-up to CL 199800. That change started to consider more
builders as active to allow release-branch-only builders to run.

However, it also made those builders show up as new empty columns when
viewing builds for the master branch. There are quite a few old FreeBSD
builders that only run on release-branch.go1.12, and it's too disruptive
to have them appear everywhere. So, hide them when viewing the master
branch of Go repo in the UI.

There can be more UI improvements to be made, and this can become too
much of a whack-a-mole to address them one by one. The scope of this CL
is to fix the most disruptive high priority problem for now. Further
improvements will happen later, with merging of app/appengine and
cmd/coordinator codebases in mind.

Updates golang/go#34738
Updates golang/go#34744

Change-Id: I3df75f8b2bbd5f6fe8097c181ee8a1b1b4354dc9
Reviewed-on: https://go-review.googlesource.com/c/build/+/199878
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
codebien added a commit to codebien/build that referenced this issue Nov 13, 2019
The filtering of builders in commitBuilders was too strict,
causing post-submit builders that are configured to run on
release-branches only to not get included in the list of
builders.

This change makes the following builders no longer skipped:

	darwin-amd64-10_10
	freebsd-386-10_3
	freebsd-386-10_4
	freebsd-386-11_1
	freebsd-amd64-10_3
	freebsd-amd64-10_4
	freebsd-amd64-11_1
	nacl-386
	nacl-amd64p32

We may need to adjust the build.golang.org UI after to avoid
including unhelpful columns, but this is a first step to get
nacl builders to run on release branches again.

Updates golang/go#34738
Updates golang/go#34744

Change-Id: Iaf2b93aedd5f44b48b9a63b57f12549fe50b1637
Reviewed-on: https://go-review.googlesource.com/c/build/+/199800
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
codebien added a commit to codebien/build that referenced this issue Nov 13, 2019
…branch

This is a follow-up to CL 199800. That change started to consider more
builders as active to allow release-branch-only builders to run.

However, it also made those builders show up as new empty columns when
viewing builds for the master branch. There are quite a few old FreeBSD
builders that only run on release-branch.go1.12, and it's too disruptive
to have them appear everywhere. So, hide them when viewing the master
branch of Go repo in the UI.

There can be more UI improvements to be made, and this can become too
much of a whack-a-mole to address them one by one. The scope of this CL
is to fix the most disruptive high priority problem for now. Further
improvements will happen later, with merging of app/appengine and
cmd/coordinator codebases in mind.

Updates golang/go#34738
Updates golang/go#34744

Change-Id: I3df75f8b2bbd5f6fe8097c181ee8a1b1b4354dc9
Reviewed-on: https://go-review.googlesource.com/c/build/+/199878
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 21, 2019

Change https://golang.org/cl/208319 mentions this issue: app/appengine: remove ancient performance/benchmarking code

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 21, 2019

Change https://golang.org/cl/208320 mentions this issue: app/appengine, cmd/genbuilderkey: move key gen from App Engine to new tool

gopherbot pushed a commit to golang/build that referenced this issue Nov 21, 2019
This was the performance/benchmark code from ~three generations ago.

It's unused and unmaintained. It broke when we moved from mercurial to
git, IIRC.

I'm attempting to modernize this code (for golang/go#34744) but it'd
be easier if there's less code to deal with.

Updates golang/go#34744

Change-Id: Ib4999830b05df9ffad9b46964022325404350b47
Reviewed-on: https://go-review.googlesource.com/c/build/+/208319
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
gopherbot pushed a commit to golang/build that referenced this issue Nov 21, 2019
… tool

This moves the /key handler to generate build keys to a standalone
tool. The old way has been largely broken for years (when using the
build.golang.org domain) due to internal App Engine changes. We have
to access it via https://build-dot-golang-org.appspot.com/key instead
to get authenticated.

Also, the App Engine go112 runtime doesn't support authenticated
handlers, so more reason to move off App Engine.

This CL is part of a series to move off the the build.golang.org App
Engine app that mirrors the git history into Datastore Entities, which
is full of complication and bugs.

These early steps are about removing a bunch of code from the App
Engine app so the important bits are easy to see and refactor.

Updates golang/go#34744

Change-Id: Iaf8e2bf458b5fea45bf05026d8a6eaf0ead88ec2
Reviewed-on: https://go-review.googlesource.com/c/build/+/208320
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 21, 2019

Change https://golang.org/cl/208322 mentions this issue: app/appengine: delete old "broke the build" code

gopherbot pushed a commit to golang/build that referenced this issue Nov 21, 2019
It hasn't worked in years.

This is part of a series of CLs to clean up the build.golang.org App
Engine app in prep for it to be modernized, refactored, and replaced.

Updates golang/go#34744
Updates golang/go#12509

Change-Id: I9f8445046961ccbe97f7b9c85c0772393bf7d547
Reviewed-on: https://go-review.googlesource.com/c/build/+/208322
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Andrew Gerrand <adg@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 21, 2019

Change https://golang.org/cl/208324 mentions this issue: app/appengine: delete more dead code

gopherbot pushed a commit to golang/build that referenced this issue Nov 21, 2019
This is part of a series of CLs to clean up the build.golang.org App
Engine app in prep for it to be modernized, refactored, and replaced,
starting with deleting dead code.

Updates golang/go#34744

Change-Id: I6cddbb44a63597a308f1d4399d21e5b70a8d83bf
Reviewed-on: https://go-review.googlesource.com/c/build/+/208324
Reviewed-by: Andrew Gerrand <adg@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 22, 2019

Change https://golang.org/cl/208397 mentions this issue: app/appengine: delete yet more dead code

gopherbot pushed a commit to golang/build that referenced this issue Nov 22, 2019
This is part of a series of CLs to clean up the build.golang.org App
Engine app in prep for it to be modernized, refactored, and replaced,
starting with deleting dead code.

Some of this was missed in CL 208319.

Updates golang/go#34744

Change-Id: I35adf4296b849dabed9d974e7eb58eeedc7d4293
Reviewed-on: https://go-review.googlesource.com/c/build/+/208397
Reviewed-by: Andrew Gerrand <adg@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 24, 2019

Change https://golang.org/cl/208678 mentions this issue: app/appengine: remove unused tagHandler, document others, remove auth from init

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 25, 2019

Change https://golang.org/cl/208697 mentions this issue: app, gitmirror, maintner: use maintner for dashboard, not datastore

gopherbot pushed a commit to golang/build that referenced this issue Nov 25, 2019
… from init

tagHandler isn't used by anything. Delete. Less code to update.

And document who calls the other handlers.

Also, remove the auth requirement from the /init handler. It's
harmless to call without auth, and the login restriction went away
with the Go 1.12+ runtime anyway, so deleting it gets us closer to
being able to use the Go 1.12/Go 1.13 runtimes. (The plan is to delete
most the code, port the small remaining bit to the cloud.google.com/go
libraries so we can update to Go 1.12/Go 1.13+, and then at that
point, since the cloud.google.com/go code will run anywhere, we can
just run it in the same process as the coordinator.)

Updates golang/go#34744

Change-Id: I929c70945a3e9e27b38b1d5899c7860470361927
Reviewed-on: https://go-review.googlesource.com/c/build/+/208678
Reviewed-by: Bryan C. Mills <bcmills@google.com>
gopherbot pushed a commit to golang/build that referenced this issue Dec 6, 2019
Historically, the build.golang.org was the entire build system, and it
maintained a parallel copy of that git history in its datastore. It
was always buggy and incomplete and things like force pushes were
scary because the datastore mirror could get out of sync. It was also
a lot of code to support that sync.

This changes build.golang.org to instead get the git history from
maintnerd, and then we can remove all the HTTP handlers around
updating it, and can remove all the gitmirror code to call it to
maintain it.

Now build.golang.org only keeps build results, keyed on the commit
hash. It's much less code, but is still an App Engine app for now.
(but it's getting small enough, that porting it to
cloud.google.com/go/datastore is looking very simple)

This also adds a new "repos" package to unify the configuration of the
various Go repos. There were incomplete & redundant copies all over
the place.

Updates golang/go#34744
Fixes golang/go#35828
Fixes golang/go#31236 (New branch=mixed support adds this when desired)
Fixes golang/go#35944

Change-Id: Ifb39417287df3dea052ba8510566d80b4bc75d51
Reviewed-on: https://go-review.googlesource.com/c/build/+/208697
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Dec 9, 2019

This is getting pretty close now.

Here's a suggested roadmap for finishing this:

  • add an HTTP handler to the coordinator for removing build results. Currently there are only two things remaining that POST to the dashboard (the build.golang.org App Engine app): the coordinator itself, and x/build/cmd/retrybuilds. If we want to get to the coordinator knowing the world state (including keeping the work list in memory), we can't have retrybuilds reaching behind its back and writing to the datastore directly. So we should for now add a proxy handler to the coordinator that just proxies the /clear-results path to build.golang.org. Then change retrybuilds to use the farmer.golang.org hostname instead of build.golang.org.

  • change the coordinator's findWork to not hit the dashboard but instead do the maintnerd GetDashboard API call itself. This'll need to start doing datastore Get calls (to either golang-org's GCP project w/ new service account, or migrate the data, which seems unnecessarily painful)

  • modify the coordinator's /clear-results handler to do the datastore deletion itself. (And it can then also immediately start the replacement work, to rebuild whatever was cleared, without waiting for findWork to run)

  • add a JSON handler to the coordinator that lists all the active post-submit builds

  • drop the dashboard memcache code for buildingURLs (the blue gophers that render on the dashboard for in-progress builds) and instead hit the new JSON endpoint to figure out what's in progress. This then removes all memcache usage in the dashboard, which isn't (as easily) available in the new App Engine Go 1.12+ runtimes.

  • update app/appengine from the App Engine datastore package to the Cloud (cloud.google.com/go/datastore) package. This is required to update to the App Engine Go 1.12 runtime. Do that to verify it all works.

  • move the app/appengine UI code to a shared spot and make the coordinator also capable of running that handler. Now that it's using cloud.google.com/go/datastore, this should be trivial. Make an HTTP mux that routes to the dashboard UI handler based on Hostname. Say, if `hostname == "build.golang.org" || hostname == "farmer-ui-test.golang.org".

  • create DNS record farmer-ui-test.golang.org to test that it works.

  • change the build.golang.org DNS record.

  • delete the App Engine app

@gopherbot
Copy link

@gopherbot gopherbot commented Dec 11, 2019

Change https://golang.org/cl/210838 mentions this issue: app/appengine, cmd/coordinator: fetch active builds from coordinator, avoid memcache

gopherbot pushed a commit to golang/build that referenced this issue Dec 11, 2019
… avoid memcache

This implements bullets 4 and 5 from the plan to unify the dashboard &
coordinator:
golang/go#34744 (comment)

Previously the coordinator would POST to the dashboard regularly, for
each active build, to say "I'm still working on this build! Write that
to memcache!". And then if somebody loaded https://build.golang.org/
it would do a big memcache multi-get to populate the little blue
gopher links to their status pages on the coordinator.

This instead turns it around. We no longer POST from the coordinator
to the dashboard, and we no longer use any memcache (which also means
migrating to the App Engine Go 1.13 runtime is easier, which drops
the built-in memcache support). Instead, the dashboard now does a GET
to the coordinator to get the list of active builds.

This also adds test coverage, which we didn't have before.

Updates golang/go#34744

Change-Id: I97a486ec362a7a00d29076c81a88d6417b138c1b
Reviewed-on: https://go-review.googlesource.com/c/build/+/210838
Reviewed-by: Alexander Rakoczy <alex@golang.org>
Run-TryBot: Alexander Rakoczy <alex@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Dec 11, 2019

Change https://golang.org/cl/210977 mentions this issue: app/appengine: migrate from App Engine to Cloud APIs, move to Go 1.13

gopherbot pushed a commit to golang/build that referenced this issue Dec 11, 2019
This is bullet 6 from the master plan at:
golang/go#34744 (comment)

Updates golang/go#34744

Change-Id: I273aed62a519ce240704e0ea190d08d0e254cdab
Reviewed-on: https://go-review.googlesource.com/c/build/+/210977
Reviewed-by: Alexander Rakoczy <alex@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Dec 11, 2019

Change https://golang.org/cl/210997 mentions this issue: app/appengine: do maintner & coordinator RPCs concurrently

@gopherbot
Copy link

@gopherbot gopherbot commented Dec 12, 2019

Change https://golang.org/cl/210998 mentions this issue: app/appengine: explicitly mark the Log.CompressedLog field as not indexed

gopherbot pushed a commit to golang/build that referenced this issue Dec 12, 2019
…exed

The old App Engine datastore API and the new Cloud datastore API have
different behaviors with regard to indexing []byte properties.

Both APIs say the same:

https://godoc.org/google.golang.org/appengine/datastore#hdr-Properties
https://godoc.org/cloud.google.com/go/datastore#hdr-Properties

> All fields are indexed by default. Strings or byte slices longer
> than 1500 bytes cannot be indexed; fields used to store long strings
> and byte slices must be tagged with "noindex" or they will cause Put
> operations to fail.

But empirically only the new cloud API actually returns an error when
putting a >1500 []byte value into a field without noindex. Not sure
why.

Updates golang/go#34744

Change-Id: I15c938e91c92304eb7074de99c19f8e93182283e
Reviewed-on: https://go-review.googlesource.com/c/build/+/210998
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit to golang/build that referenced this issue Dec 12, 2019
Fixes a TODO.

The coordinator RPC should be temporary, but a) who ever knows how
long temporary is, and b) it's easy enough to do, and c) faster is
nicer.

Updates golang/go#34744

Change-Id: Id7216e1c67dc04152565bb8c5d6181cde674a12e
Reviewed-on: https://go-review.googlesource.com/c/build/+/210997
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@toothrot toothrot self-assigned this Feb 4, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 11, 2020

Change https://golang.org/cl/219120 mentions this issue: cmd/coordinator,cmd/retrybuilds: add wipe API to coordinator

gopherbot pushed a commit to golang/build that referenced this issue Feb 20, 2020
As part of our migration to combine codebases of the Build Dashboard and
the Coordinator, the first step is to start calling a Coordinator API
for wiping release status of failed builds. This adds a gRPC API to the
coordinator, listening on the same port as the HTTPS listeners.

The Coordinator API in this implementation simply validates and forwards
a request to the dashboard API.

This change also updates cmd/retrybuilds to optionally use the
Coordinator gRPC API for wiping.

Tested locally using the live Dashboard API.

Updates golang/go#34744

Change-Id: I4b34b064625193eb11a280565d701605064a8443
Reviewed-on: https://go-review.googlesource.com/c/build/+/219120
Run-TryBot: Alexander Rakoczy <alex@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Carlos Amedee <carlos@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 3, 2020

Change https://golang.org/cl/221920 mentions this issue: cmd/coordinator: render partial build dashboard

gopherbot pushed a commit to golang/build that referenced this issue Mar 5, 2020
This change adds a build dashboard handler to the Coordinator. As part
of the effort to remove the app/appengine build dashboard, this moves a
large part of the template and logic into cmd/coordinator.

As part of this change, cmd/coordinator/internal/dashboard has been
created. I originally developed this in the main package, but the main
package is very crowded in the coordinator. Giving the dashboard its own
package also made testing easier.

Currently, this implementation only supports rendering part of the build
dashboard for the Go repository on master. It does not yet link to test
logs, and only shows successful state.

Updates golang/go#34744

Change-Id: I6ffe064b9fc5e4a3271eadfd5ac45d5baf4ebd37
Reviewed-on: https://go-review.googlesource.com/c/build/+/221920
Run-TryBot: Alexander Rakoczy <alex@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Carlos Amedee <carlos@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 5, 2020

Change https://golang.org/cl/222197 mentions this issue: cmd/coordinator/internal/dashboard: filter ErrNoSuchEntity

@toothrot
Copy link
Contributor

@toothrot toothrot commented Mar 5, 2020

I've been making some progress on this. I want to add a note to capture @dmitshur's comments in https://golang.org/cl/221920: we should improve how we handle static files.

I did a minimal amount. I think moving them into a static directory and using go generate as we do on other projects is a good solution, but we should somehow cause a build failure for a misconfiguration.

gopherbot pushed a commit to golang/build that referenced this issue Mar 9, 2020
The AppEngine dashboard filters out datastore.ErrNoSuchEntity, which
happens when we fetch a commit that has no build information yet. This
change ports that functionality to the coordinator dashboard.

Updates golang/go#34744

Change-Id: I38f6b2e1a1805fb24b9e387a5737ff8c2057b625
Reviewed-on: https://go-review.googlesource.com/c/build/+/222197
Run-TryBot: Alexander Rakoczy <alex@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Carlos Amedee <carlos@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 22, 2020

Change https://golang.org/cl/244137 mentions this issue: app/appengine: stop hiding some tested configurations for x repos

gopherbot pushed a commit to golang/build that referenced this issue Jul 22, 2020
The build dashboard can be used to view builds for the main Go repo
and other golang.org/x repos. The isUntested invocation was invalid
for repos other than the main one, which was causing results for some
tested configurations to become hidden (incorrectly replaced by '•').

Larger changes are needed before there's sufficient data to compute
the GoBranch value for all repos reliably, so for now, just update
the isUntested invocation to apply only for the main repo.

This prioritizes the ability to view test results over the ability
to see that some builds are intentionally missing because they are
configured not to run. Doing both is a part of future work.

For golang/go#40290.
For golang/go#34744.
For golang/go#28643.

Change-Id: Id43cb47abacb1036f578efbb8232ae17ad40eca9
Reviewed-on: https://go-review.googlesource.com/c/build/+/244137
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alexander Rakoczy <alex@golang.org>
Reviewed-by: Carlos Amedee <carlos@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 22, 2020

Change https://golang.org/cl/244398 mentions this issue: cmd/coordinator: restore partial support for -mode=dev

@dmitshur dmitshur self-assigned this Jul 23, 2020
gopherbot pushed a commit to golang/build that referenced this issue Jul 24, 2020
Set gceMode earlier in InitGCE. Its value is used by some of the code
that runs inside InitGCE.

Don't try to run gcePool.pollQuotaLoop in dev mode. Make the code more
clear and consistent about this and createBasepinDisks calls.

Merge oAuthHTTPClient into the "Initialized by InitGCE" var block above.

Remove initGCECalled, it has become unused.

For golang/go#34744.
For golang/go#38337.

Change-Id: Ic47870fa9aed0eded0cfdf14e18e63c1acda4554
Reviewed-on: https://go-review.googlesource.com/c/build/+/244398
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Carlos Amedee <carlos@golang.org>
Reviewed-by: Alexander Rakoczy <alex@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 28, 2020

Change https://golang.org/cl/245277 mentions this issue: maintner/maintnerd/maintapi/version: support beta and RC release tags

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.