On Thu, Jun 2, 2022 at 7:43 AM Dumitru Ceara <[email protected]> wrote:
>
> On 6/2/22 16:35, Han Zhou wrote:
> > On Thu, Jun 2, 2022 at 5:07 AM Dumitru Ceara <[email protected]> wrote:
> >>
> >> On 6/2/22 04:08, Han Zhou wrote:
> >>> On Wed, Jun 1, 2022 at 11:44 AM Mark Michelson <[email protected]>
> > wrote:
> >>>>
> >>>> On 6/1/22 13:44, Numan Siddique wrote:
> >>>>> On Wed, Jun 1, 2022 at 12:52 PM Mark Michelson <[email protected]>
> >>> wrote:
> >>>>>>
> >>>>>> Thanks Dumitru. This looks good to me.
> >>>>>>
> >>>>>> Acked-by: Mark Michelson <[email protected]>
> >>>>>
> >>>>> Thanks Dumitru.
> >>>>> I applied this patch to main and branch-22.06.
> >>>>>
> >>>>> Looks like it's not applying cleanly to branch-22.03.
> >>>>>
> >>>>> Numan
> >>>>
> >>>> I pushed the changes to 21.12 and 22.03.
> >>>>
> >>> Sorry that I just noticed this change. The original ct_lb_mark change
> >>> didn't consider the order that ovn-northd upgrades before
> > ovn-controller.
> >>> However, I think this change would cause another problem for this
> > upgrade
> >>> order.
> >>
> >> You're right, I forgot about this case.
> >>
> >>> Due to the direct usage of ct.label (rather than through an ovn action
> > from
> >>> logical flows) in ovn-controller generated LB hair-pin flows, there
was
> >>> already a compatibility issue between ovn-northd and ovn-controller
> > during
> >>> upgrade (pointed out by Numan during code review of the ct_lb_mark
> > change),
> >>> and that patch took care of the incompatibility by checking ovn-northd
> >>> version string in ovn-controller:
> >>>
> >>> 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);
> >>> }
> >>>
> >>> With this check, ovn-controller will decide to use ct.label or ct.mark
> > for
> >>> the LB hair-pin flows according to ovn-northd version, with the
> > assumption
> >>> that ovn-controller is upgraded before ovn-northd. But it didn't
support
> >>> the use case when ovn-northd is upgraded first.
> >>> Now with this change, if ovn-northd is upgraded before ovn-controller,
> > any
> >>> upgraded ovn-controller would expect the new ovn-northd using the
> >>> ct_lb_mark, which would be a problem for the hair-pin flows until all
> > the
> >>> ovn-controllers are upgraded.
> >>
> >> Sure but before the patch any ovn-northd-before-ovn-controller upgrade
> >> would have broken all load balancer use cases. Now we handle most of
> >> the traffic patterns, except hairpin.
> >>
> >> I'm working on a patch to handle the hairpin case when ovn-northd is
> >> upgraded before all ovn-controllers are upgraded.
> >>
> >> While doing that I also noticed we need to use a similar approach for
> >> ct_mark.blocked and ct_mark.ecmp_reply_port.
> >>
> >> I'll post a series to cover all this.
> >>
> > Hi Dumitru,
> >
>
> Hi Han,
>
> > Thanks for working on this. I forgot to mention another problem that the
> > fix that fixes upgrading problems may cause new upgrading problems for
the
> > users that have already upgraded to the last version.
> > Assume both northd and ovn-controller are using the latest (ct mark)
> > version of the code already before this patch, now they are upgrading to
> > the version of this patch:
> >
> > 1. If northd is upgraded first: because not all ovn-controllers are
> > upgraded, so it will use ct_lb instead of ct_lb_mark, causing a new
> > incompatibility scenario for the hair-pin flows
> > 2. If ovn-controller is upgraded first: no problem
> >
> > Maybe there are not many users that would encounter this, since the ct
mark
> > change was backported quite recently, but please take this into
> > consideration (for both upgrading orders) if you attempt to address the
> > rest of the upgrading problems for this.
>
> I don't really see an easy way to fix this scenario. However, I don't
> think this is a big concern. There's no official release including the
> ct_mark change yet.
>
I totally agree this is hard to handle.
> In my opinion things are OK if upgrades work fine (regardless of order
> of components) from version A.X to version A.Y where:
> - "A.X" is a stable release before ct_mark was used.
> - "A.Y" is the most recent minor release of stable branch "A".
>
> So as long as we fix this before the next set of stable releases I think
> we're fine.
>
I am not too concerned with the current patch, but just wanted to remind
that this needs to be considered when you work on a new fix - either fix it
at the same minor version where the ct mark change is introduced, or will
need to consider even more complex scenarios to avoid new issues (in which
case I'd rather not fixing anything other than just some documentation
about the upgrade order required).
Thanks,
Han
> Regarding users: out of the 3 CMSs that we use downstream 2 already
> ensure that ovn-controllers are upgraded before ovn-northd and DBs
> (OpenStack and OpenShift). For RHEV that's not the case but AFAIK RHEV
> doesn't need the logical switch Load_Balancer hairpin use case.
>
> Thanks,
> Dumitru
>
> >
> > Thanks,
> > Han
> >
> >>>
> >>> Thanks,
> >>> Han
> >>
> >> Thanks,
> >> Dumitru
> >>
> >>>>>
> >>>>>
> >>>>>
> >>>>>>
> >>>>>> On 6/1/22 10:36, Dumitru Ceara wrote:
> >>>>>>> Commit a075230e4a0f ("Use ct_mark for masked access to make flows
> >>>>>>> HW-offloading friendly.") started using the ct_lb_mark action
> > instead
> >>> of
> >>>>>>> ct_lb. In usual scenarios this new feature would be picked up
when
> >>> the
> >>>>>>> next stable release becomes available (i.e., 22.06.0). However,
the
> >>>>>>> commit was also backported to stable branches (branch-22.03 and
> >>>>>>> branch-21.12).
> >>>>>>>
> >>>>>>> While the supported upgrade scenario for OVN when moving to a new
> >>>>>>> stable release is to ensure that ovn-controllers are upgraded
first,
> >>>>>>> it's not really clear that this restriction applies to "z-stream"
> >>>>>>> upgrades too (e.g., from v21.12.1 to v21.12.2).
> >>>>>>>
> >>>>>>> Some CMSs, like RHEV (Red Hat Virtualization), expect
> > ovn-controllers
> >>>>>>> running older code from a stable branch to be able to interpret
all
> >>>>>>> Southbound contents generated by ovn-northd instances built from
the
> >>>>>>> same or newer versions of the stable branch.
> >>>>>>>
> >>>>>>> Ensure that ct_lb_mark is used only when all chassis registered in
> > the
> >>>>>>> Southbound support it.
> >>>>>>>
> >>>>>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2091565
> >>>>>>> Signed-off-by: Dumitru Ceara <[email protected]>
> >>>>>>> ---
> >>>>>>> NOTE: If accepted, this patch should be backported to branch-22.03
> > and
> >>>>>>> branch-21.12. The alternative is to revert a075230e4a0f and its
> >>>>>>> dependents from all stable branches.
> >>>>>>> ---
> >>>>>>> controller/chassis.c | 6 +++
> >>>>>>> include/ovn/features.h | 1 +
> >>>>>>> northd/en-lflow.c | 1 +
> >>>>>>> northd/northd.c | 112
> >>> ++++++++++++++++++++++++++++-------------
> >>>>>>> northd/northd.h | 6 +++
> >>>>>>> tests/ovn-northd.at | 69 ++++++++++++++++++++++++-
> >>>>>>> 6 files changed, 159 insertions(+), 36 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/controller/chassis.c b/controller/chassis.c
> >>>>>>> index 8a1559653..239658461 100644
> >>>>>>> --- a/controller/chassis.c
> >>>>>>> +++ b/controller/chassis.c
> >>>>>>> @@ -350,6 +350,7 @@ chassis_build_other_config(const struct
> >>> ovs_chassis_cfg *ovs_cfg,
> >>>>>>> smap_replace(config, "is-interconn",
> >>>>>>> ovs_cfg->is_interconn ? "true" : "false");
> >>>>>>> smap_replace(config, OVN_FEATURE_PORT_UP_NOTIF, "true");
> >>>>>>> + smap_replace(config, OVN_FEATURE_CT_LB_MARK, "true");
> >>>>>>> }
> >>>>>>>
> >>>>>>> /*
> >>>>>>> @@ -455,6 +456,11 @@ chassis_other_config_changed(const struct
> >>> ovs_chassis_cfg *ovs_cfg,
> >>>>>>> return true;
> >>>>>>> }
> >>>>>>>
> >>>>>>> + if (!smap_get_bool(&chassis_rec->other_config,
> >>> OVN_FEATURE_CT_LB_MARK,
> >>>>>>> + false)) {
> >>>>>>> + return true;
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> return false;
> >>>>>>> }
> >>>>>>>
> >>>>>>> diff --git a/include/ovn/features.h b/include/ovn/features.h
> >>>>>>> index d12a8eb0d..09f002287 100644
> >>>>>>> --- a/include/ovn/features.h
> >>>>>>> +++ b/include/ovn/features.h
> >>>>>>> @@ -22,6 +22,7 @@
> >>>>>>>
> >>>>>>> /* ovn-controller supported feature names. */
> >>>>>>> #define OVN_FEATURE_PORT_UP_NOTIF "port-up-notif"
> >>>>>>> +#define OVN_FEATURE_CT_LB_MARK "ct-lb-mark"
> >>>>>>>
> >>>>>>> /* OVS datapath supported features. Based on availability OVN
> >>> might generate
> >>>>>>> * different types of openflows.
> >>>>>>> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> >>>>>>> index ffbdaf4e8..fa0dfcbe0 100644
> >>>>>>> --- a/northd/en-lflow.c
> >>>>>>> +++ b/northd/en-lflow.c
> >>>>>>> @@ -60,6 +60,7 @@ void en_lflow_run(struct engine_node *node, void
> >>> *data OVS_UNUSED)
> >>>>>>> lflow_input.meter_groups = &northd_data->meter_groups;
> >>>>>>> lflow_input.lbs = &northd_data->lbs;
> >>>>>>> lflow_input.bfd_connections =
&northd_data->bfd_connections;
> >>>>>>> + lflow_input.features = &northd_data->features;
> >>>>>>> lflow_input.ovn_internal_version_changed =
> >>>>>>>
northd_data->ovn_internal_version_changed;
> >>>>>>>
> >>>>>>> diff --git a/northd/northd.c b/northd/northd.c
> >>>>>>> index 51dec36b3..511ef6376 100644
> >>>>>>> --- a/northd/northd.c
> >>>>>>> +++ b/northd/northd.c
> >>>>>>> @@ -398,6 +398,22 @@ ovn_stage_to_datapath_type(enum ovn_stage
> > stage)
> >>>>>>> }
> >>>>>>> }
> >>>>>>>
> >>>>>>> +static void
> >>>>>>> +build_chassis_features(const struct northd_input *input_data,
> >>>>>>> + struct chassis_features *chassis_features)
> >>>>>>> +{
> >>>>>>> + const struct sbrec_chassis *chassis;
> >>>>>>> +
> >>>>>>> + SBREC_CHASSIS_TABLE_FOR_EACH (chassis,
> >>> input_data->sbrec_chassis) {
> >>>>>>> + if (!smap_get_bool(&chassis->other_config,
> >>> OVN_FEATURE_CT_LB_MARK,
> >>>>>>> + false)) {
> >>>>>>> + chassis_features->ct_lb_mark = false;
> >>>>>>> + return;
> >>>>>>> + }
> >>>>>>> + }
> >>>>>>> + chassis_features->ct_lb_mark = true;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> struct ovn_chassis_qdisc_queues {
> >>>>>>> struct hmap_node key_node;
> >>>>>>> uint32_t queue_id;
> >>>>>>> @@ -3803,12 +3819,13 @@ static bool
> >>>>>>> build_lb_vip_actions(struct ovn_lb_vip *lb_vip,
> >>>>>>> struct ovn_northd_lb_vip *lb_vip_nb,
> >>>>>>> struct ds *action, char *selection_fields,
> >>>>>>> - bool ls_dp)
> >>>>>>> + bool ls_dp, bool ct_lb_mark)
> >>>>>>> {
> >>>>>>> + const char *ct_lb_action = ct_lb_mark ? "ct_lb_mark" :
"ct_lb";
> >>>>>>> bool skip_hash_fields = false, reject = false;
> >>>>>>>
> >>>>>>> if (lb_vip_nb->lb_health_check) {
> >>>>>>> - ds_put_cstr(action, "ct_lb_mark(backends=");
> >>>>>>> + ds_put_format(action, "%s(backends=", ct_lb_action);
> >>>>>>>
> >>>>>>> size_t n_active_backends = 0;
> >>>>>>> for (size_t i = 0; i < lb_vip->n_backends; i++) {
> >>>>>>> @@ -3841,7 +3858,7 @@ build_lb_vip_actions(struct ovn_lb_vip
> > *lb_vip,
> >>>>>>> } else if (lb_vip->empty_backend_rej &&
!lb_vip->n_backends)
> > {
> >>>>>>> reject = true;
> >>>>>>> } else {
> >>>>>>> - ds_put_format(action, "ct_lb_mark(backends=%s);",
> >>>>>>> + ds_put_format(action, "%s(backends=%s);", ct_lb_action,
> >>>>>>> lb_vip_nb->backend_ips);
> >>>>>>> }
> >>>>>>>
> >>>>>>> @@ -5768,13 +5785,16 @@ build_pre_lb(struct ovn_datapath *od,
const
> >>> struct shash *meter_groups,
> >>>>>>> }
> >>>>>>>
> >>>>>>> static void
> >>>>>>> -build_pre_stateful(struct ovn_datapath *od, struct hmap *lflows)
> >>>>>>> +build_pre_stateful(struct ovn_datapath *od,
> >>>>>>> + const struct chassis_features *features,
> >>>>>>> + struct hmap *lflows)
> >>>>>>> {
> >>>>>>> /* Ingress and Egress pre-stateful Table (Priority 0):
> > Packets
> >>> are
> >>>>>>> * allowed by default. */
> >>>>>>> ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_STATEFUL, 0, "1",
> >>> "next;");
> >>>>>>> ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_STATEFUL, 0,
"1",
> >>> "next;");
> >>>>>>>
> >>>>>>> + const char *ct_lb_action = features->ct_lb_mark ?
"ct_lb_mark"
> > :
> >>> "ct_lb";
> >>>>>>> const char *lb_protocols[] = {"tcp", "udp", "sctp"};
> >>>>>>> struct ds actions = DS_EMPTY_INITIALIZER;
> >>>>>>> struct ds match = DS_EMPTY_INITIALIZER;
> >>>>>>> @@ -5785,8 +5805,8 @@ build_pre_stateful(struct ovn_datapath *od,
> >>> struct hmap *lflows)
> >>>>>>> ds_put_format(&match, REGBIT_CONNTRACK_NAT" == 1 && ip4
> > &&
> >>> %s",
> >>>>>>> lb_protocols[i]);
> >>>>>>> ds_put_format(&actions, REG_ORIG_DIP_IPV4 " = ip4.dst;
"
> >>>>>>> - REG_ORIG_TP_DPORT " = %s.dst;
> >>> ct_lb_mark;",
> >>>>>>> - lb_protocols[i]);
> >>>>>>> + REG_ORIG_TP_DPORT " = %s.dst;
%s;",
> >>>>>>> + lb_protocols[i], ct_lb_action);
> >>>>>>> ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_STATEFUL,
120,
> >>>>>>> ds_cstr(&match), ds_cstr(&actions));
> >>>>>>>
> >>>>>>> @@ -5795,20 +5815,20 @@ build_pre_stateful(struct ovn_datapath
*od,
> >>> struct hmap *lflows)
> >>>>>>> ds_put_format(&match, REGBIT_CONNTRACK_NAT" == 1 && ip6
> > &&
> >>> %s",
> >>>>>>> lb_protocols[i]);
> >>>>>>> ds_put_format(&actions, REG_ORIG_DIP_IPV6 " = ip6.dst;
"
> >>>>>>> - REG_ORIG_TP_DPORT " = %s.dst;
> >>> ct_lb_mark;",
> >>>>>>> - lb_protocols[i]);
> >>>>>>> + REG_ORIG_TP_DPORT " = %s.dst;
%s;",
> >>>>>>> + lb_protocols[i], ct_lb_action);
> >>>>>>> ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_STATEFUL,
120,
> >>>>>>> ds_cstr(&match), ds_cstr(&actions));
> >>>>>>> }
> >>>>>>>
> >>>>>>> - ds_destroy(&actions);
> >>>>>>> - ds_destroy(&match);
> >>>>>>> + ds_clear(&actions);
> >>>>>>> + ds_put_format(&actions, "%s;", ct_lb_action);
> >>>>>>>
> >>>>>>> ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_STATEFUL, 110,
> >>>>>>> - REGBIT_CONNTRACK_NAT" == 1", "ct_lb_mark;");
> >>>>>>> + REGBIT_CONNTRACK_NAT" == 1",
ds_cstr(&actions));
> >>>>>>>
> >>>>>>> ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_STATEFUL, 110,
> >>>>>>> - REGBIT_CONNTRACK_NAT" == 1", "ct_lb_mark;");
> >>>>>>> + REGBIT_CONNTRACK_NAT" == 1",
ds_cstr(&actions));
> >>>>>>>
> >>>>>>> /* If REGBIT_CONNTRACK_DEFRAG is set as 1, then the packets
> >>> should be
> >>>>>>> * sent to conntrack for tracking and defragmentation. */
> >>>>>>> @@ -5817,6 +5837,9 @@ build_pre_stateful(struct ovn_datapath *od,
> >>> struct hmap *lflows)
> >>>>>>>
> >>>>>>> ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_STATEFUL, 100,
> >>>>>>> REGBIT_CONNTRACK_DEFRAG" == 1", "ct_next;");
> >>>>>>> +
> >>>>>>> + ds_destroy(&actions);
> >>>>>>> + ds_destroy(&match);
> >>>>>>> }
> >>>>>>>
> >>>>>>> static void
> >>>>>>> @@ -6684,7 +6707,7 @@ build_qos(struct ovn_datapath *od, struct
hmap
> >>> *lflows) {
> >>>>>>> }
> >>>>>>>
> >>>>>>> static void
> >>>>>>> -build_lb_rules(struct hmap *lflows, struct ovn_northd_lb *lb,
> >>>>>>> +build_lb_rules(struct hmap *lflows, struct ovn_northd_lb *lb,
bool
> >>> ct_lb_mark,
> >>>>>>> struct ds *match, struct ds *action,
> >>>>>>> const struct shash *meter_groups)
> >>>>>>> {
> >>>>>>> @@ -6734,7 +6757,8 @@ build_lb_rules(struct hmap *lflows, struct
> >>> ovn_northd_lb *lb,
> >>>>>>> /* New connections in Ingress table. */
> >>>>>>> const char *meter = NULL;
> >>>>>>> bool reject = build_lb_vip_actions(lb_vip, lb_vip_nb,
> >>> action,
> >>>>>>> - lb->selection_fields,
> >>> true);
> >>>>>>> + lb->selection_fields,
> >>> true,
> >>>>>>> + ct_lb_mark);
> >>>>>>>
> >>>>>>> ds_put_format(match, "ct.new && %s.dst == %s",
ip_match,
> >>>>>>> lb_vip->vip_str);
> >>>>>>> @@ -7583,6 +7607,7 @@ build_lswitch_flows(const struct hmap
> >>> *datapaths,
> >>>>>>> static void
> >>>>>>> build_lswitch_lflows_pre_acl_and_acl(struct ovn_datapath *od,
> >>>>>>> const struct hmap
> >>> *port_groups,
> >>>>>>> + const struct
chassis_features
> >>> *features,
> >>>>>>> struct hmap *lflows,
> >>>>>>> const struct shash
> >>> *meter_groups)
> >>>>>>> {
> >>>>>>> @@ -7591,7 +7616,7 @@ build_lswitch_lflows_pre_acl_and_acl(struct
> >>> ovn_datapath *od,
> >>>>>>>
> >>>>>>> build_pre_acls(od, port_groups, lflows);
> >>>>>>> build_pre_lb(od, meter_groups, lflows);
> >>>>>>> - build_pre_stateful(od, lflows);
> >>>>>>> + build_pre_stateful(od, features, lflows);
> >>>>>>> build_acl_hints(od, lflows);
> >>>>>>> build_acls(od, lflows, port_groups, meter_groups);
> >>>>>>> build_qos(od, lflows);
> >>>>>>> @@ -9672,8 +9697,10 @@ build_lrouter_nat_flows_for_lb(struct
> >>> ovn_lb_vip *lb_vip,
> >>>>>>> struct ovn_northd_lb_vip
*vips_nb,
> >>>>>>> struct hmap *lflows,
> >>>>>>> struct ds *match, struct ds
> > *action,
> >>>>>>> - const struct shash *meter_groups)
> >>>>>>> + const struct shash *meter_groups,
> >>>>>>> + bool ct_lb_mark)
> >>>>>>> {
> >>>>>>> + const char *ct_natted = ct_lb_mark ? "ct_mark.natted" :
> >>> "ct_label.natted";
> >>>>>>> char *skip_snat_new_action = NULL;
> >>>>>>> char *skip_snat_est_action = NULL;
> >>>>>>> char *new_match;
> >>>>>>> @@ -9683,7 +9710,8 @@ build_lrouter_nat_flows_for_lb(struct
> >>> ovn_lb_vip *lb_vip,
> >>>>>>> ds_clear(action);
> >>>>>>>
> >>>>>>> bool reject = build_lb_vip_actions(lb_vip, vips_nb, action,
> >>>>>>> - lb->selection_fields,
> > false);
> >>>>>>> + lb->selection_fields,
false,
> >>>>>>> + ct_lb_mark);
> >>>>>>>
> >>>>>>> /* Higher priority rules are added for load-balancing in
DNAT
> >>>>>>> * table. For every match (on a VIP[:port]), we add two
> > flows.
> >>>>>>> @@ -9714,13 +9742,13 @@ build_lrouter_nat_flows_for_lb(struct
> >>> ovn_lb_vip *lb_vip,
> >>>>>>> REG_ORIG_TP_DPORT_ROUTER" == %d",
> >>>>>>> ds_cstr(match), lb->proto,
> >>> lb_vip->vip_port);
> >>>>>>> est_match = xasprintf("ct.est && %s && %s && "
> >>>>>>> - REG_ORIG_TP_DPORT_ROUTER" == %d &&
"
> >>>>>>> - "ct_mark.natted == 1",
> >>>>>>> - ds_cstr(match), lb->proto,
> >>> lb_vip->vip_port);
> >>>>>>> + REG_ORIG_TP_DPORT_ROUTER" == %d &&
%s
> >>> == 1",
> >>>>>>> + ds_cstr(match), lb->proto,
> >>> lb_vip->vip_port,
> >>>>>>> + ct_natted);
> >>>>>>> } else {
> >>>>>>> new_match = xasprintf("ct.new && %s", ds_cstr(match));
> >>>>>>> - est_match = xasprintf("ct.est && %s && ct_mark.natted ==
> > 1",
> >>>>>>> - ds_cstr(match));
> >>>>>>> + est_match = xasprintf("ct.est && %s && %s == 1",
> >>>>>>> + ds_cstr(match), ct_natted);
> >>>>>>> }
> >>>>>>>
> >>>>>>> const char *ip_match = NULL;
> >>>>>>> @@ -9930,8 +9958,9 @@ build_lrouter_nat_flows_for_lb(struct
> >>> ovn_lb_vip *lb_vip,
> >>>>>>>
> >>>>>>> static void
> >>>>>>> build_lswitch_flows_for_lb(struct ovn_northd_lb *lb, struct
hmap
> >>> *lflows,
> >>>>>>> - const struct shash *meter_groups,
struct
> >>> ds *match,
> >>>>>>> - struct ds *action)
> >>>>>>> + const struct shash *meter_groups,
> >>>>>>> + const struct chassis_features
*features,
> >>>>>>> + struct ds *match, struct ds *action)
> >>>>>>> {
> >>>>>>> if (!lb->n_nb_ls) {
> >>>>>>> return;
> >>>>>>> @@ -9967,7 +9996,8 @@ build_lswitch_flows_for_lb(struct
> > ovn_northd_lb
> >>> *lb, struct hmap *lflows,
> >>>>>>> * a higher priority rule for load balancing below also
> >>> commits the
> >>>>>>> * connection, so it is okay if we do not hit the above
match
> >>> on
> >>>>>>> * REGBIT_CONNTRACK_COMMIT. */
> >>>>>>> - build_lb_rules(lflows, lb, match, action, meter_groups);
> >>>>>>> + build_lb_rules(lflows, lb, features->ct_lb_mark,
> >>>>>>> + match, action, meter_groups);
> >>>>>>> }
> >>>>>>>
> >>>>>>> /* If there are any load balancing rules, we should send the
> >>> packet to
> >>>>>>> @@ -10037,8 +10067,9 @@ build_lrouter_defrag_flows_for_lb(struct
> >>> ovn_northd_lb *lb,
> >>>>>>>
> >>>>>>> static void
> >>>>>>> build_lrouter_flows_for_lb(struct ovn_northd_lb *lb, struct
hmap
> >>> *lflows,
> >>>>>>> - const struct shash *meter_groups,
struct
> >>> ds *match,
> >>>>>>> - struct ds *action)
> >>>>>>> + const struct shash *meter_groups,
> >>>>>>> + const struct chassis_features
*features,
> >>>>>>> + struct ds *match, struct ds *action)
> >>>>>>> {
> >>>>>>> if (!lb->n_nb_lr) {
> >>>>>>> return;
> >>>>>>> @@ -10049,7 +10080,7 @@ build_lrouter_flows_for_lb(struct
> >>> ovn_northd_lb *lb, struct hmap *lflows,
> >>>>>>>
> >>>>>>> build_lrouter_nat_flows_for_lb(lb_vip, lb,
> > &lb->vips_nb[i],
> >>>>>>> lflows, match, action,
> >>>>>>> - meter_groups);
> >>>>>>> + meter_groups,
> >>> features->ct_lb_mark);
> >>>>>>>
> >>>>>>> if (!build_empty_lb_event_flow(lb_vip, lb->nlb, match,
> >>> action)) {
> >>>>>>> continue;
> >>>>>>> @@ -13522,6 +13553,7 @@ struct lswitch_flow_build_info {
> >>>>>>> const struct shash *meter_groups;
> >>>>>>> const struct hmap *lbs;
> >>>>>>> const struct hmap *bfd_connections;
> >>>>>>> + const struct chassis_features *features;
> >>>>>>> char *svc_check_match;
> >>>>>>> struct ds match;
> >>>>>>> struct ds actions;
> >>>>>>> @@ -13540,7 +13572,9 @@
> >>> build_lswitch_and_lrouter_iterate_by_od(struct ovn_datapath *od,
> >>>>>>> struct
> >>> lswitch_flow_build_info *lsi)
> >>>>>>> {
> >>>>>>> /* Build Logical Switch Flows. */
> >>>>>>> - build_lswitch_lflows_pre_acl_and_acl(od, lsi->port_groups,
> >>> lsi->lflows,
> >>>>>>> + build_lswitch_lflows_pre_acl_and_acl(od, lsi->port_groups,
> >>>>>>> + lsi->features,
> >>>>>>> + lsi->lflows,
> >>>>>>> lsi->meter_groups);
> >>>>>>>
> >>>>>>> build_fwd_group_lflows(od, lsi->lflows);
> >>>>>>> @@ -13680,10 +13714,12 @@ build_lflows_thread(void *arg)
> >>>>>>> build_lrouter_defrag_flows_for_lb(lb,
> >>> lsi->lflows,
> >>>>>>>
> > &lsi->match);
> >>>>>>> build_lrouter_flows_for_lb(lb, lsi->lflows,
> >>>>>>> - lsi->meter_groups,
> >>> &lsi->match,
> >>>>>>> - &lsi->actions);
> >>>>>>> + lsi->meter_groups,
> >>>>>>> + lsi->features,
> >>>>>>> + &lsi->match,
> >>> &lsi->actions);
> >>>>>>> build_lswitch_flows_for_lb(lb, lsi->lflows,
> >>>>>>>
lsi->meter_groups,
> >>>>>>> + lsi->features,
> >>>>>>> &lsi->match,
> >>> &lsi->actions);
> >>>>>>> }
> >>>>>>> }
> >>>>>>> @@ -13749,7 +13785,8 @@ build_lswitch_and_lrouter_flows(const
struct
> >>> hmap *datapaths,
> >>>>>>> struct hmap *igmp_groups,
> >>>>>>> const struct shash
*meter_groups,
> >>>>>>> const struct hmap *lbs,
> >>>>>>> - const struct hmap
*bfd_connections)
> >>>>>>> + const struct hmap
*bfd_connections,
> >>>>>>> + const struct chassis_features
> >>> *features)
> >>>>>>> {
> >>>>>>>
> >>>>>>> char *svc_check_match = xasprintf("eth.dst == %s",
> >>> svc_monitor_mac);
> >>>>>>> @@ -13786,6 +13823,7 @@ build_lswitch_and_lrouter_flows(const
struct
> >>> hmap *datapaths,
> >>>>>>> lsiv[index].meter_groups = meter_groups;
> >>>>>>> lsiv[index].lbs = lbs;
> >>>>>>> lsiv[index].bfd_connections = bfd_connections;
> >>>>>>> + lsiv[index].features = features;
> >>>>>>> lsiv[index].svc_check_match = svc_check_match;
> >>>>>>> lsiv[index].thread_lflow_counter = 0;
> >>>>>>> ds_init(&lsiv[index].match);
> >>>>>>> @@ -13824,6 +13862,7 @@ build_lswitch_and_lrouter_flows(const
struct
> >>> hmap *datapaths,
> >>>>>>> .meter_groups = meter_groups,
> >>>>>>> .lbs = lbs,
> >>>>>>> .bfd_connections = bfd_connections,
> >>>>>>> + .features = features,
> >>>>>>> .svc_check_match = svc_check_match,
> >>>>>>> .match = DS_EMPTY_INITIALIZER,
> >>>>>>> .actions = DS_EMPTY_INITIALIZER,
> >>>>>>> @@ -13849,9 +13888,9 @@ build_lswitch_and_lrouter_flows(const
struct
> >>> hmap *datapaths,
> >>>>>>> &lsi.match);
> >>>>>>> build_lrouter_defrag_flows_for_lb(lb, lsi.lflows,
> >>> &lsi.match);
> >>>>>>> build_lrouter_flows_for_lb(lb, lsi.lflows,
> >>> lsi.meter_groups,
> >>>>>>> - &lsi.match, &lsi.actions);
> >>>>>>> + lsi.features, &lsi.match,
> >>> &lsi.actions);
> >>>>>>> build_lswitch_flows_for_lb(lb, lsi.lflows,
> >>> lsi.meter_groups,
> >>>>>>> - &lsi.match, &lsi.actions);
> >>>>>>> + lsi.features, &lsi.match,
> >>> &lsi.actions);
> >>>>>>> }
> >>>>>>> stopwatch_stop(LFLOWS_LBS_STOPWATCH_NAME, time_msec());
> >>>>>>> stopwatch_start(LFLOWS_IGMP_STOPWATCH_NAME,
time_msec());
> >>>>>>> @@ -13998,7 +14037,8 @@ void build_lflows(struct lflow_input
> >>> *input_data,
> >>>>>>> input_data->port_groups,
> >>> &lflows,
> >>>>>>> &mcast_groups,
&igmp_groups,
> >>>>>>> input_data->meter_groups,
> >>> input_data->lbs,
> >>>>>>> - input_data->bfd_connections);
> >>>>>>> + input_data->bfd_connections,
> >>>>>>> + input_data->features);
> >>>>>>>
> >>>>>>> if (parallelization_state == STATE_INIT_HASH_SIZES) {
> >>>>>>> parallelization_state = STATE_USE_PARALLELIZATION;
> >>>>>>> @@ -15129,6 +15169,7 @@ northd_init(struct northd_data *data)
> >>>>>>> hmap_init(&data->lbs);
> >>>>>>> hmap_init(&data->bfd_connections);
> >>>>>>> ovs_list_init(&data->lr_list);
> >>>>>>> + memset(&data->features, 0, sizeof data->features);
> >>>>>>> data->ovn_internal_version_changed = false;
> >>>>>>> }
> >>>>>>>
> >>>>>>> @@ -15262,6 +15303,7 @@ ovnnb_db_run(struct northd_input
> > *input_data,
> >>>>>>> "ignore_lsp_down", true);
> >>>>>>> default_acl_drop = smap_get_bool(&nb->options,
> >>> "default_acl_drop", false);
> >>>>>>>
> >>>>>>> + build_chassis_features(input_data, &data->features);
> >>>>>>> build_datapaths(input_data, ovnsb_txn, &data->datapaths,
> >>> &data->lr_list);
> >>>>>>> build_lbs(input_data, &data->datapaths, &data->lbs);
> >>>>>>> build_ports(input_data, ovnsb_txn, sbrec_chassis_by_name,
> >>>>>>> diff --git a/northd/northd.h b/northd/northd.h
> >>>>>>> index fe8dad03a..07fcbcacc 100644
> >>>>>>> --- a/northd/northd.h
> >>>>>>> +++ b/northd/northd.h
> >>>>>>> @@ -58,6 +58,10 @@ struct northd_input {
> >>>>>>> struct ovsdb_idl_index
*sbrec_static_mac_binding_by_lport_ip;
> >>>>>>> };
> >>>>>>>
> >>>>>>> +struct chassis_features {
> >>>>>>> + bool ct_lb_mark;
> >>>>>>> +};
> >>>>>>> +
> >>>>>>> struct northd_data {
> >>>>>>> /* Global state for 'en-northd'. */
> >>>>>>> struct hmap datapaths;
> >>>>>>> @@ -68,6 +72,7 @@ struct northd_data {
> >>>>>>> struct hmap bfd_connections;
> >>>>>>> struct ovs_list lr_list;
> >>>>>>> bool ovn_internal_version_changed;
> >>>>>>> + struct chassis_features features;
> >>>>>>> };
> >>>>>>>
> >>>>>>> struct lflow_input {
> >>>>>>> @@ -89,6 +94,7 @@ struct lflow_input {
> >>>>>>> const struct shash *meter_groups;
> >>>>>>> const struct hmap *lbs;
> >>>>>>> const struct hmap *bfd_connections;
> >>>>>>> + const struct chassis_features *features;
> >>>>>>> bool ovn_internal_version_changed;
> >>>>>>> };
> >>>>>>>
> >>>>>>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> >>>>>>> index 5bd0935e7..7bb6d33ac 100644
> >>>>>>> --- a/tests/ovn-northd.at
> >>>>>>> +++ b/tests/ovn-northd.at
> >>>>>>> @@ -4957,7 +4957,8 @@ AT_CHECK([grep "lr_out_snat" lr0flows | sed
> >>> 's/table=./table=?/' | sort], [0], [
> >>>>>>> table=? (lr_out_snat ), priority=120 , match=(nd_ns),
> >>> action=(next;)
> >>>>>>> ])
> >>>>>>>
> >>>>>>> -ovn-sbctl chassis-add gw1 geneve 127.0.0.1
> >>>>>>> +check ovn-sbctl chassis-add gw1 geneve 127.0.0.1 \
> >>>>>>> + -- set chassis gw1 other_config:ct-lb-mark="true"
> >>>>>>>
> >>>>>>> # Create a distributed gw port on lr0
> >>>>>>> check ovn-nbctl ls-add public
> >>>>>>> @@ -7418,3 +7419,69 @@ AT_CHECK([cat sw0flows | grep -e port_sec |
> >>> sort | sed 's/table=./table=?/' ], [
> >>>>>>>
> >>>>>>> AT_CLEANUP
> >>>>>>> ])
> >>>>>>> +
> >>>>>>> +
> >>>>>>> +OVN_FOR_EACH_NORTHD([
> >>>>>>> +AT_SETUP([Load balancer ct_lb_mark backwards compatibility])
> >>>>>>> +AT_KEYWORDS([lb])
> >>>>>>> +ovn_start
> >>>>>>> +
> >>>>>>> +check ovn-nbctl \
> >>>>>>> + -- ls-add ls \
> >>>>>>> + -- lr-add lr -- set logical_router lr options:chassis=local \
> >>>>>>> + -- lb-add lb-test 66.66.66.66 42.42.42.2 \
> >>>>>>> + -- ls-lb-add ls lb-test \
> >>>>>>> + -- lr-lb-add lr lb-test
> >>>>>>> +
> >>>>>>> +AS_BOX([No chassis registered - use ct_lb_mark and
ct_mark.natted])
> >>>>>>> +check ovn-nbctl --wait=sb sync
> >>>>>>> +AT_CHECK([ovn-sbctl lflow-list | grep -e natted -e ct_lb], [0],
> > [dnl
> >>>>>>> + table=6 (lr_in_dnat ), priority=110 , match=(ct.est &&
> >>> ip4 && reg0 == 66.66.66.66 && ct_mark.natted == 1), action=(next;)
> >>>>>>> + table=6 (lr_in_dnat ), priority=110 , match=(ct.new &&
> >>> ip4 && reg0 == 66.66.66.66), action=(ct_lb_mark(backends=42.42.42.2);)
> >>>>>>> + table=6 (ls_in_pre_stateful ), priority=120 , match=(reg0[[2]]
> > ==
> >>> 1 && ip4 && sctp), action=(reg1 = ip4.dst; reg2[[0..15]] = sctp.dst;
> >>> ct_lb_mark;)
> >>>>>>> + table=6 (ls_in_pre_stateful ), priority=120 , match=(reg0[[2]]
> > ==
> >>> 1 && ip4 && tcp), action=(reg1 = ip4.dst; reg2[[0..15]] = tcp.dst;
> >>> ct_lb_mark;)
> >>>>>>> + table=6 (ls_in_pre_stateful ), priority=120 , match=(reg0[[2]]
> > ==
> >>> 1 && ip4 && udp), action=(reg1 = ip4.dst; reg2[[0..15]] = udp.dst;
> >>> ct_lb_mark;)
> >>>>>>> + table=6 (ls_in_pre_stateful ), priority=120 , match=(reg0[[2]]
> > ==
> >>> 1 && ip6 && sctp), action=(xxreg1 = ip6.dst; reg2[[0..15]] = sctp.dst;
> >>> ct_lb_mark;)
> >>>>>>> + table=6 (ls_in_pre_stateful ), priority=120 , match=(reg0[[2]]
> > ==
> >>> 1 && ip6 && tcp), action=(xxreg1 = ip6.dst; reg2[[0..15]] = tcp.dst;
> >>> ct_lb_mark;)
> >>>>>>> + table=6 (ls_in_pre_stateful ), priority=120 , match=(reg0[[2]]
> > ==
> >>> 1 && ip6 && udp), action=(xxreg1 = ip6.dst; reg2[[0..15]] = udp.dst;
> >>> ct_lb_mark;)
> >>>>>>> + table=6 (ls_in_pre_stateful ), priority=110 , match=(reg0[[2]]
> > ==
> >>> 1), action=(ct_lb_mark;)
> >>>>>>> + table=11(ls_in_lb ), priority=110 , match=(ct.new &&
> >>> ip4.dst == 66.66.66.66), action=(reg0[[1]] = 0; reg1 = 66.66.66.66;
> >>> ct_lb_mark(backends=42.42.42.2);)
> >>>>>>> + table=2 (ls_out_pre_stateful), priority=110 , match=(reg0[[2]]
> > ==
> >>> 1), action=(ct_lb_mark;)
> >>>>>>> +])
> >>>>>>> +
> >>>>>>> +AS_BOX([Chassis registered that doesn't support ct_lb_mark - use
> >>> ct_lb and ct_label.natted])
> >>>>>>> +check ovn-sbctl chassis-add hv geneve 127.0.0.1
> >>>>>>> +check ovn-nbctl --wait=sb sync
> >>>>>>> +AT_CHECK([ovn-sbctl lflow-list | grep -e natted -e ct_lb], [0],
> > [dnl
> >>>>>>> + table=6 (lr_in_dnat ), priority=110 , match=(ct.est &&
> >>> ip4 && reg0 == 66.66.66.66 && ct_label.natted == 1), action=(next;)
> >>>>>>> + table=6 (lr_in_dnat ), priority=110 , match=(ct.new &&
> >>> ip4 && reg0 == 66.66.66.66), action=(ct_lb(backends=42.42.42.2);)
> >>>>>>> + table=6 (ls_in_pre_stateful ), priority=120 , match=(reg0[[2]]
> > ==
> >>> 1 && ip4 && sctp), action=(reg1 = ip4.dst; reg2[[0..15]] = sctp.dst;
> > ct_lb;)
> >>>>>>> + table=6 (ls_in_pre_stateful ), priority=120 , match=(reg0[[2]]
> > ==
> >>> 1 && ip4 && tcp), action=(reg1 = ip4.dst; reg2[[0..15]] = tcp.dst;
> > ct_lb;)
> >>>>>>> + table=6 (ls_in_pre_stateful ), priority=120 , match=(reg0[[2]]
> > ==
> >>> 1 && ip4 && udp), action=(reg1 = ip4.dst; reg2[[0..15]] = udp.dst;
> > ct_lb;)
> >>>>>>> + table=6 (ls_in_pre_stateful ), priority=120 , match=(reg0[[2]]
> > ==
> >>> 1 && ip6 && sctp), action=(xxreg1 = ip6.dst; reg2[[0..15]] = sctp.dst;
> >>> ct_lb;)
> >>>>>>> + table=6 (ls_in_pre_stateful ), priority=120 , match=(reg0[[2]]
> > ==
> >>> 1 && ip6 && tcp), action=(xxreg1 = ip6.dst; reg2[[0..15]] = tcp.dst;
> > ct_lb;)
> >>>>>>> + table=6 (ls_in_pre_stateful ), priority=120 , match=(reg0[[2]]
> > ==
> >>> 1 && ip6 && udp), action=(xxreg1 = ip6.dst; reg2[[0..15]] = udp.dst;
> > ct_lb;)
> >>>>>>> + table=6 (ls_in_pre_stateful ), priority=110 , match=(reg0[[2]]
> > ==
> >>> 1), action=(ct_lb;)
> >>>>>>> + table=11(ls_in_lb ), priority=110 , match=(ct.new &&
> >>> ip4.dst == 66.66.66.66), action=(reg0[[1]] = 0; reg1 = 66.66.66.66;
> >>> ct_lb(backends=42.42.42.2);)
> >>>>>>> + table=2 (ls_out_pre_stateful), priority=110 , match=(reg0[[2]]
> > ==
> >>> 1), action=(ct_lb;)
> >>>>>>> +])
> >>>>>>> +
> >>>>>>> +AS_BOX([Chassis upgrades and supports ct_lb_mark - use ct_lb_mark
> >>> and ct_mark.natted])
> >>>>>>> +check ovn-sbctl set chassis hv other_config:ct-lb-mark=true
> >>>>>>> +check ovn-nbctl --wait=sb sync
> >>>>>>> +AT_CHECK([ovn-sbctl lflow-list | grep -e natted -e ct_lb], [0],
> > [dnl
> >>>>>>> + table=6 (lr_in_dnat ), priority=110 , match=(ct.est &&
> >>> ip4 && reg0 == 66.66.66.66 && ct_mark.natted == 1), action=(next;)
> >>>>>>> + table=6 (lr_in_dnat ), priority=110 , match=(ct.new &&
> >>> ip4 && reg0 == 66.66.66.66), action=(ct_lb_mark(backends=42.42.42.2);)
> >>>>>>> + table=6 (ls_in_pre_stateful ), priority=120 , match=(reg0[[2]]
> > ==
> >>> 1 && ip4 && sctp), action=(reg1 = ip4.dst; reg2[[0..15]] = sctp.dst;
> >>> ct_lb_mark;)
> >>>>>>> + table=6 (ls_in_pre_stateful ), priority=120 , match=(reg0[[2]]
> > ==
> >>> 1 && ip4 && tcp), action=(reg1 = ip4.dst; reg2[[0..15]] = tcp.dst;
> >>> ct_lb_mark;)
> >>>>>>> + table=6 (ls_in_pre_stateful ), priority=120 , match=(reg0[[2]]
> > ==
> >>> 1 && ip4 && udp), action=(reg1 = ip4.dst; reg2[[0..15]] = udp.dst;
> >>> ct_lb_mark;)
> >>>>>>> + table=6 (ls_in_pre_stateful ), priority=120 , match=(reg0[[2]]
> > ==
> >>> 1 && ip6 && sctp), action=(xxreg1 = ip6.dst; reg2[[0..15]] = sctp.dst;
> >>> ct_lb_mark;)
> >>>>>>> + table=6 (ls_in_pre_stateful ), priority=120 , match=(reg0[[2]]
> > ==
> >>> 1 && ip6 && tcp), action=(xxreg1 = ip6.dst; reg2[[0..15]] = tcp.dst;
> >>> ct_lb_mark;)
> >>>>>>> + table=6 (ls_in_pre_stateful ), priority=120 , match=(reg0[[2]]
> > ==
> >>> 1 && ip6 && udp), action=(xxreg1 = ip6.dst; reg2[[0..15]] = udp.dst;
> >>> ct_lb_mark;)
> >>>>>>> + table=6 (ls_in_pre_stateful ), priority=110 , match=(reg0[[2]]
> > ==
> >>> 1), action=(ct_lb_mark;)
> >>>>>>> + table=11(ls_in_lb ), priority=110 , match=(ct.new &&
> >>> ip4.dst == 66.66.66.66), action=(reg0[[1]] = 0; reg1 = 66.66.66.66;
> >>> ct_lb_mark(backends=42.42.42.2);)
> >>>>>>> + table=2 (ls_out_pre_stateful), priority=110 , match=(reg0[[2]]
> > ==
> >>> 1), action=(ct_lb_mark;)
> >>>>>>> +])
> >>>>>>> +
> >>>>>>> +AT_CLEANUP
> >>>>>>> +])
> >>>>>>>
> >>>>>>
> >>>>>> _______________________________________________
> >>>>>> dev mailing list
> >>>>>> [email protected]
> >>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>>>>>
> >>>>>
> >>>>
> >>>> _______________________________________________
> >>>> dev mailing list
> >>>> [email protected]
> >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>>
> >>
> >
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
