On 3/13/24 10:27, Martin Kalcok wrote: > On Tue, Mar 12, 2024 at 9:17 PM Martin Kalcok <martin.kal...@canonical.com> > wrote: > >> 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. >> >> 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. >> 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) >> >> 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. >> >> > Sorry to re-iterate on this @numans, I just hope I didn't misunderstood > your original comments on the v1. The way I took it is that you are OK with > using `ct_commit(dnat/snat)` and repurposing the implementation of > `ct_commit_v1`, as long as it doesn't break backwards compatibility. Or do > you think completely new action names with separate implementations are > still needed? I just don't wan't to give impression that I'm ignoring your > suggestions from v1. >
I'm with Numan on this one. I think we shouldn't repurpose an action that's not used anymore. I think we should just delete it from the code base and add new actions. It avoids any potential confusion. I posted a patch to remove the old action. Please let me know what you think. https://patchwork.ozlabs.org/project/ovn/patch/20240402153134.261811-1-dce...@redhat.com/ > >> @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? >> >> [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. >> > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
