[ovs-dev] [PATCH ovn v8 1/4] lib: Add infrastructure for VIF plug providers.

Frode Nordahl frode.nordahl at canonical.com
Fri Nov 5 11:49:39 UTC 2021


On Fri, Nov 5, 2021 at 8:40 AM Han Zhou <hzhou at ovn.org> wrote:
>
>
>
> On Tue, Nov 2, 2021 at 4:29 AM Frode Nordahl <frode.nordahl at canonical.com> wrote:
> >
> > New lib/vif-plug-provider module contains the infrastructure for
> > registering VIF plug provider classes which may be hosted inside
> > or outside the core OVN repository.
> >
> > New controller/vif-plug module adds internal interface for
> > interacting with the VIF plug providers.
> >
> > Extend build system to allow building of built-in VIF plug
> > providers and linking an externally built VIF plug provider.
> >
> > Signed-off-by: Frode Nordahl <frode.nordahl at canonical.com>
> > ---
> >  Documentation/automake.mk                     |   2 +
> >  Documentation/topics/index.rst                |   1 +
> >  .../topics/vif-plug-providers/index.rst       |  32 +
> >  .../vif-plug-providers/vif-plug-providers.rst | 209 ++++++
> >  acinclude.m4                                  |  49 ++
> >  configure.ac                                  |   2 +
> >  controller/automake.mk                        |   4 +-
> >  controller/test-vif-plug.c                    |  72 ++
> >  controller/vif-plug.c                         | 632 ++++++++++++++++++
> >  controller/vif-plug.h                         |  79 +++
> >  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 +-
> >  tests/automake.mk                             |  13 +-
> >  tests/ovn-vif-plug.at                         |   8 +
> >  17 files changed, 1619 insertions(+), 16 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
> >
> > diff --git a/Documentation/automake.mk b/Documentation/automake.mk
> > index b3fd3d62b..704c80671 100644
> > --- a/Documentation/automake.mk
> > +++ b/Documentation/automake.mk
> > @@ -28,6 +28,8 @@ DOC_SOURCE = \
> >         Documentation/topics/ovn-news-2.8.rst \
> >         Documentation/topics/role-based-access-control.rst \
> >         Documentation/topics/debugging-ddlog.rst \
> > +       Documentation/topics/vif-plug-providers/index.rst \
> > +       Documentation/topics/vif-plug-providers/vif-plug-providers.rst \
> >         Documentation/howto/index.rst \
> >         Documentation/howto/docker.rst \
> >         Documentation/howto/firewalld.rst \
> > diff --git a/Documentation/topics/index.rst b/Documentation/topics/index.rst
> > index d58d5618b..e9e49c742 100644
> > --- a/Documentation/topics/index.rst
> > +++ b/Documentation/topics/index.rst
> > @@ -41,6 +41,7 @@ OVN
> >     high-availability
> >     role-based-access-control
> >     ovn-news-2.8
> > +   vif-plug-providers/index
> >     testing
> >
> >  .. list-table::
> > diff --git a/Documentation/topics/vif-plug-providers/index.rst b/Documentation/topics/vif-plug-providers/index.rst
> > new file mode 100644
> > index 000000000..b7552ac4c
> > --- /dev/null
> > +++ b/Documentation/topics/vif-plug-providers/index.rst
> > @@ -0,0 +1,32 @@
> > +..
> > +      Licensed under the Apache License, Version 2.0 (the "License"); you may
> > +      not use this file except in compliance with the License. You may obtain
> > +      a copy of the License at
> > +
> > +          http://www.apache.org/licenses/LICENSE-2.0
> > +
> > +      Unless required by applicable law or agreed to in writing, software
> > +      distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
> > +      WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
> > +      License for the specific language governing permissions and limitations
> > +      under the License.
> > +
> > +      Convention for heading levels in OVN documentation:
> > +
> > +      =======  Heading 0 (reserved for the title in a document)
> > +      -------  Heading 1
> > +      ~~~~~~~  Heading 2
> > +      +++++++  Heading 3
> > +      '''''''  Heading 4
> > +
> > +      Avoid deeper levels because they do not render well.
> > +
> > +==================
> > +VIF Plug Providers
> > +==================
> > +
> > +
> > +.. toctree::
> > +   :maxdepth: 2
> > +
> > +   vif-plug-providers
> > diff --git a/Documentation/topics/vif-plug-providers/vif-plug-providers.rst b/Documentation/topics/vif-plug-providers/vif-plug-providers.rst
> > new file mode 100644
> > index 000000000..77ecf7e0f
> > --- /dev/null
> > +++ b/Documentation/topics/vif-plug-providers/vif-plug-providers.rst
> > @@ -0,0 +1,209 @@
> > +..
> > +      Licensed under the Apache License, Version 2.0 (the "License"); you may
> > +      not use this file except in compliance with the License. You may obtain
> > +      a copy of the License at
> > +
> > +          http://www.apache.org/licenses/LICENSE-2.0
> > +
> > +      Unless required by applicable law or agreed to in writing, software
> > +      distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
> > +      WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
> > +      License for the specific language governing permissions and limitations
> > +      under the License.
> > +
> > +      Convention for heading levels in OVN documentation:
> > +
> > +      =======  Heading 0 (reserved for the title in a document)
> > +      -------  Heading 1
> > +      ~~~~~~~  Heading 2
> > +      +++++++  Heading 3
> > +      '''''''  Heading 4
> > +
> > +      Avoid deeper levels because they do not render well.
> > +
> > +==================
> > +VIF Plug Providers
> > +==================
> > +
> > +Traditionally it has been the CMSes responsibility to create VIFs as part of
> > +instance life cycle, and subsequently manage plug/unplug operations on the
> > +integration bridge following the conventions described in the
> > +`Open vSwitch Integration Guide`_ for mapping of VIFs to OVN logical port.
> > +
> > +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.
> > +
> > +Instead of every CMS having to develop their own version of an agent to do
> > +the plugging, we provide a pluggable infrastructure in OVN that allows the
> > +`ovn-controller` to perform the plugging on CMS direction.
> > +
> > +Hardware or platform specific details for initialization and lookup of
> > +representor ports is provided by an plugging provider library hosted inside or
> > +outside the core OVN repository, and linked at OVN build time.
> > +
> > +Life Cycle of an OVN plugged VIF
> > +--------------------------------
> > +
> > +1. CMS creates a record in the OVN Northbound Logical_Switch_Port table with
> > +   the options column containing the `vif-plug-type` key with a value
> > +   corresponding to the `const char *type` provided by the VIF plug provider
> > +   implementation as well as a `requested-chassis` key with a value pointing at
> > +   the name or hostname of the chassis it wants the VIF plugged on.  Additional
> > +   VIF plug provider specific key/value pairs must be provided for successful
> > +   lookup.
> > +
> > +2. `ovn-northd` looks up the name or hostname provided in the
> > +   `requested-chassis` option and fills the OVN Southbound Port_Binding
> > +   requested_chassis column, it also copies relevant options over to the
> > +   Port_Binding record.
> > +
> > +3. `ovn-controller` monitors Southbound Port_Binding entries with a
> > +   requested_chassis column pointing at its chassis UUID.  When it encounters
> > +   an entry with option `vif-plug-type` and it has registered a VIF plug
> > +   provider matching that type, it will act on it even if no local binding
> > +   exists yet.
> > +
> > +4. It will fill the `struct vif_plug_port_ctx_in` as defined in
> > +   `lib/vif-plug.h` with `op_type` set to 'PLUG_OP_CREATE' and make a call to
> > +   the VIF plug providers `vif_plug_port_prepare` callback function.  VIF plug
> > +   provider performs lookup and fills the `struct vif_plug_port_ctx_out` as
> > +   defined in `lib/vif-plug.h`.
> > +
> > +5. `ovn-controller` creates a port and interface record in the local OVSDB
> > +   using the details provided by the VIF plug provider and also adds
> > +   `external-ids:iface-id` with value matching the logical port name and
> > +   `external-ids:ovn-plugged` with value matching the logical port
> > +   `vif-plug-type`.  When the port creation is done a call will first be made
> > +   to the VIF plug providers `vif_plug_port_finish` function and then to the
> > +   `vif_plug_port_ctx_destroy` function to free any memory allocated by the VIF
> > +   plug implementation.
> > +
> > +6. The Open vSwitch vswitchd will assign a ofport to the newly created
> > +   interface and on the next `ovn-controller` main loop iteration flows will be
> > +   installed.
> > +
> > +7. On each main loop iteration the `ovn-controller` will in addition to normal
> > +   flow processing make a call to the VIF plug provider again similar to the
> > +   first creation in case anything needs updating for the interface record.
> > +
> > +8. The port will be unplugged when an event occurs which would make the
> > +   `ovn-controller` release a logical port, for example the Logical_Switch_Port
> > +   and Port_Binding entry disappearing from the database or its
> > +   `requested_chassis` column being pointed to a different chassis.
> > +
> > +
> > +The VIF plug provider interface
> > +-------------------------------
> > +
> > +The interface between internals of OVN and a VIF plug provider is a set of
> > +callbacks as defined by the `struct vif_plug_class` in
> > +`lib/vif-plug-provider.h`.
> > +
> > +It is important to note that these callbacks will be called in the critical
> > +path of the `ovn-controller` processing loop, so care must be taken to make the
> > +implementation as efficient as possible, and under no circumstance can any of
> > +the callback functions make calls that block.
> > +
> > +On `ovn-controller` startup, VIF plug providers made available at build time
> > +will be registered by the identifier provided in the `const char *type`
> > +pointer, at this time the `init` function pointer will be called if it is
> > +non-NULL.
> > +
> > +> **Note**: apart from the `const char *type` pointer, no attempt will be made
> > +            to access VIF plug provider data or functions before the call to
> > +            the `init` has been made.
> > +
> > +On `ovn-controller` exit, the VIF plug providers registered in the above
> > +mentioned procedure will have their `destroy` function pointer called if it is
> > +non-NULL.
> > +
> > +If the VIF plug provider has internal lookup tables that need to be maintained
> > +they can define a `run` function which will be called as part of the
> > +`ovn-controller` main loop.  If there are any changes encountered the function
> > +should return 'true' to signal that further processing is necessary, 'false'
> > +otherwise.
> > +
> > +On update of Interface records the `ovn-controller` will pass on a `sset`
> > +to the `ovsport_update_iface` function containing options the plug
> > +implementation finds pertinent to maintain for successful operation.  This
> > +`sset` is retrieved by making a call to the plug implementation
> > +`vif_plug_get_maintained_iface_options` function pointer if it is non-NULL.
> > +This allows presence of other users of the OVSDB maintaining a different set of
> > +options on the same set of Interface records without wiping out their changes.
> > +
> > +Before creating or updating an existing interface record the VIF plug provider
> > +`vif_plug_port_prepare` function pointer will be called with valid pointers to
> > +`struct vif_plug_port_ctx_in` and `struct vif_plug_port_ctx_out` data
> > +structures.  If the VIF plug provider implementation is able to perform lookup
> > +it should fill the `struct vif_plug_port_ctx_out` data structure and return
> > +'true'.  The `ovn-controller` will then create or update the port/interface
> > +records and then call `vif_plug_port_finish` when the transactions commits and
> > +`vif_plug_port_ctx_destroy` to free any allocated memory.  If the VIF plug
> > +provider implementation is unable to perform lookup or prepare the desired
> > +resource at this time, it should return 'false' which will tell the
> > +`ovn-controller` to not plug the port, in this case it will not call
> > +`vif_plug_port_finish` nor `vif_plug_port_ctx_destroy`.
> > +
> > +> **Note**: The VIF plug provider implementation should exhaust all
> > +            non-blocking options to succeed with lookup from within the
> > +            `vif_plug_port_prepare` handler, including refreshing lookup
> > +            tables if necessary.
> > +
> > +Before removing port and interface records previously plugged by the
> > +`ovn-controller` as identified by presence of the Interface
> > +`external-ids:ovn-plugged` key, the `ovn-controller` will look up the
> > +`vif-plug-type` from `external-ids:ovn-plugged`, fill
> > +`struct vif_plug_port_ctx_in` with `op_type` set to 'PLUG_OP_REMOVE' and make a
> > +call to `vif_plug_port_prepare`.  After the port and interface has been removed
> > +a call will be made to `vif_plug_port_finish`.  Both calls will be made with
> > +the pointer to `vif_plug_port_ctx_out` set to 'NULL', and no call will be made
> > +to `vif_plug_port_ctx_destroy`.
> > +
> > +Building with in-tree VIF plug providers
> > +----------------------------------------
> > +
> > +VIF plug providers hosted in the OVN repository live under
> > +`lib/vif-plug-providers`:
> > +
> > +To enable them, provide the `--enable-vif-plug-providers` command line option
> > +to the configure script when building OVN.
> > +
> > +Building with an externally provided VIF plug provider
> > +------------------------------------------------------
> > +
> > +There is also infrastructure in place to support linking OVN with an externally
> > +built VIF plug provider.
> > +
> > +This external VIF plug provider must define a NULL-terminated array of pointers
> > +to `struct vif_plug_class` data structures named `vif_plug_provider_classes`.
> > +Example:
> > +
> > +.. code-block:: none
> > +
> > +   const struct vif_plug_class *vif_plug_provider_classes[] = {
> > +       &vif_plug_foo,
> > +       NULL,
> > +   };
> > +
> > +The name of the repository for the external VIF plug provider should be the
> > +same as the name of the library it produces, and the built library artifact
> > +should be placed in lib/.libs.  Example:
> > +
> > +.. code-block:: none
> > +
> > +   ovn-vif-foo/
> > +   ovn-vif-foo/lib/.libs/libovn-vif-foo.la
> > +
> > +To enable such a VIF plug provider provide the
> > +`--with-vif-plug-provider=/path/to/ovn-vif-foo` command line option to the
> > +configure script when building OVN.
> > +
> > +.. LINKS
> > +.. _Open vSwitch Integration Guide:
> > +   https://docs.openvswitch.org/en/latest/topics/integration/
> > diff --git a/acinclude.m4 b/acinclude.m4
> > index e7f829520..3e4a54086 100644
> > --- a/acinclude.m4
> > +++ b/acinclude.m4
> > @@ -441,3 +441,52 @@ AC_DEFUN([OVN_CHECK_OVS], [
> >    AC_MSG_CHECKING([OVS version])
> >    AC_MSG_RESULT([$OVSVERSION])
> >  ])
> > +
> > +dnl OVN_CHECK_VIF_PLUG_PROVIDER
> > +dnl
> > +dnl Check for external VIF plug provider
> > +AC_DEFUN([OVN_CHECK_VIF_PLUG_PROVIDER], [
> > +  AC_ARG_VAR([VIF_PLUG_PROVIDER])
> > +  AC_ARG_WITH(
> > +    [vif-plug-provider],
> > +    [AC_HELP_STRING([--with-vif-plug-provider=/path/to/provider/repository],
> > +                    [Specify path to a configured and built VIF plug provider repository])],
> > +    [if test "$withval" = yes; then
> > +       if test -z "$VIF_PLUG_PROVIDER"; then
> > +         AC_MSG_ERROR([To build with external VIF plug provider, specify the path to a configured and built plug provider repository --with-vif-plug-provider or in \$VIF_PLUG_PROVIDER]),
> > +       fi
> > +       VIF_PLUG_PROVIDER="$(realpath $VIF_PLUG_PROVIDER)"
> > +     else
> > +       VIF_PLUG_PROVIDER="$(realpath $withval)"
> > +     fi
> > +     _vif_plug_provider_name="$(basename $VIF_PLUG_PROVIDER)"
> > +     if test ! -f "$VIF_PLUG_PROVIDER/lib/.libs/lib${_vif_plug_provider_name}.la"; then
> > +       AC_MSG_ERROR([$withval is not a configured and built VIF plug provider library repository])
> > +     fi
> > +     VIF_PLUG_PROVIDER_LDFLAGS="-L$VIF_PLUG_PROVIDER/lib/.libs -l$_vif_plug_provider_name"
> > +    ],
> > +    [VIF_PLUG_PROVIDER=no])
> > +  AC_MSG_CHECKING([for VIF plug provider])
> > +  AC_MSG_RESULT([$VIF_PLUG_PROVIDER])
> > +  AC_SUBST([VIF_PLUG_PROVIDER_LDFLAGS])
> > +  AM_CONDITIONAL([HAVE_VIF_PLUG_PROVIDER], [test "$VIF_PLUG_PROVIDER" != no])
> > +  if test "$VIF_PLUG_PROVIDER" != no; then
> > +    AC_DEFINE([HAVE_VIF_PLUG_PROVIDER], [1],
> > +              [Build and link with external VIF plug provider])
> > +  fi
> > +])
> > +
> > +dnl OVN_ENABLE_VIF_PLUG
> > +dnl
> > +dnl Enable built-in plug providers
> > +AC_DEFUN([OVN_ENABLE_VIF_PLUG], [
> > +    AC_ARG_ENABLE(
> > +      [vif-plug-providers],
> > +      [AC_HELP_STRING([--enable-vif-plug-providers], [Enable building of built-in VIF plug providers])],
> > +      [], [enable_vif_plug=no])
> > +    AM_CONDITIONAL([ENABLE_VIF_PLUG], [test "$enable_vif_plug" != no])
> > +    if test "$enable_vif_plug" != no; then
> > +      AC_DEFINE([ENABLE_VIF_PLUG], [1],
> > +                [Build built-in VIF plug providers])
> > +    fi
> > +])
> > diff --git a/configure.ac b/configure.ac
> > index d1b9b4d55..5a3a5987b 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -172,6 +172,8 @@ OVS_ENABLE_SPARSE
> >  OVS_CHECK_DDLOG([0.47])
> >  OVS_CHECK_PRAGMA_MESSAGE
> >  OVN_CHECK_OVS
> > +OVN_CHECK_VIF_PLUG_PROVIDER
> > +OVN_ENABLE_VIF_PLUG
> >  OVS_CTAGS_IDENTIFIERS
> >  AC_SUBST([OVS_CFLAGS])
> >  AC_SUBST([OVS_LDFLAGS])
> > diff --git a/controller/automake.mk b/controller/automake.mk
> > index ad2d68af2..9f9b49fe0 100644
> > --- a/controller/automake.mk
> > +++ b/controller/automake.mk
> > @@ -37,7 +37,9 @@ controller_ovn_controller_SOURCES = \
> >         controller/local_data.c \
> >         controller/local_data.h \
> >         controller/ovsport.h \
> > -       controller/ovsport.c
> > +       controller/ovsport.c \
> > +       controller/vif-plug.h \
> > +       controller/vif-plug.c
> >
> >  controller_ovn_controller_LDADD = lib/libovn.la $(OVS_LIBDIR)/libopenvswitch.la
> >  man_MANS += controller/ovn-controller.8
> > diff --git a/controller/test-vif-plug.c b/controller/test-vif-plug.c
> > new file mode 100644
> > index 000000000..01ff37d8f
> > --- /dev/null
> > +++ b/controller/test-vif-plug.c
> > @@ -0,0 +1,72 @@
> > +/* Copyright (c) 2021, Canonical
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + *     http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#include <config.h>
> > +#include <errno.h>
> > +
> > +#include "vif-plug.h"
> > +#include "vif-plug-provider.h"
> > +#include "smap.h"
> > +#include "sset.h"
> > +#include "tests/ovstest.h"
> > +
> > +static void
> > +test_vif_plug(struct ovs_cmdl_context *ctx OVS_UNUSED)
> > +{
> > +    const struct vif_plug_class *vif_plug_class;
> > +
> > +    ovs_assert(vif_plug_provider_unregister("dummy") == EINVAL);
> > +
> > +    ovs_assert(!vif_plug_provider_register(&vif_plug_dummy_class));
> > +    vif_plug_class = vif_plug_provider_get("dummy");
> > +    ovs_assert(vif_plug_provider_register(&vif_plug_dummy_class) == EEXIST);
> > +
> > +    ovs_assert(
> > +        sset_contains(
> > +            vif_plug_get_maintained_iface_options(vif_plug_class),
> > +            "plug-dummy-option"));
> > +
> > +    struct vif_plug_port_ctx_in ctx_in = {
> > +        .op_type = PLUG_OP_CREATE,
> > +        .lport_name = "lsp1",
> > +        .lport_options = SMAP_INITIALIZER(&ctx_in.lport_options),
> > +    };
> > +    struct vif_plug_port_ctx_out ctx_out;
> > +    vif_plug_port_prepare(vif_plug_class, &ctx_in, &ctx_out);
> > +    ovs_assert(!strcmp(ctx_out.name, "lsp1"));
> > +    ovs_assert(!strcmp(ctx_out.type, "internal"));
> > +    ovs_assert(!strcmp(smap_get(
> > +            &ctx_out.iface_options, "vif-plug-dummy-option"), "value"));
> > +
> > +    vif_plug_port_finish(vif_plug_class, &ctx_in, &ctx_out);
> > +    vif_plug_port_ctx_destroy(vif_plug_class, &ctx_in, &ctx_out);
> > +    vif_plug_provider_destroy_all();
> > +}
> > +
> > +static void
> > +test_vif_plug_main(int argc, char *argv[])
> > +{
> > +    set_program_name(argv[0]);
> > +    static const struct ovs_cmdl_command commands[] = {
> > +        {"run", NULL, 0, 0, test_vif_plug, OVS_RO},
> > +        {NULL, NULL, 0, 0, NULL, OVS_RO},
> > +    };
> > +    struct ovs_cmdl_context ctx;
> > +    ctx.argc = argc - 1;
> > +    ctx.argv = argv + 1;
> > +    ovs_cmdl_run_command(&ctx, commands);
> > +}
> > +
> > +OVSTEST_REGISTER("test-vif-plug", test_vif_plug_main);
> > diff --git a/controller/vif-plug.c b/controller/vif-plug.c
> > new file mode 100644
> > index 000000000..2cd7cd09a
> > --- /dev/null
> > +++ b/controller/vif-plug.c
> > @@ -0,0 +1,632 @@
> > +/*
> > + * Copyright (c) 2021 Canonical
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + *     http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#include <config.h>
> > +
> > +/* OVS includes */
> > +#include "lib/vswitch-idl.h"
> > +#include "openvswitch/shash.h"
> > +#include "openvswitch/vlog.h"
> > +
> > +/* OVN includes */
> > +#include "binding.h"
> > +#include "lib/ovn-sb-idl.h"
> > +#include "lport.h"
> > +#include "ovsport.h"
> > +#include "vif-plug.h"
> > +#include "vif-plug-provider.h"
> > +
> > +VLOG_DEFINE_THIS_MODULE(vif_plug);
> > +
> > +#define OVN_PLUGGED_EXT_ID "ovn-plugged"
> > +#define VIF_PLUG_OPTION_TYPE "vif-plug-type"
> > +#define VIF_PLUG_OPTION_MTU_REQUEST "vif-plug-mtu-request"
> > +
> > +void
> > +vif_plug_register_ovs_idl(struct ovsdb_idl *ovs_idl)
> > +{
> > +    ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_mtu_request);
> > +}
> > +
> > +/* Get the class level 'maintained_iface_options' set. */
> > +const struct sset *
> > +vif_plug_get_maintained_iface_options(
> > +        const struct vif_plug_class *vif_plug_class)
> > +{
> > +    return vif_plug_class->vif_plug_get_maintained_iface_options ?
> > +           vif_plug_class->vif_plug_get_maintained_iface_options() : NULL;
> > +}
> > +
> > +/* Prepare the logical port as identified by 'ctx_in' for port creation, update
> > + * or removal as specified by 'ctx_in->op_type'.
> > + *
> > + * When 'ctx_in->op_type' is PLUG_OP_CREATE the plug implementation must fill
> > + * 'ctx_out' with data to apply to the interface record maintained by OVN on
> > + * its behalf.
> > + *
> > + * When 'ctx_in_op_type' is PLUG_OP_REMOVE 'ctx_out' should be set to NULL and
> > + * the plug implementation must not attempt to use 'ctx_out'.
> > + *
> > + * The data in 'ctx_out' is owned by the plug implementation, and a call must
> > + * be made to vif_plug_port_ctx_destroy when done with it. */
> > +bool
> > +vif_plug_port_prepare(const struct vif_plug_class *vif_plug_class,
> > +                      const struct vif_plug_port_ctx_in *ctx_in,
> > +                      struct vif_plug_port_ctx_out *ctx_out)
> > +{
> > +    return vif_plug_class->vif_plug_port_prepare(ctx_in, ctx_out);
> > +}
> > +
> > +/* Notify the VIF plug implementation that a port creation, update or removal
> > + * has been committed to the database. */
> > +void
> > +vif_plug_port_finish(const struct vif_plug_class *vif_plug_class,
> > +                     const struct vif_plug_port_ctx_in *ctx_in,
> > +                     struct vif_plug_port_ctx_out *ctx_out)
> > +{
> > +    vif_plug_class->vif_plug_port_finish(ctx_in, ctx_out);
> > +}
> > +
> > +/* Free any data allocated to 'ctx_out' in a prevous call to
> > + * vif_plug_port_prepare. */
> > +void
> > +vif_plug_port_ctx_destroy(const struct vif_plug_class *vif_plug_class,
> > +                          const struct vif_plug_port_ctx_in *ctx_in,
> > +                          struct vif_plug_port_ctx_out *ctx_out)
> > +{
> > +    vif_plug_class->vif_plug_port_ctx_destroy(ctx_in, ctx_out);
> > +}
> > +
> > +static struct vif_plug_port_ctx *
> > +build_port_ctx(const struct vif_plug_class *vif_plug,
> > +                  const enum vif_plug_op_type op_type,
> > +                  const struct vif_plug_ctx_in *vif_plug_ctx_in,
> > +                  const struct sbrec_port_binding *pb,
> > +                  const struct ovsrec_interface *iface,
> > +                  const char *iface_id)
> > +{
> > +    struct vif_plug_port_ctx *new_ctx = xzalloc(
> > +        sizeof *new_ctx);
> > +
> > +    new_ctx->vif_plug = vif_plug;
> > +    new_ctx->vif_plug_port_ctx_in.op_type = op_type;
> > +    new_ctx->vif_plug_port_ctx_in.ovs_table = vif_plug_ctx_in->ovs_table;
> > +    new_ctx->vif_plug_port_ctx_in.br_int = vif_plug_ctx_in->br_int;
> > +    new_ctx->vif_plug_port_ctx_in.lport_name = pb ?
> > +        xstrdup(pb->logical_port) : iface_id ? xstrdup(iface_id) : NULL;
> > +    /* Prepare vif_plug_port_ctx_in smaps for use.
> > +     *
> > +     * Note that smap_init does not allocate memory.  Any memory allocated by
> > +     * putting data into the vif_plug_port_ctx_in smaps will be destroyed by
> > +     * calls to smap_destroy in destroy_port_ctx */
> > +    smap_init(&new_ctx->vif_plug_port_ctx_in.lport_options);
> > +    smap_init(&new_ctx->vif_plug_port_ctx_in.iface_options);
> > +
> > +    if (pb) {
> > +        smap_clone(&new_ctx->vif_plug_port_ctx_in.lport_options,
> > +                   &pb->options);
> > +    }
> > +
> > +    if (iface) {
> > +        new_ctx->vif_plug_port_ctx_in.iface_name = xstrdup(iface->name);
> > +        new_ctx->vif_plug_port_ctx_in.iface_type = xstrdup(iface->type);
> > +        smap_clone(&new_ctx->vif_plug_port_ctx_in.iface_options,
> > +                   &iface->options);
> > +    }
> > +
> > +    /* Prepare vif_plug_port_ctx_out smaps for use.
> > +     *
> > +     * Note that smap_init does not allocate memory.  Any memory allocated by
> > +     * putting data into the vif_plug_port_ctx_out smaps is the responsibility
> > +     * of the VIF plug provider through a call to vif_plug_port_ctx_destroy. */
> > +    smap_init(&new_ctx->vif_plug_port_ctx_out.iface_options);
> > +
> > +    return new_ctx;
> > +}
> > +
> > +static void
> > +destroy_port_ctx(struct vif_plug_port_ctx *ctx)
> > +{
> > +    smap_destroy(&ctx->vif_plug_port_ctx_in.lport_options);
> > +    smap_destroy(&ctx->vif_plug_port_ctx_in.iface_options);
> > +    if (ctx->vif_plug_port_ctx_in.lport_name) {
> > +        free((char *)ctx->vif_plug_port_ctx_in.lport_name);
> > +    }
> > +    if (ctx->vif_plug_port_ctx_in.iface_name) {
> > +        free((char *)ctx->vif_plug_port_ctx_in.iface_name);
> > +    }
> > +    if (ctx->vif_plug_port_ctx_in.iface_type) {
> > +        free((char *)ctx->vif_plug_port_ctx_in.iface_type);
> > +    }
> > +    /* Note that data associated with ctx->vif_plug_port_ctx_out must be
> > +     * destroyed by the plug provider implementation with a call to
> > +     * vif_plug_port_ctx_destroy prior to calling this function */
> > +    free(ctx);
> > +}
> > +
> > +/* When we add deletion of rows to the transaction, the data structures
> > + * associated with the rows will immediately be freed from the IDL, and as
> > + * such we can no longer access them.
> > + *
> > + * Since IDL commits are handled asynchronously we can have a few engine
> > + * iterations where the deleted data shows up when iterating over table
> > + * contents, but the IDL *_is_deleted() call will not reliably categorize the
> > + * data as deleted.  This is in contrast to the IDL behaviour when some other
> > + * process deletes data from the database, so this may be an OVS IDL bug, or it
> > + * could be it's just expected that the program consuming the IDL will know not
> > + * to access rows it has deleted.
> > + *
>
> I have some concerns for this part. Firstly, let me clarify that *_is_deleted() is not supposed to be used to check if a row is deleted in current idl loop. It is instead used for telling the type of a change for a *change-tracked* row - is it deleted or newly created, etc. So this is not a bug (but probably documentation can be enhanced). However, I don't understand why would it be possible to access deleted rows in the current IDL iteration. The statement "we can have a few engine iterations where the deleted data shows up when iterating over table contents" seems unexpected behavior. It should never happen. If you encountered such behavior it may indicate something more serious, such as, did we forget to clear any references to rows somewhere? If that's the case, there can be dangling pointer access somewhere else before calling this function. It is better to make sure that doesn't happen. Did you see such problems?

Thank you for the clarification on use of *_is_deleted().

Revisiting this now I have to admit that the statement does not appear
to be true, and that I met these problems before properly addressing
the asynchronous behaviour of the code, i.e. Previous iterations of it
did not really take into account the fact that it may take some
iterations for the transaction to actually commit, and what the code
did in that situation was to try to delete the same record multiple
times, which of course will not work and cause a crash.

The shash'es are still required to follow each operation through to
commit/failure to commit, but I will amend this comment.

Just to confirm I added this:
diff --git a/controller/vif-plug.c b/controller/vif-plug.c
index 450b4aa2f..4cce3fa0e 100644
--- a/controller/vif-plug.c
+++ b/controller/vif-plug.c
@@ -570,6 +570,16 @@ vif_plug_run(struct vif_plug_ctx_in *vif_plug_ctx_in,
         vif_plug_handle_iface(iface_rec, vif_plug_ctx_in, vif_plug_ctx_out,
                               !vif_plug_prime_idl_count);
     }
+    OVSREC_INTERFACE_TABLE_FOR_EACH (iface_rec,
+                                     vif_plug_ctx_in->iface_table) {
+        VLOG_INFO("HELLO iface_rec->name=%s", iface_rec->name);
+    }
+    OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface_rec,
+                                             vif_plug_ctx_in->iface_table) {
+        VLOG_INFO("HELLO tracked iface_rec->name=%s is_deleted=%d",
+                  iface_rec->name,
+                  ovsrec_interface_is_deleted(iface_rec));
+    }

     struct sbrec_port_binding *target =
         sbrec_port_binding_index_init_row(

Which will have the controller log this:
2021-11-05T11:34:25.964Z|00155|vif_plug|INFO|HELLO iface_rec->name=lsp1
2021-11-05T11:34:25.964Z|00156|vif_plug|INFO|HELLO iface_rec->name=ovn-hv2-0
2021-11-05T11:34:25.964Z|00157|vif_plug|INFO|HELLO iface_rec->name=br-phys
2021-11-05T11:34:25.964Z|00158|vif_plug|INFO|HELLO iface_rec->name=br-phys_n1
2021-11-05T11:34:25.964Z|00159|vif_plug|INFO|HELLO iface_rec->name=br-int
2021-11-05T11:34:25.964Z|00160|vif_plug|INFO|HELLO tracked
iface_rec->name=lsp1 is_deleted=0
2021-11-05T11:34:25.983Z|00161|binding|INFO|Not claiming lport lsp1,
chassis hv1 requested-chassis (option points at non-existent chassis)
2021-11-05T11:34:25.983Z|00162|binding|INFO|Releasing lport lsp1 from
this chassis.
2021-11-05T11:34:25.983Z|00163|vif_plug|INFO|Unplugging port lsp1 from
br-int for iface-id lsp1 on this chassis.
2021-11-05T11:34:25.983Z|00164|vif_plug|INFO|HELLO iface_rec->name=ovn-hv2-0
2021-11-05T11:34:25.983Z|00165|vif_plug|INFO|HELLO iface_rec->name=br-phys
2021-11-05T11:34:25.983Z|00166|vif_plug|INFO|HELLO iface_rec->name=br-phys_n1
2021-11-05T11:34:25.983Z|00167|vif_plug|INFO|HELLO iface_rec->name=br-int
2021-11-05T11:34:25.984Z|00168|binding|INFO|Removing lport lsp1
ovn-installed in OVS
2021-11-05T11:34:25.984Z|00169|binding|INFO|Setting lport lsp1 down in
Southbound
2021-11-05T11:34:25.985Z|00170|vif_plug|INFO|HELLO iface_rec->name=ovn-hv2-0
2021-11-05T11:34:25.985Z|00171|vif_plug|INFO|HELLO iface_rec->name=br-phys
2021-11-05T11:34:25.985Z|00172|vif_plug|INFO|HELLO iface_rec->name=br-phys_n1
2021-11-05T11:34:25.985Z|00173|vif_plug|INFO|HELLO iface_rec->name=br-int
2021-11-05T11:34:25.985Z|00174|vif_plug|INFO|HELLO tracked
iface_rec->name=lsp1 is_deleted=1




> > + * To deal with this, we keep a reference for ourself to avoid attempting to
> > + * remove the same data multiple times while waiting for the transaction to
> > + * commit.  The tracking data will be cleared after commit at the end of the
> > + * ovn-controller main loop.
> > + */
> > +static void
> > +transact_delete_port(const struct vif_plug_ctx_in *vif_plug_ctx_in,
> > +                     const struct vif_plug_ctx_out *vif_plug_ctx_out,
> > +                     const struct vif_plug_port_ctx *vif_plug_port_ctx,
> > +                     const struct ovsrec_port *port)
> > +{
> > +    shash_add(vif_plug_ctx_out->deleted_iface_ids,
> > +              vif_plug_port_ctx->vif_plug_port_ctx_in.lport_name,
> > +              vif_plug_port_ctx);
> > +    ovsport_remove(vif_plug_ctx_in->br_int, port);
> > +}
> > +
> > +static void
> > +transact_create_port(const struct vif_plug_ctx_in *vif_plug_ctx_in,
> > +                     const struct vif_plug_ctx_out *vif_plug_ctx_out,
> > +                     const struct vif_plug_port_ctx *vif_plug_port_ctx,
> > +                     const struct smap *iface_external_ids,
> > +                     const int64_t mtu_request)
> > +{
> > +    shash_add(vif_plug_ctx_out->changed_iface_ids,
> > +              vif_plug_port_ctx->vif_plug_port_ctx_in.lport_name,
> > +              vif_plug_port_ctx);
> > +    ovsport_create(vif_plug_ctx_in->ovs_idl_txn, vif_plug_ctx_in->br_int,
> > +                   vif_plug_port_ctx->vif_plug_port_ctx_out.name,
> > +                   vif_plug_port_ctx->vif_plug_port_ctx_out.type,
> > +                   NULL, iface_external_ids,
> > +                   &vif_plug_port_ctx->vif_plug_port_ctx_out.iface_options,
> > +                   mtu_request);
> > +}
> > +
> > +static void
> > +transact_update_port(const struct ovsrec_interface *iface_rec,
> > +                     const struct vif_plug_ctx_in *vif_plug_ctx_in OVS_UNUSED,
> > +                     const struct vif_plug_ctx_out *vif_plug_ctx_out,
> > +                     const struct vif_plug_port_ctx *vif_plug_port_ctx,
> > +                     const struct smap *iface_external_ids,
> > +                     const int64_t mtu_request)
> > +{
> > +    shash_add(vif_plug_ctx_out->changed_iface_ids,
> > +              vif_plug_port_ctx->vif_plug_port_ctx_in.lport_name,
> > +              vif_plug_port_ctx);
> > +    ovsport_update_iface(
> > +        iface_rec,
> > +        vif_plug_port_ctx->vif_plug_port_ctx_out.type,
> > +        iface_external_ids,
> > +        NULL,
> > +        &vif_plug_port_ctx->vif_plug_port_ctx_out.iface_options,
> > +        vif_plug_get_maintained_iface_options(
> > +            vif_plug_port_ctx->vif_plug),
> > +        mtu_request);
> > +}
> > +
> > +
> > +static bool
> > +consider_unplug_iface(const struct ovsrec_interface *iface,
> > +                      const struct sbrec_port_binding *pb,
> > +                      struct vif_plug_ctx_in *vif_plug_ctx_in,
> > +                      struct vif_plug_ctx_out *vif_plug_ctx_out)
> > +{
> > +    const char *vif_plug_type = smap_get(&iface->external_ids,
> > +                                         OVN_PLUGGED_EXT_ID);
> > +    const char *iface_id = smap_get(&iface->external_ids, "iface-id");
> > +    const struct ovsrec_port *port = ovsport_lookup_by_interface(
> > +        vif_plug_ctx_in->ovsrec_port_by_interfaces,
> > +        (struct ovsrec_interface *) iface);
> > +
> > +    if (vif_plug_type && iface_id && port) {
> > +        const struct vif_plug_class *vif_plug;
> > +        if (!(vif_plug = vif_plug_provider_get(vif_plug_type))) {
> > +            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > +            VLOG_WARN_RL(&rl,
> > +                         "Unable to open VIF plug provider for "
> > +                         "%s %s iface-id %s",
> > +                         VIF_PLUG_OPTION_TYPE, vif_plug_type, iface_id);
> > +            /* While we are unable to handle this, asking for a recompute
> > +             * will not change that fact. */
> > +            return true;
> > +        }
> > +        if (!vif_plug_ctx_in->chassis_rec || !vif_plug_ctx_in->br_int
> > +            || !vif_plug_ctx_in->ovs_idl_txn)
> > +        {
> > +            /* Some of our prerequisites are not available, ask for a
> > +             * recompute. */
> > +            return false;
> > +        }
> > +
> > +        /* Our contract with the VIF plug provider is that vif_plug_port_finish
> > +         * will be called with a vif_plug_port_ctx_in object once the
> > +         * transaction commits.
> > +         *
> > +         * Since this happens asynchronously we need to allocate memory for
> > +         * and duplicate any database references so that they stay valid.
> > +         *
> > +         * The data is freed with a call to destroy_port_ctx after the
> > +         * transaction completes at the end of the ovn-controller main
> > +         * loop. */
> > +        struct vif_plug_port_ctx *vif_plug_port_ctx = build_port_ctx(
> > +            vif_plug, PLUG_OP_REMOVE, vif_plug_ctx_in, pb, iface, iface_id);
> > +
> > +        if (!vif_plug_port_prepare(vif_plug,
> > +                                   &vif_plug_port_ctx->vif_plug_port_ctx_in,
> > +                                   NULL)) {
> > +            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > +            VLOG_INFO_RL(&rl,
> > +                         "Not unplugging iface %s (iface-id %s) on direction "
> > +                         "from VIF plug provider.",
> > +                         iface->name, iface_id);
> > +            destroy_port_ctx(vif_plug_port_ctx);
> > +            return true;
> > +        }
> > +        VLOG_INFO("Unplugging port %s from %s for iface-id %s on this "
> > +                  "chassis.",
> > +                  port->name,
> > +                  vif_plug_ctx_in->br_int->name,
> > +                  iface_id);
> > +
> > +        /* Add and track delete operation to the transaction */
> > +        transact_delete_port(vif_plug_ctx_in, vif_plug_ctx_out,
> > +                             vif_plug_port_ctx, port);
> > +        return true;
> > +    }
> > +    return true;
> > +}
> > +
> > +static int64_t
> > +get_plug_mtu_request(const struct smap *lport_options)
> > +{
> > +    return smap_get_int(lport_options, VIF_PLUG_OPTION_MTU_REQUEST, 0);
> > +}
> > +
> > +static bool
> > +consider_plug_lport_create__(const struct vif_plug_class *vif_plug,
> > +                             const struct smap *iface_external_ids,
> > +                             const struct sbrec_port_binding *pb,
> > +                             struct vif_plug_ctx_in *vif_plug_ctx_in,
> > +                             struct vif_plug_ctx_out *vif_plug_ctx_out)
> > +{
> > +    if (!vif_plug_ctx_in->chassis_rec || !vif_plug_ctx_in->br_int
> > +        || !vif_plug_ctx_in->ovs_idl_txn) {
> > +        /* Some of our prerequisites are not available, ask for a recompute. */
> > +        return false;
> > +    }
> > +
> > +    /* Our contract with the VIF plug provider is that vif_plug_port_finish
> > +     * will be called with vif_plug_port_ctx_in and vif_plug_port_ctx_out
> > +     * objects once the transaction commits.
> > +     *
> > +     * Since this happens asynchronously we need to allocate memory for
> > +     * and duplicate any database references so that they stay valid.
> > +     *
> > +     * The data is freed with a call to destroy_port_ctx after the
> > +     * transaction completes at the end of the ovn-controller main
> > +     * loop. */
> > +    struct vif_plug_port_ctx *vif_plug_port_ctx = build_port_ctx(
> > +        vif_plug, PLUG_OP_CREATE, vif_plug_ctx_in, pb, NULL, NULL);
> > +
> > +    if (!vif_plug_port_prepare(vif_plug,
> > +                           &vif_plug_port_ctx->vif_plug_port_ctx_in,
> > +                           &vif_plug_port_ctx->vif_plug_port_ctx_out)) {
> > +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > +        VLOG_INFO_RL(&rl,
> > +                     "Not plugging lport %s on direction from VIF plug "
> > +                     "provider.",
> > +                     pb->logical_port);
> > +        destroy_port_ctx(vif_plug_port_ctx);
> > +        return true;
> > +    }
> > +
> > +    VLOG_INFO("Plugging port %s into %s for lport %s on this "
> > +              "chassis.",
> > +              vif_plug_port_ctx->vif_plug_port_ctx_out.name,
> > +              vif_plug_ctx_in->br_int->name,
> > +              pb->logical_port);
> > +    transact_create_port(vif_plug_ctx_in, vif_plug_ctx_out,
> > +                         vif_plug_port_ctx,
> > +                         iface_external_ids,
> > +                         get_plug_mtu_request(&pb->options));
> > +    return true;
> > +}
> > +
> > +static bool
> > +consider_plug_lport_update__(const struct vif_plug_class *vif_plug,
> > +                             const struct smap *iface_external_ids,
> > +                             const struct sbrec_port_binding *pb,
> > +                             struct local_binding *lbinding,
> > +                             struct vif_plug_ctx_in *vif_plug_ctx_in,
> > +                             struct vif_plug_ctx_out *vif_plug_ctx_out)
> > +{
> > +    if (!vif_plug_ctx_in->chassis_rec || !vif_plug_ctx_in->br_int
> > +        || !vif_plug_ctx_in->ovs_idl_txn) {
> > +        /* Some of our prerequisites are not available, ask for a recompute. */
> > +        return false;
> > +    }
> > +    /* Our contract with the VIF plug provider is that vif_plug_port_finish
> > +     * will be called with vif_plug_port_ctx_in and vif_plug_port_ctx_out
> > +     * objects once the transaction commits.
> > +     *
> > +     * Since this happens asynchronously we need to allocate memory for
> > +     * and duplicate any database references so that they stay valid.
> > +     *
> > +     * The data is freed with a call to destroy_port_ctx after the
> > +     * transaction completes at the end of the ovn-controller main
> > +     * loop. */
> > +    struct vif_plug_port_ctx *vif_plug_port_ctx = build_port_ctx(
> > +        vif_plug, PLUG_OP_CREATE, vif_plug_ctx_in, pb, NULL, NULL);
> > +
> > +    if (!vif_plug_port_prepare(vif_plug,
> > +                               &vif_plug_port_ctx->vif_plug_port_ctx_in,
> > +                               &vif_plug_port_ctx->vif_plug_port_ctx_out)) {
> > +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > +        VLOG_INFO_RL(&rl,
> > +                     "Not updating lport %s on direction from VIF plug "
> > +                     "provider.",
> > +                     pb->logical_port);
> > +        destroy_port_ctx(vif_plug_port_ctx);
> > +        return true;
> > +    }
> > +
> > +    if (strcmp(lbinding->iface->name,
> > +               vif_plug_port_ctx->vif_plug_port_ctx_out.name)) {
> > +        VLOG_WARN("Attempt of incompatible change to existing "
> > +                  "port detected, please recreate port: %s",
> > +                   pb->logical_port);
> > +        vif_plug_port_ctx_destroy(vif_plug,
> > +                                  &vif_plug_port_ctx->vif_plug_port_ctx_in,
> > +                                  &vif_plug_port_ctx->vif_plug_port_ctx_out);
> > +        destroy_port_ctx(vif_plug_port_ctx);
> > +        return false;
> > +    }
> > +    VLOG_DBG("updating iface for: %s", pb->logical_port);
> > +    transact_update_port(lbinding->iface, vif_plug_ctx_in, vif_plug_ctx_out,
> > +                         vif_plug_port_ctx, iface_external_ids,
> > +                         get_plug_mtu_request(&pb->options));
> > +
> > +    return true;
> > +}
> > +
> > +static bool
> > +consider_plug_lport(const struct sbrec_port_binding *pb,
> > +                    struct local_binding *lbinding,
> > +                    struct vif_plug_ctx_in *vif_plug_ctx_in,
> > +                    struct vif_plug_ctx_out *vif_plug_ctx_out)
> > +{
> > +    bool ret = true;
> > +    if (lport_can_bind_on_this_chassis(vif_plug_ctx_in->chassis_rec, pb)
> > +        && pb->requested_chassis == vif_plug_ctx_in->chassis_rec) {
> > +        const char *vif_plug_type = smap_get(&pb->options,
> > +                                             VIF_PLUG_OPTION_TYPE);
> > +        if (!vif_plug_type) {
> > +            /* Nothing for us to do and we don't need a recompute. */
> > +            return true;
> > +        }
> > +
> > +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > +        const struct vif_plug_class *vif_plug;
> > +        if (!(vif_plug = vif_plug_provider_get(vif_plug_type))) {
> > +            VLOG_WARN_RL(&rl,
> > +                         "Unable to open VIF plug provider for %s: '%s' "
> > +                         "lport %s",
> > +                         VIF_PLUG_OPTION_TYPE,
> > +                         vif_plug_type,
> > +                         pb->logical_port);
> > +            /* While we are unable to handle this, asking for a recompute will
> > +             * not change that fact. */
> > +            return true;
> > +        }
> > +        const struct smap iface_external_ids = SMAP_CONST2(
> > +                &iface_external_ids,
> > +                OVN_PLUGGED_EXT_ID, vif_plug_type,
> > +                "iface-id", pb->logical_port);
> > +        if (lbinding && lbinding->iface) {
> > +            if (!smap_get(&lbinding->iface->external_ids,
> > +                          OVN_PLUGGED_EXT_ID))
> > +            {
> > +                VLOG_WARN_RL(&rl,
> > +                             "CMS requested plugging of lport %s, but a port "
> > +                             "that is not maintained by OVN already exsist "
> > +                             "in local vSwitch: "UUID_FMT,
> > +                             pb->logical_port,
> > +                             UUID_ARGS(&lbinding->iface->header_.uuid));
> > +                return false;
> > +            }
> > +            ret = consider_plug_lport_update__(vif_plug, &iface_external_ids,
> > +                                               pb, lbinding, vif_plug_ctx_in,
> > +                                               vif_plug_ctx_out);
> > +        } else {
> > +            ret = consider_plug_lport_create__(vif_plug, &iface_external_ids,
> > +                                               pb, vif_plug_ctx_in,
> > +                                               vif_plug_ctx_out);
> > +        }
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> > +static bool
> > +vif_plug_iface_touched_this_txn(
> > +        const struct vif_plug_ctx_out *vif_plug_ctx_out,
> > +        const char *iface_id)
> > +{
> > +    return shash_find(vif_plug_ctx_out->changed_iface_ids, iface_id)
> > +           || shash_find(vif_plug_ctx_out->deleted_iface_ids, iface_id);
> > +}
> > +
> > +static bool
> > +vif_plug_handle_lport_vif(const struct sbrec_port_binding *pb,
> > +                          struct vif_plug_ctx_in *vif_plug_ctx_in,
> > +                          struct vif_plug_ctx_out *vif_plug_ctx_out)
> > +{
> > +    if (vif_plug_iface_touched_this_txn(vif_plug_ctx_out, pb->logical_port)) {
> > +        return true;
> > +    }
> > +    bool handled = true;
> > +    struct local_binding *lbinding = local_binding_find(
> > +        vif_plug_ctx_in->local_bindings, pb->logical_port);
> > +
> > +    if (lport_can_bind_on_this_chassis(vif_plug_ctx_in->chassis_rec, pb)) {
> > +        handled &= consider_plug_lport(pb, lbinding,
> > +                                       vif_plug_ctx_in, vif_plug_ctx_out);
> > +    } else if (lbinding && lbinding->iface) {
> > +        handled &= consider_unplug_iface(lbinding->iface, pb,
> > +                                         vif_plug_ctx_in, vif_plug_ctx_out);
> > +    }
> > +    return handled;
> > +}
> > +
> > +static bool
> > +vif_plug_handle_iface(const struct ovsrec_interface *iface_rec,
> > +                      struct vif_plug_ctx_in *vif_plug_ctx_in,
> > +                      struct vif_plug_ctx_out *vif_plug_ctx_out)
> > +{
> > +    bool handled = true;
> > +    const char *vif_plug_type = smap_get(&iface_rec->external_ids,
> > +                                         OVN_PLUGGED_EXT_ID);
> > +    const char *iface_id = smap_get(&iface_rec->external_ids, "iface-id");
> > +    if (!vif_plug_type || !iface_id
> > +        || vif_plug_iface_touched_this_txn(vif_plug_ctx_out, iface_id)) {
> > +        return true;
> > +    }
> > +    struct local_binding *lbinding = local_binding_find(
> > +        vif_plug_ctx_in->local_bindings, iface_id);
> > +    const struct sbrec_port_binding *pb = lport_lookup_by_name(
> > +        vif_plug_ctx_in->sbrec_port_binding_by_name, iface_id);
> > +    if (pb && lbinding
> > +        && lport_can_bind_on_this_chassis(vif_plug_ctx_in->chassis_rec, pb)) {
> > +        /* Something changed on a interface we have previously plugged,
> > +         * consider updating it */
> > +        handled &= consider_plug_lport(pb, lbinding,
> > +                                       vif_plug_ctx_in, vif_plug_ctx_out);
> > +    } else if (!pb
> > +               || !lport_can_bind_on_this_chassis(
> > +                       vif_plug_ctx_in->chassis_rec, pb)) {
> > +        /* No lport for this interface or it is destined for different chassis,
> > +         * consuder unplugging it */
> > +        handled &= consider_unplug_iface(iface_rec, pb,
> > +                                         vif_plug_ctx_in, vif_plug_ctx_out);
> > +    }
> > +    return handled;
> > +}
> > +
> > +void
> > +vif_plug_run(struct vif_plug_ctx_in *vif_plug_ctx_in,
> > +             struct vif_plug_ctx_out *vif_plug_ctx_out)
> > +{
> > +    /* 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 naive check so that we do not erronously unplug ports
> > +     * because the data is just not loaded yet.
> > +     */
>
> It seems this approach still has some problems.
>
> When the process starts, it is good that vif_plug_run() is called only if both br_int and chassis are not NULL, which means at least both OVS and SB DBs are connected. And then with "count" number of iterations we expect all the initial data downloading has completed, so the "count" need to be big enough. Assume that 3 is big enough, but in fact data downloading completed at the first iteration, and then we created a new PB in SB DB which triggers the second iteration. However, depends on the inactivity-probe-interval (it is configurable for SB DB connection, and in large scale environment this is usually configured to be very long, e.g. 3 min), it has to wait for ~3 min before the newly created PB can be handled by the VIF plug. If we found out that in some cases 3 is not enough and then we increase it to 10 for the worse case scenario, it means in best case scenarios - say, the downloading completes in 3 iterations, then we will have to wait for 7 * 3 = ~20min before our newly created PB during this 20 min to take effect. So I would use the counter only for deleting VIFs, but for creating VIFs I think it is better to handle it asap without checking this counter.
>
> Another problem is when connection is lost and then reconnected. In most cases the IDL content can be persisted and only download the delta when reconnected, but when it is bad luck, the IDL data would be cleared and re-download everything. So there is a chance the VIFs will be deleted since this counter here is 0 already. I think the count need to be reset to N whenever IDL data is cleared.

Good point! I increased the counter value to 10 to be sure, as well as
resetting it whenver we detect reconnection to OVNSB and only applying
the check to unplug operations:
 --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index b4d4636d8..f223e0f09 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -3504,6 +3500,7 @@ main(int argc, char *argv[])
             if (!new_ovnsb_cond_seqno) {
                 VLOG_INFO("OVNSB IDL reconnected, force recompute.");
                 engine_set_force_recompute(true);
+                vif_plug_reset_idl_prime_counter();
             }
             ovnsb_cond_seqno = new_ovnsb_cond_seqno;
         }
diff --git a/controller/vif-plug.c b/controller/vif-plug.c
index 2cd7cd09a..450b4aa2f 100644
--- a/controller/vif-plug.c
+++ b/controller/vif-plug.c
@@ -481,7 +481,8 @@ vif_plug_iface_touched_this_txn(
 static bool
 vif_plug_handle_lport_vif(const struct sbrec_port_binding *pb,
                           struct vif_plug_ctx_in *vif_plug_ctx_in,
-                          struct vif_plug_ctx_out *vif_plug_ctx_out)
+                          struct vif_plug_ctx_out *vif_plug_ctx_out,
+                          bool can_unplug)
 {
     if (vif_plug_iface_touched_this_txn(vif_plug_ctx_out, pb->logical_port)) {
         return true;
@@ -493,7 +494,7 @@ vif_plug_handle_lport_vif(const struct
sbrec_port_binding *pb,
     if (lport_can_bind_on_this_chassis(vif_plug_ctx_in->chassis_rec, pb)) {
         handled &= consider_plug_lport(pb, lbinding,
                                        vif_plug_ctx_in, vif_plug_ctx_out);
-    } else if (lbinding && lbinding->iface) {
+    } else if (can_unplug && lbinding && lbinding->iface) {
         handled &= consider_unplug_iface(lbinding->iface, pb,
                                          vif_plug_ctx_in, vif_plug_ctx_out);
     }
@@ -503,7 +504,8 @@ vif_plug_handle_lport_vif(const struct
sbrec_port_binding *pb,
 static bool
 vif_plug_handle_iface(const struct ovsrec_interface *iface_rec,
                       struct vif_plug_ctx_in *vif_plug_ctx_in,
-                      struct vif_plug_ctx_out *vif_plug_ctx_out)
+                      struct vif_plug_ctx_out *vif_plug_ctx_out,
+                      bool can_unplug)
 {
     bool handled = true;
     const char *vif_plug_type = smap_get(&iface_rec->external_ids,
@@ -523,7 +525,8 @@ vif_plug_handle_iface(const struct
ovsrec_interface *iface_rec,
          * consider updating it */
         handled &= consider_plug_lport(pb, lbinding,
                                        vif_plug_ctx_in, vif_plug_ctx_out);
-    } else if (!pb
+    } else if (can_unplug
+               && !pb
                || !lport_can_bind_on_this_chassis(
                        vif_plug_ctx_in->chassis_rec, pb)) {
         /* No lport for this interface or it is destined for different chassis,
@@ -534,19 +537,28 @@ vif_plug_handle_iface(const struct
ovsrec_interface *iface_rec,
     return handled;
 }

+/* 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.
+ */
+#define VIF_PLUG_PRIME_IDL_COUNT_SEEED 10
+static int vif_plug_prime_idl_count = VIF_PLUG_PRIME_IDL_COUNT_SEEED;
+
+void
+vif_plug_reset_idl_prime_counter(void)
+{
+    vif_plug_prime_idl_count = VIF_PLUG_PRIME_IDL_COUNT_SEEED;
+}
+
 void
 vif_plug_run(struct vif_plug_ctx_in *vif_plug_ctx_in,
              struct vif_plug_ctx_out *vif_plug_ctx_out)
 {
-    /* 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 naive check so that we do not erronously unplug ports
-     * because the data is just not loaded yet.
-     */
-    static int prime_idl_count = 3;
-    if (prime_idl_count && --prime_idl_count > 0) {
-        return;
+    if (vif_plug_prime_idl_count && --vif_plug_prime_idl_count > 0) {
+        VLOG_DBG("vif_plug_run: vif_plug_prime_idl_count=%d, will not unplug "
+                 "ports in this iteration.", vif_plug_prime_idl_count);
     }

     if (!vif_plug_ctx_in->chassis_rec) {
@@ -555,7 +567,8 @@ vif_plug_run(struct vif_plug_ctx_in *vif_plug_ctx_in,
     const struct ovsrec_interface *iface_rec;
     OVSREC_INTERFACE_TABLE_FOR_EACH (iface_rec,
                                      vif_plug_ctx_in->iface_table) {
-        vif_plug_handle_iface(iface_rec, vif_plug_ctx_in, vif_plug_ctx_out);
+        vif_plug_handle_iface(iface_rec, vif_plug_ctx_in, vif_plug_ctx_out,
+                              !vif_plug_prime_idl_count);
     }

     struct sbrec_port_binding *target =
@@ -570,7 +583,8 @@ vif_plug_run(struct vif_plug_ctx_in *vif_plug_ctx_in,
             vif_plug_ctx_in->sbrec_port_binding_by_requested_chassis) {
         enum en_lport_type lport_type = get_lport_type(pb);
         if (lport_type == LP_VIF) {
-            vif_plug_handle_lport_vif(pb, vif_plug_ctx_in, vif_plug_ctx_out);
+            vif_plug_handle_lport_vif(pb, vif_plug_ctx_in, vif_plug_ctx_out,
+                                      !vif_plug_prime_idl_count);
         }
     }
     sbrec_port_binding_index_destroy_row(target);

-- 
Frode Nordahl

> Thanks,
> Han
>
> > +    static int prime_idl_count = 3;
> > +    if (prime_idl_count && --prime_idl_count > 0) {
> > +        return;
> > +    }
> > +
> > +    if (!vif_plug_ctx_in->chassis_rec) {
> > +        return;
> > +    }
> > +    const struct ovsrec_interface *iface_rec;
> > +    OVSREC_INTERFACE_TABLE_FOR_EACH (iface_rec,
> > +                                     vif_plug_ctx_in->iface_table) {
> > +        vif_plug_handle_iface(iface_rec, vif_plug_ctx_in, vif_plug_ctx_out);
> > +    }
> > +
> > +    struct sbrec_port_binding *target =
> > +        sbrec_port_binding_index_init_row(
> > +            vif_plug_ctx_in->sbrec_port_binding_by_requested_chassis);
> > +    sbrec_port_binding_index_set_requested_chassis(
> > +        target,
> > +        vif_plug_ctx_in->chassis_rec);
> > +    const struct sbrec_port_binding *pb;
> > +    SBREC_PORT_BINDING_FOR_EACH_EQUAL (
> > +            pb, target,
> > +            vif_plug_ctx_in->sbrec_port_binding_by_requested_chassis) {
> > +        enum en_lport_type lport_type = get_lport_type(pb);
> > +        if (lport_type == LP_VIF) {
> > +            vif_plug_handle_lport_vif(pb, vif_plug_ctx_in, vif_plug_ctx_out);
> > +        }
> > +    }
> > +    sbrec_port_binding_index_destroy_row(target);
> > +}
> > +
> > +static void
> > +vif_plug_finish_deleted__(struct shash *deleted_iface_ids, bool txn_success)
> > +{
> > +    struct shash_node *node, *next;
> > +    SHASH_FOR_EACH_SAFE (node, next, deleted_iface_ids) {
> > +        struct vif_plug_port_ctx *vif_plug_port_ctx = node->data;
> > +        if (txn_success) {
> > +            vif_plug_port_finish(vif_plug_port_ctx->vif_plug,
> > +                             &vif_plug_port_ctx->vif_plug_port_ctx_in,
> > +                             NULL);
> > +        }
> > +        shash_delete(deleted_iface_ids, node);
> > +        destroy_port_ctx(vif_plug_port_ctx);
> > +    }
> > +}
> > +
> > +void
> > +vif_plug_clear_deleted(struct shash *deleted_iface_ids) {
> > +    vif_plug_finish_deleted__(deleted_iface_ids, false);
> > +}
> > +
> > +void
> > +vif_plug_finish_deleted(struct shash *deleted_iface_ids) {
> > +    vif_plug_finish_deleted__(deleted_iface_ids, true);
> > +}
> > +
> > +static void
> > +vif_plug_finish_changed__(struct shash *changed_iface_ids, bool txn_success)
> > +{
> > +    struct shash_node *node, *next;
> > +    SHASH_FOR_EACH_SAFE (node, next, changed_iface_ids) {
> > +        struct vif_plug_port_ctx *vif_plug_port_ctx = node->data;
> > +        if (txn_success) {
> > +            vif_plug_port_finish(vif_plug_port_ctx->vif_plug,
> > +                                 &vif_plug_port_ctx->vif_plug_port_ctx_in,
> > +                                 &vif_plug_port_ctx->vif_plug_port_ctx_out);
> > +        }
> > +        vif_plug_port_ctx_destroy(vif_plug_port_ctx->vif_plug,
> > +                                  &vif_plug_port_ctx->vif_plug_port_ctx_in,
> > +                                  &vif_plug_port_ctx->vif_plug_port_ctx_out);
> > +        shash_delete(changed_iface_ids, node);
> > +        destroy_port_ctx(vif_plug_port_ctx);
> > +    }
> > +}
> > +
> > +void
> > +vif_plug_clear_changed(struct shash *deleted_iface_ids) {
> > +    vif_plug_finish_changed__(deleted_iface_ids, false);
> > +}
> > +
> > +void
> > +vif_plug_finish_changed(struct shash *deleted_iface_ids) {
> > +    vif_plug_finish_changed__(deleted_iface_ids, true);
> > +}
> > diff --git a/controller/vif-plug.h b/controller/vif-plug.h
> > new file mode 100644
> > index 000000000..7a1978e38
> > --- /dev/null
> > +++ b/controller/vif-plug.h
> > @@ -0,0 +1,79 @@
> > +/*
> > + * Copyright (c) 2021 Canonical
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + *     http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#ifndef VIF_PLUG_H
> > +#define VIF_PLUG_H 1
> > +
> > +/*
> > + * VIF Plug, the controller internal interface to the VIF plug provider
> > + * infrastructure.
> > + */
> > +
> > +#include "openvswitch/shash.h"
> > +#include "smap.h"
> > +
> > +#ifdef  __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +struct vif_plug_ctx_in {
> > +    struct ovsdb_idl_txn *ovs_idl_txn;
> > +    struct ovsdb_idl_index *sbrec_port_binding_by_name;
> > +    struct ovsdb_idl_index *sbrec_port_binding_by_requested_chassis;
> > +    struct ovsdb_idl_index *ovsrec_port_by_interfaces;
> > +    const struct ovsrec_open_vswitch_table *ovs_table;
> > +    const struct ovsrec_bridge *br_int;
> > +    const struct ovsrec_interface_table *iface_table;
> > +    const struct sbrec_chassis *chassis_rec;
> > +    const struct shash *local_bindings;
> > +};
> > +
> > +struct vif_plug_ctx_out {
> > +    struct shash *deleted_iface_ids;
> > +    struct shash *changed_iface_ids;
> > +};
> > +
> > +struct vif_plug_class;
> > +struct vif_plug_port_ctx_out;
> > +struct vif_plug_port_ctx_in;
> > +
> > +const struct sset * vif_plug_get_maintained_iface_options(
> > +    const struct vif_plug_class *);
> > +
> > +bool vif_plug_port_prepare(const struct vif_plug_class *,
> > +                           const struct vif_plug_port_ctx_in *,
> > +                           struct vif_plug_port_ctx_out *);
> > +void vif_plug_port_finish(const struct vif_plug_class *,
> > +                          const struct vif_plug_port_ctx_in *,
> > +                          struct vif_plug_port_ctx_out *);
> > +void vif_plug_port_ctx_destroy(const struct vif_plug_class *,
> > +                           const struct vif_plug_port_ctx_in *,
> > +                           struct vif_plug_port_ctx_out *);
> > +
> > +struct ovsdb_idl;
> > +
> > +void vif_plug_register_ovs_idl(struct ovsdb_idl *ovs_idl);
> > +void vif_plug_run(struct vif_plug_ctx_in *, struct vif_plug_ctx_out *);
> > +void vif_plug_clear_changed(struct shash *deleted_iface_ids);
> > +void vif_plug_finish_changed(struct shash *changed_iface_ids);
> > +void vif_plug_clear_deleted(struct shash *deleted_iface_ids);
> > +void vif_plug_finish_deleted(struct shash *changed_iface_ids);
> > +
> > +#ifdef  __cplusplus
> > +}
> > +#endif
> > +
> > +#endif /* vif-plug.h */
> > diff --git a/lib/automake.mk b/lib/automake.mk
> > index 9f9f447d5..829aedfc5 100644
> > --- a/lib/automake.mk
> > +++ b/lib/automake.mk
> > @@ -4,6 +4,11 @@ lib_libovn_la_LDFLAGS = \
> >          -Wl,--version-script=$(top_builddir)/lib/libovn.sym \
> >          $(OVS_LIBDIR)/libopenvswitch.la \
> >          $(AM_LDFLAGS)
> > +
> > +if HAVE_VIF_PLUG_PROVIDER
> > +lib_libovn_la_LDFLAGS += $(VIF_PLUG_PROVIDER_LDFLAGS)
> > +endif
> > +
> >  lib_libovn_la_SOURCES = \
> >         lib/acl-log.c \
> >         lib/acl-log.h \
> > @@ -33,7 +38,10 @@ lib_libovn_la_SOURCES = \
> >         lib/inc-proc-eng.h \
> >         lib/lb.c \
> >         lib/lb.h \
> > -       lib/stopwatch-names.h
> > +       lib/stopwatch-names.h \
> > +       lib/vif-plug-provider.h \
> > +       lib/vif-plug-provider.c \
> > +       lib/vif-plug-providers/dummy/vif-plug-dummy.c
> >  nodist_lib_libovn_la_SOURCES = \
> >         lib/ovn-dirs.c \
> >         lib/ovn-nb-idl.c \
> > diff --git a/lib/vif-plug-provider.c b/lib/vif-plug-provider.c
> > new file mode 100644
> > index 000000000..798e90e26
> > --- /dev/null
> > +++ b/lib/vif-plug-provider.c
> > @@ -0,0 +1,204 @@
> > +/*
> > + * Copyright (c) 2021 Canonical
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + *     http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#include <config.h>
> > +#include "vif-plug-provider.h"
> > +
> > +#include <errno.h>
> > +#include <stdint.h>
> > +#include <string.h>
> > +
> > +#include "openvswitch/vlog.h"
> > +#include "openvswitch/shash.h"
> > +#include "smap.h"
> > +#include "sset.h"
> > +#include "lib/inc-proc-eng.h"
> > +
> > +VLOG_DEFINE_THIS_MODULE(vif_plug_provider);
> > +
> > +#ifdef ENABLE_VIF_PLUG
> > +static const struct vif_plug_class *base_vif_plug_classes[] = {
> > +};
> > +#endif
> > +
> > +static struct shash vif_plug_classes = SHASH_INITIALIZER(&vif_plug_classes);
> > +
> > +/* Protects the 'vif_plug_classes' shash. */
> > +static struct ovs_mutex vif_plug_classes_mutex = OVS_MUTEX_INITIALIZER;
> > +
> > +/* Initialize the the VIF plug infrastructure by registering known classes */
> > +void
> > +vif_plug_provider_initialize(void)
> > +{
> > +    static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> > +
> > +    if (ovsthread_once_start(&once)) {
> > +#ifdef ENABLE_VIF_PLUG
> > +        /* Register built-in VIF plug provider classes */
> > +        for (int i = 0; i < ARRAY_SIZE(base_vif_plug_classes); i++) {
> > +            vif_plug_provider_register(base_vif_plug_classes[i]);
> > +        }
> > +#endif
> > +#ifdef HAVE_VIF_PLUG_PROVIDER
> > +        /* Register external VIF plug provider classes.
> > +         *
> > +         * Note that we cannot use the ARRAY_SIZE macro here as
> > +         * vif_plug_provider_classes is defined in external code which is not
> > +         * available at compile time.  The convention is to use a
> > +         * NULL-terminated array instead. */
> > +        for (const struct vif_plug_class **pp = vif_plug_provider_classes;
> > +             pp && *pp;
> > +             pp++)
> > +        {
> > +            vif_plug_provider_register(*pp);
> > +        }
> > +#endif
> > +        ovsthread_once_done(&once);
> > +    }
> > +}
> > +
> > +static int
> > +vif_plug_provider_register__(const struct vif_plug_class *new_class)
> > +{
> > +    struct vif_plug_class *vif_plug_class;
> > +    int error;
> > +
> > +    if (shash_find(&vif_plug_classes, new_class->type)) {
> > +        VLOG_WARN("attempted to register duplicate VIF plug provider: %s",
> > +                  new_class->type);
> > +        return EEXIST;
> > +    }
> > +
> > +    error = new_class->init ? new_class->init() : 0;
> > +    if (error) {
> > +        VLOG_WARN("failed to initialize %s VIF plug provider class: %s",
> > +                  new_class->type, ovs_strerror(error));
> > +        return error;
> > +    }
> > +
> > +    vif_plug_class = xmalloc(sizeof *vif_plug_class);
> > +    memcpy(vif_plug_class, new_class, sizeof *vif_plug_class);
> > +
> > +    shash_add(&vif_plug_classes, new_class->type, vif_plug_class);
> > +
> > +    return 0;
> > +}
> > +
> > +/* Register the new VIF plug provider referred to in 'new_class' and perform
> > + * any class level initialization as specified in its vif_plug_class. */
> > +int
> > +vif_plug_provider_register(const struct vif_plug_class *new_class)
> > +{
> > +    int error;
> > +
> > +    ovs_mutex_lock(&vif_plug_classes_mutex);
> > +    error = vif_plug_provider_register__(new_class);
> > +    ovs_mutex_unlock(&vif_plug_classes_mutex);
> > +
> > +    return error;
> > +}
> > +
> > +static int
> > +vif_plug_provider_unregister__(const char *type)
> > +{
> > +    int error;
> > +    struct shash_node *node;
> > +    struct vif_plug_class *vif_plug_class;
> > +
> > +    node = shash_find(&vif_plug_classes, type);
> > +    if (!node) {
> > +        return EINVAL;
> > +    }
> > +
> > +    vif_plug_class = node->data;
> > +    error = vif_plug_class->destroy ? vif_plug_class->destroy() : 0;
> > +    if (error) {
> > +        VLOG_WARN("failed to destroy %s VIF plug class: %s",
> > +                  vif_plug_class->type, ovs_strerror(error));
> > +        return error;
> > +    }
> > +
> > +    shash_delete(&vif_plug_classes, node);
> > +    free(vif_plug_class);
> > +
> > +    return 0;
> > +}
> > +
> > +/* Unregister the VIF plug provider identified by 'type' and perform any class
> > + * level de-initialization as specified in its vif_plug_class. */
> > +int
> > +vif_plug_provider_unregister(const char *type)
> > +{
> > +    int error;
> > +
> > +    ovs_mutex_lock(&vif_plug_classes_mutex);
> > +    error = vif_plug_provider_unregister__(type);
> > +    ovs_mutex_unlock(&vif_plug_classes_mutex);
> > +
> > +    return error;
> > +}
> > +
> > +/* Check whether there are any VIF plug providers registered */
> > +bool
> > +vif_plug_provider_has_providers(void)
> > +{
> > +    return !shash_is_empty(&vif_plug_classes);
> > +}
> > +
> > +const struct vif_plug_class *
> > +vif_plug_provider_get(const char *type)
> > +{
> > +    struct vif_plug_class *vif_plug_class;
> > +
> > +    ovs_mutex_lock(&vif_plug_classes_mutex);
> > +    vif_plug_class = shash_find_data(&vif_plug_classes, type);
> > +    ovs_mutex_unlock(&vif_plug_classes_mutex);
> > +
> > +    return vif_plug_class;
> > +}
> > +
> > +/* Iterate over VIF plug providers and call their run function.
> > + *
> > + * Returns 'true' if any of the providers run functions return 'true', 'false'
> > + * otherwise.
> > + *
> > + * A return value of 'true' means that data has changed. */
> > +bool
> > +vif_plug_provider_run_all(void)
> > +{
> > +    struct shash_node *node, *next;
> > +    bool changed = false;
> > +
> > +    SHASH_FOR_EACH_SAFE (node, next, &vif_plug_classes) {
> > +        struct vif_plug_class *vif_plug_class = node->data;
> > +        if (vif_plug_class->run && vif_plug_class->run(vif_plug_class)) {
> > +            changed = true;
> > +        }
> > +    }
> > +    return changed;
> > +}
> > +
> > +/* De-initialize and unregister the VIF plug provider classes. */
> > +void
> > +vif_plug_provider_destroy_all(void)
> > +{
> > +    struct shash_node *node, *next;
> > +
> > +    SHASH_FOR_EACH_SAFE (node, next, &vif_plug_classes) {
> > +        struct vif_plug_class *vif_plug_class = node->data;
> > +        vif_plug_provider_unregister(vif_plug_class->type);
> > +    }
> > +}
> > diff --git a/lib/vif-plug-provider.h b/lib/vif-plug-provider.h
> > new file mode 100644
> > index 000000000..c0c74b073
> > --- /dev/null
> > +++ b/lib/vif-plug-provider.h
> > @@ -0,0 +1,163 @@
> > +/*
> > + * Copyright (c) 2021 Canonical
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + *     http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#ifndef VIF_PLUG_PROVIDER_H
> > +#define VIF_PLUG_PROVIDER_H 1
> > +
> > +/* Interface for VIF plug providers.
> > + *
> > + * A VIF plug provider implementation performs lookup and/or initialization of
> > + * ports, typically representor ports, using generic non-blocking hardware
> > + * interfaces.  This allows the ovn-controller to, upon the CMS's request,
> > + * create ports and interfaces in the chassis's Open vSwitch instances (also
> > + * known as vif plugging).
> > + *
> > + * This module contains the infrastructure for registering VIF plug providers
> > + * which may be hosted inside or outside the core OVN repository.
> > + */
> > +
> > +#include <stdbool.h>
> > +
> > +#include "smap.h"
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +struct vif_plug_class;
> > +struct ovsdb_idl_txn;
> > +struct ovsrec_bridge;
> > +
> > +enum vif_plug_op_type {
> > +    PLUG_OP_CREATE = 1, /* Port is created or updated */
> > +    PLUG_OP_REMOVE,     /* Port is removed from this chassis */
> > +};
> > +
> > +struct vif_plug_port_ctx_in {
> > +    /* Operation being performed */
> > +    enum vif_plug_op_type op_type;
> > +
> > +    /* These are provided so that the plug implementation may make decisions
> > +     * based on environmental factors such as settings in the open-vswitch
> > +     * table and datapath type settings on the integration bridge. */
> > +    const struct ovsrec_open_vswitch_table *ovs_table;
> > +    const struct ovsrec_bridge *br_int;
> > +
> > +    /* Name of logical port, can be useful for plugging library to track any
> > +     * per port resource initialization. */
> > +    const char *lport_name;
> > +
> > +    /* Logical port options, while OVN will forward the contents verbatim from
> > +     * the Southbound database, the convention is for the plugging library to
> > +     * only make decisions based on the vif-plug-* options. */
> > +    struct smap lport_options;
> > +
> > +    /* When OVN knows about an existing interface record associated with this
> > +     * lport, these will be filled in with information about it. */
> > +    const char *iface_name;
> > +    const char *iface_type;
> > +    struct smap iface_options;
> > +};
> > +
> > +struct vif_plug_port_ctx_out {
> > +    /* The name to use for port and interface record. */
> > +    char *name;
> > +
> > +    /* Type of interface to create. */
> > +    char *type;
> > +
> > +    /* Options to set on the interface record. */
> > +    struct smap iface_options;
> > +};
> > +
> > +struct vif_plug_port_ctx {
> > +    const struct vif_plug_class *vif_plug;
> > +    struct vif_plug_port_ctx_in vif_plug_port_ctx_in;
> > +    struct vif_plug_port_ctx_out vif_plug_port_ctx_out;
> > +};
> > +
> > +struct vif_plug_class {
> > +    /* Type of VIF plug provider in this class. */
> > +    const char *type;
> > +
> > +    /* Called when the VIF plug provider is registered, typically at program
> > +     * startup.
> > +     *
> > +     * This function may be set to null if a VIF plug class needs no
> > +     * initialization at registration time. */
> > +    int (*init)(void);
> > +
> > +    /* Called when the VIF plug provider is unregistered, typically at program
> > +     * exit.
> > +     *
> > +     * This function may be set to null if a plug class needs no
> > +     * de-initialization at unregister time.*/
> > +    int (*destroy)(void);
> > +
> > +    /* Performs periodic work needed by VIF plug provider, if any is necessary.
> > +     * Returns 'true; if anything (i.e. lookup tables) changed, 'false'
> > +     * otherwise. */
> > +    bool (*run)(struct vif_plug_class *);
> > +
> > +    /* Retrieve Interface options this VIF plug provider will maintain.  This
> > +     * sset is used to know which items to remove when maintaining the database
> > +     * record. */
> > +    const struct sset * (*vif_plug_get_maintained_iface_options)(void);
> > +
> > +    /* Pass vif_plug_port_ctx_in to VIF plug provider implementation to prepare
> > +     * for port creation/update.
> > +     *
> > +     * The VIF plug provider implemantation can perform lookup or any per port
> > +     * initialization and should fill vif_plug_port_ctx_out with data required
> > +     * for port/interface creation.  The VIF plug implementation should return
> > +     * 'true' if it wants the caller to create/update a port/interface, 'false'
> > +     * otherwise.
> > +     *
> > +     * Data in the vif_plug_port_ctx_out struct is owned by the VIF plug
> > +     * provider, and a call must be made to the vif_plug_port_ctx_destroy
> > +     * callback to free up any allocations when done with port creation/update.
> > +     */
> > +    bool (*vif_plug_port_prepare)(const struct vif_plug_port_ctx_in *,
> > +                                  struct vif_plug_port_ctx_out *);
> > +
> > +    /* Notify VIF plug provider that port update is committed to OVSDB. */
> > +    void (*vif_plug_port_finish)(const struct vif_plug_port_ctx_in *,
> > +                                 struct vif_plug_port_ctx_out *);
> > +
> > +    /* Free any allocations made by the vif_plug_port_prepare callback. */
> > +    void (*vif_plug_port_ctx_destroy)(const struct vif_plug_port_ctx_in *,
> > +                                      struct vif_plug_port_ctx_out *);
> > +};
> > +
> > +extern const struct vif_plug_class vif_plug_dummy_class;
> > +#ifdef HAVE_VIF_PLUG_PROVIDER
> > +extern const struct vif_plug_class *vif_plug_provider_classes[];
> > +#endif
> > +
> > +void vif_plug_provider_initialize(void);
> > +int vif_plug_provider_register(const struct vif_plug_class *);
> > +int vif_plug_provider_unregister(const char *type);
> > +bool vif_plug_provider_has_providers(void);
> > +const struct vif_plug_class * vif_plug_provider_get(const char *);
> > +bool vif_plug_provider_run_all(void);
> > +void vif_plug_provider_destroy_all(void);
> > +void vif_plug_dummy_enable(void);
> > +
> > +#ifdef  __cplusplus
> > +}
> > +#endif
> > +
> > +#endif /* vif-plug-provider.h */
> > diff --git a/lib/vif-plug-providers/dummy/vif-plug-dummy.c b/lib/vif-plug-providers/dummy/vif-plug-dummy.c
> > new file mode 100644
> > index 000000000..bc8953e98
> > --- /dev/null
> > +++ b/lib/vif-plug-providers/dummy/vif-plug-dummy.c
> > @@ -0,0 +1,120 @@
> > +/*
> > + * Copyright (c) 2021 Canonical
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + *     http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#include <config.h>
> > +#include "lib/vif-plug-provider.h"
> > +
> > +#include <stdint.h>
> > +
> > +#include "openvswitch/vlog.h"
> > +#include "smap.h"
> > +#include "sset.h"
> > +
> > +#ifndef IFNAMSIZ
> > +#define IFNAMSIZ 16
> > +#endif
> > +
> > +VLOG_DEFINE_THIS_MODULE(vif_plug_dummy);
> > +
> > +static struct sset vif_plug_dummy_maintained_iface_options;
> > +
> > +static int
> > +vif_plug_dummy_init(void)
> > +{
> > +    sset_init(&vif_plug_dummy_maintained_iface_options);
> > +    sset_add(&vif_plug_dummy_maintained_iface_options,
> > +             "vif-plug-dummy-option");
> > +
> > +    return 0;
> > +}
> > +
> > +static int
> > +vif_plug_dummy_destroy(void)
> > +{
> > +    sset_destroy(&vif_plug_dummy_maintained_iface_options);
> > +
> > +    return 0;
> > +}
> > +
> > +static const struct sset*
> > +vif_plug_dummy_get_maintained_iface_options(void)
> > +{
> > +    return &vif_plug_dummy_maintained_iface_options;
> > +}
> > +
> > +static bool
> > +vif_plug_dummy_run(struct vif_plug_class *plug)
> > +{
> > +    VLOG_DBG("vif_plug_dummy_run(%p)", plug);
> > +
> > +    return false;
> > +}
> > +
> > +static bool
> > +vif_plug_dummy_port_prepare(const struct vif_plug_port_ctx_in *ctx_in,
> > +                            struct vif_plug_port_ctx_out *ctx_out)
> > +{
> > +    VLOG_DBG("vif_plug_dummy_port_prepare: %s", ctx_in->lport_name);
> > +
> > +    if (ctx_in->op_type == PLUG_OP_CREATE) {
> > +        size_t lport_name_len = strlen(ctx_in->lport_name);
> > +        ctx_out->name = xzalloc(IFNAMSIZ);
> > +        memcpy(ctx_out->name, ctx_in->lport_name,
> > +               (lport_name_len < IFNAMSIZ) ? lport_name_len : IFNAMSIZ - 1);
> > +        ctx_out->type = xstrdup("internal");
> > +        smap_init(&ctx_out->iface_options);
> > +        smap_add(&ctx_out->iface_options, "vif-plug-dummy-option", "value");
> > +    }
> > +
> > +    return true;
> > +}
> > +
> > +static void
> > +vif_plug_dummy_port_finish(const struct vif_plug_port_ctx_in *ctx_in,
> > +                           struct vif_plug_port_ctx_out *ctx_out OVS_UNUSED)
> > +{
> > +    VLOG_DBG("vif_plug_dummy_port_finish: %s", ctx_in->lport_name);
> > +}
> > +
> > +static void
> > +vif_plug_dummy_port_ctx_destroy(const struct vif_plug_port_ctx_in *ctx_in,
> > +                                struct vif_plug_port_ctx_out *ctx_out)
> > +{
> > +    VLOG_DBG("vif_plug_dummy_port_ctx_destroy: %s", ctx_in->lport_name);
> > +    ovs_assert(ctx_in->op_type == PLUG_OP_CREATE);
> > +    free(ctx_out->name);
> > +    free(ctx_out->type);
> > +    smap_destroy(&ctx_out->iface_options);
> > +}
> > +
> > +const struct vif_plug_class vif_plug_dummy_class = {
> > +    .type = "dummy",
> > +    .init = vif_plug_dummy_init,
> > +    .destroy = vif_plug_dummy_destroy,
> > +    .vif_plug_get_maintained_iface_options =
> > +        vif_plug_dummy_get_maintained_iface_options,
> > +    .run = vif_plug_dummy_run,
> > +    .vif_plug_port_prepare = vif_plug_dummy_port_prepare,
> > +    .vif_plug_port_finish = vif_plug_dummy_port_finish,
> > +    .vif_plug_port_ctx_destroy = vif_plug_dummy_port_ctx_destroy,
> > +};
> > +
> > +void
> > +vif_plug_dummy_enable(void)
> > +{
> > +    vif_plug_provider_register(&vif_plug_dummy_class);
> > +}
> > +
> > diff --git a/ovn-architecture.7.xml b/ovn-architecture.7.xml
> > index a71798f68..027b3e858 100644
> > --- a/ovn-architecture.7.xml
> > +++ b/ovn-architecture.7.xml
> > @@ -67,8 +67,9 @@
> >      <li>
> >        One or more (usually many) <dfn>hypervisors</dfn>.  Hypervisors must run
> >        Open vSwitch and implement the interface described in
> > -      <code>Documentation/topics/integration.rst</code> in the OVN source tree.
> > -      Any hypervisor platform supported by Open vSwitch is acceptable.
> > +      <code>Documentation/topics/integration.rst</code> in the Open vSwitch
> > +      source tree.  Any hypervisor platform supported by Open vSwitch is
> > +      acceptable.
> >      </li>
> >
> >      <li>
> > @@ -318,11 +319,19 @@
> >
> >      <li>
> >        On a hypervisor, any VIFs that are to be attached to logical networks.
> > -      The hypervisor itself, or the integration between Open vSwitch and the
> > -      hypervisor (described in
> > -      <code>Documentation/topics/integration.rst</code>) takes care of this.
> > -      (This is not part of OVN or new to OVN; this is pre-existing integration
> > -      work that has already been done on hypervisors that support OVS.)
> > +      For instances connected through software emulated ports such as TUN/TAP
> > +      or VETH pairs, the hypervisor itself will normally create ports and plug
> > +      them into the integration bridge.  For instances connected through
> > +      representor ports, typically used with hardware offload, the
> > +      <code>ovn-controller</code> may on CMS direction consult a plugging
> > +      provider library for representor port lookup and plug them into the
> > +      integration bridge (please refer to
> > +      <code>Documentation/topics/plugging-providers.rst</code> for more
> > +      information).  In both cases the conventions described in
> > +      <code>Documentation/topics/integration.rst</code> in the Open vSwitch
> > +      source tree is followed to ensure mapping between OVN logical port and
> > +      VIF.  (This is pre-existing integration work that has already been done
> > +      on hypervisors that support OVS.)
> >      </li>
> >
> >      <li>
> > @@ -921,12 +930,12 @@
> >        Eventually, a user powers on the VM that owns the VIF.  On the hypervisor
> >        where the VM is powered on, the integration between the hypervisor and
> >        Open vSwitch (described in
> > -      <code>Documentation/topics/integration.rst</code>) adds the VIF to the OVN
> > -      integration bridge and stores <var>vif-id</var> in
> > -      <code>external_ids</code>:<code>iface-id</code> to indicate that the
> > -      interface is an instantiation of the new VIF.  (None of this code is new
> > -      in OVN; this is pre-existing integration work that has already been done
> > -      on hypervisors that support OVS.)
> > +      <code>Documentation/topics/integration.rst</code> in the Open vSwitch
> > +      source tree) adds the VIF to the OVN integration bridge and stores
> > +      <var>vif-id</var> in <code>external_ids</code>:<code>iface-id</code> to
> > +      indicate that the interface is an instantiation of the new VIF.  (None of
> > +      this code is new in OVN; this is pre-existing integration work that has
> > +      already been done on hypervisors that support OVS.)
> >      </li>
> >
> >      <li>
> > diff --git a/tests/automake.mk b/tests/automake.mk
> > index c4a7c0a5b..685d78c5b 100644
> > --- a/tests/automake.mk
> > +++ b/tests/automake.mk
> > @@ -38,7 +38,8 @@ TESTSUITE_AT = \
> >         tests/ovn-ipam.at \
> >         tests/ovn-features.at \
> >         tests/ovn-lflow-cache.at \
> > -       tests/ovn-ipsec.at
> > +       tests/ovn-ipsec.at \
> > +       tests/ovn-vif-plug.at
> >
> >  SYSTEM_KMOD_TESTSUITE_AT = \
> >         tests/system-common-macros.at \
> > @@ -243,13 +244,23 @@ tests_ovstest_SOURCES = \
> >         tests/test-ovn.c \
> >         controller/test-lflow-cache.c \
> >         controller/test-ofctrl-seqno.c \
> > +       controller/test-vif-plug.c \
> >         lib/test-ovn-features.c \
> >         northd/test-ipam.c
> >
> >  tests_ovstest_LDADD = $(OVS_LIBDIR)/daemon.lo \
> >      $(OVS_LIBDIR)/libopenvswitch.la lib/libovn.la \
> > +       controller/binding.$(OBJEXT) \
> > +       controller/encaps.$(OBJEXT) \
> > +       controller/ha-chassis.$(OBJEXT) \
> > +       controller/if-status.$(OBJEXT) \
> >         controller/lflow-cache.$(OBJEXT) \
> > +       controller/local_data.$(OBJEXT) \
> > +       controller/lport.$(OBJEXT) \
> >         controller/ofctrl-seqno.$(OBJEXT) \
> > +       controller/ovsport.$(OBJEXT) \
> > +       controller/patch.$(OBJEXT) \
> > +       controller/vif-plug.$(OBJEXT) \
> >         northd/ipam.$(OBJEXT)
> >
> >  # Python tests.
> > diff --git a/tests/ovn-vif-plug.at b/tests/ovn-vif-plug.at
> > new file mode 100644
> > index 000000000..86b0b4b84
> > --- /dev/null
> > +++ b/tests/ovn-vif-plug.at
> > @@ -0,0 +1,8 @@
> > +#
> > +# Unit tests for the lib/vif-plug-provider.c and controller/vif-plug.c modules.
> > +#
> > +AT_BANNER([OVN unit tests - vif-plug])
> > +
> > +AT_SETUP([unit test -- plugging infrastructure tests])
> > +AT_CHECK([ovstest test-plug run], [0], [])
> > +AT_CLEANUP
> > --
> > 2.32.0
> >



More information about the dev mailing list