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: make GOROOT read-only before running tests #30316

Open
bcmills opened this issue Feb 19, 2019 · 20 comments
Milestone

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Feb 19, 2019

When #28387 is fixed, we should make sure that it doesn't regress.

Can we configure the builders to make GOROOT read-only before they invoke run.bash, and perhaps restore write permissions after the tests have finished?

(CC @bradfitz @dmitshur)

@gopherbot gopherbot added this to the Unreleased milestone Feb 19, 2019
@gopherbot gopherbot added the Builders label Feb 19, 2019
@dmitshur

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Feb 19, 2019

Is it better to catch regressions to #28387 by making builders have a read-only GOROOT and hope that tests fail if they try to write, or by having a writeable GOROOT and try to verify (after tests have finished running) that GOROOT hasn't been modified?

I'm not sure if it's viable to do the latter, but I wanted to ask so that we consider it.

@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Feb 19, 2019

and perhaps restore write permissions after the tests have finished?

After the tests have finished we destroy the VM, so not point restoring write access moments before death.

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Feb 19, 2019

@dmitshur, we could do both, but it is more important to run with a read-only GOROOT and check for failing tests: tests that leave changes behind are easy for developers to detect (via git status), so they slip by much less frequently than tests that make and then unmake ephemeral changes.

The latter category is still harmful, since they will fail when users run go test all (a command intended to become more useful in module mode) with a distro-packaged copy of the Go toolchain.

Ephemeral changes can also interfere with proper cache invalidation, so we should avoid them even if GOROOT is writable.

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Feb 19, 2019

(It occurs to me, though, that we already have a “files opened” check in the cache subsystem. Probably we should check for — and reject — file writes in GOROOT explicitly there.)

@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Feb 21, 2019

The buildlet already has a recursive file list endpoint, so the build system could also just look at all files before & after a test run to see if there are any differences. This is what x/build/cmd/gomote does to decide how to incrementally push files from developer machines to buildlets.

That might be faster in that it's only doing read operations and not writes.

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Feb 21, 2019

As noted above, merely looking at the files before and after the test run is not sufficient: we don't want tests to make ephemeral changes, even if they back them out before exiting.

@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Feb 21, 2019

Ah, sorry, missed that comment.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Feb 22, 2019

Change https://golang.org/cl/163477 mentions this issue: cmd/dist: make GOROOT unwritable on builders

@bcmills bcmills added the NeedsFix label Feb 28, 2019
gopherbot pushed a commit that referenced this issue May 14, 2019
Updates #30316

Change-Id: I57e489f6bbe4a3b39c907dabe5ac41fb9939cdb4
Reviewed-on: https://go-review.googlesource.com/c/go/+/163477
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 8, 2019

Change https://golang.org/cl/189537 mentions this issue: cmd/release: create release after make.bash and before all.bash

@Lekensteyn

This comment has been minimized.

Copy link
Contributor

@Lekensteyn Lekensteyn commented Aug 13, 2019

At least the contents of go1.13beta1.linux-amd64.tar.gz tarball from https://golang.org/dl/ is read-only, is that an intentional side-effect? I noticed this when trying to use patch to patch some sources in GOROOT.

Lekensteyn added a commit to cloudflare/tls-tris that referenced this issue Aug 13, 2019
Since go commit 02d24fc25285, the GOROOT directory has been made
read-only, this affects the go1.13beta1 binary tarball as used by Travis
CI. See golang/go#30316
@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Aug 13, 2019

gopherbot pushed a commit to golang/build that referenced this issue Aug 21, 2019
Binary releases need to build Go and include binaries such as bin/go,
bin/gofmt, and others. Previously, this was accomplished by running
all.bash script for some GOOS/GOARCH pairs, and make.bash for others
where it wasn't viable to run tests as part of the release process.

This change makes the release process more consistent by always
packaging the release archive file after running make.bash. We still
run all.bash in situations where it was previously run, but we do so
after the release file has already been created. This avoids the
risk of any changes to GOROOT that may occur as part of all.bash
(including changing file permissions to be read-only) being included
in the final release file.

Add a step to check that files in the buildlet's $WORKDIR/go and
$WORKDIR/go/bin directories have expected permissions before
creating the release file.

Fixes golang/go#33537
Updates golang/go#30316

Change-Id: I7d40716dba656a8aca711377f2995df4880166c5
Reviewed-on: https://go-review.googlesource.com/c/build/+/189537
Reviewed-by: Andrew Bonventre <andybons@golang.org>
Lekensteyn added a commit to cloudflare/tls-tris that referenced this issue Aug 27, 2019
Since go commit 02d24fc25285, the GOROOT directory has been made
read-only, this affects the go1.13beta1 binary tarball as used by Travis
CI. See golang/go#30316
@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Nov 11, 2019

Making GOROOT unwritable currently seems to be ineffective for some (?) tests on some (nearly all‽) of the builders: see #32407 (comment).

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 11, 2019

Change https://golang.org/cl/206458 mentions this issue: misc: ensure that test overlay directories are writable

gopherbot pushed a commit that referenced this issue Nov 11, 2019
Otherwise, the test cannot create new files in the directory.

Updates #32407
Updates #30316

Change-Id: Ief0df94a202be92f57d458d4ab4e4daa9ec189b1
Reviewed-on: https://go-review.googlesource.com/c/go/+/206458
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Nov 12, 2019

After the tests have finished we destroy the VM, so not point restoring write access moments before death.

Is that also true for reverse builders (such as host-linux-arm5spacemonkey)?

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Nov 12, 2019

The other reason to restore access, of course, is so that we can test this behavior locally without infuriating ourselves.

(I should be able to run GO_BUILDER_NAME=linux-amd64-bcmills ./all.bash and get a reasonable simulation of what an actual builder would run.)

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 12, 2019

Change https://golang.org/cl/206757 mentions this issue: cmd/dist: save and restore original permissions in makeGOROOTUnwritable

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 12, 2019

Change https://golang.org/cl/206758 mentions this issue: cmd/dist: fail in makeGOROOTUnwritable if running as root

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Nov 12, 2019

Bingo: all of our linux- TryBots run tests as the root user of the container, so CL 163477 has no effect on them.

(https://storage.googleapis.com/go-build-log/726e41e1/linux-amd64_acf59a73.log)

gopherbot pushed a commit that referenced this issue Nov 12, 2019
Also log a message and skip the Chmods if running as root.

Updates #30316

Change-Id: Ifb68d06ce845275a72d64c808407e8609df270bc
Reviewed-on: https://go-review.googlesource.com/c/go/+/206757
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 13, 2019

Change https://golang.org/cl/206898 mentions this issue: cmd/dist: remove chatty log.Print

gopherbot pushed a commit that referenced this issue Nov 13, 2019
In CL 206757 I added a log.Printf to identify when GOROOT is not read-only.
However, it interacts badly with test sharding in the builders:
the log is repeated for every shard.

Since the log statement isn't particularly high-value, just remove it.

Updates #30316

Change-Id: I385a7f35da59e38ad8b9beef92dc11af931d9571
Reviewed-on: https://go-review.googlesource.com/c/go/+/206898
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Carlos Amedee <carlos@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 13, 2019

Change https://golang.org/cl/206901 mentions this issue: misc/cgo/testgodefs: convert test from bash to Go

gopherbot pushed a commit that referenced this issue Nov 13, 2019
The bash version of the test wrote intermediate files to its testdata directory.

Updates #28387
Updates #30316
Fixes #35536

Change-Id: Ib81b547d3c43e90df713a2172c8f399fefb53c68
Reviewed-on: https://go-review.googlesource.com/c/go/+/206901
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
codebien added a commit to codebien/build that referenced this issue Nov 13, 2019
Binary releases need to build Go and include binaries such as bin/go,
bin/gofmt, and others. Previously, this was accomplished by running
all.bash script for some GOOS/GOARCH pairs, and make.bash for others
where it wasn't viable to run tests as part of the release process.

This change makes the release process more consistent by always
packaging the release archive file after running make.bash. We still
run all.bash in situations where it was previously run, but we do so
after the release file has already been created. This avoids the
risk of any changes to GOROOT that may occur as part of all.bash
(including changing file permissions to be read-only) being included
in the final release file.

Add a step to check that files in the buildlet's $WORKDIR/go and
$WORKDIR/go/bin directories have expected permissions before
creating the release file.

Fixes golang/go#33537
Updates golang/go#30316

Change-Id: I7d40716dba656a8aca711377f2995df4880166c5
Reviewed-on: https://go-review.googlesource.com/c/build/+/189537
Reviewed-by: Andrew Bonventre <andybons@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.