[ovs-dev,ovn] ovn-controller: Avoid creating patch port for non-local datapaths.

When external-ids:ovn-monitor-all is set to true, patch ports for
non-local datapaths may be created, which is unnecessary and
confusing.  This patch avoids that by checking if a localnet port
belongs to local datapaths before creating the patch port.  It
also moves patch_run() in mainloop after engine_run(), so that
local_datapaths already reflects the up-to-date situation when
patch_run() is called.

Reported-by: Girish Moodalbail <gmoodalbail@nvidia.com>
Signed-off-by: Han Zhou <hzhou@ovn.org>
---
 controller/ovn-controller.c | 12 ++++++------
 controller/patch.c          | 16 +++++++++++++---
 controller/patch.h          |  3 ++-
 3 files changed, 21 insertions(+), 10 deletions(-)

On 2/18/20 11:26 PM, Han Zhou wrote:
> When external-ids:ovn-monitor-all is set to true, patch ports for
> non-local datapaths may be created, which is unnecessary and
> confusing.  This patch avoids that by checking if a localnet port
> belongs to local datapaths before creating the patch port.  It
> also moves patch_run() in mainloop after engine_run(), so that
> local_datapaths already reflects the up-to-date situation when
> patch_run() is called.
> 
> Reported-by: Girish Moodalbail <gmoodalbail@nvidia.com>
> Signed-off-by: Han Zhou <hzhou@ovn.org>

Hi Han,

Looks good to me.

Acked-by: Dumitru Ceara <dceara@redhat.com>

Thanks,
Dumitru

> ---
>  controller/ovn-controller.c | 12 ++++++------
>  controller/patch.c          | 16 +++++++++++++---
>  controller/patch.h          |  3 ++-
>  3 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 4d245ca..d8e2a0e 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -2011,12 +2011,6 @@ main(int argc, char *argv[])
>                  }
>  
>                  if (chassis) {
> -                    patch_run(ovs_idl_txn,
> -                              ovsrec_bridge_table_get(ovs_idl_loop.idl),
> -                              ovsrec_open_vswitch_table_get(ovs_idl_loop.idl),
> -                              ovsrec_port_table_get(ovs_idl_loop.idl),
> -                              sbrec_port_binding_table_get(ovnsb_idl_loop.idl),
> -                              br_int, chassis);
>                      encaps_run(ovs_idl_txn,
>                                 bridge_table, br_int,
>                                 sbrec_chassis_table_get(ovnsb_idl_loop.idl),
> @@ -2079,6 +2073,12 @@ main(int argc, char *argv[])
>                      }
>                      runtime_data = engine_get_data(&en_runtime_data);
>                      if (runtime_data) {
> +                        patch_run(ovs_idl_txn,
> +                            ovsrec_bridge_table_get(ovs_idl_loop.idl),
> +                            ovsrec_open_vswitch_table_get(ovs_idl_loop.idl),
> +                            ovsrec_port_table_get(ovs_idl_loop.idl),
> +                            sbrec_port_binding_table_get(ovnsb_idl_loop.idl),
> +                            br_int, chassis, &runtime_data->local_datapaths);
>                          pinctrl_run(ovnsb_idl_txn,
>                                      sbrec_datapath_binding_by_key,
>                                      sbrec_port_binding_by_datapath,
> diff --git a/controller/patch.c b/controller/patch.c
> index f2053de..349faae 100644
> --- a/controller/patch.c
> +++ b/controller/patch.c
> @@ -181,7 +181,8 @@ add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn,
>                      const struct sbrec_port_binding_table *port_binding_table,
>                      const struct ovsrec_bridge *br_int,
>                      struct shash *existing_ports,
> -                    const struct sbrec_chassis *chassis)
> +                    const struct sbrec_chassis *chassis,
> +                    const struct hmap *local_datapaths)
>  {
>      /* Get ovn-bridge-mappings. */
>      struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings);
> @@ -190,6 +191,13 @@ add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn,
>  
>      const struct sbrec_port_binding *binding;
>      SBREC_PORT_BINDING_TABLE_FOR_EACH (binding, port_binding_table) {
> +        /* When ovn-monitor-all is true, there can be port-bindings
> +         * on datapaths that are not related to this chassis. Ignore them. */
> +        if (!get_local_datapath(local_datapaths,
> +                                binding->datapath->tunnel_key)) {
> +            continue;
> +        }
> +
>          const char *patch_port_id;
>          if (!strcmp(binding->type, "localnet")) {
>              patch_port_id = "ovn-localnet-port";
> @@ -242,7 +250,8 @@ patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
>            const struct ovsrec_port_table *port_table,
>            const struct sbrec_port_binding_table *port_binding_table,
>            const struct ovsrec_bridge *br_int,
> -          const struct sbrec_chassis *chassis)
> +          const struct sbrec_chassis *chassis,
> +          const struct hmap *local_datapaths)
>  {
>      if (!ovs_idl_txn) {
>          return;
> @@ -269,7 +278,8 @@ patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
>       * 'existing_ports' any patch ports that do exist in the database and
>       * should be there. */
>      add_bridge_mappings(ovs_idl_txn, bridge_table, ovs_table,
> -                        port_binding_table, br_int, &existing_ports, chassis);
> +                        port_binding_table, br_int, &existing_ports, chassis,
> +                        local_datapaths);
>  
>      /* Now 'existing_ports' only still contains patch ports that exist in the
>       * database but shouldn't.  Delete them from the database. */
> diff --git a/controller/patch.h b/controller/patch.h
> index 49b0b2e..81a43bc 100644
> --- a/controller/patch.h
> +++ b/controller/patch.h
> @@ -41,6 +41,7 @@ void patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
>                 const struct ovsrec_port_table *,
>                 const struct sbrec_port_binding_table *,
>                 const struct ovsrec_bridge *br_int,
> -               const struct sbrec_chassis *);
> +               const struct sbrec_chassis *,
> +               const struct hmap *local_datapaths);
>  
>  #endif /* controller/patch.h */
>

On Wed, Feb 19, 2020 at 4:45 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 2/18/20 11:26 PM, Han Zhou wrote:
> > When external-ids:ovn-monitor-all is set to true, patch ports for
> > non-local datapaths may be created, which is unnecessary and
> > confusing.  This patch avoids that by checking if a localnet port
> > belongs to local datapaths before creating the patch port.  It
> > also moves patch_run() in mainloop after engine_run(), so that
> > local_datapaths already reflects the up-to-date situation when
> > patch_run() is called.
> >
> > Reported-by: Girish Moodalbail <gmoodalbail@nvidia.com>
> > Signed-off-by: Han Zhou <hzhou@ovn.org>
>
> Hi Han,
>
> Looks good to me.
>
> Acked-by: Dumitru Ceara <dceara@redhat.com>
>
> Thanks,
> Dumitru
>
Thanks for the review. I applied to master.