| Message ID | 20180713183624.16937-1-jkbs@redhat.com |
|---|---|
| Headers | show |
| Series | Daemon mode for ovn-nbctl | expand |
I've had a look through again and this series addresses the findings I previously had, so in short: Acked-by: Mark Michelson <mmichels@redhat.com> I decided to do some testing to see what sort of performance gain we see with this patch. I'm attaching four scripts that I used for testing. In all of the tests, I create a logical router and 159 logical switches connected to the router. I then create 92 logical switch ports on each logical switch (a total of 14628 ports). On alternating logical switch port additions, I create an address set. Every set of two logical switch ports gets added to the most recently created address set. For each logical switch port, I also create two ACLs. Note that the script does not perform any synchronization or port binding. The goal of the script is to isolate the performance of ovn-nbctl at scale rather than as an overall view of the performance of OVN. The scale.sh script is a straightforward version that uses individual calls to ovn-nbctl at each step. The scale_batch.sh script batches multiple operations into a single ovn-nbctl call. This way, the switch port, address set, and ACLs are created in one ovn-nbctl invocation. I ran these scripts about a month ago in a sandbox against OVS master. The scale.sh script took ~13 hours to complete. The scale_batch.sh script took ~3 hours to complete. I then modified each of these scripts to work with ovn-nbctl in daemon mode. scale.sh was modified into scale-daemon.sh, and scale_batch.sh was modified into scale_batch-daemon.sh. I applied the patch series to the current OVS master and ran the daemon versions of the scripts in a sandbox. scale-daemon.sh completed in 8 minutes 54 seconds. scale_batch-daemon.sh completed in 3 minutes 52 seconds. In both cases, this is a 99.9% decrease in the amount of time to complete. On 07/13/2018 02:36 PM, 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. > > 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 [1]. > > 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? > > A dirty, just-for-development integration with tests is available at: > > https://github.com/jsitnicki/ovs/commits/wip-nbctl-daemon-2018-07-13 > > Also, not all places that call ctl_fatal() have been converted yet to propagate > the error to the caller. As a result hitting an unconverted an error path will > cause the daemon process to die. This can be worked around with the --monitor > option. > > Daemon mode should be considered experimental. > > 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. > > Thanks, > Jakub > > [1] Except test "2563: ovn -- IPv6 ND Router Solicitation responder" which seems > broken in master @ 61b1c7acb9a2 ("netdev-bsd: Fix crash on FreeBSD."). > > Jakub Sitnicki (17): > 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: 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. > > lib/ovsdb-idl.c | 16 +- > lib/table.h | 2 + > ovn/utilities/ovn-nbctl.8.xml | 40 +++ > ovn/utilities/ovn-nbctl.c | 654 +++++++++++++++++++++++++++++++++--------- > tests/ovn-nbctl.at | 42 +++ > 5 files changed, 622 insertions(+), 132 deletions(-) > > -- > 2.14.4 >
Hi Mark, On Mon, 16 Jul 2018 14:11:19 -0400 Mark Michelson <mmichels@redhat.com> wrote: > I've had a look through again and this series addresses the findings I > previously had, so in short: > > Acked-by: Mark Michelson <mmichels@redhat.com> > > I decided to do some testing to see what sort of performance gain we see > with this patch. I'm attaching four scripts that I used for testing. > > In all of the tests, I create a logical router and 159 logical switches > connected to the router. I then create 92 logical switch ports on each > logical switch (a total of 14628 ports). On alternating logical switch > port additions, I create an address set. Every set of two logical switch > ports gets added to the most recently created address set. For each > logical switch port, I also create two ACLs. Note that the script does > not perform any synchronization or port binding. The goal of the script > is to isolate the performance of ovn-nbctl at scale rather than as an > overall view of the performance of OVN. > > The scale.sh script is a straightforward version that uses individual > calls to ovn-nbctl at each step. The scale_batch.sh script batches > multiple operations into a single ovn-nbctl call. This way, the switch > port, address set, and ACLs are created in one ovn-nbctl invocation. I > ran these scripts about a month ago in a sandbox against OVS master. The > scale.sh script took ~13 hours to complete. The scale_batch.sh script > took ~3 hours to complete. > > I then modified each of these scripts to work with ovn-nbctl in daemon > mode. scale.sh was modified into scale-daemon.sh, and scale_batch.sh was > modified into scale_batch-daemon.sh. I applied the patch series to the > current OVS master and ran the daemon versions of the scripts in a > sandbox. scale-daemon.sh completed in 8 minutes 54 seconds. > scale_batch-daemon.sh completed in 3 minutes 52 seconds. In both cases, > this is a 99.9% decrease in the amount of time to complete. Thanks a lot for the review feedback and for doing proper benchmarking of these changes. Great to have a confirmation that there is the expected speed-up. Thanks, Jakub
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. 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 [1]. 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? A dirty, just-for-development integration with tests is available at: https://github.com/jsitnicki/ovs/commits/wip-nbctl-daemon-2018-07-13 Also, not all places that call ctl_fatal() have been converted yet to propagate the error to the caller. As a result hitting an unconverted an error path will cause the daemon process to die. This can be worked around with the --monitor option. Daemon mode should be considered experimental. 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. Thanks, Jakub [1] Except test "2563: ovn -- IPv6 ND Router Solicitation responder" which seems broken in master @ 61b1c7acb9a2 ("netdev-bsd: Fix crash on FreeBSD."). Jakub Sitnicki (17): 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: 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. lib/ovsdb-idl.c | 16 +- lib/table.h | 2 + ovn/utilities/ovn-nbctl.8.xml | 40 +++ ovn/utilities/ovn-nbctl.c | 654 +++++++++++++++++++++++++++++++++--------- tests/ovn-nbctl.at | 42 +++ 5 files changed, 622 insertions(+), 132 deletions(-) -- 2.14.4