Re: [ovs-dev] [PATCH ovn v9 7/7] ovn-northd-ddlog: New implementation of ovn-northd based on ddlog.
On 2/17/21 2:49 AM, Ben Pfaff wrote: On Fri, Dec 11, 2020 at 03:43:22PM +0100, Dumitru Ceara wrote: diff --git a/tests/ovn.at b/tests/ovn.at index 6e396895826f..2e4eaf7f12f7 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -16982,6 +16982,10 @@ AT_CLEANUP OVN_FOR_EACH_NORTHD([ AT_SETUP([ovn -- IGMP snoop/querier/relay]) + +dnl This test has problems with ovn-northd-ddlog. +AT_SKIP_IF([test NORTHD_TYPE = ovn-northd-ddlog && test "$RUN_ANYWAY" != yes]) I might be wrong but I think the ddlog part still misses the logic from this commit from the ovn-northd C implementation: https://github.com/ovn-org/ovn/commit/97778ab3e422ac071faa67f9f477fd54977e9c04 Thanks very much, incorporating that fixed the IGMP and MLD snoop/querier/relay tests. No problem. The interconnection test still fails, probably because I haven't identified what's missing in that case yet. Any insight is really welcome. I don't think there were any interconnection related changes that went into ovn-northd recently but maybe Han is the better person to ask. Regards, Dumitru ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v9 7/7] ovn-northd-ddlog: New implementation of ovn-northd based on ddlog.
On 2/16/21 10:16 PM, Ben Pfaff wrote: On Fri, Dec 11, 2020 at 03:43:22PM +0100, Dumitru Ceara wrote: There are a few tests that fail (also northd-C tests) and I suspect it's because the OVS IDL split patches [0] this commit depends on don't include some of the recent fixes that went into the OVS IDL code, at least the following: I'm not sure whether the above comment was against an older form of the idl-split patches that went into master, but to make sure I took a careful look. Yes, IIRC, back then the idl-split series that was under review didn't include the changes below. - https://github.com/openvswitch/ovs/commit/97dbef6 I think that thiis is incorporated in the current OVS tree. I see the same code and comment in ovsdb-cs.c that was once in ovsdb-idl.c. - https://github.com/openvswitch/ovs/commit/91a6a45 - https://github.com/openvswitch/ovs/commit/08e130a - https://github.com/openvswitch/ovs/commit/f0d23f6 The above commits appear to me to be incorporated in the OVS tree correctly. Not much (any?) change was needed. The idl-split didn't have much effect on row tracking in general, because it's solidly on the idl side of the split. Now it's fine, thanks for double checking! ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v9 7/7] ovn-northd-ddlog: New implementation of ovn-northd based on ddlog.
On Fri, Dec 11, 2020 at 03:43:22PM +0100, Dumitru Ceara wrote: > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 6e396895826f..2e4eaf7f12f7 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -16982,6 +16982,10 @@ AT_CLEANUP > > > > OVN_FOR_EACH_NORTHD([ > > AT_SETUP([ovn -- IGMP snoop/querier/relay]) > > + > > +dnl This test has problems with ovn-northd-ddlog. > > +AT_SKIP_IF([test NORTHD_TYPE = ovn-northd-ddlog && test "$RUN_ANYWAY" != > > yes]) > > I might be wrong but I think the ddlog part still misses the logic from > this commit from the ovn-northd C implementation: > > https://github.com/ovn-org/ovn/commit/97778ab3e422ac071faa67f9f477fd54977e9c04 Thanks very much, incorporating that fixed the IGMP and MLD snoop/querier/relay tests. The interconnection test still fails, probably because I haven't identified what's missing in that case yet. Any insight is really welcome. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v9 7/7] ovn-northd-ddlog: New implementation of ovn-northd based on ddlog.
On Fri, Dec 11, 2020 at 03:43:22PM +0100, Dumitru Ceara wrote: > There are a few tests that fail (also northd-C tests) and I suspect it's > because the OVS IDL split patches [0] this commit depends on don't > include some of the recent fixes that went into the OVS IDL code, at > least the following: I'm not sure whether the above comment was against an older form of the idl-split patches that went into master, but to make sure I took a careful look. > - https://github.com/openvswitch/ovs/commit/97dbef6 I think that thiis is incorporated in the current OVS tree. I see the same code and comment in ovsdb-cs.c that was once in ovsdb-idl.c. > - https://github.com/openvswitch/ovs/commit/91a6a45 > - https://github.com/openvswitch/ovs/commit/08e130a > - https://github.com/openvswitch/ovs/commit/f0d23f6 The above commits appear to me to be incorporated in the OVS tree correctly. Not much (any?) change was needed. The idl-split didn't have much effect on row tracking in general, because it's solidly on the idl side of the split. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v9 7/7] ovn-northd-ddlog: New implementation of ovn-northd based on ddlog.
On Fri, Dec 11, 2020 at 03:43:22PM +0100, Dumitru Ceara wrote: > Also, I'm still seeing the libopenvswitchavx512.la related error we > reported earlier: > > error: failed to run `rustc` to learn about target-specific information > > Caused by: > process didn't exit successfully: `rustc - --crate-name ___ > --print=file-names -L ../../lib/.libs -L /root/ovs/lib/.libs -lssl > -lcrypto -lcap-ng /root/ovs/lib/libopenvswitchavx512.la -lpthread -lrt > -lm -lunbound -lpthread -lrt -lm -lunbound -Awarnings --crate-type bin > --crate-type rlib --crate-type dylib --crate-type cdylib --crate-type > staticlib --crate-type proc-macro --print=sysroot --print=cfg` (exit > code: 1) > --- stderr > error: multiple input filenames provided (first two filenames are `-` > and `/root/ovs/lib/libopenvswitchavx512.la`) > > I know you didn't hit this on your machines, but please let me know what > additional info you would require. I was able to fix this with the following: -8<--cut here-->8-- From: Ben Pfaff Date: Mon, 21 Dec 2020 18:17:57 -0800 Subject: [PATCH ovn] Extract dependencies from .la files recursively. Sometimes $dependency_libs in a libtool .la file points to another .la file. In that case, one must recurse, rather than just using its name directly. Signed-off-by: Ben Pfaff --- build-aux/automake.mk | 1 + build-aux/libtool-deps | 35 +++ northd/automake.mk | 5 +++-- 3 files changed, 39 insertions(+), 2 deletions(-) create mode 100755 build-aux/libtool-deps diff --git a/build-aux/automake.mk b/build-aux/automake.mk index 9007ecda9c19..8ad279789b21 100644 --- a/build-aux/automake.mk +++ b/build-aux/automake.mk @@ -6,6 +6,7 @@ EXTRA_DIST += \ build-aux/dpdkstrip.py \ build-aux/generate-dhparams-c \ build-aux/initial-tab-whitelist \ + build-aux/libtool-deps \ build-aux/sodepends.py \ build-aux/soexpand.py \ build-aux/text2c \ diff --git a/build-aux/libtool-deps b/build-aux/libtool-deps new file mode 100755 index ..c9ef66274890 --- /dev/null +++ b/build-aux/libtool-deps @@ -0,0 +1,35 @@ +#! /bin/sh + +if test $# != 1; then +echo "$0: exactly one argument required (use --help for help)" >&2 +exit 1 +elif test $1 = "--help"; then +cat <&2 + exit 1 +fi + +for dep in $dependency_libs; do + case $dep in + *.la) parse_dependencies "$dep" ;; + *) deps="$deps $dep" ;; + esac +done +} + +parse_dependencies "$1" +echo $deps diff --git a/northd/automake.mk b/northd/automake.mk index 1a28d7b8ee12..959612577101 100644 --- a/northd/automake.mk +++ b/northd/automake.mk @@ -94,9 +94,10 @@ cargo_build_01 = --features command-line --bin ovn_northd_cli cargo_build_10 = --lib cargo_build_11 = --features command-line +libtool_deps = $(srcdir)/build-aux/libtool-deps $(ddlog_targets): northd/ddlog.stamp lib/libovn.la $(OVS_LIBDIR)/libopenvswitch.la - $(AM_V_GEN)LIBOVN_DEPS=`. lib/libovn.la && echo "$$dependency_libs"` && \ - LIBOPENVSWITCH_DEPS=`. $(OVS_LIBDIR)/libopenvswitch.la && echo "$$dependency_libs"` && \ + $(AM_V_GEN)LIBOVN_DEPS=`$(libtool_deps) lib/libovn.la` && \ + LIBOPENVSWITCH_DEPS=`$(libtool_deps) $(OVS_LIBDIR)/libopenvswitch.la` && \ cd northd/ovn_northd_ddlog && \ RUSTC='$(RUSTC)' RUSTFLAGS="$(RUSTFLAGS)" \ cargo build --release $(CARGO_VERBOSE) $(cargo_build) --no-default-features --features ovsdb -- 2.28.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v9 7/7] ovn-northd-ddlog: New implementation of ovn-northd based on ddlog.
On 12/2/20 7:50 AM, Ben Pfaff wrote: > From: Leonid Ryzhyk > > This implementation is incremental, meaning that it only recalculates > what is needed for the southbound database when northbound changes > occur. It is expected to scale better than the C implementation, > for large deployments. (This may take testing and tuning to be > effective.) > > There are three tests that I'm having mysterious trouble getting > to work with DDlog. For now, I've marked the testsuite to skip > them unless RUN_ANYWAY=yes is set in the environment. > > Signed-off-by: Leonid Ryzhyk > Co-authored-by: Justin Pettit > Signed-off-by: Justin Pettit > Co-authored-by: Ben Pfaff > Signed-off-by: Ben Pfaff > --- Hi Ben, This is not a full review, just some initial comments. There are a few tests that fail (also northd-C tests) and I suspect it's because the OVS IDL split patches [0] this commit depends on don't include some of the recent fixes that went into the OVS IDL code, at least the following: - https://github.com/openvswitch/ovs/commit/97dbef6 - https://github.com/openvswitch/ovs/commit/91a6a45 - https://github.com/openvswitch/ovs/commit/08e130a - https://github.com/openvswitch/ovs/commit/f0d23f6 [0] https://patchwork.ozlabs.org/project/openvswitch/list/?series=217954 Also, I'm still seeing the libopenvswitchavx512.la related error we reported earlier: error: failed to run `rustc` to learn about target-specific information Caused by: process didn't exit successfully: `rustc - --crate-name ___ --print=file-names -L ../../lib/.libs -L /root/ovs/lib/.libs -lssl -lcrypto -lcap-ng /root/ovs/lib/libopenvswitchavx512.la -lpthread -lrt -lm -lunbound -lpthread -lrt -lm -lunbound -Awarnings --crate-type bin --crate-type rlib --crate-type dylib --crate-type cdylib --crate-type staticlib --crate-type proc-macro --print=sysroot --print=cfg` (exit code: 1) --- stderr error: multiple input filenames provided (first two filenames are `-` and `/root/ovs/lib/libopenvswitchavx512.la`) I know you didn't hit this on your machines, but please let me know what additional info you would require. [...] > > diff --git a/tests/ovn.at b/tests/ovn.at > index 6e396895826f..2e4eaf7f12f7 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -16982,6 +16982,10 @@ AT_CLEANUP > > OVN_FOR_EACH_NORTHD([ > AT_SETUP([ovn -- IGMP snoop/querier/relay]) > + > +dnl This test has problems with ovn-northd-ddlog. > +AT_SKIP_IF([test NORTHD_TYPE = ovn-northd-ddlog && test "$RUN_ANYWAY" != > yes]) I might be wrong but I think the ddlog part still misses the logic from this commit from the ovn-northd C implementation: https://github.com/ovn-org/ovn/commit/97778ab3e422ac071faa67f9f477fd54977e9c04 Thanks, Dumitru ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v9 7/7] ovn-northd-ddlog: New implementation of ovn-northd based on ddlog.
Bleep bloop. Greetings Ben Pfaff, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Comment with 'xxx' marker #3534 FILE: northd/ovn-northd-ddlog.c:164: .cs = ovsdb_cs_create(database, 1 /* XXX */, &northd_cs_ops, ctx), WARNING: Comment with 'xxx' marker #4322 FILE: northd/ovn-northd-ddlog.c:952: * XXX If the transaction we're sending to the database fails, then WARNING: Line lacks whitespace around operator WARNING: Line lacks whitespace around operator WARNING: Line lacks whitespace around operator #4375 FILE: northd/ovn-northd-ddlog.c:1005: --ovnnb-db=DATABASE connect to ovn-nb database at DATABASE\n\ WARNING: Line lacks whitespace around operator WARNING: Line lacks whitespace around operator WARNING: Line lacks whitespace around operator #4377 FILE: northd/ovn-northd-ddlog.c:1007: --ovnsb-db=DATABASE connect to ovn-sb database at DATABASE\n\ WARNING: Line lacks whitespace around operator WARNING: Line lacks whitespace around operator #4379 FILE: northd/ovn-northd-ddlog.c:1009: --unixctl=SOCKET override default control socket name\n\ WARNING: Line has trailing whitespace #6140 FILE: northd/ovn_northd.dl:177: var l3dgw_port = peer.and_then(|p| p.router.l3dgw_port), WARNING: Line has non-spaces leading whitespace WARNING: Line has trailing whitespace #8808 FILE: northd/ovn_northd.dl:2845: WARNING: Line has non-spaces leading whitespace WARNING: Line has trailing whitespace #13416 FILE: northd/ovn_northd.dl:7453: Lines checked: 14016, Warnings: 15, Errors: 0 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
