Opened 2 years ago

Closed 22 months ago

#5454 closed task (complete)

HA: implement ha configuration parsing in the hooks library

Reported by: tomek Owned by: marcin
Priority: medium Milestone: Kea1.4
Component: high-availability 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 (last modified by tomek)

Once #5447 is done, we need to extend the library with the ability to parse HA configuration.

For details see HADesign.

Subtickets

Change History (9)

comment:1 Changed 2 years ago by tomek

  • Description modified (diff)

comment:2 Changed 23 months ago by marcin

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

comment:3 Changed 22 months ago by marcin

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

I have implemented configuration parsing in libkea-ha. It only accepts simple configurations which specify HA mode, server names, URLs etc. It doesn't provide configuration for heartbeat interval and other parameters related to failure detection as it is planned for another time/milestone.

The new code is in the premium repo, branch trac5454. It must be compiled together with the Kea core, branch trac5451 as this branch contains the definition of the http::Url class.

Proposed ChangeLog entry:

2X.	[func]		marcin
	Implemented configuration parsing for libkea-ha hooks library.
	(Trac #5454, git cafe)

comment:4 Changed 22 months ago by tmark

  • Owner changed from UnAssigned to tmark

comment:5 follow-up: Changed 22 months ago by tmark

  • Owner changed from tmark to marcin

Overall it looks good.

I fixed a minor typo in commentary so please pull.


It can be handy to have conversions to/from string/enums,
especially if one wants to log things or decide if they're valid,
like these:

extern NameChangeFormat? stringToNcrFormat(const std::string& fmt_str);
extern std::string ncrFormatToString(NameChangeFormat? format);

We really ought to find/use a wrapper for this stuff.

I not insisting you do this, but am suggesting it.


HAConfig::validate():

Perhaps a switch on ha_mode rather than ifs or at least
an if-else-if.

Not sure how much more complicated per-mode config might get, you
could perhaps make mode validate methods.


ha_config_unittest.cc

testInvalidConfig() provides no means to verify the reason the config
failed. I suppose we don't do this in a lot areas, one possiblity would
pass in the expected error text, and try-catch() the exception.

Might be a nice to have but I'll leave that to you.


Builds and high_availability unit tests pass under Ubuntu16.04 (legal-log tests
segfault but that's an issue for Francis)

comment:6 in reply to: ↑ 5 Changed 22 months ago by marcin

Replying to tmark:

Overall it looks good.

I fixed a minor typo in commentary so please pull.

Thanks.


It can be handy to have conversions to/from string/enums,
especially if one wants to log things or decide if they're valid,
like these:

extern NameChangeFormat? stringToNcrFormat(const std::string& fmt_str);
extern std::string ncrFormatToString(NameChangeFormat? format);

We really ought to find/use a wrapper for this stuff.

I not insisting you do this, but am suggesting it.

I added conversion methods and suitable unit tests.


HAConfig::validate():

Perhaps a switch on ha_mode rather than ifs or at least
an if-else-if.

Not sure how much more complicated per-mode config might get, you
could perhaps make mode validate methods.

I decided to leave it as is.


ha_config_unittest.cc

testInvalidConfig() provides no means to verify the reason the config
failed. I suppose we don't do this in a lot areas, one possiblity would
pass in the expected error text, and try-catch() the exception.

Might be a nice to have but I'll leave that to you.

The test now verifies error messages. It actually captured a couple issues with tests/code.


Builds and high_availability unit tests pass under Ubuntu16.04 (legal-log tests
segfault but that's an issue for Francis)

comment:7 Changed 22 months ago by marcin

  • Owner changed from marcin to tmark

comment:8 Changed 22 months ago by tmark

  • Owner changed from tmark to marcin

Changes look, good merge ahoy!

comment:9 Changed 22 months ago by marcin

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

Merged with commit 49b91649dbb70882a81de8f5a9f1786832695aa5

Note: See TracTickets for help on using tickets.