On Tue, Jun 6, 2023 at 7:57 AM Dumitru Ceara <[email protected]> wrote: > > We don't need to explicitly add port bindings that were already bound > locally. We implicitly get those because we monitor the datapaths > they're attached to. > > When performing an ovn-heater 500-node density-heavy scale test [0], with > conditional monitoring enabled, the unreasonably long poll intervals on > the Southbound database (the ones that took more than one second) are > measured as: > > --------------------------------------------------------------------- > Count Min Max Median Mean 95 percentile > --------------------------------------------------------------------- > 56.0 1010.0 2664.0 1455.5 1544.9 2163.0 > 77.0 1015.0 3460.0 1896.0 1858.2 2907.8 > 69.0 1010.0 3118.0 1618.0 1688.1 2582.4 > --------------------------------------------------------------------- > 202.0 1010.0 3460.0 1610.0 1713.3 2711.5 > > Compared to the baseline results (also with conditional monitoring > enabled): > --------------------------------------------------------------------- > Count Min Max Median Mean 95 percentile > --------------------------------------------------------------------- > 141.0 1003.0 18338.0 1721.0 2625.4 7626.0 > 151.0 1019.0 80297.0 1808.0 3410.7 9089.0 > 165.0 1007.0 50736.0 2354.0 3067.7 7309.8 > --------------------------------------------------------------------- > 457.0 1003.0 80297.0 2131.0 3044.6 7799.6 > > We see significant improvement on the server side. This is explained > by the fact that now the Southbound database server doesn't need to > apply as many conditions as before when filtering individual monitor > contents. > Thanks Dumitru for the great improvement! This is very helpful for the high port-density environment. Just to make sure I understand the test result correctly, in [0], it shows 22500 pods and 500 nodes, so is it 45 pods per node?
> Note: Sub-ports - OVN switch ports with parent_port set - have to be > monitored unconditionally as we cannot efficiently determine their local > datapaths without including all local OVS interfaces in the monitor. > This, however, should not be a huge issue because the majority of ports > are regular VIFs, not sub-ports. I am not sure if we can make such a conclusion. For the current ovn-k8s or environments similar to that, it shouldn't be a problem. However, for environments that model pods/containers as sub-ports of the VM VIFs, probably most of the majority of the ports would be sub-ports. This is what sub-ports are designed for, right? So, I think this would be a significant change of data monitored for those environments. I'd suggest at least we should properly document the implication in the documents (such as ovn-monitor-all, and also the sub-port related parts). There may also be such users who prefer not monitoring all sub-ports (for efficiency of ovn-controller) sacrificing SB DB performance (probably they don't have very high port density so the conditional monitoring impact is not that big). I am not aware of any such users yet, but if they complain, we will have to provide a knob, if no better ideas. Other than this, the patch looks good to me. Acked-by: Han Zhou <[email protected]> > > [0] https://github.com/ovn-org/ovn-heater/blob/main/test-scenarios/ocp-500-density-heavy.yml > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2139194 > Reported-by: Ilya Maximets <[email protected]> > Signed-off-by: Dumitru Ceara <[email protected]> > --- > controller/binding.c | 2 +- > controller/binding.h | 2 +- > controller/ovn-controller.c | 19 ++++++++++++++++--- > 3 files changed, 18 insertions(+), 5 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index 9b0647b70e..ad03332915 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -761,7 +761,7 @@ local_binding_data_destroy(struct local_binding_data *lbinding_data) > } > > const struct sbrec_port_binding * > -local_binding_get_primary_pb(struct shash *local_bindings, > +local_binding_get_primary_pb(const struct shash *local_bindings, > const char *pb_name) > { > struct local_binding *lbinding = > diff --git a/controller/binding.h b/controller/binding.h > index 0e57f02ee5..319cbd263c 100644 > --- a/controller/binding.h > +++ b/controller/binding.h > @@ -150,7 +150,7 @@ void local_binding_data_init(struct local_binding_data *); > void local_binding_data_destroy(struct local_binding_data *); > > const struct sbrec_port_binding *local_binding_get_primary_pb( > - struct shash *local_bindings, const char *pb_name); > + const struct shash *local_bindings, const char *pb_name); > ofp_port_t local_binding_get_lport_ofport(const struct shash *local_bindings, > const char *pb_name); > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 3a81a13fb0..c82f0697e8 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -199,6 +199,7 @@ static unsigned int > update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > const struct sbrec_chassis *chassis, > const struct sset *local_ifaces, > + const struct shash *local_bindings, > struct hmap *local_datapaths, > bool monitor_all) > { > @@ -297,10 +298,21 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > > if (local_ifaces) { > const char *name; > + > + ovs_assert(local_bindings); > SSET_FOR_EACH (name, local_ifaces) { > + /* Skip the VIFs we bound already, we should have a local datapath > + * for those. */ > + const struct sbrec_port_binding *local_pb > + = local_binding_get_primary_pb(local_bindings, name); > + if (local_pb && get_lport_type(local_pb) == LP_VIF) { > + continue; > + } > sbrec_port_binding_add_clause_logical_port(&pb, OVSDB_F_EQ, name); > - sbrec_port_binding_add_clause_parent_port(&pb, OVSDB_F_EQ, name); > } > + /* Monitor all sub-ports unconditionally; we don't expect a lot of > + * them in the SB database. */ > + sbrec_port_binding_add_clause_parent_port(&pb, OVSDB_F_NE, NULL); > } > if (local_datapaths) { > const struct local_datapath *ld; > @@ -609,7 +621,7 @@ update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl, > * extra cost. Instead, it is called after the engine execution only > * when it is necessary. */ > unsigned int next_cond_seqno = > - update_sb_monitors(ovnsb_idl, NULL, NULL, NULL, true); > + update_sb_monitors(ovnsb_idl, NULL, NULL, NULL, NULL, true); > if (sb_cond_seqno) { > *sb_cond_seqno = next_cond_seqno; > } > @@ -4712,7 +4724,7 @@ main(int argc, char *argv[]) > ovsdb_idl_omit(ovnsb_idl_loop.idl, > &sbrec_chassis_private_col_external_ids); > > - update_sb_monitors(ovnsb_idl_loop.idl, NULL, NULL, NULL, false); > + update_sb_monitors(ovnsb_idl_loop.idl, NULL, NULL, NULL, NULL, false); > > stopwatch_create(CONTROLLER_LOOP_STOPWATCH_NAME, SW_MS); > stopwatch_create(OFCTRL_PUT_STOPWATCH_NAME, SW_MS); > @@ -5355,6 +5367,7 @@ main(int argc, char *argv[]) > update_sb_monitors( > ovnsb_idl_loop.idl, chassis, > &runtime_data->local_lports, > + &runtime_data->lbinding_data.bindings, > &runtime_data->local_datapaths, > sb_monitor_all); > } > -- > 2.31.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
