-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
Better error messages when IP address autodetection fails #25271
Better error messages when IP address autodetection fails #25271
Conversation
Thanks for doing this! My only feedback is that the Other than that, it looks good. |
a02d51c
to
11b9940
Compare
Thanks @aaronlehmann the pull request has been updated. |
LGTM |
@@ -223,14 +223,16 @@ ifaceLoop: | |||
// and exactly one IPv6 address, favor IPv4 over IPv6. | |||
if interfaceAddr4 != nil { | |||
if systemAddr != nil { | |||
return nil, errMultipleIPs | |||
return nil, fmt.Errorf("could not choose an IP address to advertise since this system has multiple addresses (%s on %s and %s on %s) on different interfaces", systemAddr, systemInterface.Name, interfaceAddr4, intf.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor suggestion: Move the addresses and interfaces after "on different interfaces".
could not choose an IP address to advertise since this system has multiple addresses on different interfaces (%s on %s and %s on %s)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @aaronlehmann, PR has been updated.
This fix tries to improve error messages when IP address autodetection fails, as is specified in 25141. Previously, error messages only indicate that multiple IPs exist when autodetection fails. In this fix, if one interface consists of multiple addresses or multiple interfaces consist of addresses, the error messages output the address names and interface names so that end user could take notice. This fix is verified manually. When multiple addresses exist on multiple interfaces: ``` $ sudo docker swarm init Error response from daemon: could not choose an IP address to advertise since this system has multiple addresses on different interfaces (192.168.186.128 on ens33 and 192.168.100.199 on eth10) - specify one with --advertise-addr ``` When multiple addresses exist on single interface: ``` $ sudo docker swarm init Error response from daemon: could not choose an IP address to advertise since this system has multiple addresses on interface ens33 (192.168.186.128 and 192.168.55.199) - specify one with --advertise-addr ``` This fix fixes 25141. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
11b9940
to
59db010
Compare
LGTM 👼 |
@aaronlehmann this is for 1.13? or should this be included in 1.12.1 if we do a patch release? |
@thaJeztah: I don't know if it should be in 1.12.1 according to the general policy, since it is not a fix for a serious bug. That said, it's a low-risk change that will help people understand errors - so it seems like it would be nice to have in a patch release. |
Given that auto-detection isn't perfect yet, I think this may help us debugging issues, so I'm adding this to the 1.12.1 milestone /cc @tiborvass |
This fix tries to improve error messages when IP address autodetection fails, as is specified in #25141.
Previously, error messages only indicate that multiple IPs exist when autodetection fails. In this fix, if one interface consists of multiple addresses or multiple interfaces consist of addresses, the error messages output the address names and interface names so that end user could take notice.
This fix is verified manually.
When multiple addresses exist on multiple interfaces:
When multiple addresses exist on single interface:
This fix fixes #25141.
Signed-off-by: Yong Tang yong.tang.github@outlook.com