Improvements for remote controller code by jedevc · Pull Request #1620 · docker/buildx

jedevc · 2023-02-10T14:48:46Z

A few commits with various improvements for the remote controller code.

Don't kill the remote controller immediately after build. We should keep the server alive, since the client can't know exactly what's running on the controller - also we want the behavior for debugging or not to remain relatively similar. At some point in the future, we could introduce logic to the server where it can shut itself down, if no builds are currently being run, however, the server should dictate this, instead of each individual client.
Add more informative server exit messages, so that the reasons for exit are always available in a log.
Add signal handling for SIGTERM (in addition to the existing SIGINT). SIGKILL can be caused by pkill and similar programs, which is useful for debugging, etc.
Switch to using context.Context for timeout handling with GRPC instead of a custom loop - this simplifies the code, makes it more configurable, and gives us better performance.
Use unique files for each buildx version. Instead of only having a single controller server, we allow for having multiple, one for each version of buildx, so we should never have any version clashes, meaning user interaction is less necessary. This means we can dramatically change the protocol between buildx versions, without needing to worry about any form of compatibility.
- We also should keep separate log files for each server - but I'm not quite sure how that should interact with a log_file specified in config.toml. Ideas for this welcome 😄

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 <[email protected]>

When exiting, we should ideally always print a message, and give details as to exactly what error we received. Signed-off-by: Justin Chadwell <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

ktock

Thank you, overall LGTM

ktock · 2023-02-11T10:08:08Z

+	defaultLogFilename    = fmt.Sprintf("buildx.%s.log", version.Revision)
+	defaultSocketFilename = fmt.Sprintf("buildx.%s.sock", version.Revision)
+	defaultPIDFilename    = fmt.Sprintf("buildx.%s.pid", version.Revision)


nit: Should be encoded not to include "/" (e.g. sha256?)

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 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

// key string used for kubernetes "sticky" mode

contextPathHash, err := filepath.Abs(in.ContextPath)

if err != nil {

contextPathHash = in.ContextPath

}

. Then sha1 enc.

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.

Added a follow-up in #1637.

jedevc · 2023-02-14T11:18:29Z

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 /, which is likely different to the working directory (so relative paths will resolve to the wrong location).

Signed-off-by: Justin Chadwell <[email protected]>

ktock

Thank you. LGTM.

jedevc added 5 commits February 10, 2023 14:40

build: don't kill remote controller after build

56b9e78

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 <[email protected]>

controller: add more informative server exit messages

1b91bc2

When exiting, we should ideally always print a message, and give details as to exactly what error we received. Signed-off-by: Justin Chadwell <[email protected]>

controller: exit cleanly on SIGTERM

abda257

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 <[email protected]>

controller: use unique files per buildx version

ed9ea24

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 <[email protected]>

jedevc requested review from ktock and tonistiigi February 10, 2023 14:48

jedevc added the area/debug label Feb 10, 2023

ktock reviewed Feb 11, 2023

View reviewed changes

controller: set absolute path of server binary before execution

54f4dc8

Signed-off-by: Justin Chadwell <[email protected]>

jedevc force-pushed the remote-controller-fixes branch from d850278 to 54f4dc8 Compare February 14, 2023 11:21

jedevc mentioned this pull request Feb 15, 2023

monitor: Enable to exec into the container #1626

Merged

ktock approved these changes Feb 16, 2023

View reviewed changes

jedevc requested a review from crazy-max February 16, 2023 11:25

jedevc mentioned this pull request Feb 16, 2023

version: add fallback to fill revision from BuildInfo #1637

Closed

crazy-max approved these changes Feb 21, 2023

View reviewed changes

jedevc merged commit ed4fd96 into docker:master Feb 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements for remote controller code#1620

Improvements for remote controller code#1620
jedevc merged 6 commits intodocker:masterfrom
jedevc:remote-controller-fixes

jedevc commented Feb 10, 2023

Uh oh!

ktock left a comment

Uh oh!

ktock Feb 11, 2023

Uh oh!

jedevc Feb 13, 2023

Uh oh!

crazy-max Feb 16, 2023 •

edited

Loading

Uh oh!

crazy-max Feb 16, 2023

Uh oh!

jedevc Feb 16, 2023

Uh oh!

jedevc commented Feb 14, 2023

Uh oh!

ktock left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	// key string used for kubernetes "sticky" mode
	contextPathHash, err := filepath.Abs(in.ContextPath)
	if err != nil {
	contextPathHash = in.ContextPath
	}

Conversation

jedevc commented Feb 10, 2023

Uh oh!

ktock left a comment

Choose a reason for hiding this comment

Uh oh!

ktock Feb 11, 2023

Choose a reason for hiding this comment

Uh oh!

jedevc Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

crazy-max Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crazy-max Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

jedevc Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

jedevc commented Feb 14, 2023

Uh oh!

ktock left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

crazy-max Feb 16, 2023 •

edited

Loading