1) Encapsulate global service monitor addresses into a structure 2) Simplify network function service monitor creation by passing the nbrec_network_function object directly instead of individual fields. 3) Moved required filed setting inside the service monitor lookup/create function and removed assert bc function cannot return null. 4) chassis_name parameter is now set outside the lookup/create function.
Signed-off-by: Alexandra Rukomoinikova <[email protected]> --- v2 --> v3: 1) combined the last two patches in v2 and removed the changes that were not agreed upon. 2) moved chassis_name setting outside lookup/create function - This separation clarifies that only fields which are immutable (cannot be changed without recreating the monitor) are handled inside this function. --- northd/en-global-config.c | 58 ++++++++------- northd/en-global-config.h | 28 +++---- northd/en-lflow.c | 3 +- northd/en-northd.c | 8 +- northd/en-sync-sb.c | 4 +- northd/northd.c | 150 +++++++++++++++++--------------------- northd/northd.h | 8 +- 7 files changed, 119 insertions(+), 140 deletions(-) diff --git a/northd/en-global-config.c b/northd/en-global-config.c index 2556b2888..5d6050578 100644 --- a/northd/en-global-config.c +++ b/northd/en-global-config.c @@ -124,16 +124,19 @@ en_global_config_run(struct engine_node *node , void *data) const char *mac_addr_prefix = set_mac_prefix(smap_get(&nb->options, "mac_prefix")); - const char *monitor_mac = smap_get(&nb->options, "svc_monitor_mac"); - if (monitor_mac) { - if (eth_addr_from_string(monitor_mac, - &config_data->svc_monitor_mac_ea)) { - snprintf(config_data->svc_monitor_mac, - sizeof config_data->svc_monitor_mac, + struct svc_monitor_addresses *svc_addresses = + &config_data->svc_global_addresses; + + const char *src_monitor_mac = smap_get(&nb->options, "svc_monitor_mac"); + if (src_monitor_mac) { + if (eth_addr_from_string(src_monitor_mac, + &svc_addresses->mac_ea_src)) { + snprintf(svc_addresses->mac_src, + sizeof svc_addresses->mac_src, ETH_ADDR_FMT, - ETH_ADDR_ARGS(config_data->svc_monitor_mac_ea)); + ETH_ADDR_ARGS(svc_addresses->mac_ea_src)); } else { - monitor_mac = NULL; + src_monitor_mac = NULL; } } @@ -141,22 +144,21 @@ en_global_config_run(struct engine_node *node , void *data) "svc_monitor_mac_dst"); if (dst_monitor_mac) { if (eth_addr_from_string(dst_monitor_mac, - &config_data->svc_monitor_mac_ea_dst)) { - snprintf(config_data->svc_monitor_mac_dst, - sizeof config_data->svc_monitor_mac_dst, + &svc_addresses->mac_ea_dst)) { + snprintf(svc_addresses->mac_dst, + sizeof svc_addresses->mac_dst, ETH_ADDR_FMT, - ETH_ADDR_ARGS(config_data->svc_monitor_mac_ea_dst)); + ETH_ADDR_ARGS(svc_addresses->mac_ea_dst)); } else { dst_monitor_mac = NULL; } } const char *monitor_ip = smap_get(&nb->options, "svc_monitor_ip"); - update_svc_monitor_addr(monitor_ip, &config_data->svc_monitor_ip); + update_svc_monitor_addr(monitor_ip, &svc_addresses->ip_src); const char *monitor_ip_dst = smap_get(&nb->options, "svc_monitor_ip_dst"); - update_svc_monitor_addr(monitor_ip_dst, - &config_data->svc_monitor_ip_dst); + update_svc_monitor_addr(monitor_ip_dst, &svc_addresses->ip_dst); struct smap *options = &config_data->nb_options; smap_destroy(options); @@ -164,22 +166,22 @@ en_global_config_run(struct engine_node *node , void *data) smap_replace(options, "mac_prefix", mac_addr_prefix); - if (!monitor_mac) { - eth_addr_random(&config_data->svc_monitor_mac_ea); - snprintf(config_data->svc_monitor_mac, - sizeof config_data->svc_monitor_mac, ETH_ADDR_FMT, - ETH_ADDR_ARGS(config_data->svc_monitor_mac_ea)); + if (!src_monitor_mac) { + eth_addr_random(&svc_addresses->mac_ea_src); + snprintf(svc_addresses->mac_src, + sizeof svc_addresses->mac_src, ETH_ADDR_FMT, + ETH_ADDR_ARGS(svc_addresses->mac_ea_src)); smap_replace(options, "svc_monitor_mac", - config_data->svc_monitor_mac); + svc_addresses->mac_src); } if (!dst_monitor_mac) { - eth_addr_random(&config_data->svc_monitor_mac_ea_dst); - snprintf(config_data->svc_monitor_mac_dst, - sizeof config_data->svc_monitor_mac_dst, ETH_ADDR_FMT, - ETH_ADDR_ARGS(config_data->svc_monitor_mac_ea_dst)); + eth_addr_random(&svc_addresses->mac_ea_dst); + snprintf(svc_addresses->mac_dst, + sizeof svc_addresses->mac_dst, ETH_ADDR_FMT, + ETH_ADDR_ARGS(svc_addresses->mac_ea_dst)); smap_replace(options, "svc_monitor_mac_dst", - config_data->svc_monitor_mac_dst); + svc_addresses->mac_dst); } bool ic_vxlan_mode = false; @@ -244,8 +246,8 @@ void en_global_config_cleanup(void *data OVS_UNUSED) struct ed_type_global_config *config_data = data; smap_destroy(&config_data->nb_options); smap_destroy(&config_data->sb_options); - free(config_data->svc_monitor_ip); - free(config_data->svc_monitor_ip_dst); + free(config_data->svc_global_addresses.ip_src); + free(config_data->svc_global_addresses.ip_dst); destroy_debug_config(); } diff --git a/northd/en-global-config.h b/northd/en-global-config.h index 413cd3849..0c75ab246 100644 --- a/northd/en-global-config.h +++ b/northd/en-global-config.h @@ -30,27 +30,29 @@ struct global_config_tracked_data { bool chassis_features_changed; }; -/* struct which maintains the data of the engine node global_config. */ -struct ed_type_global_config { - struct smap nb_options; - struct smap sb_options; - const struct nbrec_nb_global *nb_global; - const struct sbrec_sb_global *sb_global; - +struct svc_monitor_addresses { /* MAC allocated for service monitor usage. Just one pair is allocated * for this purpose and ovn-controller's on each chassis will make use * of this pair when sending out the packets to monitor the services * defined in Service_Monitor Southbound table. Since these packets * are locally handled, having just one pair is good enough. */ - char svc_monitor_mac[ETH_ADDR_STRLEN + 1]; - struct eth_addr svc_monitor_mac_ea; - char svc_monitor_mac_dst[ETH_ADDR_STRLEN + 1]; - struct eth_addr svc_monitor_mac_ea_dst; + char mac_src[ETH_ADDR_STRLEN + 1]; + struct eth_addr mac_ea_src; + char mac_dst[ETH_ADDR_STRLEN + 1]; + struct eth_addr mac_ea_dst; /* IP addresses configured for NF service monitor usage. */ - char *svc_monitor_ip; - char *svc_monitor_ip_dst; + char *ip_src; + char *ip_dst; +}; +/* struct which maintains the data of the engine node global_config. */ +struct ed_type_global_config { + struct smap nb_options; + struct smap sb_options; + const struct nbrec_nb_global *nb_global; + const struct sbrec_sb_global *sb_global; + struct svc_monitor_addresses svc_global_addresses; struct chassis_features features; bool ovn_internal_version_changed; diff --git a/northd/en-lflow.c b/northd/en-lflow.c index 940bd18bd..704fae709 100644 --- a/northd/en-lflow.c +++ b/northd/en-lflow.c @@ -111,7 +111,8 @@ lflow_get_input_data(struct engine_node *node, lflow_input->features = &global_config->features; lflow_input->ovn_internal_version_changed = global_config->ovn_internal_version_changed; - lflow_input->svc_monitor_mac = global_config->svc_monitor_mac; + lflow_input->svc_monitor_mac = + global_config->svc_global_addresses.mac_src; struct ed_type_sampling_app_data *sampling_app_data = engine_get_input_data("sampling_app", node); diff --git a/northd/en-northd.c b/northd/en-northd.c index ef74175a2..d4f49917b 100644 --- a/northd/en-northd.c +++ b/northd/en-northd.c @@ -115,12 +115,8 @@ northd_get_input_data(struct engine_node *node, engine_get_input_data("global_config", node); input_data->nb_options = &global_config->nb_options; input_data->sb_options = &global_config->sb_options; - input_data->svc_monitor_mac = global_config->svc_monitor_mac; - input_data->svc_monitor_mac_ea = global_config->svc_monitor_mac_ea; - input_data->svc_monitor_mac_dst = global_config->svc_monitor_mac_dst; - input_data->svc_monitor_mac_ea_dst = global_config->svc_monitor_mac_ea_dst; - input_data->svc_monitor_ip = global_config->svc_monitor_ip; - input_data->svc_monitor_ip_dst = global_config->svc_monitor_ip_dst; + + input_data->svc_global_addresses = &global_config->svc_global_addresses; input_data->features = &global_config->features; input_data->vxlan_mode = global_config->vxlan_mode; diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c index fbe26eb8c..0b2a85b3b 100644 --- a/northd/en-sync-sb.c +++ b/northd/en-sync-sb.c @@ -106,8 +106,8 @@ en_sync_to_sb_addr_set_run(struct engine_node *node, void *data OVS_UNUSED) nb_port_group_table, sb_address_set_table, &lr_stateful_data->table, &northd_data->lr_datapaths, - global_config->svc_monitor_mac, - global_config->svc_monitor_mac_dst); + global_config->svc_global_addresses.mac_src, + global_config->svc_global_addresses.mac_dst); return EN_UPDATED; } diff --git a/northd/northd.c b/northd/northd.c index 98f97af26..abf38d79f 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -3097,7 +3097,7 @@ create_or_get_service_mon(struct ovsdb_idl_txn *ovnsb_txn, const char *logical_port, const char *logical_input_port, uint16_t service_port, const char *protocol, - const char *chassis_name, bool remote_backend) + bool remote_backend) { struct service_monitor_info *mon_info = get_service_mon(local_svc_monitors_map, @@ -3105,11 +3105,6 @@ create_or_get_service_mon(struct ovsdb_idl_txn *ovnsb_txn, ip, logical_port, service_port, protocol); if (mon_info) { - if (chassis_name && strcmp(mon_info->sbrec_mon->chassis_name, - chassis_name)) { - sbrec_service_monitor_set_chassis_name(mon_info->sbrec_mon, - chassis_name); - } /* * if a similar record was created by the interconet database, * then we transfer ownership rights to delete to northd: @@ -3118,6 +3113,7 @@ create_or_get_service_mon(struct ovsdb_idl_txn *ovnsb_txn, */ sbrec_service_monitor_set_ic_learned(mon_info->sbrec_mon, false); + mon_info->required = true; return mon_info; } @@ -3139,36 +3135,27 @@ create_or_get_service_mon(struct ovsdb_idl_txn *ovnsb_txn, sbrec_service_monitor_set_protocol(sbrec_mon, protocol); sbrec_service_monitor_set_remote(sbrec_mon, remote_backend); sbrec_service_monitor_set_ic_learned(sbrec_mon, false); - if (chassis_name) { - sbrec_service_monitor_set_chassis_name(sbrec_mon, chassis_name); - } + mon_info = xzalloc(sizeof *mon_info); mon_info->sbrec_mon = sbrec_mon; + mon_info->required = true; hmap_insert(local_svc_monitors_map, &mon_info->hmap_node, hash); + return mon_info; } static void ovn_nf_svc_create(struct ovsdb_idl_txn *ovnsb_txn, + const struct nbrec_network_function *nbrec_nf, + const struct svc_monitor_addresses *svc_global_addresses, struct hmap *local_svc_monitors_map, struct hmap *ic_learned_svc_monitors_map, struct sset *svc_monitor_lsps, - struct hmap *ls_ports, - const char *mac_src, const char *mac_dst, - const char *ip_src, const char *ip_dst, - const char *logical_port, const char *logical_input_port, - const struct smap *health_check_options) + struct hmap *ls_ports) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); - if (!ip_src || !ip_dst || !mac_src || !mac_dst) { - VLOG_ERR_RL(&rl, "NetworkFunction: invalid service monitor " - "src_mac: %s dst_mac:%s src_ip:%s dst_ip:%s", - mac_src, mac_dst, ip_src, ip_dst); - return; - } - - const char *ports[] = {logical_port, logical_input_port}; + const char *ports[] = {nbrec_nf->outport->name, nbrec_nf->inport->name}; const char *chassis_name = NULL; bool port_up = true; @@ -3198,51 +3185,55 @@ ovn_nf_svc_create(struct ovsdb_idl_txn *ovnsb_txn, create_or_get_service_mon(ovnsb_txn, local_svc_monitors_map, ic_learned_svc_monitors_map, - "network-function", ip_dst, - logical_port, - logical_input_port, - 0, - "icmp", - chassis_name, - false); - ovs_assert(mon_info); - sbrec_service_monitor_set_options( - mon_info->sbrec_mon, health_check_options); + "network-function", + svc_global_addresses->ip_dst, + nbrec_nf->outport->name, + nbrec_nf->inport->name, + 0, "icmp", false); + + set_service_mon_options(mon_info->sbrec_mon, + &nbrec_nf->health_check->options, NULL); if (!mon_info->sbrec_mon->src_mac || - strcmp(mon_info->sbrec_mon->src_mac, mac_src)) { + strcmp(mon_info->sbrec_mon->src_mac, svc_global_addresses->mac_src)) { sbrec_service_monitor_set_src_mac(mon_info->sbrec_mon, - mac_src); + svc_global_addresses->mac_src); } if (!mon_info->sbrec_mon->mac || - strcmp(mon_info->sbrec_mon->mac, mac_dst)) { + strcmp(mon_info->sbrec_mon->mac, svc_global_addresses->mac_dst)) { sbrec_service_monitor_set_mac(mon_info->sbrec_mon, - mac_dst); + svc_global_addresses->mac_dst); } if (!mon_info->sbrec_mon->src_ip || - strcmp(mon_info->sbrec_mon->src_ip, ip_src)) { - sbrec_service_monitor_set_src_ip(mon_info->sbrec_mon, ip_src); + strcmp(mon_info->sbrec_mon->src_ip, svc_global_addresses->ip_src)) { + sbrec_service_monitor_set_src_ip(mon_info->sbrec_mon, + svc_global_addresses->ip_src); } if (!mon_info->sbrec_mon->ip || - strcmp(mon_info->sbrec_mon->ip, ip_dst)) { - sbrec_service_monitor_set_ip(mon_info->sbrec_mon, ip_dst); + strcmp(mon_info->sbrec_mon->ip, svc_global_addresses->ip_dst)) { + sbrec_service_monitor_set_ip(mon_info->sbrec_mon, + svc_global_addresses->ip_dst); } if (!port_up && mon_info->sbrec_mon->status && !strcmp(mon_info->sbrec_mon->status, "online")) { sbrec_service_monitor_set_status(mon_info->sbrec_mon, "offline"); } - mon_info->required = true; + + if (chassis_name && strcmp(mon_info->sbrec_mon->chassis_name, + chassis_name)) { + sbrec_service_monitor_set_chassis_name(mon_info->sbrec_mon, + chassis_name); + } } static void ovn_lb_svc_create(struct ovsdb_idl_txn *ovnsb_txn, const struct ovn_northd_lb *lb, - const char *svc_monitor_mac, - const struct eth_addr *svc_monitor_mac_ea, + const struct svc_monitor_addresses *svc_global_addresses, struct hmap *ls_ports, struct sset *svc_monitor_lsps, struct hmap *local_svc_monitors_map, @@ -3285,8 +3276,8 @@ ovn_lb_svc_create(struct ovsdb_idl_txn *ovnsb_txn, source_mac_ea = &backend_nb->svc_mon_lrp->lrp_networks.ea; source_mac = backend_nb->svc_mon_lrp->lrp_networks.ea_s; } else { - source_mac_ea = svc_monitor_mac_ea; - source_mac = svc_monitor_mac; + source_mac_ea = &svc_global_addresses->mac_ea_src; + source_mac = svc_global_addresses->mac_src; } const char *protocol = lb->nlb->protocol; @@ -3294,11 +3285,6 @@ ovn_lb_svc_create(struct ovsdb_idl_txn *ovnsb_txn, protocol = "tcp"; } - const char *chassis_name = NULL; - if (!backend_nb->remote_backend && op->sb->chassis) { - chassis_name = op->sb->chassis->name; - } - struct service_monitor_info *mon_info = create_or_get_service_mon(ovnsb_txn, local_svc_monitors_map, @@ -3309,9 +3295,8 @@ ovn_lb_svc_create(struct ovsdb_idl_txn *ovnsb_txn, NULL, backend->port, protocol, - chassis_name, backend_nb->remote_backend); - ovs_assert(mon_info); + set_service_mon_options(mon_info->sbrec_mon, &lb_vip_nb->lb_health_check->options, backend_nb->az_name); @@ -3339,7 +3324,12 @@ ovn_lb_svc_create(struct ovsdb_idl_txn *ovnsb_txn, "offline"); } - mon_info->required = true; + if (!backend_nb->remote_backend && op->sb->chassis && + strcmp(mon_info->sbrec_mon->chassis_name, + op->sb->chassis->name)) { + sbrec_service_monitor_set_chassis_name(mon_info->sbrec_mon, + op->sb->chassis->name); + } } } } @@ -3592,12 +3582,12 @@ build_lb_datapaths(const struct hmap *lbs, const struct hmap *lb_groups, } static void -ovn_lsp_svc_monitors_process_port(struct ovsdb_idl_txn *ovnsb_txn, - const struct ovn_port *op, - const char *svc_monitor_mac, - const struct eth_addr *svc_monitor_mac_ea, - struct hmap *local_svc_monitors_map, - struct sset *svc_monitor_lsps) +ovn_lsp_svc_monitors_process_port( + struct ovsdb_idl_txn *ovnsb_txn, + const struct ovn_port *op, + const struct svc_monitor_addresses *svc_global_addresses, + struct hmap *local_svc_monitors_map, + struct sset *svc_monitor_lsps) { sset_add(svc_monitor_lsps, op->key); @@ -3630,20 +3620,17 @@ ovn_lsp_svc_monitors_process_port(struct ovsdb_idl_txn *ovnsb_txn, NULL, "logical-switch-port", lsp_hc->address, op->key, NULL, lsp_hc->port, lsp_hc->protocol, - (op->sb && op->sb->chassis) ? - op->sb->chassis->name : NULL, false); - mon_info->required = true; set_service_mon_options(mon_info->sbrec_mon, &lsp_hc->options, NULL); struct eth_addr ea; if (!mon_info->sbrec_mon->src_mac || !eth_addr_from_string(mon_info->sbrec_mon->src_mac, &ea) || - !eth_addr_equals(ea, *svc_monitor_mac_ea)) { + !eth_addr_equals(ea, svc_global_addresses->mac_ea_src)) { sbrec_service_monitor_set_src_mac(mon_info->sbrec_mon, - svc_monitor_mac); + svc_global_addresses->mac_src); } if (!mon_info->sbrec_mon->src_ip || @@ -3657,6 +3644,12 @@ ovn_lsp_svc_monitors_process_port(struct ovsdb_idl_txn *ovnsb_txn, !strcmp(mon_info->sbrec_mon->status, "online")) { sbrec_service_monitor_set_status(mon_info->sbrec_mon, "offline"); } + + if (op->sb->chassis && strcmp(mon_info->sbrec_mon->chassis_name, + op->sb->chassis->name)) { + sbrec_service_monitor_set_chassis_name(mon_info->sbrec_mon, + op->sb->chassis->name); + } } } @@ -3664,11 +3657,7 @@ static void build_svc_monitors_data( struct ovsdb_idl_txn *ovnsb_txn, struct ovsdb_idl_index *sbrec_service_monitor_by_learned_type, - const char *svc_monitor_mac, - const struct eth_addr *svc_monitor_mac_ea, - const char *svc_monitor_mac_dst, - const char *svc_monitor_ip, - const char *svc_monitor_ip_dst, + const struct svc_monitor_addresses *svc_global_addresses, struct hmap *ls_ports, struct hmap *lb_dps_map, const struct nbrec_network_function_table *nbrec_network_function_table, struct sset *svc_monitor_lsps, @@ -3700,8 +3689,7 @@ build_svc_monitors_data( struct ovn_lb_datapaths *lb_dps; HMAP_FOR_EACH (lb_dps, hmap_node, lb_dps_map) { ovn_lb_svc_create(ovnsb_txn, lb_dps->lb, - svc_monitor_mac, - svc_monitor_mac_ea, + svc_global_addresses, ls_ports, svc_monitor_lsps, local_svc_monitors_map, @@ -3713,14 +3701,12 @@ build_svc_monitors_data( nbrec_network_function_table) { if (nbrec_nf->health_check) { ovn_nf_svc_create(ovnsb_txn, + nbrec_nf, + svc_global_addresses, local_svc_monitors_map, ic_learned_svc_monitors_map, svc_monitor_lsps, - ls_ports, - svc_monitor_mac, svc_monitor_mac_dst, - svc_monitor_ip, svc_monitor_ip_dst, - nbrec_nf->outport->name, nbrec_nf->inport->name, - &nbrec_nf->health_check->options); + ls_ports); } } @@ -3728,8 +3714,8 @@ build_svc_monitors_data( const struct ovn_port *op; HMAPX_FOR_EACH (hmapx_node, monitored_ports_map) { op = hmapx_node->data; - ovn_lsp_svc_monitors_process_port(ovnsb_txn, op, svc_monitor_mac, - svc_monitor_mac_ea, + ovn_lsp_svc_monitors_process_port(ovnsb_txn, op, + svc_global_addresses, local_svc_monitors_map, svc_monitor_lsps); } @@ -20935,11 +20921,7 @@ ovnnb_db_run(struct northd_input *input_data, &data->lb_group_datapaths_map); build_svc_monitors_data(ovnsb_txn, input_data->sbrec_service_monitor_by_learned_type, - input_data->svc_monitor_mac, - &input_data->svc_monitor_mac_ea, - input_data->svc_monitor_mac_dst, - input_data->svc_monitor_ip, - input_data->svc_monitor_ip_dst, + input_data->svc_global_addresses, &data->ls_ports, &data->lb_datapaths_map, input_data->nbrec_network_function_table, &data->svc_monitor_lsps, &data->local_svc_monitors_map, @@ -20950,7 +20932,7 @@ ovnnb_db_run(struct northd_input *input_data, input_data->nbrec_network_function_group_table, &data->local_svc_monitors_map, input_data->ic_learned_svc_monitors_map, - input_data->svc_monitor_ip_dst); + input_data->svc_global_addresses->ip_dst); build_ipam(&data->ls_datapaths.datapaths); build_lrouter_groups(&data->lr_ports, &data->lr_datapaths); build_ip_mcast(ovnsb_txn, input_data->sbrec_ip_multicast_table, diff --git a/northd/northd.h b/northd/northd.h index cc05c3b1f..8fb3e06be 100644 --- a/northd/northd.h +++ b/northd/northd.h @@ -22,6 +22,7 @@ #include "lib/sset.h" #include "lib/hmapx.h" #include "northd/en-port-group.h" +#include "northd/en-global-config.h" #include "northd/ipam.h" #include "northd/lb.h" #include "openvswitch/hmap.h" @@ -69,12 +70,7 @@ struct northd_input { /* Global config data node inputs. */ const struct smap *nb_options; const struct smap *sb_options; - const char *svc_monitor_mac; - struct eth_addr svc_monitor_mac_ea; - const char *svc_monitor_mac_dst; - struct eth_addr svc_monitor_mac_ea_dst; - char *svc_monitor_ip; - char *svc_monitor_ip_dst; + const struct svc_monitor_addresses *svc_global_addresses; const struct chassis_features *features; bool vxlan_mode; -- 2.48.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
