diff mbox series

[ovs-dev,v2] northd: Fix incorrect warning logs when handling port binding changes.

Message ID 20230816082740.6665-1-numans@ovn.org
State Changes Requested
Headers show
Series [ovs-dev,v2] northd: Fix incorrect warning logs when handling port binding changes. | expand

Checks

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 success github build: passed

Commit Message

Numan Siddique Aug. 16, 2023, 8:27 a.m. UTC 
From: Numan Siddique <numans@ovn.org>

When changes to port bindings corresponding to router ports are
handled by northd engine node, incorrect warning logs (like below)
are logged.

----
northd|WARN|A port-binding for lrp0 is created but the LSP is not found.
----

Fix these warnings.

Fixes: 3b120ccf7f7c ("northd: Incremental processing of SB port_binding in "northd" node.")

CC: Han Zhou <hzhou@ovn.org>
Signed-off-by: Numan Siddique <numans@ovn.org>
---

v1 -> v2
-------
  * v1 was not returning false in northd_handle_sb_port_binding_changes()
    for router port - port bindings because of which IPv6 PD system test
    was failing.  Fixed it in v2.

 controller/binding.c |  75 --------------------------------
 controller/binding.h |  20 +--------
 lib/ovn-util.c       | 101 +++++++++++++++++++++++++++++++++++++++++++
 lib/ovn-util.h       |  21 +++++++++
 northd/northd.c      |   7 +++
 5 files changed, 130 insertions(+), 94 deletions(-)

Comments

Han Zhou Aug. 17, 2023, 6:17 a.m. UTC | #1 
On Wed, Aug 16, 2023 at 1:27 AM <numans@ovn.org> wrote:
>
> From: Numan Siddique <numans@ovn.org>
>
> When changes to port bindings corresponding to router ports are
> handled by northd engine node, incorrect warning logs (like below)
> are logged.
>
> ----
> northd|WARN|A port-binding for lrp0 is created but the LSP is not found.
> ----
>
> Fix these warnings.
>
> Fixes: 3b120ccf7f7c ("northd: Incremental processing of SB port_binding
in "northd" node.")
>
> CC: Han Zhou <hzhou@ovn.org>
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---
>
> v1 -> v2
> -------
>   * v1 was not returning false in northd_handle_sb_port_binding_changes()
>     for router port - port bindings because of which IPv6 PD system test
>     was failing.  Fixed it in v2.
>
>  controller/binding.c |  75 --------------------------------
>  controller/binding.h |  20 +--------
>  lib/ovn-util.c       | 101 +++++++++++++++++++++++++++++++++++++++++++
>  lib/ovn-util.h       |  21 +++++++++
>  northd/northd.c      |   7 +++
>  5 files changed, 130 insertions(+), 94 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 3ac0c35df..a521f2828 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -756,7 +756,6 @@ static void binding_lport_clear_port_sec(struct
binding_lport *);
>  static bool binding_lport_update_port_sec(
>      struct binding_lport *, const struct sbrec_port_binding *);
>
> -static char *get_lport_type_str(enum en_lport_type lport_type);
>  static bool ovs_iface_matches_lport_iface_id_ver(
>      const struct ovsrec_interface *,
>      const struct sbrec_port_binding *);
> @@ -1055,80 +1054,6 @@ binding_dump_local_bindings(struct
local_binding_data *lbinding_data,
>      free(nodes);
>  }
>
> -static bool
> -is_lport_vif(const struct sbrec_port_binding *pb)
> -{
> -    return !pb->type[0];
> -}
> -
> -enum en_lport_type
> -get_lport_type(const struct sbrec_port_binding *pb)
> -{
> -    if (is_lport_vif(pb)) {
> -        if (pb->parent_port && pb->parent_port[0]) {
> -            return LP_CONTAINER;
> -        }
> -        return LP_VIF;
> -    } else if (!strcmp(pb->type, "patch")) {
> -        return LP_PATCH;
> -    } else if (!strcmp(pb->type, "chassisredirect")) {
> -        return LP_CHASSISREDIRECT;
> -    } else if (!strcmp(pb->type, "l3gateway")) {
> -        return LP_L3GATEWAY;
> -    } else if (!strcmp(pb->type, "localnet")) {
> -        return LP_LOCALNET;
> -    } else if (!strcmp(pb->type, "localport")) {
> -        return LP_LOCALPORT;
> -    } else if (!strcmp(pb->type, "l2gateway")) {
> -        return LP_L2GATEWAY;
> -    } else if (!strcmp(pb->type, "virtual")) {
> -        return LP_VIRTUAL;
> -    } else if (!strcmp(pb->type, "external")) {
> -        return LP_EXTERNAL;
> -    } else if (!strcmp(pb->type, "remote")) {
> -        return LP_REMOTE;
> -    } else if (!strcmp(pb->type, "vtep")) {
> -        return LP_VTEP;
> -    }
> -
> -    return LP_UNKNOWN;
> -}
> -
> -static char *
> -get_lport_type_str(enum en_lport_type lport_type)
> -{
> -    switch (lport_type) {
> -    case LP_VIF:
> -        return "VIF";
> -    case LP_CONTAINER:
> -        return "CONTAINER";
> -    case LP_VIRTUAL:
> -        return "VIRTUAL";
> -    case LP_PATCH:
> -        return "PATCH";
> -    case LP_CHASSISREDIRECT:
> -        return "CHASSISREDIRECT";
> -    case LP_L3GATEWAY:
> -        return "L3GATEWAY";
> -    case LP_LOCALNET:
> -        return "PATCH";
> -    case LP_LOCALPORT:
> -        return "LOCALPORT";
> -    case LP_L2GATEWAY:
> -        return "L2GATEWAY";
> -    case LP_EXTERNAL:
> -        return "EXTERNAL";
> -    case LP_REMOTE:
> -        return "REMOTE";
> -    case LP_VTEP:
> -        return "VTEP";
> -    case LP_UNKNOWN:
> -        return "UNKNOWN";
> -    }
> -
> -    OVS_NOT_REACHED();
> -}
> -
>  void
>  set_pb_chassis_in_sbrec(const struct sbrec_port_binding *pb,
>                          const struct sbrec_chassis *chassis_rec,
> diff --git a/controller/binding.h b/controller/binding.h
> index 235e5860d..24bc84079 100644
> --- a/controller/binding.h
> +++ b/controller/binding.h
> @@ -22,6 +22,7 @@
>  #include "openvswitch/hmap.h"
>  #include "openvswitch/uuid.h"
>  #include "openvswitch/list.h"
> +# include "lib/ovn-util.h"
>  #include "sset.h"
>  #include "lport.h"
>
> @@ -217,25 +218,6 @@ void port_binding_set_down(const struct
sbrec_chassis *chassis_rec,
>                             const char *iface_id,
>                             const struct uuid *pb_uuid);
>
> -/* Corresponds to each Port_Binding.type. */
> -enum en_lport_type {
> -    LP_UNKNOWN,
> -    LP_VIF,
> -    LP_CONTAINER,
> -    LP_PATCH,
> -    LP_L3GATEWAY,
> -    LP_LOCALNET,
> -    LP_LOCALPORT,
> -    LP_L2GATEWAY,
> -    LP_VTEP,
> -    LP_CHASSISREDIRECT,
> -    LP_VIRTUAL,
> -    LP_EXTERNAL,
> -    LP_REMOTE
> -};
> -
> -enum en_lport_type get_lport_type(const struct sbrec_port_binding *);
> -
>  /* This structure represents a logical port (or port binding)
>   * which is associated with 'struct local_binding'.
>   *
> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> index 080ad4c0c..3a237b7fe 100644
> --- a/lib/ovn-util.c
> +++ b/lib/ovn-util.c
> @@ -1168,3 +1168,104 @@ encode_fqdn_string(const char *fqdn, size_t *len)
>
>      return encoded;
>  }
> +
> +static bool
> +is_lport_vif(const struct sbrec_port_binding *pb)
> +{
> +    return !pb->type[0];
> +}
> +
> +enum en_lport_type
> +get_lport_type(const struct sbrec_port_binding *pb)
> +{
> +    if (is_lport_vif(pb)) {
> +        if (pb->parent_port && pb->parent_port[0]) {
> +            return LP_CONTAINER;
> +        }
> +        return LP_VIF;
> +    } else if (!strcmp(pb->type, "patch")) {
> +        return LP_PATCH;
> +    } else if (!strcmp(pb->type, "chassisredirect")) {
> +        return LP_CHASSISREDIRECT;
> +    } else if (!strcmp(pb->type, "l3gateway")) {
> +        return LP_L3GATEWAY;
> +    } else if (!strcmp(pb->type, "localnet")) {
> +        return LP_LOCALNET;
> +    } else if (!strcmp(pb->type, "localport")) {
> +        return LP_LOCALPORT;
> +    } else if (!strcmp(pb->type, "l2gateway")) {
> +        return LP_L2GATEWAY;
> +    } else if (!strcmp(pb->type, "virtual")) {
> +        return LP_VIRTUAL;
> +    } else if (!strcmp(pb->type, "external")) {
> +        return LP_EXTERNAL;
> +    } else if (!strcmp(pb->type, "remote")) {
> +        return LP_REMOTE;
> +    } else if (!strcmp(pb->type, "vtep")) {
> +        return LP_VTEP;
> +    }
> +
> +    return LP_UNKNOWN;
> +}
> +
> +char *
> +get_lport_type_str(enum en_lport_type lport_type)
> +{
> +    switch (lport_type) {
> +    case LP_VIF:
> +        return "VIF";
> +    case LP_CONTAINER:
> +        return "CONTAINER";
> +    case LP_VIRTUAL:
> +        return "VIRTUAL";
> +    case LP_PATCH:
> +        return "PATCH";
> +    case LP_CHASSISREDIRECT:
> +        return "CHASSISREDIRECT";
> +    case LP_L3GATEWAY:
> +        return "L3GATEWAY";
> +    case LP_LOCALNET:
> +        return "LOCALNET";
> +    case LP_LOCALPORT:
> +        return "LOCALPORT";
> +    case LP_L2GATEWAY:
> +        return "L2GATEWAY";
> +    case LP_EXTERNAL:
> +        return "EXTERNAL";
> +    case LP_REMOTE:
> +        return "REMOTE";
> +    case LP_VTEP:
> +        return "VTEP";
> +    case LP_UNKNOWN:
> +        return "UNKNOWN";
> +    }
> +
> +    OVS_NOT_REACHED();
> +}
> +
> +bool
> +is_pb_router_type(const struct sbrec_port_binding *pb)
> +{
> +    enum en_lport_type lport_type = get_lport_type(pb);
> +
> +    switch (lport_type) {
> +    case LP_PATCH:
> +    case LP_CHASSISREDIRECT:
> +    case LP_L3GATEWAY:
> +    case LP_L2GATEWAY:
> +        return true;
> +
> +    case LP_VIF:
> +    case LP_CONTAINER:
> +    case LP_VIRTUAL:
> +    case LP_LOCALNET:
> +    case LP_LOCALPORT:
> +    case LP_REMOTE:
> +    case LP_VTEP:
> +    case LP_EXTERNAL:
> +    case LP_UNKNOWN:
> +        return false;
> +    }
> +
> +    return false;
> +}
> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> index 5ebdd8adb..b056a6c0e 100644
> --- a/lib/ovn-util.h
> +++ b/lib/ovn-util.h
> @@ -409,4 +409,25 @@ void flow_collector_ids_clear(struct
flow_collector_ids *);
>   * The returned pointer has to be freed by caller. */
>  char *encode_fqdn_string(const char *fqdn, size_t *len);
>
> +/* Corresponds to each Port_Binding.type. */
> +enum en_lport_type {
> +    LP_UNKNOWN,
> +    LP_VIF,
> +    LP_CONTAINER,
> +    LP_PATCH,
> +    LP_L3GATEWAY,
> +    LP_LOCALNET,
> +    LP_LOCALPORT,
> +    LP_L2GATEWAY,
> +    LP_VTEP,
> +    LP_CHASSISREDIRECT,
> +    LP_VIRTUAL,
> +    LP_EXTERNAL,
> +    LP_REMOTE
> +};
> +
> +enum en_lport_type get_lport_type(const struct sbrec_port_binding *);
> +char *get_lport_type_str(enum en_lport_type lport_type);
> +bool is_pb_router_type(const struct sbrec_port_binding *);
> +
>  #endif /* OVN_UTIL_H */
> diff --git a/northd/northd.c b/northd/northd.c
> index 9a12a94ae..49a2f8082 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -5271,6 +5271,13 @@ northd_handle_sb_port_binding_changes(
>          if (op && !op->lsp_can_be_inc_processed) {
>              return false;
>          }
> +
> +        if (!op) {
> +            if (is_pb_router_type(pb)) {
> +                return false;
> +            }
> +        }
> +

Can we check the type at the beginning of the loop, to avoid unnecessary
lookup for a lrp in the ls_ports?

Thanks,
Han

>          if (sbrec_port_binding_is_new(pb)) {
>              /* Most likely the PB was created by northd and this is the
>               * notification of that trasaction. So we just update the sb
> --
> 2.40.1
>
Numan Siddique Aug. 17, 2023, 7:01 a.m. UTC | #2 
On Thu, Aug 17, 2023 at 11:47 AM Han Zhou <hzhou@ovn.org> wrote:
>
> On Wed, Aug 16, 2023 at 1:27 AM <numans@ovn.org> wrote:
> >
> > From: Numan Siddique <numans@ovn.org>
> >
> > When changes to port bindings corresponding to router ports are
> > handled by northd engine node, incorrect warning logs (like below)
> > are logged.
> >
> > ----
> > northd|WARN|A port-binding for lrp0 is created but the LSP is not found.
> > ----
> >
> > Fix these warnings.
> >
> > Fixes: 3b120ccf7f7c ("northd: Incremental processing of SB port_binding
> in "northd" node.")
> >
> > CC: Han Zhou <hzhou@ovn.org>
> > Signed-off-by: Numan Siddique <numans@ovn.org>
> > ---
> >
> > v1 -> v2
> > -------
> >   * v1 was not returning false in northd_handle_sb_port_binding_changes()
> >     for router port - port bindings because of which IPv6 PD system test
> >     was failing.  Fixed it in v2.
> >
> >  controller/binding.c |  75 --------------------------------
> >  controller/binding.h |  20 +--------
> >  lib/ovn-util.c       | 101 +++++++++++++++++++++++++++++++++++++++++++
> >  lib/ovn-util.h       |  21 +++++++++
> >  northd/northd.c      |   7 +++
> >  5 files changed, 130 insertions(+), 94 deletions(-)
> >
> > diff --git a/controller/binding.c b/controller/binding.c
> > index 3ac0c35df..a521f2828 100644
> > --- a/controller/binding.c
> > +++ b/controller/binding.c
> > @@ -756,7 +756,6 @@ static void binding_lport_clear_port_sec(struct
> binding_lport *);
> >  static bool binding_lport_update_port_sec(
> >      struct binding_lport *, const struct sbrec_port_binding *);
> >
> > -static char *get_lport_type_str(enum en_lport_type lport_type);
> >  static bool ovs_iface_matches_lport_iface_id_ver(
> >      const struct ovsrec_interface *,
> >      const struct sbrec_port_binding *);
> > @@ -1055,80 +1054,6 @@ binding_dump_local_bindings(struct
> local_binding_data *lbinding_data,
> >      free(nodes);
> >  }
> >
> > -static bool
> > -is_lport_vif(const struct sbrec_port_binding *pb)
> > -{
> > -    return !pb->type[0];
> > -}
> > -
> > -enum en_lport_type
> > -get_lport_type(const struct sbrec_port_binding *pb)
> > -{
> > -    if (is_lport_vif(pb)) {
> > -        if (pb->parent_port && pb->parent_port[0]) {
> > -            return LP_CONTAINER;
> > -        }
> > -        return LP_VIF;
> > -    } else if (!strcmp(pb->type, "patch")) {
> > -        return LP_PATCH;
> > -    } else if (!strcmp(pb->type, "chassisredirect")) {
> > -        return LP_CHASSISREDIRECT;
> > -    } else if (!strcmp(pb->type, "l3gateway")) {
> > -        return LP_L3GATEWAY;
> > -    } else if (!strcmp(pb->type, "localnet")) {
> > -        return LP_LOCALNET;
> > -    } else if (!strcmp(pb->type, "localport")) {
> > -        return LP_LOCALPORT;
> > -    } else if (!strcmp(pb->type, "l2gateway")) {
> > -        return LP_L2GATEWAY;
> > -    } else if (!strcmp(pb->type, "virtual")) {
> > -        return LP_VIRTUAL;
> > -    } else if (!strcmp(pb->type, "external")) {
> > -        return LP_EXTERNAL;
> > -    } else if (!strcmp(pb->type, "remote")) {
> > -        return LP_REMOTE;
> > -    } else if (!strcmp(pb->type, "vtep")) {
> > -        return LP_VTEP;
> > -    }
> > -
> > -    return LP_UNKNOWN;
> > -}
> > -
> > -static char *
> > -get_lport_type_str(enum en_lport_type lport_type)
> > -{
> > -    switch (lport_type) {
> > -    case LP_VIF:
> > -        return "VIF";
> > -    case LP_CONTAINER:
> > -        return "CONTAINER";
> > -    case LP_VIRTUAL:
> > -        return "VIRTUAL";
> > -    case LP_PATCH:
> > -        return "PATCH";
> > -    case LP_CHASSISREDIRECT:
> > -        return "CHASSISREDIRECT";
> > -    case LP_L3GATEWAY:
> > -        return "L3GATEWAY";
> > -    case LP_LOCALNET:
> > -        return "PATCH";
> > -    case LP_LOCALPORT:
> > -        return "LOCALPORT";
> > -    case LP_L2GATEWAY:
> > -        return "L2GATEWAY";
> > -    case LP_EXTERNAL:
> > -        return "EXTERNAL";
> > -    case LP_REMOTE:
> > -        return "REMOTE";
> > -    case LP_VTEP:
> > -        return "VTEP";
> > -    case LP_UNKNOWN:
> > -        return "UNKNOWN";
> > -    }
> > -
> > -    OVS_NOT_REACHED();
> > -}
> > -
> >  void
> >  set_pb_chassis_in_sbrec(const struct sbrec_port_binding *pb,
> >                          const struct sbrec_chassis *chassis_rec,
> > diff --git a/controller/binding.h b/controller/binding.h
> > index 235e5860d..24bc84079 100644
> > --- a/controller/binding.h
> > +++ b/controller/binding.h
> > @@ -22,6 +22,7 @@
> >  #include "openvswitch/hmap.h"
> >  #include "openvswitch/uuid.h"
> >  #include "openvswitch/list.h"
> > +# include "lib/ovn-util.h"
> >  #include "sset.h"
> >  #include "lport.h"
> >
> > @@ -217,25 +218,6 @@ void port_binding_set_down(const struct
> sbrec_chassis *chassis_rec,
> >                             const char *iface_id,
> >                             const struct uuid *pb_uuid);
> >
> > -/* Corresponds to each Port_Binding.type. */
> > -enum en_lport_type {
> > -    LP_UNKNOWN,
> > -    LP_VIF,
> > -    LP_CONTAINER,
> > -    LP_PATCH,
> > -    LP_L3GATEWAY,
> > -    LP_LOCALNET,
> > -    LP_LOCALPORT,
> > -    LP_L2GATEWAY,
> > -    LP_VTEP,
> > -    LP_CHASSISREDIRECT,
> > -    LP_VIRTUAL,
> > -    LP_EXTERNAL,
> > -    LP_REMOTE
> > -};
> > -
> > -enum en_lport_type get_lport_type(const struct sbrec_port_binding *);
> > -
> >  /* This structure represents a logical port (or port binding)
> >   * which is associated with 'struct local_binding'.
> >   *
> > diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> > index 080ad4c0c..3a237b7fe 100644
> > --- a/lib/ovn-util.c
> > +++ b/lib/ovn-util.c
> > @@ -1168,3 +1168,104 @@ encode_fqdn_string(const char *fqdn, size_t *len)
> >
> >      return encoded;
> >  }
> > +
> > +static bool
> > +is_lport_vif(const struct sbrec_port_binding *pb)
> > +{
> > +    return !pb->type[0];
> > +}
> > +
> > +enum en_lport_type
> > +get_lport_type(const struct sbrec_port_binding *pb)
> > +{
> > +    if (is_lport_vif(pb)) {
> > +        if (pb->parent_port && pb->parent_port[0]) {
> > +            return LP_CONTAINER;
> > +        }
> > +        return LP_VIF;
> > +    } else if (!strcmp(pb->type, "patch")) {
> > +        return LP_PATCH;
> > +    } else if (!strcmp(pb->type, "chassisredirect")) {
> > +        return LP_CHASSISREDIRECT;
> > +    } else if (!strcmp(pb->type, "l3gateway")) {
> > +        return LP_L3GATEWAY;
> > +    } else if (!strcmp(pb->type, "localnet")) {
> > +        return LP_LOCALNET;
> > +    } else if (!strcmp(pb->type, "localport")) {
> > +        return LP_LOCALPORT;
> > +    } else if (!strcmp(pb->type, "l2gateway")) {
> > +        return LP_L2GATEWAY;
> > +    } else if (!strcmp(pb->type, "virtual")) {
> > +        return LP_VIRTUAL;
> > +    } else if (!strcmp(pb->type, "external")) {
> > +        return LP_EXTERNAL;
> > +    } else if (!strcmp(pb->type, "remote")) {
> > +        return LP_REMOTE;
> > +    } else if (!strcmp(pb->type, "vtep")) {
> > +        return LP_VTEP;
> > +    }
> > +
> > +    return LP_UNKNOWN;
> > +}
> > +
> > +char *
> > +get_lport_type_str(enum en_lport_type lport_type)
> > +{
> > +    switch (lport_type) {
> > +    case LP_VIF:
> > +        return "VIF";
> > +    case LP_CONTAINER:
> > +        return "CONTAINER";
> > +    case LP_VIRTUAL:
> > +        return "VIRTUAL";
> > +    case LP_PATCH:
> > +        return "PATCH";
> > +    case LP_CHASSISREDIRECT:
> > +        return "CHASSISREDIRECT";
> > +    case LP_L3GATEWAY:
> > +        return "L3GATEWAY";
> > +    case LP_LOCALNET:
> > +        return "LOCALNET";
> > +    case LP_LOCALPORT:
> > +        return "LOCALPORT";
> > +    case LP_L2GATEWAY:
> > +        return "L2GATEWAY";
> > +    case LP_EXTERNAL:
> > +        return "EXTERNAL";
> > +    case LP_REMOTE:
> > +        return "REMOTE";
> > +    case LP_VTEP:
> > +        return "VTEP";
> > +    case LP_UNKNOWN:
> > +        return "UNKNOWN";
> > +    }
> > +
> > +    OVS_NOT_REACHED();
> > +}
> > +
> > +bool
> > +is_pb_router_type(const struct sbrec_port_binding *pb)
> > +{
> > +    enum en_lport_type lport_type = get_lport_type(pb);
> > +
> > +    switch (lport_type) {
> > +    case LP_PATCH:
> > +    case LP_CHASSISREDIRECT:
> > +    case LP_L3GATEWAY:
> > +    case LP_L2GATEWAY:
> > +        return true;
> > +
> > +    case LP_VIF:
> > +    case LP_CONTAINER:
> > +    case LP_VIRTUAL:
> > +    case LP_LOCALNET:
> > +    case LP_LOCALPORT:
> > +    case LP_REMOTE:
> > +    case LP_VTEP:
> > +    case LP_EXTERNAL:
> > +    case LP_UNKNOWN:
> > +        return false;
> > +    }
> > +
> > +    return false;
> > +}
> > diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> > index 5ebdd8adb..b056a6c0e 100644
> > --- a/lib/ovn-util.h
> > +++ b/lib/ovn-util.h
> > @@ -409,4 +409,25 @@ void flow_collector_ids_clear(struct
> flow_collector_ids *);
> >   * The returned pointer has to be freed by caller. */
> >  char *encode_fqdn_string(const char *fqdn, size_t *len);
> >
> > +/* Corresponds to each Port_Binding.type. */
> > +enum en_lport_type {
> > +    LP_UNKNOWN,
> > +    LP_VIF,
> > +    LP_CONTAINER,
> > +    LP_PATCH,
> > +    LP_L3GATEWAY,
> > +    LP_LOCALNET,
> > +    LP_LOCALPORT,
> > +    LP_L2GATEWAY,
> > +    LP_VTEP,
> > +    LP_CHASSISREDIRECT,
> > +    LP_VIRTUAL,
> > +    LP_EXTERNAL,
> > +    LP_REMOTE
> > +};
> > +
> > +enum en_lport_type get_lport_type(const struct sbrec_port_binding *);
> > +char *get_lport_type_str(enum en_lport_type lport_type);
> > +bool is_pb_router_type(const struct sbrec_port_binding *);
> > +
> >  #endif /* OVN_UTIL_H */
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 9a12a94ae..49a2f8082 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -5271,6 +5271,13 @@ northd_handle_sb_port_binding_changes(
> >          if (op && !op->lsp_can_be_inc_processed) {
> >              return false;
> >          }
> > +
> > +        if (!op) {
> > +            if (is_pb_router_type(pb)) {
> > +                return false;
> > +            }
> > +        }
> > +
>
> Can we check the type at the beginning of the loop, to avoid unnecessary
> lookup for a lrp in the ls_ports?

That's a good point.  I'll update v3.

Numan

>
> Thanks,
> Han
>
> >          if (sbrec_port_binding_is_new(pb)) {
> >              /* Most likely the PB was created by northd and this is the
> >               * notification of that trasaction. So we just update the sb
> > --
> > 2.40.1
> >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff mbox series

Patch

diff --git a/controller/binding.c b/controller/binding.c
index 3ac0c35df..a521f2828 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -756,7 +756,6 @@  static void binding_lport_clear_port_sec(struct binding_lport *);
 static bool binding_lport_update_port_sec(
     struct binding_lport *, const struct sbrec_port_binding *);
 
-static char *get_lport_type_str(enum en_lport_type lport_type);
 static bool ovs_iface_matches_lport_iface_id_ver(
     const struct ovsrec_interface *,
     const struct sbrec_port_binding *);
@@ -1055,80 +1054,6 @@  binding_dump_local_bindings(struct local_binding_data *lbinding_data,
     free(nodes);
 }
 
-static bool
-is_lport_vif(const struct sbrec_port_binding *pb)
-{
-    return !pb->type[0];
-}
-
-enum en_lport_type
-get_lport_type(const struct sbrec_port_binding *pb)
-{
-    if (is_lport_vif(pb)) {
-        if (pb->parent_port && pb->parent_port[0]) {
-            return LP_CONTAINER;
-        }
-        return LP_VIF;
-    } else if (!strcmp(pb->type, "patch")) {
-        return LP_PATCH;
-    } else if (!strcmp(pb->type, "chassisredirect")) {
-        return LP_CHASSISREDIRECT;
-    } else if (!strcmp(pb->type, "l3gateway")) {
-        return LP_L3GATEWAY;
-    } else if (!strcmp(pb->type, "localnet")) {
-        return LP_LOCALNET;
-    } else if (!strcmp(pb->type, "localport")) {
-        return LP_LOCALPORT;
-    } else if (!strcmp(pb->type, "l2gateway")) {
-        return LP_L2GATEWAY;
-    } else if (!strcmp(pb->type, "virtual")) {
-        return LP_VIRTUAL;
-    } else if (!strcmp(pb->type, "external")) {
-        return LP_EXTERNAL;
-    } else if (!strcmp(pb->type, "remote")) {
-        return LP_REMOTE;
-    } else if (!strcmp(pb->type, "vtep")) {
-        return LP_VTEP;
-    }
-
-    return LP_UNKNOWN;
-}
-
-static char *
-get_lport_type_str(enum en_lport_type lport_type)
-{
-    switch (lport_type) {
-    case LP_VIF:
-        return "VIF";
-    case LP_CONTAINER:
-        return "CONTAINER";
-    case LP_VIRTUAL:
-        return "VIRTUAL";
-    case LP_PATCH:
-        return "PATCH";
-    case LP_CHASSISREDIRECT:
-        return "CHASSISREDIRECT";
-    case LP_L3GATEWAY:
-        return "L3GATEWAY";
-    case LP_LOCALNET:
-        return "PATCH";
-    case LP_LOCALPORT:
-        return "LOCALPORT";
-    case LP_L2GATEWAY:
-        return "L2GATEWAY";
-    case LP_EXTERNAL:
-        return "EXTERNAL";
-    case LP_REMOTE:
-        return "REMOTE";
-    case LP_VTEP:
-        return "VTEP";
-    case LP_UNKNOWN:
-        return "UNKNOWN";
-    }
-
-    OVS_NOT_REACHED();
-}
-
 void
 set_pb_chassis_in_sbrec(const struct sbrec_port_binding *pb,
                         const struct sbrec_chassis *chassis_rec,
diff --git a/controller/binding.h b/controller/binding.h
index 235e5860d..24bc84079 100644
--- a/controller/binding.h
+++ b/controller/binding.h
@@ -22,6 +22,7 @@ 
 #include "openvswitch/hmap.h"
 #include "openvswitch/uuid.h"
 #include "openvswitch/list.h"
+# include "lib/ovn-util.h"
 #include "sset.h"
 #include "lport.h"
 
@@ -217,25 +218,6 @@  void port_binding_set_down(const struct sbrec_chassis *chassis_rec,
                            const char *iface_id,
                            const struct uuid *pb_uuid);
 
-/* Corresponds to each Port_Binding.type. */
-enum en_lport_type {
-    LP_UNKNOWN,
-    LP_VIF,
-    LP_CONTAINER,
-    LP_PATCH,
-    LP_L3GATEWAY,
-    LP_LOCALNET,
-    LP_LOCALPORT,
-    LP_L2GATEWAY,
-    LP_VTEP,
-    LP_CHASSISREDIRECT,
-    LP_VIRTUAL,
-    LP_EXTERNAL,
-    LP_REMOTE
-};
-
-enum en_lport_type get_lport_type(const struct sbrec_port_binding *);
-
 /* This structure represents a logical port (or port binding)
  * which is associated with 'struct local_binding'.
  *
diff --git a/lib/ovn-util.c b/lib/ovn-util.c
index 080ad4c0c..3a237b7fe 100644
--- a/lib/ovn-util.c
+++ b/lib/ovn-util.c
@@ -1168,3 +1168,104 @@  encode_fqdn_string(const char *fqdn, size_t *len)
 
     return encoded;
 }
+
+static bool
+is_lport_vif(const struct sbrec_port_binding *pb)
+{
+    return !pb->type[0];
+}
+
+enum en_lport_type
+get_lport_type(const struct sbrec_port_binding *pb)
+{
+    if (is_lport_vif(pb)) {
+        if (pb->parent_port && pb->parent_port[0]) {
+            return LP_CONTAINER;
+        }
+        return LP_VIF;
+    } else if (!strcmp(pb->type, "patch")) {
+        return LP_PATCH;
+    } else if (!strcmp(pb->type, "chassisredirect")) {
+        return LP_CHASSISREDIRECT;
+    } else if (!strcmp(pb->type, "l3gateway")) {
+        return LP_L3GATEWAY;
+    } else if (!strcmp(pb->type, "localnet")) {
+        return LP_LOCALNET;
+    } else if (!strcmp(pb->type, "localport")) {
+        return LP_LOCALPORT;
+    } else if (!strcmp(pb->type, "l2gateway")) {
+        return LP_L2GATEWAY;
+    } else if (!strcmp(pb->type, "virtual")) {
+        return LP_VIRTUAL;
+    } else if (!strcmp(pb->type, "external")) {
+        return LP_EXTERNAL;
+    } else if (!strcmp(pb->type, "remote")) {
+        return LP_REMOTE;
+    } else if (!strcmp(pb->type, "vtep")) {
+        return LP_VTEP;
+    }
+
+    return LP_UNKNOWN;
+}
+
+char *
+get_lport_type_str(enum en_lport_type lport_type)
+{
+    switch (lport_type) {
+    case LP_VIF:
+        return "VIF";
+    case LP_CONTAINER:
+        return "CONTAINER";
+    case LP_VIRTUAL:
+        return "VIRTUAL";
+    case LP_PATCH:
+        return "PATCH";
+    case LP_CHASSISREDIRECT:
+        return "CHASSISREDIRECT";
+    case LP_L3GATEWAY:
+        return "L3GATEWAY";
+    case LP_LOCALNET:
+        return "LOCALNET";
+    case LP_LOCALPORT:
+        return "LOCALPORT";
+    case LP_L2GATEWAY:
+        return "L2GATEWAY";
+    case LP_EXTERNAL:
+        return "EXTERNAL";
+    case LP_REMOTE:
+        return "REMOTE";
+    case LP_VTEP:
+        return "VTEP";
+    case LP_UNKNOWN:
+        return "UNKNOWN";
+    }
+
+    OVS_NOT_REACHED();
+}
+
+bool
+is_pb_router_type(const struct sbrec_port_binding *pb)
+{
+    enum en_lport_type lport_type = get_lport_type(pb);
+
+    switch (lport_type) {
+    case LP_PATCH:
+    case LP_CHASSISREDIRECT:
+    case LP_L3GATEWAY:
+    case LP_L2GATEWAY:
+        return true;
+
+    case LP_VIF:
+    case LP_CONTAINER:
+    case LP_VIRTUAL:
+    case LP_LOCALNET:
+    case LP_LOCALPORT:
+    case LP_REMOTE:
+    case LP_VTEP:
+    case LP_EXTERNAL:
+    case LP_UNKNOWN:
+        return false;
+    }
+
+    return false;
+}
diff --git a/lib/ovn-util.h b/lib/ovn-util.h
index 5ebdd8adb..b056a6c0e 100644
--- a/lib/ovn-util.h
+++ b/lib/ovn-util.h
@@ -409,4 +409,25 @@  void flow_collector_ids_clear(struct flow_collector_ids *);
  * The returned pointer has to be freed by caller. */
 char *encode_fqdn_string(const char *fqdn, size_t *len);
 
+/* Corresponds to each Port_Binding.type. */
+enum en_lport_type {
+    LP_UNKNOWN,
+    LP_VIF,
+    LP_CONTAINER,
+    LP_PATCH,
+    LP_L3GATEWAY,
+    LP_LOCALNET,
+    LP_LOCALPORT,
+    LP_L2GATEWAY,
+    LP_VTEP,
+    LP_CHASSISREDIRECT,
+    LP_VIRTUAL,
+    LP_EXTERNAL,
+    LP_REMOTE
+};
+
+enum en_lport_type get_lport_type(const struct sbrec_port_binding *);
+char *get_lport_type_str(enum en_lport_type lport_type);
+bool is_pb_router_type(const struct sbrec_port_binding *);
+
 #endif /* OVN_UTIL_H */
diff --git a/northd/northd.c b/northd/northd.c
index 9a12a94ae..49a2f8082 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -5271,6 +5271,13 @@  northd_handle_sb_port_binding_changes(
         if (op && !op->lsp_can_be_inc_processed) {
             return false;
         }
+
+        if (!op) {
+            if (is_pb_router_type(pb)) {
+                return false;
+            }
+        }
+
         if (sbrec_port_binding_is_new(pb)) {
             /* Most likely the PB was created by northd and this is the
              * notification of that trasaction. So we just update the sb