Opened 3 years ago

Closed 3 years ago

#5150 closed enhancement (complete)

Implement config-test command

Reported by: tomek Owned by: fdupont
Priority: medium Milestone: Kea1.2
Component: remote-management Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Mozilla Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 2
Total Hours: 4 Internal?: no

Description

With 3770 work, we gained the ability to test configuration from the command line. However, we also need to make it available over the control channel. This is covered by this ticket.

For details, see http://kea.isc.org/wiki/ControlAPIRequirements.

Subtickets

Change History (21)

comment:1 Changed 3 years ago by tomek

  • Summary changed from Implement test-config support to Implement test-config command

comment:2 Changed 3 years ago by fdupont

There are at least 4 ways to implement with of course different semantics from forking a process using the new -t file command to reconfig forcing a rollback.

comment:3 Changed 3 years ago by tomek

  • Sub-Project changed from DHCP to Mozilla

comment:4 Changed 3 years ago by fdupont

Refining last comment into:
There are two principal ways to perform this:

  • the external way using the command line
  • the internal way using the check_only flag in configure

Note we have the choice for DHCPv4, DHCPv6 and D2 servers, and for the control agent for itself or for one of these 3 servers. So for instance we can implement the external way only indirectly in the control agent (it only assumes the control agent can execute a shell command).

BTW the is ticket depends on #5152.

comment:5 Changed 3 years ago by fdupont

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

Change the dependency: even if #5152 is related there is no sense to provide test-config without set-config so the dependency is more #102 (trac102a).

comment:6 Changed 3 years ago by fdupont

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

I did two branches:

  • trac5150 which is based on #102 and adds support to DHCPv[46] servers
  • trac5150_process which is based on trac5150 with trac5152 merged into and which adds support to lib process with tests using kea-shell

Both are available for review (but please review #102 before and #5152 too for the second).

comment:7 Changed 3 years ago by fdupont

Taken to change command name.

comment:8 Changed 3 years ago by fdupont

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

comment:9 Changed 3 years ago by fdupont

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

Done, the trac5150_process branch is ready for review.

comment:10 Changed 3 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:11 follow-up: Changed 3 years ago by tomek

#102 is now reviewed. I have no idea why you said that #5150 (testing configuration) depends on #102 (reporting software version). These are completely different functionalities. Anyway, will get back to #5150 review tomorrow.

comment:12 in reply to: ↑ 11 Changed 3 years ago by fdupont

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

Replying to tomek:

#102 is now reviewed. I have no idea why you said that #5150 (testing configuration) depends on #102 (reporting software version). These are completely different functionalities. Anyway, will get back to #5150 review tomorrow.

=> both use control channel commands so I adopt a style for easy commands (#102) and keep it for more complex commands (#5150). Note it seems for src/lib/process you strongly disagree with this style so I am taken #5150 to update it. If you want to not wait and review the DHCPvX server part the trac5150 (vs trac5150_process) branch was based on trac102a before the name change and reorg.

comment:13 Changed 3 years ago by fdupont

Rebasing on trac102b.

comment:14 Changed 3 years ago by fdupont

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

Should be available for review again.

comment:15 Changed 3 years ago by tomek

  • Summary changed from Implement test-config command to Implement config-test command

Title updated (test-config => config-test)

comment:16 follow-up: Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 0 to 2
  • Owner changed from tomek to fdupont
  • Total Hours changed from 0 to 2

This review covers only two last commits (those specific to 5150), i.e.
9adddb71a94abff89ca499eb07a14a0ab01062f2 and
19206bad07e987afc7b7bc7c927cabda5a78b193. Note that earlier commits
from trac102a are reviewed in #102. Also keep in mind that 102 code
was significantly changed, so please DO NOT MERGE this code as is.
It seems that cherry-picking the last three (two yours and one mine with
corrections) is the best way forward, but I leave that up to you.

As agreed on jabber on Thursday, the command should
be called config-test, rather than test-config. Updated the code.
Please pull.

ctrl_dhcpX_srv.cc
The code in commandTestConfigHandler should use constants defined
in command_interpreter.h (isc::config::CONTROL_RESULT_ERROR instead of
1). Updated.

ctrl_dhcpX_srv_unittest.cc
Removed several C-style comments. Also wrapped too long lines.
Our coding guidelines say that the lines should in general be no longer
than 80 chars, but we can do up to 100 if needed.


The code builds and unit-tests pass on Ubuntu 16.04 x64.

This change requires ChangeLog? entry. If you don't have any better
text, here's my proposal:

12XX.	[func]		fdupont
	A new command: config-test has been implemented in DHCPv4 and
	DHCPv6 servers. It allows checking whether new configuration
	looks correct.
	(Trac #5150, git tbd)

comment:17 in reply to: ↑ 16 Changed 3 years ago by fdupont

Replying to tomek:

This review covers only two last commits (those specific to 5150), i.e.
9adddb71a94abff89ca499eb07a14a0ab01062f2 and
19206bad07e987afc7b7bc7c927cabda5a78b193. Note that earlier commits
from trac102a are reviewed in #102. Also keep in mind that 102 code
was significantly changed, so please DO NOT MERGE this code as is.
It seems that cherry-picking the last three (two yours and one mine with
corrections) is the best way forward, but I leave that up to you.

=> I am afraid you reviewed an old branch: the last one is trac5150a.

As agreed on jabber on Thursday, the command should
be called config-test, rather than test-config. Updated the code.
Please pull.

=> it was updated when this decision was taken. And all branches
newer than trac5150 itself include lib process code.

ctrl_dhcpX_srv.cc
The code in commandTestConfigHandler should use constants defined
in command_interpreter.h (isc::config::CONTROL_RESULT_ERROR instead of
1). Updated.

=> I agree. I chase for this from time to time.

ctrl_dhcpX_srv_unittest.cc
Removed several C-style comments. Also wrapped too long lines.
Our coding guidelines say that the lines should in general be no longer
than 80 chars, but we can do up to 100 if needed.

=> as I am against lines longer than 80 characters (I coded in Fortran
many many years ago) I am a bit surprised but I have no concern
in making the code readable.

This change requires ChangeLog? entry. If you don't have any better
text, here's my proposal:

12XX.	[func]		fdupont
	A new command: config-test has been implemented in DHCPv4 and
	DHCPv6 servers. It allows checking whether new configuration
	looks correct.
	(Trac #5150, git tbd)

=> I leave this for the next week as there is no possible trivial merge.

comment:18 Changed 3 years ago by fdupont

I reviewed the changes done for trac5150 and merged them to trac5150a (not yet committed).

comment:19 Changed 3 years ago by fdupont

  • Owner changed from fdupont to tomek

Committed. I fixed a few missing stuff from rebasing.
trac5150a is now ready for review (lib process, bin/agent and bin/shell).

comment:20 Changed 3 years ago by tomek

  • Owner changed from tomek to fdupont
  • Total Hours changed from 2 to 4

I did review the changes from trac5150a. They mostly consisted the changes that I reviewed earlier on trac5150. The new changes look good.

The code builds and unit-tests pass on MacOS 10.11.5.

Ok, the code seems to be ready for merge.

comment:21 Changed 3 years ago by fdupont

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

Merged, closing.

Note: See TracTickets for help on using tickets.