[ovs-dev] [PATCH ovn] controller: do not claim locaports

Lorenzo Bianconi lorenzo.bianconi at redhat.com
Fri Mar 26 15:14:44 UTC 2021


> s/locaports/localports
> 
> On 19/03/2021 15:28, Lorenzo Bianconi wrote:
> > Localports should not be binded to any specific controller but should be
> maybe "Local ports should not be bound...."
> > available to each one. Fix the issue avoiding port claims in
> > consider_iface_claim routine for localport. 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1937662
> In the commit message, could you explain a little more about what the
> bug was an how this is resolving it?
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
> > ---
> >  controller/binding.c | 9 ++++++++-
> >  tests/ovn.at         | 3 +++
> >  2 files changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/controller/binding.c b/controller/binding.c
> > index 4e6c75696..dec4baebf 100644
> > --- a/controller/binding.c
> > +++ b/controller/binding.c
> > @@ -1796,6 +1796,8 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec,
> >                       struct binding_ctx_out *b_ctx_out,
> >                       struct hmap *qos_map)
> >  {
> > +    const struct sbrec_port_binding *pb = NULL;
> > +
> >      update_local_lports(iface_id, b_ctx_out);
> >      smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, iface_id);
> >  
> > @@ -1803,6 +1805,11 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec,
> >          local_binding_find(b_ctx_out->local_bindings, iface_id);
> >  
> >      if (!lbinding) {
> > +        pb = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name, iface_id);
> > +        if (pb && get_lport_type(pb) == LP_LOCALPORT) {
> > +            /* nothing to do for localports. */
> > +            return true;
> > +        }
> 
> This is a bit of a nit, but but do we care if there is a lbinding for
> this case? They seem to be orthogonal. Maybe it could be moved to the
> top of this function?

Hi Mark,

thx for the review.
it is just an optimization to avoid the lookup if not necessary but I guess we
can move it above. I will fix it in v2.

> 
> >          lbinding = local
> 
> _binding_create(iface_id, iface_rec, NULL, BT_VIF);
> >          local_binding_add(b_ctx_out->local_bindings, lbinding);
> >      } else {
> > @@ -1810,7 +1817,7 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec,
> >      }
> >  
> >      if (!lbinding->pb || strcmp(lbinding->name, lbinding->pb->logical_port)) {
> > -        lbinding->pb = lport_lookup_by_name(
> > +        lbinding->pb = pb ? pb : lport_lookup_by_name(
> 
> Why is this change needed? It should work without this change?

same as before, just an optimization. I will fix it in v2.

Regards,
Lorenzo

> 
> >              b_ctx_in->sbrec_port_binding_by_name, lbinding->name);
> >          if (lbinding->pb && !strcmp(lbinding->pb->type, "virtual")) {
> >              lbinding->pb = NULL;
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index b751d6db2..655ff08f0 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -11567,6 +11567,9 @@ test_packet() {
> >      fi
> >  }
> >  
> > +# Check the localport is not claimed by the hvs
> > +chassis_lp=$(fetch_column port_binding chassis logical_port=lp01)
> > +AT_CHECK([$(fetch_column port_binding chassis logical_port=lp01)], [0])
> >  
> >  # lp11 and lp21 are on different hypervisors
> >  test_packet 11 f0:00:00:00:00:21 f0:00:00:00:00:11 1121 lp21 lp21
> > 
> 
> I did some tests and it seemed to work as expected. Thanks Lorenzo.
> 
> Reviewed-by: Mark D. Gray <mark.d.gray at redhat.com>
> 


More information about the dev mailing list