Allow specifying persistent peer ids#3014
Allow specifying persistent peer ids#3014sinkingpoint wants to merge 2 commits intoprometheus:mainfrom
Conversation
sinkingpoint
force-pushed
the
sinkingpoint/persistant_peer_ids
branch
from
179b407 to
7bc0cf7
Compare
simonpasquier
left a comment
simonpasquier
left a comment
There was a problem hiding this comment.
Thanks! As I said in #2285, I see no real reason why the peer names are auto-generated. And in kubernetes land, we could rely on the statefulset names to guarantee uniqueness.
It would be nice to have a new acceptance test to verify that it works. I've tested locally and it seems to work fine though name conflicts aren't easy to detect (they appear in the memberlist logs but those are only available at the debug level). I suppose that we'd need to implement the ConflictDelegate interface (https://pkg.go.dev/github.com/hashicorp/memberlist#ConflictDelegate) for the delegate struct?
| clusterBindAddr = kingpin.Flag("cluster.listen-address", "Listen address for cluster. Set to empty string to disable HA mode."). | ||
| Default(defaultClusterAddr).String() | ||
| clusterAdvertiseAddr = kingpin.Flag("cluster.advertise-address", "Explicit address to advertise in cluster.").String() | ||
| clusterPeerName = kingpin.Flag("cluster.peer-name", "Explicit name of the peer, rather than generating a random one").Default("").String() |
There was a problem hiding this comment.
Maybe insist that peer names should be unique across all nodes of the cluster?
sinkingpoint
force-pushed
the
sinkingpoint/persistant_peer_ids
branch
from
7bc0cf7 to
89eefc9
Compare
|
Thanks for the review @simonpasquier ! I've added in an acceptance test to test the behaviour. Detecting duplicate names is an interesting problem... Yes - we can implement the ConflictDelegate interface, but the question is what to do with that information - we can log it (easy), or attempt to shut down one of the duplicates, but that, in practice gets a bit messy. In particular, identifying which peer is the "invalid" one is kind of difficult - memberlist doesn't have the idea of an "authoritative" node for a given name |
|
Sorry for the lag (PTO interruption on my side). I agree that there's no practical that we can decide which peer is more legitimate than the other. I think that logging the issue + adding a metric/alert would be good enough. WDYT? |
|
Apologies for leaving this... I think that sounds reasonable. Will push that today |
Currently the default behaviour is to generate a new, random peer id on every startup. This makes clustering a bit weird, so this commit adds in an optional cli arg that allows specifying a custom peer id that we can persist across runs Signed-off-by: sinkingpoint <colin@quirl.co.nz>
sinkingpoint
force-pushed
the
sinkingpoint/persistant_peer_ids
branch
from
89eefc9 to
15904d6
Compare
This adds in the delegate as a `memberlist.ConflictDelegate` so that we get a callback when we encounter conflicts in peer ids which we then log and increment a new metric to allow monitoring the conflict condition Signed-off-by: sinkingpoint <colin@quirl.co.nz>
sinkingpoint
force-pushed
the
sinkingpoint/persistant_peer_ids
branch
from
15904d6 to
c32479d
Compare
|
Fixed in #4636 |
3 participants
Currently the default behaviour is to generate a new, random peer id on
every startup. This makes clustering a bit weird, so this commit adds in
an optional cli arg that allows specifying a custom peer id that we can
persist across runs
This addresses #2285
Signed-off-by: sinkingpoint colin@quirl.co.nz