fix(containers): improve port binding error message for clarity #596
Conversation
Summary of ChangesHello @ilopezluna, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the user experience when encountering port binding conflicts during container startup. By detecting specific port-in-use errors, the system can now provide a more actionable and user-friendly error message, guiding users directly to a potential solution rather than just reporting a generic failure. This change aims to reduce user frustration and streamline troubleshooting for common setup issues. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
isPortBindingErrorcheck relies on matching specific substrings in the error message, which is brittle across Docker versions/localizations; consider checking for typed errors or well-known error codes if available instead of string contains. - The enhanced error message doesn't indicate which port/host failed; including the actual
host:portvalues in the user-facing message would make it clearer which binding is conflicting. - The returned error string mixes
fmt.Errorfformatting with embedded newlines, which can be hard to log and wrap consistently; consider returning a single-line error and letting the caller handle multi-line user guidance or using a dedicated error type for user hints.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `isPortBindingError` check relies on matching specific substrings in the error message, which is brittle across Docker versions/localizations; consider checking for typed errors or well-known error codes if available instead of string contains.
- The enhanced error message doesn't indicate which port/host failed; including the actual `host:port` values in the user-facing message would make it clearer which binding is conflicting.
- The returned error string mixes `fmt.Errorf` formatting with embedded newlines, which can be hard to log and wrap consistently; consider returning a single-line error and letting the caller handle multi-line user guidance or using a dedicated error type for user hints.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the error handling for port binding failures by detecting when a port is already in use and providing a more informative error message to the user. This is a great enhancement for user experience. The implementation introduces a helper function to check for specific error strings and then uses it to return a custom error. My review includes a couple of suggestions to improve maintainability by extracting hardcoded strings into constants.
| func isPortBindingError(err error) bool { | ||
| if err == nil { | ||
| return false | ||
| } | ||
| errStr := err.Error() | ||
| return strings.Contains(errStr, "ports are not available") && | ||
| (strings.Contains(errStr, "address already in use") || | ||
| strings.Contains(errStr, "Only one usage of each socket address")) | ||
| } |
There was a problem hiding this comment.
To improve readability and maintainability, it's a good practice to extract magic strings into named constants. This makes the code easier to understand and modify in the future.
func isPortBindingError(err error) bool {
if err == nil {
return false
}
const (
portBindingErrorStr = "ports are not available"
addressInUseErrorStr = "address already in use"
socketAddressUsageErrorStr = "Only one usage of each socket address"
)
errStr := err.Error()
return strings.Contains(errStr, portBindingErrorStr) &&
(strings.Contains(errStr, addressInUseErrorStr) ||
strings.Contains(errStr, socketAddressUsageErrorStr))
}| if isPortBindingError(err) { | ||
| return fmt.Errorf("failed to start container %s: %w\n\nThe port may already be in use by Docker Desktop's model runner.\nTry running: docker desktop disable model-runner", controllerContainerName, err) | ||
| } |
There was a problem hiding this comment.
To improve code clarity and make long strings more manageable, consider defining this error format string as a constant.
if isPortBindingError(err) {
const portInUseErrorFmt = "failed to start container %s: %w\n\nThe port may already be in use by Docker Desktop's model runner.\nTry running: docker desktop disable model-runner"
return fmt.Errorf(portInUseErrorFmt, controllerContainerName, err)
}
| errStr := err.Error() | ||
| return strings.Contains(errStr, "ports are not available") && | ||
| (strings.Contains(errStr, "address already in use") || | ||
| strings.Contains(errStr, "Only one usage of each socket address")) |
There was a problem hiding this comment.
How do you get this one?
There was a problem hiding this comment.
You can force it by using install-runner when DD has TCP enabled (its the same that @krissetto got: #526)
| const tlsCertContainerPath = "/etc/model-runner/certs" | ||
|
|
||
| // isPortBindingError checks if the error indicates a port is already in use | ||
| func isPortBindingError(err error) bool { |
There was a problem hiding this comment.
Maybe it'd be nice to actually check whether it's another DMR instance running on that port, perhaps by checking http://localhost:12434/engines/status.
There was a problem hiding this comment.
@doringeman Actually that would require to do this extra http request every single time. Because it does only affect to DD users that install vllm on WSL I prefer to not add the additional check and just add the hint in the error message.
Co-authored-by: Dorin-Andrei Geman <dorin.geman@docker.com>
Address #526 by providing the command to disable DD TCP: