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. @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 <[email protected]> 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 <[email protected]> > --- > 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
