Skip to content
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.

Sign up for GitHub

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

Jump to bottom

BIND backend: Several improvements for b2b-migrate #5810

Merged
merged 4 commits into from
Nov 2, 2017
Merged

BIND backend: Several improvements for b2b-migrate #5810

merged 4 commits into from
Nov 2, 2017

Conversation

pieterlexis
Copy link
Contributor

Short description

While attempting to do a migration from the BIND backend to gsqlite3, several issues arose:

The last issue stemmed from an incomplete implementation in #5115.

This PR aims to fix all these issues.

Note: the b2b-migrate patch for setting the masters correctly and the patch to add the masters to the DomainInfo might need to be backported to 4.0.x.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled and tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@pieterlexis
Add DomainInfo.masters in BindBackend2::getAllDomains()
a222f04 
Closes PowerDNS#5807
@pieterlexis
pdnsutil: Set domain masters to the full list
305cf6a 
Before, only the last master of all the master would eb added becuase of
the way DNSBackend::setMaster() is implemented in gsql backends.
@pieterlexis pieterlexis added this to the auth-4.1.0 milestone Oct 11, 2017
cmouse
cmouse reviewed Oct 11, 2017
View reviewed changes
modules/bindbackend/bindbackend2.cc Outdated
@@ -353,6 +353,7 @@ void Bind2Backend::getAllDomains(vector<DomainInfo> *domains, bool include_disab
di.zone=i->d_name;
di.last_check=i->d_lastcheck;
di.kind=i->d_masters.empty() ? DomainInfo::Master : DomainInfo::Slave; //TODO: what about Native?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i->d_kind instead?

@pieterlexis pieterlexis force-pushed the b2b-migrate-bind-fixes branch from 2f755ce to f2bb2b6 Compare October 11, 2017 10:40
cmouse
cmouse reviewed Oct 11, 2017
View reviewed changes
modules/bindbackend/bindbackend2.cc Outdated
@@ -302,7 +302,8 @@ void Bind2Backend::getUpdatedMasters(vector<DomainInfo> *changedDomains)
ReadLock rl(&s_state_lock);

for(state_t::const_iterator i = s_state.begin(); i != s_state.end() ; ++i) {
if(!i->d_masters.empty() && this->alsoNotify.empty() && i->d_also_notify.empty())
if(i->d_kind != DomainInfo::Slave ||
(!i->d_masters.empty() && this->alsoNotify.empty() && i->d_also_notify.empty()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should these be removed or are Master domains with masters considered slaves?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I'd have to check what the expected behaviour is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the gsqlite3 backend, we SELECT ... where type='MASTER'. This also means that alsoNotify and d_masters don't even matter. We should not use NATIVE or SLAVE zones here to make the BIND backend behave consistent with the gsql backends. I'll make that so.

@pieterlexis
Copy link
Contributor Author

paging @mind04 for a review

@pieterlexis
BIND: save the kind of domain in BB2DomainInfo
bc67ec2 
And use this in all places where this makes sense. This change will use
a size_t number of bytes more per loaded zone.

This change is mostly for `b2b-migrate` to be 1:1.
@pieterlexis pieterlexis force-pushed the b2b-migrate-bind-fixes branch from f2bb2b6 to bc67ec2 Compare October 16, 2017 11:03
@mind04
Copy link
Contributor

mind04 commented Oct 23, 2017

Two questions:

  1. Was the interaction between master and slave checked? (notifies and freshness (soa) checks still working)
  2. Is the behaviour of bind backed still compatible with real bind after this change for master/slave zones and also-notify?

@pieterlexis
Copy link
Contributor Author

Was the interaction between master and slave checked? (notifies and freshness (soa) checks still working)

Yes, this was tested and it works

Is the behaviour of bind backed still compatible with real bind after this change for master/slave zones and also-notify?

The behaviour indeed differs from before (now that I re-read this code), fixing.

@pieterlexis
BIND: re-instate also-notify behaviour
078023c 
@mind04 noticed the behaviour introduced in bc67ec2 was not the same as
before.
ahupowerdns
ahupowerdns approved these changes Nov 2, 2017
View reviewed changes
Copy link
Contributor

@ahupowerdns ahupowerdns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Minor of minorest nits, string master="" is superfluous. But let's not let that stop us.

@aerique aerique merged commit da0b87e into PowerDNS:master Nov 2, 2017
@pieterlexis pieterlexis deleted the b2b-migrate-bind-fixes branch November 3, 2017 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmouse cmouse cmouse left review comments

@ahupowerdns ahupowerdns ahupowerdns approved these changes

Assignees

@pieterlexis pieterlexis

Labels
auth
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@pieterlexis @mind04 @cmouse @ahupowerdns @aerique @rgacogne