-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
Comments
Change https://golang.org/cl/227768 mentions this issue: |
Change https://golang.org/cl/227141 mentions this issue: |
Change https://golang.org/cl/227769 mentions this issue: |
Change https://golang.org/cl/227920 mentions this issue: |
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>
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>
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>
Change https://golang.org/cl/243337 mentions this issue: |
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>
Change https://golang.org/cl/244398 mentions this issue: |
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>
Change https://golang.org/cl/247907 mentions this issue: |
Change https://golang.org/cl/247908 mentions this issue: |
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>
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>
Change https://golang.org/cl/368674 mentions this issue: |
Change https://golang.org/cl/368675 mentions this issue: |
Change https://golang.org/cl/368676 mentions this issue: |
This change moves the function which returns the appropriate pool for the configuration passed in into the pool package. This work is being done as part of a project to break the coordinator into seperate parts. Updates golang/go#48742 Updates golang/go#38337 Change-Id: Ie5b3fc2da6534fca6e55ba6bb710db5e206efe00 Reviewed-on: https://go-review.googlesource.com/c/build/+/368674 Trust: Carlos Amedee <carlos@golang.org> Run-TryBot: Carlos Amedee <carlos@golang.org> Reviewed-by: Heschi Kreinick <heschi@google.com> Reviewed-by: Alex Rakoczy <alex@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org>
This change moves the coordinator event logging into a package. Updates golang/go#38337 Updates golang/go#48742 Change-Id: If3714ca741f48ba703e4585e3cbe3755e66b8613 Reviewed-on: https://go-review.googlesource.com/c/build/+/368675 Trust: Carlos Amedee <carlos@golang.org> Run-TryBot: Carlos Amedee <carlos@golang.org> Reviewed-by: Heschi Kreinick <heschi@google.com> Reviewed-by: Alex Rakoczy <alex@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org>
This moves the coordinator scheduler into a package. The span has also been moved into the schedule package. It also adds a mostly uimplemented fake scheduler. Updates golang/go#38337 Updates golang/go#48742 Change-Id: I980241e8e8ba2acafa38f732fe480e66d9d3a4f3 Reviewed-on: https://go-review.googlesource.com/c/build/+/368676 Trust: Carlos Amedee <carlos@golang.org> Run-TryBot: Carlos Amedee <carlos@golang.org> Reviewed-by: Heschi Kreinick <heschi@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
This issue was created when we were going to add builders on Ec2. That work has been completed. This issue should be scoped for that work. We've also moved away from using the coordinator. I believe this issue has been completed. Please feel free to reopen if you feel this was closed in error. |
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
The text was updated successfully, but these errors were encountered: