| Message ID | 1608197000-637-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ovn-controller: Always run the I-P OVS Interface change handler. | expand |
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 <dalvarez@redhat.com> Reported-by: Krzysztof Klimonda <kklimonda@syntaxhighlighted.com> 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. 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 <dceara@redhat.com> > --- > 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 >
On Thu, Dec 17, 2020 at 3:47 PM Dumitru Ceara <dceara@redhat.com> 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 <dalvarez@redhat.com> > > Reported-by: Krzysztof Klimonda <kklimonda@syntaxhighlighted.com> > > 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 <numans@ovn.org> 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 <dceara@redhat.com> > > --- > > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 12/17/20 2:30 PM, Numan Siddique wrote: > On Thu, Dec 17, 2020 at 3:47 PM Dumitru Ceara <dceara@redhat.com> 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 <dalvarez@redhat.com> >> >> Reported-by: Krzysztof Klimonda <kklimonda@syntaxhighlighted.com> >> >> 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 <numans@ovn.org> > Thanks for the review! > I think if we were maintaining a separate flow table for physical > flows, we could > have cleared that flow table before calling physical_run. > This might work too indeed. Also, I think the incremental processing engine could also be improved to avoid the ambiguity between NULL change handler and a change handler that always return false. In either case, I think it's better to make such intrusive changes only after we release 20.12. > Thanks > Numan > Regards, Dumitru
On 12/17/20 8:56 AM, Dumitru Ceara wrote: > On 12/17/20 2:30 PM, Numan Siddique wrote: >> On Thu, Dec 17, 2020 at 3:47 PM Dumitru Ceara <dceara@redhat.com> 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 <dalvarez@redhat.com> >>> >>> Reported-by: Krzysztof Klimonda <kklimonda@syntaxhighlighted.com> >>> >>> 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 <numans@ovn.org> >> > > Thanks for the review! > >> I think if we were maintaining a separate flow table for physical >> flows, we could >> have cleared that flow table before calling physical_run. >> > > This might work too indeed. Also, I think the incremental processing > engine could also be improved to avoid the ambiguity between NULL change > handler and a change handler that always return false. > > In either case, I think it's better to make such intrusive changes only > after we release 20.12. > I agree on waiting until after 20.12 is released. I think this change is indicative of other underlying issues, too. It's odd that skipping a change handler results in some computation not happening during the recompute. That being said, I pushed this to master and branch-20.12. >> Thanks >> Numan >> > > Regards, > Dumitru > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Thu, Dec 17, 2020 at 10:23 AM Dumitru Ceara <dceara@redhat.com> 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 <dalvarez@redhat.com> > 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 <dceara@redhat.com> > --- > 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 > -- > 1.8.3.1 > > Tested-by: Daniel Alvarez <dalvarez@redhat.com> Acked-by: Daniel Alvarez <dalvarez@redhat.com> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On 12/17/20 3:28 PM, Mark Michelson wrote: > On 12/17/20 8:56 AM, Dumitru Ceara wrote: >> On 12/17/20 2:30 PM, Numan Siddique wrote: >>> On Thu, Dec 17, 2020 at 3:47 PM Dumitru Ceara <dceara@redhat.com> 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 <dalvarez@redhat.com> >>>> >>>> Reported-by: Krzysztof Klimonda <kklimonda@syntaxhighlighted.com> >>>> >>>> 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 <numans@ovn.org> >>> >> >> Thanks for the review! >> >>> I think if we were maintaining a separate flow table for physical >>> flows, we could >>> have cleared that flow table before calling physical_run. >>> >> >> This might work too indeed. Also, I think the incremental processing >> engine could also be improved to avoid the ambiguity between NULL change >> handler and a change handler that always return false. >> >> In either case, I think it's better to make such intrusive changes only >> after we release 20.12. >> > > I agree on waiting until after 20.12 is released. I think this change is > indicative of other underlying issues, too. It's odd that skipping a > change handler results in some computation not happening during the > recompute. > > That being said, I pushed this to master and branch-20.12. > Thanks Mark! I sent backport patches for branch-20.09 and branch-20.06 (only with minor changes in the test): http://patchwork.ozlabs.org/project/ovn/patch/1608219845-6518-1-git-send-email-dceara@redhat.com/ http://patchwork.ozlabs.org/project/ovn/patch/1608220140-14239-1-git-send-email-dceara@redhat.com/ Regards, Dumitru
On Thu, Dec 17, 2020 at 6:28 AM Mark Michelson <mmichels@redhat.com> wrote: > > On 12/17/20 8:56 AM, Dumitru Ceara wrote: > > On 12/17/20 2:30 PM, Numan Siddique wrote: > >> On Thu, Dec 17, 2020 at 3:47 PM Dumitru Ceara <dceara@redhat.com> 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 <dalvarez@redhat.com> > >>> > >>> Reported-by: Krzysztof Klimonda <kklimonda@syntaxhighlighted.com> > >>> > >>> 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 <numans@ovn.org> > >> > > > > Thanks for the review! > > > >> I think if we were maintaining a separate flow table for physical > >> flows, we could > >> have cleared that flow table before calling physical_run. > >> > > > > This might work too indeed. Also, I think the incremental processing > > engine could also be improved to avoid the ambiguity between NULL change > > handler and a change handler that always return false. > > > > In either case, I think it's better to make such intrusive changes only > > after we release 20.12. > > > > I agree on waiting until after 20.12 is released. I think this change is > indicative of other underlying issues, too. It's odd that skipping a > change handler results in some computation not happening during the > recompute. Hi Mark, your concern is reasonable. It seems we are now paying the debt for not following the I-P model strictly. The en_physical_flow_changes_handler node is a wrapper node added for some convenient outcome, but it also added some tricky maintainability issues. It seems this fix may also further deviate on this path, and it becomes harder and harder to maintain the correctness of the engine going down this path. As mentioned by Numans, the dependency graph should be fixed by separating the flow_output to physical flows and logical flows, as we had discussed during the reviews when the en_physical_flow_changes_handler was initially added, to strictly follow the dependency graph and declarative data processing and avoid relying on special "triggers" in the code or ordering of change handlers. This was a big TODO. Please find the context here: https://mail.openvswitch.org/pipermail/ovs-dev/2020-May/370940.html Nevertheless, I agree to quickly fix it now with the approach used by this patch. Thanks, Han > > That being said, I pushed this to master and branch-20.12. > > >> Thanks > >> Numan > >> > > > > Regards, > > Dumitru > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Thu, Dec 17, 2020 at 10:33 PM Han Zhou <zhouhan@gmail.com> wrote: > > On Thu, Dec 17, 2020 at 6:28 AM Mark Michelson <mmichels@redhat.com> wrote: > > > > On 12/17/20 8:56 AM, Dumitru Ceara wrote: > > > On 12/17/20 2:30 PM, Numan Siddique wrote: > > >> On Thu, Dec 17, 2020 at 3:47 PM Dumitru Ceara <dceara@redhat.com> > 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 <dalvarez@redhat.com> > > >>> > > >>> Reported-by: Krzysztof Klimonda <kklimonda@syntaxhighlighted.com> > > >>> > > >>> 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 <numans@ovn.org> > > >> > > > > > > Thanks for the review! > > > > > >> I think if we were maintaining a separate flow table for physical > > >> flows, we could > > >> have cleared that flow table before calling physical_run. > > >> > > > > > > This might work too indeed. Also, I think the incremental processing > > > engine could also be improved to avoid the ambiguity between NULL change > > > handler and a change handler that always return false. > > > > > > In either case, I think it's better to make such intrusive changes only > > > after we release 20.12. > > > > > > > I agree on waiting until after 20.12 is released. I think this change is > > indicative of other underlying issues, too. It's odd that skipping a > > change handler results in some computation not happening during the > > recompute. > > Hi Mark, your concern is reasonable. It seems we are now paying the debt > for not following the I-P model strictly. The > en_physical_flow_changes_handler node is a wrapper node added for some > convenient outcome, but it also added some tricky maintainability issues. > It seems this fix may also further deviate on this path, and it becomes > harder and harder to maintain the correctness of the engine going down this > path. As mentioned by Numans, the dependency graph should be fixed by > separating the flow_output to physical flows and logical flows, as we had > discussed during the reviews when the en_physical_flow_changes_handler was > initially added, to strictly follow the dependency graph and declarative > data processing and avoid relying on special "triggers" in the code or > ordering of change handlers. This was a big TODO. Please find the context > here: https://mail.openvswitch.org/pipermail/ovs-dev/2020-May/370940.html > > Nevertheless, I agree to quickly fix it now with the approach used by this > patch. I had started working on the split of physical flow and logical flow output [1], but I got busy with other stuff and didn't resume this activity, which I should have (although I never forgot about it :)). It's time to revisit that I suppose. I will work on it. [1] - https://github.com/numansiddique/ovn/commit/085879980c597f5fff71c7a799c80f5594e62e32 Thanks Numan > > Thanks, > Han > > > > That being said, I pushed this to master and branch-20.12. > > > > >> Thanks > > >> Numan > > >> > > > > > > Regards, > > > Dumitru > > > > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Fri, Dec 18, 2020 at 1:22 AM Numan Siddique <numans@ovn.org> wrote: > > On Thu, Dec 17, 2020 at 10:33 PM Han Zhou <zhouhan@gmail.com> wrote: > > > > On Thu, Dec 17, 2020 at 6:28 AM Mark Michelson <mmichels@redhat.com> wrote: > > > > > > On 12/17/20 8:56 AM, Dumitru Ceara wrote: > > > > On 12/17/20 2:30 PM, Numan Siddique wrote: > > > >> On Thu, Dec 17, 2020 at 3:47 PM Dumitru Ceara <dceara@redhat.com> > > 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 <dalvarez@redhat.com> > > > >>> > > > >>> Reported-by: Krzysztof Klimonda <kklimonda@syntaxhighlighted.com> > > > >>> > > > >>> 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 <numans@ovn.org> > > > >> > > > > > > > > Thanks for the review! > > > > > > > >> I think if we were maintaining a separate flow table for physical > > > >> flows, we could > > > >> have cleared that flow table before calling physical_run. > > > >> > > > > > > > > This might work too indeed. Also, I think the incremental processing > > > > engine could also be improved to avoid the ambiguity between NULL change > > > > handler and a change handler that always return false. > > > > > > > > In either case, I think it's better to make such intrusive changes only > > > > after we release 20.12. > > > > > > > > > > I agree on waiting until after 20.12 is released. I think this change is > > > indicative of other underlying issues, too. It's odd that skipping a > > > change handler results in some computation not happening during the > > > recompute. > > > > Hi Mark, your concern is reasonable. It seems we are now paying the debt > > for not following the I-P model strictly. The > > en_physical_flow_changes_handler node is a wrapper node added for some > > convenient outcome, but it also added some tricky maintainability issues. > > It seems this fix may also further deviate on this path, and it becomes > > harder and harder to maintain the correctness of the engine going down this > > path. As mentioned by Numans, the dependency graph should be fixed by > > separating the flow_output to physical flows and logical flows, as we had > > discussed during the reviews when the en_physical_flow_changes_handler was > > initially added, to strictly follow the dependency graph and declarative > > data processing and avoid relying on special "triggers" in the code or > > ordering of change handlers. This was a big TODO. Please find the context > > here: https://mail.openvswitch.org/pipermail/ovs-dev/2020-May/370940.html > > > > Nevertheless, I agree to quickly fix it now with the approach used by this > > patch. > > I had started working on the split of physical flow and logical flow output > [1], but I got busy with other stuff and didn't resume this activity, > which I should > have (although I never forgot about it :)). > > It's time to revisit that I suppose. I will work on it. > > [1] - https://github.com/numansiddique/ovn/commit/085879980c597f5fff71c7a799c80f5594e62e32 > > Thanks > Numan > Thanks Numan! > > > > Thanks, > > Han > > > > > > That being said, I pushed this to master and branch-20.12. > > > > > > >> Thanks > > > >> Numan > > > >> > > > > > > > > Regards, > > > > Dumitru > > > > > > > > _______________________________________________ > > > > dev mailing list > > > > dev@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
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
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 <dalvarez@redhat.com> 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 <dceara@redhat.com> --- controller/ovn-controller.c | 30 ++++++++++++++----- tests/ovn.at | 72 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 8 deletions(-)