On 6/2/22 16:57, Han Zhou wrote: > 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).
I posted a series just now that should cover all cases we discussed here: https://patchwork.ozlabs.org/project/ovn/list/?series=303103&state=* If the approach seems OK to everyone I can go ahead prepare backports for branch-22.06, branch-22.03, branch-21.12. None of the stable releases includes the ct_mark change so if we get these in before the next round of minor releases (I think Mark wanted to do that tomorrow) we should be good. Thanks, Dumitru > > 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
