Initial attempt at SRV based cluster discovery

[dalehamel]

@slackpad for review.

This is an initial attempt at supporting SRV-based discovery for #305.

If the code looks familiar, it's because I blatantly stole it from the mdns discovery code. I've done an initial test manually and things seem to work.

As written it should:

• Accept an SRV record to look up as 'srvname'
• Periodically (every minute) poll that record and attempt to join all peers listed within the record

Possible enhancements:

• Mark seen hosts, as mdns does
• sort returned SRV hosts by priority (SRV includes priorities), and join highest priority first.
• Support for recursive SRV (not in the spec, and a bit of a weird use case, but it does happen)

[dalehamel]

I've added a second commit that allows for multiple SRV records to be supplied, though I'm not sure if the correct convention would be to stick with how tags do it (define -tags multiple times to get a list), or if accepting a string that is a CSV is fine.

[dalehamel]

I've updated again so that it uses AppendSliceValue to allow multiple records to be specified.

[dalehamel]

I've done real-world test, and this replaces our previous approach of a crontab quite nicely.

[dalehamel]

ping @ryanuber as you also seem like a good reviewer here

[dalehamel]

ping

[dalehamel]

sorry to keep nagging, any chance we can get some reviewers on this or chat about it in IRC?

[slackpad]

@dalehamel I should be able to look at this tonight or tomorrow. Working through a backlog of things, but this is on the list :-)

[dalehamel]

Great, thanks! Sorry for nagging again :)

On Tuesday, September 8, 2015, James Phillips notifications@github.com
wrote:

@dalehamel https://github.com/dalehamel I should be able to look at
this tonight or tomorrow. Working through a backlog of things, but this is
on the list :-)

—
Reply to this email directly or view it on GitHub
#310 (comment).

[dalehamel]

@slackpad friendly reminder to bump this again :)

[slackpad]

Should add a period to the end here.

[dalehamel]

done

[slackpad]

Looking good - I made an initial pass with some feedback.

[dalehamel]

thanks @slackpad, addressing your comments now ❤️

[dalehamel]

@slackpad I've implemented the single-loop approach and it's much simpler. I'm going to add the seen and members stuff you suggested, as I had previously been considering that as well.

[dalehamel]

@slackpad I've made the changes you've suggested here and am ready for another review:

• It's done using a single loop, waking every 60 seconds to try and join again. The actual call to findSRV has a timeout of 30 seconds, so that goroutines can't pile up, and we don't block forever waiting on it.
• Members are checked on every run, and we only return SRV records for members not in the cluster. Join will be called only when there are new members to try.
  • Note - this can be done repeatedly if the SRV record is bad, and it's pointing to a bad host. This should be evident from the logs.

I've tested this locally and it seems to behave as expected:

• I threw a sleep into findSRV that was longer than the timeout interval, and it causes the timeout to happen, but a new goroutine gets spawned on the next iteration after sleeping
• I've created a dummy SRV record that points to localhost at two different ports, and once they have found each other they stop looking.
• If a node leaves, it can still be re-added (online alive nodes are considered in the memberlist)

I am not maintaining a separate 'seen' hash, as I think that's unnecessary extra complexity, as we should assume that the SRV record is good (or will soon be good, once the TTL rolls around).

[slackpad]

I don't think this will do what you expect since the channel is made inside the loop. Even if you hoist it up, I think this'll accumulate goroutines that are blocked trying to write to the channel. I'd just make a blocking call to findSRV() and let it do its thing since there's not a good way to "recall" it if it does timeout.

[slackpad]

Had an interesting conversation with @ryanuber about this as well. Would it make more sense to make this a startup option like -join_srv and -retry_join_srv, and you'd basically call this once or in a loop until it succeeds? If a new server is coming up, it will use this to join the cluster once it finds somebody alive in the SRV list to join to. This will save the ongoing polling because only the people who want to join will be polling, everybody else is good because they are part of the cluster (it won't hurt anything, but it won't really be useful).

[dalehamel]

Yeah I agree, I had considered that this might be happening (I'm super new at go), but thanks for confirming it. I'll drop it back to a simple loop, and add the startup option you suggest.

[dalehamel]

@slackpad I believe I've addressed your comment on more closely mirroring join and retry-join.

Please let me know if this doesn't do what you meant with respect to wan/lan profiles, but I think what you meant was to use the existing values for join retry attempts / intervals.

Ready for another pass at the review here!

[slackpad]

This is totally what I was thinking. The one remaining WAN-related detail (if you think SRV records would be useful for WAN joining in addition to LAN joining) is to add WAN versions of these. Instead of copy-paste the LAN ones, if we could pass a function down to do the join ( then I think you could re-use all of this stuff. Basically you'd pass in a function that calls agent.Join() for the LAN version, and agent.JoinWAN() for the LAN version.

[dalehamel]

@slackpad it might be useful, but I think it could probably make it into a different PR as for now it's basically YAGNI, at least for us.

Aside from that, are we good to merge here?

[slackpad]

Made another pass and noted a few things. I think it's super close - only remaining bits are the comments from this round, some unit tests, and doc updates.

[ryanuber]

These should probably be underscores to follow the config file convention of all of the other options.

[dalehamel]

@slackpad @ryanuber I addressed most of the comments here.

Maybe the best way forward would be to just drop the polling as you guys suggested, and work on a separate PR to add that so that we can at least get the SRV stuff into core.

For the time being, we can continue to use a cron record and will still benefit from having SRV support in serf core.

[dalehamel]

@slackpad @ryanuber as mentioned in my previous comment, i've gone ahead and removed the polling and performed the simplifications requested.

Ready for another pass, as I believe all comments have been addressed.

[slackpad]

The latest round looks good - can you add some unit tests and update the Serf docs (they are under /website in the same repo)?

Agree it would be good to get the startup SRV support in for now, and then add the polling later in a follow-on PR if needed.

[dalehamel]

@slackpad will do, i'm not too familiar with unit testing in go but i'll try and reverse engineer what you guys have.

One thing i was considering was adding SRV support to the join command as well, I'll take a look at scoping that out.

[dalehamel]

@slackpad I've hit a bit of a road block on the unit testing.

I need to fake an SRV record in some way, but I'm not sure how to stub this out in an elegant way.

Do you have any advice on how to approach this? I'm not sure what the best way to stub a DNS resolver in golang would be.

[slackpad]

Hi @dalehamel - check out Consul's DNS unit test, it does this, specifically the framework-y part:

https://github.com/hashicorp/consul/blob/master/command/agent/dns_test.go#L51-L72

And here's an example usage of it:

https://github.com/hashicorp/consul/blob/master/command/agent/dns_test.go#L322-L325

[dalehamel]

great, thanks @slackpad i'll take a look.

[F21]

Any update on this?

[ssenaria]

Bump for an update.

[jason-riddle]

?

[joeyguerra]

This is super old. Is anyone actively maintaining this?

[hashicorp-cla]

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes

_{Have you signed the CLA already but the status is still pending? Recheck it.}

[dnephin]

Sorry for the silence on this PR. We had a look at this as part of trying to clean up some older PRs. The functionality seems to make sense. Other than a rebase, I think we would need a test to cover the new functionality.

If anyone is still interested in this and is able to reabse and add a test we can keep it open. Otherwise we'll probably close this PR in a few months.