On Wed, Jun 3, 2020 at 11:43 AM Han Zhou <hz...@ovn.org> wrote:
> On Thu, May 28, 2020 at 4:06 AM <num...@ovn.org> wrote:
> >
> > From: Numan Siddique <num...@ovn.org>
> >
> > When ovn-controller updates the nb_cfg column of its chassis,
> > this results in full recomputation on all the nodes. This results
> > in wastage of CPU cycles. To address this, this patch handles
> > sbrec_chassis changes incrementally.
> >
> > We don't need to handle any sbrec_chassis changes during runtime_data
> > stage, because before engine_run() is called, encaps_run() is called
> > which will handle any chassis/encap changes.
> >
> > For new chassis addition and deletion, we need to add/delete flows for
> > the tunnel ports created/deleted. So physical_run() is called for this.
> >
> > For any chassis updates, we can ignore this for flow computation.
> >
> > This patch handles all these.
> >
> > Acked-by: Mark Michelson <mmich...@redhat.com>
> > Signed-off-by: Numan Siddique <num...@ovn.org>
> > ---
> > controller/ovn-controller.c | 50 ++++++++++++++++++++++++++++++-------
> > 1 file changed, 41 insertions(+), 9 deletions(-)
> >
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 2a177ef9b..56744a39e 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -1461,6 +1461,7 @@ static void init_physical_ctx(struct engine_node
> *node,
> > engine_ovsdb_node_get_index(
> > engine_get_input("SB_chassis", node),
> > "name");
> > +
> > if (chassis_id) {
> > chassis = chassis_lookup_by_name(sbrec_chassis_by_name,
> chassis_id);
> > }
> > @@ -1536,8 +1537,8 @@ static void init_lflow_ctx(struct engine_node
> *node,
> > const struct sbrec_chassis *chassis = NULL;
> > struct ovsdb_idl_index *sbrec_chassis_by_name =
> > engine_ovsdb_node_get_index(
> > - engine_get_input("SB_chassis", node),
> > - "name");
> > + engine_get_input("SB_chassis", node),
> > + "name");
> > if (chassis_id) {
> > chassis = chassis_lookup_by_name(sbrec_chassis_by_name,
> chassis_id);
> > }
> > @@ -1617,8 +1618,8 @@ en_flow_output_run(struct engine_node *node, void
> *data)
> >
> > struct ovsdb_idl_index *sbrec_chassis_by_name =
> > engine_ovsdb_node_get_index(
> > - engine_get_input("SB_chassis", node),
> > - "name");
> > + engine_get_input("SB_chassis", node),
> > + "name");
> >
> > const struct sbrec_chassis *chassis = NULL;
> > if (chassis_id) {
> > @@ -1775,8 +1776,8 @@ _flow_output_resource_ref_handler(struct
> engine_node *node, void *data,
> >
> > struct ovsdb_idl_index *sbrec_chassis_by_name =
> > engine_ovsdb_node_get_index(
> > - engine_get_input("SB_chassis", node),
> > - "name");
> > + engine_get_input("SB_chassis", node),
> > + "name");
> > const struct sbrec_chassis *chassis = NULL;
> > if (chassis_id) {
> > chassis = chassis_lookup_by_name(sbrec_chassis_by_name,
> chassis_id);
> > @@ -1948,6 +1949,31 @@ physical_flow_changes_ovs_iface_handler(struct
> engine_node *node,
> > return true;
> > }
> >
> > +/* Handles sbrec_chassis changes.
> > + * If a new chassis is added or removed return false, so that
> > + * physical flows are programmed.
> > + * For any updates, there is no need for any flow computation.
>
> Are we sure about this? At least I saw chassis->hostname, and
> chassis->other_config are used in physical_run().
> For example:
> put_replace_router_port_mac_flows() -> chassis_get_mac() uses
> other_config:ovn-chassis-mac-mappings. If this is updated, the flows should
> change.
>
> For hostname, it is used here.
> if (ofport && requested_chassis && requested_chassis[0] &&
> strcmp(requested_chassis, chassis->name) &&
> strcmp(requested_chassis, chassis->hostname)) {
> /* Even though there is an ofport for this port_binding, it is
> * requested on a different chassis. So ignore this ofport.
> */
> ofport = 0;
> }
> So if chassis->hostname changed, the flow change may be needed as well.
>
> There are also many places using chassis->name, but I assume name should
> never be changed so that should be ok.
>
> For every input, we need to check how they are used in full compute and
> then will know how to handle/ignore in change handlers. (Current test cases
> can't be relied because there are many corner cases not covered)
>
> (sorry that I didn't spot this when reviewing earlier versions)
>
No worries. Thanks for the review.
Great catch. I totally missed that. I'll explore further on this. If it is
too complicated to
handle, I'll just drop this patch from the series.
Thanks
Numan
>
> Thanks,
> Han
>
> > + * Encap changes will also result in sbrec_chassis changes,
> > + * but we handle encap changes separately.
> > + */
> > +static bool
> > +physical_flow_changes_sb_chassis_handler(struct engine_node *node
> OVS_UNUSED,
> > + void *data OVS_UNUSED)
> > +{
> > + struct sbrec_chassis_table *chassis_table =
> > + (struct sbrec_chassis_table *)EN_OVSDB_GET(
> > + engine_get_input("SB_chassis", node));
> > +
> > + const struct sbrec_chassis *ch;
> > + SBREC_CHASSIS_TABLE_FOR_EACH_TRACKED (ch, chassis_table) {
> > + if (sbrec_chassis_is_deleted(ch) || sbrec_chassis_is_new(ch)) {
> > + return false;
> > + }
> > + }
> > +
> > + return true;
> > +}
> > +
> > static bool
> > flow_output_physical_flow_changes_handler(struct engine_node *node, void
> *data)
> > {
> > @@ -2200,6 +2226,10 @@ main(int argc, char *argv[])
> > NULL);
> > engine_add_input(&en_physical_flow_changes, &en_ovs_interface,
> > physical_flow_changes_ovs_iface_handler);
> > + engine_add_input(&en_physical_flow_changes, &en_sb_chassis,
> > + physical_flow_changes_sb_chassis_handler);
> > + engine_add_input(&en_physical_flow_changes, &en_sb_encap,
> > + NULL);
> >
> > engine_add_input(&en_flow_output, &en_addr_sets,
> > flow_output_addr_sets_handler);
> > @@ -2218,9 +2248,10 @@ main(int argc, char *argv[])
> >
> > engine_add_input(&en_flow_output, &en_ovs_open_vswitch, NULL);
> > engine_add_input(&en_flow_output, &en_ovs_bridge, NULL);
> > + engine_add_input(&en_flow_output, &en_sb_chassis,
> > + flow_output_noop_handler);
> > + engine_add_input(&en_flow_output, &en_ovs_bridge, NULL);
> >
> > - engine_add_input(&en_flow_output, &en_sb_chassis, NULL);
> > - engine_add_input(&en_flow_output, &en_sb_encap, NULL);
> > engine_add_input(&en_flow_output, &en_sb_multicast_group,
> > flow_output_sb_multicast_group_handler);
> > engine_add_input(&en_flow_output, &en_sb_port_binding,
> > @@ -2247,7 +2278,8 @@ main(int argc, char *argv[])
> > runtime_data_ovs_interface_handler);
> > engine_add_input(&en_runtime_data, &en_ovs_qos, NULL);
> >
> > - engine_add_input(&en_runtime_data, &en_sb_chassis, NULL);
> > + engine_add_input(&en_runtime_data, &en_sb_chassis,
> > + runtime_data_noop_handler);
> > engine_add_input(&en_runtime_data, &en_sb_datapath_binding,
> > runtime_data_sb_datapath_binding_handler);
> > engine_add_input(&en_runtime_data, &en_sb_port_binding,
> > --
> > 2.26.2
> >
> > _______________________________________________
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
