[ovs-dev] [PATCH ovn v6 03/12] controller: Make use of Port_Binding:requested_chassis

Numan Siddique numans at ovn.org
Tue Oct 5 02:37:09 UTC 2021


On Tue, Sep 28, 2021 at 11:54 AM Frode Nordahl
<frode.nordahl at canonical.com> wrote:
>
> Improve the efficiency of the requested-chassis feature by using
> the new Southbound Port_Binding:requested_chassis column instead
> of each chassis performing string comparison for every
> Port_Binding record processed.
>
> Signed-off-by: Frode Nordahl <frode.nordahl at canonical.com>

Hi Frode,

Thanks for adding this feature and for the patience as it is taking a
little longer to review these patches.

Overall the patch series LGTM.  There is one problem though with this
patch which needs to be addressed.

Once we apply this patch 3,  existing deployments will break if
ovn-controllers are first updated/upgraded.

Since the Southbound DB is not upgraded yet,  you will see the below
warning in the ovn-controller log
and ovn-controller will release existing claimed logical ports if
requesed-chassis option is set.

----
2021-10-05T01:45:09.146Z|00026|ovsdb_idl|WARN|Port_Binding table in
OVN_Southbound database lacks requested_chassis column (database needs
upgrade?)
...
...
2021-10-05T01:46:41.011Z|00033|binding|INFO|Not claiming lport
sw0-port1, chassis ovn-chassis-1 requested-chassis (option points at
non-existent chassis)
--------

This needs to be handled properly.   My suggestion would be to make
use of the IDL generated function -
sbrec_server_has_port_binding_table_col_requested_chassis()
and decide to use the new approach or old approach in the function
can_bind_on_this_chassis().  What do you think ?

Thanks
Numan



> ---
>  controller/binding.c        | 36 +++++++++++++++++++-----------------
>  controller/ovn-controller.c |  3 +++
>  controller/physical.c       | 12 +++++++-----
>  3 files changed, 29 insertions(+), 22 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index c037b2352..a2eb86c89 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -1056,11 +1056,15 @@ is_binding_lport_this_chassis(struct binding_lport *b_lport,
>
>  static bool
>  can_bind_on_this_chassis(const struct sbrec_chassis *chassis_rec,
> -                         const char *requested_chassis)
> +                         const struct sbrec_port_binding *pb)
>  {
> -    return !requested_chassis || !requested_chassis[0]
> -           || !strcmp(requested_chassis, chassis_rec->name)
> -           || !strcmp(requested_chassis, chassis_rec->hostname);
> +    /* We need to check for presence of the requested-chassis option in
> +     * addittion to checking the pb->requested_chassis column because this
> +     * column will be set to NULL whenever the option points to a non-existent
> +     * chassis.  As the controller routinely clears its own chassis record this
> +     * might occur more often than one might think. */
> +    return !smap_get(&pb->options, "requested-chassis")
> +           || chassis_rec == pb->requested_chassis;
>  }
>
>  /* Returns 'true' if the 'lbinding' has binding lports of type LP_CONTAINER,
> @@ -1098,7 +1102,7 @@ release_binding_lport(const struct sbrec_chassis *chassis_rec,
>
>  static bool
>  consider_vif_lport_(const struct sbrec_port_binding *pb,
> -                    bool can_bind, const char *vif_chassis,
> +                    bool can_bind,
>                      struct binding_ctx_in *b_ctx_in,
>                      struct binding_ctx_out *b_ctx_out,
>                      struct binding_lport *b_lport,
> @@ -1139,7 +1143,10 @@ consider_vif_lport_(const struct sbrec_port_binding *pb,
>                               "requested-chassis %s",
>                               pb->logical_port,
>                               b_ctx_in->chassis_rec->name,
> -                             vif_chassis);
> +                             pb->requested_chassis ?
> +                             pb->requested_chassis->name : "(option points at "
> +                                                           "non-existent "
> +                                                           "chassis)");
>          }
>      }
>
> @@ -1162,9 +1169,7 @@ consider_vif_lport(const struct sbrec_port_binding *pb,
>                     struct local_binding *lbinding,
>                     struct hmap *qos_map)
>  {
> -    const char *vif_chassis = smap_get(&pb->options, "requested-chassis");
> -    bool can_bind = can_bind_on_this_chassis(b_ctx_in->chassis_rec,
> -                                             vif_chassis);
> +    bool can_bind = can_bind_on_this_chassis(b_ctx_in->chassis_rec, pb);
>
>      if (!lbinding) {
>          lbinding = local_binding_find(&b_ctx_out->lbinding_data->bindings,
> @@ -1194,8 +1199,8 @@ consider_vif_lport(const struct sbrec_port_binding *pb,
>          }
>      }
>
> -    return consider_vif_lport_(pb, can_bind, vif_chassis, b_ctx_in,
> -                               b_ctx_out, b_lport, qos_map);
> +    return consider_vif_lport_(pb, can_bind, b_ctx_in, b_ctx_out,
> +                               b_lport, qos_map);
>  }
>
>  static bool
> @@ -1279,12 +1284,9 @@ consider_container_lport(const struct sbrec_port_binding *pb,
>      }
>
>      ovs_assert(parent_b_lport && parent_b_lport->pb);
> -    const char *vif_chassis = smap_get(&parent_b_lport->pb->options,
> -                                       "requested-chassis");
> -    bool can_bind = can_bind_on_this_chassis(b_ctx_in->chassis_rec,
> -                                             vif_chassis);
> +    bool can_bind = can_bind_on_this_chassis(b_ctx_in->chassis_rec, pb);
>
> -    return consider_vif_lport_(pb, can_bind, vif_chassis, b_ctx_in, b_ctx_out,
> +    return consider_vif_lport_(pb, can_bind, b_ctx_in, b_ctx_out,
>                                 container_b_lport, qos_map);
>  }
>
> @@ -1333,7 +1335,7 @@ consider_virtual_lport(const struct sbrec_port_binding *pb,
>          }
>      }
>
> -    if (!consider_vif_lport_(pb, true, NULL, b_ctx_in, b_ctx_out,
> +    if (!consider_vif_lport_(pb, true, b_ctx_in, b_ctx_out,
>                               virtual_b_lport, qos_map)) {
>          return false;
>      }
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index a719beb0e..b0e4174aa 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -232,6 +232,9 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
>          sbrec_port_binding_add_clause_chassis(&pb, OVSDB_F_EQ,
>                                                &chassis->header_.uuid);
>
> +        sbrec_port_binding_add_clause_requested_chassis(
> +            &pb, OVSDB_F_EQ, &chassis->header_.uuid);
> +
>          /* Ensure that we find out about l2gateway and l3gateway ports that
>           * should be present on this chassis.  Otherwise, we might never find
>           * out about those ports, if their datapaths don't otherwise have a VIF
> diff --git a/controller/physical.c b/controller/physical.c
> index 0cfb158c8..780093638 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -1066,11 +1066,13 @@ consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
>      } else {
>          ofport = local_binding_get_lport_ofport(local_bindings,
>                                                  binding->logical_port);
> -        const char *requested_chassis = smap_get(&binding->options,
> -                                                 "requested-chassis");
> -        if (ofport && requested_chassis && requested_chassis[0] &&
> -            strcmp(requested_chassis, chassis->name) &&
> -            strcmp(requested_chassis, chassis->hostname)) {
> +        /* We need to check for presence of the requested-chassis option in
> +         * addittion to checking the pb->requested_chassis column because this
> +         * column will be set to NULL whenever the option points to a
> +         * non-existent chassis.  As the controller routinely clears its own
> +         * chassis record this might occur more often than one might think. */
> +        if (ofport && smap_get(&binding->options, "requested-chassis")
> +            && binding->requested_chassis != chassis) {
>              /* Even though there is an ofport for this port_binding, it is
>               * requested on a different chassis. So ignore this ofport.
>               */
> --
> 2.32.0
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list