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

cmd/go: can't run go builds concurrently if they download modules #26794

Closed
allenpetersen opened this issue Aug 3, 2018 · 20 comments
Closed
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@allenpetersen
Copy link

What version of Go are you using (go version)?

go1.11beta2

What operating system and processor architecture are you using (go env)?

amd64 docker container

Our current build workflow is to run glide install first and then start a number of go build jobs in parallel.

When I take a clean system and start the go build jobs in parallel they fail due to issues downloading modules, git shallow.lock files and race conditions creating and populating /go/src/mod directories.

If I first do something like go get ./foo ./bar ./baz to download the modules I can run the builds in parallel.

Is there an equivalent of glide install or dep ensure?
Is there a plan to make downloading of modules thread safe?

@ianlancetaylor ianlancetaylor changed the title Workflow modules and parallel builds cmd/go: can't run go builds in parallel if they download modules Aug 3, 2018
@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. modules labels Aug 3, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone Aug 3, 2018
@ianlancetaylor
Copy link
Contributor

CC @bcmills

@rsc
Copy link
Contributor

rsc commented Aug 7, 2018

For now it should suffice to do 'go list ./foo ./bar ./baz' before your builds.

There is a plan to make downloading of modules by parallel go commands
safe but we haven't done that yet.

There is also a plan to provide 'go mod download' to prefetch all the
downloaded modules. That could also stand in for 'glide install' but
go list is enough and exists today.

Why are you running multiple go build commands in parallel instead of
a single go build command?

@allenpetersen
Copy link
Author

Why are you running multiple go build commands in parallel instead of
a single go build command?

We have many services in a monorepo written in a number of different languages. The top level CI builder doesn't know about what language they are written it. It just invokes a number of Makefiles most of which happen to be Go programs.

@apparentlymart
Copy link

We have a different "multiple builds at once" use-case at hashicorp/terraform: we build the same application source (and thus the same dependencies) for a number of different GOOS/GOARCH pairs concurrently during our release process.

We are currently doing this with gox, which is a wrapper around go build that deals with templating the executable output paths to produce a systematic result. A number of other projects at HashiCorp, and I believe also elsewhere, are using this tool in the same way.

It sounds like it may be viable for gox to simply run an additional single command to completion before it starts its parallel work to ensure that all of the dependencies are available. Would that be go list ./..., or similar?

@wsc1
Copy link

wsc1 commented Aug 25, 2018

We have multiple builds at once at zikichombo.org as well. It is a set of libraries, each a module with very heterogenous versioning projected. We are counting on the module system to allow independent releases, but internally have a need to download and build the whole set of libraries as part of work flow.

wfscheper added a commit to wfscheper/hugo that referenced this issue Sep 15, 2018
This change avoids the need to copy the source out of the GOPATH by
applying GO111MODULE=on as an environment variable. Additionally, avoids
issues with parallel module downloads (see golang/go#26794 and
golang/go#27372) by running 'go mod download' during the install step.
wfscheper added a commit to wfscheper/hugo that referenced this issue Sep 15, 2018
This change avoids the need to copy the source out of the GOPATH by
applying GO111MODULE=on as an environment variable. Additionally, avoids
issues with parallel module downloads (see golang/go#26794 and
golang/go#27372) by running 'go mod download' during the install step.
wfscheper added a commit to wfscheper/hugo that referenced this issue Sep 15, 2018
This change avoids the need to copy the source out of the GOPATH by
applying GO111MODULE=on as an environment variable. Additionally, avoids
issues with parallel module downloads (see golang/go#26794 and
golang/go#27372) by running 'go mod download' during the install step.
@bcmills bcmills changed the title cmd/go: can't run go builds in parallel if they download modules cmd/go: can't run go builds concurrently if they download modules Sep 25, 2018
@thepudds
Copy link
Contributor

thepudds commented Oct 10, 2018

Russ wrote this above comment #26794 (comment) back in August:

There is also a plan to provide 'go mod download' to prefetch all the
downloaded modules. That could also stand in for 'glide install' but
go list is enough and exists today.

As brief update, go mod download (documentation) now exists in 1.11, and hence that can now be used instead of go list ./foo ./bar ./baz before a build.

@allenpetersen
Copy link
Author

I did switch our build process to call go mod download as a pre-build step and that is working fine for us. This works around the race condition.

I haven't gone back to test this but the issue of having parallel builds fail due to races for the on disk module cache may still remain.

@gopherbot
Copy link

Change https://golang.org/cl/141218 mentions this issue: cmd/go: support background processes in TestScript

@gopherbot
Copy link

Change https://golang.org/cl/145177 mentions this issue: internal/syscall: add LockFileEx and UnlockFileEx for use in cmd/go on Windows

@gopherbot
Copy link

Change https://golang.org/cl/145178 mentions this issue: cmd/go/internal/lockedfile: add package and support library

gopherbot pushed a commit that referenced this issue Oct 29, 2018
This will be used to test fixes for bugs in concurrent 'go' command
invocations, such as #26794.

See the README changes for a description of the semantics.

Updates #26794

Change-Id: I897e7b2d11ff4549a4711002eadd6a54f033ce0b
Reviewed-on: https://go-review.googlesource.com/c/141218
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/151341 mentions this issue: cmd/go/internal/modfetch: make Repo.Zip write to an io.Writer instead of a temporary file

@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 28, 2018
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 28, 2018
gopherbot pushed a commit that referenced this issue Nov 29, 2018
renameio.WriteFile writes files atomically by renaming temporary files.

See the subsequent changes for usage examples.

Updates #26794
Updates #22397

Change-Id: I4bfe3125a53f58060587f98afbb4260bb1cc3d32
Reviewed-on: https://go-review.googlesource.com/c/146377
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Giovanni Bajo <rasky@develer.com>
Reviewed-by: Russ Cox <rsc@golang.org>
gopherbot pushed a commit that referenced this issue Nov 29, 2018
Updates #26794

Change-Id: I2a50e3b756ff6a2bbaee4737ca7ed053b01c8d0e
Reviewed-on: https://go-review.googlesource.com/c/146378
Reviewed-by: Russ Cox <rsc@golang.org>
gopherbot pushed a commit that referenced this issue Nov 29, 2018
lockedfile.File passes through to os.File, with Open, Create, and OpenFile
functions that mimic the corresponding os functions but acquire locks
automatically, releasing them when the file is closed.

lockedfile.Sentinel is a simplified wrapper around lockedfile.OpenFile for the
common use-case of files that signal the status of idempotent tasks.

lockedfile.Mutex is a Mutex-like synchronization primitive implemented in terms
of file locks.

lockedfile.Read is like ioutil.Read, but obtains a read-lock.

lockedfile.Write is like ioutil.Write, but obtains a write-lock and can be used
for read-only files with idempotent contents.

Updates #26794

Change-Id: I50f7132c71d2727862eed54411f3f27e1af55cad
Reviewed-on: https://go-review.googlesource.com/c/145178
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
gopherbot pushed a commit that referenced this issue Nov 29, 2018
Also check to make sure we don't overwrite a newer timestamp with an
older one.

testexpire.txt may be written concurrently, and a partially-written
timestamp may appear much older than the actual intended one. We don't
want to re-run tests that should still be cached.

Updates #26794

Change-Id: If56348e799f0e7be3c5bc91b4a336e23ad99f791
Reviewed-on: https://go-review.googlesource.com/c/146379
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
gopherbot pushed a commit that referenced this issue Nov 29, 2018
… of a temporary file

This will be used to eliminate a redundant copy in CL 145178.

(It also decouples two design points that were previously coupled: the
destination of the zip output and the program logic to write that
output.)

Updates #26794

Change-Id: I6cfd5a33c162c0016a1b83a278003684560a3772
Reviewed-on: https://go-review.googlesource.com/c/151341
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
gopherbot pushed a commit that referenced this issue Nov 29, 2018
We employ the following new locking mechanisms:

• Zip files and list files within the module cache are written using
  atomic renames of temporary files, so that GOPROXY servers reading
  from the cache will never serve incomplete content.

• A lock file for each module version guards downloading and extraction of
  (immutable) module contents.

• A lock file alongside each version list (named 'list.lock')
  guards updates to the list.

• A single lock file in the module cache guards updates to all go.sum
  files. The go.sum files themselves are written using an atomic
  rename to ensure that we never accidentally discard existing sums.

Updates #26794

RELNOTE=yes

Change-Id: I16ef8b06ee4bd7b94d0c0a6f5d17e1cecc379076
Reviewed-on: https://go-review.googlesource.com/c/146382
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
gopherbot pushed a commit that referenced this issue Nov 29, 2018
Use an arbitrary lockfile to serialize edits, and use atomic renames
to actually write the go.mod file so that we never drop version
requirements due to a command failing partway through a write.

Multiple invocations of the 'go' command may read the go.mod file
concurrently, and will see some consistent version even if some other
invocation changes it concurrently.

Multiple commands may attempt to write the go.mod file concurrently.
One writer will succeed and write a consistent, complete go.mod file.
The others will detect the changed contents and fail explicitly: it is
not, in general, possible to resolve two conflicting changes to module
requirements, so we surface the problem to the user rather than trying
to solve the problem heuristically.

Updates #26794

Change-Id: Ia1a06a01ef93fa9be664f560eb83bb86b0207443
Reviewed-on: https://go-review.googlesource.com/c/146380
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
gopherbot pushed a commit that referenced this issue Nov 29, 2018
The lockfile guards calls that may change the repo's filesystem contents.

We don't know how robust VCS implementations are to running
simultaneous commands, and this way we don't need to care: only one
'go' command at a time will modify any given repository.

If we can guarantee that particular VCS implementations are robust
enough across all of the VCS tool versions we support, we may be able
to remove some of this locking to improve parallelism.

Updates #26794

Change-Id: I578524974f5015629239cef43d3793aee2b9075c
Reviewed-on: https://go-review.googlesource.com/c/146381
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
@bcmills
Copy link
Contributor

bcmills commented Nov 29, 2018

The stack of changes to fix concurrent invocations has landed, and should be included in the Go 1.12 beta.

If you see any module cache corruption on a build of the go command from commit a2b4ac6 or later, please open a new issue with details.

@bcmills bcmills closed this as completed Nov 29, 2018
@gopherbot
Copy link

Change https://golang.org/cl/151798 mentions this issue: cmd/go/internal/modfetch: make directories read-only after renaming, not before

gopherbot pushed a commit that referenced this issue Nov 30, 2018
…not before

The call to os.Rename was failing on the darwin builders, despite having passed in the TryBots.
(I tested this change by running 'go test cmd/go' manually on a darwin gomote.)

This fixes the builder failures after CL 146382.

Updates #26794
Fixes #29030

Change-Id: I3644773421789f65e56f183d077b4e4fd17b8325
Reviewed-on: https://go-review.googlesource.com/c/151798
Reviewed-by: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Nov 29, 2019
@rsc rsc unassigned bcmills Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

8 participants