| Message ID | 20230814090738.58016-1-numans@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] northd: Fix incorrect warning logs when handling port binding changes. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On 8/14/23 11:07, 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. > > 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 <hzhou@ovn.org> > Signed-off-by: Numan Siddique <numans@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 *,
On Tue, 15 Aug, 2023, 2:09 pm Dumitru Ceara, <dceara@redhat.com> wrote: > On 8/14/23 11:07, 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. > > > > 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 <hzhou@ovn.org> > > Signed-off-by: Numan Siddique <numans@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. > Hi Dumitru, Thanks. I thought I fixed that issue. I'll update v2 fixing that. Numan 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Mon, Aug 14, 2023 at 2:07 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. > > 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 <hzhou@ovn.org> > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > 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); Thanks Numan for the fix! Since we don't handle router ports incrementally anyway, this check can be put at the beginning of the loop and if it is router port, we just fallback to recompute (return false). Would that be a simpler fix? Regards, Han > + 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 *, > -- > 2.40.1 >
On Wed, Aug 16, 2023 at 10:52 AM Han Zhou <hzhou@ovn.org> wrote: > > On Mon, Aug 14, 2023 at 2:07 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. > > > > 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 <hzhou@ovn.org> > > Signed-off-by: Numan Siddique <numans@ovn.org> > > --- > > 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); > > Thanks Numan for the fix! Since we don't handle router ports incrementally > anyway, this check can be put at the beginning of the loop and if it is > router port, we just fallback to recompute (return false). Would that be a > simpler fix? Yes. I just submitted v2 doing the same. Request to take a look. Numan > > Regards, > Han > > > + 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 *, > > -- > > 2.40.1 > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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 *,