On Wed, Aug 16, 2023 at 1:27 AM <[email protected]> wrote:
>
> From: Numan Siddique <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Numan Siddique <[email protected]>
> ---
>
> 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
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
