This patch handles the logical switch creation incrementally in the northd engine node. The dependent engine nodes - ls_stateful, lflow and few others still fall back to full recompute, which will be handled in separate patches.
Reported-at: https://issues.redhat.com/browse/FDP-754 Co-authored-by: Numan Siddique <num...@ovn.org> Signed-off-by: Numan Siddique <num...@ovn.org> Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com> --- northd/en-datapath-logical-switch.c | 16 +++ northd/en-datapath-logical-switch.h | 4 + northd/en-lflow.c | 4 + northd/en-ls-stateful.c | 4 + northd/en-northd.c | 16 +++ northd/en-northd.h | 2 + northd/inc-proc-northd.c | 8 +- northd/northd.c | 174 +++++++++++++++++++++++++++- northd/northd.h | 19 +++ tests/ovn-northd.at | 70 ++++++++++- 10 files changed, 308 insertions(+), 9 deletions(-) diff --git a/northd/en-datapath-logical-switch.c b/northd/en-datapath-logical-switch.c index 2ef2f7d9a..34b29ab92 100644 --- a/northd/en-datapath-logical-switch.c +++ b/northd/en-datapath-logical-switch.c @@ -139,6 +139,22 @@ en_datapath_synced_logical_switch_init(struct engine_node *node OVS_UNUSED, return switch_map; } +struct ovn_synced_logical_switch * +datapath_synced_logical_switch_find( + const struct ovn_synced_logical_switch_map *switch_map, + const struct nbrec_logical_switch *nb_ls) +{ + struct ovn_synced_logical_switch *lsw; + HMAP_FOR_EACH_WITH_HASH (lsw, hmap_node, uuid_hash(&nb_ls->header_.uuid), + &switch_map->synced_switches) { + if (!strcmp(lsw->nb->name, nb_ls->name)) { + return lsw; + } + } + return NULL; +} + + enum engine_node_state en_datapath_synced_logical_switch_run(struct engine_node *node , void *data) { diff --git a/northd/en-datapath-logical-switch.h b/northd/en-datapath-logical-switch.h index 1190b7be8..8e2f497c0 100644 --- a/northd/en-datapath-logical-switch.h +++ b/northd/en-datapath-logical-switch.h @@ -41,6 +41,10 @@ struct ovn_synced_logical_switch_map { void *en_datapath_synced_logical_switch_init(struct engine_node *, struct engine_arg *); +struct ovn_synced_logical_switch * +datapath_synced_logical_switch_find( + const struct ovn_synced_logical_switch_map *switch_map, + const struct nbrec_logical_switch *nb_ls); enum engine_node_state en_datapath_synced_logical_switch_run( struct engine_node *, void *data); void en_datapath_synced_logical_switch_cleanup(void *data); diff --git a/northd/en-lflow.c b/northd/en-lflow.c index 63565ef80..539113556 100644 --- a/northd/en-lflow.c +++ b/northd/en-lflow.c @@ -135,6 +135,10 @@ lflow_northd_handler(struct engine_node *node, return EN_UNHANDLED; } + if (northd_has_lswitchs_in_tracked_data(&northd_data->trk_data)) { + return EN_UNHANDLED; + } + const struct engine_context *eng_ctx = engine_get_context(); struct lflow_data *lflow_data = data; diff --git a/northd/en-ls-stateful.c b/northd/en-ls-stateful.c index 7f07a6506..448ccc3c4 100644 --- a/northd/en-ls-stateful.c +++ b/northd/en-ls-stateful.c @@ -131,6 +131,10 @@ ls_stateful_northd_handler(struct engine_node *node, void *data_) return EN_UNHANDLED; } + if (northd_has_lswitchs_in_tracked_data(&northd_data->trk_data)) { + return EN_UNHANDLED; + } + if (!northd_has_ls_lbs_in_tracked_data(&northd_data->trk_data) && !northd_has_ls_acls_in_tracked_data(&northd_data->trk_data)) { return EN_HANDLED_UNCHANGED; diff --git a/northd/en-northd.c b/northd/en-northd.c index 02be1968e..f2bc80c57 100644 --- a/northd/en-northd.c +++ b/northd/en-northd.c @@ -182,6 +182,22 @@ northd_nb_logical_switch_handler(struct engine_node *node, return EN_HANDLED_UNCHANGED; } +enum engine_input_handler_result +northd_sb_datapath_binding_handler(struct engine_node *node, void *data) +{ + struct northd_input input_data; + struct northd_data *nd = data; + + northd_get_input_data(node, &input_data); + if (!northd_handle_sb_datapath_binding_changes( + input_data.sbrec_datapath_binding_table, &nd->ls_datapaths, + &nd->lr_datapaths)) { + return EN_UNHANDLED; + } + + return EN_HANDLED_UPDATED; +} + enum engine_input_handler_result northd_sb_port_binding_handler(struct engine_node *node, void *data) diff --git a/northd/en-northd.h b/northd/en-northd.h index 4f744a6c5..740e2d30b 100644 --- a/northd/en-northd.h +++ b/northd/en-northd.h @@ -23,6 +23,8 @@ enum engine_input_handler_result northd_nb_logical_router_handler(struct engine_node *, void *data); enum engine_input_handler_result northd_sb_port_binding_handler(struct engine_node *, void *data); +enum engine_input_handler_result +northd_sb_datapath_binding_handler(struct engine_node *, void *data); enum engine_input_handler_result northd_lb_data_handler(struct engine_node *, void *data); enum engine_input_handler_result diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c index 7aa4bdf3f..08483dfd0 100644 --- a/northd/inc-proc-northd.c +++ b/northd/inc-proc-northd.c @@ -284,10 +284,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, engine_add_input(&en_northd, &en_sb_chassis, NULL); engine_add_input(&en_northd, &en_sb_mirror, NULL); engine_add_input(&en_northd, &en_sb_meter, NULL); - engine_add_input(&en_northd, &en_sb_datapath_binding, NULL); engine_add_input(&en_northd, &en_sb_dns, NULL); engine_add_input(&en_northd, &en_sb_ha_chassis_group, NULL); - engine_add_input(&en_northd, &en_sb_ip_multicast, NULL); engine_add_input(&en_northd, &en_sb_service_monitor, NULL); engine_add_input(&en_northd, &en_sb_static_mac_binding, NULL); engine_add_input(&en_northd, &en_sb_chassis_template_var, NULL); @@ -306,6 +304,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, engine_add_input(&en_northd, &en_sb_mac_binding, engine_noop_handler); + engine_add_input(&en_northd, &en_sb_datapath_binding, + northd_sb_datapath_binding_handler); engine_add_input(&en_northd, &en_sb_port_binding, northd_sb_port_binding_handler); engine_add_input(&en_northd, &en_nb_logical_switch, @@ -341,6 +341,10 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, engine_add_input(&en_northd, &en_port_binding_paired_mirror, engine_noop_handler); + /* No need for an explicit handler for the SB datapath and + * SB IP Multicast changes.*/ + engine_add_input(&en_northd, &en_sb_ip_multicast, engine_noop_handler); + engine_add_input(&en_lr_nat, &en_northd, lr_nat_northd_handler); engine_add_input(&en_lr_stateful, &en_northd, lr_stateful_northd_handler); diff --git a/northd/northd.c b/northd/northd.c index b700ca28e..3e8cb8981 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -811,8 +811,11 @@ ods_build_array_index(struct ovn_datapaths *datapaths) * doesn't matter if they are different on every iteration. */ size_t index = 0; - datapaths->array = xrealloc(datapaths->array, - ods_size(datapaths) * sizeof *datapaths->array); + datapaths->n_array = ods_size(datapaths); + datapaths->n_allocated_array = datapaths->n_array + 10; + datapaths->array = xrealloc( + datapaths->array, + datapaths->n_allocated_array * sizeof *datapaths->array); struct ovn_datapath *od; HMAP_FOR_EACH (od, key_node, &datapaths->datapaths) { @@ -822,6 +825,23 @@ ods_build_array_index(struct ovn_datapaths *datapaths) } } +static void +ods_assign_array_index(struct ovn_datapaths *datapaths, + struct ovn_datapath *od) +{ + ovs_assert(ods_size(datapaths) == (datapaths->n_array + 1)); + od->index = datapaths->n_array; + + if (datapaths->n_array == datapaths->n_allocated_array) { + datapaths->n_allocated_array += 10; + datapaths->array = xrealloc( + datapaths->array, + datapaths->n_allocated_array * sizeof *datapaths->array); + } + datapaths->array[datapaths->n_array++] = od; + od->datapaths = datapaths; +} + /* Initializes 'ls_datapaths' to contain a "struct ovn_datapath" for every * logical switch, and initializes 'lr_datapaths' to contain a * "struct ovn_datapath" for every logical router. @@ -4012,6 +4032,12 @@ build_ports(struct ovsdb_idl_txn *ovnsb_txn, sset_destroy(&active_ha_chassis_grps); } +static void +destroy_tracked_dps(struct tracked_dps *trk_dps) +{ + hmapx_clear(&trk_dps->crupdated); +} + static void destroy_tracked_ovn_ports(struct tracked_ovn_ports *trk_ovn_ports) { @@ -4048,6 +4074,7 @@ void destroy_northd_data_tracked_changes(struct northd_data *nd) { struct northd_tracked_data *trk_changes = &nd->trk_data; + destroy_tracked_dps(&trk_changes->trk_switches); destroy_tracked_ovn_ports(&trk_changes->trk_lsps); destroy_tracked_lbs(&trk_changes->trk_lbs); hmapx_clear(&trk_changes->trk_nat_lrs); @@ -4061,6 +4088,7 @@ init_northd_tracked_data(struct northd_data *nd) { struct northd_tracked_data *trk_data = &nd->trk_data; trk_data->type = NORTHD_TRACKED_NONE; + hmapx_init(&trk_data->trk_switches.crupdated); hmapx_init(&trk_data->trk_lsps.created); hmapx_init(&trk_data->trk_lsps.updated); hmapx_init(&trk_data->trk_lsps.deleted); @@ -4076,6 +4104,7 @@ destroy_northd_tracked_data(struct northd_data *nd) { struct northd_tracked_data *trk_data = &nd->trk_data; trk_data->type = NORTHD_TRACKED_NONE; + hmapx_destroy(&trk_data->trk_switches.crupdated); hmapx_destroy(&trk_data->trk_lsps.created); hmapx_destroy(&trk_data->trk_lsps.updated); hmapx_destroy(&trk_data->trk_lsps.deleted); @@ -4524,15 +4553,73 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, const struct northd_input *ni, struct northd_data *nd) { - const struct nbrec_logical_switch *changed_ls; struct northd_tracked_data *trk_data = &nd->trk_data; + nd->trk_data.type = NORTHD_TRACKED_NONE; - NBREC_LOGICAL_SWITCH_TABLE_FOR_EACH_TRACKED (changed_ls, + /* Loop to handle deleted logical switches. */ + const struct nbrec_logical_switch *deleted_ls; + NBREC_LOGICAL_SWITCH_TABLE_FOR_EACH_TRACKED (deleted_ls, + ni->nbrec_logical_switch_table) { + if (nbrec_logical_switch_is_deleted(deleted_ls)) { + goto fail; + } + } + + /* Loop to handle only newly created logical switches and to + * create ovn_datapath objects. */ + const struct nbrec_logical_switch *new_ls; + NBREC_LOGICAL_SWITCH_TABLE_FOR_EACH_TRACKED (new_ls, ni->nbrec_logical_switch_table) { - if (nbrec_logical_switch_is_new(changed_ls) || - nbrec_logical_switch_is_deleted(changed_ls)) { + if (!nbrec_logical_switch_is_new(new_ls)) { + continue; + } + + /* If a logical switch is created with the below columns set, + * then we can't handle this yet. Goto fail. */ + if (new_ls->copp || new_ls->n_dns_records || + new_ls->n_forwarding_groups || new_ls->n_qos_rules) { goto fail; } + + struct ovn_synced_logical_switch *lsw = + datapath_synced_logical_switch_find(ni->synced_lses, new_ls); + if (!lsw) { + goto fail; + } + + + struct ovn_datapath *od = ovn_datapath_create( + &nd->ls_datapaths.datapaths, &new_ls->header_.uuid, new_ls, + NULL, lsw->sb); + + ods_assign_array_index(&nd->ls_datapaths, od); + init_ipam_info_for_datapath(od); + init_mcast_info_for_datapath(od); + + /* Create SB:IP_Multicast for the logical switch. */ + const struct sbrec_ip_multicast *ip_mcast = + sbrec_ip_multicast_insert(ovnsb_idl_txn); + store_mcast_info_for_switch_datapath(ip_mcast, od); + + if (!ls_handle_lsp_changes(ovnsb_idl_txn, new_ls, + ni, nd, od, &trk_data->trk_lsps)) { + goto fail; + } + + if (new_ls->n_acls) { + hmapx_add(&trk_data->ls_with_changed_acls, od); + } + hmapx_add(&trk_data->trk_switches.crupdated, od); + } + + /* Loop to handle updated logical switches. */ + const struct nbrec_logical_switch *changed_ls; + NBREC_LOGICAL_SWITCH_TABLE_FOR_EACH_TRACKED (changed_ls, + ni->nbrec_logical_switch_table) { + if (nbrec_logical_switch_is_new(changed_ls)) { + continue; + } + struct ovn_datapath *od = ovn_datapath_find_( &nd->ls_datapaths.datapaths, &changed_ls->header_.uuid); @@ -4559,6 +4646,10 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, } } + if (!hmapx_is_empty(&trk_data->trk_switches.crupdated)) { + trk_data->type |= NORTHD_TRACKED_SWITCHES; + } + if (!hmapx_is_empty(&trk_data->trk_lsps.created) || !hmapx_is_empty(&trk_data->trk_lsps.updated) || !hmapx_is_empty(&trk_data->trk_lsps.deleted)) { @@ -4766,6 +4857,77 @@ fail: return false; } +bool +northd_handle_sb_datapath_binding_changes( + const struct sbrec_datapath_binding_table *sbrec_dp_table, + const struct ovn_datapaths *ls_datapaths, + const struct ovn_datapaths *lr_datapaths) +{ + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); + + const struct sbrec_datapath_binding *sbrec_dp; + SBREC_DATAPATH_BINDING_TABLE_FOR_EACH_TRACKED (sbrec_dp, sbrec_dp_table) { + const char *name = smap_get(&sbrec_dp->external_ids, "name"); + if (!name) { + VLOG_WARN_RL(&rl, "Datapath "UUID_FMT" is present in the" + "Southbound DB without valid external_ids keys", + UUID_ARGS(&sbrec_dp->header_.uuid)); + return false; + } + + struct ovn_datapath *od; + if (!strcmp(sbrec_dp->type, "logical-switch")) { + od = ovn_datapath_find_(&ls_datapaths->datapaths, + sbrec_dp->nb_uuid); + } else { + od = ovn_datapath_find_(&lr_datapaths->datapaths, + sbrec_dp->nb_uuid); + } + + if (!sbrec_datapath_binding_is_deleted(sbrec_dp) && !od) { + VLOG_WARN_RL(&rl, "Datapath "UUID_FMT" is present in the " + "Southbound DB with mismatching " + "external-ids:logical-switch/router "UUID_FMT, + UUID_ARGS(&sbrec_dp->header_.uuid), + UUID_ARGS(sbrec_dp->nb_uuid)); + return false; + } + + if (sbrec_datapath_binding_is_new(sbrec_dp)) { + if (!od) { + VLOG_WARN_RL(&rl, "A datapath-binding for %s is created but " + "the %s is not found.", name, sbrec_dp->type); + return false; + } + od->sb = sbrec_dp; + } else if (sbrec_datapath_binding_is_deleted(sbrec_dp)) { + /* Most likely the sbrec_dp was deleted by northd and this is the + * notification of that transaction, and we can ignore in this + * case. Fallback to recompute otherwise, to avoid dangling + * sb idl pointers and other unexpected behavior. */ + if (od && od->sb == sbrec_dp) { + VLOG_WARN_RL(&rl, "A datapath-binding for %s is deleted but " + "the %s still exists.", name, sbrec_dp->type); + return false; + } + } else { + /* sbrec_dp is updated.*/ + if (!od) { + VLOG_WARN_RL(&rl, "A datapath-binding for %s is updated but " + "the %s is not found.", name, sbrec_dp->type); + return false; + } + if (od->sb != sbrec_dp) { + VLOG_WARN_RL(&rl, "A datapath-binding for %s is updated with " + "a new IDL row, which is unusual.", name); + return false; + } + } + } + + return true; +} + bool northd_handle_sb_port_binding_changes( const struct sbrec_port_binding_table *sbrec_port_binding_table, diff --git a/northd/northd.h b/northd/northd.h index ac4499ef7..88405665c 100644 --- a/northd/northd.h +++ b/northd/northd.h @@ -99,6 +99,8 @@ struct ovn_datapaths { /* The array index of each element in 'datapaths'. */ struct ovn_datapath **array; + size_t n_array; + size_t n_allocated_array; }; static inline size_t @@ -119,6 +121,11 @@ enum redirected_routing_protcol_flag_type { REDIRECT_BFD = (1 << 1), }; +struct tracked_dps { + /* Tracked created or updated datapaths. */ + struct hmapx crupdated; +}; + struct tracked_ovn_ports { /* tracked created ports. * hmapx node data is 'struct ovn_port *' */ @@ -150,6 +157,7 @@ enum northd_tracked_data_type { NORTHD_TRACKED_LR_NATS = (1 << 2), NORTHD_TRACKED_LS_LBS = (1 << 3), NORTHD_TRACKED_LS_ACLS = (1 << 4), + NORTHD_TRACKED_SWITCHES = (1 << 5), }; /* Track what's changed in the northd engine node. @@ -158,6 +166,7 @@ enum northd_tracked_data_type { struct northd_tracked_data { /* Indicates the type of data tracked. One or all of NORTHD_TRACKED_*. */ enum northd_tracked_data_type type; + struct tracked_dps trk_switches; struct tracked_ovn_ports trk_lsps; struct tracked_lbs trk_lbs; @@ -893,6 +902,10 @@ bool lflow_handle_ls_stateful_changes(struct ovsdb_idl_txn *, struct ls_stateful_tracked_data *, struct lflow_input *, struct lflow_table *lflows); +bool northd_handle_sb_datapath_binding_changes( + const struct sbrec_datapath_binding_table *, + const struct ovn_datapaths *ls_datapaths, + const struct ovn_datapaths *lr_datapaths); bool northd_handle_sb_port_binding_changes( const struct sbrec_port_binding_table *, struct hmap *ls_ports, struct hmap *lr_ports); @@ -962,6 +975,12 @@ northd_has_ls_acls_in_tracked_data(struct northd_tracked_data *trk_nd_changes) return trk_nd_changes->type & NORTHD_TRACKED_LS_ACLS; } +static inline bool +northd_has_lswitchs_in_tracked_data(struct northd_tracked_data *trk_nd_changes) +{ + return trk_nd_changes->type & NORTHD_TRACKED_SWITCHES; +} + /* Returns 'true' if the IPv4 'addr' is on the same subnet with one of the * IPs configured on the router port. */ diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 33cd8a4ae..a94f01923 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -12595,7 +12595,7 @@ check ovn-nbctl --wait=sb lsp-add sw0 sw0p1 -- lsp-set-addresses sw0p1 "00:00:20 check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats check ovn-nbctl --wait=sb lr-add lr0 -check_engine_stats northd recompute nocompute +check_engine_stats northd recompute compute check_engine_stats lr_nat recompute nocompute check_engine_stats lr_stateful recompute nocompute check_engine_stats sync_to_sb_pb recompute nocompute @@ -14656,6 +14656,74 @@ AT_CHECK([grep "lr_in_dnat" lr1flows | ovn_strip_lflows | grep "30.0.0.1"], [0], AT_CLEANUP ]) +OVN_FOR_EACH_NORTHD_NO_HV([ +AT_SETUP([Logical switch incremental processing]) + +ovn_start + +net_add n1 +sim_add hv1 +as hv1 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.11 + +check ovn-sbctl chassis-add gw1 geneve 127.0.0.1 +check ovn-nbctl --wait=sb sync + +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +check ovn-nbctl --wait=sb ls-add sw0 +check_engine_stats northd norecompute compute +check_engine_stats ls_stateful recompute compute +check_engine_stats lflow recompute nocompute + +# For the below engine nodes, en_northd is input. So check +# their engine status. +check_engine_stats lr_stateful recompute compute +check_engine_stats route_policies recompute compute +check_engine_stats routes recompute compute +check_engine_stats bfd_sync recompute compute +check_engine_stats sync_to_sb_lb recompute compute +check_engine_stats sync_to_sb_pb recompute compute + +CHECK_NO_CHANGE_AFTER_RECOMPUTE((1)) + +# Update the logical switch. +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +check ovn-nbctl --wait=sb set logical_switch sw0 other_config:foo=bar + +check_engine_stats northd recompute nocompute +check_engine_stats ls_stateful recompute nocompute +check_engine_stats lflow recompute nocompute + +# For the below engine nodes, en_northd is input. So check +# their engine status. +check_engine_stats lr_stateful recompute nocompute +check_engine_stats route_policies recompute nocompute +check_engine_stats routes recompute nocompute +check_engine_stats bfd_sync recompute nocompute +check_engine_stats sync_to_sb_lb recompute nocompute +check_engine_stats sync_to_sb_pb recompute nocompute + +# Delete the logical switch +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +check ovn-nbctl --wait=sb ls-del sw0 +check_engine_stats northd recompute compute +check_engine_stats ls_stateful recompute nocompute +check_engine_stats lflow recompute nocompute + +# For the below engine nodes, en_northd is input. So check +# their engine status. +check_engine_stats lr_stateful recompute nocompute +check_engine_stats route_policies recompute nocompute +check_engine_stats routes recompute nocompute +check_engine_stats bfd_sync recompute nocompute +check_engine_stats sync_to_sb_lb recompute nocompute +check_engine_stats sync_to_sb_pb recompute nocompute + +OVN_CLEANUP([hv1]) +AT_CLEANUP +]) + AT_SETUP([RBAC -- Recover builtin role and permissions]) ovn_start -- 2.50.0 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
