Make nodenames unique in Gossip cluster #1451
Conversation
|
|
||
| var nodeName string | ||
| if len(hostname)+len(id) > 63 { | ||
| nodeName = hostname[:63-len(id)] + "-" + id |
There was a problem hiding this comment.
I think if len(hostname) == 63, the extra - will make nodename 64 bytes long.
There was a problem hiding this comment.
Its similar to an fqdn where individual components are 63 bytes with a . in between.
There was a problem hiding this comment.
IOW, with - the length can be 64 bytes.
There was a problem hiding this comment.
Ah, ok I did not read well the comment, I thought you wanted to keep the 63 length restriction for the nodename as well, but it actually says hostname.
|
|
||
| var nodeName string | ||
| if len(hostname)+len(id) > 63 { | ||
| nodeName = hostname[:63-len(id)] + "-" + id |
There was a problem hiding this comment.
Ah, ok I did not read well the comment, I thought you wanted to keep the 63 length restriction for the nodename as well, but it actually says hostname.
| id := stringid.TruncateID(stringid.GenerateRandomID()) | ||
|
|
||
| var nodeName string | ||
| if len(hostname)+len(id) > 63 { |
There was a problem hiding this comment.
Why are you trying to truncate nodeName to < 64 bytes?
There was a problem hiding this comment.
Since we have the random ID now the names are guaranteed to be unique. So there is no need to use the entire hostname which could be of different lengths across hosts. Truncating is just to keep the nodename length consistent across the cluster.
There was a problem hiding this comment.
But hostnames are more meaningful and if somebody used hostnames in their cluster with same prefix then there is a potential that we truncate the differentiating parts and keep the same part and it would be hard to read logs if that is the case. Do we really need to keep node name length consistent across the cluster?
There was a problem hiding this comment.
It does not keep it consistent becasue if a hostname is 10 chars long and another is 12 chars long, the cluster seens nodenames will be 23 and 25 chars long.
There was a problem hiding this comment.
Neither memberlist or nor our code has restriction on the node name length. Since keeping the hosntame fully is the preferred option I will change it.
| @@ -247,9 +248,12 @@ func (c *controller) agentInit(bindAddrOrInterface, advertiseAddr string) error | |||
|
|
|||
| keys, tags := c.getKeys(subsysGossip) | |||
| hostname, _ := os.Hostname() | |||
| nodeName := hostname + "-" + stringid.TruncateID(stringid.GenerateRandomID()) | |||
| logrus.Infof("Gossip cluster hostname ", nodeName) | |||
There was a problem hiding this comment.
If you want to keep this log, please add : %s
Signed-off-by: Santhosh Manohar <santhosh@docker.com>
|
LGTM |
Conflicting names in the cluster can lead to silent failures in the service discovery, routing mesh etc. When joining the cluster make sure the nodenames are unique.
Signed-off-by: Santhosh Manohar santhosh@docker.com