[ovs-dev] [PATCH ovn v4 3/9] controller: Make use of Port_Binding:requested_chassis
Numan Siddique
numans at ovn.org
Thu Sep 16 16:12:01 UTC 2021
On Fri, Sep 3, 2021 at 3:28 PM 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 option processing and string comparison.
>
> Signed-off-by: Frode Nordahl <frode.nordahl at canonical.com>
I've one comment which seems to be discussed in v3 of the first patch.
Looks to me this patch would break the existing behavior (which the
patch 2 addresses).
Right now when CMS sets requested-chassis=foo for a logical port 'P1',
then other ovn-controllers
will not claim 'P1' if there is a VIF for this even if chassis 'foo'
doesn't exist.
With this patch series, other ovn-controller will claim 'P1' for the
above scenario I mentioned.
Is this going to be a problem ? Or are we going to break some existing
deployments ?
I'd suggest not breaking this scenario. If the
port_binding->options:requested-chassis is set,
then ovn-controller should not claim that port binding if the
port_binding->requested_chassis is
set. In other words ovn-controller should claim only if
port_binding->requested_chassis == this_chassis
when the port_binding->options:requested-chassis is set.
Does this make sense ?
Numan
> ---
> controller/binding.c | 29 ++++++++++++-----------------
> controller/ovn-controller.c | 3 +++
> controller/physical.c | 7 ++-----
> 3 files changed, 17 insertions(+), 22 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 34935bb9c..938e1d81d 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -1051,11 +1051,10 @@ 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);
> + return !pb->requested_chassis
> + || chassis_rec == pb->requested_chassis;
> }
>
> /* Returns 'true' if the 'lbinding' has binding lports of type LP_CONTAINER,
> @@ -1093,7 +1092,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,
> @@ -1134,7 +1133,8 @@ 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 : "(none)");
> }
> }
>
> @@ -1157,9 +1157,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,
> @@ -1189,8 +1187,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
> @@ -1274,12 +1272,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);
> }
>
> @@ -1328,7 +1323,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 0031a1035..7387a177b 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 6f2c1cea0..2f6bd0e91 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -1066,11 +1066,8 @@ 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)) {
> + if (ofport && binding->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