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

Add TempFailureCacheTTLAction #6003

Merged
merged 5 commits into from Jan 8, 2018

Conversation

Projects
None yet
2 participants
@zeha
Collaborator

zeha commented Nov 27, 2017

Short description

Adds a request Action to set the packetcache TTL used for temporary failures, (= ServFail + Refused).

RFC. Will add docs, test etc when not everyone is shouting at me for this.

Usage example:

addAction("deduktiva.com.", TempFailureCacheTTLAction(1))
addAction("244.77.in-addr.arpa.", TempFailureCacheTTLAction(86400))

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)
  • checked that this code was merged to master

@zeha zeha requested a review from rgacogne Nov 27, 2017

{
if (responseLen < sizeof(dnsheader))
return;
uint32_t minTTL;
if (rcode == RCode::ServFail || rcode == RCode::Refused) {
minTTL = d_tempFailureTTL;
if (tempFailureTTL != std::numeric_limits<uint32_t>::max()) {

This comment has been minimized.

@rgacogne

rgacogne Nov 28, 2017

Member

It might be cleaner to make tempFailureTTL a boost::optional<uint32_t> instead of relying on a magic value? I know we haven't been consistent in the existing code..

@@ -832,6 +832,10 @@ void moreLua(bool client)
#endif
});
g_lua.writeFunction("TempFailureTTLAction", [](int maxTTL) {

This comment has been minimized.

@rgacogne

rgacogne Nov 28, 2017

Member

It would be more consistent to make maxTTL a uint32_t here.

@@ -337,13 +337,14 @@ void* tcpClientThread(int pipefd)
string poolname;
int delayMsec=0;
uint32_t tempFailureTTL = std::numeric_limits<uint32_t>::max();

This comment has been minimized.

@rgacogne

rgacogne Nov 28, 2017

Member

I have the feeling it would be better to make tempFailureTTL a member of DNSQuestion, it would make it easier to expose it to Lua later, if we want to. We should probably do the same thing for delayMsec and poolname, but that's another story.

@@ -1009,6 +1009,9 @@ bool processQuery(LocalHolders& holders, DNSQuestion& dq, string& poolname, int*
case DNSAction::Action::Delay:
*delayMsec = static_cast<int>(pdns_stou(ruleresult)); // sorry
break;
case DNSAction::Action::SetTempFailureTTL:

This comment has been minimized.

@rgacogne

rgacogne Nov 28, 2017

Member

If we make tempFailureTTL a member of DNSQuestion we don't need to add a new DNSAction::Action value, TempFailureTTLAction can just set the value the way SkipCacheAction does.

@@ -418,6 +419,7 @@ struct IDState
uint16_t origID; // 2
uint16_t origFlags; // 2
int delayMsec;
uint32_t tempFailureTTL;

This comment has been minimized.

@rgacogne

rgacogne Nov 28, 2017

Member

It would be slightly better to move this field with the other uint32_t one, for alignment purpose.

{}
TempFailureTTLAction::Action operator()(DNSQuestion* dq, string* ruleresult) const override
{
cerr << "going through TempFailureTTLAction " << endl;

This comment has been minimized.

@rgacogne

rgacogne Nov 28, 2017

Member

The log message will have to go, of course :)

@@ -1301,6 +1301,25 @@ private:
std::string d_reason;
};
class TempFailureTTLAction : public DNSAction

This comment has been minimized.

@rgacogne

rgacogne Nov 28, 2017

Member

I would like the name of the action to reflect that it only applies to the cache, but something like TempFailureCacheTTLAction is quite a mouthful..

@zeha

This comment has been minimized.

Collaborator

zeha commented Dec 10, 2017

Addressed @rgacogne notes.

I'd think dq.tempFailureTTL should also be exposed to Lua, but now that it is a boost::optional<uint32_t> I'm not so sure how...

@rgacogne

This comment has been minimized.

Member

rgacogne commented Dec 11, 2017

I'd think dq.tempFailureTTL should also be exposed to Lua, but now that it is a boost::optional<uint32_t> I'm not so sure how...

You can easily write custom member handlers using registerMember, making it easy to set and get the actual value. The not-so-easy part is how to convey the fact that the value is currently not set to Lua. Perhaps you could write a custom isTempFailureTTLSet() method?

@zeha

This comment has been minimized.

Collaborator

zeha commented Dec 11, 2017

Perhaps you could write a custom isTempFailureTTLSet() method?

Yeah... for symmetry maybe that also needs an unsetTempFailure() method?

This is probably not the last boost::optional that will be exposed... can we expose an Optional interface to Lua? (Happy to chat about this on IRC, haven't fully thought out this yet.)

@rgacogne

This comment has been minimized.

Member

rgacogne commented Dec 18, 2017

This PR will need to be rebased following the merge of #5947, sorry about that!

@zeha zeha force-pushed the zeha:dnsdist-packetcache-ttlcap branch 2 times, most recently from 5d0299c to 391b961 Dec 18, 2017

@zeha zeha changed the title from RFC: Add TempFailureTTLAction to RFC: Add TempFailureCacheTTLAction Dec 18, 2017

@zeha zeha force-pushed the zeha:dnsdist-packetcache-ttlcap branch from 391b961 to acb8f5d Dec 18, 2017

@zeha zeha changed the title from RFC: Add TempFailureCacheTTLAction to WIP: Add TempFailureCacheTTLAction Dec 18, 2017

@zeha zeha changed the title from WIP: Add TempFailureCacheTTLAction to Add TempFailureCacheTTLAction Jan 6, 2018

@zeha

This comment has been minimized.

Collaborator

zeha commented Jan 6, 2018

Tests, Docs, Lua binding added. Turns out LuaWrapper happily exposes boost::optional!

@rgacogne

Looks good, a few nits

found = PC.get(dq, a.wirelength(), pwR.getHeader()->id, responseBuf, &responseBufSize, &key, 0, true);
BOOST_CHECK_EQUAL(found, false);
// Insert with failure-TTL non-zero (-> should not enter cache).

This comment has been minimized.

@rgacogne

rgacogne Jan 8, 2018

Member

s/should not enter cache/should enter cache/ ?

self.assertEquals(receivedResponse, response)
# next query should hit the cache
(receivedQuery, receivedResponse) = self.sendUDPQuery(query, response)

This comment has been minimized.

@rgacogne

rgacogne Jan 8, 2018

Member

If you expect the query to hit the cache, I don't think you should provide a response. Otherwise it will be added to the queue and this might prove an issue later because you won't get the expected answer for the next test. It's not an issue there because you pass the exact same answer to the next test, but you might want to use:

self.sendUDPQuery(query, response=None, useQueue=False)
@zeha

This comment has been minimized.

Collaborator

zeha commented Jan 8, 2018

Nits addressed, thanks!

@rgacogne rgacogne merged commit a674c79 into PowerDNS:master Jan 8, 2018

1 check passed

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

@zeha zeha deleted the zeha:dnsdist-packetcache-ttlcap branch Jan 29, 2018

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