Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#2955 closed enhancement (complete)

DHCP-DDNS : Create DHCP-DDNS server class

Reported by: stephen Owned by: tmark
Priority: medium Milestone: Sprint-DHCP-20130606
Component: ~dhcp-ddns(obsolete) Version:
Keywords: 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: 0
Total Hours: 0 Internal?: no

Description

Create the DhcpDdnsSrv class, as described in the DDNS design.

Subtickets

Change History (9)

comment:1 Changed 7 years ago by tmark

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

comment:2 Changed 7 years ago by tmark

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

I've created the initial, bare-bones implementation DHCP-DDNS service process class, D2Process. In the original design this was referred to as DhcpDdnsSrv. Since there
was some disagreement over whether this represented a "server" or not and becaue "DhcpDdns" is
a chore to work with, I've named it D2Process. It plays the same role as designed: It is the top-level object of the "application" providing the event processing and business logic specific to processing DNS name change requests. It is intended to be "controlled" by a managing object (aka D2Controller, being constructed under trac2956) that is responsible for providing D2Process with its runtime environment, either as BIND10 module or stand-alone.

D2Process implements an interface defined in new abstract class DProcess. This abstraction was a natural byproduct of my effort to understand and mimic the DHCP servers controller-server dynamic. It could serve as the basis for refactoring, should we decide to pursue it.


The following new files have been added:

src/bin/d2/d_process.h - defines the DProcess base class
src/bin/d2/d2_process.h - defines the D2Process class
src/bin/d2/d2_process.cc - implements the D2Process class
src/bin/d2/tests/d2_process_unittests.cc - initial unit tests

comment:3 follow-up: Changed 7 years ago by stephen

  • Owner changed from UnAssigned to tmark

Reviewed commit b70a49c18a153f40aaf129d6d3f25039e052e887

src/bin/d2/Makefile.am
Suggest devoting a separate line to d_process.h - it fits in better with the idea of one line per module.

src/bin/d2/d2_messages.mes
Although we said we could use D2 in the source code, I don't think it's a good idea to use it in messages to the user, especially if the process will appear as something line b10-dhcp-ddns. In this case, instead of D2 a prefix for the messages of DHCPDDNS_ would be appropriate. D2PRC is probably OK, although DHCPDDNS_PRC_ would be better.

Message files should not include a ":" between the message identifier and the message text. Neither should the message end with a full-stop. The first token on the line is the identifier, everything else is the rest of the message. The formatting is determined by the logging code.

D2_STARTING: perhaps better phrased as "DHCP DDNS client starting". Also, look at the other BIND 10 servers: although "starting" messages are debug messages, there is usually an informational "started" message.

D2_SHUTDOWN: s/normal shutting down/normal shutdown/ Also note that the text description says "debug" but the message is logged at INFO severity.

D2PRC_SHUTDOWN: s/normal shutting down/normal shutdown/

D2PRC_SHUTDOWN: I think we should expand the description (and the preceding one) to explain the difference between the service and the process. (And it's OK for a debug message to be fairly technical and say something like "This message is issued at a lower-level in the code than the DHCPDDNS_SHUTDOWN message: it indicates the shutdown command has been successfully transmitted to the D2Process object associated with the service.")

D2PRC_FAILED: This message has an argument passed to it, so the message text should include the argument placeholder "%1".

src/bin/d2/d2_process.h
init(): as this is a concrete class, I would think that init actually does something rather than "likely" do something.

src/bin/d2/d_process.h
Having both DProcess and D2Process are a bit confusing. As the former is an abstract class for the latter, D2ProcessBase is perhaps better. However, as this is intended to be the base class for a managed asynchronous application, perhaps a better name is something like AsynchAppManager, AsynchAppManagerBase or something along those lines. If the name is changed, the name of the exception should also be changed to reflect it.

Header typos:
s/asyncrhonous/asynchronous/
s/terms fo management/terms of management/

run(): where are EXIT_SUCCESS and EXIT_FAILURE defined? If only two status codes can be returned, perhaps it is better declared "bool"?

Rather than make the elements protected, a "getXxxx()" method is better: it allows access to the elements, but hides the implementation from all classes (including derived ones).

src/bin/d2/tests/d2_process_unittests.cc
Test normalShutdown and fatalErrorShutdown:

  • spaces needed around multiplication: s/2*1000/2 * 1000/.
  • suggest that the comment for setting up the timer mentions that the argument is expected to be milliseconds. (My first thought was that it wopuld be a long test!)
  • watch out for the test on elapsed.seconds(). Can you guarantee that specifying a 2,000 millisecond wait will always result in a wait equal to or longer than that figure? It would perhaps be better to get the elapsed milliseconds and verify that it is in the range (say) 1,995 to 2,100.

ChangeLog
This change will need a ChangeLong entry, which will need approval.

I think that's it...

... Oh, er, and it's good start, nice work :-)

comment:4 in reply to: ↑ 3 Changed 7 years ago by tmark

  • Owner changed from tmark to stephen

Reviewed commit b70a49c18a153f40aaf129d6d3f25039e052e887

src/bin/d2/Makefile.am
Suggest devoting a separate line to d_process.h - it fits in better with the idea of one line per module.

Done.

src/bin/d2/d2_messages.mes
Although we said we could use D2 in the source code, I don't think it's a good idea to use it in messages to the user, especially if the process will appear as something line b10-dhcp-ddns. In this case, instead of D2 a prefix for the messages of DHCPDDNS_ would be appropriate. D2PRC is probably OK, although DHCPDDNS_PRC_ would be better.

The "D2_" prefix is being replaced in #2956, with D2CTL_. Logs emanating at the controller level would be D2CTL_ and those at the process level would be D2PRC_.

Although not checked in yet, it looks like this at run time:

2013-05-24 13:29:39.346 DEBUG [b10-dchp-ddns/35050] D2CTL_STARTING controller starting, pid: 35050
2013-05-24 13:29:39.347 DEBUG [b10-dhcp-ddns/35050] D2CTL_INIT_PROCESS initializing application proces
2013-05-24 13:29:39.347 DEBUG [b10-dhcp-ddns/35050] D2CTL_STANDALONE skipping message queue, running standalone
2013-05-24 13:29:39.347 DEBUG [b10-dhcp-ddns/35050] D2CTL_RUN_PROCESS starting application proces event loop
2013-05-24 13:29:39.347 DEBUG [b10-dhcp-ddsn/35050] D2PRC_RUN_ENTER process has entered the event loop.

Since the module name is b10-dchp-ddns, having that again in the log is not much benefit. I'd like to stick with the D2CTL_ and D2PRC_
for now.

Message files should not include a ":" between the message identifier and the message text. Neither should the message end with a full-stop. The first token on the line is the identifier, everything else is the rest of the message. The formatting is determined by the logging code.

Not sure why I stuck the colons in there. They've been removed along with the full stops.

D2_STARTING: perhaps better phrased as "DHCP DDNS client starting". Also, look at the other BIND 10 servers: although "starting" messages are debug messages, there is usually an informational "started" message.

Replaced with D2CTL_STARTING.

D2_SHUTDOWN: s/normal shutting down/normal shutdown/ Also note that the text description says "debug" but the message is logged at INFO severity.

Replaced with D2CTL_STOPPING.

D2PRC_SHUTDOWN: s/normal shutting down/normal shutdown/

D2PRC_SHUTDOWN: I think we should expand the description (and the preceding one) to explain the difference between the service and the process. (And it's OK for a debug message to be fairly technical and say something like "This message is issued at a lower-level in the code than the DHCPDDNS_SHUTDOWN message: it indicates the shutdown command has been successfully transmitted to the D2Process object associated with the service.")

Text should be clearer between the two.

D2PRC_FAILED: This message has an argument passed to it, so the message text should include the argument placeholder "%1".

Nice catch.

src/bin/d2/d2_process.h
init(): as this is a concrete class, I would think that init actually does something rather than "likely" do something.

It will eventually do stuff. There is a lot of concrete to pour yet.

src/bin/d2/d_process.h
Having both DProcess and D2Process are a bit confusing. As the former is an abstract class for the latter, D2ProcessBase is perhaps better. However, as this is intended to be the base class for a managed asynchronous application, perhaps a better name is something like AsynchAppManager, AsynchAppManagerBase or something along those lines. If the name is changed, the name of the exception should also be changed to reflect it.

I thought about calling it an something like DApplication etc, but then figured the people would holler about it not being called a "Module"since it will plug into BIND10. I thought about calling it a "DModule" but then figured Tomek would holler about using it outside of BIND10, so in the end a settled on DProcess. I have renamed it DProcessBase to help distinguish it from D2Process.

Header typos:
s/asyncrhonous/asynchronous/
s/terms fo management/terms of management/

run(): where are EXIT_SUCCESS and EXIT_FAILURE defined? If only two status codes can be returned, perhaps it is better declared "bool"?

EXIT_SUCCESS and EXIT_FAILURE are apparently defined in <cstdlib>. I had to look them up myself. They're used in the dhcp4 and 6
servers, so I thought, "Oh, I better use those". In actuality, I am going to alter init, run, and shutdown to be voids and throw
if there is an error, as part of #2956. It is cleaner for the controller to handle them that way.

Rather than make the elements protected, a "getXxxx()" method is better: it allows access to the elements, but hides the implementation from all classes (including derived ones).

Done.

src/bin/d2/tests/d2_process_unittests.cc
Test normalShutdown and fatalErrorShutdown:

  • spaces needed around multiplication: s/2*1000/2 * 1000/.
  • suggest that the comment for setting up the timer mentions that the argument is expected to be milliseconds. (My first thought was that it wopuld be a long test!)
  • watch out for the test on elapsed.seconds(). Can you guarantee that specifying a 2,000 millisecond wait will always result in a wait equal to or longer than that figure? It would perhaps be better to get the elapsed milliseconds and verify that it is in the range (say) 1,995 to 2,100.

No, I don't think I'd gaurantee it. I did think about that before but it worked numerous times so I moved on. However, in the interests of not having intermittent unit test failures, I have taken your suggestion and structured the tests to test within a range.

ChangeLog
This change will need a ChangeLong entry, which will need approval.

How's this?

6xx [func] tmark

Created the initial, bare-bones implementation of DHCP-DDNS service
process class, D2Process, and the abstract class from which it derives,
DProcessBase. D2Process will provide the DHCP-DDNS specific event loop
and business logic.
(Trac #2955, git TBD)

I think that's it...

... Oh, er, and it's good start, nice work :-)

Why thank you, very kindly sir.

comment:5 follow-up: Changed 7 years ago by stephen

  • Owner changed from stephen to tmark

Reviewed commit a9b9f641970b7f2db1b1183fab3deaec454b5b75

src/bin/d2/d2_messages.mes

Since the module name is b10-dchp-ddns, having that again in the log is not much benefit. I'd like to stick with the D2CTL_ and D2PRC_ for now.

The problem here is that the messages are read by the user. "D2xxx" is OK for source code etc. as anyone who gets that far is a developer and will soon get into the way of thinking. But the general user will be puzzled as to what "D2" is.

In part this has been overcome in that the text of the first three messages starts "DHCP-DDNS xxx", so identifying the module. But here, the long string is going into the message text rather than the identification so there is not a lot of gain. (And to be consistent, DHCP-DDNS has to go into the text of all messages.)

Arguably, adding "DHCP-DDNS" to the start of all messages is not needed given that all have an explanation in which the process name can be put. This suggests that the explanation associated with all messages should include the name of the relevant process, "b10-dhcp-ddns". (BTW, one change I missed in the last review was that the explanation of some of the messages talk about "D2": that should be replaced by "b10-dhcp-ddns".) Against this is that the point that the first contact the user will have of the message is the appearance in the log (or message file) of message identification plus text. Ideally this should be descriptive enough for them to guess as to what the message is about, which means that restricting the process name to the message explanation is not enough. So we are back to including "DHCP-DDNS" somewhere in the identification or message text.

Finally, it seems that all these contortions are taking place to avoid a message prefix starting "DHCPDDNS_XXX". This is not overly long: the "stats-httpd" process uses a prefix of STATSHTTPD, so the precedent has been set for a message identification of such a length.

src/bin/d2/d_process.h
shouldShutDown(): Should the "D" be capitalised in "ShutDown"? Elsewhere the word is used without a capital "D" (except in the member variable name which is shut_down_flag).

shouldShutDown(): parentheses needed in the "return" statement.

getName(): should be declared "const" on the grounds that a const reference is returned so the class cannot be changed.

shut_down_flag: see above for a comment about the name. Also, as a member variable, the name should terminate with an underscore.

comment:6 in reply to: ↑ 5 Changed 7 years ago by tmark

  • Owner changed from tmark to stephen

Replying to stephen:

Reviewed commit a9b9f641970b7f2db1b1183fab3deaec454b5b75

src/bin/d2/d2_messages.mes

Since the module name is b10-dchp-ddns, having that again in the log is not much benefit. I'd like to stick with the D2CTL_ and D2PRC_ for now.

The problem here is that the messages are read by the user. "D2xxx" is OK for source code etc. as anyone who gets that far is a developer and will soon get into the way of thinking. But the general user will be puzzled as to what "D2" is.

In part this has been overcome in that the text of the first three messages starts "DHCP-DDNS xxx", so identifying the module. But here, the long string is going into the message text rather than the identification so there is not a lot of gain. (And to be consistent, DHCP-DDNS has to go into the text of all messages.)

Arguably, adding "DHCP-DDNS" to the start of all messages is not needed given that all have an explanation in which the process name can be put. This suggests that the explanation associated with all messages should include the name of the relevant process, "b10-dhcp-ddns". (BTW, one change I missed in the last review was that the explanation of some of the messages talk about "D2": that should be replaced by "b10-dhcp-ddns".) Against this is that the point that the first contact the user will have of the message is the appearance in the log (or message file) of message identification plus text. Ideally this should be descriptive enough for them to guess as to what the message is about, which means that restricting the process name to the message explanation is not enough. So we are back to including "DHCP-DDNS" somewhere in the identification or message text.

Finally, it seems that all these contortions are taking place to avoid a message prefix starting "DHCPDDNS_XXX". This is not overly long: the "stats-httpd" process uses a prefix of STATSHTTPD, so the precedent has been set for a message identification of such a length.

In the interests of review simplicity, a new ticket, #2978, has been created to address this issue.

src/bin/d2/d_process.h
shouldShutDown(): Should the "D" be capitalised in "ShutDown"? Elsewhere the word is used without a capital "D" (except in the member variable name which is shut_down_flag).

shouldShutDown(): parentheses needed in the "return" statement.

getName(): should be declared "const" on the grounds that a const reference is returned so the class cannot be changed.

shut_down_flag: see above for a comment about the name. Also, as a member variable, the name should terminate with an underscore.

Corrected all of the above.

comment:7 Changed 7 years ago by stephen

  • Owner changed from stephen to tmark

Reviewed commit ece829ff0b6b5117ecdfe22439c24b64a8523683

All changes OK, as is the suggested ChangeLog entry.

Please merge.

comment:8 Changed 7 years ago by tmark

  • Resolution set to complete
  • Status changed from reviewing to closed

Changes merged in under commit #dbe4772246039a1257b6492936fda2a8600cd245.
Added ChangeLog? entry 622.

comment:9 Changed 7 years ago by tmark

  • Component changed from dhcp to dhcp-ddns
Note: See TracTickets for help on using tickets.