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/[email protected]/ Tested-by: Vladislav Odintsov <[email protected]<mailto:[email protected]>> Regards, Vladislav Odintsov On 8 Sep 2021, at 20:14, Numan Siddique <[email protected]<mailto:[email protected]>> wrote: On Wed, Sep 8, 2021 at 5:59 AM Mark Gray <[email protected]<mailto:[email protected]>> wrote: On 07/09/2021 18:15, [email protected]<mailto:[email protected]> wrote: From: Numan Siddique <[email protected]<mailto:[email protected]>> 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 <[email protected]<mailto:[email protected]>> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387435.html Signed-off-by: Numan Siddique <[email protected]<mailto:[email protected]>> --- 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 [email protected]<mailto:[email protected]> https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected]<mailto:[email protected]> https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
