On Wed, Jun 28, 2023 at 5:46 AM Dumitru Ceara <[email protected]> wrote: > > On 6/2/23 06:11, Han Zhou wrote: > > This patch introduces a change handler for 'northd' input within the > > 'lflow' node. It specifically handles cases when VIFs are created, which > > is an easier start for lflow incremental processing. Support for > > update/delete will be added later. > > > > Below are the performance test results simulating an ovn-k8s topology of 500 > > nodes x 50 lsp per node: > > > > Before: > > ovn-nbctl --wait=hv --print-wait-time lsp-add ls_1_0001 lsp_1_0001_01 -- lsp-set-addresses lsp_1_0001_01 "ff:f1:bb:00:01:01 1.0.1.101" -- lsp-set-port-security lsp_1_0001_01 "ff:f1:bb:00:01:01 1.0.1.101" > > ovn-northd completion: 773ms > > > > After: > > ovn-nbctl --wait=hv --print-wait-time lsp-add ls_1_0001 lsp_1_0001_01 -- lsp-set-addresses lsp_1_0001_01 "ff:f1:bb:00:01:01 1.0.1.101" -- lsp-set-port-security lsp_1_0001_01 "ff:f1:bb:00:01:01 1.0.1.101" > > ovn-northd completion: 30ms > > > > It is more than 95% reduction (or 20x faster). > > > > Signed-off-by: Han Zhou <[email protected]> > > Reviewed-by: Ales Musil <[email protected]> > > --- > > northd/en-lflow.c | 82 ++++++++----- > > northd/en-lflow.h | 1 + > > northd/inc-proc-northd.c | 2 +- > > northd/northd.c | 245 +++++++++++++++++++++++++++++++++------ > > northd/northd.h | 6 +- > > tests/ovn-northd.at | 4 +- > > 6 files changed, 277 insertions(+), 63 deletions(-) > > > > diff --git a/northd/en-lflow.c b/northd/en-lflow.c > > index 081ec7c353ed..28ab1c67fb8f 100644 > > --- a/northd/en-lflow.c > > +++ b/northd/en-lflow.c > > @@ -30,43 +30,49 @@ > > > > VLOG_DEFINE_THIS_MODULE(en_lflow); > > > > -void en_lflow_run(struct engine_node *node, void *data) > > +static void > > +lflow_get_input_data(struct engine_node *node, > > + struct lflow_input *lflow_input) > > { > > - const struct engine_context *eng_ctx = engine_get_context(); > > - > > - struct lflow_input lflow_input; > > - > > struct northd_data *northd_data = engine_get_input_data("northd", node); > > - > > - struct hmap bfd_connections = HMAP_INITIALIZER(&bfd_connections); > > - > > - lflow_input.nbrec_bfd_table = > > + lflow_input->nbrec_bfd_table = > > EN_OVSDB_GET(engine_get_input("NB_bfd", node)); > > - lflow_input.sbrec_bfd_table = > > + lflow_input->sbrec_bfd_table = > > EN_OVSDB_GET(engine_get_input("SB_bfd", node)); > > - lflow_input.sbrec_logical_flow_table = > > + lflow_input->sbrec_logical_flow_table = > > EN_OVSDB_GET(engine_get_input("SB_logical_flow", node)); > > - lflow_input.sbrec_multicast_group_table = > > + lflow_input->sbrec_multicast_group_table = > > EN_OVSDB_GET(engine_get_input("SB_multicast_group", node)); > > - lflow_input.sbrec_igmp_group_table = > > + lflow_input->sbrec_igmp_group_table = > > EN_OVSDB_GET(engine_get_input("SB_igmp_group", node)); > > > > - lflow_input.sbrec_mcast_group_by_name_dp = > > + lflow_input->sbrec_mcast_group_by_name_dp = > > engine_ovsdb_node_get_index( > > engine_get_input("SB_multicast_group", node), > > "sbrec_mcast_group_by_name"); > > > > - lflow_input.ls_datapaths = &northd_data->ls_datapaths; > > - lflow_input.lr_datapaths = &northd_data->lr_datapaths; > > - lflow_input.ls_ports = &northd_data->ls_ports; > > - lflow_input.lr_ports = &northd_data->lr_ports; > > - lflow_input.port_groups = &northd_data->port_groups; > > - lflow_input.meter_groups = &northd_data->meter_groups; > > - lflow_input.lbs = &northd_data->lbs; > > - lflow_input.bfd_connections = &bfd_connections; > > - lflow_input.features = &northd_data->features; > > - lflow_input.ovn_internal_version_changed = > > + lflow_input->ls_datapaths = &northd_data->ls_datapaths; > > + lflow_input->lr_datapaths = &northd_data->lr_datapaths; > > + lflow_input->ls_ports = &northd_data->ls_ports; > > + lflow_input->lr_ports = &northd_data->lr_ports; > > + lflow_input->port_groups = &northd_data->port_groups; > > + lflow_input->meter_groups = &northd_data->meter_groups; > > + lflow_input->lbs = &northd_data->lbs; > > + lflow_input->features = &northd_data->features; > > + lflow_input->ovn_internal_version_changed = > > northd_data->ovn_internal_version_changed; > > + lflow_input->bfd_connections = NULL; > > +} > > + > > +void en_lflow_run(struct engine_node *node, void *data) > > +{ > > + const struct engine_context *eng_ctx = engine_get_context(); > > + > > + struct lflow_input lflow_input; > > + lflow_get_input_data(node, &lflow_input); > > + > > + struct hmap bfd_connections = HMAP_INITIALIZER(&bfd_connections); > > + lflow_input.bfd_connections = &bfd_connections; > > > > struct lflow_data *lflow_data = data; > > lflow_data_destroy(lflow_data); > > @@ -76,8 +82,8 @@ void en_lflow_run(struct engine_node *node, void *data) > > build_bfd_table(eng_ctx->ovnsb_idl_txn, > > lflow_input.nbrec_bfd_table, > > lflow_input.sbrec_bfd_table, > > - &bfd_connections, > > - &northd_data->lr_ports); > > + lflow_input.lr_ports, > > + &bfd_connections); > > build_lflows(eng_ctx->ovnsb_idl_txn, &lflow_input, &lflow_data->lflows); > > bfd_cleanup_connections(lflow_input.nbrec_bfd_table, > > &bfd_connections); > > @@ -87,6 +93,30 @@ void en_lflow_run(struct engine_node *node, void *data) > > engine_set_node_state(node, EN_UPDATED); > > } > > > > +bool > > +lflow_northd_handler(struct engine_node *node, > > + void *data) > > +{ > > + struct northd_data *northd_data = engine_get_input_data("northd", node); > > + if (!northd_data->change_tracked) { > > + return false; > > + } > > + const struct engine_context *eng_ctx = engine_get_context(); > > + struct lflow_data *lflow_data = data; > > + > > + struct lflow_input lflow_input; > > + lflow_get_input_data(node, &lflow_input); > > + > > + if (!lflow_handle_northd_ls_changes(eng_ctx->ovnsb_idl_txn, > > + &northd_data->tracked_ls_changes, > > + &lflow_input, &lflow_data->lflows)) { > > + return false; > > + } > > + > > + engine_set_node_state(node, EN_UPDATED); > > + return true; > > +} > > + > > void *en_lflow_init(struct engine_node *node OVS_UNUSED, > > struct engine_arg *arg OVS_UNUSED) > > { > > diff --git a/northd/en-lflow.h b/northd/en-lflow.h > > index 0e4d522ff3fe..5e3fbc25e3e0 100644 > > --- a/northd/en-lflow.h > > +++ b/northd/en-lflow.h > > @@ -12,5 +12,6 @@ > > void en_lflow_run(struct engine_node *node, void *data); > > void *en_lflow_init(struct engine_node *node, struct engine_arg *arg); > > void en_lflow_cleanup(void *data); > > +bool lflow_northd_handler(struct engine_node *, void *data); > > > > #endif /* EN_LFLOW_H */ > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > > index f992a9ec8420..f6ceb8280624 100644 > > --- a/northd/inc-proc-northd.c > > +++ b/northd/inc-proc-northd.c > > @@ -182,7 +182,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > engine_add_input(&en_lflow, &en_sb_logical_flow, NULL); > > engine_add_input(&en_lflow, &en_sb_multicast_group, NULL); > > engine_add_input(&en_lflow, &en_sb_igmp_group, NULL); > > - engine_add_input(&en_lflow, &en_northd, NULL); > > + engine_add_input(&en_lflow, &en_northd, lflow_northd_handler); > > > > engine_add_input(&en_sync_to_sb_addr_set, &en_nb_address_set, > > sync_to_sb_addr_set_nb_address_set_handler); > > diff --git a/northd/northd.c b/northd/northd.c > > index 196653ad3d0e..f8deed53bd8c 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -5472,6 +5472,7 @@ ovn_igmp_group_destroy(struct hmap *igmp_groups, > > > > struct ovn_lflow { > > struct hmap_node hmap_node; > > + struct ovs_list list_node; > > > > struct ovn_datapath *od; /* 'logical_datapath' in SB schema. */ > > unsigned long *dpg_bitmap; /* Bitmap of all datapaths by their 'index'.*/ > > @@ -5531,6 +5532,7 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct ovn_datapath *od, > > char *match, char *actions, char *io_port, char *ctrl_meter, > > char *stage_hint, const char *where) > > { > > + ovs_list_init(&lflow->list_node); > > lflow->dpg_bitmap = bitmap_allocate(dp_bitmap_len); > > lflow->od = od; > > lflow->stage = stage; > > @@ -5659,7 +5661,18 @@ ovn_dp_group_add_with_reference(struct ovn_lflow *lflow_ref, > > > > /* Adds a row with the specified contents to the Logical_Flow table. > > * Version to use when hash bucket locking is NOT required. > > + * > > + * Note: This function can add generated lflows to the global variable > > + * temp_lflow_list as its output, controlled by the global variable > > + * add_lflow_to_temp_list. The caller of the ovn_lflow_add_... marcros can get > > + * a list of lflows generated by setting add_lflow_to_temp_list to true. The > > + * caller is responsible for initializing the temp_lflow_list, and also > > + * reset the add_lflow_to_temp_list to false when it is no longer needed. > > + * XXX: this mechanism is temporary and will be replaced when we add hash index > > + * to lflow_data and refactor related functions. > > */ > > +static bool add_lflow_to_temp_list = false; > > +static struct ovs_list temp_lflow_list; > > static void > > do_ovn_lflow_add(struct hmap *lflow_map, const struct ovn_datapath *od, > > const unsigned long *dp_bitmap, size_t dp_bitmap_len, > > @@ -5676,12 +5689,17 @@ do_ovn_lflow_add(struct hmap *lflow_map, const struct ovn_datapath *od, > > size_t bitmap_len = od ? ods_size(od->datapaths) : dp_bitmap_len; > > ovs_assert(bitmap_len); > > > > - old_lflow = ovn_lflow_find(lflow_map, NULL, stage, priority, match, > > - actions, ctrl_meter, hash); > > - if (old_lflow) { > > - ovn_dp_group_add_with_reference(old_lflow, od, dp_bitmap, > > - bitmap_len); > > - return; > > + if (add_lflow_to_temp_list) { > > + ovs_assert(od); > > + ovs_assert(!dp_bitmap); > > + } else { > > + old_lflow = ovn_lflow_find(lflow_map, NULL, stage, priority, match, > > + actions, ctrl_meter, hash); > > + if (old_lflow) { > > + ovn_dp_group_add_with_reference(old_lflow, od, dp_bitmap, > > + bitmap_len); > > + return; > > + } > > } > > > > lflow = xmalloc(sizeof *lflow); > > @@ -5702,6 +5720,10 @@ do_ovn_lflow_add(struct hmap *lflow_map, const struct ovn_datapath *od, > > hmap_insert_fast(lflow_map, &lflow->hmap_node, hash); > > thread_lflow_counter++; > > } > > + > > + if (add_lflow_to_temp_list) { > > + ovs_list_insert(&temp_lflow_list, &lflow->list_node); > > + } > > } > > > > /* Adds a row with the specified contents to the Logical_Flow table. */ > > @@ -9947,7 +9969,7 @@ void > > build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn, > > const struct nbrec_bfd_table *nbrec_bfd_table, > > const struct sbrec_bfd_table *sbrec_bfd_table, > > - struct hmap *bfd_connections, struct hmap *lr_ports) > > + const struct hmap *lr_ports, struct hmap *bfd_connections) > > { > > struct hmap sb_only = HMAP_INITIALIZER(&sb_only); > > const struct sbrec_bfd *sb_bt; > > @@ -15299,32 +15321,28 @@ build_lswitch_and_lrouter_iterate_by_lr(struct ovn_datapath *od, > > */ > > static void > > build_lswitch_and_lrouter_iterate_by_lsp(struct ovn_port *op, > > - struct lswitch_flow_build_info *lsi) > > + const struct hmap *ls_ports, > > + const struct hmap *lr_ports, > > + const struct shash *meter_groups, > > + struct ds *match, > > + struct ds *actions, > > + struct hmap *lflows) > > { > > ovs_assert(op->nbsp); > > > > /* Build Logical Switch Flows. */ > > - build_lswitch_port_sec_op(op, lsi->lflows, &lsi->actions, &lsi->match); > > - build_lswitch_learn_fdb_op(op, lsi->lflows, &lsi->actions, > > - &lsi->match); > > - build_lswitch_arp_nd_responder_skip_local(op, lsi->lflows, > > - &lsi->match); > > - build_lswitch_arp_nd_responder_known_ips(op, lsi->lflows, > > - lsi->ls_ports, > > - lsi->meter_groups, > > - &lsi->actions, > > - &lsi->match); > > - build_lswitch_dhcp_options_and_response(op, lsi->lflows, > > - lsi->meter_groups); > > - build_lswitch_external_port(op, lsi->lflows); > > - build_lswitch_ip_unicast_lookup(op, lsi->lflows, &lsi->actions, > > - &lsi->match); > > + build_lswitch_port_sec_op(op, lflows, actions, match); > > + build_lswitch_learn_fdb_op(op, lflows, actions, match); > > + build_lswitch_arp_nd_responder_skip_local(op, lflows, match); > > + build_lswitch_arp_nd_responder_known_ips(op, lflows, ls_ports, > > + meter_groups, actions, match); > > + build_lswitch_dhcp_options_and_response(op, lflows, meter_groups); > > + build_lswitch_external_port(op, lflows); > > + build_lswitch_ip_unicast_lookup(op, lflows, actions, match); > > > > /* Build Logical Router Flows. */ > > - build_ip_routing_flows_for_router_type_lsp(op, lsi->lr_ports, > > - lsi->lflows); > > - build_arp_resolve_flows_for_lsp(op, lsi->lflows, lsi->lr_ports, > > - &lsi->match, &lsi->actions); > > + build_ip_routing_flows_for_router_type_lsp(op, lr_ports, lflows); > > + build_arp_resolve_flows_for_lsp(op, lflows, lr_ports, match, actions); > > } > > > > /* Helper function to combine all lflow generation which is iterated by logical > > @@ -15410,7 +15428,12 @@ build_lflows_thread(void *arg) > > if (stop_parallel_processing()) { > > return NULL; > > } > > - build_lswitch_and_lrouter_iterate_by_lsp(op, lsi); > > + build_lswitch_and_lrouter_iterate_by_lsp(op, lsi->ls_ports, > > + lsi->lr_ports, > > + lsi->meter_groups, > > + &lsi->match, > > + &lsi->actions, > > + lsi->lflows); > > } > > } > > for (bnum = control->id; > > @@ -15595,7 +15618,11 @@ build_lswitch_and_lrouter_flows(const struct ovn_datapaths *ls_datapaths, > > stopwatch_stop(LFLOWS_DATAPATHS_STOPWATCH_NAME, time_msec()); > > stopwatch_start(LFLOWS_PORTS_STOPWATCH_NAME, time_msec()); > > HMAP_FOR_EACH (op, key_node, ls_ports) { > > - build_lswitch_and_lrouter_iterate_by_lsp(op, &lsi); > > + build_lswitch_and_lrouter_iterate_by_lsp(op, lsi.ls_ports, > > + lsi.lr_ports, > > + lsi.meter_groups, > > + &lsi.match, &lsi.actions, > > + lsi.lflows); > > } > > HMAP_FOR_EACH (op, key_node, lr_ports) { > > build_lswitch_and_lrouter_iterate_by_lrp(op, &lsi); > > @@ -15677,6 +15704,20 @@ build_mcast_groups(const struct sbrec_igmp_group_table *sbrec_igmp_group_table, > > struct hmap *mcast_groups, > > struct hmap *igmp_groups); > > > > +static struct sbrec_multicast_group * > > +create_sb_multicast_group(struct ovsdb_idl_txn *ovnsb_txn, > > + const struct sbrec_datapath_binding *dp, > > + const char *name, > > + int64_t tunnel_key) > > +{ > > + struct sbrec_multicast_group *sbmc = > > + sbrec_multicast_group_insert(ovnsb_txn); > > + sbrec_multicast_group_set_datapath(sbmc, dp); > > + sbrec_multicast_group_set_name(sbmc, name); > > + sbrec_multicast_group_set_tunnel_key(sbmc, tunnel_key); > > + return sbmc; > > +} > > + > > /* Updates the Logical_Flow and Multicast_Group tables in the OVN_SB database, > > * constructing their contents based on the OVN_NB database. */ > > void build_lflows(struct ovsdb_idl_txn *ovnsb_txn, > > @@ -16002,10 +16043,8 @@ void build_lflows(struct ovsdb_idl_txn *ovnsb_txn, > > ovn_multicast_destroy(&mcast_groups, mc); > > continue; > > } > > - sbmc = sbrec_multicast_group_insert(ovnsb_txn); > > - sbrec_multicast_group_set_datapath(sbmc, mc->datapath->sb); > > - sbrec_multicast_group_set_name(sbmc, mc->group->name); > > - sbrec_multicast_group_set_tunnel_key(sbmc, mc->group->key); > > + sbmc = create_sb_multicast_group(ovnsb_txn, mc->datapath->sb, > > + mc->group->name, mc->group->key); > > ovn_multicast_update_sbrec(mc, sbmc); > > ovn_multicast_destroy(&mcast_groups, mc); > > } > > @@ -16020,6 +16059,146 @@ void build_lflows(struct ovsdb_idl_txn *ovnsb_txn, > > hmap_destroy(&mcast_groups); > > } > > > > +bool lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn, > > + struct tracked_ls_changes *ls_changes, > > + struct lflow_input *lflow_input, > > + struct hmap *lflows) > > +{ > > + struct ls_change *ls_change; > > + LIST_FOR_EACH (ls_change, list_node, &ls_changes->updated) { > > + ovs_list_init(&temp_lflow_list); > > + add_lflow_to_temp_list = true; > > + if (!ovs_list_is_empty(&ls_change->updated_ports) || > > + !ovs_list_is_empty(&ls_change->deleted_ports)) { > > + /* XXX: implement lflow index so that we can handle updated and > > + * deleted LSPs incrementally. */ > > + ovs_list_init(&temp_lflow_list); > > + add_lflow_to_temp_list = false; > > + return false; > > + } > > + > > + const struct sbrec_multicast_group *sbmc_flood = > > + mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp, > > + MC_FLOOD, ls_change->od->sb); > > + const struct sbrec_multicast_group *sbmc_flood_l2 = > > + mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp, > > + MC_FLOOD_L2, ls_change->od->sb); > > + const struct sbrec_multicast_group *sbmc_unknown = > > + mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp, > > + MC_UNKNOWN, ls_change->od->sb); > > + > > + struct ovn_port *op; > > + LIST_FOR_EACH (op, list, &ls_change->added_ports) { > > + struct ds match = DS_EMPTY_INITIALIZER; > > + struct ds actions = DS_EMPTY_INITIALIZER; > > + build_lswitch_and_lrouter_iterate_by_lsp(op, lflow_input->ls_ports, > > + lflow_input->lr_ports, > > + lflow_input->meter_groups, > > + &match, &actions, > > + lflows); > > + ds_destroy(&match); > > + ds_destroy(&actions); > > + if (!sbmc_flood) { > > + sbmc_flood = create_sb_multicast_group(ovnsb_txn, > > + ls_change->od->sb, MC_FLOOD, OVN_MCAST_FLOOD_TUNNEL_KEY); > > + } > > + sbrec_multicast_group_update_ports_addvalue(sbmc_flood, op->sb); > > + > > + if (!sbmc_flood_l2) { > > + sbmc_flood_l2 = create_sb_multicast_group(ovnsb_txn, > > + ls_change->od->sb, MC_FLOOD_L2, > > + OVN_MCAST_FLOOD_L2_TUNNEL_KEY); > > + } > > + sbrec_multicast_group_update_ports_addvalue(sbmc_flood_l2, op->sb); > > + > > + if (op->has_unknown) { > > + if (!sbmc_unknown) { > > + sbmc_unknown = create_sb_multicast_group(ovnsb_txn, > > + ls_change->od->sb, MC_UNKNOWN, > > + OVN_MCAST_UNKNOWN_TUNNEL_KEY); > > + } > > + sbrec_multicast_group_update_ports_addvalue(sbmc_unknown, > > + op->sb); > > + } > > + } > > + /* Sync the newly added flows to SB. */ > > + struct ovn_lflow *lflow; > > + LIST_FOR_EACH (lflow, list_node, &temp_lflow_list) { > > + size_t n_datapaths; > > + struct ovn_datapath **datapaths_array; > > + if (ovn_stage_to_datapath_type(lflow->stage) == DP_SWITCH) { > > + n_datapaths = ods_size(lflow_input->ls_datapaths); > > + datapaths_array = lflow_input->ls_datapaths->array; > > + } else { > > + n_datapaths = ods_size(lflow_input->lr_datapaths); > > + datapaths_array = lflow_input->lr_datapaths->array; > > + } > > + uint32_t n_ods = bitmap_count1(lflow->dpg_bitmap, n_datapaths); > > + ovs_assert(n_ods == 1); > > + /* There is only one datapath, so it should be moved out of the > > + * group to a single 'od'. */ > > + size_t index = bitmap_scan(lflow->dpg_bitmap, true, 0, > > + n_datapaths); > > + > > + bitmap_set0(lflow->dpg_bitmap, index); > > + lflow->od = datapaths_array[index]; > > + > > + /* Logical flow should be re-hashed to allow lookups. */ > > + uint32_t hash = hmap_node_hash(&lflow->hmap_node); > > + /* Remove from lflows. */ > > + hmap_remove(lflows, &lflow->hmap_node); > > + hash = ovn_logical_flow_hash_datapath(&lflow->od->sb->header_.uuid, > > + hash); > > + /* Add back. */ > > + hmap_insert(lflows, &lflow->hmap_node, hash); > > + > > + /* Sync to SB. */ > > + const struct sbrec_logical_flow *sbflow; > > + sbflow = sbrec_logical_flow_insert(ovnsb_txn); > > + const char *pipeline = ovn_stage_get_pipeline_name(lflow->stage); > > + uint8_t table = ovn_stage_get_table(lflow->stage); > > + sbrec_logical_flow_set_logical_datapath(sbflow, lflow->od->sb); > > + sbrec_logical_flow_set_logical_dp_group(sbflow, NULL); > > + sbrec_logical_flow_set_pipeline(sbflow, pipeline); > > + sbrec_logical_flow_set_table_id(sbflow, table); > > + sbrec_logical_flow_set_priority(sbflow, lflow->priority); > > + sbrec_logical_flow_set_match(sbflow, lflow->match); > > + sbrec_logical_flow_set_actions(sbflow, lflow->actions); > > + if (lflow->io_port) { > > + struct smap tags = SMAP_INITIALIZER(&tags); > > + smap_add(&tags, "in_out_port", lflow->io_port); > > + sbrec_logical_flow_set_tags(sbflow, &tags); > > + smap_destroy(&tags); > > + } > > + sbrec_logical_flow_set_controller_meter(sbflow, lflow->ctrl_meter); > > + /* Trim the source locator lflow->where, which looks something like > > + * "ovn/northd/northd.c:1234", down to just the part following the > > + * last slash, e.g. "northd.c:1234". */ > > + const char *slash = strrchr(lflow->where, '/'); > > +#if _WIN32 > > + const char *backslash = strrchr(lflow->where, '\\'); > > + if (!slash || backslash > slash) { > > + slash = backslash; > > + } > > +#endif > > + const char *where = slash ? slash + 1 : lflow->where; > > + > > + struct smap ids = SMAP_INITIALIZER(&ids); > > + smap_add(&ids, "stage-name", ovn_stage_to_str(lflow->stage)); > > + smap_add(&ids, "source", where); > > + if (lflow->stage_hint) { > > + smap_add(&ids, "stage-hint", lflow->stage_hint); > > + } > > + sbrec_logical_flow_set_external_ids(sbflow, &ids); > > + smap_destroy(&ids); > > + } > > Hi Han, Numan, > > I'm quite worried about this kind of duplication. Most of this code is > the same as the one that iterates the lflow hash map in build_lflows(). > With some subtle differences. > > In my opinion we need to refactor this. It will be impossible to > maintain correctly when we have to make changes in one of the two cases. > Agree. I think I was lazy when implementing this. In this pending patch for VIF update/deletion I-P ( https://patchwork.ozlabs.org/project/ovn/list/?series=360158) I have abstracted a function:
sync_lsp_lflows_to_sb I will send a follow up patch to try reusing the code in the recompute path. > Again, sorry for bringing this up so late. But, as we add more I-P, > I think it's best if we try to avoid this kind of duplication. > > If you're busy I can try to see if I can refactor this part and maybe > figure out the lflow index TODO: > > /* XXX: implement lflow index so that we can handle updated and > * deleted LSPs incrementally. */ > This TODO is addressed by the above mentioned series. It would be great if you could review it, too. Thanks, Han > Wdyt? > > Thanks, > Dumitru > > > + } > > + ovs_list_init(&temp_lflow_list); > > + add_lflow_to_temp_list = false; > > + return true; > > + > > +} > > + > > /* Each port group in Port_Group table in OVN_Northbound has a corresponding > > * entry in Port_Group table in OVN_Southbound. In OVN_Northbound the entries > > * contains lport uuids, while in OVN_Southbound we store the lport names. > > diff --git a/northd/northd.h b/northd/northd.h > > index 1fd9bed477e7..6cb88fe87915 100644 > > --- a/northd/northd.h > > +++ b/northd/northd.h > > @@ -331,11 +331,15 @@ void northd_indices_create(struct northd_data *data, > > void build_lflows(struct ovsdb_idl_txn *ovnsb_txn, > > struct lflow_input *input_data, > > struct hmap *lflows); > > +bool lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn, > > + struct tracked_ls_changes *, > > + struct lflow_input *, struct hmap *lflows); > > > > void build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn, > > const struct nbrec_bfd_table *, > > const struct sbrec_bfd_table *, > > - struct hmap *bfd_connections, struct hmap *lr_ports); > > + const struct hmap *lr_ports, > > + struct hmap *bfd_connections); > > void bfd_cleanup_connections(const struct nbrec_bfd_table *, > > struct hmap *bfd_map); > > void run_update_worker_pool(int n_threads); > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index 74ae84530112..635cd2d892e6 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -9506,14 +9506,14 @@ check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=hv lsp-add ls0 lsp0-1 -- lsp-set-addresses lsp0-1 "aa:aa:aa:00:00:01 192.168.0.11" > > AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd recompute], [0], [3 > > ]) > > -AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow recompute], [0], [6 > > +AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow recompute], [0], [5 > > ]) > > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > > check ovn-nbctl --wait=hv lsp-add ls0 lsp0-2 -- lsp-set-addresses lsp0-2 "aa:aa:aa:00:00:02 192.168.0.12" > > AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd recompute], [0], [3 > > ]) > > -AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow recompute], [0], [6 > > +AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow recompute], [0], [5 > > ]) > > > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
