| Message ID | 20211219140941.2279071-3-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Commit | 6e13565dd32fb2cf5517f51ca06956e2052c4bba |
| Headers | show |
| Series | ovsdb: Bug fixes + Relay txn history and performance. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On Sun, 2021-12-19 at 15:09 +0100, Ilya Maximets wrote: > If a single transaction exceeds the size of the whole database (e.g., > a lot of rows got removed and new ones added), transaction history > will > be drained. This leads to sending UUID_ZERO to the clients as the > last > transaction id in the next monitor update, because monitor doesn't > know what was the actual last transaction id. In case of a re- > connect > that will cause re-downloading of the whole database, since the > client's last_id will be out of sync. > > One solution would be to store the last transaction ID separately > from the actual transactions, but that will require a careful > management in cases where database gets reset and the history needs > to be cleared. Keeping the one last transaction instead to avoid > the problem. That should not be a big concern in terms of memory > consumption, because this last transaction will be removed from the > history once the next transaction appeared. This is also not a > concern > for a fast re-sync, because this last transaction will not be used > for the monitor reply; it's either client already has it, so no need > to send, or it's a history miss. > > The test updated to not check the number of atoms if there is only > one transaction in the history. > > Fixes: 317b1bfd7dd3 ("ovsdb: Don't let transaction history grow > larger than the database.") > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > ovsdb/transaction.c | 6 ++++-- > tests/ovsdb-server.at | 16 +++++++++------- > 2 files changed, 13 insertions(+), 9 deletions(-) > Acked-by: Mike Pattrick <mkp@redhat.com> > diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c > index 88e052800..db86d847c 100644 > --- a/ovsdb/transaction.c > +++ b/ovsdb/transaction.c > @@ -1595,8 +1595,10 @@ ovsdb_txn_history_run(struct ovsdb *db) > /* Remove old histories to limit the size of the > history. Removing until > * the number of ovsdb atoms in history becomes less than the > number of > * atoms in the database, because it will be faster to just get > a database > - * snapshot than re-constructing changes from the history that > big. */ > - while (db->n_txn_history && > + * snapshot than re-constructing changes from the history that > big. > + * Keeping at least one transaction to avoid sending UUID_ZERO > as a last id > + * if all entries got removed due to the size limit. */ > + while (db->n_txn_history > 1 && > (db->n_txn_history > 100 || > db->n_txn_history_atoms > db->n_atoms)) { > struct ovsdb_txn_history_node *txn_h_node = CONTAINER_OF( > diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at > index 2b742f78b..876cb836c 100644 > --- a/tests/ovsdb-server.at > +++ b/tests/ovsdb-server.at > @@ -1250,10 +1250,11 @@ dnl the total database size, so the > transaction history will not be able > dnl to hold all of them. > dnl > dnl The test verifies that the number of atoms in the transaction > history > -dnl is always less than the number of atoms in the database. > -get_n_atoms () { > +dnl is always less than the number of atoms in the database, except > for > +dnl a case where there is only one transaction in a history. > +get_memory_value () { > n=$(ovs-appctl -t ovsdb-server memory/show dnl > - | tr ' ' '\n' | grep atoms | grep "^$1:" | cut -d ':' -f > 2) > + | tr ' ' '\n' | grep "^$1:" | cut -d ':' -f 2) > if test X"$n" == "X"; then > n=0 > fi > @@ -1261,8 +1262,9 @@ get_n_atoms () { > } > > check_atoms () { > - n_db_atoms=$(get_n_atoms atoms) > - n_txn_history_atoms=$(get_n_atoms txn-history-atoms) > + if test $(get_memory_value txn-history) -eq 1; then return; fi > + n_db_atoms=$(get_memory_value atoms) > + n_txn_history_atoms=$(get_memory_value txn-history-atoms) > echo "n_db_atoms: $n_db_atoms" > echo "n_txn_history_atoms: $n_txn_history_atoms" > AT_CHECK([test $n_txn_history_atoms -le $n_db_atoms]) > @@ -1274,7 +1276,7 @@ add_ports () { > done > } > > -initial_db_atoms=$(get_n_atoms atoms) > +initial_db_atoms=$(get_memory_value atoms) > > for i in $(seq 1 100); do > cmd=$(add_ports $i $(($i / 4 + 1))) > @@ -1289,7 +1291,7 @@ done > > dnl After removing all the bridges, the number of atoms in the > database > dnl should return to its initial value. > -AT_CHECK([test $(get_n_atoms atoms) -eq $initial_db_atoms]) > +AT_CHECK([test $(get_memory_value atoms) -eq $initial_db_atoms]) > > OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > AT_CLEANUP
On Sun, Dec 19, 2021 at 6:09 AM Ilya Maximets <i.maximets@ovn.org> wrote: > > If a single transaction exceeds the size of the whole database (e.g., > a lot of rows got removed and new ones added), transaction history will > be drained. This leads to sending UUID_ZERO to the clients as the last > transaction id in the next monitor update, because monitor doesn't > know what was the actual last transaction id. In case of a re-connect > that will cause re-downloading of the whole database, since the > client's last_id will be out of sync. > > One solution would be to store the last transaction ID separately > from the actual transactions, but that will require a careful > management in cases where database gets reset and the history needs > to be cleared. Keeping the one last transaction instead to avoid > the problem. That should not be a big concern in terms of memory > consumption, because this last transaction will be removed from the > history once the next transaction appeared. This is also not a concern > for a fast re-sync, because this last transaction will not be used > for the monitor reply; it's either client already has it, so no need > to send, or it's a history miss. > > The test updated to not check the number of atoms if there is only > one transaction in the history. > > Fixes: 317b1bfd7dd3 ("ovsdb: Don't let transaction history grow larger than the database.") > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > ovsdb/transaction.c | 6 ++++-- > tests/ovsdb-server.at | 16 +++++++++------- > 2 files changed, 13 insertions(+), 9 deletions(-) > > diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c > index 88e052800..db86d847c 100644 > --- a/ovsdb/transaction.c > +++ b/ovsdb/transaction.c > @@ -1595,8 +1595,10 @@ ovsdb_txn_history_run(struct ovsdb *db) > /* Remove old histories to limit the size of the history. Removing until > * the number of ovsdb atoms in history becomes less than the number of > * atoms in the database, because it will be faster to just get a database > - * snapshot than re-constructing changes from the history that big. */ > - while (db->n_txn_history && > + * snapshot than re-constructing changes from the history that big. > + * Keeping at least one transaction to avoid sending UUID_ZERO as a last id > + * if all entries got removed due to the size limit. */ > + while (db->n_txn_history > 1 && > (db->n_txn_history > 100 || > db->n_txn_history_atoms > db->n_atoms)) { > struct ovsdb_txn_history_node *txn_h_node = CONTAINER_OF( > diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at > index 2b742f78b..876cb836c 100644 > --- a/tests/ovsdb-server.at > +++ b/tests/ovsdb-server.at > @@ -1250,10 +1250,11 @@ dnl the total database size, so the transaction history will not be able > dnl to hold all of them. > dnl > dnl The test verifies that the number of atoms in the transaction history > -dnl is always less than the number of atoms in the database. > -get_n_atoms () { > +dnl is always less than the number of atoms in the database, except for > +dnl a case where there is only one transaction in a history. > +get_memory_value () { > n=$(ovs-appctl -t ovsdb-server memory/show dnl > - | tr ' ' '\n' | grep atoms | grep "^$1:" | cut -d ':' -f 2) > + | tr ' ' '\n' | grep "^$1:" | cut -d ':' -f 2) > if test X"$n" == "X"; then > n=0 > fi > @@ -1261,8 +1262,9 @@ get_n_atoms () { > } > > check_atoms () { > - n_db_atoms=$(get_n_atoms atoms) > - n_txn_history_atoms=$(get_n_atoms txn-history-atoms) > + if test $(get_memory_value txn-history) -eq 1; then return; fi > + n_db_atoms=$(get_memory_value atoms) > + n_txn_history_atoms=$(get_memory_value txn-history-atoms) > echo "n_db_atoms: $n_db_atoms" > echo "n_txn_history_atoms: $n_txn_history_atoms" > AT_CHECK([test $n_txn_history_atoms -le $n_db_atoms]) > @@ -1274,7 +1276,7 @@ add_ports () { > done > } > > -initial_db_atoms=$(get_n_atoms atoms) > +initial_db_atoms=$(get_memory_value atoms) > > for i in $(seq 1 100); do > cmd=$(add_ports $i $(($i / 4 + 1))) > @@ -1289,7 +1291,7 @@ done > > dnl After removing all the bridges, the number of atoms in the database > dnl should return to its initial value. > -AT_CHECK([test $(get_n_atoms atoms) -eq $initial_db_atoms]) > +AT_CHECK([test $(get_memory_value atoms) -eq $initial_db_atoms]) > > OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > AT_CLEANUP > -- > 2.31.1 > Acked-by: Han Zhou <hzhou@ovn.org>
On 1/25/22 02:34, Han Zhou wrote: > > > On Sun, Dec 19, 2021 at 6:09 AM Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org>> wrote: >> >> If a single transaction exceeds the size of the whole database (e.g., >> a lot of rows got removed and new ones added), transaction history will >> be drained. This leads to sending UUID_ZERO to the clients as the last >> transaction id in the next monitor update, because monitor doesn't >> know what was the actual last transaction id. In case of a re-connect >> that will cause re-downloading of the whole database, since the >> client's last_id will be out of sync. >> >> One solution would be to store the last transaction ID separately >> from the actual transactions, but that will require a careful >> management in cases where database gets reset and the history needs >> to be cleared. Keeping the one last transaction instead to avoid >> the problem. That should not be a big concern in terms of memory >> consumption, because this last transaction will be removed from the >> history once the next transaction appeared. This is also not a concern >> for a fast re-sync, because this last transaction will not be used >> for the monitor reply; it's either client already has it, so no need >> to send, or it's a history miss. >> >> The test updated to not check the number of atoms if there is only >> one transaction in the history. >> >> Fixes: 317b1bfd7dd3 ("ovsdb: Don't let transaction history grow larger than the database.") >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org>> >> --- >> ovsdb/transaction.c | 6 ++++-- >> tests/ovsdb-server.at <http://ovsdb-server.at> | 16 +++++++++------- >> 2 files changed, 13 insertions(+), 9 deletions(-) >> >> > > Acked-by: Han Zhou <hzhou@ovn.org <mailto:hzhou@ovn.org>> Thanks, Han and Mike! Applied and backported to branch-2.17. Bets regards, Ilya Maximets.
diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c index 88e052800..db86d847c 100644 --- a/ovsdb/transaction.c +++ b/ovsdb/transaction.c @@ -1595,8 +1595,10 @@ ovsdb_txn_history_run(struct ovsdb *db) /* Remove old histories to limit the size of the history. Removing until * the number of ovsdb atoms in history becomes less than the number of * atoms in the database, because it will be faster to just get a database - * snapshot than re-constructing changes from the history that big. */ - while (db->n_txn_history && + * snapshot than re-constructing changes from the history that big. + * Keeping at least one transaction to avoid sending UUID_ZERO as a last id + * if all entries got removed due to the size limit. */ + while (db->n_txn_history > 1 && (db->n_txn_history > 100 || db->n_txn_history_atoms > db->n_atoms)) { struct ovsdb_txn_history_node *txn_h_node = CONTAINER_OF( diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at index 2b742f78b..876cb836c 100644 --- a/tests/ovsdb-server.at +++ b/tests/ovsdb-server.at @@ -1250,10 +1250,11 @@ dnl the total database size, so the transaction history will not be able dnl to hold all of them. dnl dnl The test verifies that the number of atoms in the transaction history -dnl is always less than the number of atoms in the database. -get_n_atoms () { +dnl is always less than the number of atoms in the database, except for +dnl a case where there is only one transaction in a history. +get_memory_value () { n=$(ovs-appctl -t ovsdb-server memory/show dnl - | tr ' ' '\n' | grep atoms | grep "^$1:" | cut -d ':' -f 2) + | tr ' ' '\n' | grep "^$1:" | cut -d ':' -f 2) if test X"$n" == "X"; then n=0 fi @@ -1261,8 +1262,9 @@ get_n_atoms () { } check_atoms () { - n_db_atoms=$(get_n_atoms atoms) - n_txn_history_atoms=$(get_n_atoms txn-history-atoms) + if test $(get_memory_value txn-history) -eq 1; then return; fi + n_db_atoms=$(get_memory_value atoms) + n_txn_history_atoms=$(get_memory_value txn-history-atoms) echo "n_db_atoms: $n_db_atoms" echo "n_txn_history_atoms: $n_txn_history_atoms" AT_CHECK([test $n_txn_history_atoms -le $n_db_atoms]) @@ -1274,7 +1276,7 @@ add_ports () { done } -initial_db_atoms=$(get_n_atoms atoms) +initial_db_atoms=$(get_memory_value atoms) for i in $(seq 1 100); do cmd=$(add_ports $i $(($i / 4 + 1))) @@ -1289,7 +1291,7 @@ done dnl After removing all the bridges, the number of atoms in the database dnl should return to its initial value. -AT_CHECK([test $(get_n_atoms atoms) -eq $initial_db_atoms]) +AT_CHECK([test $(get_memory_value atoms) -eq $initial_db_atoms]) OVS_APP_EXIT_AND_WAIT([ovsdb-server]) AT_CLEANUP
If a single transaction exceeds the size of the whole database (e.g., a lot of rows got removed and new ones added), transaction history will be drained. This leads to sending UUID_ZERO to the clients as the last transaction id in the next monitor update, because monitor doesn't know what was the actual last transaction id. In case of a re-connect that will cause re-downloading of the whole database, since the client's last_id will be out of sync. One solution would be to store the last transaction ID separately from the actual transactions, but that will require a careful management in cases where database gets reset and the history needs to be cleared. Keeping the one last transaction instead to avoid the problem. That should not be a big concern in terms of memory consumption, because this last transaction will be removed from the history once the next transaction appeared. This is also not a concern for a fast re-sync, because this last transaction will not be used for the monitor reply; it's either client already has it, so no need to send, or it's a history miss. The test updated to not check the number of atoms if there is only one transaction in the history. Fixes: 317b1bfd7dd3 ("ovsdb: Don't let transaction history grow larger than the database.") Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- ovsdb/transaction.c | 6 ++++-- tests/ovsdb-server.at | 16 +++++++++------- 2 files changed, 13 insertions(+), 9 deletions(-)