Support for multiple responses in mDNS #559

inercia · 2015-04-14T15:51:51Z

This pull request is the stepping stone for full support of multiple responses in WeaveDNS. It focuses in the mDNS client and server, where it provides

support for "insistent" Lookups (maybe not the best name): mDNS queries that try to obtain as many responses as possible for up to 500ms (and, in consequence, they block for that period of time). This will be used in the next PR by a "updater" process that will refresh the ZoneDb information by sending periodic queries to peers...
support for multiple responses from all the Lookup*() methods
a common ZoneRecord interface for many structs that contain names and IPs (used in Lookup methods, but also in the future database records)
support for multiple records when building packets with make*() methods
improved unit tests for mDNS

nameserver/addrs.go

+
+/////////////////////////////////////////////////////////////
+
+// simple IPv4 type that can be used as a key in a map (in contrast with net.IP), used for sets of IPs, etc...


nameserver/addrs.go

+func ipToIPv4(nip net.IP) IPv4  { ip := nip.To4(); return IPv4([4]byte{ip[0], ip[1], ip[2], ip[3]}) }
+
+// Parse an address (eg, "10.13.12.11") and return the corresponding IPv4 (eg, "10.13.12.11")
+func addrToIPv4(addr string) IPv4 {


inercia · 2015-04-20T15:06:41Z

@awh I have fixed the parsing mechanism for IP addresses as you suggested, and I have also added a unit test for that...

awh · 2015-04-22T16:34:01Z

It looks like a response containing multiple addresses is only permuted if the answer comes from the cache. Shouldn't shuffleAnswers be moved outside the cache so that it can treat lookups from the local zone and the mDNS client in the same way?

awh · 2015-04-22T16:37:52Z

In the case where queryHandler is asked to resolve a name which is a) not in the cache and b) has records in the Zone local to the weaveDNS instance, it looks like we never go out to the rest of the weaveDNS instances to see if they have additional answers for that name? The presence of an entry locally short circuits the loop over the ZoneLookup array...

inercia · 2015-04-23T09:07:13Z

@awh That's right: the presence of an entry locally ends the lookup. We currently do not support multiple responses to clients, so once you have a valid answer to the query, you are done.

And regarding shuffleAnswers, I would prefer to delay that change, as we currently we do not really shuffle answers (we do not support multiple answers yet, so there is nothing to shuffle!)

nameserver/addrs.go

+func (i Record) String() string {
+	var buf bytes.Buffer
+	if len(i.Name()) > 0 {
+		buf.WriteString(fmt.Sprintf("%s", i.Name()))


…l possible responses from mDNS servers. Support for multiple responses from Lookup*() methods Support for multiple records when building packets with make*() methods Use a common "ZoneRecord" interface for structs that contain names and IPs

awh · 2015-04-23T13:20:48Z

@awh That's right: the presence of an entry locally ends the lookup. We currently do not support multiple responses to clients, so once you have a valid answer to the query, you are done.

At first I thought you meant 'before this PR' by 'currently', but on closer reading I can see the PR widens all the interfaces to take a slice but still works with single values. Can you confirm my understanding that this PR doesn't change the user observed behaviour at all - it's just internal restructuring in support of future changes?

inercia · 2015-04-23T15:02:20Z

@awh Exactly. This PR does not provide an user visible change. It only provides "insistent" queries at the mDNS layer, but they are not used at all in this PR...

awh · 2015-04-24T11:01:37Z

OK in that case LGTM. One final comment - and I really can't stress this enough - separating out different logical changes within your PRs into individual commits makes things much easier on the reviewer, with the consequence that things will get reviewed quicker. Perhaps each of your bullet points could have been teased out into different commits, and there are other things hiding in this PR too like the reformatting of the logging and moving the port binding into the start methods. I don't know how familiar you are with git rebase -i, but I didn't appreciate until recently how powerful it is for restructuring the commit history of a branch after the fact - if you've not used it in anger I recommend taking a look. Also recently learned about git add -p for staging individual hunks in a file, which gives you a way of doing it as you go...

Support for multiple responses in mDNS

inercia · 2015-04-24T11:24:37Z

Thanks @awh

Regarding how to organize the PR, I understand that it can get a bit messy because of the mix of different logical changes. But, in fact I usually do a git rebase -i but for squashing all the changes in my branch. And I also use git add -p. In fact this PR is the result of squashing all the changes in one commit, then resetting and then git add -p'ing these changes... :)

I do that because my branches are in pending state for too long, and that means that I have to rebase master almost everyday (for keeping them mergeable in case the PR reviewer decides it can be merged). This daily rebase is really painful when the PR is more than 1 or 2 commits, but it is trivial when all changes have been squashed into only one commit...

Please let me know of any way this workflow could be improved, but I've found this the only suitable solution for this project due to the long time it takes to have the PRs reviewed...

rade · 2015-04-24T11:43:20Z

please move this meta discussion to email.

inercia reviewed Apr 14, 2015
View reviewed changes

nameserver/addrs.go

/////////////////////////////////////////////////////////////

// simple IPv4 type that can be used as a key in a map (in contrast with net.IP), used for sets of IPs, etc...

This comment was marked as abuse.

Sign in to view

awh self-assigned this Apr 15, 2015

awh reviewed Apr 16, 2015
View reviewed changes

awh assigned inercia and unassigned awh Apr 17, 2015

inercia force-pushed the weave-338 branch from 7fbef10 to c06efe9 Compare April 20, 2015 15:03

inercia force-pushed the weave-338 branch from fc7e6d0 to afd3875 Compare April 22, 2015 07:31

awh reviewed Apr 23, 2015
View reviewed changes

nameserver/addrs.go

func (i Record) String() string {

var buf bytes.Buffer

if len(i.Name()) > 0 {

buf.WriteString(fmt.Sprintf("%s", i.Name()))

This comment was marked as abuse.

Sign in to view

inercia force-pushed the weave-338 branch 3 times, most recently from 90ef2ac to 7603180 Compare April 23, 2015 10:57

inercia added 3 commits April 23, 2015 13:01

Run gofmt

56859a9

Make "go vet" happy

41ae6e0

inercia force-pushed the weave-338 branch from 7603180 to 41ae6e0 Compare April 23, 2015 11:02

awh added a commit that referenced this pull request Apr 24, 2015

Merge pull request #559 from inercia/weave-338

443b116

Support for multiple responses in mDNS

awh merged commit 443b116 into weaveworks:master Apr 24, 2015

rade unassigned inercia Jan 15, 2016

rade added this to the 0.11.0 milestone Jan 15, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for multiple responses in mDNS #559

Support for multiple responses in mDNS #559

inercia commented Apr 14, 2015

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

inercia commented Apr 20, 2015

awh commented Apr 22, 2015

awh commented Apr 22, 2015

inercia commented Apr 23, 2015

This comment was marked as abuse.

awh commented Apr 23, 2015

inercia commented Apr 23, 2015

awh commented Apr 24, 2015

inercia commented Apr 24, 2015

rade commented Apr 24, 2015


		/////////////////////////////////////////////////////////////

		// simple IPv4 type that can be used as a key in a map (in contrast with net.IP), used for sets of IPs, etc...

Support for multiple responses in mDNS #559

Support for multiple responses in mDNS #559

Conversation

inercia commented Apr 14, 2015

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

inercia commented Apr 20, 2015

awh commented Apr 22, 2015

awh commented Apr 22, 2015

inercia commented Apr 23, 2015

This comment was marked as abuse.

awh commented Apr 23, 2015

inercia commented Apr 23, 2015

awh commented Apr 24, 2015

inercia commented Apr 24, 2015

rade commented Apr 24, 2015