[ovs-dev,v2] controller: do not claim localports

Localports should not be binded to any specific controller but should be
available to each hv however in the current codebase consider_iface_claim
routine is claiming the localport for a specific chassis.
Fix the issue skipping localports in consider_iface_claim routine.

https://bugzilla.redhat.com/show_bug.cgi?id=1937662

Reviewed-by: Mark D. Gray <mark.d.gray@redhat.com>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
Changes since v1:
- move lport_lookup before local_binding_find
- fix commit message
---
 controller/binding.c | 10 ++++++++--
 tests/ovn.at         |  3 +++
 2 files changed, 11 insertions(+), 2 deletions(-)

LGTM

Acked-by: Mark Michelson <mmichels@redhat.com>

On 3/26/21 1:15 PM, Lorenzo Bianconi wrote:
> Localports should not be binded to any specific controller but should be
> available to each hv however in the current codebase consider_iface_claim
> routine is claiming the localport for a specific chassis.
> Fix the issue skipping localports in consider_iface_claim routine.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1937662
> 
> Reviewed-by: Mark D. Gray <mark.d.gray@redhat.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
> Changes since v1:
> - move lport_lookup before local_binding_find
> - fix commit message
> ---
>   controller/binding.c | 10 ++++++++--
>   tests/ovn.at         |  3 +++
>   2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/controller/binding.c b/controller/binding.c
> index 4e6c75696..7bb26f819 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -1799,6 +1799,13 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec,
>       update_local_lports(iface_id, b_ctx_out);
>       smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, iface_id);
>   
> +    const struct sbrec_port_binding *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;
> +    }
> +
>       struct local_binding *lbinding =
>           local_binding_find(b_ctx_out->local_bindings, iface_id);
>   
> @@ -1810,8 +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(
> -            b_ctx_in->sbrec_port_binding_by_name, lbinding->name);
> +        lbinding->pb = pb;
>           if (lbinding->pb && !strcmp(lbinding->pb->type, "virtual")) {
>               lbinding->pb = NULL;
>           }
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 391a8bcd9..d36e2483f 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
>

On 26/03/2021 17:15, Lorenzo Bianconi wrote:
> Localports should not be binded to any specific controller but should be
> available to each hv however in the current codebase consider_iface_claim
> routine is claiming the localport for a specific chassis.
> Fix the issue skipping localports in consider_iface_claim routine.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1937662
> 
> Reviewed-by: Mark D. Gray <mark.d.gray@redhat.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
> Changes since v1:
> - move lport_lookup before local_binding_find
> - fix commit message
> ---
>  controller/binding.c | 10 ++++++++--
>  tests/ovn.at         |  3 +++
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/controller/binding.c b/controller/binding.c
> index 4e6c75696..7bb26f819 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -1799,6 +1799,13 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec,
>      update_local_lports(iface_id, b_ctx_out);
>      smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, iface_id);
>  
> +    const struct sbrec_port_binding *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;
> +    }
> +
>      struct local_binding *lbinding =
>          local_binding_find(b_ctx_out->local_bindings, iface_id);
>  
> @@ -1810,8 +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(
> -            b_ctx_in->sbrec_port_binding_by_name, lbinding->name);
> +        lbinding->pb = pb;

'pb' could be 'NULL' here but I don't think that matters?
>          if (lbinding->pb && !strcmp(lbinding->pb->type, "virtual")) {
>              lbinding->pb = NULL;
>          }
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 391a8bcd9..d36e2483f 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
> 

Tested it and it seems to work as expected. Thanks Lorenzo!!

Acked-by: Mark D. Gray <mark.d.gray@redhat.com>

On Tue, Mar 30, 2021 at 6:12 AM Mark Gray <mark.d.gray@redhat.com> wrote:
>
> On 26/03/2021 17:15, Lorenzo Bianconi wrote:
> > Localports should not be binded to any specific controller but should be
> > available to each hv however in the current codebase consider_iface_claim
> > routine is claiming the localport for a specific chassis.
> > Fix the issue skipping localports in consider_iface_claim routine.
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=1937662
> >
> > Reviewed-by: Mark D. Gray <mark.d.gray@redhat.com>
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>

Hi Lorenzo,

This patch needs a rebase.  Can you please submit v3 resolving the conflicts.

Numan

> > ---
> > Changes since v1:
> > - move lport_lookup before local_binding_find
> > - fix commit message
> > ---
> >  controller/binding.c | 10 ++++++++--
> >  tests/ovn.at         |  3 +++
> >  2 files changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/controller/binding.c b/controller/binding.c
> > index 4e6c75696..7bb26f819 100644
> > --- a/controller/binding.c
> > +++ b/controller/binding.c
> > @@ -1799,6 +1799,13 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec,
> >      update_local_lports(iface_id, b_ctx_out);
> >      smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, iface_id);
> >
> > +    const struct sbrec_port_binding *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;
> > +    }
> > +
> >      struct local_binding *lbinding =
> >          local_binding_find(b_ctx_out->local_bindings, iface_id);
> >
> > @@ -1810,8 +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(
> > -            b_ctx_in->sbrec_port_binding_by_name, lbinding->name);
> > +        lbinding->pb = pb;
>
> 'pb' could be 'NULL' here but I don't think that matters?
> >          if (lbinding->pb && !strcmp(lbinding->pb->type, "virtual")) {
> >              lbinding->pb = NULL;
> >          }
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 391a8bcd9..d36e2483f 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
> >
>
> Tested it and it seems to work as expected. Thanks Lorenzo!!
>
> Acked-by: Mark D. Gray <mark.d.gray@redhat.com>
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

> On Tue, Mar 30, 2021 at 6:12 AM Mark Gray <mark.d.gray@redhat.com> wrote:
> >
> > On 26/03/2021 17:15, Lorenzo Bianconi wrote:
> > > Localports should not be binded to any specific controller but should be
> > > available to each hv however in the current codebase consider_iface_claim
> > > routine is claiming the localport for a specific chassis.
> > > Fix the issue skipping localports in consider_iface_claim routine.
> > >
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1937662
> > >
> > > Reviewed-by: Mark D. Gray <mark.d.gray@redhat.com>
> > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> 
> Hi Lorenzo,
> 
> This patch needs a rebase.  Can you please submit v3 resolving the conflicts.

ack Numan, I will post v3 soon.

Regards,
Lorenzo

> 
> Numan
> 
> > > ---
> > > Changes since v1:
> > > - move lport_lookup before local_binding_find
> > > - fix commit message
> > > ---
> > >  controller/binding.c | 10 ++++++++--
> > >  tests/ovn.at         |  3 +++
> > >  2 files changed, 11 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/controller/binding.c b/controller/binding.c
> > > index 4e6c75696..7bb26f819 100644
> > > --- a/controller/binding.c
> > > +++ b/controller/binding.c
> > > @@ -1799,6 +1799,13 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec,
> > >      update_local_lports(iface_id, b_ctx_out);
> > >      smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, iface_id);
> > >
> > > +    const struct sbrec_port_binding *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;
> > > +    }
> > > +
> > >      struct local_binding *lbinding =
> > >          local_binding_find(b_ctx_out->local_bindings, iface_id);
> > >
> > > @@ -1810,8 +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(
> > > -            b_ctx_in->sbrec_port_binding_by_name, lbinding->name);
> > > +        lbinding->pb = pb;
> >
> > 'pb' could be 'NULL' here but I don't think that matters?
> > >          if (lbinding->pb && !strcmp(lbinding->pb->type, "virtual")) {
> > >              lbinding->pb = NULL;
> > >          }
> > > diff --git a/tests/ovn.at b/tests/ovn.at
> > > index 391a8bcd9..d36e2483f 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
> > >
> >
> > Tested it and it seems to work as expected. Thanks Lorenzo!!
> >
> > Acked-by: Mark D. Gray <mark.d.gray@redhat.com>
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>