[ovs-dev] [PATCH ovn v10 0/4] Introduce infrastructure for VIF plug providers.

Frode Nordahl frode.nordahl at canonical.com
Tue Nov 16 10:27:42 UTC 2021


On Tue, Nov 16, 2021 at 3:35 AM Numan Siddique <numans at ovn.org> wrote:
>
> On Tue, Nov 9, 2021 at 3:03 PM Han Zhou <hzhou at ovn.org> wrote:
> >
> > On Tue, Nov 9, 2021 at 11:12 AM Numan Siddique <numans at ovn.org> wrote:
> > >
> > > On Tue, Nov 9, 2021 at 1:38 AM Frode Nordahl
> > > <frode.nordahl at canonical.com> wrote:
> > > >
> > > > On Mon, Nov 8, 2021 at 11:08 PM Numan Siddique <numans at ovn.org> wrote:
> > > > >
> > > > > On Fri, Nov 5, 2021 at 5:32 PM Frode Nordahl
> > > > > <frode.nordahl at canonical.com> wrote:
> > > > > >
> > > > > > fre. 5. nov. 2021, 20:43 skrev Han Zhou <hzhou at ovn.org>:
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On Fri, Nov 5, 2021 at 7:00 AM Frode Nordahl <
> > frode.nordahl at canonical.com>
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > Introduce infrastructure for VIF plug providers and add feature
> > to
> > > > > > > > ovn-controller to add and remove ports on the integration
> > bridge as
> > > > > > > > directed by CMS through Logical_Switch_Port options.
> > > > > > > >
> > > > > > > > Traditionally it has been the CMSs responsibility to create
> > Virtual
> > > > > > > > Interfaces (VIFs) as part of instance (Container, Pod, Virtual
> > > > > > > > Machine etc.) life cycle, and subsequently manage plug/unplug
> > > > > > > > operations on the Open vSwitch integration bridge.
> > > > > > > >
> > > > > > > > With the advent of NICs connected to multiple distinct CPUs we
> > can
> > > > > > > > have a topology where the instance runs on one host and Open
> > > > > > > > vSwitch and OVN runs on a different host, the smartnic control
> > plane
> > > > > > > > CPU.  The host facing interfaces will be visible to Open vSwitch
> > > > > > > > and OVN as representor ports.
> > > > > > > >
> > > > > > > > The actions necessary for plugging and unplugging the
> > representor
> > > > > > > > port in Open vSwitch running on the smartnic control plane CPU
> > would
> > > > > > > > be the same for every CMS.
> > > > > > > >
> > > > > > > > Hardware or platform specific details for initialization and
> > lookup
> > > > > > > > of representor ports is provided by an plugging provider hosted
> > > > > > > > inside or outside the core OVN repository, and linked at OVN
> > build
> > > > > > > > time.
> > > > > > > >
> > > > > > > > RFC1 -> RFC2:
> > > > > > > >  - Introduce the plug-provider interface, remove
> > hardware/platform
> > > > > > > >    dependent code.
> > > > > > > >  - Add ovsport module.
> > > > > > > >  - Integrate with binding module.
> > > > > > > >  - Split into multiple patches.
> > > > > > > >
> > > > > > > > RFC2 -> v1:
> > > > > > > >  - Extend build system, `--with-plug-provider`.
> > > > > > > >  - Check for required data in b_ctx_in and lbinding data
> > structures.
> > > > > > > >  - Split consider_plug_lport into update and create processing
> > > > > > > >    functions.
> > > > > > > >  - Consider unplug on release where relevant.
> > > > > > > >  - Add ovn-controller `--enable-dummy-plug` option for testing.
> > > > > > > >  - Consistent function naming and move ovsport module to
> > controller/.
> > > > > > > >  - Rename plug-test.c -> plug-dummy.c.
> > > > > > > >  - Add functional- and unit- tests.
> > > > > > > >
> > > > > > > > v1 -> v2:
> > > > > > > >  - Move update to controller/binding.h from patch 6 -> patch 5.
> > > > > > > >  - Fix lint problems reported by 0-day Robot.
> > > > > > > >
> > > > > > > > v2 -> v3:
> > > > > > > >  - Fix build system extension for plug provider.
> > > > > > > >  - Implement DDlog version of northd change.
> > > > > > > >  - Rebase on tip.
> > > > > > > >
> > > > > > > > v3 -> v4:
> > > > > > > >  - sb:Port_Binding:plugged_by ->
> > sb:Port_Binding:requested_chassis.
> > > > > > > >  - Move documentation of plugin specific options to plugin
> > > > > > > >    implementation.
> > > > > > > >  - ovn-northd-ddlog: squash changes into same patch as C
> > version, rework
> > > > > > > >    the DDlog implementation.
> > > > > > > >  - controller: Use requested_chassis column instead of parsing
> > options.
> > > > > > > >  - plug-provider: Remove the extra class instantiation layer and
> > > > > > > >    refcounting.  Add documentation and various tweaks.
> > > > > > > >  - controller: Add engine node for plug provider so that a
> > plugin run
> > > > > > > >    function can run at an appropriate time and also allow
> > feeding back
> > > > > > > >    information about changes that can not be handled
> > incrementally.
> > > > > > > >  - binding: Fix return values for plug functions so they adhere
> > to the
> > > > > > > >    incremental processing engine contract.
> > > > > > > >  - Add support for building in-tree plug providers.
> > > > > > > >
> > > > > > > > v4 -> v5:
> > > > > > > >  - binding: Make some data structures and functions public to
> > allow
> > > > > > > >    other modules to make use of its local binding tracking data
> > > > > > > >    structures.
> > > > > > > >  - Add separate incremental processing engine nodes for the plug
> > > > > > > >    provider.
> > > > > > > >  - Do change handling in the controller/plug module rather than
> > piggy
> > > > > > > >    backing on the binding module.
> > > > > > > >  - Deal with asynchronous notification to plug provider class
> > and
> > > > > > > >    subsequent freeing of data structures when the transaction
> > commits.
> > > > > > > >  - Drop the representor plugin as in-tree plugin to address
> > concerns
> > > > > > > about
> > > > > > > >    it being Linux-only and having netlink/kernel dependencies.
> > Will
> > > > > > > >    upstream to ovn-org/ovn-vif instead.
> > > > > > > >
> > > > > > > > v5 -> v6:
> > > > > > > >  - Fix 0-day Robot documentation lexer warning treated as error:
> > > > > > > >
> > > > > > >
> > https://mail.openvswitch.org/pipermail/ovs-build/2021-September/017538.html
> > > > > > > >
> > > > > > > > v6 -> v7:
> > > > > > > >  - lport: Fall back to strcmp when requested-chassis option is
> > set
> > > > > > > >           but Port_Binding->requested_chassis is not filled yet.
> > > > > > > >  - physical: Make use of common lport_can_bind_on_this_chassis
> > helper
> > > > > > > >              instead of duplicating the inverse check in the
> > code.
> > > > > > > >  - tests: Add test cases to cover currently uncovered
> > requested-chassis
> > > > > > > >           functionality.
> > > > > > > >  - plug: Make smaps struct members instead of pointers.
> > > > > > > >  - plug: Don't use is_deleted function when not iterating over
> > tracked
> > > > > > > >          records.  Avoid adding multiple delete or update iface
> > requests
> > > > > > > >          to the same transaction.
> > > > > > > >  - plug: For full recompute, don't process until northd has
> > populated
> > > > > > > >          Port_Binding->requested_chassis to avoid thrashing on
> > startup.
> > > > > > > >          Also fix order of processing.
> > > > > > > >  - plug: Handle failed transactions appropriately.
> > > > > > > >  - tests: Clean up and extend plug test case.
> > > > > > > >
> > > > > > > > v7 -> v8:
> > > > > > > >  - Patches 1 through 9 from v7 was merged.
> > > > > > > >  - Use Port_Binding:requested_chassis index when iterating over
> > PBs.
> > > > > > > >  - Drop separate I-P engine nodes for the VIF plug providers at
> > this
> > > > > > > >    time, we can revisit the need for it in the event of VIF plug
> > > > > > > >    providers gaining support for Scalable Functions and the
> > increased
> > > > > > > >    density that entails.
> > > > > > > >  - Rename module, objects and folders from 'plug-' to
> > 'vif-plug-*' to
> > > > > > > >    avoid using too generic names.
> > > > > > > >  - Don't call vif_plug_port_ctx_destroy on vif_plug_port_prepare
> > > > > > > >    failure, make the convention that plug provider should clean
> > up after
> > > > > > > >    itself in that case.
> > > > > > > >  - Simplify the unplug/plug thrashing avoidance on startup with
> > a rather
> > > > > > > >    naive counter to ensure IDL has data prior to attempting any
> > > > > > > >    processing.  Once we identify the root of the issue this can
> > be
> > > > > > > removed.
> > > > > > > >
> > > > > > > > v8 -> v9:
> > > > > > > >  - Amend comment with inaccurate statements about behavior of
> > tracked
> > > > > > > >    data.
> > > > > > > >  - Increase the IDL prime counter and reset it whenever the
> > OVNSB DB
> > > > > > > >    reconnects.
> > > > > > > >  - Don't add indexes not used by I-P as inputs to the I-P
> > engine.
> > > > > > > >  - Update NEWS statement.
> > > > > > > >  - Fix Documentation path reference in ovn-architecture.7.xml
> > > > > > > >
> > > > > > > > v9 -> v10:
> > > > > > > >  - Move creation of indexes from patch 2 -> patch 3
> > > > > > > >
> > > > > > > > Previous discussion:
> > > > > > > >  - RFC1:
> > > > > > >
> > https://patchwork.ozlabs.org/project/ovn/patch/20210509140305.1910796-1-frode.nordahl@canonical.com/
> > > > > > > >  - RFC2:
> > > > > > >
> > https://patchwork.ozlabs.org/project/ovn/cover/20210805145013.3033919-1-frode.nordahl@gmail.com/
> > > > > > > >
> > > > > > > > Frode Nordahl (4):
> > > > > > > >   lib: Add infrastructure for VIF plug providers.
> > > > > > > >   ovn-controller: Prepare VIF plug provider infrastructure.
> > > > > > > >   controller: Consider plugging VIF on CMS request.
> > > > > > > >   NEWS: Add note on infrastructure for VIF plug providers.
> > > > > > > >
> > > > > > > >  Documentation/automake.mk                     |   2 +
> > > > > > > >  Documentation/topics/index.rst                |   1 +
> > > > > > > >  .../topics/vif-plug-providers/index.rst       |  32 +
> > > > > > > >  .../vif-plug-providers/vif-plug-providers.rst | 209 ++++++
> > > > > > > >  NEWS                                          |   6 +
> > > > > > > >  acinclude.m4                                  |  49 ++
> > > > > > > >  configure.ac                                  |   2 +
> > > > > > > >  controller/automake.mk                        |   4 +-
> > > > > > > >  controller/ovn-controller.c                   |  81 ++-
> > > > > > > >  controller/test-vif-plug.c                    |  72 ++
> > > > > > > >  controller/vif-plug.c                         | 634
> > ++++++++++++++++++
> > > > > > > >  controller/vif-plug.h                         |  80 +++
> > > > > > > >  lib/automake.mk                               |  10 +-
> > > > > > > >  lib/vif-plug-provider.c                       | 204 ++++++
> > > > > > > >  lib/vif-plug-provider.h                       | 163 +++++
> > > > > > > >  lib/vif-plug-providers/dummy/vif-plug-dummy.c | 120 ++++
> > > > > > > >  ovn-architecture.7.xml                        |  35 +-
> > > > > > > >  ovn-nb.xml                                    |  21 +
> > > > > > > >  tests/automake.mk                             |  13 +-
> > > > > > > >  tests/ovn-macros.at                           |   2 +-
> > > > > > > >  tests/ovn-vif-plug.at                         |   8 +
> > > > > > > >  tests/ovn.at                                  | 121 ++++
> > > > > > > >  22 files changed, 1851 insertions(+), 18 deletions(-)
> > > > > > > >  create mode 100644
> > Documentation/topics/vif-plug-providers/index.rst
> > > > > > > >  create mode 100644
> > > > > > > Documentation/topics/vif-plug-providers/vif-plug-providers.rst
> > > > > > > >  create mode 100644 controller/test-vif-plug.c
> > > > > > > >  create mode 100644 controller/vif-plug.c
> > > > > > > >  create mode 100644 controller/vif-plug.h
> > > > > > > >  create mode 100644 lib/vif-plug-provider.c
> > > > > > > >  create mode 100644 lib/vif-plug-provider.h
> > > > > > > >  create mode 100644
> > lib/vif-plug-providers/dummy/vif-plug-dummy.c
> > > > > > > >  create mode 100644 tests/ovn-vif-plug.at
> > > > > > > >
> > > > > > > > --
> > > > > > > > 2.32.0
> > > > > > > >
> > > > > > >
> > > > > > > Thanks Frode! I applied the series to the main branch with a
> > minor comment
> > > > > > > update in patch1, see below. I didn't want to bother you for
> > another round
> > > > > > > of patches for this, so hope it's fine.
> > > > > > >
> > > > > >
> > > > > > Thank you for extending that comment in-line, makes it more clear
> > what
> > > > > > issue we are working around.
> > > > > >
> > > > > > Thanks for Numan's reviews for the previous versions as well, and I
> > added
> > > > > > > his acks except for the last patch which was added after his ack
> > for the
> > > > > > > series.
> > > > > > >
> > > > > >
> > > > > > Many thanks to both of you for reviews and merges, and for bearing
> > with me
> > > > > > as I have discovered the innards of the OVN code base through the
> > course of
> > > > > > adding this feature. The end result is much better because of it.
> > And you
> > > > > > well keep seeing contributions coming from me.
> > > > >
> > > > > That's great.  Thanks.
> > > > >
> > > > > Are you planning or prefer to have ovn-vif as a project in ovn-org
> > github repo ?
> > > >
> > > > Yes, having ovn-vif as a project in the ovn-org GitHub organization
> > > > would be great.
> > > >
> > > > I have staged a repository in [0] that currently hosts the representor
> > > > VIF plug provider, and the repository is prepared to hold multiple VIF
> > > > plug provider implementations.
> > > >
> > > > It is laid out in the same way as other OVS/OVN projects and uses
> > > > appropriate parts of the OVS/OVN build system (original authors
> > > > attributed in AUTHORS.rst). It contains contributing documentation
> > > > that includes the OVS Developer's Certificate of Origin and there is
> > > > also a proposed maintainers document (I'd be happy to include any OVN
> > > > maintainers in that document if appropriate). There is of course also
> > > > a GitHub Actions based CI that exercizes integrated builds.
> > > >
> > > > From my point of view it is ready for import at this moment.
> > > >
> > > > 0: https://github.com/fnordahl/ovn-vif
> > >
> > > Thanks.
> > >
> > > +1 from me for having ovn-vif project in ovn-org.
> > >
> > > @Han Zhou  @Mark Michelson and everyone
> > >
> > > Please let me know if you're fine or have any objections.
> > >
>
> > > Thanks
> > > Numan
> > >
> >
> > +1 from me.
>
> Thanks Han.
>
> @Frode -  I've cloned the repository from the repo
> https://github.com/fnordahl/ovn-vif to here -
> https://github.com/ovn-org/ovn-vif
>
> Cloning this repo is fine right ?
>
> Let me know if you've any questions.

Perfect, thank you Numan!

-- 
Frode Nordahl

> Thanks
> Numan
>
> >
> > Thanks,
> > Han
> >
> > > >
> > > > --
> > > > Frode Nordahl
> > > >
> > > > > Thanks
> > > > > Numan
> > > > >
> > > > > >
> > > > > > Cheers!
> > > > > >
> > > > > > --
> > > > > > Frode Nordahl
> > > > > >
> > > > > >
> > > > > > > Regards,
> > > > > > > Han
> > > > > > >
> > > > > > >
> > > > > > >
> > -----------------------------------------------------------------------------------------------------------------------------------
> > > > > > > diff --git a/controller/vif-plug.c b/controller/vif-plug.c
> > > > > > > index 1015a4c13..62b75263c 100644
> > > > > > > --- a/controller/vif-plug.c
> > > > > > > +++ b/controller/vif-plug.c
> > > > > > > @@ -526,10 +526,11 @@ vif_plug_handle_iface(const struct
> > ovsrec_interface
> > > > > > > *iface_rec,
> > > > > > >  }
> > > > > > >
> > > > > > >  /* On initial startup or on IDL reconnect, several rounds of the
> > main
> > > > > > > loop may
> > > > > > > - * run before data is actually loaded in the IDL.  This
> > situation is
> > > > > > > currently
> > > > > > > - * not reflected in a call to ovsdb_idl_has_ever_connected().
> > Until we
> > > > > > > find
> > > > > > > - * the root of this issue we need this counter so that we do not
> > > > > > > erronously
> > > > > > > - * unplug ports because the data is just not loaded yet.
> > > > > > > + * run before data is actually loaded in the IDL, primarily
> > depending on
> > > > > > > + * conditional monitoring status and other events that could
> > trigger main
> > > > > > > loop
> > > > > > > + * runs during this period.  Until we find a reliable way to
> > determine the
> > > > > > > + * completeness of the initial data downloading we need this
> > counter so
> > > > > > > that we
> > > > > > > + * do not erronously unplug ports because the data is just not
> > loaded yet.
> > > > > > >   */
> > > > > > >  #define VIF_PLUG_PRIME_IDL_COUNT_SEEED 10
> > > > > > >  static int vif_plug_prime_idl_count =
> > VIF_PLUG_PRIME_IDL_COUNT_SEEED;
> > > > > > >
> > > > > > _______________________________________________
> > > > > > dev mailing list
> > > > > > dev at openvswitch.org
> > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > > > >
> > > > _______________________________________________
> > > > dev mailing list
> > > > dev at openvswitch.org
> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >


More information about the dev mailing list