Opened 5 years ago

Closed 3 years ago

#3590 closed enhancement (complete)

Migrate remaining items from CfgMgr to Configuration class

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

Description

SSIA. That's a follow up to #3534.

Subtickets

Change History (24)

comment:1 Changed 5 years ago by marcin

During implementation of the #3630 I realized that when the lease database backend is not specified in the configuration file the server will start up just fine with no warning. The error will be emitted when the server starts to receive messages from the clients, and will complain that the lease manager is not initialized. The lease database backend is a mandatory parameter and the server should fail configuration when it is not specified. Alternatively, the default lease database backend should be created.

This ticket should account for this.

comment:2 Changed 5 years ago by hschempf

  • Milestone changed from Kea0.9.1 to Kea0.9.2

comment:3 Changed 5 years ago by hschempf

  • Milestone changed from Kea0.9.2 to Kea1.1

comment:4 Changed 4 years ago by tomek

  • Milestone changed from Kea1.1 to DHCP Outstanding Tasks

comment:5 Changed 4 years ago by tomek

  • Milestone changed from DHCP Outstanding Tasks to Outstanding Tasks

Milestone renamed

comment:6 Changed 3 years ago by tomek

  • Milestone changed from Outstanding Tasks to Kea1.2

comment:7 Changed 3 years ago by tomek

  • Component changed from dhcpconf to configuration

comment:8 Changed 3 years ago by tomek

  • Component changed from configuration to remote-management

comment:9 Changed 3 years ago by fdupont

Title and Marcin's comment:1 don't match. IMHO there are 2 different problems:

  • make lease database either with a default or mandatory (in the second case update #5124).
  • migrate things from configuration manager to server configuration (and there is a short list of such things).

comment:10 Changed 3 years ago by tomek

  • Priority changed from medium to low

comment:11 Changed 3 years ago by tomek

  • Priority changed from low to medium

comment:12 Changed 3 years ago by fdupont

  • Parent Tickets set to 5114

Splitting the ticket. Keep the second point to this ticket, the first part goes into #5144.

comment:13 Changed 3 years ago by fdupont

Maling a specific case for the hooks libraries stuff which has no Cfg structure (state is in the parser only): #5145

comment:14 Changed 3 years ago by fdupont

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

comment:15 Changed 3 years ago by fdupont

  • Summary changed from Migrate lease backend info from CfgMgr to Configuration class to Migrate remaining items from CfgMgr to Configuration class

comment:16 Changed 3 years ago by fdupont

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

Done:

  • removed @todo about parameter inheritance (it is possible there are still some obsolete comments)
  • removed unused CfgMgr::isDuplicate() (isDuplicate() is in cfg_subnets files)
  • moved echo client-id set/get from CfgMgr to SrvConfig, renamed them to set and get, and moved the unit test
  • updated dhcp4 echo client-id unit test using staging server config

Ready for review.

comment:17 Changed 3 years ago by tomek

  • Sub-Project changed from DHCP to Mozilla

comment:18 Changed 3 years ago by tomek

  • Owner changed from UnAssigned to tomek

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

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

Your changes are good, but they do not cover everything.

I have removed some obsolete code from CfgMgr?. Also, added hooks
libraries information to SrvConfig?. I don't think unit-tests for hook
libraries are necessary at this stage. The behaviour of hooks parser is
already tested and we will be able to check if hooks information is
retrieved properly in #5114.

Please pull and review. If you're ok with the changes, then there's
one more thing to move. cfgmgr has ddnsEnabled, setD2ClientConfig,
getD2ClientConfig and getD2ClientMgr(). Those all belong to SrvConfig?
in my opinion.

comment:20 Changed 3 years ago by tomek

After discussing with Thomas, it seems that "each kea-dhcpX configuration doesn't get its own D2ClientMgr, it is expected to have one that is given a new config to apply (or attempt to apply) the ClientMgr? holds the currently open D2 connection/queue and knows how/when to close/re-open when given a new configuration ... OR restore if the new config doesn't work".

As such, it's ok to keep D2ClientMgr in CfgMgr?. So there's nothing to do here, except review the changes I pushed. Once that is done, this can be merged and the ticket closed.

comment:21 in reply to: ↑ 19 Changed 3 years ago by fdupont

  • Owner changed from fdupont to tomek

Replying to tomek:

Your changes are good, but they do not cover everything.

=> there was not the goal.

I have removed some obsolete code from CfgMgr?.

=> it is always a good thing to remove code. My concern is more about
what was added.

Also, added hooks
libraries information to SrvConfig?. I don't think unit-tests for hook
libraries are necessary at this stage. The behaviour of hooks parser is
already tested and we will be able to check if hooks information is
retrieved properly in #5114.

=> If you read the history of this ticket I split it into #5145 which
exactly fully addressed this issue. So please remove your new code,
merge trac5145 into trac3590, review the result, update the description
of this ticket and close #5145 as duplicate.

Please pull and review. If you're ok with the changes, then there's
one more thing to move. cfgmgr has ddnsEnabled, setD2ClientConfig,
getD2ClientConfig and getD2ClientMgr(). Those all belong to SrvConfig?
in my opinion.

=> I consider to do this but the first step should be to rename the
config manager setD2ClientConfig which does something very different
than its homonym. (PS: this applies even if we keep D2ClientMgr in
CfgMgr? but I am not sure this is in the scope of this ticket).

comment:22 Changed 3 years ago by tomek

  • Owner changed from tomek to fdupont

Initiall I have cherry-picked all changes from #5145 into trac3590 (merging
would introduce much more changes as 5145 and 3590 branched off
from master from different points), but then I removed them
from this branch. See explanation below.

The changes on #5145 are good in general, but I have two
observations. First is that we have a whole new class for something
that could have been achieved with existing HookLibsCollection?. Your
code is cleaner, but it makes Kea code bigger. One day we will be
faced with the problem of running Kea in embedded environment and with
every class adding extra size, it will be an impossible task to make
Kea smaller. This is a minor issue, so let's not spend too much time
on this.

Second issue is more substantial. The class you proposed modified
hooks parser. You may not be aware, but that class is being used
in CA and is moved to src/lib/hooks as part of #5134. That's a
critical work that is required for Marcin to continue with his
Kittiwake efforts. The code on #5134 relies on
HooksLibrariesParser::loadLibraries() to load libraries in agent
code. We cannot remove that method at this time.

As such, here's what I propose:

  1. merge 5134 first
  2. revert my change regarding hooks on 3590
  3. merge 3590 (without any hook changes)
  4. move 5145 to Kea1.2, rebase it after #1 and #3 are merged, update 5145 as needed, review and merge

If you agree with this proposal, then:

  • please pull 3590 (the only change is removal of earlier hook commit)
  • you merge 3590
  • let Marcin review 5134
  • I'll move 5145 to kea1.2
  • once I merge 5134, you can rebase 5145

comment:23 Changed 3 years ago by fdupont

  • Owner changed from fdupont to tomek

I pulled trac3590, checked the last commit reverted the previous one, merged, moved loadlibraries() at the end of blocks and pushed.
So now we can close the ticket and go to the next step of the plan, can't we?

comment:24 Changed 3 years ago by tomek

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

Thanks for merging this. Ok, moving ahead with the plan.
This ticket is now merged, closing. We're waiting for Marcin to complete #5134 review (Tomek will address comments). Once merged, Francis will rebase #5145, Tomek will review.

Note: See TracTickets for help on using tickets.