> On Thu, May 13, 2021 at 8:33 AM Lorenzo Bianconi <
> [email protected]> wrote:
> >
> > Properly update logical/openflow flows for localport removing the
> > interface from the ovs bridge. Openflows in table 65 are not recomputed
> > removing a localport from an ovs-bridge and the ovs bridge ends-up with
> > a stale configuration adding the interface back. Fix the issue taking
> > care of localport special case in physical_handle_ovs_iface_changes
> > routine.
> >
> > Signed-off-by: Lorenzo Bianconi <[email protected]>
> > ---
> > controller/ovn-controller.c | 1 +
> > controller/physical.c | 6 +++++-
> > tests/ovn.at | 21 +++++++++++++++++++++
> > 3 files changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 67c51a86f..8514e35ea 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -1836,6 +1836,7 @@ en_physical_flow_changes_run(struct engine_node
> *node, void *data)
> > {
> > struct ed_type_pfc_data *pfc_tdata = data;
> > pfc_tdata->recompute_physical_flows = true;
> > + pfc_tdata->ovs_ifaces_changed = true;
>
> Thanks for fixing this.
Hi Han,
thx for the review.
>
> > engine_set_node_state(node, EN_UPDATED);
> > }
> >
> > diff --git a/controller/physical.c b/controller/physical.c
> > index 96c959d18..725959678 100644
> > --- a/controller/physical.c
> > +++ b/controller/physical.c
> > @@ -1874,7 +1874,11 @@ physical_handle_ovs_iface_changes(struct
> physical_ctx *p_ctx,
> > const struct sbrec_port_binding *lb_pb =
> > local_binding_get_primary_pb(p_ctx->local_bindings,
> iface_id);
> > if (!lb_pb) {
> > - continue;
> > + lb_pb =
> lport_lookup_by_name(p_ctx->sbrec_port_binding_by_name,
> > + iface_id);
> > + if (!lb_pb || strcmp(lb_pb->type, "localport")) {
> > + continue;
> > + }
>
> It would be better to add some comments to clarify why localport needs this
> special processing but other port types don't.
> I think the reason why the regular VIFs don't need proceeding here is
> because there will be port-binding update later after the chassis unclaim
> the port, and in port-binding handler the related flows will be removed.
> However, for localport ports, the port-binding is not updated because it is
> shared by chassises, so the flow removing should be taken care as soon as
> the OVS interface is deleted. And I think why you need to call
> lport_lookup_by_name() to check the existence of port-binding is because
> when the port-binding doesn't exists any more, it means we already received
> a port-binding update and handled it, so the related flow should have been
> removed, thus no need to handle the OVS interface change here, right? It
> took me some effort to understand this. If my understanding is correct,
> please add some comments so that people can follow the logic easier.
sure, I will add the comment in v2.
>
> In addition, I think this if block can be optimized a little, so that the
> lport_lookup_by_name() is called only if it is localport type.
I guess we need to run lport_lookup_by_name in order to check if it is a
localport or am I missing something?
>
> > }
> >
> > int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0;
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 747967576..06ec60a02 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -11870,6 +11870,27 @@ AT_CHECK([
> > test 0 -eq $pkts
> > ])
> >
> > +AT_CHECK([ovs-ofctl dump-flows br-int |awk '/output/{print substr($8,
> 16, 16)}' |sort], [0], [dnl
> > +1
> > +2
> > +3
> > +])
> > +
> > +# remove the localport from br-int and re-create it
> > +check ovs-vsctl del-port vif2
> > +AT_CHECK([ovs-ofctl dump-flows br-int |awk '/output/{print substr($8,
> 16, 16)}' |sort], [0], [dnl
> > +1
> > +3
> > +])
> > +
> > +check ovs-vsctl add-port br-int vif2 \
> > + -- set Interface vif2 external-ids:iface-id=lsp
> > +AT_CHECK([ovs-ofctl dump-flows br-int |awk '/output/{print substr($8,
> 16, 16)}' |sort], [0], [dnl
> > +1
> > +3
> > +4
> > +])
> > +
> > OVN_CLEANUP([hv1])
> > AT_CLEANUP
> > ])
>
> Thanks for updating the test case. But I have two comments here:
> 1) It seems you are not testing the localport here, because "lsp" is a
> regular port while "lp" is the localport port in this test case. Maybe you
> were to delete vif1.
> 2) The test case itself is about "localport suppress gARP", so I'd suggest
> not hijacking this test case to cover the scenario you are fixing, but
> instead either updating the general localport test case "ovn -- 2 HVs, 1
> lport/HV, localport ports", or create a new one just for this scenario.
> What do you think?
sure, I will fix them in v2
>
> Thanks again for fixing the issue!
yw :)
Regards,
Lorenzo
>
> Han
> > --
> > 2.31.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
