On Tue, Nov 9, 2021 at 3:03 PM Han Zhou <hz...@ovn.org> wrote: > > On Tue, Nov 9, 2021 at 11:12 AM Numan Siddique <num...@ovn.org> wrote: > > > > On Tue, Nov 9, 2021 at 1:38 AM Frode Nordahl > > <frode.nord...@canonical.com> wrote: > > > > > > On Mon, Nov 8, 2021 at 11:08 PM Numan Siddique <num...@ovn.org> wrote: > > > > > > > > On Fri, Nov 5, 2021 at 5:32 PM Frode Nordahl > > > > <frode.nord...@canonical.com> wrote: > > > > > > > > > > fre. 5. nov. 2021, 20:43 skrev Han Zhou <hz...@ovn.org>: > > > > > > > > > > > > > > > > > > > > > > > On Fri, Nov 5, 2021 at 7:00 AM Frode Nordahl < > frode.nord...@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.nord...@canonical.com/ > > > > > > > - RFC2: > > > > > > > https://patchwork.ozlabs.org/project/ovn/cover/20210805145013.3033919-1-frode.nord...@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. 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 > > > > > d...@openvswitch.org > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > > _______________________________________________ > > > dev mailing list > > > d...@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
