On 8/13/24 06:03, 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 be unopenable
> by ovsdb.
>
> 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
> such
> ---
> tests/ovsdb-server.at | 59 +++++++++++++++++++++++++++++++++++++++++++
> utilities/ovs-lib.in | 35 ++++++++++++++++++++-----
> 2 files changed, 88 insertions(+), 6 deletions(-)
>
> diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at
> index ce6d32aee..18ede1cf9 100644
> --- a/tests/ovsdb-server.at
> +++ b/tests/ovsdb-server.at
> @@ -105,6 +105,65 @@ AT_CHECK([uuidfilt output], [0],
> ]], [])
> AT_CLEANUP
>
> +AT_SETUP([truncated database recreated])
Maybe "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.
> +_PATH=$PATH
> +. ovs-lib
> +PATH=$_PATH
This seems a little dangerous, though I'm not sure what's a better solution.
Do you know where exactly the PATH is getting reset?
> +
> +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
> previous test but
> +dnl includes a mid-operation upgrade.
This is already a multi-line comment. Might as well limit the line length.
And double spaces between sentences.
> +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 'syntax error: db: parse error.* in header line "xxx"'
> stderr], [0], [ignore])
With 'grep -q' the ', [0], [ignore]' will not be needed.
> +cat stdout >> output
Should this be > instead of >> ?
> +AT_CHECK([uuidfilt output], [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], [0])
'[0]' is not needed.
> +
> +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], [0])
Ditto.
> +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 7812a94ee..6128f0cae 100644
> --- a/utilities/ovs-lib.in
> +++ b/utilities/ovs-lib.in
> @@ -441,18 +441,41 @@ 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
> + 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.
> + DB_FILE="$1"
> +
> + if test ! -e "$DB_FILE"; then
> + return 1
> + elif ! ovsdb_tool db-name "$DB_FILE" >/dev/null 2>&1; then
> + backup_db "$DB_FILE"
> + rm -f "$DB_FILE"
> + return 1
> + fi
> +
> + return 0
> +}
> +
> upgrade_db () {
> DB_FILE="$1"
> DB_SCHEMA="$2"
>
> schemaver=`ovsdb_tool schema-version "$DB_SCHEMA"`
> - if test ! -e "$DB_FILE"; then
> + if ! validate_db "$DB_FILE"; then
IMO, such a check complicates the understanding, especially because
validation has side effects.
Maybe it'll be be better to just call 'validate_db' in the beginning
of the function and then keep all the other checks as-is?
The same for the cluster functions below.
In that case it might be good to print something like "XX is not a
valid database file" from the validate_db as well before backing up.
WDYT?
Best regards, Ilya Maximets.
> log_warning_msg "$DB_FILE does not exist"
> install_dir `dirname $DB_FILE`
> create_db "$DB_FILE" "$DB_SCHEMA"
> @@ -513,7 +536,7 @@ create_cluster () {
> election_timer_arg="--election-timer=$ELECTION_TIMER_MS"
> fi
>
> - if test ! -e "$DB_FILE"; then
> + if ! validate_db "$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
> # Convert standalone database to clustered.
> @@ -530,7 +553,7 @@ join_cluster() {
> LOCAL_ADDR="$3"
> REMOTE_ADDR="$4"
>
> - if test -e "$DB_FILE" && ovsdb_tool db-is-standalone "$DB_FILE"; then
> + if validate_db "$DB_FILE" && ovsdb_tool db-is-standalone "$DB_FILE"; then
> backup_db || return 1
> rm $DB_FILE
> fi
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
