On 1/7/25 15:49, Mike Pattrick wrote:
> Previously ovs-lib would assume a database is valid if the file exists,
> however, it is possible for the database file to exist but not be valid
> for ovsdb to open it.
>
> Now existence checks are augmented with schema checksum validation.
> Databases with an invalid schema are removed.
>
> Reported-at: https://issues.redhat.com/browse/FDP-689
> Reported-by: Ihar Hrachyshka
> Signed-off-by: Mike Pattrick <[email protected]>
>
> ---
> v2:
> - Back up database before deleting it
> - Use the db-name command to check for validity of file
> - Added test to verify that valid clustered databases are detected as
> v3:
> - Removed sourcing of lsb functions
> - Corrected comment formatting.
> - Restored test instead of valdidate_db check
>
> ---
> tests/ovsdb-server.at | 56 +++++++++++++++++++++++++++++++++++++++++++
> utilities/ovs-lib.in | 32 ++++++++++++++++++++++---
> 2 files changed, 85 insertions(+), 3 deletions(-)
>
> diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at
> index b71947511..69f3d41fc 100644
> --- a/tests/ovsdb-server.at
> +++ b/tests/ovsdb-server.at
> @@ -105,6 +105,62 @@ AT_CHECK([uuidfilt output], [0],
> ]], [])
> AT_CLEANUP
>
> +AT_SETUP([database without valid schema is recreated])
> +AT_KEYWORDS([ovsdb unix])
> +AT_SKIP_IF([test "$IS_WIN32" = "yes"])
> +ordinal_schema > schema
> +
> +dnl Loading ovs-lib resets our PATH, save a copy and then restore.
This comment is outdated.
> +. ovs-lib
> +
> +dnl Check that DB is recreated when schema corrupted.
> +echo 'x' > db
> +AT_CHECK([upgrade_db db schema], [0], [stdout], [ignore])
> +AT_CHECK([grep -c "db does not exist\|Creating empty database db" stdout],
> [0], [2
> +])
> +AT_CHECK([validate_db db], [0])
> +
> +AT_DATA([txnfile], [[ovsdb-client transact unix:socket \
> +'["ordinals",
> + {"op": "insert",
> + "table": "ordinals",
> + "row": {"number": 1, "name": "one"}}]'
> +]])
> +AT_CHECK([ovsdb-server --remote=punix:socket db --run="sh txnfile"], [0],
> [stdout], [stderr])
> +
> +dnl Check that DB is not recreated on corrupted log. This is similar to the
> +dnl previous test but includes a mid-operation upgrade.
> +echo 'xxx' >> db
> +AT_CHECK([upgrade_db db schema], [0], [], [ignore])
> +
> +dnl Validate that the db can now be used.
> +AT_DATA([txnfile], [[ovsdb-client transact unix:socket \
> +'["ordinals",
> + {"op": "select",
> + "table": "ordinals",
> + "where": []}]'
> +]])
> +AT_CHECK([ovsdb-server --remote=punix:socket db --run="sh txnfile"], [0],
> [stdout], [stderr])
> +AT_CHECK([grep -q 'syntax error: db: parse error.* in header line "xxx"'
> stderr])
> +AT_CHECK([uuidfilt stdout], [0],
> +
> [[[{"rows":[{"_uuid":["uuid","<0>"],"_version":["uuid","<1>"],"name":"one","number":1}]}]
> +]], [])
> +
> +dnl Validate then create and join cluster.
> +echo 'x' > db
> +AT_CHECK([create_cluster db schema tcp:1.1.1.1:1111 1000], [0], [stdout], [])
> +AT_CHECK([grep -Ec 'Backing up database|Creating cluster database db'
> stdout], [0], [2
> +])
> +AT_CHECK([validate_db db])
> +
> +dnl Join a cluster with a corrupted db.
> +echo 'x' > db
> +AT_CHECK([join_cluster db schema tcp:1.1.1.1:1111 tcp:2.2.2.2:2222], [0],
> [stdout], [])
> +AT_CHECK([grep -Ec 'Backing up database|Joining db to cluster' stdout], [0],
> [2
> +])
> +AT_CHECK([validate_db db])
We're missing the 'Check that DB is not recreated on corrupted log' test for
the clustered database. My concern is that 'db-name' fully loads the clustered
database.
> +AT_CLEANUP
> +
> AT_SETUP([truncating database log with bad transaction])
> AT_KEYWORDS([ovsdb server positive unix])
> AT_SKIP_IF([test "$IS_WIN32" = "yes"])
> diff --git a/utilities/ovs-lib.in b/utilities/ovs-lib.in
> index 582fdd0df..74805e47e 100644
> --- a/utilities/ovs-lib.in
> +++ b/utilities/ovs-lib.in
> @@ -428,16 +428,40 @@ create_db () {
>
> backup_db () {
> # Back up the old version.
> - version=`ovsdb_tool db-version "$DB_FILE"`
> - cksum=`ovsdb_tool db-cksum "$DB_FILE" | awk '{print $1}'`
> - backup=$DB_FILE.backup$version-$cksum
> + if test ! -e "$DB_FILE"; then
> + return 0
> + elif ovsdb_tool db-is-standalone "$DB_FILE" 2>/dev/null; then
> + version=`ovsdb_tool db-version "$DB_FILE"`
> + cksum=`ovsdb_tool db-cksum "$DB_FILE" | awk '{print $1}'`
> + backup=$DB_FILE.backup$version-$cksum
> + else
> + # Support for clsutered databases.
*clustered
> + backup=`mktemp -q $DB_FILE.XXXXXXXXXX`
> + fi
> action "Backing up database to $backup" cp "$DB_FILE" "$backup" ||
> return 1
> }
>
> +validate_db () {
> + # Returns 0 if $DB_FILE is present and at least has a db name.
> + # Returns 1 if $DB_FILE is not present.
This should say 'Returns 1 otherwise', otherwise it's an incomplete
description.
> + DB_FILE="$1"
> +
> + if test ! -e "$DB_FILE"; then
> + return 1
> + elif ! ovsdb_tool db-name "$DB_FILE" >/dev/null 2>&1; then
I'm a little concerned about this check, because it reads the whole
first record of the database, which is the whole database in case
of a compacted clustered database file. So, it may take a minute
for this process to complete, consuming a lot of CPU and memory.
Effectively. this will double the time it takes to start a database
server, which may be problematic at scale.
IIRC, in the original report the file was just empty. Maybe it's
better to check for that? Or maybe we can just grep for the magic
OVSDB CLUSTER | OVSDB JSON in the file and not validate the records?
WDYT?
> + backup_db "$DB_FILE"
> + action "Removing invalid database file $DB_FILE" rm -f "$DB_FILE"
> + return 1
> + fi
> +
> + return 0
> +}
> +
> upgrade_db () {
> DB_FILE="$1"
> DB_SCHEMA="$2"
>
> + validate_db "$DB_FILE"
> schemaver=`ovsdb_tool schema-version "$DB_SCHEMA"`
> if test ! -e "$DB_FILE"; then
> log_warning_msg "$DB_FILE does not exist"
> @@ -500,6 +524,7 @@ create_cluster () {
> election_timer_arg="--election-timer=$ELECTION_TIMER_MS"
> fi
>
> + validate_db "$DB_FILE"
> if test ! -e "$DB_FILE"; then
> action "Creating cluster database $DB_FILE" ovsdb_tool
> $election_timer_arg create-cluster "$DB_FILE" "$DB_SCHEMA" "$LOCAL_ADDR"
> elif ovsdb_tool db-is-standalone "$DB_FILE"; then
> @@ -517,6 +542,7 @@ join_cluster() {
> LOCAL_ADDR="$3"
> REMOTE_ADDR="$4"
>
> + validate_db "$DB_FILE"
> if test -e "$DB_FILE" && ovsdb_tool db-is-standalone "$DB_FILE"; then
> backup_db || return 1
> rm $DB_FILE
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
