Added PeerManagement.Type = 'member-list'

[thrawn01]

Which problem is this PR solving?

This PR removes the need to run Redis in the deployment environment. All member-list requires is a DNS entry
pointing to a single active instance of Refinery to discover and monitor the health of the rest of the cluster. See https://github.com/hashicorp/memberlist

This allows customers who are using nomad, k8s or consul to deploy Refinery without the need for Redis.

Short description of the changes

• This PR builds on the changes in Added PeerManagement.Timeout config option #491
• Added three new config options MemberListListenAddr, MemberListKnownMembers and MemberListAdvertiseAddr
• Added Close(ctx context.Context) error to the Peers interface to allow for clean shutdown and de-registration of an instance in the member-list cluster.

Race condition in Peers interface design

Since peer.NewPeers() launches async go routines for both redis and member-list. it is possible that a callback could occur before we call RegisterUpdatedPeersCallback()to register the call back, therefore missing the initial callback.

For example, in memberlist_test.go we should be able to assert that p0 will receive 3 callbacks, one for itself, and then 2 for each additional member. However, because of the race condition, we might only get 2 callbacks.

We could change the Peers interface to instead register a callback during peer.NewPeers(conf, cb) call OR we could add a Start() method to the Peers interface, such that users would RegisterUpdatedPeersCallback() and then call Start() when ready to participate in receiving updates.

[kentquirk]

I haven't reviewed this yet, but the preferred fix to the race condition problem would be the Start() method approach.

In fact, to keep this more manageable, any chance you could make that fix and the change to the PeerManagement API as a separate PR from adding the memberlist support?

[Baliedge]

I've found an unexpected behavior in the peering functionality that pertains to both Redis and this new Memberlist modes.

When in Redis mode, it will bind a peer listener on the public IP, overriding whatever was set for a bind IP in config GetPeerListenAddr(). Derrick brought this logic into memberlist, but it ultimately caused unit test issues because it would bind to the public IP and the tests would connect to 127.0.0.1 and fail.

For memberlist mode, I removed the public IP logic to fix unit tests because they wanted to bind to localhost, not public IP. But this creates a disparity with Redis mode and I don't think it's my place to change the behavior of pre-existing logic.

My recommendation is to remove the configuration override that binds to public IP only. But, you may also provide a GetPublicIP() function. This way, devs have a choice when populating the configuration structure.

cc: @kentquirk

[Baliedge]

@thrawn01, I'm having some trouble getting this to work. There's some bugs in the peer list update once memberlist discovers a peer. But the real problem is that in Nomad, we can't effectively point it to other peers to prime the memberlist mesh. The consul service alias doesn't get created until after the service is up and running and health check passes. But, it's failing on startup because it can't find the alias and the health check fails.

I'm going to give etcd a try in a separate PR.

[MikeGoldsmith]

Thanks for creating the PR @thrawn01. It's very thorough 👍🏻

This is interesting for us and we are open to adding support for MemberLists. As @Baliedge indicated, somethings are not working as expected with nomad.

Please can you take a look?

[Baliedge]

After discussion with @MikeGoldsmith, we decided that Memberlist probably isn't a good solution for our use case. It seems to work well when you have a dedicated cluster master with a static host that you can point peers to. But that isn't our environment. Under our Nomad environment all nodes are dynamic and may come and go with every new deployment.

Instead, I feel it would be best to focus on integrating using our existing etcd service as we do with our other services.

Additionally, there are two behaviors that don't impact production, but prevent end-to-end integration testing.

First, the peer listener binds to the detected public IP, overriding the IP set in PeerListenAddr configuration. This is by design with the assumption you would never need multiple peers on the same host. However, this impacts testability because you only have 1 IP to work with.

Second, peering logic identifies peers by IP address only, without the port. As a result, peering would not support multiple peers on same host but different ports. This design requires each peer to have different IPs and bind on the same port. As before, this also impacts testability.

We agreed to leave these two behaviors unchanged in our peering changes. They may be addressed in later efforts.

[MikeGoldsmith]

I'm going to close this for now. If someone wants to continue working on this, with mind of the points @Baliedge highlighted above, please re-open :)