Message ID | 1582064765-98657-1-git-send-email-hzhou@ovn.org |
---|---|
State | Accepted |
Headers | show |
Series | [ovs-dev,ovn] ovn-controller: Avoid creating patch port for non-local datapaths. | expand |
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.
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 */
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(-)