New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deresource & uphash: gigabit DNSSEC root-server performance #4467

Merged
merged 30 commits into from Sep 14, 2016

Conversation

Projects
None yet
1 participant
@ahupowerdns
Member

ahupowerdns commented Sep 13, 2016

This commit does a number of unrelated things that are difficult to untangle. We remove the ASCII DNSResourceRecord from the hot path of packet assembly in auth. It is still around for looking up various other things, and these can go later.
In addition, we hash the storage of records in the bindbackend. We also hash the packetcache.
This PR comes with a few additional unit tests, and incidentally fixes some bugs in the LDAP backend and in the MyDNS backend.
Finally as an example, this PR makes the randombackend go 'native' and directly supply records that can be sent to packets. With no ASCII.
The performance benefit of this PR is measured in "factors" for being a root-server.

ahupowerdns added some commits Sep 3, 2016

reenable LDAP testing, plus fix up partial SOA records from backends.…
… Hopefully this was the reason for the failing LDAP backend tests.
make it possible to compare pointers/references to DNSRecordContent. …
…There is a generic comparison function based on getZoneRepresentation(), and specializations for A, AAAA, MX and NS. Rest to follow.

Also make DNSPacket::addRecord() use this infrastructure
@ahupowerdns

This comment has been minimized.

Show comment
Hide comment
@ahupowerdns

ahupowerdns Sep 13, 2016

Member

Here you will find a review of all the changes in this PR.

In short the goal was to replace the DNSResourceRecord, which contains a human readable string in its content field ("1.2.3.4"), by DNSZoneRecord. The DNSZoneRecord contains the same kind of information as the DNSResourceRecord, except mostly in binary, parsed form, that can be copied straight into a response packet.

The DNSZoneRecord in turn consists of an auth field, domain_id, ECS scopeMask, some DNSSEC metadata, plus a DNSRecord. The DNSRecord has things like name, type, class, ttl, place and actual content.

To enable this move to DNSZoneRecord, which saves a TON of ascii-roundtripping, a lot had to happen. Some things are not strictly related to this change but do conflict with it so went in on the same go:

  • Bindbackend: add a hashed index
  • Removal of labelReverse function which had useful bot confusing semantics. This function involved a full roundtrip from DNSName to ASCII.
  • Removal and replacement of ascii-based dotConcat and makeRelative (!!)
  • packetcache: add a hashed index, make the query cache store DNSZoneRecords
  • ueberbackend & dnsbackend: add a get(DNSZoneRecord&) method which by default shims get(DNSResourceRecord). This also deals with SOA default record fields.
  • DNSRecordContent: get an actual fast operator==. DNSResourceRecord compare was 'toLower(content)'. Only NS, A, AAAA and MX have a fast operator== right now.
  • Unit tests: operator== tests
  • MyDNSbackend turned out to return a broken SOA record on ANY queries, which tripped up our new code
    • LDAPBackend was broken in several places, and the tests required this. Has been fixed and tests now succeed.
  • Randombackend: has gone DNSZoneRecord native, for super random speed
  • Interim code for converting SOAData from/to ascii or SOARecordContent. Eventually this should all go native
  • DNSPacket additional processing selection code made needless copies of everything
  • an ENTRecordContent() so we can pass ENTs around as MOADNS objects
  • The actual manual labour of adjusting the code to DNSZoneRecord

Things that might cause problems:

  • How we deal with bad data from the database is now way different. We discover the problem way earlier. This may change our robustness for good or bad
  • Everything we test passes. But there as a lot that compiled and did not pass earlier. specifically to do with ENTs and NSEC3 opt-in/opt-out. We might have missed cases.
  • The UeberBackend stored a cache miss twice. Once as addNegCache() and once as addCache(). I have removed the second addCache() "and nothing appeared to change".
  • How we deal with MX/SRV/CNAME to the root has changed. It used to be that we added a . sometimes. We now no longer do this, but zone2ldap and zone2sql do the right thing and put a . in there to encode the root. I think we actually did not do this right previously but you never saw the problem, and now we throw on "MX 12".

Reasonable followup PRs:

  • operator== for further record types
  • DNSZoneRecord native BINDbackend, geoipbackend
  • DNSPacket to lose getString() and just hand out the vector we already made
  • getSOA() now prefers the 'old' get(DNSRecordContent) which is super slow, getSOAUncached() does not benefit from the pre-translated DNSZoneContent in the query cache
  • AAAARecordContent uses a std::string internally (!!)
  • Further removal of use of get(DNSResourceRecord&) - not a must on the slow path
Member

ahupowerdns commented Sep 13, 2016

Here you will find a review of all the changes in this PR.

In short the goal was to replace the DNSResourceRecord, which contains a human readable string in its content field ("1.2.3.4"), by DNSZoneRecord. The DNSZoneRecord contains the same kind of information as the DNSResourceRecord, except mostly in binary, parsed form, that can be copied straight into a response packet.

The DNSZoneRecord in turn consists of an auth field, domain_id, ECS scopeMask, some DNSSEC metadata, plus a DNSRecord. The DNSRecord has things like name, type, class, ttl, place and actual content.

To enable this move to DNSZoneRecord, which saves a TON of ascii-roundtripping, a lot had to happen. Some things are not strictly related to this change but do conflict with it so went in on the same go:

  • Bindbackend: add a hashed index
  • Removal of labelReverse function which had useful bot confusing semantics. This function involved a full roundtrip from DNSName to ASCII.
  • Removal and replacement of ascii-based dotConcat and makeRelative (!!)
  • packetcache: add a hashed index, make the query cache store DNSZoneRecords
  • ueberbackend & dnsbackend: add a get(DNSZoneRecord&) method which by default shims get(DNSResourceRecord). This also deals with SOA default record fields.
  • DNSRecordContent: get an actual fast operator==. DNSResourceRecord compare was 'toLower(content)'. Only NS, A, AAAA and MX have a fast operator== right now.
  • Unit tests: operator== tests
  • MyDNSbackend turned out to return a broken SOA record on ANY queries, which tripped up our new code
    • LDAPBackend was broken in several places, and the tests required this. Has been fixed and tests now succeed.
  • Randombackend: has gone DNSZoneRecord native, for super random speed
  • Interim code for converting SOAData from/to ascii or SOARecordContent. Eventually this should all go native
  • DNSPacket additional processing selection code made needless copies of everything
  • an ENTRecordContent() so we can pass ENTs around as MOADNS objects
  • The actual manual labour of adjusting the code to DNSZoneRecord

Things that might cause problems:

  • How we deal with bad data from the database is now way different. We discover the problem way earlier. This may change our robustness for good or bad
  • Everything we test passes. But there as a lot that compiled and did not pass earlier. specifically to do with ENTs and NSEC3 opt-in/opt-out. We might have missed cases.
  • The UeberBackend stored a cache miss twice. Once as addNegCache() and once as addCache(). I have removed the second addCache() "and nothing appeared to change".
  • How we deal with MX/SRV/CNAME to the root has changed. It used to be that we added a . sometimes. We now no longer do this, but zone2ldap and zone2sql do the right thing and put a . in there to encode the root. I think we actually did not do this right previously but you never saw the problem, and now we throw on "MX 12".

Reasonable followup PRs:

  • operator== for further record types
  • DNSZoneRecord native BINDbackend, geoipbackend
  • DNSPacket to lose getString() and just hand out the vector we already made
  • getSOA() now prefers the 'old' get(DNSRecordContent) which is super slow, getSOAUncached() does not benefit from the pre-translated DNSZoneContent in the query cache
  • AAAARecordContent uses a std::string internally (!!)
  • Further removal of use of get(DNSResourceRecord&) - not a must on the slow path
@ahupowerdns

This comment has been minimized.

Show comment
Hide comment
@ahupowerdns

ahupowerdns Sep 14, 2016

Member

All measured on a gigabit/s network, which gets saturated by this branch. 4.0.1 was tweaked for maximum performance, 'deresource' mostly left at default values. 4 core Intel(R) Core(TM) i7-4770K CPU @ 3.50GHz.

Before:
image

After:
image

Plotted using calidns data:
image

And the bandwidth plot:
image
Showing bandwidth saturation on the gigabit/s network.

Member

ahupowerdns commented Sep 14, 2016

All measured on a gigabit/s network, which gets saturated by this branch. 4.0.1 was tweaked for maximum performance, 'deresource' mostly left at default values. 4 core Intel(R) Core(TM) i7-4770K CPU @ 3.50GHz.

Before:
image

After:
image

Plotted using calidns data:
image

And the bandwidth plot:
image
Showing bandwidth saturation on the gigabit/s network.

@ahupowerdns ahupowerdns merged commit b7eeb50 into PowerDNS:master Sep 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ahupowerdns ahupowerdns deleted the ahupowerdns:deresource branch Sep 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment