[ovs-dev] [PATCH v5 00/21] Daemon mode for ovn-nbctl

Mark Michelson mmichels at redhat.com
Thu Jul 19 16:26:24 UTC 2018


On 07/19/2018 09:51 AM, Jakub Sitnicki wrote:
> This series extends ovn-nbctl tool with support for the daemon mode, where
> ovn-nbctl acts a long-lived process that accepts commands over a UNIX socket.
> The daemon can be started the same way as any other OVS/OVN server:
> 
>    ovn-nbctl --detach --pidfile --log-file
> 
> While commands can be issued to it using the 'ovs-appctl' tool:
> 
>    ovs-appctl -t ovn-nbctl run [OPTIONS] COMMAND [-- [OPTIONS] COMMAND] ...
> 
> Although, the plan for future work is to extend ovn-nbctl so that it can control
> the daemon, if one is running.
> 
> The motivation and the main benefit from the daemon mode is that the contents of
> NB database have to be obtained only once, when the first command is ran. On big
> databases (1000's of logical ports) this results in a speed up per command in
> the range of 100's of milliseconds.
> 
> Mark Michelson has benchmarked previous version of this patchset and
> demonstrated a significant speed-up in the run-time of his benchmark [1].
> 
> The changes are functional to the point that all test cases in the ovn-nbctl
> test suite (tests/ovn-nbctl.at) and OVN test suite (tests/ovn.at) pass when
> running ovn-nbctl as a daemon [2].
> 
> However, I'm still thinking how to nicely integrate the daemon mode with the
> test suite so that the existing tests can be run using either the normal or the
> daemon mode. Any ideas?

When I think about this, I think of two obstacles:
1) What is a good way to run a test under both daemon mode and 
non-daemon mode?
2) What is the best way to compose a test that will run under both 
daemon mode and non-daemon mode

(2) seems like the easier problem to mechanically solve. You can perform 
a find and replace to change all current instances of `ovn-nbctl` to 
some bash function. The bash function could determine whether daemon 
mode is in use. If not in daemon mode, call `ovn-nbctl` directly. If in 
daemon mode, call `ovs-appctl -t ovn-nbctl run`.

This isn't a good long term solution though. It will be difficult to 
enforce the use of the bash function instead of calling ovn-nbctl. A 
potentially better long-term solution would be not to require ovs-appctl 
when communicating with the ovn-nbctl daemon. If ovn-nbctl could be 
called directly, it would make things easier.

(1) is a bit more tricky in my opinion.

One idea would be to pass in a flag to the testsuite that says to start 
an ovn-nbctl daemon when ovn_start() is called. This way, the entire 
runthrough of the test would be for ovn-nbctl daemon mode. In CI 
systems, we'd want to perform two runs of the testsuite, one in daemon 
mode, and one not in daemon mode. This idea is a bit overkill though 
since most tests in the testsuite are not even OVN tests. This also 
doesn't scale well in case there become other ways we want to run 
variants of existing tests.

A better idea is to somehow make existing tests that use ovn-nbctl run 
once in daemon mode and once in non-daemon mode. This is difficult, 
though, because the structure of the tests doesn't allow for an easy way 
to repeat the test. The "easiest" way I can think to do this is to 
modify the existing ovn test files to be .in files that at build time 
generate a daemon version of the test and a non-daemon version of the 
test. Notice "easiest" in quotation marks though. The advantage of doing 
it this way is that it simultaneously clears up problem (2) as well 
since the autogenerated files will have the proper invocations in them.

I suspect other people might have much better ideas than me, though :)

> 
> A dirty, just-for-development integration with tests is available at:
> 
>    https://github.com/jsitnicki/ovs/commits/wip-nbctl-daemon-2018-07-19
> 
> There is a related series pending review/merge that converts the remaining
> ctl_fatal() callers to propate the error [3]. It goes together with this
> patchset but they don't depend on each other.
> 
> Daemon mode should be considered experimental.
> 
> Thanks,
> Jakub
> 
> [1] https://mail.openvswitch.org/pipermail/ovs-dev/2018-July/349742.html
> 
> [2] Except test "2563: ovn -- IPv6 ND Router Solicitation responder" which seems
>      broken in master @ 61b1c7acb9a2 ("netdev-bsd: Fix crash on FreeBSD.").
> 
> [3] https://patchwork.ozlabs.org/project/openvswitch/list/?series=55975
> 
> 
> Changes v4 -> v5:
> 
> - Fix a build error in the middle of the series. Reported by 0-day robot:
>    https://mail.openvswitch.org/pipermail/ovs-dev/2018-July/349864.html
> 
> - Reword patch 7 description.
> 
> Changes v3 -> v4:
> 
> - Propagate errors from the commands parser and send them back to the JSON-RPC
>    client when running in daemon mode.
> 
> - Fix-up getopt()'s global state initialization before parsing a command line.
> 
> - Distinguish between unknown-option and missing-argument errors when parsing
>    command line options in daemon mode.
> 
> - Fix a double-free of transaction resources on error path.
> 
> - Add test for commands parser error paths.
> 
> - Use 'dnl' instead of '#' for comments in tests.
> 
> - To review just the interdiff run:
> 
>    git fetch --tags https://github.com/jsitnicki/ovs
>    git diff wip-nbctl-daemon-2018-07-{13,19}
> 
> Changes v2 -> v3:
> 
> - Wait for IDL to connect before detaching and running the unixctl server. Also,
>    terminate the daemon when the connection to the NB DB has failed. This
>    behavior is modeled after ovn-trace daemon mode.
> 
> - Add a dedicated option parser for the daemon. Parse only the options that are
>    have an effect on the main loop. Logging options are not supported. Usual way
>    to change the log levels for daemons is with 'vlog/set' command. Reported by
>    Mark Michelson.
> 
> - Drop the test for 'sync' command. The test is flawed as pointed out by Mark
>    Michelson. Reading the '*_cfg' value after 'ovn-nbctl --wait=X sync' has
>    returned does not prove that '--wait' has blocked until the '*_cfg' value has
>    changed. At the same time, we cannot read out the updated '*_cfg' value in the
>    same transaction as we change it in.
> 
> - To review just the interdiff run:
> 
>    git fetch --tags https://github.com/jsitnicki/ovs
>    git diff wip-nbctl-daemon-2018-07-{12,13}
> 
> Changes v1 -> v2:
> 
> - Work around a limitation in GCC 4.8.5 (RHEL/CentOS 7.5) that doesn't let us
>    use a constant expression as a RHS value in a structure assigment. Found by
>    the 0-day bot.
> 
> Changes RFC -> v1:
> 
> - Rebase onto master @ 61b1c7acb9a2 ("netdev-bsd: Fix crash on FreeBSD.").
> 
> - Add support for commands that use tabular output ('find' and 'list') thanks to
>    recent work in table module by Ben Pfaff. This includes support for options
>    that control table formatting.
> 
> - Run prerequisites routines. In the end this is needed to support the 'sync'
>    command, which itself is more like an option than command (has to get
>    processed before the commands run). This is also the reason for minor changes
>    in the IDL.
> 
> - Add support for --dry-run, --wait / --no-wait, --timeout, and --oneline
>    options. Timeout handling is different than in the normal mode - we only time
>    out in poll_block(). Still, it serves the purpose avoiding an infinite 'sync'.
> 
> - Add tests for ovn-nbctl 'sync' command, dry-run mode, and oneline-formatted
>    output.
> 
> - Extend the ovn-nbctl man-page with description of daemon mode.
> 
> - Remove extraneous return at the end of a void function. Pointed out by Ben
>    Pfaff.
> 
> - Drop WIP patch for integration with tests from the series until a right
>    approach can be found. Integration with tests that was used during development
>    is available at:
> 
>    https://github.com/jsitnicki/ovs/commits/wip-nbctl-daemon-2018-07-11
> 
> - Minor style cleanups.
> 
> 
> Jakub Sitnicki (21):
>    table: Introduce a constant for default table style.
>    ovsdb-idl: Allow monitoring columns that are already monitored.
>    ovn-nbctl: Extract the main loop.
>    ovn-nbctl: Pull up destroying commands from do_nbctl().
>    ovn-nbctl: Pull up releasing IDL from do_nbctl().
>    ovn-nbctl: Signal need to try again via an output param.
>    ovn-nbctl: Don't destroy the transaction twice on error.
>    db-ctl-base: Propagate error from parse_command().
>    db-ctl-base: Propagate errors from the commands parser.
>    ovn-nbctl: Propagate the error from do_nbctl().
>    ovn-nbctl: Propagate errors from the main loop.
>    ovn-nbctl: Propagate errors from prerequisites runner.
>    ovn-nbctl: Introduce a poll_timer based wait timeout.
>    ovn-nbctl: Extract helper for printing oneline output.
>    ovn-nbctl: Extract handling of options that affect main loop.
>    ovn-nbctl: Extract a helper for building short options string.
>    ovn-nbctl: Extract a helper for appending command options.
>    ovn-nbctl: Initial support for daemon mode.
>    tests: Add test for ovn-nbctl dry run mode.
>    tests: Add test for oneline-formatted output for ovn-nbctl.
>    tests: Add test for ovn-nbctl's command parser error paths.
> 
>   lib/db-ctl-base.c             |  85 ++++--
>   lib/db-ctl-base.h             |   6 +-
>   lib/ovsdb-idl.c               |  16 +-
>   lib/table.h                   |   2 +
>   ovn/utilities/ovn-nbctl.8.xml |  40 +++
>   ovn/utilities/ovn-nbctl.c     | 674 ++++++++++++++++++++++++++++++++++--------
>   ovn/utilities/ovn-sbctl.c     |   7 +-
>   tests/ovn-nbctl.at            | 118 ++++++++
>   utilities/ovs-vsctl.c         |   7 +-
>   vtep/vtep-ctl.c               |   7 +-
>   10 files changed, 796 insertions(+), 166 deletions(-)
> 
> --
> 2.14.4
> 



More information about the dev mailing list