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.
I agree with Mark here. Otherwise, we would need to keep the option
explicitly forever, which looks quite noisy.
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.
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
