On Tue, Mar 12, 2024 at 9:18 PM Martin Kalcok <martin.kal...@canonical.com> wrote:
Hi Martin, sorry for the late reply. Following up on the comments from v1. > > @amusil You were right that the struct in actions.h was not necessary > then. However I also noticed that I forgot to modify `format_CT_COMMIT_V1` > function and for that I think the struct is necessary. I need to > distinguish whether the `ct_commit` action was called with dnat, snat, or > without any argument to format it properly. If you still don't like it, I > can try to figure out how to do it without the struct, but I couldn't > figure out an obvious solution, so I left it in there in this version. > I still think we should basically remove any other functionality from ct_commit_v1 and leave just those two options. > > Regarding the action formatting unit tests, I have two > assumptions/questions: > 1. There's no way to distinguish router/switch datapaths in these tests. I > saw that both `ct_commit_nat(dnat)` and `ct_commit_nat(snat)` [0] expect to > encode into the same zone, although they'd output different zones if they > were used in LR datapath. > Yeah that's correct, this is limitation/intention of the way we encode the actions in the test-ovn.c. > 2. When action formats into identical string as was its input (e.g. > "ct_commit(snat)" -> "ct_commit(snat)"), the test should not use "format > as" check, otherwise it fails. (This one took me a while to figure out, as > it was not obvious from the testlog why it was failing) > That is correct and a little confusing, if the formatting is the same as the original input the test utility doesn't produce the output again. > > Are these correct? > > @numans I though a lot about your suggestions for different action names, > but I think that current "ct_commit(snat/dnat)" fits semantically the most. > Brand new actions would share pretty much all of the code with current > "ct_commit_v1" handling. So to address your comments regarding the backward > compatibility, I added new feature flag, following Ales' approach in [1]. I > believe that this should avoid problems of backward incompatibility in > cases when northd is upgraded but controller is not. > I don't think the plan is to backport those or is it? In case of this being considered as a feature we don't need the feature flag, but that depends on decision from maintainers. > > @0-day Robot: I forgot to run checkpatch.py, my bad. However the only > problem is 81 char line in ovn-sb.xml and there are already many lines that > go over this limit. Should I create v3 if this turns out to be the only > modification needed? > That's fine, as you said there are instances where it is over the 79 limit. > > [0] > https://github.com/ovn-org/ovn/blob/b92ad9e0b408a202273d69ba573f2538e53c6e48/tests/ovn.at#L1500-L1511 > [1] > https://github.com/ovn-org/ovn/commit/43f741c2f029a68a11436e5b14c5bbda6e207dd3#diff-ca917e7415d06776f8ee2baf6102a866c5c31f998e4df93ff8eaa246b65e1da2 > > On Tue, Mar 12, 2024 at 8:45 PM Martin Kalcok <martin.kal...@canonical.com> > wrote: > >> Action `ct_commit` currently does not allow specifying zone into >> which connection is committed. For example, in LR datapath, the >> `ct_commit` >> will always use the DNAT zone. >> >> This change adds option to use `ct_commit(snat)` or `ct_commit(dnat)` to >> explicitly specify the zone into which the connection will be committed. >> It also comes with new feature flag OVN_FEATURE_CT_COMMIT_TO_ZONE to avoid >> incompatibility between northd and controller in case when controller does >> not suport these actions. >> >> Original behavior of `ct_commit` without the arguments remains unchanged. >> >> Signed-off-by: Martin Kalcok <martin.kal...@canonical.com> >> --- >> controller/chassis.c | 8 ++++++++ >> include/ovn/actions.h | 9 +++++++++ >> include/ovn/features.h | 1 + >> lib/actions.c | 29 ++++++++++++++++++++++++++++- >> northd/en-global-config.c | 10 ++++++++++ >> northd/en-global-config.h | 1 + >> ovn-sb.xml | 10 ++++++++++ >> tests/ovn.at | 7 +++++++ >> 8 files changed, 74 insertions(+), 1 deletion(-) >> >> diff --git a/controller/chassis.c b/controller/chassis.c >> index ad75df288..9bb2eba95 100644 >> --- a/controller/chassis.c >> +++ b/controller/chassis.c >> @@ -371,6 +371,7 @@ chassis_build_other_config(const struct >> ovs_chassis_cfg *ovs_cfg, >> smap_replace(config, OVN_FEATURE_FDB_TIMESTAMP, "true"); >> smap_replace(config, OVN_FEATURE_LS_DPG_COLUMN, "true"); >> smap_replace(config, OVN_FEATURE_CT_COMMIT_NAT_V2, "true"); >> + smap_replace(config, OVN_FEATURE_CT_COMMIT_TO_ZONE, "true"); >> } >> >> /* >> @@ -516,6 +517,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_COMMIT_TO_ZONE, >> + false)) { >> + return true; >> + } >> + >> return false; >> } >> >> @@ -648,6 +655,7 @@ update_supported_sset(struct sset *supported) >> sset_add(supported, OVN_FEATURE_FDB_TIMESTAMP); >> sset_add(supported, OVN_FEATURE_LS_DPG_COLUMN); >> sset_add(supported, OVN_FEATURE_CT_COMMIT_NAT_V2); >> + sset_add(supported, OVN_FEATURE_CT_COMMIT_TO_ZONE); >> } >> >> static void >> diff --git a/include/ovn/actions.h b/include/ovn/actions.h >> index 49fb96fc6..ce9597cf5 100644 >> --- a/include/ovn/actions.h >> +++ b/include/ovn/actions.h >> @@ -259,11 +259,20 @@ struct ovnact_ct_next { >> uint8_t ltable; /* Logical table ID of next table. */ >> }; >> >> +/* Conntrack zone to be used for commiting CT entries by the action. >> + * DEFAULT uses default zone for given datapath. */ >> +enum ovnact_ct_zone { >> + OVNACT_CT_ZONE_DEFAULT, >> + OVNACT_CT_ZONE_SNAT, >> + OVNACT_CT_ZONE_DNAT, >> +}; >> + >> /* OVNACT_CT_COMMIT_V1. */ >> struct ovnact_ct_commit_v1 { >> struct ovnact ovnact; >> uint32_t ct_mark, ct_mark_mask; >> ovs_be128 ct_label, ct_label_mask; >> + enum ovnact_ct_zone zone; >> }; >> >> /* Type of NAT used for the particular action. >> diff --git a/include/ovn/features.h b/include/ovn/features.h >> index 08f1d8288..35a5d8ba0 100644 >> --- a/include/ovn/features.h >> +++ b/include/ovn/features.h >> @@ -28,6 +28,7 @@ >> #define OVN_FEATURE_FDB_TIMESTAMP "fdb-timestamp" >> #define OVN_FEATURE_LS_DPG_COLUMN "ls-dpg-column" >> #define OVN_FEATURE_CT_COMMIT_NAT_V2 "ct-commit-nat-v2" >> +#define OVN_FEATURE_CT_COMMIT_TO_ZONE "ct-commit-to-zone" >> >> /* OVS datapath supported features. Based on availability OVN might >> generate >> * different types of openflows. >> diff --git a/lib/actions.c b/lib/actions.c >> index a45874dfb..9e27a68a5 100644 >> --- a/lib/actions.c >> +++ b/lib/actions.c >> @@ -707,6 +707,7 @@ static void >> parse_ct_commit_v1_arg(struct action_context *ctx, >> struct ovnact_ct_commit_v1 *cc) >> { >> + cc->zone = OVNACT_CT_ZONE_DEFAULT; >> if (lexer_match_id(ctx->lexer, "ct_mark")) { >> if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) { >> return; >> @@ -737,6 +738,10 @@ parse_ct_commit_v1_arg(struct action_context *ctx, >> return; >> } >> lexer_get(ctx->lexer); >> + } else if (lexer_match_id(ctx->lexer, "snat")) { >> + cc->zone = OVNACT_CT_ZONE_SNAT; >> + } else if (lexer_match_id(ctx->lexer, "dnat")) { >> + cc->zone = OVNACT_CT_ZONE_DNAT; >> } else { >> lexer_syntax_error(ctx->lexer, NULL); >> } >> @@ -800,6 +805,18 @@ format_CT_COMMIT_V1(const struct ovnact_ct_commit_v1 >> *cc, struct ds *s) >> ds_put_hex(s, &cc->ct_label_mask, sizeof cc->ct_label_mask); >> } >> } >> + if (cc->zone != OVNACT_CT_ZONE_DEFAULT) { >> + if (ds_last(s) != '(') { >> + ds_put_cstr(s, ", "); >> + } >> + >> + if (cc->zone == OVNACT_CT_ZONE_SNAT) { >> + ds_put_cstr(s, "snat"); >> + } else if (cc->zone == OVNACT_CT_ZONE_DNAT) { >> + ds_put_cstr(s, "dnat"); >> + } >> + } >> + >> if (!ds_chomp(s, '(')) { >> ds_put_char(s, ')'); >> } >> @@ -814,7 +831,17 @@ encode_CT_COMMIT_V1(const struct ovnact_ct_commit_v1 >> *cc, >> struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); >> ct->flags = NX_CT_F_COMMIT; >> ct->recirc_table = NX_CT_RECIRC_NONE; >> - ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE); >> + >> + if (ep->is_switch) { >> + ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE); >> + } else { >> + if (cc->zone == OVNACT_CT_ZONE_SNAT) { >> + ct->zone_src.field = mf_from_id(MFF_LOG_SNAT_ZONE); >> + } else { >> + ct->zone_src.field = mf_from_id(MFF_LOG_DNAT_ZONE); >> + } >> + } >> + >> ct->zone_src.ofs = 0; >> ct->zone_src.n_bits = 16; >> >> diff --git a/northd/en-global-config.c b/northd/en-global-config.c >> index 34e393b33..193deaebc 100644 >> --- a/northd/en-global-config.c >> +++ b/northd/en-global-config.c >> @@ -370,6 +370,7 @@ northd_enable_all_features(struct >> ed_type_global_config *data) >> .fdb_timestamp = true, >> .ls_dpg_column = true, >> .ct_commit_nat_v2 = true, >> + .ct_commit_to_zone = true, >> }; >> } >> >> @@ -439,6 +440,15 @@ build_chassis_features(const struct >> sbrec_chassis_table *sbrec_chassis_table, >> chassis_features->ct_commit_nat_v2) { >> chassis_features->ct_commit_nat_v2 = false; >> } >> + >> + bool ct_commit_to_zone = >> + smap_get_bool(&chassis->other_config, >> + OVN_FEATURE_CT_COMMIT_TO_ZONE, >> + false); >> + if (!ct_commit_to_zone && >> + chassis_features->ct_commit_to_zone) { >> + chassis_features->ct_commit_to_zone = false; >> + } >> } >> } >> >> diff --git a/northd/en-global-config.h b/northd/en-global-config.h >> index 38d732808..842bcee70 100644 >> --- a/northd/en-global-config.h >> +++ b/northd/en-global-config.h >> @@ -20,6 +20,7 @@ struct chassis_features { >> bool fdb_timestamp; >> bool ls_dpg_column; >> bool ct_commit_nat_v2; >> + bool ct_commit_to_zone; >> }; >> >> struct global_config_tracked_data { >> diff --git a/ovn-sb.xml b/ovn-sb.xml >> index ac4e585f2..f5f2208da 100644 >> --- a/ovn-sb.xml >> +++ b/ovn-sb.xml >> @@ -1405,6 +1405,8 @@ >> <dt><code>ct_commit { ct_mark=<var>value[/mask]</var>; >> };</code></dt> >> <dt><code>ct_commit { ct_label=<var>value[/mask]</var>; >> };</code></dt> >> <dt><code>ct_commit { ct_mark=<var>value[/mask]</var>; >> ct_label=<var>value[/mask]</var>; };</code></dt> >> + <dt><code>ct_commit(snat);</code></dt> >> + <dt><code>ct_commit(dnat);</code></dt> >> <dd> >> <p> >> Commit the flow to the connection tracking entry associated >> with it >> @@ -1421,6 +1423,14 @@ >> in order to have specific bits set. >> </p> >> >> + <p> >> + In Logical Router Datapath, parameters >> <code>ct_commit(snat)</code> >> + or <code>ct_commit(dnat) </code> can be used to explicitly >> specify >> + into which zone should be connection committed. Without this >> + parameter, the connection is committed to the default zone >> for the >> + Datapath. These parameters have no effect in Logical Switch >> Datapath. >> + </p> >> + >> <p> >> Note that if you want processing to continue in the next >> table, >> you must execute the <code>next</code> action after >> diff --git a/tests/ovn.at b/tests/ovn.at >> index d26c95054..11e6430ed 100644 >> --- a/tests/ovn.at >> +++ b/tests/ovn.at >> @@ -1349,6 +1349,13 @@ ct_commit(ct_label=18446744073709551615); >> ct_commit(ct_label=18446744073709551616); >> Decimal constants must be less than 2**64. >> >> +ct_commit(dnat); >> + encodes as ct(commit,zone=NXM_NX_REG13[0..15]) >> + has prereqs ip >> +ct_commit(snat); >> + encodes as ct(commit,zone=NXM_NX_REG13[0..15]) >> + has prereqs ip >> + >> ct_mark = 12345 >> Field ct_mark is not modifiable. >> ct_label = 0xcafe >> -- >> 2.40.1 >> >> > > -- > Best Regards, > Martin Kalcok. > In general this patch looks fine, however there we still require to make the decision around ct_commit_v1. Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> amu...@redhat.com <https://red.ht/sig> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
