Opened 7 years ago

Closed 7 years ago

#3083 closed enhancement (complete)

Changes to allocation engine to propagate the information about the FQDNs

Reported by: marcin Owned by: marcin
Priority: medium Milestone: Sprint-DHCP-20130904
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

The allocation engine retrieves lease information from the lease data base when it allocates the lease. The lease data includes the information about the FQDNs stored for the client which should be accessed when the lease is being allocated or renewed. Based on this information server should make decisions about updating, adding, removing DNS records. In order for the server to make FQDN related decisions the information about the FQDNs should be propagated from the allocation engine. This ticket implements it.

Subtickets

Change History (9)

comment:1 Changed 7 years ago by marcin

  • Milestone changed from DHCP Outstanding Tasks to Sprint-DHCP-20130821

comment:2 Changed 7 years ago by marcin

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

comment:3 Changed 7 years ago by tomek

  • Milestone changed from Sprint-DHCP-20130821 to Sprint-DHCP-20130904

comment:4 Changed 7 years ago by marcin

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

I added two extensions to the Allocation Engine:

  • additional pointer passed to allocateAddress4 method which is used to pass the instance of the old lease. Using the copy copy of the old lease and the new lease, the server may determine what updates to DDNS should be performed.
  • additional parameters which set the FQDN specific flags hostname, fwd and rev update flags for the allocated lease

I also made a change to the memfile backend. With this change the backend returns the copy of the lease, not the lease from the container. That way, the modification on the returned lease instance does not modify the lease in the lease container. Also implemented updateLease4 and updateLease6 to allow updates of the lease in the data base.

comment:5 Changed 7 years ago by tmark

  • Owner changed from UnAssigned to tmark

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

  • Owner changed from tmark to marcin

Changes here are straight forward. A couple of questions and some misspellings. Unit tests run cleanly with valgrind, cppcheck looks clean too.


alloc_engine.h

/ engine. In particular, the DHCP server should be able to check whether
/ existing lease was returned, or new lease was allocated. When existing
/ lease was returned, server should check whether the FQDN has changed
/ between the allocation of the old and new lease. If so, server should
/ perform appropriate DNS update. If not, server may choose to not
/ perform the update. The information about the old lease is returned via
/ @c old_lease parameter. If NULL value is returned, it is an indication

General question, should any of the above behavior be configurable?


alloc_engine.h

Misspellings "responisibility" and "responibility" in comments for
fwd_dns_update and rev_dns_update parameters.


memfile_lease_mgr.cc

Memfile_LeaesMgr::updateLease4 and updateLease6 both warn that the pointers are not validated. Is this a performance consideration? Almost everywhere else
in our codebase it seems like we check. The code will crash if they are
invalid. What would Stephen say?


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

  • Owner changed from marcin to tmark

Replying to tmark:

Changes here are straight forward. A couple of questions and some misspellings. Unit tests run cleanly with valgrind, cppcheck looks clean too.

Thanks!

Before I corrected the spelling, I merged master to trac3083 to resolve the conflicts with recent changes to the Allocation Engine to implement hooks. Therefore, when you pull from the branch you will observe many commits being added to it.

Proposed ChangeLog entry:

6XX.	[func]*		marcin
	libdhcpsrv: Implemented changes to lease allocation engine to
	propagate information about client's FQDN.
	(Trac #3083, git abcdefg)

Other responses below:


alloc_engine.h

/ engine. In particular, the DHCP server should be able to check whether
/ existing lease was returned, or new lease was allocated. When existing
/ lease was returned, server should check whether the FQDN has changed
/ between the allocation of the old and new lease. If so, server should
/ perform appropriate DNS update. If not, server may choose to not
/ perform the update. The information about the old lease is returned via
/ @c old_lease parameter. If NULL value is returned, it is an indication

General question, should any of the above behavior be configurable?

The server logic which is described here will be implemented with the #3035. This will also include some stub configuration for the server behaviour.

To answer your question... I believe it doesn't need to be configurable whether the server updates the existing lease or it doesn't when nothing has changed in the notion of the client's FQDN. Why would server do that when it has sufficient amount of information that nothing has changed? If we ever decide that it has to, implementation of the additional parameter that turns the "always update mode" will be straightforward. Probably as simple as addition of the one ''if'' statement (and the parameter in the config parser of course).

The general idea behind returning the instance of the old lease is that server may make any decision by comparing the two leases. In particular, it may remove the old DNS record when it detects changes in FQDN. Any changes to the server's behaviour should be transparent to the allocation engine, which should blindly pass the FQDN data to and from the database.


alloc_engine.h

Misspellings "responisibility" and "responibility" in comments for
fwd_dns_update and rev_dns_update parameters.

Thanks. I corrected those, plus I made some other small corrections in comments. I also modified the style of the new parameters descriptions (second line aligned with the start of the description of the first line), to make it consistent with existing parameters' descriptions.


memfile_lease_mgr.cc

Memfile_LeaesMgr::updateLease4 and updateLease6 both warn that the pointers are not validated. Is this a performance consideration? Almost everywhere else
in our codebase it seems like we check. The code will crash if they are
invalid. What would Stephen say?

I agree. I simply followed the rule for all other functions in either Memfile or MySQL backends, where lease pointers are not sanity-checked. It doesn't make sense to make two functions throw exceptions when there are 10 other functions which don't. Since backends are in the libdhcpsrv, which could theoretically be used by the external code I think it may be a good idea to sanity-check arguments of public functions in backends. I created a ticket for this: #3120.


comment:8 Changed 7 years ago by tmark

  • Owner changed from tmark to marcin

Changes and responses are acceptable. Please merge it in.

comment:9 Changed 7 years ago by marcin

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

Merged with commit: 37af28303d1cd61f675faea969cd1159df65bf9d.

Note: See TracTickets for help on using tickets.