Avoid code duplication by moving the section testing code to its own
function.
Also, verify the length of the kv_list meets the expectations.
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
Signed-off-by: Simon Horman <horms@ovn.org>
Add the sample action to those that can be called in nested actions
(such as clone).
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
Signed-off-by: Simon Horman <horms@ovn.org>
Parsing of info and matches was being tested as generic k-v parsing.
Also verify we don't find any unexpected field.
Also, verify the length of the kv_list meets the expectations.
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
Signed-off-by: Simon Horman <horms@ovn.org>
Initialize the cooperative multitasking module for the
ovsdb-server.
The server side schema conversion process used for storage
engines such as RAFT is time consuming, yield during
processing.
After the schema conversion is done, the processing of JSON-RPC
sessions and OVSDB monitors for reconnecting clients can
overrun the configured election timer.
The destruction of JSON objects representing the database
contents has been identified as one of the primary offenders.
Make use of yielding version of the JSON object destroy function
to mitigate.
This series has been tested by checking success of schema
conversion, ensuring no involuntary leader change occurs with
election timer configurations as low as 750 msec, on a 75MB
database with ~ 100 connected clients as produced by the
ovn-heater ocp-120-density-light test scenario.
Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Creating and destroying JSON objects may be time consuming.
Add json_serialized_object_create_with_yield() and
json_destroy_with_yield() functions that make use of the
cooperative multitasking module to yield during processing,
allowing time sensitive tasks in other parts of the program
to be completed during processing.
We keep these new functions private to OVS by adding a new
lib/json.h header file.
The include guard in the public include/openvswitch/json.h is
updated to contain the OPENVSWITCH prefix to be in line with the
other public header files, allowing us to use the non-prefixed
version in our private lib/json.h.
Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
The OVSDB server is mostly synchronous and single threaded. The
OVSDB RAFT storage engine operate under strict deadlines with
operational impact should the deadline be overrun.
Register for cooperative multitasking so that long running
processing elsewhere in the program may yield to allow stable
maintenance of the cluster.
Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
One of the goals of Open vSwitch is to be as resource efficient as
possible. Core parts of the program has been implemented as
asynchronous state machines, and when absolutely necessary
additional threads are used.
Introduce cooperative multitasking module which allow us to
interleave important processing with long running tasks while
avoiding the additional resource consumption of threads and
complexity of asynchronous state machines.
We will use this module to ensure long running processing in the
OVSDB server does not interfere with stable maintenance of the
RAFT cluster in subsequent patches.
Suggested-by: Ilya Maximets <i.maximets@ovn.org>
Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
It may be desirable to make use of time warp functionality in unit
tests.
Separate logic from time/stop unixctl into timeval_stop() and add
a new timeval_warp() interface for directing monotonic clock into
slow path and advancing the current monotonic directly.
This will be used in a patch that implements unit tests for the
cooperative multitasking module.
Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Expose the mcast group protocol via the mdb/show
command output.
Signed-off-by: Mohammad Heib <mheib@redhat.com>
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Store igmp/mld protocol version into the
mcast_group internally, the multicast snooping feature
is used by many OVS consumers and those consumers heavily rely
on the OVS implementation to manage/deal with mcast groups,
some of those consumers also need to deal/expose the mcast protocol
to the end user for debuggability purposes.
OVN for example needs to expose the protocol version to the end user
to match between the protocol version used in the OVN logical switches
and the uplink ports
Therefore, instead of implementing this in each OVS consumer that needs
to deal mcast group protocol version which will be very complicated
implementation since it rely on the OVS code, saving the protocol to
the mdb inside OVS will give that consumer access to the protocol version
very easily.
Signed-off-by: Mohammad Heib <mheib@redhat.com>
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
This patch adds ASAN and UBSAN GitHub action tests for both
the userspace and kernel datapaths.
Acked-by: Simon Horman <horms@ovn.org>
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
This patch combines the existing ubsan and asan GitHub actions
tests into one.
Acked-by: Simon Horman <horms@ovn.org>
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Previously tests that were generating a SIGSEGV were excluded
from UBSAN runs. With the correct environment variable, these
tests can be run. This is working even with the clang version
supplied by Ubuntu 20.04.
Acked-by: Simon Horman <horms@ovn.org>
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
This patch identifies new static analysis issues during a GitHub action
run and reports them. The process involves analyzing the changes introduced
in the current commit and comparing them to those in the preceding commit.
However, there are two cases when the GitHub push action runner does not
provide enough details to determine the preceding commit. These cases are
a new branch or a forced push. The strategy for these exceptions is to
find the first common commit on any upstream branch, and use that.
An example error output might look like this:
error level: +0 -0 no changes
warning level: +2 +0
New issue "deadcode.DeadStores Value stored to 'remote' is never read" (1 occurrence)
file:///home/runner/work/ovs/ovs/vswitchd/ovs-vswitchd.c:86
New issue "unix.Malloc Potential leak of memory pointed to by 'remote'" (1 occurrence)
file:///home/runner/work/ovs/ovs/vswitchd/ovs-vswitchd.c:95
note level: +0 -0 no changes
all levels: +2 +0
Acked-by: Simon Horman <horms@ovn.org>
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
The make clang-analyze target reports an 'Dereference of null
pointer' and an 'Uninitialized argument value' issue due to
it assumes some function can return NULL.
This patch annotates these functions, so the static analyzer
is aware of this.
Acked-by: Dumitru Ceara <dceara@redhat.com>
Acked-by: Simon Horman <horms@ovn.org>
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Action PORT_ID has been deprecated. Use REPRESENTED_PORT instead.
Acked-by: Kevin Traynor <ktraynor@redhat.com>
Signed-off-by: Ivan Malov <ivan.malov@arknetworks.am>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Large time warps can cause the 'long flow dump duration' log message
to happen. However, due to the level change, they could now cause
failures. This patch will stop these messages from failing the tests.
Fixes: 9bcfb8fb7784 ("ofproto-dpif-upcall: Change flow dump duration message to WARN level.")
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Basic relay and active-backup command execution tests extended
to run some copies of ovsdb-server processes with --config-file.
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
OVSDB server maintains a temporary file with the current database
configuration for the case it is restarted by a monitor process
after a crash. On startup the configuration from command line
arguments is stored there in a JSON format, also whenever user
changes the configuration with different UnixCtl commands, those
changes are getting added to the file. When restarted from the
crash it reads the configuration from the file and continues
with all the necessary remotes and databases.
This change allows it to be an external user-provided file that
OVSDB server will read the configuration from. The file can be
specified with a --config-file command line argument and it is
mutually exclusive with most other command line arguments that
set up remotes or databases, it is also mutually exclusive with
use of appctl commands that modify same configurations, e.g.
add/remove-db or add/remove-remote.
If the user wants to change the configuration of a running server,
they may change the file and call ovsdb-server/reload appctl.
OVSDB server will open a file, read and parse it, compare the
new configuration with the current one and adjust the running
configuration as needed. OVSDB server will try to keep existing
databases and connections intact, if the change can be applied
without disrupting the normal operation.
User-provided files are not trustworthy, so extra checks were
added to ensure a correct file format. If the file cannot be
correctly parsed, e.g. contains invalid JSON, no changes will
be applied and the server will keep using the previous
configuration until the next reload.
If config-file is provided for active-backup databases, permanent
disconnection of one of the backup databases no longer leads to
switching all other databases to 'active'. Only the disconnected
one will transition, since all of them have their own records in
the configuration file.
With this change, users can run all types of databases within
the same ovsdb-server process at the same time.
Simple configuration may look like this:
{
"remotes": {
"punix:db.sock": {},
"pssl:6641": {
"inactivity-probe": 16000,
"read-only": false,
"role": "ovn-controller"
}
},
"databases": {
"conf.db": {},
"sb.db": {
"service-model": "active-backup",
"backup": true,
"source": {
"tcp:127.0.0.1:6644": null
}
},
"OVN_Northbound": {
"service-model": "relay",
"source": {
"ssl:[fe:::1]:6642,ssl:[fe:::2]:6642": {
"max-backoff": 8000,
"inactivity-probe": 10000
}
}
}
}
}
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Allow setting all the JSON-RPC session options at once.
While at it, allow updating options the same way the source
can be updated while calling 'ovsdb_relay_add_db()' if the
relay is already configured.
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Allow setting all the options for the source connection, not only the
inactivity probe interval.
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Set all the options for the source connection, not only the
inactivity probe interval.
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Just introduced structure 'jsonrpc_session_options' is the same
as part of the 'ovsdb_jsonrpc_options'. In fact, these options
do really belong to a lower layer. So, replace a copy of these
fields with a structure, so it can be easily passed to jsonrpc's
'jsonrpc_session_set_options()'.
Not creating separate JSON parsing/formatting functions to avoid
creating an extra nesting level for the users who will write
the JSON definition in a configuration file. I.e. keeping the
JSON object flat. Also, not changing the 'db_config->options'
to be 'jsonrpc_session_options', even though we don't need the
'role' or 'read-only' fields. This allows us to use the same
JSON parsing function for both the remotes ans database sources.
Can be changed in the future, but for now keeping as is to avoid
extra code complication.
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
It's useful to have a way to update all the JSON-RPC session
options all at once and not call 3 separate functions every
time. This may also allow the internals of these options
to be better abstracted, i.e. allow users to not know what
are these options exactly.
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
It is currently not possible for the role to change in runtime
(unless a manual DB transaction is crafted), but it will be with
addition of a config file.
If the role changes, listening socket will be closed, and all
the connections to this remote will be terminated.
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Adding a --config-file option that will be used in the future to
allow users to provide the database server configuration via a
JSON file.
For now, it does nothing useful, but we define it as mutually
exclusive with all the command line options and UnixCtl commands
that configure values that will be available via a config file.
This will ensure that we don't have too many ways of configuring
the same thing at the same time.
New appctl command 'ovsdb-server/reload' is going to signal OVSDB
server that it needs to re-read the configuration file.
While at it, adding a missing 'usage' line for '--no-dbs'. This
option is rarely used, so it doesn't seem to be worth a separate fix.
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Add a new structure 'db_config' that holds the user-provided
configuration of the database. And attach this configuration
to each of the databases on the server.
Each database has a service model: standalone, clustered, relay
or active-backup. Relays and A-B databases have a source, each
source has its own set of JSON-RPC session options. A-B also
have an indicator of it being active or backup and an optional
list of tables to exclude from replication.
All of that should be stored per database in the temporary
configuration file that is used in order to restore the config
after the OVSDB crash. For that, the save/load functions are
also updates.
This change is written in generic way assuming all the databases
can have different configuration including service model.
The only user-visible change here is a slight modification of
the ovsdb-server/sync-status appctl, since it now needs to
skip databases that are not active-backup and also should report
active-backup databases that are currently active, i.e. not
added to the replication module.
If the service model is not defined in the configuration, it
is assumed to be standalone or clustered, and determined from
the storage type while opening the database. If the service
model is defined, but doesn't match the actual storage type
in the database file, ovsdb-server will fail to open the
database. This should never happen with internally generated
config file, but may happen in the future with user-provided
configuration files. In this case the service model is used
for verification purposes only, if administrator wants to
assert a particular model.
Since the database 'source' connections can't use 'role' or
'read-only' options, a new flag added to corresponding JSON
parsing functions to skip these fields.
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
When database is added to the replication, it should no longer
accept transactions that can modify it. When it's removed from
the replication, it should be writable again. Add this logic
to the replication module itself, so it can be removed from the
main ovsdb-server later.
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Refactoring of the replication code, so each database is handled
separately from each other. Supposed to work the same way as before
with the only difference that each backup database will have its own
connection to the source and will have its own state machine.
From the user's perspective, the only visible difference is that
ovsdb-server/sync-status appctl now shows the status of each
database separately.
If one of the connections is permanently broken, all the databases
will be switched to active. This is done in order to preserve the
old behavior where we had only one connection.
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Small refactoring so we can re-use this function in later commits.
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Store JSON-RPC options for each remote separately, so it will be
possible to have different configurations per remote in the future.
These are also stored to and loaded from the temporary file that
OVSDB is using to restore runtime configuration of the server
restarted by the monitor process after a crash.
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
These functions will be needed when we'll need to load/save
configuration of each OVSDB remote separately.
The parsing function is written in a way that it updates the
provided options and doesn't create a new structure. This
is done in order for different callers to have their own
default values and only update them with what is provided
by the user explicitly. For example, replication and relay
have different default probe intervals.
Acked-by: Mike Pattrick <mkp@redhat.com>
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Currently, the read-only option can be set on connections or JSON-RPC
server as a whole. However, there is no way to allow modifications in
one database, but not in the other.
Adding an internal read-only flag for a database itself. Will be used
later for running active and backup databases in a single process.
Marking the _Server database as read only is not necessary, because
modifications of internal databases are not allowed anyway, but it
doesn't hurt.
Acked-by: Mike Pattrick <mkp@redhat.com>
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Martin Kennelly observes that even though this data is available to
humans via the journal/log files, these aren't exactly easy for a
developer to make any kind of behavioral inferences. This kind of
log and counter would be useful when checking on system health to
let us know that an Open vSwitch component is noticing some kind of
system level hiccup.
Add a new coverage counter to track information on these events, and
let a developer or system engineer know how long these events have
occurred with some historical context.
Reported-at: https://lists.linuxfoundation.org/pipermail/ovs-discuss/2023-June/052523.html
Suggested-by: Martin Kennelly <mkennell@redhat.com>
Co-Authored-By: Aaron Conole <aconole@redhat.com>
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Acked-by: Simon Horman <horms@ovn.org>
Acked-by: Ilya Maximets <i.maximets@ovn.org>
Signed-off-by: Aaron Conole <aconole@redhat.com>
Currently the 'Spent an unreasonably long Xms dumping flows' message
is set to the INFO level. However, based on this, we are also
drastically limiting the number of flows in the datapath, and this
would warrant a WARNING level.
Acked-by: Simon Horman <horms@ovn.org>
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Acked-by: Ilya Maximets <i.maximets@ovn.org>
Signed-off-by: Aaron Conole <aconole@redhat.com>
In case the difference between 'old' and 'new' rows is readily
available, it can be used to construct added/removed datums
instead. Diffs are typically much smaller than the column
itself. This change more than doubles the performance of a
transaction replay.
For example, with this change applied, initial read of OVSDB
file containing 136K small transactions for large OVN port
groups and address sets on my laptop takes 11 seconds vs 24
seconds without.
Acked-by: Mike Pattrick <mkp@redhat.com>
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Database file contains the column diff, but it is discarded once
the 'new' state of a row is constructed. Keep it in the transaction
row, as it can be used later by other parts of the code.
Diffs do not live long, we keep them around only while transaction
is alive, so should not affect memory consumption.
Users for this data will be added in later commits.
Acked-by: Mike Pattrick <mkp@redhat.com>
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Add new 'monitor' command to test-ovsdb utilities to make them just
run IDL loop infinitely. Other commands can still be placed before
the 'monitor', e.g. setting up conditions, tracking, running a few
transactions.
Having that, adding a couple test cases for IDL with online database
conversion. Test checks that IDL receives monitor cancellation
notification and successfully re-sends monitor requests.
While at it, adding debug logging to ovsdb-server processes for easier
debugging.
While working on a test the issue was discovered that schema for
standalone databases is not getting updated in the _Server database
after conversion. Checking the new schema bits only for clustered
databases for now.
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Simon Horman <horms@ovn.org>
Currently python-ovs claims to be "db change aware" but does not
parse the "monitor_canceled" notification. Transactions can continue
being made, but the monitor updates will not be sent. This handles
monitor_cancel similarly to how ovsdb-cs currently does.
Fixes: c39751e44539 ("python: Monitor Database table to manage lifecycle of IDL client.")
Signed-off-by: Terry Wilson <twilson@redhat.com>
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Simon Horman <horms@ovn.org>
All existing test iterations assume that the FTP server is running on a
standard port, which may not always be the case. These tests helped find
problems in conntrack alg processing with non-standard ports.
Perform the necessary adjustments to ensure the test suite can start the
L7 server on a user-provided port.
Signed-off-by: Viacheslav Galaktionov <viacheslav.galaktionov@arknetworks.am>
Signed-off-by: Aaron Conole <aconole@redhat.com>
When a packet hits a flow rule without an explicitly specified helper,
OvS has to rely on automatic application layer gateway detection to
find related connections. This works as long as services are running on
their standard ports, e.g. when FTP servers use TCP port 21.
However, sometimes it's necessary to run services on non-standard ports.
In that case, there is no way for OvS to guess which protocol is used
within a given flow. Of course, this means that no related connections
can be recognized.
When a connection is committed with a particular helper, it's reasonable
to assume this helper will be used in subsequent CT actions, as long as
they don't override it. Achieve this behaviour by using the committed
connection's helper when a flow rule does not specify one.
Signed-off-by: Viacheslav Galaktionov <viacheslav.galaktionov@arknetworks.am>
Acked-by: Ivan Malov <ivan.malov@arknetworks.am>
Signed-off-by: Aaron Conole <aconole@redhat.com>
The current protocol detection logic relies on two pieces of metadata
passed as arguments: tp_src and tp_dst, which represent the L4 source
and destination port numbers from the flow that triggered the current
flow rule first, and was responsible for creating the current DP flow.
Since multiple network flows of many different kinds, potentially using
different protocols on all layers, can be processed by one flow rule,
using the metadata of some unrelated flow might lead to unexpected
results. For example, ICMP type and code can be interpreted as TCP
source and destination ports. This can confuse the code responsible for
the helper selection, leading to errors in traffic handling and
incorrect detection of related flows.
One of the easiest ways to fix this problem is to simply remove the
tp_src and tp_dst parameters from the picture. The current code base has
no good use for them.
The helper selection logic was based on these values and therefore needs
to be changed. Ensure that the helper specified in a flow rule is used,
given it is compatible with the L4 protocol of the packet. When a flow
rule does not specify a helper, one can still be picked using the given
packet's metadata like TCP/UDP ports.
Signed-off-by: Viacheslav Galaktionov <viacheslav.galaktionov@arknetworks.am>
Signed-off-by: Aaron Conole <aconole@redhat.com>
Linux kernel commit ebddb1404900 ("net: move the nat function to
nf_nat_ovs for ovs and tc") introduced a regression into the kernel
datapath which prevented the openvswitch match key from being updated
when nat was undone for packets in the related conntrack state. This
issue caused these packets (usually ICMP/ICMPv6 error packets) to
match the wrong openflow rule.
This issue was fixed in linux kernel commit e6345d2824a3 ("netfilter:
nf_nat: fix action not being set for all ct states").
This test will fail for linux kernel versions v6.2 to v6.6, so test is
skipped for versions lower than v6.7.
Link: https://lore.kernel.org/netdev/20231221224311.130319-1-brad@faucet.nz/
Suggested-by: Aaron Conole <aconole@redhat.com>
Signed-off-by: Brad Cowie <brad@faucet.nz>
Signed-off-by: Aaron Conole <aconole@redhat.com>
