Opened 5 years ago

Closed 5 years ago

#3882 closed enhancement (complete)

-V (extended version) should return database details

Reported by: fdupont Owned by: fdupont
Priority: low Milestone: Kea0.9.2-beta
Component: Unclassified 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

#3513 followup with:

  • name of the backend (as in #3513)
  • version of the backend (defined in xxx_lease_mgr.h)
  • version of the library:
    • memfile: nothing
    • mysql: mysql_get_client_info()
    • postgresql: pgsql_version()

Subtickets

Change History (21)

comment:1 Changed 5 years ago by fdupont

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

comment:2 Changed 5 years ago by fdupont

Used PQlibVersion() for PostgreSQL.

comment:3 Changed 5 years ago by fdupont

Ready for review. Note it is stacked on #3859 which is itself stacked on #3513. To check it you need to configure the 3 database options too...
Another comment: the #ifdef's are required because we use derived types (vs implementation type members) and inheritance is in the wrong way for this purpose.

comment:4 Changed 5 years ago by fdupont

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

comment:5 Changed 5 years ago by fdupont

Merged changed from #3513 review.
Handle multiple databases.
Now the -V reports something like:

0.9.1-git
tarball
linked with:
log4plus 1.1.2
OpenSSL 0.9.8zd 8 Jan 2015
database:
MySQL backend 2.0 library 10.0.19-MariaDB
PostgreSQL backend 1.0 library 90403
Memfile backend 2.0

cut & paste from my iMac...

comment:6 Changed 5 years ago by fdupont

A question for the reviewer: should I add the database stuff for kea-lfc? According to otool -L (the OS X ldd) it is linked with database libraries too...

comment:7 Changed 5 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea0.9.2

comment:8 Changed 5 years ago by fdupont

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

comment:9 Changed 5 years ago by fdupont

I am rebasing it on the master as #3513 and #3859 were merged.

comment:10 Changed 5 years ago by fdupont

Rebased branch trac3882a ready for review.

comment:11 Changed 5 years ago by fdupont

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

comment:12 Changed 5 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:13 follow-up: Changed 5 years ago by stephen

  • Owner changed from stephen to fdupont

Reviewed commit 530c329c882bed38549c5c7b85b715b46b945ff7

src/bin/d2/d_controller.cc
src/bin/dhcp4/dhcp4_srv.cc
src/bin/dhcp6/dhcp6_srv.cc
I suggest that the code that retrieves the extended version be placed into a separate stand-alone function located in dhcpsrv. As it is, the code is repeated in each of these files. (Although in dhcp6_srv.cc, the line outputting EXTENDED_VERSION appears twice.)

src/lib/dhcpsrv/mysql_lease_mgr.cc
src/lib/dhcpsrv/pgsql_lease_mgr.cc
getDBVersion(): As the backend and library are two separate items, I suggest you separate them by a comma (i.e. s/" library/", library/)

ChangeLog
This will need a ChangeLog entry. I suggest:

Include database software details in extended version information. 

(The code only returns details about the libraries and backends, not about the version of the schema in the database.)


A question for the reviewer: should I add the database stuff for kea-lfc?

I think so, although it is only ever linked against the memfile library. Just call the same extended version standalone function (see above) used by the other code.

comment:14 in reply to: ↑ 13 Changed 5 years ago by fdupont

Replying to stephen:

Reviewed commit 530c329c882bed38549c5c7b85b715b46b945ff7

src/bin/d2/d_controller.cc
src/bin/dhcp4/dhcp4_srv.cc
src/bin/dhcp6/dhcp6_srv.cc
I suggest that the code that retrieves the extended version be placed into a separate stand-alone function located in dhcpsrv. As it is, the code is repeated in each of these files.

=> I am not sure it is a good idea because it removes flexibility in what and where the code is bound. And BTW it was not asked for the crypto backends (aka #3618).

(Although in dhcp6_srv.cc, the line outputting EXTENDED_VERSION appears twice.)

=> argh! I fixed already once this... it seems it is still there.

src/lib/dhcpsrv/mysql_lease_mgr.cc
src/lib/dhcpsrv/pgsql_lease_mgr.cc
getDBVersion(): As the backend and library are two separate items, I suggest you separate them by a comma (i.e. s/" library/", library/)

=> I'll do it.

ChangeLog
This will need a ChangeLog entry. I suggest:

Include database software details in extended version information. 

(The code only returns details about the libraries and backends, not about the version of the schema in the database.)

=> I thought about the schema version but it can't be done without connecting first to the database so it is a good idea but not to do there (I expect the schema version is logged as soon as it is available. If it is not the case it should be added (i.e., ticket, etc)).

A question for the reviewer: should I add the database stuff for kea-lfc?

I think so, although it is only ever linked against the memfile library.

=> I'll add this.

comment:15 Changed 5 years ago by fdupont

BTW did you review trac3882a because the double EXTENDED_VERSION was fixed in the rebasing?

comment:16 Changed 5 years ago by fdupont

Addressed comments in trac3882a.

comment:17 Changed 5 years ago by fdupont

  • Owner changed from fdupont to UnAssigned

comment:18 Changed 5 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:19 Changed 5 years ago by stephen

  • Owner changed from stephen to fdupont

Reviewed commit c6727e5b1dd02595634a7116d222f6832fbf8fa0

I thought about the schema version but it can't be done without connecting first to the database so it is a good idea but not to do there (I expect the schema version is logged as soon as it is available. If it is not the case it should be added (i.e., ticket, etc)).

You are right: we don't want to connect to the database when printing out version details.

As to the logging of the database, I don't think the schema version (store din the database) is logged at the moment; I believe that the version information in the database is ignored. However, once 1.0 is released, schema version information becomes important and the servers should check whether they are running against a compatible schema. In this case, I'm guessing that an incompatible schema will cause an error to be logged.

BTW did you review trac3882a

My original review was of trac3882. In this review, I reviewed trac3882a.

All OK, please merge.

comment:20 Changed 5 years ago by fdupont

Merged. Closing...
About the schema version: it will become critical at the first version change.

comment:21 Changed 5 years ago by fdupont

  • Resolution set to complete
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.