[ovs-dev] controller: fix physical flow update for localport

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 <lorenzo.bianconi@redhat.com>
---
 controller/ovn-controller.c |  1 +
 controller/physical.c       |  6 +++++-
 tests/ovn.at                | 21 +++++++++++++++++++++
 3 files changed, 27 insertions(+), 1 deletion(-)

On Thu, May 13, 2021 at 8:33 AM Lorenzo Bianconi <
lorenzo.bianconi@redhat.com> 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 <lorenzo.bianconi@redhat.com>
> ---
>  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.

>      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.

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.

>          }
>
>          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?

Thanks again for fixing the issue!

Han
> --
> 2.31.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

> On Thu, May 13, 2021 at 8:33 AM Lorenzo Bianconi <
> lorenzo.bianconi@redhat.com> 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 <lorenzo.bianconi@redhat.com>
> > ---
> >  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
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev