#5442 closed enhancement (complete)

Quiesce Kea via command

Reported by: marcin Owned by: marcin
Priority: medium Milestone: Kea1.4
Component: remote-management 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

This is one of the first tickets required to support High Availability in Kea. If there are two servers working as failover peers and one of the servers is recovering after a failure, it must be possible to freeze another server to make sure that it doesn't do DHCP while the peer is synchronizing the database. Eventually we want to be able to select specific subnets or networks for which we'll freeze DHCP, but this ticket only calls for supporting the general case of freezing the DHCP function for all scopes.

Subtickets

Change History (12)

comment:1 Changed 19 months ago by marcin

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

comment:2 Changed 19 months ago by marcin

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

This ticket is ready for review. It currently allows for globally disabling/enabling the service. We're going to implement selective disabling of the DHCP service in future tickets.

Proposed ChangeLog entry:

13XX.	[func]		marcin
	Added support for "dhcp-enable" and "dhcp-disable" commands in
	the DHCPv4 and DHCPv6 server.
	(Trac #5442, git abcd)

comment:3 Changed 19 months ago by marcin

  • Milestone changed from Kea-proposed to Kea1.4

Moving this to 1.4 as it is required for HA implementation.

comment:4 Changed 19 months ago by fdupont

  • Owner changed from UnAssigned to fdupont

comment:5 follow-up: Changed 19 months ago by fdupont

  • Owner changed from fdupont to marcin

I don't understand this piece of code

    // If the DHCP service has been globally disabled, drop the packet.
    if (network_state_.isServiceEnabled()) {
        LOG_DEBUG(bad_packet4_logger, DBG_DHCP4_BASIC,
                  DHCP4_PACKET_DROP_0008)
            .arg(query->getLabel());
        processPacket(query, rsp);
    }

I suggest to reverse the if and to move the processing in an else branch as:

    // If the DHCP service has been globally disabled, drop the packet.
    if (!etwork_state_.isServiceEnabled()) {
        LOG_DEBUG(bad_packet4_logger, DBG_DHCP4_BASIC,
                  DHCP4_PACKET_DROP_0008)
            .arg(query->getLabel());
    } else {
        processPacket(query, rsp);
    }

Of course I have the same concern about dhcp6_srv.cc.

In ctrl_dhcp6_srv.cc a 4 -> 6 in message << "DHCPv4 service disabled";

BTW you have a 4 -> 6 in dhcp6_messages.mes in DHCP6_PACKET_DROP_PARSE_FAIL caught in git diff context.
I suggest to check the whole file (search for 4s) and its sibling in dhcp4 directory.

In network_state.cc please use NETWORK_STATE_TIMER_NAME as the argument of timer_mgr_->setup() (first for consistency and second image what happens with a typo :-)

Please add in network state unit tests a new one which verifies the default (i.e. just after construction) is "enable" for v4 and v6.
All tests are about v4. At the exception of the new one IMHO it does not matter so I suggest to use v6 one test on two (i.e. alternate v4 and v6).

Spelling: network_state.h: tiemout fulfils explcitly

network_state.cc: couting
network_state_unittest.cc: runninr

I didn't change things as I didn't checkout the branch...

Built and ran tests on my macOS high sierra dec box.

comment:6 follow-up: Changed 19 months ago by fdupont

BTW when the subnet and shared network feature will be added IMHO it is important to insist on the fact it applies to the selected subnet in the (todo) documentation.

comment:7 in reply to: ↑ 5 Changed 19 months ago by marcin

  • Owner changed from marcin to fdupont

Replying to fdupont:

I don't understand this piece of code

    // If the DHCP service has been globally disabled, drop the packet.
    if (network_state_.isServiceEnabled()) {
        LOG_DEBUG(bad_packet4_logger, DBG_DHCP4_BASIC,
                  DHCP4_PACKET_DROP_0008)
            .arg(query->getLabel());
        processPacket(query, rsp);
    }

I suggest to reverse the if and to move the processing in an else branch as:

    // If the DHCP service has been globally disabled, drop the packet.
    if (!etwork_state_.isServiceEnabled()) {
        LOG_DEBUG(bad_packet4_logger, DBG_DHCP4_BASIC,
                  DHCP4_PACKET_DROP_0008)
            .arg(query->getLabel());
    } else {
        processPacket(query, rsp);
    }

Of course I have the same concern about dhcp6_srv.cc.

Stupid me. Things like this happen usually when you do something and think about something else at the same time :-) Good catch.

In ctrl_dhcp6_srv.cc a 4 -> 6 in message << "DHCPv4 service disabled";

Corrected.

BTW you have a 4 -> 6 in dhcp6_messages.mes in DHCP6_PACKET_DROP_PARSE_FAIL caught in git diff context.
I suggest to check the whole file (search for 4s) and its sibling in dhcp4 directory.

Corrected and checked.

In network_state.cc please use NETWORK_STATE_TIMER_NAME as the argument of timer_mgr_->setup() (first for consistency and second image what happens with a typo :-)

Made the change.

Please add in network state unit tests a new one which verifies the default (i.e. just after construction) is "enable" for v4 and v6.
All tests are about v4. At the exception of the new one IMHO it does not matter so I suggest to use v6 one test on two (i.e. alternate v4 and v6).

I created another test for v6 specifically.

Spelling: network_state.h: tiemout fulfils explcitly

network_state.cc: couting
network_state_unittest.cc: runninr

Corrected those typos.

I didn't change things as I didn't checkout the branch...

Built and ran tests on my macOS high sierra dec box.

Thanks for the review.

comment:8 in reply to: ↑ 6 ; follow-up: Changed 19 months ago by marcin

Replying to fdupont:

BTW when the subnet and shared network feature will be added IMHO it is important to insist on the fact it applies to the selected subnet in the (todo) documentation.

I am not sure if I should do anything about it now, and what exactly I should do.

comment:9 in reply to: ↑ 8 Changed 19 months ago by fdupont

Replying to marcin:

Replying to fdupont:

BTW when the subnet and shared network feature will be added IMHO it is important to insist on the fact it applies to the selected subnet in the (todo) documentation.

I am not sure if I should do anything about it now, and what exactly I should do.

=> add some logic to drop packets after subnet selection. It is related to #5443 so I expect it will be done once.

comment:10 Changed 19 months ago by fdupont

  • Owner changed from fdupont to marcin

Reviewed last changes. I added a unit test to check the default is the enable state. Please pull, review it and merge.

comment:11 Changed 19 months ago by fdupont

BTW according to what ISC DHCP does we'll need a default value for the case no subnet is selected.

comment:12 Changed 19 months ago by marcin

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

Thanks for the reviews. Merged with commit 36dc68ff7aa8b3cfd265c4f982d10248590039bd

Note: See TracTickets for help on using tickets.