Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

Replace mDNS peering with gossip #826

Closed
awh opened this issue Jun 2, 2015 · 13 comments
Closed

Replace mDNS peering with gossip #826

awh opened this issue Jun 2, 2015 · 13 comments
Assignees
Milestone

Comments

@awh
Copy link
Contributor

awh commented Jun 2, 2015

WeaveDNS peers currently use mDNS to communicate registrations amongst themselves in response to queries. This has a number of disadvantages:

  • It is not possible to tell when we have received the set of all available answers, leading to the use of a heuristic timeout
  • Reverse lookups are similarly hard to implement, as we must rely on the absence of answers within a defined time period to know when to fallback to external DNS. This introduces unnecessary delay ([dns] constant delay on reverse lookups of non-weave IPs #473), and packet loss can result in false negatives
  • Name updates (least common operation) require no network operations, whereas lookups (most common operation) do

These issues can be mitigated to a degree by adopting additional mDNS features intended to promote performance and scalability, however:

  • Doing so would introduce much additional implementation complexity to address problems which have already been effectively solved by the router gossip protocol
  • The features in question were designed on the assumption that multicast traffic is free, which is not true in weave networks. Extending them further to work around this results in even more complexity

As a consequence, we intend to replace mDNS with the router gossip mechanism so that updates are proactively propagated to all peers, resulting in the following advantages:

  • Each peer operates on the assumption that its local view (maintained by gossip) of the distributed name mapping is authoritative. Forward and reverse lookups are answered immediately from this mapping with no timeouts or network operations; updates are propagated asynchronously so registration is effectively a local operation from the point of view of clients updating the mapping
  • The caching and cache invalidation mechanism can be removed, resulting in a smaller codebase
  • Reuse of the gossip protocol obviates the need for additional implementation work on mDNS; existing mDNS code can be removed

Depends on #741 for access to the router gossip mechanism.

@tomwilkie
Copy link
Contributor

Here is the WIP - https://github.com/weaveworks/weave/commits/gossipdns

Its a fresh implementation, not sharing anything with existing code base. It is embedded in the router, and uses topology 'gossip' to propagate changes.

@squaremo
Copy link
Contributor

squaremo commented Jul 3, 2015

May I suggest, for compatibility with #602, that any option this implementation has in common with its predecessor is

  1. renamed --dns-x and hyphenated if necessary
  2. given a deprecated alias to both -x and --x (where x is the unhyphenated form)

A rough list of candidates:

* `--fallback` (`--dns-fallback`)
* `--maxanswers` (`--dns-max-answers`)
* `--watch` (`--dns-watch`)
* `--ttl` (`--dns-ttl`)
* `--negttl` (`--dns-neg-ttl` or ?)
* `--udpbuf` (`--dns-udb-bufsize`?)

@tomwilkie
Copy link
Contributor

Good suggestion; I expect this DNS implementation to have significantly
fewer knobs.

On Friday, 3 July 2015, Michael Bridgen [email protected] wrote:

May I suggest, for compatibility with #602
#602, that any option this
implementation has in common with its predecessor is

  1. renamed --dns-x and hyphenated if necessary
  2. given a deprecated alias to both -x and --x (where x is the
    unhyphenated form)

A rough list of candidates:

  • --fallback (--dns-fallback)
  • --maxanswers (--dns-max-answers)
  • --watch (--dns-watch)
  • --ttl (--dns-ttl)
  • --negttl (--dns-neg-ttl or ?)
  • --udpbuf (--dns-udb-bufsize?)


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

@tomwilkie
Copy link
Contributor

From email with @rade:

Problem I'm trying to solve: weave script needs to do different things
depending on wether dns is enabled or not. Currently uses the
presence of the dns container to indicate that.

I could:

  • Make dns always run. No option not to have it.
  • Add a /args (or something) handler in the router, to expose the
    arguments the router was run with, and grep it out of that
  • Add it to /status, and grep it out of that.

Whats your preference?


  • Make dns always run. No option not to have it.

That.

@tomwilkie
Copy link
Contributor

Some qq for @rade, @squaremo and @inercia re: dns arguments

  • --fallback (--dns-fallback)

Gossip dns reads this info from /etc/resolv.conf (like the current dns code), but doesn't offer an option to override, so doesn't have this parameter. Do you think I should add that? I can't find an issue referencing why it was added to the existing code.

  • --maxanswers (--dns-max-answers)

Gossip dns returns all answers. Do we want to allow users to limit the number of responses? Whats the usecase?

  • --watch (--dns-watch)

We do this by default, and we use the connection that exists for ipam in the router. I don't think it should be an option, as without it 'garbage collection' of entries won't work. What do you think?

  • --negttl (--dns-neg-ttl or ?)

We don't distinguish negative results from positive results in gossip dns, they all have the same ttl. Is this needed?

@rade
Copy link
Member

rade commented Jul 6, 2015

my $0.02...

--fallback

iirc I took it out and @inercia put it back :) Apparently useful for testing. I'd rather remove it.

--maxanswers

premature optimisation. remove.

--watch

remove.

--negttl

remove.

@tomwilkie
Copy link
Contributor

Great - that a noop for me then!

@rade
Copy link
Member

rade commented Jul 6, 2015

Great - that a noop for me then!

Well, don't just take my word for it.

@rade
Copy link
Member

rade commented Jul 6, 2015

Make dns always run. No option not to have it.

...but please make sure bin/multiweave still works.

@squaremo
Copy link
Contributor

squaremo commented Jul 6, 2015

--fallback (--dns-fallback)

Gossip dns reads this info from /etc/resolv.conf (like the current dns code), but doesn't offer an option to override, so doesn't have this parameter. Do you think I should add that? I can't find an issue referencing why it was added to the existing code.

For testing, I believe; possibly the tests could be rewritten to supply a --dns argument to the weave container. I don't know if it's useful as an option for users.

--maxanswers (--dns-max-answers)

Gossip dns returns all answers. Do we want to allow users to limit the number of responses? Whats the usecase?

I don't know.

--watch (--dns-watch)

We do this by default, and we use the connection that exists for ipam in the router. I don't think it should be an option, as without it 'garbage collection' of entries won't work. What do you think?

I don't think there's a strong motivation for --watch; there may be a motivation for disabling (or not enabling) liveness checks on individual entries.

--negttl (--dns-neg-ttl or ?)

We don't distinguish negative results from positive results in gossip dns, they all have the same ttl. Is this needed?

The TTL for a negative answer is derived differently to a TTL for which you have a record, for the obvious reason; if not able to be derived (which is the case here I think) it should at least be distinct.

@tomwilkie
Copy link
Contributor

@squaremo not sure I follow - for 'upstream' / proxied queries, we just return whatever the upstream sent us, and we do no caching. So we don't modify the ttl here.

For authoritative queries (in weave.local), we consult our in memory database, and return the answer from there (zero length or otherwise). Are you saying the TTL on this response should be different if the answer is of zero length? Why?

Note, unlike the mdns code, there is no reason to make the ttl very short for these in memory queries.

@rade
Copy link
Member

rade commented Jul 6, 2015

Note, unlike the mdns code, there is no reason to make the ttl very short for these in memory queries.

Quite the opposite. Some dns resolvers cache based on the ttl. The new code finally allows us to have short ttls without a massive performance impact.

@tomwilkie
Copy link
Contributor

Yes sorry I meant the exact opposite of what I said. I agree, we should
have a short TTL here.

On Mon, Jul 6, 2015 at 4:05 PM, Matthias Radestock <[email protected]

wrote:

Note, unlike the mdns code, there is no reason to make the ttl very short
for these in memory queries.

Quite the opposite. Some dns resolvers cache based on the ttl. The new
code finally allows us to have short ttls without a massive performance
impact.


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

@rade rade modified the milestones: current, 1.1.0 Jul 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants