[ovs-dev] [PATCH ovn] controller: Fall back to full recompute of pflow_output for vtep lport changes.

[Numan Siddique]

On Wed, Sep 15, 2021 at 8:12 PM Odintsov Vladislav <VlOdintsov at croc.ru> wrote:
>
> Hi Numan,
>
> for me this patch solves the issue.
> Actually, it’s a good point about adding test for ovn-controller-vtep to eliminate this problem in future.
> However there was another problem with HW VTEP support in OVN with using stateful services in the datapath.
>
> I’ve sent a patch series with a test, which reproduces the mentioned problem. Bigfix is in a third patch. Please try those patches:
> https://patchwork.ozlabs.org/project/ovn/cover/20210916000624.1609-1-odivlad@gmail.com/
>
> Tested-by: Vladislav Odintsov <odivlad at gmail.com<mailto:odivlad at gmail.com>>

Thanks for testing it out and adding the test case in your other series.

I applied this patch to the main branch.

Numan

>
> Regards,
> Vladislav Odintsov
>
> On 8 Sep 2021, at 20:14, Numan Siddique <numans at ovn.org<mailto:numans at ovn.org>> wrote:
>
> On Wed, Sep 8, 2021 at 5:59 AM Mark Gray <mark.d.gray at redhat.com<mailto:mark.d.gray at redhat.com>> wrote:
>
> On 07/09/2021 18:15, numans at ovn.org<mailto:numans at ovn.org> wrote:
> From: Numan Siddique <numans at ovn.org<mailto:numans at ovn.org>>
>
> When a vtep logical port changes, necessary flows are not added as
> expected.  This is because the function physical_handle_flows_for_lport()
> in physical.c does not add flows required for vtep logical ports.
> These flows are added by physical_run(). So fall back to full recompute
> of pflow_output engine node when vtep lports change.
>
> Reported-by: Odintsov Vladislav <VlOdintsov at croc.ru<mailto:VlOdintsov at croc.ru>>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387435.html
> Signed-off-by: Numan Siddique <numans at ovn.org<mailto:numans at ovn.org>>
> ---
> controller/ovn-controller.c | 11 ++++++++---
> controller/physical.c       |  9 ++++++++-
> controller/physical.h       |  2 +-
> 3 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 0031a1035..d98ebbbfa 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -2867,7 +2867,10 @@ pflow_output_sb_port_binding_handler(struct engine_node *node,
>     const struct sbrec_port_binding *pb;
>     SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, p_ctx.port_binding_table) {
>         bool removed = sbrec_port_binding_is_deleted(pb);
> -        physical_handle_flows_for_lport(pb, removed, &p_ctx, &pfo->flow_table);
> +        if (!physical_handle_flows_for_lport(pb, removed, &p_ctx,
> +                                             &pfo->flow_table)) {
> +            return false;
> +        }
>     }
>
>     engine_set_node_state(node, EN_UPDATED);
> @@ -2930,8 +2933,10 @@ pflow_output_runtime_data_handler(struct engine_node *node, void *data)
>             struct tracked_lport *lport = shash_node->data;
>             bool removed =
>                 lport->tracked_type == TRACKED_RESOURCE_REMOVED ? true: false;
> -            physical_handle_flows_for_lport(lport->pb, removed, &p_ctx,
> -                                            &pfo->flow_table);
> +            if (!physical_handle_flows_for_lport(lport->pb, removed, &p_ctx,
> +                                                 &pfo->flow_table)) {
> +                return false;
> +            }
>         }
>     }
>
> diff --git a/controller/physical.c b/controller/physical.c
> index 6f2c1cea0..ffb9f9952 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -1509,11 +1509,16 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve,
>     sset_destroy(&remote_chassis);
> }
>
> -void
> +bool
> physical_handle_flows_for_lport(const struct sbrec_port_binding *pb,
>                                 bool removed, struct physical_ctx *p_ctx,
>                                 struct ovn_desired_flow_table *flow_table)
> {
> +    if (!strcmp(pb->type, "vtep")) {
> +        /* Cannot handle changes to vtep lports (yet). */
> +        return false;
> +    }
> +
>     ofctrl_remove_flows(flow_table, &pb->header_.uuid);
>
>     if (!strcmp(pb->type, "external")) {
> @@ -1553,6 +1558,8 @@ physical_handle_flows_for_lport(const struct sbrec_port_binding *pb,
>                               p_ctx->chassis, flow_table, &ofpacts);
>         ofpbuf_uninit(&ofpacts);
>     }
> +
> +    return true;
> }
>
> void
> diff --git a/controller/physical.h b/controller/physical.h
> index c4540ad7f..ee4b1ae1f 100644
> --- a/controller/physical.h
> +++ b/controller/physical.h
> @@ -65,7 +65,7 @@ void physical_run(struct physical_ctx *,
>                   struct ovn_desired_flow_table *);
> void physical_handle_mc_group_changes(struct physical_ctx *,
>                                       struct ovn_desired_flow_table *);
> -void physical_handle_flows_for_lport(const struct sbrec_port_binding *,
> +bool physical_handle_flows_for_lport(const struct sbrec_port_binding *,
>                                      bool removed,
>                                      struct physical_ctx *,
>                                      struct ovn_desired_flow_table *);
>
>
> The change looks ok to me. I am not really sure how to test it though?
> Any suggestions?
>
> Thanks for the review.  Vladislav will test this patch out.  He confirmed to me
> in the thread where he reported the issue.
>
> Numan
>
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org<mailto:dev at openvswitch.org>
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org<mailto:dev at openvswitch.org>
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev