Opened 8 years ago

Closed 6 years ago

#1555 closed enhancement (complete)

refactor: DHCP listening on more than one interface, timeout

Reported by: tomek Owned by: marcin
Priority: medium Milestone: Sprint-DHCP-20130731
Component: dhcp 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

Both DHCPv4 and DHCPv6 components listen on a single socket. They need to use select() to listen on many sockets. Also, timeout must be specified. That will be needed later on, when certain actions will happen based on time, rather than packet reception (i.e. lease expiration).

Time must be specified on sub-second resolution (ms or us).

Subtickets

Change History (10)

comment:1 Changed 8 years ago by shane

  • Milestone changed from New Tasks to DHCP 2012

comment:2 Changed 6 years ago by tomek

The code was apparently updated to listen on every interface present. There is a configuration parameter Dhcp/Interface and Dhcp6/Interface (which should be renamed to plural form as it is a list possibly more then one interface) that takes a list of interfaces. It can be specified, but the values are ignored.

The original idea was to allow explicit interface names to be specific or a keyword 'all' which will autodetect and open on all (as it does now).

IfaceMgr::openSockets{4,6} methods should be updated.

Also see ticket #3038.

comment:3 Changed 6 years ago by marcin

  • Milestone changed from DHCP Outstanding Tasks to Sprint-DHCP-20130717
  • Owner set to marcin
  • Status changed from new to accepted
  • Type changed from defect to enhancement

comment:4 Changed 6 years ago by marcin

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

I have updated the code as follows:

  • renamed ''interface'' to ''interfaces'' parameter which holds the list of interfaces
  • Interface names or asterisk can be specified on the list. The asterisk means ''all interfaces''. it can be used concurrently with explicit interface names.
  • When configuration is committed, all sockets are reopened according to the new configuration.
  • Interface Manager holds additional flag for each interface which marks if it is active or inactive. Inactive interfaces are not used to listen to DHCP traffic.
  • Bind10 updated with the examples of interface selection.

Important note: One of the commits cleans whitespaces in multiple files. I recommend to use:

git diff -w dd01c785f0d8eb616ee4179299dc53e0d2c0015c

to generate the diff. The -w flag will exclude whitespace changes from the diff.

Please review.

comment:5 Changed 6 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:6 follow-up: Changed 6 years ago by tomek

  • Owner changed from tomek to marcin

General comment: Is there any specific use case that would make sense to mix wildcard and specific interface name? Wildcard implies all interfaces, including those that are explicitly mentioned.

bind10-guide.xml
Please update first example in section 17.2 ("Dhcp4/interface" => "Dhcp4/interfaces") and in 18.2 ("Dhcp6/interface" => "Dhcp6/interfaces")

ctrl_dhcp4_srv.cc
Please move ControlledDhcpv4Srv::openActiveSockets to Dhcpv4Srv class. Please add @todo in its code to mention that it should be optimized and not close existing open sockets. We don't care about closing/reopening UDP and raw much, but some time in the future we'll have TCP sockets as well (for failover and for bulk leasequery).

I like the concept of interfaces being active, but we'll need to expand it a bit in the future. Interfaces are sometimes marked as to be activated, but they are not physically ready. Dibbler supports such mode of operation. For example you can say that interface wifi0 should be configured, but it is not ready yet, so the server (or client) periodically monitors its state and starts configuration once physical layer is up and running (e.g. wifi association or Ethernet cable plugged). While the current approach is ok for now, it will eventually be evolved into a list of interfaces that should be activated. (a diff between currently activated and the list user specified).

openActiveSockets(): You never check if iface_ptr is non-NULL. The code will crash if it is NULL.

dhcp4_srv.h
getPort() - please add a comment that the server listens on port 67 and any other ports are used for testing only.

dhcp4_srv_unittest.cc
Dhcp4ParserTest.selectedInterfaces - make sure that test works on machines and systems that do not have ethX interfaces, e.g. FreeBSD.

iface_mgr.cc
Please study "if" clauses around lines 297 and 364. I was not aware of the comma operator. BTW I find this page very englightening: http://en.wikipedia.org/wiki/Comma_operator To be honest, I was not aware that such an operator even exists.

activateAllIfaces(): I'm on a fence with this one. On one hand, we could expand * into list of all interfaces. On the other hand, there are cases where interfaces may appear later (e.g. ppp0), so it may be useful to keep the flag. When new interface appears later and the flag is set, we could then immediately mark it as active.

cfgmgr.h
The comment for activateAllIfaces() and deleteActiveIfaces() is wrong. It does not configure the server (i.e. the function does not cause the server to open sockets), it merely stores the list.

Most DHCPv4 comments also apply to DHCPv6.

In general, a very good work. Thank you for doing this.

Last edited 6 years ago by tomek (previous) (diff)

comment:7 in reply to: ↑ 6 ; follow-up: Changed 6 years ago by marcin

  • Owner changed from marcin to tomek

Replying to tomek:

General comment: Is there any specific use case that would make sense to mix wildcard and specific interface name? Wildcard implies all interfaces, including those that are explicitly mentioned.

IMHO, it is useful if you have existing configuration with explicit interface names listed and you want to enable all temporarily. You can do it by simply adding an asterisk to the list (to enable all) and remove it if you want to fall back to earlier configuration.

bind10-guide.xml
Please update first example in section 17.2 ("Dhcp4/interface" => "Dhcp4/interfaces") and in 18.2 ("Dhcp6/interface" => "Dhcp6/interfaces")

Done

ctrl_dhcp4_srv.cc
Please move ControlledDhcpv4Srv::openActiveSockets to Dhcpv4Srv class. Please add @todo in its code to mention that it should be optimized and not close existing open sockets. We don't care about closing/reopening UDP and raw much, but some time in the future we'll have TCP sockets as well (for failover and for bulk leasequery).

I like the concept of interfaces being active, but we'll need to expand it a bit in the future. Interfaces are sometimes marked as to be activated, but they are not physically ready. Dibbler supports such mode of operation. For example you can say that interface wifi0 should be configured, but it is not ready yet, so the server (or client) periodically monitors its state and starts configuration once physical layer is up and running (e.g. wifi association or Ethernet cable plugged). While the current approach is ok for now, it will eventually be evolved into a list of interfaces that should be activated. (a diff between currently activated and the list user specified).

If they are not physically ready, they will not be opened - IfaceMgr? doesn't open sockets on interfaces which are not running. I believe, we should have some extended support for the validation of the interfaces being input by the user - in particular, we could check that the interface name is valid. I guess, that was not the objective of this ticket so I simply took the simplest approach.

openActiveSockets(): You never check if iface_ptr is non-NULL. The code will crash if it is NULL.

I know, but it shouldn't be NULL because interface should be always present. Note that the code iterates over the collection of existing interfaces and for each of them calls getIface. The getIface wouldn't return NULL for the interface that we already found existent.

dhcp4_srv.h
getPort() - please add a comment that the server listens on port 67 and any other ports are used for testing only.

Added for DHCPv4 and v6.

dhcp4_srv_unittest.cc
Dhcp4ParserTest.selectedInterfaces - make sure that test works on machines and systems that do not have ethX interfaces, e.g. FreeBSD. This test is independent from the IfaceMgr? so it should't be an issue. But yes. I will run the test on our build bot before I merge.

iface_mgr.cc
Please study "if" clauses around lines 297 and 364. I was not aware of the comma operator. BTW I find this page very englightening: http://en.wikipedia.org/wiki/Comma_operator To be honest, I was not aware that such an operator even exists.

We both have learned something. Obviously it was a typo. Thanks for catching this.

activateAllIfaces(): I'm on a fence with this one. On one hand, we could expand * into list of all interfaces. On the other hand, there are cases where interfaces may appear later (e.g. ppp0), so it may be useful to keep the flag. When new interface appears later and the flag is set, we could then immediately mark it as active.

I like the flexibility that the asterisk give with respect to addition of new interfaces. If we ever decide that this design is wrong it will be little effort to change it.

cfgmgr.h
The comment for activateAllIfaces() and deleteActiveIfaces() is wrong. It does not configure the server (i.e. the function does not cause the server to open sockets), it merely stores the list.

Well, it depends what you mean by "configure". The CfgMgr is a class which holds the server 's current configuration. So, changing the state of the CfgMgr could be called configuration. But, since it is likely to be misinterpreted, I changed the comments.

Most DHCPv4 comments also apply to DHCPv6.

Done for DHCPv6 either. At this point, I again regret that we don't have shared code here.

In general, a very good work. Thank you for doing this.

Thanks

Proposed ChangeLog entry:

6XX.	[func]		marcin
	b10-dhcp4, b10-dhcp6: Implemented selection of the interfaces
	that server listens on, using Configuration Manager. It is
	possible to specify interface names explicitly or use asterisk
	to specify that server should listen on all available interfaces.
	Sockets are reopened according to the new configuration as
	soon as it is applied.
	(Trac #1555, git abc)

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

  • Owner changed from tomek to marcin

Replying to marcin:

Replying to tomek:

General comment: Is there any specific use case that would make sense to mix wildcard and specific interface name? Wildcard implies all interfaces, including those that are explicitly mentioned.

IMHO, it is useful if you have existing configuration with explicit interface names listed and you want to enable all temporarily. You can do it by simply adding an asterisk to the list (to enable all) and remove it if you want to fall back to earlier configuration.

Fair enough.

openActiveSockets(): You never check if iface_ptr is non-NULL. The code will crash if it is NULL.

I know, but it shouldn't be NULL because interface should be always present. Note that the code iterates over the collection of existing interfaces and for each of them calls getIface. The getIface wouldn't return NULL for the interface that we already found existent.

It shouldn't, but let's be on the safe side. Please add if (!iface) throw something; and be done with it.

iface_mgr.cc
Please study "if" clauses around lines 297 and 364. I was not aware of the comma operator. BTW I find this page very englightening: http://en.wikipedia.org/wiki/Comma_operator To be honest, I was not aware that such an operator even exists.

We both have learned something. Obviously it was a typo. Thanks for catching this.

Yes, we did. It's an interesting trick and will increase our chances in the obfuscated C code contest. :)

I like the flexibility that the asterisk give with respect to addition of new interfaces. If we ever decide that this design is wrong it will be little effort to change it.

Ok, let the code stay as it is.

Well, it depends what you mean by "configure". The CfgMgr is a class which holds the server 's current configuration. So, changing the state of the CfgMgr could be called configuration. But, since it is likely to be misinterpreted, I changed the comments.

Thanks.

Done for DHCPv6 either. At this point, I again regret that we don't have shared code here.

Although the code looks similar, there are subtle differences that would make working with the common code a pain or at least would require a lot of careful thought. And since the Pkt4 and Pkt6 do not share common base class, you couldn't really share much mode.

Proposed ChangeLog entry:

6XX.	[func]		marcin
	b10-dhcp4, b10-dhcp6: Implemented selection of the interfaces
	that server listens on, using Configuration Manager. It is
	possible to specify interface names explicitly or use asterisk
	to specify that server should listen on all available interfaces.
	Sockets are reopened according to the new configuration as
	soon as it is applied.
	(Trac #1555, git abc)

Looks good. Please add two if (!iface) in the code and then it is ready for merge (assuming you checked that the tests that use ethX names work on systems that don't have such interface names).

comment:9 Changed 6 years ago by tomek

An extra comment about getIface() and the general way how IfaceMgr? stores interface info. We may rework it some time in the future to use name->object and ifindex->object maps for performance reasons. I'm aware of one production deployment that has over 100 interface names (lots of vlans), so the performance degradation may be non-trivial in such case. If we rework the code, this method may suddenly start returning NULLs.

One simple "if" is not that big of a deal.

comment:10 Changed 6 years ago by marcin

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

Merged with the commit f48a3bff3fbbd15584d788a264d5966154394f04.

Note: See TracTickets for help on using tickets.