Opened 3 years ago

Closed 3 years ago

#5152 closed enhancement (complete)

Check D2 and agent configuration file

Reported by: fdupont 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: DHCP Feature Depending on Ticket:
Estimated Difficulty: 4 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Cf #3770. Depends on #5110 for the D2 parser update.

Subtickets

Change History (11)

comment:1 Changed 3 years ago by fdupont

Depends on #5134 for the agent parser.

comment:2 Changed 3 years ago by fdupont

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

comment:3 Changed 3 years ago by fdupont

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

Done. I added some code in src/lib/process to make an internal support of a check-config command easier.
BTW the control agent documentation does not yet exist so was not updated...

comment:4 Changed 3 years ago by tmark

  • Owner changed from UnAssigned to tmark

comment:5 follow-up: Changed 3 years ago by tmark

  • Owner changed from tmark to fdupont

Overall you threaded this through pretty nicely.

d_process.h

DProcess::configure()
Why not make check_only parameter default to false?


d2_process.c:

D2Process::configure()

I think this log message should indicate the value of check_only:

    LOG_DEBUG(d2_logger, DBGLVL_TRACE_BASIC,
              DHCP_DDNS_CONFIGURE).arg(config_set->str());

Maybe change the message to this:

    % DHCP_DDNS_CONFIGURE configuration %1 received: %2

and invoke like this:

    LOG_DEBUG(d2_logger, DBGLVL_TRACE_BASIC,
              DHCP_DDNS_CONFIGURE)
              .arg(check_only ? "check" : "update")
              .arg(config_set->str());

d_controller.h

Please expand on what check_only_ means in its commentary, something like:
"Indicates if command line argument to config check only was specified"

You defined isCheckOnly() and setCheckOnly(), yet they are never used nor
tested.

d_controller.cc

DControllerBase::launch() -

Shouldn't this if statement:

77 if (!test_mode && check_only_) {

simply be this:

77 if (check_only_) {

I would greatly prefer to see everything inside that block moved to its
own method, checkConfigOnly(), so the block is compressed to:

if (isCheckOnly()) {

checkConfigOnly();
return;

}

Configuration checking is the "exceptional" case and encapsulating it in a
method unclutters launch() who's primary function is start the process
for live operation.

comment:6 in reply to: ↑ 5 Changed 3 years ago by fdupont

Replying to tmark:

Overall you threaded this through pretty nicely.

d_process.h

DProcess::configure()
Why not make check_only parameter default to false?

=> mainly because I didn't touch it but I agree it should.

d2_process.c:

D2Process::configure()

I think this log message should indicate the value of check_only:

    LOG_DEBUG(d2_logger, DBGLVL_TRACE_BASIC,
              DHCP_DDNS_CONFIGURE).arg(config_set->str());

Maybe change the message to this:

    % DHCP_DDNS_CONFIGURE configuration %1 received: %2

and invoke like this:

    LOG_DEBUG(d2_logger, DBGLVL_TRACE_BASIC,
              DHCP_DDNS_CONFIGURE)
              .arg(check_only ? "check" : "update")
              .arg(config_set->str());

=> it is a bit intrusive but I agree it is an improvement.

d_controller.h

Please expand on what check_only_ means in its commentary, something like:
"Indicates if command line argument to config check only was specified"

You defined isCheckOnly() and setCheckOnly(), yet they are never used nor
tested.

=> hum, I believe setCheckOnly should be removed.

d_controller.cc

DControllerBase::launch() -

Shouldn't this if statement:

77 if (!test_mode && check_only_) {

simply be this:

77 if (check_only_) {

=> I was afraid of test_mode misused.

I would greatly prefer to see everything inside that block moved to its
own method, checkConfigOnly(), so the block is compressed to:

if (isCheckOnly()) {

checkConfigOnly();
return;

}

Configuration checking is the "exceptional" case and encapsulating it in a
method unclutters launch() who's primary function is start the process
for live operation.

=> yes, it is cleaner.

I am applying these changes and giving back the ticket to you.

comment:7 Changed 3 years ago by fdupont

  • Owner changed from fdupont to tmark

Finally I kept setCheckOnly() and added a @todo saying it and setVerbose() should be found an use case for. And as the lib process unit tests failed from time to time I added an explicit initialization of check_only_ in the controller constructor.
Ready for a final review (and ChangeLog proposal?)

comment:8 Changed 3 years ago by tmark

Changes look fine. Changelog could be:

    Command line option -t support added to libprocess and  implemented
    for kea-dhcp-ddns. It allows configuration sanity checking. Note that
    some parameters, such as ip-address, port, and DNS server addresses,
    are not fully checked as sockets are not opened or connections are not
    attempted.
Last edited 3 years ago by tmark (previous) (diff)

comment:9 Changed 3 years ago by tmark

  • Owner changed from tmark to fdupont

comment:10 Changed 3 years ago by fdupont

Merged.

comment:11 Changed 3 years ago by fdupont

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

Closing. I'll port the config-get and config-write code in lib process during #5150 rebase.

Note: See TracTickets for help on using tickets.