Add retry logic and diagnostics for buildkitd connection timeout#440
Conversation
Agent-Logs-Url: https://github.com/joshuabremerdexcom/earthbuild/sessions/cc114f9b-f70a-4d95-a569-8ebc603fbe64 Co-authored-by: joshuabremerdexcom <59564543+joshuabremerdexcom@users.noreply.github.com>
ddd1967 to
d41349c
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces a connectWithRetry function in buildkitd/buildkitd.go to implement a retry mechanism for buildkitd connections, accompanied by diagnostic logging. Additionally, it removes unused Echo v5 dependencies from monorepo examples and updates various dependency versions. Review feedback suggests refining the logging strategy to reduce noise by deferring diagnostics until a failure occurs and improving code readability by avoiding variable initializers within if statements.
| infos, err := fe.ContainerInfo(ctx, containerName) | ||
| if err != nil { | ||
| bkCons.Warnf("Warning: could not check %q container status: %v\n", containerName, err) | ||
| } else if ci, ok := infos[containerName]; ok { | ||
| bkCons.Printf("Container %q status: %s\n", containerName, ci.Status) | ||
| } else { | ||
| bkCons.Printf("Container %q not found\n", containerName) | ||
| } |
There was a problem hiding this comment.
In accordance with the project's general rules, avoid using an if statement with an initializer. Declaring the variable on a preceding line improves readability and ensures compatibility with linting rules.
infos, err := fe.ContainerInfo(ctx, containerName)
if err != nil {
bkCons.Warnf("Warning: could not check %q container status: %v\n", containerName, err)
return
}
ci, ok := infos[containerName]
if ok {
bkCons.Printf("Container %q status: %s\n", containerName, ci.Status)
} else {
bkCons.Printf("Container %q not found\n", containerName)
}References
- In Go, avoid using an if statement with an initializer. Instead, declare the variable on a preceding line to prevent potential conflicts with project linting rules.
| } | ||
| } | ||
|
|
||
| logContainerAndDockerStatus() |
There was a problem hiding this comment.
Calling logContainerAndDockerStatus() at the start of connectWithRetry adds unnecessary noise to the console output during the happy path (when the first connection attempt succeeds). Since maybeRestart calls this function on every execution when a container is already running, this information will be printed frequently. Consider removing this call so diagnostics are only logged if a connection attempt fails.
| return info, workerInfo, nil | ||
| } | ||
|
|
||
| bkCons.VerbosePrintf("Connection attempt %d to buildkitd failed: %v\n", attempt, connErr) |
There was a problem hiding this comment.
To provide visibility when connection issues occur without cluttering the happy path, consider logging the container and Docker status after the first failed attempt.
bkCons.VerbosePrintf("Connection attempt %d to buildkitd failed: %v\n", attempt, connErr)
if attempt == 1 {
logContainerAndDockerStatus()
}|
Sorry, i was trying this out in my own fork. I'm still figuring this out before i try to contribute to this repo |
Agent-Logs-Url: https://github.com/joshuabremerdexcom/earthbuild/sessions/cc114f9b-f70a-4d95-a569-8ebc603fbe64