fixes PowerDNS/pdns/657

I noticed this as a result of the changes in 3.2 whereby each receiver gets its own distributor - whenever the dynhandler calls ->getBackend() it will only find the one in its' current thread, so reload won't propergate to backends within other threads.

However, I suspect this condition has always been around as a race - if I have multiple distributors and I call reload; one thread is issuing reloads while all the other backend threads are currently executing (potentially). I guess if you call reload in previous versions on a highly loaded server with multiple backends you could probably get it to crash.

The attached patch changes it so that each backend has a variable which is changed to signal that a reload or rediscover has been requested. When processing gets to an appropriate point ie before it next queries the backend database, such an operation will be run. Note that it is not run in ::get as we don't want a backend to be reloaded between performing a lookup and getting the records.

This patch means that reload/reinitialize are not done straight away as in previous versions, but on a moderately loaded server this should not be a problem as they will be performed the next time a backend is called.

diff --git a/pdns/dynhandler.cc b/pdns/dynhandler.cc
index 133d771d6bc736d58d2f0015bf2e11f9772eb5e5..663a47f8fc35b833a88d570bf41eb742f9037ec0 100644 (file)
--- a/pdns/dynhandler.cc
+++ b/pdns/dynhandler.cc
@@ -279,23 +279,13 @@ string DLNotifyHandler(const vector<string>&parts, Utility::pid_t ppid)
 
 string DLRediscoverHandler(const vector<string>&parts, Utility::pid_t ppid)
 {
-  PacketHandler P;
-  try {
-    L<<Logger::Error<<"Rediscovery was requested"<<endl;
-    string status="Ok";
-    P.getBackend()->rediscover(&status);
-    return status;
-  }
-  catch(PDNSException &ae) {
-    return ae.reason;
-  }
-
+  UeberBackend::rediscover_all();
+  return "Ok";
 }
 
 string DLReloadHandler(const vector<string>&parts, Utility::pid_t ppid)
 {
-  PacketHandler P;
-  P.getBackend()->reload();
+  UeberBackend::reload_all();
   L<<Logger::Error<<"Reload was requested"<<endl;
   return "Ok";
 }

diff --git a/pdns/ueberbackend.cc b/pdns/ueberbackend.cc
index fb27c53dcc1edd713f9ef0e330f1738209096792..ebb30a63746d0f2631b122f60c2f8b66ef983220 100644 (file)
--- a/pdns/ueberbackend.cc
+++ b/pdns/ueberbackend.cc
@@ -100,6 +100,7 @@ void UeberBackend::go(void)
 
 bool UeberBackend::getDomainInfo(const string &domain, DomainInfo &di)
 {
+  check_op_requests();
   for(vector<DNSBackend *>::const_iterator i=backends.begin();i!=backends.end();++i)
     if((*i)->getDomainInfo(domain, di))
       return true;
@@ -119,6 +120,7 @@ bool UeberBackend::createDomain(const string &domain)
 int UeberBackend::addDomainKey(const string& name, const KeyData& key)
 {
   int ret;
+  check_op_requests();
   BOOST_FOREACH(DNSBackend* db, backends) {
     if((ret = db->addDomainKey(name, key)) >= 0)
       return ret;
@@ -127,6 +129,7 @@ int UeberBackend::addDomainKey(const string& name, const KeyData& key)
 }
 bool UeberBackend::getDomainKeys(const string& name, unsigned int kind, std::vector<KeyData>& keys)
 {
+  check_op_requests();
   BOOST_FOREACH(DNSBackend* db, backends) {
     if(db->getDomainKeys(name, kind, keys))
       return true;
@@ -136,6 +139,7 @@ bool UeberBackend::getDomainKeys(const string& name, unsigned int kind, std::vec
 
 bool UeberBackend::getDomainMetadata(const string& name, const std::string& kind, std::vector<std::string>& meta)
 {
+  check_op_requests();
   BOOST_FOREACH(DNSBackend* db, backends) {
     if(db->getDomainMetadata(name, kind, meta))
       return true;
@@ -145,6 +149,7 @@ bool UeberBackend::getDomainMetadata(const string& name, const std::string& kind
 
 bool UeberBackend::setDomainMetadata(const string& name, const std::string& kind, const std::vector<std::string>& meta)
 {
+  check_op_requests();
   BOOST_FOREACH(DNSBackend* db, backends) {
     if(db->setDomainMetadata(name, kind, meta))
       return true;
@@ -154,6 +159,7 @@ bool UeberBackend::setDomainMetadata(const string& name, const std::string& kind
 
 bool UeberBackend::activateDomainKey(const string& name, unsigned int id)
 {
+  check_op_requests();
   BOOST_FOREACH(DNSBackend* db, backends) {
     if(db->activateDomainKey(name, id))
       return true;
@@ -163,6 +169,7 @@ bool UeberBackend::activateDomainKey(const string& name, unsigned int id)
 
 bool UeberBackend::deactivateDomainKey(const string& name, unsigned int id)
 {
+  check_op_requests();
   BOOST_FOREACH(DNSBackend* db, backends) {
     if(db->deactivateDomainKey(name, id))
       return true;
@@ -172,6 +179,7 @@ bool UeberBackend::deactivateDomainKey(const string& name, unsigned int id)
 
 bool UeberBackend::removeDomainKey(const string& name, unsigned int id)
 {
+  check_op_requests();
   BOOST_FOREACH(DNSBackend* db, backends) {
     if(db->removeDomainKey(name, id))
       return true;
@@ -182,6 +190,7 @@ bool UeberBackend::removeDomainKey(const string& name, unsigned int id)
 
 bool UeberBackend::getTSIGKey(const string& name, string* algorithm, string* content)
 {
+  check_op_requests();
   BOOST_FOREACH(DNSBackend* db, backends) {
     if(db->getTSIGKey(name, algorithm, content))
       return true;
@@ -216,7 +225,21 @@ bool UeberBackend::getTSIGKeys(std::vector< struct TSIGKey > &keys)
   return true;
 }
 
+/* Called from anywhere to signal a reload of all backend databases */
+void UeberBackend::reload_all()
+{
+  BOOST_FOREACH(UeberBackend* b, instances) {
+    b->cur_op_request = RELOAD;
+  }
+}
+void UeberBackend::rediscover_all()
+{
+  BOOST_FOREACH(UeberBackend* b, instances) {
+    b->cur_op_request = REDISCOVER;
+  }
+}
 
+/* OPS */
 void UeberBackend::reload()
 {
   for ( vector< DNSBackend * >::iterator i = backends.begin(); i != backends.end(); ++i )
@@ -232,14 +255,13 @@ void UeberBackend::rediscover(string *status)
   {
     string tmpstr;
     ( *i )->rediscover(&tmpstr);
-    if(status) 
-      *status+=tmpstr + (i!=backends.begin() ? "\n" : "");
   }
 }
 
 
 void UeberBackend::getUnfreshSlaveInfos(vector<DomainInfo>* domains)
 {
+  check_op_requests();
   for ( vector< DNSBackend * >::iterator i = backends.begin(); i != backends.end(); ++i )
   {
     ( *i )->getUnfreshSlaveInfos( domains );
@@ -250,12 +272,34 @@ void UeberBackend::getUnfreshSlaveInfos(vector<DomainInfo>* domains)
 
 void UeberBackend::getUpdatedMasters(vector<DomainInfo>* domains)
 {
+  check_op_requests();
   for ( vector< DNSBackend * >::iterator i = backends.begin(); i != backends.end(); ++i )
   {
     ( *i )->getUpdatedMasters( domains );
   }
 }
 
+void UeberBackend::check_op_requests()
+{
+    if( !cur_op_request )
+        return;
+
+    switch(cur_op_request) {
+
+        case RELOAD:
+            reload();
+            break;
+
+        case REDISCOVER:
+            rediscover();
+            break;
+
+        default:
+            break;
+    }
+    cur_op_request = NONE;
+}
+
 /** special trick - if sd.db is set to -1, the cache is ignored */
 bool UeberBackend::getSOA(const string &domain, SOAData &sd, DNSPacket *p)
 {
@@ -277,6 +321,7 @@ bool UeberBackend::getSOA(const string &domain, SOAData &sd, DNSPacket *p)
     }
   }
     
+  check_op_requests();
   for(vector<DNSBackend *>::const_iterator i=backends.begin();i!=backends.end();++i)
     if((*i)->getSOA(domain, sd, p)) {
       DNSResourceRecord rr;
@@ -297,6 +342,7 @@ bool UeberBackend::getSOA(const string &domain, SOAData &sd, DNSPacket *p)
 
 bool UeberBackend::superMasterBackend(const string &ip, const string &domain, const vector<DNSResourceRecord>&nsset, string *account, DNSBackend **db)
 {
+  check_op_requests();
   for(vector<DNSBackend *>::const_iterator i=backends.begin();i!=backends.end();++i)
     if((*i)->superMasterBackend(ip,domain,nsset,account, db))
       return true;
@@ -332,6 +378,12 @@ void del(DNSBackend* d)
   delete d;
 }
 
+void cleanup_backends(UeberBackend *b)
+{
+  for_each(b->backends.begin(),b->backends.end(),del);
+  b->backends.clear();
+}
+
 void UeberBackend::cleanup()
 {
   pthread_mutex_lock(&instances_lock);
@@ -341,7 +393,7 @@ void UeberBackend::cleanup()
 
   pthread_mutex_unlock(&instances_lock);
 
-  for_each(backends.begin(),backends.end(),del);
+  cleanup_backends(this);
 }
 
 // silly Solaris fix
@@ -418,6 +470,7 @@ void UeberBackend::addCache(const Question &q, const vector<DNSResourceRecord> &
 
 void UeberBackend::alsoNotifies(const string &domain, set<string> *ips)
 {
+  check_op_requests();
   for ( vector< DNSBackend * >::iterator i = backends.begin(); i != backends.end(); ++i )
     (*i)->alsoNotifies(domain,ips);
 }
@@ -460,32 +513,34 @@ void UeberBackend::lookup(const QType &qtype,const string &qname, DNSPacket *pkt
     stale=true; // please recycle us! 
     throw PDNSException("We are stale, please recycle");
   }
+
+  check_op_requests();
+
+  d_question.qtype=qtype;
+  d_question.qname=qname;
+  d_question.zoneId=zoneId;
+  int cstat=cacheHas(d_question, d_answers);
+  if(cstat<0) { // nothing
+    d_negcached=d_cached=false;
+    d_answers.clear(); 
+    (d_handle.d_hinterBackend=backends[d_handle.i++])->lookup(qtype, qname,pkt_p,zoneId);
+  } 
+  else if(cstat==0) {
+    d_negcached=true;
+    d_cached=false;
+    d_answers.clear();
+  }
   else {
-    d_question.qtype=qtype;
-    d_question.qname=qname;
-    d_question.zoneId=zoneId;
-    int cstat=cacheHas(d_question, d_answers);
-    if(cstat<0) { // nothing
-      d_negcached=d_cached=false;
-      d_answers.clear(); 
-      (d_handle.d_hinterBackend=backends[d_handle.i++])->lookup(qtype, qname,pkt_p,zoneId);
-    } 
-    else if(cstat==0) {
-      d_negcached=true;
-      d_cached=false;
-      d_answers.clear();
-    }
-    else {
-      d_negcached=false;
-      d_cached=true;
-      d_cachehandleiter = d_answers.begin();
-    }
+    d_negcached=false;
+    d_cached=true;
+    d_cachehandleiter = d_answers.begin();
   }
 
   d_handle.parent=this;
 }
 
 void UeberBackend::getAllDomains(vector<DomainInfo> *domains) {
+  check_op_requests();
   for (vector<DNSBackend*>::iterator i = backends.begin(); i != backends.end(); ++i )
   {
     (*i)->getAllDomains(domains);
@@ -505,6 +560,8 @@ bool UeberBackend::get(DNSResourceRecord &rr)
     }
     return false;
   }
+
+  /* Don't check_op_requests - don't reload a backend in the middle of a zone fetch operation */
   if(!d_handle.get(rr)) {
     if(!d_ancount && !d_handle.qname.empty()) // don't cache axfr
       addNegCache(d_question);

diff --git a/pdns/ueberbackend.hh b/pdns/ueberbackend.hh
index 3c29cff0b77c4533e4b0721c6d18d7ed04b6b5f4..32a93d48462e309cd106b9b885e532ec2509182c 100644 (file)
--- a/pdns/ueberbackend.hh
+++ b/pdns/ueberbackend.hh
@@ -78,6 +78,8 @@ public:
       instructions to load new modules */
   static void *DynListener(void *);
   static void go(void);
+  static void reload_all();
+  static void rediscover_all();
 
   /** This contains all registered backends. The DynListener modifies this list for us when
       new modules are loaded */
@@ -168,6 +170,15 @@ private:
   static bool d_go;
   static int s_s;
   static string s_status; 
+
+  // Operational requests for the backends
+  enum backend_op_requests {
+    NONE = 0,
+    RELOAD,
+    REDISCOVER
+  } cur_op_request;
+  void check_op_requests();
+
   int d_ancount;
   
   bool stale;