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?
> 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).
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
