Improvements for remote controller code #1620
Conversation
We don't know if other builds might be running, etc, so we should allow the server to decide when to exit. Signed-off-by: Justin Chadwell <me@jedevc.com>
When exiting, we should ideally always print a message, and give details as to exactly what error we received. Signed-off-by: Justin Chadwell <me@jedevc.com>
This signal may be sent using an external tool such as pkill, and since we can handle it neatly, we should. Signed-off-by: Justin Chadwell <me@jedevc.com>
This patch improves timeout logic for testing and creating a buildx server. Instead of needing to have a custom loop, we can just use the primitives provided by go and grpc for easier handling. Additionally, without the explicit time.Sleeps, we defer to GRPCs exponential backoff algorithm which should provide substantially better results. Signed-off-by: Justin Chadwell <me@jedevc.com>
This ensures that we should never accidentally connect to a server with a mismatched version, while also allowing us to run multiple buildx servers at a time. Signed-off-by: Justin Chadwell <me@jedevc.com>
| defaultLogFilename = fmt.Sprintf("buildx.%s.log", version.Revision) | ||
| defaultSocketFilename = fmt.Sprintf("buildx.%s.sock", version.Revision) | ||
| defaultPIDFilename = fmt.Sprintf("buildx.%s.pid", version.Revision) |
There was a problem hiding this comment.
nit: Should be encoded not to include "/" (e.g. sha256?)
There was a problem hiding this comment.
I think these revisions are already sha256-encoded, they're derived from the git revision.
@crazy-max does this make sense to you? Is there a different version identifier that might make more sense here / do we need to add a round of sha256 to remove special characters?
There was a problem hiding this comment.
There is an edge case where version.Revision can be empty if not built from the Dockerfile. I think we could use the same pattern as the kubernetes driver with contextPathHash if that makes sense:
buildx/controller/build/build.go
Lines 182 to 186 in 74f64f8
sha1 enc.
There was a problem hiding this comment.
As discussed I got confused as this is for buildx server and not build commands. We could use https://pkg.go.dev/runtime/debug#ReadBuildInfo and read vcs.revision.
There was a problem hiding this comment.
Added a follow-up in #1637.
|
Added an updated commit to ensure that we take the absolute path of the buildx binary, since on exec we change the working directory to |
Signed-off-by: Justin Chadwell <me@jedevc.com>
d850278 to
54f4dc8
Compare
A few commits with various improvements for the remote controller code.
context.Contextfor timeout handling with GRPC instead of a custom loop - this simplifies the code, makes it more configurable, and gives us better performance.log_filespecified inconfig.toml. Ideas for this welcome 😄