Opened 12 months ago

Closed 9 months ago

#5543 closed enhancement (complete)

wipe all leases command

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Kea1.4
Component: management API Version: git
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

We do have commands to remove all leases from specific subnet: lease4-wipe and lease6-wipe. We should provide the capability to wipe leases from all subnets in one command.

The likely approach would be to make the subnet-id parameter optional. If not specified, it would wipe all leases. Alternatively, we could give subnet-id 0 a special meaning.

Subtickets

Change History (7)

comment:1 Changed 12 months ago by tomek

  • Summary changed from wipe all leases to wipe all leases command

comment:2 Changed 10 months ago by tomek

  • Milestone changed from Kea1.x to Kea1.4

As requested by a customer, we will provide this ability in 1.4.

comment:3 Changed 10 months ago by tomek

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

comment:4 Changed 9 months ago by tomek

  • Owner changed from tomek to Unassigned
  • Status changed from assigned to reviewing

The code is now ready for review. Proposed changelog:

1384.	[func]		tomek
	lease4-wipe and lease6-wipe are now able to wipe all leases
	from all configured subnets if subnet-id specified is 0.
	(Trac #5543, git tbd)

comment:5 Changed 9 months ago by marcin

  • Owner changed from Unassigned to marcin

comment:6 follow-up: Changed 9 months ago by marcin

  • Owner changed from marcin to tomek

Reviewed commit e5b837a422c913fe9815edfe5a81866d46ee5771

doc/guide/hooks.xml
While I concur that it shouldn't be the case that there are multiple versions of the same command, one that requires subnet-id (to wipe leases by subnet-id) and another one which requires that subnet-id is not present (to wipe all leases), I also think that we should ALLOW not specifying subnet-id if someone wants to wipe all leases. In other words, I'd allow two forms:

"command": "lease4-wipe",
"arguments": {
    "subnet-id": 0
}

and

"command": "lease4-wipe"

You don't know if in the future we won't require wiping leases by some other arguments: e.g. "wipe all IA_NAs", "wipe all from a pool", "wipe all for shared network". In fact, shared network can be considered a special type of a "subnet" and if someone wants to wipe leases for a subnet I don't see why someone who is extensively aggregating subnets with shared networks wouldn't want to remove all leases for a shared network he is de-activating.

In such case, it is probably not a best idea to REQUIRE that subnet-id is specified, because it will be irrelevant in such cases. Therefore, I suggest that we are less restrictive and allow for not specifying subnet-id. Still, subnet-id of 0 is fine as an alternative.

src/hooks/dhcp/lease_cmds/lease_cmds.cc
I am not very enthusiastic to iterate over all subnets to wipe all leases. First of all, there may be leases in the database which are not associated with any subnet (because the subnet is gone from the configuration). Secondly, it has performance implications for installations which configure many subnets. Admittedlly, when you wipe your database, you probably don't service DHCP clients at the same time. But the responsiveness of the system to subsequent commands is worse. It is very simple to delete all database records with a single query and I think we should be going into that direction.

If nothing else, we should at least have a todo in the code or check with the customers what the use cases are for the leaseX-wipe commands.

I also have a concern about logging all subnet identifiers for which the leases have been wiped. If you have 1000 subnets it will be unreadable in the log.

Also, on that matter. The list of subnet ids is terminated by a spurious space character. It can be easily fixed by prefixing an id with a space character rather than appending id with a space character following it.

comment:7 in reply to: ↑ 6 Changed 9 months ago by tomek

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

Replying to marcin:

Reviewed commit e5b837a422c913fe9815edfe5a81866d46ee5771

doc/guide/hooks.xml
While I concur that it shouldn't be the case that there are multiple versions of the same command, one that requires subnet-id (to wipe leases by subnet-id) and another one which requires that subnet-id is not present (to wipe all leases), I also think that we should ALLOW not specifying subnet-id if someone wants to wipe all leases. In other words, I'd allow two forms:

"command": "lease4-wipe",
"arguments": {
    "subnet-id": 0
}

and

"command": "lease4-wipe"

You don't know if in the future we won't require wiping leases by some other arguments: e.g. "wipe all IA_NAs", "wipe all from a pool", "wipe all for shared network". In fact, shared network can be considered a special type of a "subnet" and if someone wants to wipe leases for a subnet I don't see why someone who is extensively aggregating subnets with shared networks wouldn't want to remove all leases for a shared network he is de-activating.

In such case, it is probably not a best idea to REQUIRE that subnet-id is specified, because it will be irrelevant in such cases. Therefore, I suggest that we are less restrictive and allow for not specifying subnet-id. Still, subnet-id of 0 is fine as an alternative.

Ack. Updated code to work with either subnet-id equal 0 or not subnet-id specified.


src/hooks/dhcp/lease_cmds/lease_cmds.cc
I am not very enthusiastic to iterate over all subnets to wipe all leases. First of all, there may be leases in the database which are not associated with any subnet (because the subnet is gone from the configuration). Secondly, it has performance implications for installations which configure many subnets. Admittedlly, when you wipe your database, you probably don't service DHCP clients at the same time. But the responsiveness of the system to subsequent commands is worse. It is very simple to delete all database records with a single query and I think we should be going into that direction.

If nothing else, we should at least have a todo in the code or check with the customers what the use cases are for the leaseX-wipe commands.

Todo added.

I also have a concern about logging all subnet identifiers for which the leases have been wiped. If you have 1000 subnets it will be unreadable in the log.

Also, on that matter. The list of subnet ids is terminated by a spurious space character. It can be easily fixed by prefixing an id with a space character rather than appending id with a space character following it.

Updated as suggested.

Thanks for the review. The code is now merged on master.

Note: See TracTickets for help on using tickets.