Message ID | 20210907171507.1617879-1-numans@ovn.org |
---|---|
State | Accepted |
Headers | show |
Series | [ovs-dev] controller: Fall back to full recompute of pflow_output for vtep lport changes. | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On 07/09/2021 18:15, numans@ovn.org wrote: > From: Numan Siddique <numans@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@croc.ru> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387435.html > Signed-off-by: Numan Siddique <numans@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?
On Wed, Sep 8, 2021 at 5:59 AM Mark Gray <mark.d.gray@redhat.com> wrote: > > On 07/09/2021 18:15, numans@ovn.org wrote: > > From: Numan Siddique <numans@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@croc.ru> > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387435.html > > Signed-off-by: Numan Siddique <numans@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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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@gmail.com<mailto:odivlad@gmail.com>> Regards, Vladislav Odintsov On 8 Sep 2021, at 20:14, Numan Siddique <numans@ovn.org<mailto:numans@ovn.org>> wrote: On Wed, Sep 8, 2021 at 5:59 AM Mark Gray <mark.d.gray@redhat.com<mailto:mark.d.gray@redhat.com>> wrote: On 07/09/2021 18:15, numans@ovn.org<mailto:numans@ovn.org> wrote: From: Numan Siddique <numans@ovn.org<mailto:numans@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@croc.ru<mailto:VlOdintsov@croc.ru>> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387435.html Signed-off-by: Numan Siddique <numans@ovn.org<mailto:numans@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
On Wed, Sep 15, 2021 at 8:12 PM Odintsov Vladislav <VlOdintsov@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@gmail.com<mailto:odivlad@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@ovn.org<mailto:numans@ovn.org>> wrote: > > On Wed, Sep 8, 2021 at 5:59 AM Mark Gray <mark.d.gray@redhat.com<mailto:mark.d.gray@redhat.com>> wrote: > > On 07/09/2021 18:15, numans@ovn.org<mailto:numans@ovn.org> wrote: > From: Numan Siddique <numans@ovn.org<mailto:numans@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@croc.ru<mailto:VlOdintsov@croc.ru>> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387435.html > Signed-off-by: Numan Siddique <numans@ovn.org<mailto:numans@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@openvswitch.org<mailto:dev@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ > dev mailing list > dev@openvswitch.org<mailto: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 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 *);