On 8/14/23 11:07, num...@ovn.org wrote:
> From: Numan Siddique <num...@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.
>
> Since we are preserving the "lr_ports" data in the northd engine
> across engine runs, it is important to store the port binding
> address in 'op->sb' after the transaction is committed. And this
> patch does that.
>
> Fixes: 3b120ccf7f7c ("northd: Incremental processing of SB port_binding in
> "northd" node.")
>
> CC: Han Zhou <hz...@ovn.org>
> Signed-off-by: Numan Siddique <num...@ovn.org>
> ---
Hi Numan,
This change seems to break IPv6 prefix delegation (the ovsrobot CI run
failed when running system tests):
https://github.com/ovsrobot/ovn/actions/runs/5854010538
I guess we somehow miss updating the LSP when the port binding update is
handled by I-P handlers but I didn't check closely.
Regards,
Dumitru
> controller/binding.c | 75 --------------------------------
> controller/binding.h | 20 +--------
> lib/ovn-util.c | 101 +++++++++++++++++++++++++++++++++++++++++++
> lib/ovn-util.h | 21 +++++++++
> northd/en-northd.c | 2 +-
> northd/northd.c | 19 ++++++--
> northd/northd.h | 3 +-
> 7 files changed, 141 insertions(+), 100 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 3ac0c35df3..a521f2828e 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 235e5860d0..24bc840791 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 080ad4c0ce..3a237b7fe3 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 5ebdd8adb0..b056a6c0ed 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/en-northd.c b/northd/en-northd.c
> index 044fa70190..b931f812ab 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -198,7 +198,7 @@ northd_sb_port_binding_handler(struct engine_node *node,
> northd_get_input_data(node, &input_data);
>
> if (!northd_handle_sb_port_binding_changes(
> - input_data.sbrec_port_binding_table, &nd->ls_ports)) {
> + input_data.sbrec_port_binding_table, &nd->ls_ports, &nd->lr_ports)) {
> return false;
> }
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 9a12a94ae2..d355cf7c68 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -5262,22 +5262,32 @@ fail:
> bool
> northd_handle_sb_port_binding_changes(
> const struct sbrec_port_binding_table *sbrec_port_binding_table,
> - struct hmap *ls_ports)
> + struct hmap *ls_ports, struct hmap *lr_ports)
> {
> const struct sbrec_port_binding *pb;
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, sbrec_port_binding_table)
> {
> struct ovn_port *op = ovn_port_find(ls_ports, pb->logical_port);
> + bool is_router_port = false;
> if (op && !op->lsp_can_be_inc_processed) {
> return false;
> }
> +
> + if (!op) {
> + is_router_port = is_pb_router_type(pb);
> + if (is_router_port) {
> + op = ovn_port_find(lr_ports, pb->logical_port);
> + }
> + }
> +
> 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
> * pointer in northd data. Fallback to recompute otherwise. */
> if (!op) {
> VLOG_WARN_RL(&rl, "A port-binding for %s is created but the "
> - "LSP is not found.", pb->logical_port);
> + "%s is not found.", pb->logical_port,
> + is_router_port ? "LRP" : "LSP");
> return false;
> }
> op->sb = pb;
> @@ -5288,7 +5298,7 @@ northd_handle_sb_port_binding_changes(
> * sb idl pointers and other unexpected behavior. */
> if (op) {
> VLOG_WARN_RL(&rl, "A port-binding for %s is deleted but the "
> - "LSP still exists.", pb->logical_port);
> + "LSP/LRP still exists.", pb->logical_port);
> return false;
> }
> } else {
> @@ -5298,7 +5308,8 @@ northd_handle_sb_port_binding_changes(
> * Fallback to recompute for anything unexpected. */
> if (!op) {
> VLOG_WARN_RL(&rl, "A port-binding for %s is updated but the "
> - "LSP is not found.", pb->logical_port);
> + "%s is not found.", pb->logical_port,
> + is_router_port ? "LRP" : "LSP");
> return false;
> }
> if (op->sb != pb) {
> diff --git a/northd/northd.h b/northd/northd.h
> index f3e63b1e1a..5759a4ee0e 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -342,7 +342,8 @@ bool lflow_handle_northd_ls_changes(struct ovsdb_idl_txn
> *ovnsb_txn,
> struct tracked_ls_changes *,
> struct lflow_input *, struct hmap
> *lflows);
> bool northd_handle_sb_port_binding_changes(
> - const struct sbrec_port_binding_table *, struct hmap *ls_ports);
> + const struct sbrec_port_binding_table *, struct hmap *ls_ports,
> + struct hmap *lr_ports);
>
> void build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
> const struct nbrec_bfd_table *,
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
