On Tue, 15 Aug, 2023, 2:09 pm Dumitru Ceara, <dce...@redhat.com> wrote:
> 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.
>
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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
