ovsdb_idl_row_destroy() doesn't free the memory of row structure itself.
This is because of the ovsdb change tracking feature: the deleted row
may be accessed in the current iteration of main loop. The function
ovsdb_idl_row_destroy_postprocess() is called at the end of
ovsdb_idl_run() to free the deleted rows that are not tracked; the
function ovsdb_idl_db_track_clear() is called (indirectly) by user
at the end of each main loop iteration to free the deleted rows that
are tracked. However, in ovsdb_idl_db_clear(), which may be called when
a session is reset, or when the idl is destroyed, it didn't call
ovsdb_idl_row_destroy_postprocess(), which would result in all the
untracked rows leaked. This patch fixes that.
Signed-off-by: Han Zhou <hzhou8@ebay.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Reported by Address Sanitizer.
Fixes: 5e07b8f93f03 ("ovsdb-idl: New function ovsdb_idl_create_unconnected().")
Signed-off-by: Han Zhou <hzhou8@ebay.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Use monitor_cond_since to request changes after last version of local
data when connection to server is reset, without clearing the local
data. It falls back to clearing and repopulating all the data when
the requested id cannot be fulfilled by the server.
Test result at ovn-scale-test environment using clustered mode:
- 1K HVs (ovsdb clients)
- 10K lports
Without the patch it took 30+ min for the SB ovsdb-server to calm down
and HVs to stablize the connectin and finish syncing data.
With the patch there were no noticible CPU spike of SB ovsdb-server,
and all HVs were in sync with SB within 1 min, which is the probe
interval set in this test (so it took at most 1 min for HVs to notice
the TCP connection reset and reconnect and resync finished immediately
after that).
Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-September/047457.html
Signed-off-by: Han Zhou <hzhou8@ebay.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Use monitor_cond_since in C IDL. If it is not supported by server,
fall back to old method (monitor_cond, or monitor).
Signed-off-by: Han Zhou <hzhou8@ebay.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Ordinarily the IDL finds out in advance whether a particular database is
on its server, or it finds out via notifications. But it's also a good
idea to adopt a belt-and-suspenders approach so that, if the IDL does
receive an "unknown database" error, we treat it as a "soft" error that
can be fixed by reconnecting to another server, rather than a "hard" error
that should cause an immediate abort.
Acked-by: Mark Michelson <mmichels@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Until now the code here would happily try to send transactions to the
database server even if the database connection was not in the correct
state. In some cases this could lead to strange behavior, such as sending
a database transaction for a database that the IDL had just learned did not
exist on the server.
Acked-by: Mark Michelson <mmichels@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
This fixes the change-tracking feature. The seqno change is needed so that
the change-tracking helper function ..._is_new() can work properly.
Fixes: 102781cc02c6 ("ovsdb-idl: Track changes for table references.")
Signed-off-by: Han Zhou <hzhou8@ebay.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
This new function makes it possible to create an instance of the IDL
without connecting it to a remote OVSDB server. The caller can then
connect and disconnect using ovsdb_idl_set_remote(); the ability to
disconnect is a new feature.
With this patch, the ovsdb_idl 'session' member can be null whereas
previously it was always nonnull. The scattered changes throughout
ovsdb-idl are to cope with this new possibility.
An upcoming patch will introduce the first user of this new feature.
Signed-off-by: Ben Pfaff <blp@ovn.org>
When transactions modified tables with indexes, the indexes were not
properly updated to reflect the changes. For deleted rows, in particular,
this could cause use-after-free errors.
This commit fixes the problem and adds some simple test cases provided by
Han Zhou that, without the fix, cause a crash.
Reported-by: Han Zhou <zhouhan@gmail.com>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-August/047185.html
Signed-off-by: Ben Pfaff <blp@ovn.org>
If a change of a row is tracked, make sure the rows that reference
this row are also added in tracked changes, unless change tracking
is not required for those rows.
Signed-off-by: Han Zhou <hzhou8@ebay.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
There are places with the pattern:
if (!ovs_list_is_empty(&row->track_node)) {
ovs_list_remove(&row->track_node);
}
ovs_list_push_back(&row->table->track_list,
&row->track_node);
It seems to be trying to prevent double removing the node from a list,
but actually it doesn't, and may be misleading.
The function ovs_list_is_empty() returns true only if the node has been
initialized with ovs_list_init(). If a node is deleted but not
initialized, the check will return false and the above code will continue
deleting it again. But if a node is already initialized, calling
ovs_list_remove() is a no-op. So the check is not necessary and misleading.
In fact there is already a double removal resulted by this code: the function
ovsdb_idl_db_clear() removes the node but it then calls ovsdb_idl_row_destroy()
immediately which removes the node again. It should not result in any real
issue yet since the list was not changed so the second removal just
assigns the pointers with same value.
It is in fact not necessary to remove and then add back to the list, because
the purpose of the change tracking is just to tell if a row is changed
or not. So this patch removes the "check and remove" code before adding
the node to a list, but instead, adding it to the list only if it is
not in a list. This way it ensures the node is added to a list only once
in one idl loop. (ovsdb_idl_db_track_clear() will be called in each
iteration and the check ovs_list_is_empty() will return true at the first
check in next iteration).
Signed-off-by: Han Zhou <hzhou8@ebay.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
The comment was added when the feature was introduced but what it
described is not what is implemented, probably because of revisions
after code reviews.
Signed-off-by: Han Zhou <hzhou8@ebay.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
In current IDL index code it doesn't updated index when handling
"update2" messages, which is the default case. The consequence
is that when a row is updated, the index is not updated accordingly,
and even worse, it causes crash when calling ovsdb_idl_destroy().
It can be easily reproduced by the test cases added in this patch.
Signed-off-by: Han Zhou <hzhou8@ebay.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Add 'connection-status' command to ovs-appctl utility in order to check
if a given chassis is currently connected to SB db
Acked-by: Mark Michelson <mmichels@redhat.com>
Co-authored-by: aginwala <aginwala@ebay.com>
Signed-off-by: aginwala <aginwala@ebay.com>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
If IDL was created with monitoring and alerts turned on by default for
all columns, then there is no harm in allowing the API users to ask
again for monitoring and alerts to be enabled for any given column.
This allows us to run prerequisites handlers for db-ctl and ovn-nbctl
commands once the IDL has already ran once.
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
In ovsdb_idl_db_track_clear(), it needs to free the deleted row.
However, it unnecessary to call ovsdb_idl_row_clear_old(), because
this has been called in ovsdb_idl_row_destroy(). It is also confusing
because it is called only if:
if (ovsdb_idl_row_is_orphan(row))
This is contradict with the check in ovsdb_idl_row_clear_old():
if (!ovsdb_idl_row_is_orphan(row))
(Currently the tracked row doesn't maintain any data, so there is no
leak.)
Signed-off-by: Han Zhou <hzhou8@ebay.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
When inserting data into a "singleton" table (one that has maxRows ==
1), there is a check that ensures that the table is currently empty
before inserting the row. The intention is to prevent races where
multiple clients might attempt to insert rows at the same time.
The problem is that this singleton check can cause legitimate
transactions to fail. Specifically, a transaction that attempts to
delete the current content of the table and insert new data will cause
the singleton check to fail since the table currently has data.
This patch corrects the issue by keeping a count of the rows being
deleted and added to singleton tables. If the total is larger than zero,
then the net operation is attempting to insert rows. If the total is
less than zero, then the net operation is attempting to remove rows. If
the total is zero, then the operation is inserting and deleting an equal
number of rows (or is just updating rows). We only add the singleton
check if the total is larger than zero.
This patch also includes a new test for singleton tables that ensures
that the maxRows constraint works as expected.
Signed-off-by: Mark Michelson <mmichels@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
The design of the compound index feature in the C OVSDB IDL was unusual.
Indexes were generally referenced only by name rather than by pointer, and
could be obtained only from the top-level ovsdb_idl object. To iterate or
otherwise search an index required explicitly creating a special
ovsdb_idl_cursor object, which at least seemed somewhat heavy-weight given
that it required a string lookup in a table of indexes.
This commit redesigns the compound index interface. It discards the use of
names for indexes, instead having clients pass in a pointer to the index
object itself. It simplifies how indexes are created, gets rid of the need
for explicit cursor objects, and updates all of the users to the new
interface.
The underlying reason for this commit is to make it easier in
ovn-controller to keep track of the dependencies for a given function, by
making the indexes explicit arguments to any function that needs to use
them.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Han Zhou <hzhou8@ebay.com>
Several OVS structs contain embedded named unions, like this:
struct {
...
union {
...
} u;
};
C11 standardized a feature that many compilers already implemented
anyway, where an embedded union may be unnamed, like this:
struct {
...
union {
...
};
};
This is more convenient because it allows the programmer to omit "u."
in many places. OVS already used this feature in several places. This
commit embraces it in several others.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Justin Pettit <jpettit@ovn.org>
Tested-by: Alin Gabriel Serdean <aserdean@ovn.org>
Acked-by: Alin Gabriel Serdean <aserdean@ovn.org>
This patch fixes the memory leak reported by valgrind in testing
"learning action - TCPv6 port learning"
150 (40 direct, 110 indirect) bytes in 1 blocks are definitely lost in loss record 329 of 363
at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x51D0D4: xmalloc (util.c:120)
by 0x572E17: json_create (json.c:1442)
by 0x572E17: json_array_create (json.c:217)
by 0x572E17: json_array_create_2 (json.c:238)
by 0x4F69EA: ovsdb_idl_db_init (ovsdb-idl.c:424)
by 0x4F6A58: ovsdb_idl_create (ovsdb-idl.c:454)
by 0x40FAC7: bridge_init (bridge.c:396)
by 0x406F93: main (ovs-vswitchd.c:106)
3,727 (40 direct, 3,687 indirect) bytes in 1 blocks are definitely lost in loss record 358 of 363
at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x51D0D4: xmalloc (util.c:120)
by 0x572EBA: json_create (json.c:1442)
by 0x572EBA: json_object_create (json.c:254)
by 0x573254: json_parser_push_object (json.c:1264)
by 0x573254: json_parse_value.isra.12 (json.c:1293)
by 0x57339F: json_parser_input (json.c:1398)
by 0x5742C1: json_lex_input (json.c:982)
by 0x5748EB: json_parser_feed (json.c:1140)
by 0x57597A: jsonrpc_recv.part.7 (jsonrpc.c:332)
by 0x5768A7: jsonrpc_recv (jsonrpc.c:1140)
by 0x5768A7: jsonrpc_session_recv (jsonrpc.c:1113)
by 0x4F4E5C: ovsdb_idl_run (ovsdb-idl.c:818)
by 0x4100F9: bridge_run (bridge.c:2949)
by 0x406FB4: main (ovs-vswitchd.c:121)
Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
OVSDB_IDL_TRACK is not valid without OVSDB_IDL_ALERT, so it should
be turned off as well in ovsdb_idl_omit_alert().
Signed-off-by: Han Zhou <hzhou8@ebay.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
This commit adds support for OVSDB clustering via Raft. Please read
ovsdb(7) for information on how to set up a clustered database. It is
simple and boils down to running "ovsdb-tool create-cluster" on one server
and "ovsdb-tool join-cluster" on each of the others and then starting
ovsdb-server in the usual way on all of them.
One you have a clustered database, you configure ovn-controller and
ovn-northd to use it by pointing them to all of the servers, e.g. where
previously you might have said "tcp:1.2.3.4" was the database server,
now you say that it is "tcp:1.2.3.4,tcp:5.6.7.8,tcp:9.10.11.12".
This also adds support for database clustering to ovs-sandbox.
Acked-by: Justin Pettit <jpettit@ovn.org>
Tested-by: aginwala <aginwala@asu.edu>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Until now, a given ovsdb-idl instances has only monitored a single
database. In an upcoming commit, it will grow to also monitor a second
database that represents the state of the database server itself. Much of
the work is the same for both databases, so this commit breaks the common
code and data out into new data structures and functions.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Justin Pettit <jpettit@ovn.org>
Long ago, a <monitor-requests> object in the OVSDB protocol mapped a table
name to a single <monitor-request>. Since then, it has mapped a table name
to an *array of* <monitor-request> objects, but the OVSDB IDL has never
been updated to use the modern form. This commit makes that change.
Reported-by: Anil Jangam <anilj.mailing@gmail.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Justin Pettit <jpettit@ovn.org>
A synthetic column is one that is not present in the actual database but
instead calculated by code in the client based on columns in the row. This
can be useful to avoid repeatedly calculating the same function of a row.
Signed-off-by: Ben Pfaff <blp@ovn.org>
This allows slight code simplifications across the tree.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Tested-by: Yifeng Sun <pkusunyifeng@gmail.com>
Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com>
By verifying that singleton tables (that is, tables that should have exactly
one row) are empty when they emit transactions that insert into them,
ovs-vsctl and similar tools tolerate initialization races, where more than one
client at a time tries to initialize a singleton table.
The upshot is that if you create a database and then run multiple ovs-vsctl
(etc.) commands against it in parallel (without first initializing it
serially), then without this patch sometimes you will sometimes get failures
but this patch avoids them.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Alin Gabriel Serdean <aserdean@ovn.org>
This was used to uniquely identify the monitor, but there's no need for
that. A fixed monitor name works fine.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Alin Gabriel Serdean <aserdean@ovn.org>
If the database server sent an error reply to a monitor_cond request, and
the error was not a JSON string, then passing the error to json_string()
caused an assertion failure.
Found by inspection.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Alin Gabriel Serdean <aserdean@ovn.org>
This change documents the IDL state machine, adds other comments,
and fixes a spelling error in a comment.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Alin Gabriel Serdean <aserdean@ovn.org>
Poll-loop is the core to implement main loop. It should be available in
libopenvswitch.
Signed-off-by: Xiao Liang <shaw.leon@gmail.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Valgrind testcase 2339 (ovn -- ipam connectivity) reports the leak below:
45 (32 direct, 13 indirect) bytes in 1 blocks are definitely lost in loss record 65 of 83
at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x4A6D64: xmalloc (util.c:120)
by 0x49C847: shash_add_nocopy__ (shash.c:109)
by 0x49C847: shash_add_nocopy (shash.c:121)
by 0x49CA85: shash_add (shash.c:129)
by 0x49CA85: shash_add_once (shash.c:136)
by 0x4914B5: ovsdb_idl_create_index (ovsdb-idl.c:2067)
by 0x406C98: create_ovnsb_indexes (ovn-controller.c:568)
by 0x406C98: main (ovn-controller.c:619)
The leak happens when vsdb_idl_table is freed but its indexes are not freed.
Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
IDL index should be able to be used without having to be in a
transaction. However, current implementation leads to crash if
a reference type column is being set in an index row for querying
purpose when it is not in a transaction. It is because of the
uninitialized arcs and unnecessary updates of the arcs. This patch
fixes it by identifying index rows by a magic uuid, so that when
parsing index row, the arcs are not updated. A new test case is
added to cover this scenario.
Signed-off-by: Han Zhou <zhouhan@gmail.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Now that the 'new' datum is named 'new_datum', be more consistent by
renaming 'old' to 'old_datum' to match.
Signed-off-by: Joe Stringer <joe@ovn.org>
Acked-by: Ben Pfaff <blp@ovn.org>
In C++, 'new' is a keyword. If this is used as the name for a field,
then C++ compilers can get confused about the context and fail to
compile references to such fields. Rename the field to 'new_datum' to
avoid this issue.
Signed-off-by: Joe Stringer <joe@ovn.org>
Acked-by: Ben Pfaff <blp@ovn.org>
In C++, 'mutable' is a keyword. If this is used as the name for a field,
then C++ compilers can get confused about the context and fail to
compile references to such fields. Rename the field to 'is_mutable' to
avoid this issue.
Signed-off-by: Joe Stringer <joe@ovn.org>
Acked-by: Ben Pfaff <blp@ovn.org>
In C++, 'class' is a keyword. If this is used as the name for a field,
then C++ compilers can get confused about the context and fail to
compile references to such fields. Rename the field to 'class_' to
avoid this issue.
Signed-off-by: Joe Stringer <joe@ovn.org>
Acked-by: Ben Pfaff <blp@ovn.org>
This patch adds support for the creation of multicolumn indexes
in the C IDL to enable for efficient search and retrieval of database
rows by key.
Signed-off-by: Esteban Rodriguez Betancourt <estebarb@hpe.com>
Co-authored-by: Lance Richardson <lrichard@redhat.com>
Signed-off-by: Lance Richardson <lrichard@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Shadowing is when a variable with a given name in an inner scope hides a
different variable with the same name in a surrounding scope. This is
generally undesirable because it can confuse programmers. This commit
eliminates most of it.
Found with -Wshadow=local in GCC 7. The repo is not really ready to enable
this option by default because of a few cases that are harder to fix, and
harmless, such as nested use of CMAP_FOR_EACH.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Andy Zhou <azhou@ovn.org>
Add suport for ovsdb RBAC (role-based access control). This includes:
- Support for "RBAC_Role" table. A db schema containing a table
by this name will enable role-based access controls using
this table for RBAC role configuration.
The "RBAC_Role" table has one row per role, with each row having a
"name" column (role name) and a "permissions" column (map of
table name to UUID of row in separate permission table.) The
permission table has one row per access control configuration,
with the following columns:
"name" - name of table to which this row applies
"authorization" - set of column names and column:key pairs
to be compared against client ID to
determine authorization status
"insert_delete" - boolean, true if insertions and
authorized deletions are allowed.
"update" - Set of columns and column:key pairs for
which authorized updates are allowed.
- Support for a new "role" column in the remote configuration
table.
- Logic for applying the RBAC role and permission tables, in
combination with session role from the remote connection table
and client id, to determine whether operations modifying database
contents should be permitted.
- Support for specifying RBAC role string as a command-line option
to ovsdb-tool (Ben Pfaff).
Signed-off-by: Lance Richardson <lrichard@redhat.com>
Co-authored-by: Ben Pfaff <blp@ovn.org>
Signed-off-by: Ben Pfaff <blp@ovn.org>
The 'table' field is redundant because the required 'column' field
implies the table that the column is a part of.
This simplifies the users and makes it harder to get these things wrong.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Andy Zhou <azhou@ovn.org>
To allow client to know when the conditional monitoring changes
has been accepted by the OVSDB server and the 'idl' contents has
been updated to match the new conditions.
Signed-off-by: Andy Zhou <azhou@ovn.org>
Acked-by: Ben Pfaff <blp@ovn.org>
When generating conditional monitoring update request, current code
failed to update idl's 'request-id'. This bug causes the reply
message of the update request, regardless an ACK or a NACK, be
logged as an unexpected message at the debug level and ignored by
the core idl logic.
In addition, the idl should not generate another conditional
monitoring update request when there is an outstanding request.
So that the requests and their reply are properly serialized.
When the conditional monitoring is nacked by the server, drop idl
into a client visible error state.
Signed-off-by: Andy Zhou <azhou@ovn.org>
Acked-by: Ben Pfaff <blp@ovn.org>
In case connection is reset when there are buffered but unsent
conditions, these conditions will be sent as the new "monitor_cond"
message that will be sent after the idl reconnects.
Without this patch, those conditions will be unnecessarily sent again
with following monitoring condition update message.
Signed-off-by: Andy Zhou <azhou@ovn.org>
Acked-by: Ben Pfaff <blp@ovn.org>
Most users of OVSDB react to whatever is currently in their view of the
database, as opposed to keeping track of changes and reacting to those
changes individually. The interface to conditional monitoring was
different, in that it expected the client to say what to add or remove from
monitoring instead of what to monitor. This seemed reasonable at the time,
but in practice it turns out that the usual approach actually works better,
because the condition is generally a function of the data visible in the
database. This commit changes the approach.
This commit also changes the meaning of an empty condition for a table.
Previously, an empty condition meant to replicate every row. Now, an empty
condition means to replicate no rows. This is more convenient for code
that gradually constructs conditions, because it does not need special
cases for replicating nothing.
This commit also changes the internal implementation of conditions from
linked lists to arrays. I just couldn't see an advantage to using linked
lists.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Liran Schour <lirans@il.ibm.com>
The 'tc' member of struct ovsdb_idl_condition was written but never read,
so remove it.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Mickey Spiegel <mickeys.dev@gmail.com>
Some upcoming tests will add extra trickiness to the IDL internal graph.
This worries me, because the IDL doesn't have any checks for its graph
consistency. This commit adds some.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Andy Zhou <azhou@ovn.org>
