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: move away from package level variables and global state #38337

Open
cagedmantis opened this issue Apr 9, 2020 · 8 comments
Open

x/build: move away from package level variables and global state #38337

cagedmantis opened this issue Apr 9, 2020 · 8 comments
Milestone

Comments

@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented Apr 9, 2020

Many of the packages in x/build have code which contain package level variables. Certain packages use these package level variables in a way where they functionally become global state. It is difficult to write effective unit tests for these packages. It is also difficult to trace all the interaction points between portions of code because of the use of global state. This issue should be used to track an effort to reduce package level variables and global state while at the same time increasing test coverage.

/cc @toothrot @dmitshur @andybons

@gopherbot gopherbot added this to the Unreleased milestone Apr 9, 2020
@gopherbot gopherbot added the Builders label Apr 9, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 10, 2020

Change https://golang.org/cl/227768 mentions this issue: internal/pool: move the kubernetes buildlet pool into a pool package

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 10, 2020

Change https://golang.org/cl/227141 mentions this issue: internal/pool: move the gce buildlet pool into a pool package

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 10, 2020

Change https://golang.org/cl/227769 mentions this issue: internal/pool: move the reverse buildlet pool into a pool package

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 10, 2020

Change https://golang.org/cl/227920 mentions this issue: internal/pool: remove package level accessors

@andybons andybons added the NeedsFix label Apr 10, 2020
gopherbot pushed a commit to golang/build that referenced this issue Apr 20, 2020
This CL creates the internal/coordinator/pool package intended to
contain all buildlet pool implementations. In order to keep this
change small and carefully discover where the interactions are
between the gce buildlet pool and the rest of the coordinator
are, this change only moves the gce buildlet over to the new
package.

The next steps will be to move the rest of the buildlet pools
over to this package. After that we will restructure the
implementations themselves in order to increase test coverage
and increase the ease of testing.

Updates golang/go#36841
Updates golang/go#38337

Change-Id: If82ae1b584bd77c697aa84fadf9011c9e79fa409
Reviewed-on: https://go-review.googlesource.com/c/build/+/227141
Run-TryBot: Carlos Amedee <carlos@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alexander Rakoczy <alex@golang.org>
gopherbot pushed a commit to golang/build that referenced this issue Apr 20, 2020
This is a set in a series of steps which will move everything buildlet
pool related into a pool package.

Updates golang/go#36841
Updates golang/go#38337

Change-Id: Ic7a0ccd7838345036df2e72b13084070541cb63c
Reviewed-on: https://go-review.googlesource.com/c/build/+/227769
Run-TryBot: Carlos Amedee <carlos@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alexander Rakoczy <alex@golang.org>
gopherbot pushed a commit to golang/build that referenced this issue Apr 20, 2020
This change moves all package level accessors into a struct as
suggested by Alex:
https://go-review.googlesource.com/c/build/+/227141/6/internal/coordinator/pool/gce.go#227
This will make it easier to move away from global state in
future changes.

Updates golang/go#36841
Updates golang/go#38337

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

@gopherbot gopherbot commented Jul 17, 2020

Change https://golang.org/cl/243337 mentions this issue: internal/pool: create a common buildlet naming function

gopherbot pushed a commit to golang/build that referenced this issue Jul 20, 2020
The logic for naming buildlets appears in each particular buildlet
pool implementation. This change creates a function which contains
the buildlet naming logic. This is being added before the addition of
a new buildlet pool.

Updates golang/go#36841
Updates golang/go#38337

Change-Id: I8c03f9b513efde14414bcc6d823f3cf1a59c8f70
Reviewed-on: https://go-review.googlesource.com/c/build/+/243337
Run-TryBot: Carlos Amedee <carlos@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
Reviewed-by: Alexander Rakoczy <alex@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

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 Aug 12, 2020

Change https://golang.org/cl/247907 mentions this issue: internal/coordinator/pool: add ec2 pool

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 17, 2020

Change https://golang.org/cl/247908 mentions this issue: cmd/coordinator: enable EC2 buildlet pool

gopherbot pushed a commit to golang/build that referenced this issue Aug 18, 2020
The EC2 buildlet pool added by this commit will manage the lifecycle
of buildlets running on EC2. EC2 VMs will only be created for the
ARM64 architecture. As VMs are requested, the pool will ensure that
there are enough resources for the VM to be created and keep track of
the VMs created. Once a VM is destroyed, the resources will be made
available for other VMs. This pool will only keep instances as they
are needed.

Updates golang/go#36841
Updates golang/go#38337

Change-Id: Ic777485c0b0a69ec13726c58b49e9fdc1df4777e
Reviewed-on: https://go-review.googlesource.com/c/build/+/247907
Run-TryBot: Carlos Amedee <carlos@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alexander Rakoczy <alex@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit to golang/build that referenced this issue Aug 18, 2020
This change enables the EC2 buildlet pools.

Updates golang/go#38337
Fixes golang/go#36841

Change-Id: I3f9384c039dce8fab529650bc6acdbeda40f5819
Reviewed-on: https://go-review.googlesource.com/c/build/+/247908
Run-TryBot: Carlos Amedee <carlos@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alexander Rakoczy <alex@golang.org>
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
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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