Opened 7 years ago

Closed 6 years ago

#3089 closed enhancement (complete)

Integrate NameTransaction management into D2UpdateMgr

Reported by: tmark Owned by: tmark
Priority: medium Milestone: Kea0.8
Component: ~dhcp-ddns(obsolete) Version:
Keywords: Task# 6.5 Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 2
Total Hours: 31 Internal?: no

Description

Task# 6.5 UpdateMgr? should be able to create transactions based upon a NameChangeRequests? and monitor them through completion.

Subtickets

Change History (13)

comment:1 Changed 7 years ago by muks

  • Sub-Project changed from DNS to DHCP

comment:2 Changed 7 years ago by tmark

  • Milestone changed from New Tasks to DHCP Outstanding Tasks

comment:3 Changed 6 years ago by tomek

  • Milestone changed from DHCP Outstanding Tasks to DHCP-Kea1.0-alpha

comment:4 Changed 6 years ago by tmark

  • Owner set to tmark
  • Status changed from new to assigned

comment:5 Changed 6 years ago by tmark

  • Owner changed from tmark to UnAssigned
  • Status changed from assigned to reviewing

Until now, D2UpdateMgr has created instances of NameChangeTransaction when building transactions. This allowed the D2UpdateMgr to be developed and tested ahead of the lower order classes. Now that both NameAddTransaction and NameRemoveTransaction have been implemented, this ticket updates D2UpdateMgr to use them accordingly. This basic change is in D2UpdateMgr::makeTransaction().

Since the transaction classes are fully functional, the unit tests added to D2UpdateMgr with this ticket are relatively rich. In these tests the transactions build real update requests which are actually sent via !DNSClient to a FauxServer which provides real responses back.

During this testing numerous small changes were identified and made:

  • added toText method DnsServerInfo for better logging.
  • added debug level log messages for DNS request send and response received
  • altered D2UpdateMgr::checkFinishedTransactions() to use !isModelDone() to rather than NameChangeRequest::status to identify transaction completeness
  • DNSClient was modified to store a reference to the caller's message pointer (see DNSClient::response_). This circumvents an issue in lib/dns whereby a Message can only be rendered "fromWire" once.
  • NameChangeTransaction::startTransaction, sets the NCR status to ST_PENDING. This had been overlooked.
  • NameChangeTransaction::clearDnsUpdateRequest, now clears update_attempts_. The value was not being reset prior to starting a new request.
  • NameChangeTransaction::DNS_UPDATE_DEFAULT_TIMEOUT has been temporarily shortened until it is made configurable. This is to keep unit test times to reasonable limits. Making this value configurable is addressed in trac# 3268.
  • several new convenience methods, accessors were added for testing purposes

Several new tests were added to d2_update_mgr_unittests.cc:

  • TEST_F(D2UpdateMgrTest, addTransaction)
  • TEST_F(D2UpdateMgrTest, removeTransaction)
  • TEST_F(D2UpdateMgrTest, errorTransaction)
  • TEST_F(D2UpdateMgrTest, multiTransaction)
  • TEST_F(D2UpdateMgrTest, multiTransactionTimeout)

A new class, !TimedIO was added to nc_test_utils, which provides the means to conduct !IOService based tests backed with a "safety" timer.

FauxServer was modified to be able to reschedule it's next receive. This allows
it to endlessly receive and respond, once initially started.

Sadly there are no new classes or state diagrams. The following ChangeLog? is proposed:

7xx.    [func]      [tmark]

    b10-dhcp-ddns D2UpdateMgr now uses the newly implemented 
    NameAddTransaction and NameRemoveTransaction classes.  This allows
    it conduction actual DNS update exchanges based upon queued
    NameChangeRequests.
    (Trac# 3089     git TBD)
Last edited 6 years ago by tmark (previous) (diff)

comment:6 Changed 6 years ago by tmark

  • Add Hours to Ticket changed from 0 to 23
  • Total Hours changed from 0 to 23

comment:7 Changed 6 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:8 Changed 6 years ago by marcin

  • Owner changed from marcin to tmark

Reviewed commit 78eb8a858925785728cf22052c79f09597d96017

General comment: now that we have new year we should update copyright headers to 2014.

src/bin/d2/d2_messages.mes
DHCP_DDNS_UPDATE_REQUEST_SENT: Spurious space between ''server'' and '':''.

Same for the DHCP_DDBS_UPDATE_RESPONSE_RECEIVED.

src/bin/d2/d2_update_mgr.cc
makeTransaction: awkward alignment of the second line here:

        trans.reset(new NameRemoveTransaction(io_service_, next_ncr,
                                           forward_domain, reverse_domain));

I wonder if any debug logger message should precede the transaction start:

trans->startTransaction();

src/bin/d2/dns_client.cc
Declaration of response_:

D2UpdateMessagePtr& response_;

It would be useful to have some text explaining why we have to store the reference to the object. If it circumvents an issue in Message object it would be useful to describe the nature of this problem and. If there is anything we plan to do about this problem, there could be a reference to a ticket.

DNSClient Constructor: if this is a valid condition that the response_placeholder is (or should be) NULL, it should be stated in the constructor description. Ini particular I am not sure that this part remains valid:

 Caller is responsible for allocating this object.

src/bin/d2/nc_trans.h
There is that:

+#if 0
+    /// @todo  This value will be configurable in the near future, but
+    /// until it is there is no way to replace its value.  For now
+    /// we will define it to be relatively short, so unit tests will
+    /// run within reasonable amount of time.
     static const unsigned int DNS_UPDATE_DEFAULT_TIMEOUT = 5 * 1000;
+#else
+    static const unsigned int DNS_UPDATE_DEFAULT_TIMEOUT = 100;
+#endif

Is it a leftover from debugging?

clearDnsUpdateRequest: Spurious dot sign at the end of brief.

src/bin/d2/tests/d2_update_mgr_unittests.cc
D2UpdateMgrWrapper: The brief description: ''Wrapper class for D2UpdateMgr to provide access non-public methods'' is missing ''to'' before ''non-public''.

src/bin/d2/tests/nc_test_utils.cc
Is toHexText used anywhere?

Also, I wonder if you could just make use of the isc::util::encode::encodeHex and decodeHex functions instead of making conversion on your own?

src/bin/d2/tests/nc_test_utils.h
isReceivePending could be documented. in fact the class members of FauxServer could be documented at least.

TimedIO class is not documented.

comment:9 Changed 6 years ago by marcin

  • Add Hours to Ticket changed from 23 to 3
  • Total Hours changed from 23 to 26

comment:10 follow-up: Changed 6 years ago by tmark

  • Owner changed from tmark to marcin
  • Total Hours changed from 26 to 29

Replying to marcin:

Reviewed commit 78eb8a858925785728cf22052c79f09597d96017

General comment: now that we have new year we should update copyright headers to 2014.

Agreed. In my own defense this ticket went into review in 2013. So there!

src/bin/d2/d2_messages.mes
DHCP_DDNS_UPDATE_REQUEST_SENT: Spurious space between ''server'' and '':''.

Same for the DHCP_DDBS_UPDATE_RESPONSE_RECEIVED.

Got it. How did you even notice this?

src/bin/d2/d2_update_mgr.cc
makeTransaction: awkward alignment of the second line here:

        trans.reset(new NameRemoveTransaction(io_service_, next_ncr,
                                           forward_domain, reverse_domain));

Fixed.

I wonder if any debug logger message should precede the transaction start:

trans->startTransaction();

Good idea. I added a log statement inside startTransaction.

src/bin/d2/dns_client.cc
Declaration of response_:

D2UpdateMessagePtr& response_;

It would be useful to have some text explaining why we have to store the reference to the object. If it circumvents an issue in Message object it would be useful to describe the nature of this problem and. If there is anything we plan to do about this problem, there could be a reference to a ticket.

I added commentary in dns_client.h to DNSCLientImpl constructor param.

As to whether we do anything about this or not, I don't know. It is something of a short coming, at
least from an API standpoint. Nothing states that Message::fromWire is a one-shot deal. If you try you
get a segfault or an obscure exception.

DNSClient Constructor: if this is a valid condition that the response_placeholder is (or should be) NULL, it should be stated in the constructor description. Ini particular I am not sure that this part remains valid:

 Caller is responsible for allocating this object.

First, I restated the commentary to be accurate. Secondly, I enforce that it be NULL in the !DNSCLientImpl constructor. This had a minor ripple effect in terms of unit testing but as a whole is cleaner.

src/bin/d2/nc_trans.h
There is that:

+#if 0
+    /// @todo  This value will be configurable in the near future, but
+    /// until it is there is no way to replace its value.  For now
+    /// we will define it to be relatively short, so unit tests will
+    /// run within reasonable amount of time.
     static const unsigned int DNS_UPDATE_DEFAULT_TIMEOUT = 5 * 1000;
+#else
+    static const unsigned int DNS_UPDATE_DEFAULT_TIMEOUT = 100;
+#endif

Is it a leftover from debugging?

No this is not left over from debugging those I suppose the #if 0 is unecessary since the todo is
there. I have removed the #if 0 and revised the todo commentary.

clearDnsUpdateRequest: Spurious dot sign at the end of brief.

Ok, Eagle-Eye. Got it.

src/bin/d2/tests/d2_update_mgr_unittests.cc
D2UpdateMgrWrapper: The brief description: ''Wrapper class for D2UpdateMgr to provide access non-public methods'' is missing ''to'' before ''non-public''.

Ok, got it.

src/bin/d2/tests/nc_test_utils.cc
Is toHexText used anywhere?

Also, I wonder if you could just make use of the isc::util::encode::encodeHex and decodeHex functions instead of making conversion on your own?

No, it is not used in any actual tests but I did use it for debugging some packet issues during testing.
I didn't use decodeHex because my function makes nice readable dumps of 16 bytes per line, not just continous strings of hex digits.

I left it in for future use and don't see the harm of having it, it's in unit test code.

src/bin/d2/tests/nc_test_utils.h
isReceivePending could be documented. in fact the class members of FauxServer could be documented at least.

Boy, I gotta explain everything. lol. Done.

TimedIO class is not documented.

What it's not obvious? Done.

comment:11 in reply to: ↑ 10 Changed 6 years ago by marcin

  • Owner changed from marcin to tmark

Replying to tmark:

Replying to marcin:

Reviewed commit 78eb8a858925785728cf22052c79f09597d96017

General comment: now that we have new year we should update copyright headers to 2014.

Agreed. In my own defense this ticket went into review in 2013. So there!

Yup. What was I thinking?! But now that you made a few changes after review, you should think about updating copyright headers in files modified:

  • d2_update_mgr.cc
  • dns_client.cc
  • dns_client.h
  • nc_trans.cc
  • d2_update_mgr_unittests.cc
  • dns_client_unittests.cc
  • nc_trans_unittests.cc

src/bin/d2/d2_messages.mes
DHCP_DDNS_UPDATE_REQUEST_SENT: Spurious space between ''server'' and '':''.

Same for the DHCP_DDBS_UPDATE_RESPONSE_RECEIVED.

Got it. How did you even notice this?

Hey, reviews are about reading the code. Or I misunderstood something ;)

src/bin/d2/d2_update_mgr.cc
makeTransaction: awkward alignment of the second line here:

        trans.reset(new NameRemoveTransaction(io_service_, next_ncr,
                                           forward_domain, reverse_domain));

Fixed.

ok.

I wonder if any debug logger message should precede the transaction start:

trans->startTransaction();

Good idea. I added a log statement inside startTransaction.

Thank you! Did you want to add any description for this new message too?

src/bin/d2/dns_client.cc
Declaration of response_:

D2UpdateMessagePtr& response_;

It would be useful to have some text explaining why we have to store the reference to the object. If it circumvents an issue in Message object it would be useful to describe the nature of this problem and. If there is anything we plan to do about this problem, there could be a reference to a ticket.

I added commentary in dns_client.h to DNSCLientImpl constructor param.

The description is good! Just a small typo: This allows a single DNSClientImpl instance to be used in for multiple, sequential IOFetch calls.

As to whether we do anything about this or not, I don't know. It is something of a short coming, at
least from an API standpoint. Nothing states that Message::fromWire is a one-shot deal. If you try you
get a segfault or an obscure exception.

Perhaps we should file a ticket which will suggest that the documentation should be updated for Message that the fromWire is a one-shot deal?

DNSClient Constructor: if this is a valid condition that the response_placeholder is (or should be) NULL, it should be stated in the constructor description. Ini particular I am not sure that this part remains valid:

 Caller is responsible for allocating this object.

First, I restated the commentary to be accurate. Secondly, I enforce that it be NULL in the !DNSCLientImpl constructor. This had a minor ripple effect in terms of unit testing but as a whole is cleaner.

Ok.

src/bin/d2/nc_trans.h
There is that:

+#if 0
+    /// @todo  This value will be configurable in the near future, but
+    /// until it is there is no way to replace its value.  For now
+    /// we will define it to be relatively short, so unit tests will
+    /// run within reasonable amount of time.
     static const unsigned int DNS_UPDATE_DEFAULT_TIMEOUT = 5 * 1000;
+#else
+    static const unsigned int DNS_UPDATE_DEFAULT_TIMEOUT = 100;
+#endif

Is it a leftover from debugging?

No this is not left over from debugging those I suppose the #if 0 is unecessary since the todo is
there. I have removed the #if 0 and revised the todo commentary.

Just to be clear. How did you know whether timeout of 100 should be left, not 5 * 1000?

clearDnsUpdateRequest: Spurious dot sign at the end of brief.

Ok, Eagle-Eye. Got it.

You're welcome QA team.

src/bin/d2/tests/d2_update_mgr_unittests.cc
D2UpdateMgrWrapper: The brief description: ''Wrapper class for D2UpdateMgr to provide access non-public methods'' is missing ''to'' before ''non-public''.

Ok, got it.

ok.

src/bin/d2/tests/nc_test_utils.cc
Is toHexText used anywhere?

Also, I wonder if you could just make use of the isc::util::encode::encodeHex and decodeHex functions instead of making conversion on your own?

No, it is not used in any actual tests but I did use it for debugging some packet issues during testing.
I didn't use decodeHex because my function makes nice readable dumps of 16 bytes per line, not just continous strings of hex digits.

I left it in for future use and don't see the harm of having it, it's in unit test code.

Yes, there is no harm. But the comment regarding the purpose if the function is always useful.

src/bin/d2/tests/nc_test_utils.h
isReceivePending could be documented. in fact the class members of FauxServer could be documented at least.

Boy, I gotta explain everything. lol. Done.

Thanks :)

TimedIO class is not documented.

What it's not obvious? Done.

Now everything is obvious. :)

comment:12 Changed 6 years ago by marcin

BTW, I think that suggested changes are trivial so I don't have to see this ticket again.

comment:13 Changed 6 years ago by tmark

  • Add Hours to Ticket changed from 3 to 2
  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 29 to 31

Replying to marcin:

Replying to tmark:

Replying to marcin:

Reviewed commit 78eb8a858925785728cf22052c79f09597d96017

General comment: now that we have new year we should update copyright headers to 2014.

Agreed. In my own defense this ticket went into review in 2013. So there!

Yup. What was I thinking?! But now that you made a few changes after review, you should think about updating copyright headers in files modified:

  • d2_update_mgr.cc
  • dns_client.cc
  • dns_client.h
  • nc_trans.cc
  • d2_update_mgr_unittests.cc
  • dns_client_unittests.cc
  • nc_trans_unittests.cc

I will update them as I edit them in upcoming changes. ;)

src/bin/d2/d2_messages.mes
DHCP_DDNS_UPDATE_REQUEST_SENT: Spurious space between ''server'' and '':''.

Same for the DHCP_DDBS_UPDATE_RESPONSE_RECEIVED.

Got it. How did you even notice this?

Hey, reviews are about reading the code. Or I misunderstood something ;)

src/bin/d2/d2_update_mgr.cc
makeTransaction: awkward alignment of the second line here:

        trans.reset(new NameRemoveTransaction(io_service_, next_ncr,
                                           forward_domain, reverse_domain));

Fixed.

ok.

I wonder if any debug logger message should precede the transaction start:

trans->startTransaction();

Good idea. I added a log statement inside startTransaction.

Thank you! Did you want to add any description for this new message too?

src/bin/d2/dns_client.cc
Declaration of response_:

D2UpdateMessagePtr& response_;

It would be useful to have some text explaining why we have to store the reference to the object. If it circumvents an issue in Message object it would be useful to describe the nature of this problem and. If there is anything we plan to do about this problem, there could be a reference to a ticket.

I added commentary in dns_client.h to DNSCLientImpl constructor param.

The description is good! Just a small typo: This allows a single DNSClientImpl instance to be used in for multiple, sequential IOFetch calls.

Corrected.

As to whether we do anything about this or not, I don't know. It is something of a short coming, at
least from an API standpoint. Nothing states that Message::fromWire is a one-shot deal. If you try you
get a segfault or an obscure exception.

Perhaps we should file a ticket which will suggest that the documentation should be updated for Message that the fromWire is a one-shot deal?o

I have created Trac# 3286 to cover this and updated dns_client.cc commentary accordingly.

DNSClient Constructor: if this is a valid condition that the response_placeholder is (or should be) NULL, it should be stated in the constructor description. Ini particular I am not sure that this part remains valid:

 Caller is responsible for allocating this object.

First, I restated the commentary to be accurate. Secondly, I enforce that it be NULL in the !DNSCLientImpl constructor. This had a minor ripple effect in terms of unit testing but as a whole is cleaner.

Ok.

src/bin/d2/nc_trans.h
There is that:

+#if 0
+    /// @todo  This value will be configurable in the near future, but
+    /// until it is there is no way to replace its value.  For now
+    /// we will define it to be relatively short, so unit tests will
+    /// run within reasonable amount of time.
     static const unsigned int DNS_UPDATE_DEFAULT_TIMEOUT = 5 * 1000;
+#else
+    static const unsigned int DNS_UPDATE_DEFAULT_TIMEOUT = 100;
+#endif

Is it a leftover from debugging?

No this is not left over from debugging those I suppose the #if 0 is unecessary since the todo is
there. I have removed the #if 0 and revised the todo commentary.

Just to be clear. How did you know whether timeout of 100 should be left, not 5 * 1000?

I kept the value (100ms) that is most useful for unit tests. Once it becomes configurable its default value will be re-aligned with something more viable for production use.

clearDnsUpdateRequest: Spurious dot sign at the end of brief.

Ok, Eagle-Eye. Got it.

You're welcome QA team.

src/bin/d2/tests/d2_update_mgr_unittests.cc
D2UpdateMgrWrapper: The brief description: ''Wrapper class for D2UpdateMgr to provide access non-public methods'' is missing ''to'' before ''non-public''.

Ok, got it.

ok.

src/bin/d2/tests/nc_test_utils.cc
Is toHexText used anywhere?

Also, I wonder if you could just make use of the isc::util::encode::encodeHex and decodeHex functions instead of making conversion on your own?

No, it is not used in any actual tests but I did use it for debugging some packet issues during testing.
I didn't use decodeHex because my function makes nice readable dumps of 16 bytes per line, not just continous strings of hex digits.

I left it in for future use and don't see the harm of having it, it's in unit test code.

Yes, there is no harm. But the comment regarding the purpose if the function is always useful.

Added a bit more explanation.

src/bin/d2/tests/nc_test_utils.h
isReceivePending could be documented. in fact the class members of FauxServer could be documented at least.

Boy, I gotta explain everything. lol. Done.

Thanks :)

TimedIO class is not documented.

What it's not obvious? Done.

Now everything is obvious. :)

Merged Changes in with git 9ff948a169e1c1f3ad9e1bad1568375590a3ef42

Added ChangeLog? entry 725 (git 87670dec20b372cb0afca087f1c7181de7f7fe59)

Note: See TracTickets for help on using tickets.