Opened 8 years ago

Closed 8 years ago

#1708 closed enhancement (fixed)

Integrate DHCP6 process startup into BIND 10

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Sprint-DHCP-20120903
Component: dhcp6 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

Currently the processes have to be started from the command line. This change makes them part of BIND 10, able to be started and stopped from bindctl.

This ticket is for v6 only. See #1651 for companion ticket for v4.

Subtickets

Change History (11)

comment:1 Changed 8 years ago by stephen

  • Milestone changed from Sprint-DHCP-20120514 to DHCP 2012

comment:2 Changed 8 years ago by stephen

  • Milestone changed from DHCP 2012 to DHCP-Sprint-20120611

comment:3 Changed 8 years ago by tomek

  • Owner set to tomek
  • Status changed from new to accepted

comment:4 Changed 8 years ago by tomek

  • Owner changed from tomek to UnAssigned
  • Status changed from accepted to reviewing

This ticket also fixes up several things in dhcp4 support for msgq.

Also implemented '-s' parameter (stand-alone). Both b10-dhcp{4,6} modules now connect to msgq, but such connection can be disabled. I tried to make this as non-intrusive and user friendly as possible.

Ready for review.

comment:5 Changed 8 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:6 follow-up: Changed 8 years ago by stephen

  • Owner changed from stephen to tomek

Reviewed commit f788c425a50d504d7d23d3945b67f2ad25513495

src/bin/dhcp4/main.cc
main(): Why does the ControlledDhcp4Srv have to be created via "new"? It can be created as an automatic object, which removes the need to delete the server when run() returns.

Suggest boost::lexical_cast be used to interpret the port number - the current use of strtol with a null value for endptr will not catch all invalid values.

src/bin/dhcp6/dhcp6_srv.cc
Constructor: IFaceMgr::instance() is called and if there is a failure, the shutdown_ variable is set and an error is logged. Why is the condition then ignored (the code immediately goes on to call countIfaces() on the created instance). And if there are no interfaces, another error is recorded, shutdown_ is set, yet the error is ignored and the code attempts to open sockets. Why are the errors ignored?

src/bin/dhcp6/ctrl_dhcp6_srv.cc
Note: This code is very similar to that in src/bin/dhcp6/ctrl_dhcp4_srv.cc (from ticket #1651).

src/bin/dhcp6/main.cc
main(): Why does the software start up a ControlledDhcpv6Srv object then, after run() returns and the server is deleted, start up a Dhcp6Srv object?

main(): Why does the ControlledDhcp4Srv have to be created via "new"? It can be created as an automatic object, which removes the need to delete the server when run() returns.

Suggest boost::lexical_cast be used to interpret the port number - the current use of strtol with a null value for endptr will not catch all invalid values.

dhcp6 and dhcp4 main() are practically identical. As they depend on a type (the type of the server), they could be combined into a single templated function.

Overall
I suggest we open a new ticket to refactor the common control code in dhcp4 and dhcp6 into a single set of functions/classes (and tests).

comment:7 Changed 8 years ago by tomek

Added an extra feature that makes msgq connection optional. It is necessary to make tests running much simpler (no bind10 framework running is needed). Tests for that new feature added. Although I wrote the code some time ago, I just realized that I haven't pushed it before.

comment:8 in reply to: ↑ 6 Changed 8 years ago by tomek

Replying to stephen:

Reviewed commit f788c425a50d504d7d23d3945b67f2ad25513495

src/bin/dhcp4/main.cc
main(): Why does the ControlledDhcp4Srv have to be created via "new"? It can be created as an automatic object, which removes the need to delete the server when run() returns.

new/delete gives you better control over when object is deleted. But there's nothing this flexibility could be useful for in this particular context, so changed to automatic object. I did the same for dhcpv6 object.

Suggest boost::lexical_cast be used to interpret the port number - the current use of strtol with a null value for endptr will not catch all invalid values.

I have never used lexical_casts before. Nice!

Also added 2 extra tests for each component checking if this lexical_cast really works. It seems it does :)

src/bin/dhcp6/dhcp6_srv.cc
Constructor: IFaceMgr::instance() is called and if there is a failure, the shutdown_ variable is set and an error is logged. Why is the condition then ignored (the code immediately goes on to call countIfaces() on the created instance). And if there are no interfaces, another error is recorded, shutdown_ is set, yet the error is ignored and the code attempts to open sockets. Why are the errors ignored?

That is a side effect (or a leftover, if you prefer) of the issue with detecting interfaces on non-Linux systems. It used to be that those platforms usually returned 0 interfaces unless someone edited interfaces.txt file which was usually not the case.

The idea was to go with graceful shutdown (create service and then shut it down immediately) rather than fail. This would in turn allow gentler clean-up (like notifying other DHCP components, closing leasequery or failover sockets etc). While there's no clear benefit at this stage of development, we will save some time of not requiring refactoring later.

But you are right. It does not make sense to continue. I have modified the code to abort on the first error it encounters. Also updated, dhcpv4 code.

src/bin/dhcp6/ctrl_dhcp6_srv.cc
Note: This code is very similar to that in src/bin/dhcp6/ctrl_dhcp4_srv.cc (from ticket #1651).

Yes, those classes are almost identical.

src/bin/dhcp6/main.cc
main(): Why does the software start up a ControlledDhcpv6Srv object then, after run() returns and the server is deleted, start up a Dhcp6Srv object?

Where do you see Dhcpv6Srv started after ControlledDhcpv6Srv is created and run()?

main(): Why does the ControlledDhcp4Srv have to be created via "new"? It can be created as an automatic object, which removes the need to delete the server when run() returns.

Done.

Suggest boost::lexical_cast be used to interpret the port number - the current use of strtol with a null value for endptr will not catch all invalid values.

Done.

dhcp6 and dhcp4 main() are practically identical. As they depend on a type (the type of the server), they could be combined into a single templated function.

Agree. Unification is good in general, but the code may start looking differently once we grow more features, e.g. failover in v4, prefix delegation or bulk leasequery in v6, etc. We should also consider if common code will not bring up dependency hell (like common code could possibly require v6 failover methods in dhcpv4 implementation). These are things that should be avoided. Nevertheless, unless there are serious reasons to do otherwise, unification is the answer. See new ticket #2146.

Overall
I suggest we open a new ticket to refactor the common control code in dhcp4 and dhcp6 into a single set of functions/classes (and tests).

Done, see ticket #2146.

comment:9 Changed 8 years ago by tomek

  • Owner changed from tomek to stephen

All changed checked in, please review.

comment:10 Changed 8 years ago by stephen

  • Owner changed from stephen to tomek

src/bin/dhcp4/dhcp4_srv.cc
Would suggest that "shutdown_" be initialized to "false" on entry to the Dhcp4Srv constructor (or set "false" in the "try" block). That way, the "return" in the "catch" clause becomes unnecessary.

src/bin/dhcp4/main.cc
Trivial point: suggest use "port_number < 1" rather than "port_number <= 0" in the test for the "-p" switch. The limits against which port_number is checked are then 1 and 65535, which are the values mentioned in the error message.

Another trivial point: suggest making both the order of the options in "getopt()" and the order that they are processed in the "case" statement alphabetical order - it is more logical and marginally easier to read.

src/bin/dhcp4/tests/dhcp4_test.py
Comments in test_skip_msgq refer to invalid port number, not disabling of the message queue.

src/bin/dhcp6/dhcp6_srv.cc
Would suggest that "shutdown_" be initialized to "false" on entry to the Dhcp4Srv constructor (or set "false" in the "try" block). That way, the "return" in the "catch" clause becomes unnecessary.

src/bin/dhcp6/main.cc

Where do you see Dhcpv6Srv started after ControlledDhcpv6Srv is created and run()?

In the bit that reads:

        ControlledDhcpv6Srv* server = new ControlledDhcpv6Srv(port_number);
        server->run();
        delete server;
        server = NULL;

        cout << "[b10-dhcp6] Initiating DHCPv6 operation." << endl;

        /// @todo: pass verbose to the actual server once logging is implemented
        Dhcpv6Srv* srv = new Dhcpv6Srv(port_number);

        srv->run();

(in commit f788c425a50d504d7d23d3945b67f2ad25513495). However that code has now been replaced.

Same comments apply here that were made in src/bin/dhcp4/main.cc.

src/bin/dhcp6/tests/dhcp6_test.py
Comments in test_skip_msgq refer to invalid port number, not the disabling of the message queue.

src/lib/dhcp/iface_mgr.cc
"if (maxfd < session_socket_)" needs braces around the "if" body.

Suggest adding a "TODO" for refactoring IfaceMgr::receive6() - it's fairly long. At the very least it could be separated into a method that identifies the socket on which the information is received and a method that reads the data.

I don't need to see it again after those changes have been made, and so you can commit.

comment:11 Changed 8 years ago by tomek

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

Changes pushed to master.

Note: See TracTickets for help on using tickets.