Change the way ovn-controller decides whether it should match on ct_mark.natted or ct_label.natted for hairpin load balancer traffic. Until now this was done solely based on the northd-reported internal version.
However, to cover the case when OVN central components are not the last ones to be updated, ovn-northd now explicitly informs ovn-controller whether it should use ct_mark or ct_label.natted via a new option in the OVN_Southbound.SB_Global record: lb_hairpin_use_ct_mark. Signed-off-by: Dumitru Ceara <dce...@redhat.com> --- controller/lflow.c | 40 ++++++++++---------- controller/lflow.h | 2 + controller/ovn-controller.c | 86 ++++++++++++++++++++----------------------- northd/northd.c | 31 ++++++++++------ ovn-sb.xml | 16 ++++++++ tests/ovn-controller.at | 8 ++-- 6 files changed, 99 insertions(+), 84 deletions(-) diff --git a/controller/lflow.c b/controller/lflow.c index 7a3419305..934b23698 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -1942,7 +1942,7 @@ add_lb_vip_hairpin_flows(struct ovn_controller_lb *lb, struct ovn_lb_vip *lb_vip, struct ovn_lb_backend *lb_backend, uint8_t lb_proto, - bool check_ct_label_for_lb_hairpin, + bool use_ct_mark, struct ovn_desired_flow_table *flow_table) { uint64_t stub[1024 / 8]; @@ -2033,18 +2033,19 @@ add_lb_vip_hairpin_flows(struct ovn_controller_lb *lb, * - packets must have ip.src == ip.dst at this point. * - the destination protocol and port must be of a valid backend that * has the same IP as ip.dst. + * + * During upgrades logical flows might still use the old way of storing + * ct.natted in ct_label. For backwards compatibility, only use ct_mark + * if ovn-northd notified ovn-controller to do that. */ - uint32_t lb_ct_mark = OVN_CT_NATTED; - match_set_ct_mark_masked(&hairpin_match, lb_ct_mark, lb_ct_mark); - - ofctrl_add_flow(flow_table, OFTABLE_CHK_LB_HAIRPIN, 100, - lb->slb->header_.uuid.parts[0], &hairpin_match, - &ofpacts, &lb->slb->header_.uuid); + if (use_ct_mark) { + uint32_t lb_ct_mark = OVN_CT_NATTED; + match_set_ct_mark_masked(&hairpin_match, lb_ct_mark, lb_ct_mark); - /* The below flow is identical to the above except that it checks - * ct_label.natted instead of ct_mark.natted, for backward compatibility - * during the upgrade from a previous version that uses ct_label. */ - if (check_ct_label_for_lb_hairpin) { + ofctrl_add_flow(flow_table, OFTABLE_CHK_LB_HAIRPIN, 100, + lb->slb->header_.uuid.parts[0], &hairpin_match, + &ofpacts, &lb->slb->header_.uuid); + } else { match_set_ct_mark_masked(&hairpin_match, 0, 0); ovs_u128 lb_ct_label = { .u64.lo = OVN_CT_NATTED, @@ -2328,7 +2329,7 @@ add_lb_ct_snat_hairpin_flows(struct ovn_controller_lb *lb, static void consider_lb_hairpin_flows(const struct sbrec_load_balancer *sbrec_lb, const struct hmap *local_datapaths, - bool check_ct_label_for_lb_hairpin, + bool use_ct_mark, struct ovn_desired_flow_table *flow_table, struct simap *ids) { @@ -2368,8 +2369,7 @@ consider_lb_hairpin_flows(const struct sbrec_load_balancer *sbrec_lb, struct ovn_lb_backend *lb_backend = &lb_vip->backends[j]; add_lb_vip_hairpin_flows(lb, lb_vip, lb_backend, lb_proto, - check_ct_label_for_lb_hairpin, - flow_table); + use_ct_mark, flow_table); } } @@ -2382,8 +2382,7 @@ consider_lb_hairpin_flows(const struct sbrec_load_balancer *sbrec_lb, * backends to handle the load balanced hairpin traffic. */ static void add_lb_hairpin_flows(const struct sbrec_load_balancer_table *lb_table, - const struct hmap *local_datapaths, - bool check_ct_label_for_lb_hairpin, + const struct hmap *local_datapaths, bool use_ct_mark, struct ovn_desired_flow_table *flow_table, struct simap *ids, struct id_pool *pool) @@ -2406,8 +2405,7 @@ add_lb_hairpin_flows(const struct sbrec_load_balancer_table *lb_table, ovs_assert(id_pool_alloc_id(pool, &id)); simap_put(ids, lb->name, id); } - consider_lb_hairpin_flows(lb, local_datapaths, - check_ct_label_for_lb_hairpin, + consider_lb_hairpin_flows(lb, local_datapaths, use_ct_mark, flow_table, ids); } } @@ -2545,7 +2543,7 @@ lflow_run(struct lflow_ctx_in *l_ctx_in, struct lflow_ctx_out *l_ctx_out) l_ctx_in->local_datapaths, l_ctx_out->flow_table); add_lb_hairpin_flows(l_ctx_in->lb_table, l_ctx_in->local_datapaths, - l_ctx_in->check_ct_label_for_lb_hairpin, + l_ctx_in->lb_hairpin_use_ct_mark, l_ctx_out->flow_table, l_ctx_out->hairpin_lb_ids, l_ctx_out->hairpin_id_pool); @@ -2709,7 +2707,7 @@ lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp, * associated. */ for (size_t i = 0; i < n_dp_lbs; i++) { consider_lb_hairpin_flows(dp_lbs[i], l_ctx_in->local_datapaths, - l_ctx_in->check_ct_label_for_lb_hairpin, + l_ctx_in->lb_hairpin_use_ct_mark, l_ctx_out->flow_table, l_ctx_out->hairpin_lb_ids); } @@ -2840,7 +2838,7 @@ lflow_handle_changed_lbs(struct lflow_ctx_in *l_ctx_in, VLOG_DBG("Add load balancer hairpin flows for "UUID_FMT, UUID_ARGS(&lb->header_.uuid)); consider_lb_hairpin_flows(lb, l_ctx_in->local_datapaths, - l_ctx_in->check_ct_label_for_lb_hairpin, + l_ctx_in->lb_hairpin_use_ct_mark, l_ctx_out->flow_table, l_ctx_out->hairpin_lb_ids); } diff --git a/controller/lflow.h b/controller/lflow.h index ad9449d3a..342967917 100644 --- a/controller/lflow.h +++ b/controller/lflow.h @@ -160,7 +160,7 @@ struct lflow_ctx_in { const struct sset *related_lport_ids; const struct shash *binding_lports; const struct hmap *chassis_tunnels; - bool check_ct_label_for_lb_hairpin; + bool lb_hairpin_use_ct_mark; }; struct lflow_ctx_out { diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index b597c0e37..4bb33b1ca 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -131,6 +131,9 @@ static const char *ssl_ca_cert_file; #define DEFAULT_LFLOW_CACHE_WMARK_PERC 50 #define DEFAULT_LFLOW_CACHE_TRIM_TO_MS 30000 +/* SB Global options defaults. */ +#define DEFAULT_SB_GLOBAL_LB_HAIRPIN_USE_CT_MARK false + struct controller_engine_ctx { struct lflow_cache *lflow_cache; struct if_status_mgr *if_mgr; @@ -486,13 +489,6 @@ get_ovs_chassis_id(const struct ovsrec_open_vswitch_table *ovs_table) return chassis_id; } -static bool -get_check_ct_label_for_lb_hairpin(const char *northd_internal_ver) -{ - unsigned int minor = ovn_parse_internal_version_minor(northd_internal_ver); - return (minor <= 3); -} - static void update_ssl_config(const struct ovsrec_ssl_table *ssl_table) { @@ -2292,60 +2288,58 @@ non_vif_data_ovs_iface_handler(struct engine_node *node, void *data OVS_UNUSED) return local_nonvif_data_handle_ovs_iface_changes(iface_table); } -struct ed_type_northd_internal_version { - char *ver; +struct ed_type_northd_options { + bool lb_hairpin_use_ct_mark; }; static void * -en_northd_internal_version_init(struct engine_node *node OVS_UNUSED, - struct engine_arg *arg OVS_UNUSED) +en_northd_options_init(struct engine_node *node OVS_UNUSED, + struct engine_arg *arg OVS_UNUSED) { - struct ed_type_northd_internal_version *n_ver = xzalloc(sizeof *n_ver); - n_ver->ver = xstrdup(""); - return n_ver; + struct ed_type_northd_options *n_opts = xzalloc(sizeof *n_opts); + return n_opts; } static void -en_northd_internal_version_cleanup(void *data) +en_northd_options_cleanup(void *data OVS_UNUSED) { - struct ed_type_northd_internal_version *n_ver = data; - free(n_ver->ver); } static void -en_northd_internal_version_run(struct engine_node *node, void *data) +en_northd_options_run(struct engine_node *node, void *data) { - struct ed_type_northd_internal_version *n_ver = data; - struct sbrec_sb_global_table *sb_global_table = - (struct sbrec_sb_global_table *)EN_OVSDB_GET( - engine_get_input("SB_sb_global", node)); + struct ed_type_northd_options *n_opts = data; + const struct sbrec_sb_global_table *sb_global_table = + EN_OVSDB_GET(engine_get_input("SB_sb_global", node)); const struct sbrec_sb_global *sb_global = sbrec_sb_global_table_first(sb_global_table); - free(n_ver->ver); - n_ver->ver = - xstrdup(sb_global ? smap_get_def(&sb_global->options, - "northd_internal_version", "") : ""); + + n_opts->lb_hairpin_use_ct_mark = + sb_global + ? smap_get_bool(&sb_global->options, "lb_hairpin_use_ct_mark", + DEFAULT_SB_GLOBAL_LB_HAIRPIN_USE_CT_MARK) + : DEFAULT_SB_GLOBAL_LB_HAIRPIN_USE_CT_MARK; engine_set_node_state(node, EN_UPDATED); } static bool -en_northd_internal_version_sb_sb_global_handler(struct engine_node *node, - void *data) +en_northd_options_sb_sb_global_handler(struct engine_node *node, void *data) { - struct ed_type_northd_internal_version *n_ver = data; - struct sbrec_sb_global_table *sb_global_table = - (struct sbrec_sb_global_table *)EN_OVSDB_GET( - engine_get_input("SB_sb_global", node)); + struct ed_type_northd_options *n_opts = data; + const struct sbrec_sb_global_table *sb_global_table = + EN_OVSDB_GET(engine_get_input("SB_sb_global", node)); const struct sbrec_sb_global *sb_global = sbrec_sb_global_table_first(sb_global_table); - const char *new_ver = - sb_global ? smap_get_def(&sb_global->options, - "northd_internal_version", "") : ""; - if (strcmp(new_ver, n_ver->ver)) { - free(n_ver->ver); - n_ver->ver = xstrdup(new_ver); + bool lb_hairpin_use_ct_mark = + sb_global + ? smap_get_bool(&sb_global->options, "lb_hairpin_use_ct_mark", + DEFAULT_SB_GLOBAL_LB_HAIRPIN_USE_CT_MARK) + : DEFAULT_SB_GLOBAL_LB_HAIRPIN_USE_CT_MARK; + + if (lb_hairpin_use_ct_mark != n_opts->lb_hairpin_use_ct_mark) { + n_opts->lb_hairpin_use_ct_mark = lb_hairpin_use_ct_mark; engine_set_node_state(node, EN_UPDATED); } return true; @@ -2496,9 +2490,8 @@ init_lflow_ctx(struct engine_node *node, engine_get_input_data("port_groups", node); struct shash *port_groups = &pg_data->port_groups_cs_local; - struct ed_type_northd_internal_version *n_ver = - engine_get_input_data("northd_internal_version", node); - ovs_assert(n_ver); + struct ed_type_northd_options *n_opts = + engine_get_input_data("northd_options", node); l_ctx_in->sbrec_multicast_group_by_name_datapath = sbrec_mc_group_by_name_dp; @@ -2529,8 +2522,7 @@ init_lflow_ctx(struct engine_node *node, l_ctx_in->related_lport_ids = &rt_data->related_lports.lport_ids; l_ctx_in->binding_lports = &rt_data->lbinding_data.lports; l_ctx_in->chassis_tunnels = &non_vif_data->chassis_tunnels; - l_ctx_in->check_ct_label_for_lb_hairpin = - get_check_ct_label_for_lb_hairpin(n_ver->ver); + l_ctx_in->lb_hairpin_use_ct_mark = n_opts->lb_hairpin_use_ct_mark; l_ctx_out->flow_table = &fo->flow_table; l_ctx_out->group_table = &fo->group_table; @@ -3458,7 +3450,7 @@ main(int argc, char *argv[]) ENGINE_NODE(flow_output, "flow_output"); ENGINE_NODE_WITH_CLEAR_TRACK_DATA(addr_sets, "addr_sets"); ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_groups, "port_groups"); - ENGINE_NODE(northd_internal_version, "northd_internal_version"); + ENGINE_NODE(northd_options, "northd_options"); #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR); SB_NODES @@ -3507,10 +3499,10 @@ main(int argc, char *argv[]) engine_add_input(&en_pflow_output, &en_ovs_open_vswitch, NULL); engine_add_input(&en_pflow_output, &en_ovs_bridge, NULL); - engine_add_input(&en_northd_internal_version, &en_sb_sb_global, - en_northd_internal_version_sb_sb_global_handler); + engine_add_input(&en_northd_options, &en_sb_sb_global, + en_northd_options_sb_sb_global_handler); - engine_add_input(&en_lflow_output, &en_northd_internal_version, NULL); + engine_add_input(&en_lflow_output, &en_northd_options, NULL); /* Keep en_addr_sets before en_runtime_data because * lflow_output_runtime_data_handler may *partially* reprocess a lflow when diff --git a/northd/northd.c b/northd/northd.c index 450e05ad6..3b3900cea 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -15252,15 +15252,6 @@ ovnnb_db_run(struct northd_input *input_data, if (!nb) { nb = nbrec_nb_global_insert(ovnnb_txn); } - const struct sbrec_sb_global *sb = sbrec_sb_global_table_first( - input_data->sbrec_sb_global_table); - if (!sb) { - sb = sbrec_sb_global_insert(ovnsb_txn); - } - if (nb->ipsec != sb->ipsec) { - sbrec_sb_global_set_ipsec(sb, nb->ipsec); - } - sbrec_sb_global_set_options(sb, &nb->options); const char *mac_addr_prefix = set_mac_prefix(smap_get(&nb->options, "mac_prefix")); @@ -15306,8 +15297,6 @@ ovnnb_db_run(struct northd_input *input_data, nbrec_nb_global_set_options(nb, &options); } - smap_destroy(&options); - bool old_use_ldp = use_logical_dp_groups; use_logical_dp_groups = smap_get_bool(&nb->options, "use_logical_dp_groups", true); @@ -15351,6 +15340,26 @@ ovnnb_db_run(struct northd_input *input_data, sync_dns_entries(input_data, ovnsb_txn, &data->datapaths); cleanup_stale_fdb_entries(input_data, &data->datapaths); stopwatch_stop(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec()); + + /* Set up SB_Global (depends on chassis features). */ + const struct sbrec_sb_global *sb = sbrec_sb_global_table_first( + input_data->sbrec_sb_global_table); + if (!sb) { + sb = sbrec_sb_global_insert(ovnsb_txn); + } + if (nb->ipsec != sb->ipsec) { + sbrec_sb_global_set_ipsec(sb, nb->ipsec); + } + + /* Inform ovn-controllers whether LB flows will use ct_mark + * (i.e., only if all chassis support it). + */ + smap_replace(&options, "lb_hairpin_use_ct_mark", + data->features.ct_lb_mark ? "true" : "false"); + if (!smap_equal(&sb->options, &options)) { + sbrec_sb_global_set_options(sb, &options); + } + smap_destroy(&options); } /* Stores the list of chassis which references an ha_chassis_group. diff --git a/ovn-sb.xml b/ovn-sb.xml index 2dc0d5bea..529e3ee7a 100644 --- a/ovn-sb.xml +++ b/ovn-sb.xml @@ -199,6 +199,22 @@ tunnel interfaces. </column> </group> + + <group title="Options for configuring Load Balancers"> + <p> + These options apply when <code>ovn-controller</code> configures + load balancer related flows. + </p> + + <column name="options" key="lb_hairpin_use_ct_mark"> + This value is automatically set to <code>true</code> by + <code>ovn-northd</code> when action <code>ct_lb_mark</code> is used + for new load balancer sessions. <code>ovn-controller</code> then + knows that it should check <code>ct_mark.natted</code> to detect + load balanced traffic. + </column> + </group> + </group> <group title="Connection Options"> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index 71463187b..b8db342b9 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -2058,7 +2058,7 @@ AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [2 OVN_CLEANUP([hv1]) AT_CLEANUP -AT_SETUP([ovn-controller - I-P handle northd_internal_version change]) +AT_SETUP([ovn-controller - I-P handle lb_hairpin_use_ct_mark change]) ovn_start --backup-northd=none @@ -2089,8 +2089,8 @@ lflow_run_new=$(read_counter lflow_run) AT_CHECK([echo $(($lflow_run_new - $lflow_run_old))], [0], [0 ]) -# northd_internal_version update in sb_global:options should trigger lflow_run. -# The below steps should cause northd_internal_version change twice. One by +# lb_hairpin_use_ct_mark update in sb_global:options should trigger lflow_run. +# The below steps should cause lb_hairpin_use_ct_mark change twice. One by # ovn-sbctl, and the other by ovn-northd to change it back. # In some cases, both changes are catched by ovn-controller in the same run, @@ -2098,7 +2098,7 @@ AT_CHECK([echo $(($lflow_run_new - $lflow_run_old))], [0], [0 OVS_WAIT_UNTIL([ lflow_run_old=$(read_counter lflow_run) - check ovn-sbctl set SB_Global . options:northd_internal_version=foo + check ovn-sbctl set SB_Global . options:lb_hairpin_use_ct_mark=false check ovn-nbctl --wait=hv sync lflow_run_new=$(read_counter lflow_run) test x"$(($lflow_run_new - $lflow_run_old))" = x2 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev