On Wed, Sep 15, 2021 at 8:12 PM Odintsov Vladislav <[email protected]> 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/[email protected]/ > > Tested-by: Vladislav Odintsov <[email protected]<mailto:[email protected]>>
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 > <[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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
