On 1/18/23 13:27, Numan Siddique wrote: > On Thu, Jan 12, 2023 at 6:26 AM Dumitru Ceara <[email protected]> wrote: >> >> On 1/12/23 11:35, Ales Musil wrote: >>> On Thu, Jan 12, 2023 at 11:30 AM Dumitru Ceara <[email protected]> wrote: >>> >>>> On 1/10/23 16:58, Ales Musil wrote: >>>>> On Mon, Jan 9, 2023 at 3:35 PM Ales Musil <[email protected]> wrote: >>>>> >>>>>> In order to be backward compatible add feature flag >>>>>> that ensures that the CT related flows are skipped >>>>>> if needed. >>>>>> >>>>>> Signed-off-by: Ales Musil <[email protected]> >>>>>> --- >>>>>> v2: Fix the wrong * position. >>>>>> --- >>>>>> controller/chassis.c | 7 +++ >>>>>> include/ovn/features.h | 1 + >>>>>> northd/northd.c | 51 +++++++++++++------ >>>>>> northd/northd.h | 1 + >>>>>> tests/ovn-northd.at | 109 +++++++++++++++++++++++++++++++++++++++-- >>>>>> 5 files changed, 149 insertions(+), 20 deletions(-) >>>>>> >>>>>> diff --git a/controller/chassis.c b/controller/chassis.c >>>>>> index 685d9b2ae..98f8da2be 100644 >>>>>> --- a/controller/chassis.c >>>>>> +++ b/controller/chassis.c >>>>>> @@ -352,6 +352,7 @@ chassis_build_other_config(const struct >>>>>> ovs_chassis_cfg *ovs_cfg, >>>>>> smap_replace(config, OVN_FEATURE_PORT_UP_NOTIF, "true"); >>>>>> smap_replace(config, OVN_FEATURE_CT_NO_MASKED_LABEL, "true"); >>>>>> smap_replace(config, OVN_FEATURE_MAC_BINDING_TIMESTAMP, "true"); >>>>>> + smap_replace(config, OVN_FEATURE_CT_LB_RELATED, "true"); >>>>>> } >>>>>> >>>>>> /* >>>>>> @@ -469,6 +470,12 @@ 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_RELATED, >>>>>> + false)) { >>>>>> + return true; >>>>>> + } >>>>>> + >>>>>> return false; >>>>>> } >>>>>> >>>>>> diff --git a/include/ovn/features.h b/include/ovn/features.h >>>>>> index 679f67457..5bcd68739 100644 >>>>>> --- a/include/ovn/features.h >>>>>> +++ b/include/ovn/features.h >>>>>> @@ -24,6 +24,7 @@ >>>>>> #define OVN_FEATURE_PORT_UP_NOTIF "port-up-notif" >>>>>> #define OVN_FEATURE_CT_NO_MASKED_LABEL "ct-no-masked-label" >>>>>> #define OVN_FEATURE_MAC_BINDING_TIMESTAMP "mac-binding-timestamp" >>>>>> +#define OVN_FEATURE_CT_LB_RELATED "ovn-ct-lb-related" >>>>>> >>>>>> /* OVS datapath supported features. Based on availability OVN might >>>>>> generate >>>>>> * different types of openflows. >>>>>> diff --git a/northd/northd.c b/northd/northd.c >>>>>> index 4751feab4..72f7118a8 100644 >>>>>> --- a/northd/northd.c >>>>>> +++ b/northd/northd.c >>>>>> @@ -447,6 +447,15 @@ build_chassis_features(const struct northd_input >>>>>> *input_data, >>>>>> chassis_features->mac_binding_timestamp) { >>>>>> chassis_features->mac_binding_timestamp = false; >>>>>> } >>>>>> + >>>>>> + bool ct_lb_related = >>>>>> + smap_get_bool(&chassis->other_config, >>>>>> + OVN_FEATURE_CT_LB_RELATED, >>>>>> + false); >>>>>> + if (!ct_lb_related && >>>>>> + chassis_features->ct_lb_related) { >>>>>> + chassis_features->ct_lb_related = false; >>>>>> + } >>>>>> } >>>>>> } >>>>>> >>>>>> @@ -6786,14 +6795,17 @@ build_acls(struct ovn_datapath *od, const struct >>>>>> chassis_features *features, >>>>>> * a dynamically negotiated FTP data channel), but will allow >>>>>> * related traffic such as an ICMP Port Unreachable through >>>>>> * that's generated from a non-listening UDP port. */ >>>>>> + const char *ct_acl_action = features->ct_lb_related >>>>>> + ? "ct_commit_nat;" >>>>>> + : "next;"; >>>>>> ds_clear(&match); >>>>>> ds_put_format(&match, "!ct.est && ct.rel && !ct.new%s && %s == >>>> 0", >>>>>> use_ct_inv_match ? " && !ct.inv" : "", >>>>>> ct_blocked_match); >>>>>> ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX - 3, >>>>>> - ds_cstr(&match), "ct_commit_nat;"); >>>>>> + ds_cstr(&match), ct_acl_action); >>>>>> ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX - 3, >>>>>> - ds_cstr(&match), "ct_commit_nat;"); >>>>>> + ds_cstr(&match), ct_acl_action); >>>>>> >>>>>> /* Ingress and Egress ACL Table (Priority 65532). >>>>>> * >>>>>> @@ -10484,9 +10496,11 @@ build_lrouter_nat_flows_for_lb(struct >>>> ovn_lb_vip >>>>>> *lb_vip, >>>>>> struct hmap *lflows, >>>>>> struct ds *match, struct ds *action, >>>>>> const struct shash *meter_groups, >>>>>> - bool ct_lb_mark) >>>>>> + const struct chassis_features *features) >>>>>> { >>>>>> - const char *ct_natted = ct_lb_mark ? "ct_mark.natted" : >>>>>> "ct_label.natted"; >>>>>> + const char *ct_natted = features->ct_no_masked_label >>>>>> + ? "ct_mark.natted" >>>>>> + : "ct_label.natted"; >>>>>> char *skip_snat_new_action = NULL; >>>>>> char *skip_snat_est_action = NULL; >>>>>> char *new_match; >>>>>> @@ -10497,7 +10511,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip >>>>>> *lb_vip, >>>>>> >>>>>> bool reject = build_lb_vip_actions(lb_vip, vips_nb, action, >>>>>> lb->selection_fields, false, >>>>>> - ct_lb_mark); >>>>>> + features->ct_no_masked_label); >>>>>> bool drop = !!strncmp(ds_cstr(action), "ct_lb", strlen("ct_lb")); >>>>>> if (!drop) { >>>>>> /* Remove the trailing ");". */ >>>>>> @@ -10519,9 +10533,11 @@ build_lrouter_nat_flows_for_lb(struct >>>> ovn_lb_vip >>>>>> *lb_vip, >>>>>> } >>>>>> >>>>>> if (lb->skip_snat) { >>>>>> + const char *skip_snat = features->ct_lb_related && !drop >>>>>> + ? "; skip_snat);" >>>>>> + : ""; >>>>>> skip_snat_new_action = xasprintf("flags.skip_snat_for_lb = 1; >>>>>> %s%s", >>>>>> - ds_cstr(action), >>>>>> - drop ? "" : "; skip_snat);"); >>>>>> + ds_cstr(action), skip_snat); >>>>>> skip_snat_est_action = xasprintf("flags.skip_snat_for_lb = 1; " >>>>>> "next;"); >>>>>> } >>>>>> @@ -10654,9 +10670,11 @@ build_lrouter_nat_flows_for_lb(struct >>>> ovn_lb_vip >>>>>> *lb_vip, >>>>>> skip_snat_new_action, est_match, >>>>>> skip_snat_est_action, lflows, prio, meter_groups); >>>>>> >>>>>> + const char *force_snat = features->ct_lb_related && !drop >>>>>> + ? "; force_snat);" >>>>>> + : ""; >>>>>> char *new_actions = xasprintf("flags.force_snat_for_lb = 1; %s%s", >>>>>> - ds_cstr(action), >>>>>> - drop ? "" : "; force_snat);"); >>>>>> + ds_cstr(action), force_snat); >>>>>> build_gw_lrouter_nat_flows_for_lb(lb, gw_router_force_snat, >>>>>> n_gw_router_force_snat, reject, new_match, >>>>>> new_actions, est_match, >>>>>> @@ -10911,7 +10929,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, >>>>>> - features->ct_no_masked_label); >>>>>> + features); >>>>>> >>>>>> if (!build_empty_lb_event_flow(lb_vip, lb, match, action)) { >>>>>> continue; >>>>>> @@ -14221,7 +14239,7 @@ build_lrouter_nat_defrag_and_lb(struct >>>>>> ovn_datapath *od, struct hmap *lflows, >>>>>> const struct hmap *ports, struct ds >>>>>> *match, >>>>>> struct ds *actions, >>>>>> const struct shash *meter_groups, >>>>>> - bool ct_lb_mark) >>>>>> + const struct chassis_features >>>> *features) >>>>>> { >>>>>> if (!od->nbr) { >>>>>> return; >>>>>> @@ -14252,9 +14270,11 @@ build_lrouter_nat_defrag_and_lb(struct >>>>>> ovn_datapath *od, struct hmap *lflows, >>>>>> * a dynamically negotiated FTP data channel), but will allow >>>>>> * related traffic such as an ICMP Port Unreachable through >>>>>> * that's generated from a non-listening UDP port. */ >>>>>> - if (od->has_lb_vip) { >>>>>> + if (od->has_lb_vip && features->ct_lb_related) { >>>>>> ds_clear(match); >>>>>> - const char *ct_flag_reg = ct_lb_mark ? "ct_mark" : "ct_label"; >>>>>> + const char *ct_flag_reg = features->ct_no_masked_label >>>>>> + ? "ct_mark" >>>>>> + : "ct_label"; >>>>>> >>>>>> ds_put_cstr(match, "ct.rel && !ct.est && !ct.new"); >>>>>> size_t match_len = match->length; >>>>>> @@ -14477,7 +14497,7 @@ build_lrouter_nat_defrag_and_lb(struct >>>>>> ovn_datapath *od, struct hmap *lflows, >>>>>> >>>>>> if (od->nbr->n_nat) { >>>>>> ds_clear(match); >>>>>> - const char *ct_natted = ct_lb_mark ? >>>>>> + const char *ct_natted = features->ct_no_masked_label ? >>>>>> "ct_mark.natted" : >>>>>> "ct_label.natted"; >>>>>> ds_put_format(match, "ip && %s == 1", ct_natted); >>>>>> @@ -14594,7 +14614,7 @@ build_lswitch_and_lrouter_iterate_by_od(struct >>>>>> ovn_datapath *od, >>>>>> build_lrouter_arp_nd_for_datapath(od, lsi->lflows, >>>> lsi->meter_groups); >>>>>> build_lrouter_nat_defrag_and_lb(od, lsi->lflows, lsi->ports, >>>>>> &lsi->match, >>>>>> &lsi->actions, lsi->meter_groups, >>>>>> - lsi->features->ct_no_masked_label); >>>>>> + lsi->features); >>>>>> build_lb_affinity_default_flows(od, lsi->lflows); >>>>>> } >>>>>> >>>>>> @@ -16086,6 +16106,7 @@ northd_init(struct northd_data *data) >>>>>> data->features = (struct chassis_features) { >>>>>> .ct_no_masked_label = true, >>>>>> .mac_binding_timestamp = true, >>>>>> + .ct_lb_related = true, >>>>>> }; >>>>>> data->ovn_internal_version_changed = false; >>>>>> } >>>>>> diff --git a/northd/northd.h b/northd/northd.h >>>>>> index ff8727cb7..4d9055296 100644 >>>>>> --- a/northd/northd.h >>>>>> +++ b/northd/northd.h >>>>>> @@ -71,6 +71,7 @@ struct northd_input { >>>>>> struct chassis_features { >>>>>> bool ct_no_masked_label; >>>>>> bool mac_binding_timestamp; >>>>>> + bool ct_lb_related; >>>>>> }; >>>>>> >>>>>> struct northd_data { >>>>>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >>>>>> index 56c1e6c2e..2eb735bbe 100644 >>>>>> --- a/tests/ovn-northd.at >>>>>> +++ b/tests/ovn-northd.at >>>>>> @@ -5127,7 +5127,8 @@ AT_CHECK([grep "lr_out_snat" lr0flows | sed >>>>>> 's/table=./table=?/' | sort], [0], [ >>>>>> ]) >>>>>> >>>>>> check ovn-sbctl chassis-add gw1 geneve 127.0.0.1 \ >>>>>> - -- set chassis gw1 other_config:ct-no-masked-label="true" >>>>>> + -- set chassis gw1 other_config:ct-no-masked-label="true" \ >>>>>> + -- set chassis gw1 other_config:ovn-ct-lb-related="true" >>>>>> >>>>>> # Create a distributed gw port on lr0 >>>>>> check ovn-nbctl ls-add public >>>>>> @@ -7875,7 +7876,7 @@ AT_CHECK([ovn-sbctl lflow-list | grep >>>>>> 'ls.*acl.*blocked' ], [0], [dnl >>>>>> table=7 (ls_in_acl_hint ), priority=4 , match=(!ct.new && >>>> ct.est >>>>>> && !ct.rpl && ct_label.blocked == 0), action=(reg0[[8]] = 1; reg0[[10]] >>>> = >>>>>> 1; next;) >>>>>> table=7 (ls_in_acl_hint ), priority=2 , match=(ct.est && >>>>>> ct_label.blocked == 1), action=(reg0[[9]] = 1; next;) >>>>>> table=7 (ls_in_acl_hint ), priority=1 , match=(ct.est && >>>>>> ct_label.blocked == 0), action=(reg0[[10]] = 1; next;) >>>>>> - table=8 (ls_in_acl ), priority=65532, match=(!ct.est && >>>> ct.rel >>>>>> && !ct.new && !ct.inv && ct_label.blocked == 0), action=(ct_commit_nat;) >>>>>> + table=8 (ls_in_acl ), priority=65532, match=(!ct.est && >>>> ct.rel >>>>>> && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) >>>>>> table=8 (ls_in_acl ), priority=65532, match=(ct.est && >>>> !ct.rel >>>>>> && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), >>>>>> action=(reg0[[9]] = 0; reg0[[10]] = 0; next;) >>>>>> table=8 (ls_in_acl ), priority=65532, match=(ct.inv || >>>> (ct.est >>>>>> && ct.rpl && ct_label.blocked == 1)), action=(drop;) >>>>>> table=8 (ls_in_acl ), priority=1 , match=(ip && ct.est && >>>>>> ct_label.blocked == 1), action=(reg0[[1]] = 1; next;) >>>>>> @@ -7883,7 +7884,7 @@ AT_CHECK([ovn-sbctl lflow-list | grep >>>>>> 'ls.*acl.*blocked' ], [0], [dnl >>>>>> table=3 (ls_out_acl_hint ), priority=4 , match=(!ct.new && >>>> ct.est >>>>>> && !ct.rpl && ct_label.blocked == 0), action=(reg0[[8]] = 1; reg0[[10]] >>>> = >>>>>> 1; next;) >>>>>> table=3 (ls_out_acl_hint ), priority=2 , match=(ct.est && >>>>>> ct_label.blocked == 1), action=(reg0[[9]] = 1; next;) >>>>>> table=3 (ls_out_acl_hint ), priority=1 , match=(ct.est && >>>>>> ct_label.blocked == 0), action=(reg0[[10]] = 1; next;) >>>>>> - table=4 (ls_out_acl ), priority=65532, match=(!ct.est && >>>> ct.rel >>>>>> && !ct.new && !ct.inv && ct_label.blocked == 0), action=(ct_commit_nat;) >>>>>> + table=4 (ls_out_acl ), priority=65532, match=(!ct.est && >>>> ct.rel >>>>>> && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) >>>>>> table=4 (ls_out_acl ), priority=65532, match=(ct.est && >>>> !ct.rel >>>>>> && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), >>>> action=(next;) >>>>>> table=4 (ls_out_acl ), priority=65532, match=(ct.inv || >>>> (ct.est >>>>>> && ct.rpl && ct_label.blocked == 1)), action=(drop;) >>>>>> table=4 (ls_out_acl ), priority=1 , match=(ip && ct.est && >>>>>> ct_label.blocked == 1), action=(reg0[[1]] = 1; next;) >>>>>> @@ -7897,7 +7898,7 @@ AT_CHECK([ovn-sbctl lflow-list | grep >>>>>> 'ls.*acl.*blocked' ], [0], [dnl >>>>>> table=7 (ls_in_acl_hint ), priority=4 , match=(!ct.new && >>>> ct.est >>>>>> && !ct.rpl && ct_mark.blocked == 0), action=(reg0[[8]] = 1; reg0[[10]] >>>> = 1; >>>>>> next;) >>>>>> table=7 (ls_in_acl_hint ), priority=2 , match=(ct.est && >>>>>> ct_mark.blocked == 1), action=(reg0[[9]] = 1; next;) >>>>>> table=7 (ls_in_acl_hint ), priority=1 , match=(ct.est && >>>>>> ct_mark.blocked == 0), action=(reg0[[10]] = 1; next;) >>>>>> - table=8 (ls_in_acl ), priority=65532, match=(!ct.est && >>>> ct.rel >>>>>> && !ct.new && !ct.inv && ct_mark.blocked == 0), action=(ct_commit_nat;) >>>>>> + table=8 (ls_in_acl ), priority=65532, match=(!ct.est && >>>> ct.rel >>>>>> && !ct.new && !ct.inv && ct_mark.blocked == 0), action=(next;) >>>>>> table=8 (ls_in_acl ), priority=65532, match=(ct.est && >>>> !ct.rel >>>>>> && !ct.new && !ct.inv && ct.rpl && ct_mark.blocked == 0), >>>> action=(reg0[[9]] >>>>>> = 0; reg0[[10]] = 0; next;) >>>>>> table=8 (ls_in_acl ), priority=65532, match=(ct.inv || >>>> (ct.est >>>>>> && ct.rpl && ct_mark.blocked == 1)), action=(drop;) >>>>>> table=8 (ls_in_acl ), priority=1 , match=(ip && ct.est && >>>>>> ct_mark.blocked == 1), action=(reg0[[1]] = 1; next;) >>>>>> @@ -7905,7 +7906,7 @@ AT_CHECK([ovn-sbctl lflow-list | grep >>>>>> 'ls.*acl.*blocked' ], [0], [dnl >>>>>> table=3 (ls_out_acl_hint ), priority=4 , match=(!ct.new && >>>> ct.est >>>>>> && !ct.rpl && ct_mark.blocked == 0), action=(reg0[[8]] = 1; reg0[[10]] >>>> = 1; >>>>>> next;) >>>>>> table=3 (ls_out_acl_hint ), priority=2 , match=(ct.est && >>>>>> ct_mark.blocked == 1), action=(reg0[[9]] = 1; next;) >>>>>> table=3 (ls_out_acl_hint ), priority=1 , match=(ct.est && >>>>>> ct_mark.blocked == 0), action=(reg0[[10]] = 1; next;) >>>>>> - table=4 (ls_out_acl ), priority=65532, match=(!ct.est && >>>> ct.rel >>>>>> && !ct.new && !ct.inv && ct_mark.blocked == 0), action=(ct_commit_nat;) >>>>>> + table=4 (ls_out_acl ), priority=65532, match=(!ct.est && >>>> ct.rel >>>>>> && !ct.new && !ct.inv && ct_mark.blocked == 0), action=(next;) >>>>>> table=4 (ls_out_acl ), priority=65532, match=(ct.est && >>>> !ct.rel >>>>>> && !ct.new && !ct.inv && ct.rpl && ct_mark.blocked == 0), action=(next;) >>>>>> table=4 (ls_out_acl ), priority=65532, match=(ct.inv || >>>> (ct.est >>>>>> && ct.rpl && ct_mark.blocked == 1)), action=(drop;) >>>>>> table=4 (ls_out_acl ), priority=1 , match=(ip && ct.est && >>>>>> ct_mark.blocked == 1), action=(reg0[[1]] = 1; next;) >>>>>> @@ -8425,3 +8426,101 @@ check_row_count sb:Chassis_Template_Var 0 >>>>>> >>>>>> AT_CLEANUP >>>>>> ]) >>>>>> + >>>>>> +OVN_FOR_EACH_NORTHD_NO_HV([ >>>>>> +AT_SETUP([Load balancer CT related 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 192.168.0.1 192.168.1.10 \ >>>>>> + -- ls-lb-add ls lb-test \ >>>>>> + -- lr-lb-add lr lb-test >>>>>> + >>>>>> +m4_define([DUMP_FLOWS_SORTED], [sed 's/table=[[0-9]]\{1,2\}/table=?/' | >>>>>> sort]) >>>>>> + >>>>>> +AS_BOX([No chassis registered - CT related flows should be installed]) >>>>>> +check ovn-nbctl --wait=sb sync >>>>>> +ovn-sbctl dump-flows | DUMP_FLOWS_SORTED > lflows0 >>>>>> + >>>>>> +AT_CHECK([grep -e "lr_in_defrag" -e "lr_in_dnat" lflows0], [0], [dnl >>>>>> + table=? (lr_in_defrag ), priority=0 , match=(1), >>>> action=(next;) >>>>>> + table=? (lr_in_defrag ), priority=100 , match=(ip && ip4.dst >>>> == >>>>>> 192.168.0.1), action=(reg0 = 192.168.0.1; ct_dnat;) >>>>>> + table=? (lr_in_defrag ), priority=50 , match=(icmp || icmp6), >>>>>> action=(ct_dnat;) >>>>>> + table=? (lr_in_dnat ), priority=0 , match=(1), >>>> action=(next;) >>>>>> + table=? (lr_in_dnat ), priority=110 , match=(ct.est && >>>> !ct.rel >>>>>> && ip4 && reg0 == 192.168.0.1 && ct_mark.natted == 1), action=(next;) >>>>>> + table=? (lr_in_dnat ), priority=110 , match=(ct.new && >>>> !ct.rel >>>>>> && ip4 && reg0 == 192.168.0.1), >>>> action=(ct_lb_mark(backends=192.168.1.10);) >>>>>> + table=? (lr_in_dnat ), priority=50 , match=(ct.rel && >>>> !ct.est >>>>>> && !ct.new), action=(ct_commit_nat;) >>>>>> + table=? (lr_in_dnat ), priority=70 , match=(ct.rel && >>>> !ct.est >>>>>> && !ct.new && ct_mark.force_snat == 1), action=(flags.force_snat_for_lb >>>> = >>>>>> 1; ct_commit_nat;) >>>>>> + table=? (lr_in_dnat ), priority=70 , match=(ct.rel && >>>> !ct.est >>>>>> && !ct.new && ct_mark.skip_snat == 1), action=(flags.skip_snat_for_lb = >>>> 1; >>>>>> ct_commit_nat;) >>>>>> +]) >>>>>> + >>>>>> +AT_CHECK([grep -e "ls_in_acl" -e "ls_out_acl" lflows0 | grep >>>>>> "priority=65532"], [0], [dnl >>>>>> + table=? (ls_in_acl ), priority=65532, match=(!ct.est && >>>> ct.rel >>>>>> && !ct.new && !ct.inv && ct_mark.blocked == 0), action=(ct_commit_nat;) >>>>>> + table=? (ls_in_acl ), priority=65532, match=(ct.est && >>>> !ct.rel >>>>>> && !ct.new && !ct.inv && ct.rpl && ct_mark.blocked == 0), >>>> action=(reg0[[9]] >>>>>> = 0; reg0[[10]] = 0; next;) >>>>>> + table=? (ls_in_acl ), priority=65532, match=(ct.inv || >>>> (ct.est >>>>>> && ct.rpl && ct_mark.blocked == 1)), action=(drop;) >>>>>> + table=? (ls_in_acl ), priority=65532, match=(nd || nd_ra || >>>>>> nd_rs || mldv1 || mldv2), action=(next;) >>>>>> + table=? (ls_out_acl ), priority=65532, match=(!ct.est && >>>> ct.rel >>>>>> && !ct.new && !ct.inv && ct_mark.blocked == 0), action=(ct_commit_nat;) >>>>>> + table=? (ls_out_acl ), priority=65532, match=(ct.est && >>>> !ct.rel >>>>>> && !ct.new && !ct.inv && ct.rpl && ct_mark.blocked == 0), action=(next;) >>>>>> + table=? (ls_out_acl ), priority=65532, match=(ct.inv || >>>> (ct.est >>>>>> && ct.rpl && ct_mark.blocked == 1)), action=(drop;) >>>>>> + table=? (ls_out_acl ), priority=65532, match=(nd || nd_ra || >>>>>> nd_rs || mldv1 || mldv2), action=(next;) >>>>>> +]) >>>>>> + >>>>>> + >>>>>> +AS_BOX([Chassis registered that doesn't support CT related]) >>>>>> +check ovn-sbctl chassis-add hv geneve 127.0.0.1 >>>>>> +check ovn-nbctl --wait=sb sync >>>>>> +ovn-sbctl dump-flows | DUMP_FLOWS_SORTED > lflows1 >>>>>> + >>>>>> +AT_CHECK([grep -e "lr_in_defrag" -e "lr_in_dnat" lflows1], [0], [dnl >>>>>> + table=? (lr_in_defrag ), priority=0 , match=(1), >>>> action=(next;) >>>>>> + table=? (lr_in_defrag ), priority=100 , match=(ip && ip4.dst >>>> == >>>>>> 192.168.0.1), action=(reg0 = 192.168.0.1; ct_dnat;) >>>>>> + table=? (lr_in_dnat ), priority=0 , match=(1), >>>> action=(next;) >>>>>> + table=? (lr_in_dnat ), priority=110 , match=(ct.est && >>>> !ct.rel >>>>>> && ip4 && reg0 == 192.168.0.1 && ct_label.natted == 1), action=(next;) >>>>>> + table=? (lr_in_dnat ), priority=110 , match=(ct.new && >>>> !ct.rel >>>>>> && ip4 && reg0 == 192.168.0.1), action=(ct_lb(backends=192.168.1.10);) >>>>>> +]) >>>>>> + >>>>>> +AT_CHECK([grep -e "ls_in_acl" -e "ls_out_acl" lflows1 | grep >>>>>> "priority=65532"], [0], [dnl >>>>>> + table=? (ls_in_acl ), priority=65532, match=(!ct.est && >>>> ct.rel >>>>>> && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) >>>>>> + table=? (ls_in_acl ), priority=65532, match=(ct.est && >>>> !ct.rel >>>>>> && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), >>>>>> action=(reg0[[9]] = 0; reg0[[10]] = 0; next;) >>>>>> + table=? (ls_in_acl ), priority=65532, match=(ct.inv || >>>> (ct.est >>>>>> && ct.rpl && ct_label.blocked == 1)), action=(drop;) >>>>>> + table=? (ls_in_acl ), priority=65532, match=(nd || nd_ra || >>>>>> nd_rs || mldv1 || mldv2), action=(next;) >>>>>> + table=? (ls_out_acl ), priority=65532, match=(!ct.est && >>>> ct.rel >>>>>> && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) >>>>>> + table=? (ls_out_acl ), priority=65532, match=(ct.est && >>>> !ct.rel >>>>>> && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), >>>> action=(next;) >>>>>> + table=? (ls_out_acl ), priority=65532, match=(ct.inv || >>>> (ct.est >>>>>> && ct.rpl && ct_label.blocked == 1)), action=(drop;) >>>>>> + table=? (ls_out_acl ), priority=65532, match=(nd || nd_ra || >>>>>> nd_rs || mldv1 || mldv2), action=(next;) >>>>>> +]) >>>>>> + >>>>>> +AS_BOX([Chassis upgrades and supports CT related]) >>>>>> +check ovn-sbctl set chassis hv other_config:ct-no-masked-label=true >>>>>> +check ovn-sbctl set chassis hv other_config:ovn-ct-lb-related=true >>>>>> +check ovn-nbctl --wait=sb sync >>>>>> +ovn-sbctl dump-flows | DUMP_FLOWS_SORTED > lflows2 >>>>>> + >>>>>> +AT_CHECK([grep -e "lr_in_defrag" -e "lr_in_dnat" lflows2], [0], [dnl >>>>>> + table=? (lr_in_defrag ), priority=0 , match=(1), >>>> action=(next;) >>>>>> + table=? (lr_in_defrag ), priority=100 , match=(ip && ip4.dst >>>> == >>>>>> 192.168.0.1), action=(reg0 = 192.168.0.1; ct_dnat;) >>>>>> + table=? (lr_in_defrag ), priority=50 , match=(icmp || icmp6), >>>>>> action=(ct_dnat;) >>>>>> + table=? (lr_in_dnat ), priority=0 , match=(1), >>>> action=(next;) >>>>>> + table=? (lr_in_dnat ), priority=110 , match=(ct.est && >>>> !ct.rel >>>>>> && ip4 && reg0 == 192.168.0.1 && ct_mark.natted == 1), action=(next;) >>>>>> + table=? (lr_in_dnat ), priority=110 , match=(ct.new && >>>> !ct.rel >>>>>> && ip4 && reg0 == 192.168.0.1), >>>> action=(ct_lb_mark(backends=192.168.1.10);) >>>>>> + table=? (lr_in_dnat ), priority=50 , match=(ct.rel && >>>> !ct.est >>>>>> && !ct.new), action=(ct_commit_nat;) >>>>>> + table=? (lr_in_dnat ), priority=70 , match=(ct.rel && >>>> !ct.est >>>>>> && !ct.new && ct_mark.force_snat == 1), action=(flags.force_snat_for_lb >>>> = >>>>>> 1; ct_commit_nat;) >>>>>> + table=? (lr_in_dnat ), priority=70 , match=(ct.rel && >>>> !ct.est >>>>>> && !ct.new && ct_mark.skip_snat == 1), action=(flags.skip_snat_for_lb = >>>> 1; >>>>>> ct_commit_nat;) >>>>>> +]) >>>>>> + >>>>>> +AT_CHECK([grep -e "ls_in_acl" -e "ls_out_acl" lflows2 | grep >>>>>> "priority=65532"], [0], [dnl >>>>>> + table=? (ls_in_acl ), priority=65532, match=(!ct.est && >>>> ct.rel >>>>>> && !ct.new && !ct.inv && ct_mark.blocked == 0), action=(ct_commit_nat;) >>>>>> + table=? (ls_in_acl ), priority=65532, match=(ct.est && >>>> !ct.rel >>>>>> && !ct.new && !ct.inv && ct.rpl && ct_mark.blocked == 0), >>>> action=(reg0[[9]] >>>>>> = 0; reg0[[10]] = 0; next;) >>>>>> + table=? (ls_in_acl ), priority=65532, match=(ct.inv || >>>> (ct.est >>>>>> && ct.rpl && ct_mark.blocked == 1)), action=(drop;) >>>>>> + table=? (ls_in_acl ), priority=65532, match=(nd || nd_ra || >>>>>> nd_rs || mldv1 || mldv2), action=(next;) >>>>>> + table=? (ls_out_acl ), priority=65532, match=(!ct.est && >>>> ct.rel >>>>>> && !ct.new && !ct.inv && ct_mark.blocked == 0), action=(ct_commit_nat;) >>>>>> + table=? (ls_out_acl ), priority=65532, match=(ct.est && >>>> !ct.rel >>>>>> && !ct.new && !ct.inv && ct.rpl && ct_mark.blocked == 0), action=(next;) >>>>>> + table=? (ls_out_acl ), priority=65532, match=(ct.inv || >>>> (ct.est >>>>>> && ct.rpl && ct_mark.blocked == 1)), action=(drop;) >>>>>> + table=? (ls_out_acl ), priority=65532, match=(nd || nd_ra || >>>>>> nd_rs || mldv1 || mldv2), action=(next;) >>>>>> +]) >>>>>> + >>>>>> +AT_CLEANUP >>>>>> +]) >>>>>> -- >>>>>> 2.39.0 >>>>>> >>>>>> >>>>> Just to make it clear, we want to backport the "ICMP related" series [0] >>>>> down to 21.12. This is needed so we are backward compatible with older >>>>> controllers on those branches. >>>>> Also that being said, we are missing Reported-at: tag for this patch. >>>>> >>>> >>>> Thanks for the explanation, Ales! The code looks OK to me but I have >>>> one complaint: the feature we're actually guarding is "ct_commit_nat". >>>> It happens to be used for LB related traffic. >>>> >>>> I think I'd change the feature name to OVN_FEATURE_CT_COMMIT_NAT or >>>> similar. >>>> >>>> What do you think? >>>> >>>> Thanks, >>>> Dumitru >>>> >>>> >>> It's a bit tricky. We are actually guarding both ct_commit_nat, but also >>> the support of skip_snat, >>> force_snat in ct_lb/ct_lb_mark commands. So that's the reason why it's not >>> called ct_commit_nat. >>> >> >> Ah, you're right. That's a bit unfortunate, I guess. Hoping that we >> don't have to backport more features in the future: >> >> Acked-by: Dumitru Ceara <[email protected]> > > Acked-by: Numan Siddique <[email protected]> > > One question before applying - Does this patch require updating the > documentation in ovn-northd.8.xml ? > > I'm not sure what we are doing for existing feature flags. >
Until now we only documented the lflows that get generated if the feature is supported. I'm afraid it's too much noise if we document the backwards compatible version too. What do you think? Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
