On Thu, Aug 17, 2023 at 11:47 AM Han Zhou <hz...@ovn.org> wrote:
>
> On Wed, Aug 16, 2023 at 1:27 AM <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.
> >
> > 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>
> > ---
> >
> > 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
> 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
