Thanks Mark,

I appreciate the attention and agree that it would make sense to get
this landed earlier in the release cycle.

We tested the series for BGP scenarios in the last weeks, and I found
at least one missing piece - patch ports were cleaned up by both
controllers from all bridges, not just the integration bridge that
belongs to it. I've just sent the v9 series that includes a patch to
handle this [1]. (Plus a fix to a new test case and a tiny
documentation update.)

I'm looking forward to seeing an updated Ack of yours on v9.

Thanks a lot,
Ihar

[1] 
https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/

On Fri, Jan 6, 2023 at 1:01 PM Mark Michelson <[email protected]> wrote:
>
> Hi Ihar,
>
> I've taken a fresh look at the series after the holidays, and the
> patches look good to merge:
>
> Acked-by: Mark Michelson <[email protected]>
>
> I'm not going to claim I can predict every issue that might occur with
> this, but I think this is the ideal time to merge. It's early in the
> development cycle for 23.03, and this will give us plenty of time to
> determine if there are critical issues before 23.03 is released. It's
> also clearly marked as experimental, and should not affect deployments
> that do not attempt to use the feature. Perhaps by the time the 24.03
> LTS is ready to release, we may be able to remove the "experimental"
> flag from this feature.
>
> On 11/30/22 21:26, Ihar Hrachyshka wrote:
> > This series adds support to run multiple ovn-controller instances using
> > the same vswitchd instance. This may be used to reuse a single host
> > level vswitchd installation to run multiple CMS (e.g. k8s and
> > openstack), each having its own OVN stack running on a separate
> > integration bridge.
> >
> > This setup may, in some instances, simplify administration of the
> > system, since the admin no longer needs to maintain separate vswitchd
> > installations (e.g. in separate containers). This is also helpful when
> > running different datapath types for the mixed setup.
> >
> > v1: initial series
> > v2: change tunnel port naming scheme: include "chassis index" instead of
> >      its name for source chassis.
> > v2: formatting adjustments.
> > v3: fixed build due to ovs_abort missing arguments.
> > v3: added documentation to CLI and system-id-override file.
> > v3: added documentation for chassis specific db config options.
> > v3: documented the ability to run multiple controllers on the same host,
> >      while mentioning that this support is highly experimental.
> > v3: updated NEWS file to include the note about the new experimental
> >      issue.
> > v3: rebased.
> > v4: fixed a memory leak in get_chassis_idx.
> > v5: actually fix the leak...
> > v6: fix race condition in new test cases where ssl db configuration was
> >      removed before ovn-controller has a chance to read it from db,
> >      making it fail to start and process ports
> > v7: addresses Mark's comments from v6, specifically:
> >      - 1/7: Clean up allocated index on exit
> >      - 1/7: Remove hardcoded 16 with sizeof(CHASSIS_IDX_PREFIX) - 1
> >      - 1/7: Explain in comments why the first chassis uses an empty
> >        string index and not “0”
> >      - 2/7: Document that requested-chassis should use unique chassis
> >        names, not hostnames
> >      - 2/7: Document that unique own-bridges should be used for multiple
> >        co-hosted controllers
> >      - 2/7: Reworked logic for get_chassis_external_value functions to
> >        avoid duplication with smap_get_<type> functions
> >      - 2/7: Use wait_column in tests
> >      - 3/7: Add information about the directory where system-id-override
> >        file is located
> >      - 3/7: Use FILE * functions from stdio.h instead of Unix lower level
> >        equivalents
> >      - 3/7: Use wait_column in tests
> >      - 4/7: Also validate that CLI takes precedence over override file,
> >        not only db
> >      - 4/7: Use wait_column in tests
> >      - 5/7: - (no changes)
> >      - 6/7: Move start_virtual_controller to ovn-macros.at to avoid code
> >        duplication
> >      - 6/7: Added comments explaining why test cases running two
> >        controllers use the same sandbox name for both
> >      - 7/7: - (no changes)
> > v8:
> >      - 1/7: fixed test case for ovn-chassis-idx population
> >      - 1/7: fixed SIGSEGV errors in chassis_cleanup for chassis index
> >
> > Ihar Hrachyshka (7):
> >    Include "chassis index" into tunnel port name
> >    Support ovn-...-<chassis> specific global ovsdb options
> >    Allow to override system-id via file
> >    Support passing chassis name via CLI
> >    Don't touch tunnel ports from a different br-int
> >    Add connectivity test for 2 controllers on the same host
> >    Document experimental support for co-hosted controllers
> >
> >   NEWS                            |   2 +
> >   controller/chassis.c            | 280 ++++++++++++++++++++++++------
> >   controller/chassis.h            |  15 +-
> >   controller/encaps.c             |  76 ++++----
> >   controller/encaps.h             |   1 -
> >   controller/ovn-controller.8.xml |  35 +++-
> >   controller/ovn-controller.c     | 182 +++++++++++++------
> >   controller/patch.c              |   6 +-
> >   controller/physical.c           |   2 +-
> >   lib/ovn-util.c                  |  79 +++++++++
> >   lib/ovn-util.h                  |  26 +++
> >   ovn-nb.xml                      |   8 +
> >   tests/automake.mk               |   1 +
> >   tests/ovn-macros.at             |  36 +++-
> >   tests/ovn.at                    | 299 ++++++++++++++++++++++++++++++++
> >   tests/ovs-macros.at             |   2 +
> >   16 files changed, 892 insertions(+), 158 deletions(-)
> >
>

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to