| Message ID | 20210928155325.2290444-4-frode.nordahl@canonical.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Introduce infrastructure for plug providers | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On Tue, Sep 28, 2021 at 11:54 AM Frode Nordahl <frode.nordahl@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@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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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. */
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@canonical.com> --- controller/binding.c | 36 +++++++++++++++++++----------------- controller/ovn-controller.c | 3 +++ controller/physical.c | 12 +++++++----- 3 files changed, 29 insertions(+), 22 deletions(-)