On Thu, Dec 17, 2020 at 3:47 PM Dumitru Ceara <[email protected]> wrote: > > On 12/17/20 10:23 AM, Dumitru Ceara wrote: > > The incremental processing engine implementation is such that if an > > input node gets updated but the output node doesn't have a change > > handler for it then the output node is immediately recomputed. That is, > > no other input change handlers are executed. > > > > In case of the en_physical_flow_changes node, if a ct-zone changes we > > were also skipping the OVS interface change handler. That is incorrect > > as there is an implicit requirement that flows for OVS interfaces that > > got deleted should be cleared before physical_run() is called. > > > > Instead, we now add an explicit change handler for ct-zone changes. > > This change handler never processes ct-zone updates incrementally but > > ensures that the OVS interface change handler is also called. > > > > Moreover, OVS interface changes (including deletes) are now processed > > before physical_run() is called in the flow_output > > physical_flow_changes_handler. This is a requirement because otherwise > > physical_run() will fail to add flows for new OVS interfaces that > > correspond to unchanged Port_Bindings. > > > > Reported-by: Daniel Alvarez <[email protected]> > > Reported-by: Krzysztof Klimonda <[email protected]> > > Sorry, I forgot to credit Chris for the report too. I can add the above > in a v2 if needed but I'll wait for some reviews before that.
Thanks for the fix. Acked-by: Numan Siddique <[email protected]> I think if we were maintaining a separate flow table for physical flows, we could have cleared that flow table before calling physical_run. Thanks Numan > > Regards, > Dumitru > > > Reported-at: https://bugzilla.redhat.com/1908391 > > Fixes: a3005f0dc777 ("ovn-controller: I-P for ct zone and OVS interface > > changes in flow output stage.") > > Signed-off-by: Dumitru Ceara <[email protected]> > > --- > > controller/ovn-controller.c | 30 ++++++++++++++----- > > tests/ovn.at | 72 > > +++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 94 insertions(+), 8 deletions(-) > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index b5f0c13..5708b7b 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -1714,6 +1714,16 @@ en_physical_flow_changes_run(struct engine_node > > *node, void *data) > > engine_set_node_state(node, EN_UPDATED); > > } > > > > +/* ct_zone changes are not handled incrementally but a handler is required > > + * to avoid skipping the ovs_iface incremental change handler. > > + */ > > +static bool > > +physical_flow_changes_ct_zones_handler(struct engine_node *node OVS_UNUSED, > > + void *data OVS_UNUSED) > > +{ > > + return false; > > +} > > + > > /* There are OVS interface changes. Indicate to the flow_output engine > > * to handle these OVS interface changes for physical flow computations. */ > > static bool > > @@ -2256,6 +2266,13 @@ flow_output_physical_flow_changes_handler(struct > > engine_node *node, void *data) > > struct ed_type_pfc_data *pfc_data = > > engine_get_input_data("physical_flow_changes", node); > > > > + /* If there are OVS interface changes. Try to handle them > > incrementally. */ > > + if (pfc_data->ovs_ifaces_changed) { > > + if (!physical_handle_ovs_iface_changes(&p_ctx, &fo->flow_table)) { > > + return false; > > + } > > + } > > + > > if (pfc_data->recompute_physical_flows) { > > /* This indicates that we need to recompute the physical flows. */ > > physical_clear_unassoc_flows_with_db(&fo->flow_table); > > @@ -2265,12 +2282,6 @@ flow_output_physical_flow_changes_handler(struct > > engine_node *node, void *data) > > return true; > > } > > > > - if (pfc_data->ovs_ifaces_changed) { > > - /* There are OVS interface changes. Try to handle them > > - * incrementally. */ > > - return physical_handle_ovs_iface_changes(&p_ctx, &fo->flow_table); > > - } > > - > > return true; > > } > > > > @@ -2549,11 +2560,14 @@ main(int argc, char *argv[]) > > /* Engine node physical_flow_changes indicates whether > > * we can recompute only physical flows or we can > > * incrementally process the physical flows. > > + * > > + * Note: The order of inputs is important, all OVS interface changes > > must > > + * be handled before any ct_zone changes. > > */ > > - engine_add_input(&en_physical_flow_changes, &en_ct_zones, > > - 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_ct_zones, > > + physical_flow_changes_ct_zones_handler); > > > > engine_add_input(&en_flow_output, &en_addr_sets, > > flow_output_addr_sets_handler); > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 80bc099..66088a7 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -22059,6 +22059,78 @@ OVS_WAIT_UNTIL([test x$(as hv1 ovn-appctl -t > > ovn-controller debug/status) = "xru > > OVN_CLEANUP([hv1]) > > AT_CLEANUP > > > > +AT_SETUP([ovn -- Multiple OVS port changes Incremental Processing]) > > +ovn_start > > + > > +net_add n1 > > +sim_add hv1 > > +as hv1 > > +ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.10 > > + > > +check ovn-nbctl ls-add sw > > +check ovn-nbctl lsp-add sw lsp1 > > +check ovn-nbctl lsp-add sw lsp2 > > + > > +as hv1 > > +ovs-vsctl \ > > + -- add-port br-int vif1 \ > > + -- set Interface vif1 external_ids:iface-id=lsp1 \ > > + ofport-request=1 > > +ovs-vsctl \ > > + -- add-port br-int vif2 \ > > + -- set Interface vif2 external_ids:iface-id=lsp2 \ > > + ofport-request=2 > > + > > +# Wait for ports to be bound. > > +wait_row_count Chassis 1 name=hv1 > > +ch=$(fetch_column Chassis _uuid name=hv1) > > +wait_row_count Port_Binding 1 logical_port=lsp1 chassis=$ch > > +wait_row_count Port_Binding 1 logical_port=lsp2 chassis=$ch > > + > > +AS_BOX([check output flows for initial interfaces]) > > +as hv1 ovs-ofctl dump-flows br-int table=65 > offlows_table65.txt > > +AT_CAPTURE_FILE([offlows_table65.txt]) > > +AT_CHECK_UNQUOTED([grep -c "output:1" offlows_table65.txt], [0], [dnl > > +1 > > +]) > > +AT_CHECK_UNQUOTED([grep -c "output:2" offlows_table65.txt], [0], [dnl > > +1 > > +]) > > + > > +AS_BOX([delete and add OVS interfaces and force batch of updates]) > > +as hv1 ovn-appctl -t ovn-controller debug/pause > > + > > +as hv1 > > +ovs-vsctl \ > > + -- del-port vif1 \ > > + -- del-port vif2 > > + > > +as hv1 > > +ovs-vsctl \ > > + -- add-port br-int vif1 \ > > + -- set Interface vif1 external_ids:iface-id=lsp1 \ > > + ofport-request=3 \ > > + -- add-port br-int vif2 \ > > + -- set Interface vif2 external_ids:iface-id=lsp2 \ > > + ofport-request=4 > > + > > +as hv1 ovn-appctl -t ovn-controller debug/resume > > +check ovn-nbctl --wait=hv sync > > + > > +AS_BOX([check output flows for new interfaces]) > > +as hv1 ovs-ofctl dump-flows br-int table=65 > offlows_table65_2.txt > > +AT_CAPTURE_FILE([offlows_table65_2.txt]) > > +AT_CHECK_UNQUOTED([grep -c "output:3" offlows_table65_2.txt], [0], [dnl > > +1 > > +]) > > +AT_CHECK_UNQUOTED([grep -c "output:4" offlows_table65_2.txt], [0], [dnl > > +1 > > +]) > > + > > +OVN_CLEANUP([hv1]) > > +AT_CLEANUP > > + > > # Test dropping traffic destined to router owned IPs. > > AT_SETUP([ovn -- gateway router drop traffic for own IPs]) > > ovn_start > > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
