On 6/2/22 21:08, Han Zhou wrote: > On Thu, Jun 2, 2022 at 11:39 AM Dumitru Ceara <[email protected]> wrote: >> >> On 6/2/22 19:22, Han Zhou wrote: >>> On Thu, Jun 2, 2022 at 10:09 AM Mark Michelson <[email protected]> > wrote: >>>> >>>> On 6/2/22 11:22, Dumitru Ceara wrote: >>>>> 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.Load_Balancer record: hairpin_use_ct_mark. >>>>> >>>>> Signed-off-by: Dumitru Ceara <[email protected]> >>>>> --- >>>>> controller/lflow.c | 32 ++++++++++++-------------------- >>>>> controller/lflow.h | 1 - >>>>> controller/ovn-controller.c | 9 --------- >>>>> lib/lb.c | 3 +++ >>>>> lib/lb.h | 4 ++++ >>>>> northd/northd.c | 8 ++++++-- >>>>> ovn-sb.xml | 8 ++++++++ >>>>> tests/ovn-northd.at | 2 +- >>>>> 8 files changed, 34 insertions(+), 33 deletions(-) >>>>> >>>>> diff --git a/controller/lflow.c b/controller/lflow.c >>>>> index 7a3419305..0d9184285 100644 >>>>> --- a/controller/lflow.c >>>>> +++ b/controller/lflow.c >>>>> @@ -1942,7 +1942,6 @@ 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, >>>>> struct ovn_desired_flow_table *flow_table) >>>>> { >>>>> uint64_t stub[1024 / 8]; >>>>> @@ -2033,18 +2032,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 flow 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 (lb->hairpin_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 +2328,6 @@ 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, >>>>> struct ovn_desired_flow_table *flow_table, >>>>> struct simap *ids) >>>>> { >>>>> @@ -2368,7 +2367,6 @@ 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); >>>>> } >>>>> } >>>>> @@ -2383,7 +2381,6 @@ consider_lb_hairpin_flows(const struct >>> sbrec_load_balancer *sbrec_lb, >>>>> 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, >>>>> struct ovn_desired_flow_table *flow_table, >>>>> struct simap *ids, >>>>> struct id_pool *pool) >>>>> @@ -2406,9 +2403,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, >>>>> - flow_table, ids); >>>>> + consider_lb_hairpin_flows(lb, local_datapaths, flow_table, >>> ids); >>>>> } >>>>> } >>>>> >>>>> @@ -2545,7 +2540,6 @@ 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_out->flow_table, >>>>> l_ctx_out->hairpin_lb_ids, >>>>> l_ctx_out->hairpin_id_pool); >>>>> @@ -2709,7 +2703,6 @@ 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_out->flow_table, >>>>> l_ctx_out->hairpin_lb_ids); >>>>> } >>>>> @@ -2840,7 +2833,6 @@ 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_out->flow_table, >>>>> l_ctx_out->hairpin_lb_ids); >>>>> } >>>>> diff --git a/controller/lflow.h b/controller/lflow.h >>>>> index ad9449d3a..2aa896a75 100644 >>>>> --- a/controller/lflow.h >>>>> +++ b/controller/lflow.h >>>>> @@ -160,7 +160,6 @@ 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; >>>>> }; >>>>> >>>>> struct lflow_ctx_out { >>>>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >>>>> index b597c0e37..b9801c42d 100644 >>>>> --- a/controller/ovn-controller.c >>>>> +++ b/controller/ovn-controller.c >>>>> @@ -486,13 +486,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) >>>>> { >>>>> @@ -2529,8 +2522,6 @@ 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_out->flow_table = &fo->flow_table; >>>>> l_ctx_out->group_table = &fo->group_table; >>>>> diff --git a/lib/lb.c b/lib/lb.c >>>>> index 7b0ed1abe..6a48175a7 100644 >>>>> --- a/lib/lb.c >>>>> +++ b/lib/lb.c >>>>> @@ -304,6 +304,9 @@ ovn_controller_lb_create(const struct >>> sbrec_load_balancer *sbrec_lb) >>>>> lb->hairpin_orig_tuple = smap_get_bool(&sbrec_lb->options, >>>>> "hairpin_orig_tuple", >>>>> false); >>>>> + lb->hairpin_use_ct_mark = smap_get_bool(&sbrec_lb->options, >>>>> + "hairpin_use_ct_mark", >>>>> + false); >>>> >>>> For the backports to 22.03 and 21.12, I understand why this would >>>> default to false. However, for "new" versions of OVN (main and 22.06), >>>> why don't we default to true? I know this would cause trouble for the >>>> case of upgrading the central by a major version before updating the >>>> hosts, but I thought the point of these patches was to make sure that >>>> minor release upgrades would not break. >> >> Thanks Mark, Han, for the review! >> >>> >>> I agree with Mark here. Otherwise, we would need to keep the option >>> explicitly forever, which looks quite noisy. >> >> Makes sense. What if I add a third patch that flips the default to >> "true" and we only apply this third patch on main branch, does that >> sound OK to you? > > Sounds good. > >> >>> In addition, I wonder if it is better to just have a global NB option >>> rather than per LB option? A global option looks more clear/obvious and >>> sufficient. >> >> We could try but then we need to add more incremental processing support >> to ovn-controller and have an explicit "load balancer" node that has >> SB_Global as input. Currently LB hairpin flows are added as part of the >> "lflow_output" node processing (or its input change handlers). >> > The I-P code was added for handling northd internal version changes: > engine_add_input(&en_lflow_output, &en_northd_internal_version, NULL); > > I think we don't need an "load balancer" node. Since you removed the > dependency from the northd internal version, the above dependency is not > needed anymore, but instead, we can replace it with a "en_northd_options" > node, which can check any global options (in this case the new option that > matters for LB hairpin), and potentially be extended in the future for > other options changes check, including the northd-internal-version. What do > you think? (this was indeed an option I thought about for that original > change, but ended up using the internal version to avoid introducing a new > option and having a burden to remove it in the future). > > If you think this would impact the release deadline, I am ok with any > approach. It is not a big deal.
Thanks, Han, your suggestion works fine! I posted a v2 addressing all the things we discussed here and on patch 2/2: https://patchwork.ozlabs.org/project/ovn/list/?series=303236&state=* I didn't add your and Mark's ACKs though as I changed a reasonable amount of code, albeit nothing too complex. But it would be nice if you could have another look. In case it looks OK, I prepared backports for all stable branches. I put the links in the cover letter of the v2. Thanks, Dumitru > > Thanks, > Han > >> On the other hand, if we flip the default for hairpin_use_ct_mark to >> "true" in 22.06.0 and make ovn-northd clear it in the SB.Load_Balancer >> table then the "noise" would be gone. This has the "advantage" of less >> I-P code in ovn-controller. >> >> Please let me know what you think. >> >> I can try to spin up a v2 soon or early tomorrow to try not to delay the >> releases too much. >> >> Thanks, >> Dumitru >> >>> >>> Thanks, >>> Han >>> >>>> >>>>> ovn_lb_get_hairpin_snat_ip(&sbrec_lb->header_.uuid, >>> &sbrec_lb->options, >>>>> &lb->hairpin_snat_ips); >>>>> return lb; >>>>> diff --git a/lib/lb.h b/lib/lb.h >>>>> index 832ed31fb..d703c6bf5 100644 >>>>> --- a/lib/lb.h >>>>> +++ b/lib/lb.h >>>>> @@ -101,6 +101,10 @@ struct ovn_controller_lb { >>>>> bool hairpin_orig_tuple; /* True if ovn-northd stores the > original >>>>> * destination tuple in registers. >>>>> */ >>>>> + bool hairpin_use_ct_mark; /* True if ovn-northd uses ct_mark for >>>>> + * load balancer sessions. False if it >>> uses >>>>> + * ct_label. >>>>> + */ >>>>> >>>>> struct lport_addresses hairpin_snat_ips; /* IP (v4 and/or v6) to >>> be used >>>>> * as source for >>> hairpinned >>>>> diff --git a/northd/northd.c b/northd/northd.c >>>>> index 450e05ad6..6e2f2a880 100644 >>>>> --- a/northd/northd.c >>>>> +++ b/northd/northd.c >>>>> @@ -4146,7 +4146,8 @@ build_lb_port_related_data(struct hmap >>> *datapaths, struct hmap *ports, >>>>> */ >>>>> static void >>>>> sync_lbs(struct northd_input *input_data, struct ovsdb_idl_txn >>> *ovnsb_txn, >>>>> - struct hmap *datapaths, struct hmap *lbs) >>>>> + struct hmap *datapaths, struct hmap *lbs, >>>>> + const struct chassis_features *features) >>>>> { >>>>> struct ovn_northd_lb *lb; >>>>> >>>>> @@ -4193,6 +4194,8 @@ sync_lbs(struct northd_input *input_data, struct >>> ovsdb_idl_txn *ovnsb_txn, >>>>> struct smap options; >>>>> smap_clone(&options, &lb->nlb->options); >>>>> smap_replace(&options, "hairpin_orig_tuple", "true"); >>>>> + smap_replace(&options, "hairpin_use_ct_mark", >>>>> + features->ct_lb_mark ? "true" : "false"); >>>>> >>>>> struct sbrec_datapath_binding **lb_dps = >>>>> xmalloc(lb->n_nb_ls * sizeof *lb_dps); >>>>> @@ -15344,7 +15347,8 @@ ovnnb_db_run(struct northd_input *input_data, >>>>> ovn_update_ipv6_options(&data->ports); >>>>> ovn_update_ipv6_prefix(&data->ports); >>>>> >>>>> - sync_lbs(input_data, ovnsb_txn, &data->datapaths, &data->lbs); >>>>> + sync_lbs(input_data, ovnsb_txn, &data->datapaths, &data->lbs, >>>>> + &data->features); >>>>> sync_address_sets(input_data, ovnsb_txn, &data->datapaths); >>>>> sync_port_groups(input_data, ovnsb_txn, &data->port_groups); >>>>> sync_meters(input_data, ovnsb_txn, &data->meter_groups); >>>>> diff --git a/ovn-sb.xml b/ovn-sb.xml >>>>> index 2dc0d5bea..b8b8074ec 100644 >>>>> --- a/ovn-sb.xml >>>>> +++ b/ovn-sb.xml >>>>> @@ -4566,6 +4566,14 @@ tcp.flags = RST; >>>>> of the load balanced packets are stored in registers >>>>> <code>reg1, reg2, xxreg1</code>. >>>>> </column> >>>>> + <column name="options" key="hairpin_use_ct_mark" >>>>> + type='{"type": "boolean"}'> >>>>> + 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 title="Common Columns"> >>>>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >>>>> index 7bb6d33ac..4bb815c7b 100644 >>>>> --- a/tests/ovn-northd.at >>>>> +++ b/tests/ovn-northd.at >>>>> @@ -2563,7 +2563,7 @@ check_column "" sb:datapath_binding >>> load_balancers external_ids:name=sw1 >>>>> echo >>>>> echo "__file__:__line__: Set hairpin_snat_ip on lb1 and check that > SB >>> DB is updated." >>>>> check ovn-nbctl --wait=sb set Load_Balancer lb1 >>> options:hairpin_snat_ip="42.42.42.42 4242::4242" >>>>> -check_column "$lb1_uuid" sb:load_balancer _uuid name=lb1 >>> options='{hairpin_orig_tuple="true", hairpin_snat_ip="42.42.42.42 >>> 4242::4242"}' >>>>> +check_column "$lb1_uuid" sb:load_balancer _uuid name=lb1 >>> options='{hairpin_orig_tuple="true", hairpin_snat_ip="42.42.42.42 >>> 4242::4242", hairpin_use_ct_mark="true"}' >>>>> >>>>> echo >>>>> echo "__file__:__line__: Delete load balancers lb1 and lbg1 and > check >>> that datapath sw1's load_balancers is still empty." >>>>> >>>> >>> >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
