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/tools/godoc: Dockerize and convert to run on GAE Flex #27205

Open
andybons opened this Issue Aug 24, 2018 · 33 comments

Comments

Projects
None yet
7 participants
@andybons
Member

andybons commented Aug 24, 2018

godoc relies on https://golang.org/doc/go1.10#goroot but GAE Classic is still running Go 1.9.

Convert to use GAE Flex so that we can rid ourselves of these limitations (and deploy godoc from the release-branch.go1.11)

/cc @bradfitz @dmitshur @katiehockman

@andybons andybons added this to the Unreleased milestone Aug 24, 2018

@andybons andybons self-assigned this Aug 24, 2018

@bradfitz

This comment has been minimized.

Member

bradfitz commented Aug 24, 2018

Milestone: Go 1.6 :)

@bradfitz

This comment has been minimized.

Member

bradfitz commented Aug 24, 2018

Also, because we're using a Go 1.10 x/tools on golang.org now, certain links don't work from the release notes & blog post, like:

https://golang.org/cmd/go/#hdr-Modules__module_versions__and_more

Because the godoc heading rules. This should be a heading:

Modules, module versions, and more

But it renders as a plain paragraph at the moment.

@dmitshur

This comment has been minimized.

Member

dmitshur commented Aug 24, 2018

Similarly, this statement from https://golang.org/doc/go1.11#godoc is not yet true on https://golang.org/pkg/:

The godoc web server now shows which version of Go introduced new API features.

As a workaround, in the meantime, you can refer to https://tip.golang.org/pkg/. E.g.:

https://tip.golang.org/pkg/os/#UserCacheDir

@steren

This comment has been minimized.

steren commented Aug 30, 2018

FYI, Go 1.11 on the App Engine standard environment is coming.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Aug 30, 2018

@steren, that doesn't help us. App Engine's Go runtime by definition will always be behind what we need, because we have to release Go before App Engine can release a Go runtime for that version. So we can't use App Engine's provided runtimes. We can only do a custom runtime (BYODockerfile).

So we're moving off App Engine classic.

@steren

This comment has been minimized.

steren commented Aug 30, 2018

I see, yes in that case, a Dockerfile approach seems to be appropriate.

@broady

This comment has been minimized.

Member

broady commented Aug 30, 2018

Checklist of things to do.

  • Create Dockerfile
  • Update deployment process/script to use env: flex
  • Consider introducing another build flag to include "golang.org" stuff (short, dl). Currently the "appengine" build tag is used for this (see appinit.go). An environment variable might be even better.

Done already: networking/routing. We did this work already about a year ago.

Cannot use "google.golang.org/appengine" packages in Flex, so these must be migrated. Significant packages/code that needs to be updated:

  • cmd/godoc/appinit.go (main entrypoint for golang.org)
  • godoc/dl (downloads page uses datastore/memcache)
  • godoc/short (short URL facility, uses datastore/memcache)

I guess the main questions are what we use to replace the memcache bits in dl and short. It's probably right to use golang.org/x/playground's cache.

Relevant deps of cmd/godoc, which will all need to be checked for any appengine stuff:

golang.org/x/tools/blog
golang.org/x/tools/blog/atom
golang.org/x/tools/godoc
golang.org/x/tools/godoc/analysis
golang.org/x/tools/godoc/dl
golang.org/x/tools/godoc/proxy
golang.org/x/tools/godoc/redirect
golang.org/x/tools/godoc/short
golang.org/x/tools/godoc/static
golang.org/x/tools/godoc/util
golang.org/x/tools/godoc/vfs
golang.org/x/tools/godoc/vfs/httpfs
golang.org/x/tools/godoc/vfs/mapfs
golang.org/x/tools/godoc/vfs/zipfs
golang.org/x/tools/present
@gopherbot

This comment has been minimized.

gopherbot commented Sep 4, 2018

Change https://golang.org/cl/133355 mentions this issue: godoc: migrate to App Engine flexible

@broady

This comment has been minimized.

Member

broady commented Sep 4, 2018

One more thing we need to do: move the shortlink admin service to another GAE app or just a local command.

It uses appengine/user to check the viewer has admin access. There's no simple migration for that (need to set up OAuth/Firebase Auth).

@broady broady self-assigned this Sep 5, 2018

@bradfitz

This comment has been minimized.

Member

bradfitz commented Sep 5, 2018

How about we migrate the shortlinks to a plain text file stored in git?

The admin service is broken even for classic App Engine on custom domains anyway and when I want to add a new link, I spend more time remembering/finding the ugly workaround URL than I would if I just TBR'd a CL to a data file.

We can cache the data file in memory and on cache miss (for new links) fault to Gerrit for fetching the file at HEAD, rate limited to once every 5 minutes or so.

@broady

This comment has been minimized.

Member

broady commented Sep 5, 2018

I like that, it also gives non-admins a list of available shortlinks.

Where should the list live? x/tools/godoc/short/links.tsv?

@bradfitz

This comment has been minimized.

Member

bradfitz commented Sep 5, 2018

That works. But slight preference for links.txt(or links.dat) instead and not requiring tabs. I'd just use any whitespace. Ignore comment lines starting with '#' otherwise just use strings.Fields and get first element is short key and second is full URL.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Sep 25, 2018

It's now been over a month for this "Soon" bug.

What remains?

@andybons

This comment has been minimized.

Member

andybons commented Sep 26, 2018

Ping @broady, who last I heard had a working version locally and was working on the deploy scripts. He's in Sydney at the moment so there may be some delay, though.

@broady

This comment has been minimized.

Member

broady commented Sep 28, 2018

Update... what remains:

  • porting over the patches in existing deployment scripts:
    • google analytics
    • hash.map/hash.go for mercurial -> git redirects
  • package listing is missing, but package docs work just fine (this is because the package listing looks at GOROOT/pkg, which is empty because I do a clean checkout of the git repo)

Fast-follow:

  • ability to add shortlinks (old app had a login, GAE Flex doesn't have authed handlers)
  • update hostname filtering to allow newly deployed versions - so we can smoke test before migrating traffic to the new version (hostnames look like 20180907t155022-dot-golang-org.appspot.com) - currently controlled with GODOC_ENFORCE_HOSTS env var.
  • port over other stuff from internal deployment scripts:
    • regression tests
    • version number prettification
  • remove zipfs (#27959)
  • remove zipping from build process (generate-index.bash)
  • build a given Go version from scratch in the builder phase, rather than using FROM golang
@broady

This comment has been minimized.

Member

broady commented Sep 28, 2018

Update: well, I sank several hours into figuring out why the package listing was empty.

It looks like there have been numerous changes to x/tools/godoc/vfs, my best guess is CL 101295.

Precisely, this change to zipfs:
golang/tools@16d1af8#diff-6b09edd71f61887ea342b82eb207149b

/cc @agnivade

@agnivade

This comment has been minimized.

Member

agnivade commented Sep 28, 2018

Yes, I made that change. But it was also fixed later with #27162. Are you sure you have the latest 1.11 ?

So this comment -

this is because the package listing looks at GOROOT/pkg, which is empty because I do a clean checkout of the git repo

is not true anymore. Package listing will look at the resolved goroot now - which should be the bin/ folder inside a go distribution where godoc, and go are kept together. Plus, if there is any confusion, you can pass the -goroot flag yourself.

Let me know if I can be of any further help.

@broady

This comment has been minimized.

Member

broady commented Sep 28, 2018

Yes, I made that change. But it was also fixed later with #27162. Are you sure you have the latest 1.11 ?

It's broken for zipfs. It works fine on real filesystem. See the exact lines linked two posts above.

"abspath" is a path to a file, not a directory, so it'll almost never match. So, RootType is almost always empty, which results in an empty entry in the package list. (strangely, a <tr> is still emitted.)

Anyway, this is a discussion for a new issue, not this one.

@agnivade

This comment has been minimized.

Member

agnivade commented Sep 28, 2018

Ah, my apologies for this ! Will send a fix.

@gopherbot

This comment has been minimized.

gopherbot commented Sep 28, 2018

Change https://golang.org/cl/138435 mentions this issue: godoc/vfs/zipfs: join paths to get correct RootType

gopherbot pushed a commit to golang/tools that referenced this issue Sep 28, 2018

godoc/vfs/zipfs: join paths to get correct RootType
The logic was incorrectly written from vfs/os.go. zipfs filesystem
passes actual paths during dirTree building. So we need to join the paths
to determine whether they are under GOROOT or GOPATH.

Updates golang/go#27205

Change-Id: Ic4330fce02c6ebfc44ae21122e2412c33f0cd45a
Reviewed-on: https://go-review.googlesource.com/138435
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

gopherbot pushed a commit to golang/tools that referenced this issue Oct 2, 2018

godoc: migrate to App Engine flexible
See bug for more details on exactly what was migrated.

Notably:
* No more Google-internal deployment scripts; see README.godoc-app and
  the Makefile for details.
* Build tag "golangorg" is used for the godoc configuration used for
  golang.org.
* Use of App Engine libraries replaced with GCP client libraries.
* Redis is used to replace App Engine memcache.
* Google analytics is controlled by an environment variable.
* Regression tests have been migrated from Google-internal.
* hg -> git hash map is moved from Google-internal.

Updates golang/go#27205.

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

This comment has been minimized.

gopherbot commented Oct 2, 2018

Change https://golang.org/cl/138978 mentions this issue: cmd/godoc: improve deployment scripts, add buildinfo

gopherbot pushed a commit to golang/tools that referenced this issue Oct 2, 2018

cmd/godoc: improve deployment scripts, add buildinfo
* Build Go from a given version (make.bash)
* Add a /buildinfo file that describes the inputs of the
  build/deployment.
* Use Makefile/environment variables to override Go version and
  Docker tag.

Updates golang/go#27205.

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

This comment has been minimized.

gopherbot commented Oct 3, 2018

Change https://golang.org/cl/139237 mentions this issue: cmd/godoc: re-enable host checking, allow test versions

@gopherbot

This comment has been minimized.

gopherbot commented Oct 3, 2018

Change https://golang.org/cl/139238 mentions this issue: cmd/godoc: move regression tests to a go test

gopherbot pushed a commit to golang/tools that referenced this issue Oct 3, 2018

cmd/godoc: re-enable host checking, allow test versions
test.golang.org is no longer -- instead allow access to version-specific
App Engine URLs (like 20181002t1342-dot-golang-org.appspot.com).

App Engine Flex uses the X-Forwarded-Proto to signify the proto used by
the originating request (it always uses h1 on 8080 when proxying the
request).

Updates golang/go#27205.

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

gopherbot pushed a commit to golang/tools that referenced this issue Oct 3, 2018

cmd/godoc: move regression tests to a go test
Run them separately from the other tests in godoc_test by requiring a
regtest.host flag and by filtering on the test name.

Updates golang/go#27205.

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

This comment has been minimized.

Member

agnivade commented Oct 3, 2018

Now that site is live, is there anything else pending on this ?

@broady

This comment has been minimized.

Member

broady commented Oct 3, 2018

@agnivade yes, see the unchecked items in the thread.

I'm also going to:

  • build Docker image using Cloud Build, so the only dependency (other than Make) for the person deploying is gcloud. At present, the person deploying must have Docker installed.
@gopherbot

This comment has been minimized.

gopherbot commented Oct 3, 2018

Change https://golang.org/cl/139239 mentions this issue: cmd/godoc: add make publish to migrate traffic

@agnivade

This comment has been minimized.

Member

agnivade commented Oct 3, 2018

Ah thanks. Though I think removing zipfs is orthogonal to this issue, and should not block finishing this task.

@gopherbot

This comment has been minimized.

gopherbot commented Oct 3, 2018

Change https://golang.org/cl/139240 mentions this issue: cmd/godoc: add cloud build config

@agnivade

This comment has been minimized.

Member

agnivade commented Oct 3, 2018

@broady - I still don't see the version tags that are supposed to appear.

See - https://tip.golang.org/pkg/net/http/#SameSite vs https://golang.org/pkg/net/http/#SameSite. There is a "1.11" in the previous. Am I missing something ?

@broady

This comment has been minimized.

Member

broady commented Oct 3, 2018

Yes, I was wondering about that, too. I haven't debugged it, yet.

edit: I think I found it...

	// Initialize the version info before readTemplates, which saves
	// the map value in a method value.
	corpus.InitVersionInfo()

This needs to be added to appinit.go.

I'll add it.

edit: see golang.org/cl/139557

gopherbot pushed a commit to golang/tools that referenced this issue Oct 4, 2018

cmd/godoc: add `make publish` to migrate traffic
Also rename `make build` and `make push` to `make docker-build` and
`make docker-push` in preparation to introduce Cloud Build (removing
the dependency on Docker).

Updates golang/go#27205.

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

gopherbot pushed a commit to golang/tools that referenced this issue Oct 4, 2018

cmd/godoc: add cloud build config
Deploys no longer depend on Docker.

With only Make and gcloud installed, the following should deploy a new version:

$ git clone https://go.googlesource.com/tools
$ cd tools
$ cd cmd/godoc
$ make cloud-build deploy

Updates golang/go#27205.

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

This comment has been minimized.

Member

broady commented Oct 6, 2018

One more...

  • remove build tag from main.go (i.e., merge appinit.go and main.go)
@dmitshur

This comment has been minimized.

Member

dmitshur commented Nov 1, 2018

It seems this issue is mostly resolved, with a minor cleanup followup task left. We should either remove the Soon label, since I don't think it applies now, or close this and open a new issue for the remaining cleanup task.

(/cc @andybons since you added the Soon label.)

@andybons andybons removed the Soon label Nov 1, 2018

@andybons

This comment has been minimized.

Member

andybons commented Nov 1, 2018

Removed soon label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment