On Thu, Dec 17, 2020 at 9:19 PM Dumitru Ceara <[email protected]> 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-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]>
> Reported-by: Krzysztof Klimonda <[email protected]>
> Acked-by: Numan Siddique <[email protected]>
> (cherry picked from commit a2bf85296b2d0abe807d1cadf2ed29482318df11)
Thanks. Backported to respective branches.
Numan
> ---
> controller/ovn-controller.c | 30 ++++++++++++-----
> tests/ovn.at | 78
> +++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 100 insertions(+), 8 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index ed1a99e..0ffaf34 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -1488,6 +1488,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
> @@ -1965,6 +1975,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);
> @@ -1972,12 +1989,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;
> }
>
> @@ -2184,11 +2195,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 3f9d2c8..07eb952 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -20888,6 +20888,84 @@ 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
> +
> +ovn-nbctl ls-add sw
> +ovn-nbctl lsp-add sw lsp1
> +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.
> +OVS_WAIT_UNTIL([ovn-sbctl --columns _uuid find Chassis name=hv1 | wc -l],
> [0], [dnl
> +1
> +])
> +ch=$(ovn-sbctl --bare --columns _uuid find Chassis name=hv1)
> +OVS_WAIT_UNTIL([ovn-sbctl --columns _uuid find Port_Binding
> logical_port=lsp1 chassis=$ch | wc -l], [0]. [dnl
> +1
> +])
> +OVS_WAIT_UNTIL([ovn-sbctl --columns _uuid find Port_Binding
> logical_port=lsp2 chassis=$ch | wc -l], [0]. [dnl
> +1
> +])
> +
> +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
> +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
> +
> AT_SETUP([ovn -- ARP replies for SNAT external ips])
> ovn_start
>
> --
> 1.8.3.1
>
> _______________________________________________
> 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
