On Wed, Apr 17, 2024 at 1:05 PM Dumitru Ceara <[email protected]> wrote:
> On 4/17/24 09:04, Ales Musil wrote: > > On Mon, Apr 15, 2024 at 2:15 PM Martin Kalcok < > [email protected]> > > wrote: > > > >> This change adds a new action 'ct_commit_to_zone' that enables users to > >> commit > >> the flow into a specific zone in the connection tracker. > >> > >> A new feature flag, OVN_FEATURE_CT_COMMIT_TO_ZONE, is also included to > >> avoid > >> issues during upgrade in case the northd is upgraded to a version that > >> supports > >> this action before the controller is upgraded. > >> > >> Note that this action is meaningful only in the context of Logical > Router > >> datapath. Logical Switch datapath does not use multiple zones and this > >> action > >> falls back to committing the connection into the default zone for the > >> Logical > >> Switch. > >> > >> Signed-off-by: Martin Kalcok <[email protected]> > >> --- > >> > > > > Hi Martin, > > > > Hi Martin, Ales, > > > thank you for the followup. I have a couple of comments down below. > > > > I have a few of my own. :) > > > > >> controller/chassis.c | 8 ++++++ > >> include/ovn/actions.h | 1 + > >> include/ovn/features.h | 1 + > >> lib/actions.c | 60 +++++++++++++++++++++++++++++++++++++++ > >> northd/en-global-config.c | 11 +++++++ > >> northd/en-global-config.h | 1 + > >> ovn-sb.xml | 24 ++++++++++++++++ > >> tests/ovn.at | 16 +++++++++++ > >> utilities/ovn-trace.c | 1 + > >> 9 files changed, 123 insertions(+) > >> > >> 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 8e794450c..4bcd1f58c 100644 > >> --- a/include/ovn/actions.h > >> +++ b/include/ovn/actions.h > >> @@ -68,6 +68,7 @@ struct collector_set_ids; > >> OVNACT(CT_NEXT, ovnact_ct_next) \ > >> /* CT_COMMIT_V1 is not supported anymore. */ \ > >> OVNACT(CT_COMMIT_V2, ovnact_nest) \ > >> + OVNACT(CT_COMMIT_TO_ZONE, ovnact_ct_commit_nat) \ > >> OVNACT(CT_DNAT, ovnact_ct_nat) \ > >> OVNACT(CT_SNAT, ovnact_ct_nat) \ > >> OVNACT(CT_DNAT_IN_CZONE, ovnact_ct_nat) \ > >> 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 39bb5bc8a..f26817018 100644 > >> --- a/lib/actions.c > >> +++ b/lib/actions.c > >> @@ -791,6 +791,64 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on, > >> ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset); > >> ct = ofpacts->header; > >> ofpact_finish(ofpacts, &ct->ofpact); > >> +} > >> + > >> +static void > >> +parse_CT_COMMIT_TO_ZONE(struct action_context *ctx) > >> +{ > >> + add_prerequisite(ctx, "ip"); > >> + > >> + struct ovnact_ct_commit_nat *ct_commit = > >> + ovnact_put_CT_COMMIT_TO_ZONE(ctx->ovnacts); > >> > > > > The reuse "ovnact_ct_commit_nat" doesn't feel right, it should be renamed > > if anything with a flag indicating whether there should be nat or not. By > > doing that the parse + encoding for ct_commit_nat and ct_commit_to_zone > can > > be the same differentiated by the flag. > > > > Just to be clear, your suggestion is to change the ct_commit_to_zone() > action so that it accepts an optional "nat" argument, right? That would > allow actions like: > > ct_commit_to_zone(snat, nat) > Actually no, it can be done as a follow up as you said however this suggestion was more just to reuse the actions.c functionality we already have for ct_commit_nat. > > Can we do this as a follow up and also update northd to generate flows > with ct_commit_to_zone() instead of ct_commit_nat()? > > > > > >> + > >> + lexer_force_match(ctx->lexer, LEX_T_LPAREN); > >> + > >> + if (lexer_match_id(ctx->lexer, "dnat")) { > >> + ct_commit->dnat_zone = true; > >> + } else if (lexer_match_id(ctx->lexer, "snat")) { > >> + ct_commit->dnat_zone = false; > >> + } else { > >> + lexer_syntax_error(ctx->lexer, "expecting parameter 'dnat' or > >> 'snat'"); > >> + } > >> + > >> + lexer_force_match(ctx->lexer, LEX_T_RPAREN); > >> +} > >> + > >> +static void > >> +format_CT_COMMIT_TO_ZONE(const struct ovnact_ct_commit_nat *ct_commit, > >> + struct ds *s) > >> +{ > >> + ds_put_format(s, "ct_commit_to_zone(%s);", > >> + ct_commit->dnat_zone ? "dnat" : "snat"); > >> + > >> +} > >> + > >> +static void > >> +encode_CT_COMMIT_TO_ZONE(const struct ovnact_ct_commit_nat *ct_commit, > >> + const struct ovnact_encode_params *ep, > >> + struct ofpbuf *ofpacts) > >> +{ > >> + struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); > >> + ct->flags = NX_CT_F_COMMIT; > >> + ct->recirc_table = NX_CT_RECIRC_NONE; > >> > > > > I don't think this action is ever used standalone so we can actually > > include the next table to avoid writing "ct_commit_to_zone(snat); next;". > > > > > > We'd have to update ovn-trace too in that case. I'm a bit on the fence > with this one though. We do have "ct_next;" already and that implicitly > advances to the next table. But I think I prefer being explicit in the > logical flow actions (and adding "; next;" in northd). In any case, not > a must, I'm OK with implicit next too. > Only ct_commit is actually explicit about it, any other will implicitly advance to the next table. > > >> + ct->zone_src.ofs = 0; > >> + ct->zone_src.n_bits = 16; > >> + > >> + if (ep->is_switch) { > >> + ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE); > >> + } else { > >> + ct->zone_src.field = mf_from_id(ct_commit->dnat_zone > >> + ? MFF_LOG_DNAT_ZONE > >> + : MFF_LOG_SNAT_ZONE); > >> + } > >> + > >> + size_t set_field_offset = ofpacts->size; > >> + ofpbuf_pull(ofpacts, set_field_offset); > >> + ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset); > >> + ct = ofpacts->header; > >> + ofpact_finish(ofpacts, &ct->ofpact); > >> > > > > This is the "manual" way of finishing the action encoding. There is a way > > shorter and more explicit way to do it: > > > > ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); > > ofpacts->header = ct; > > ofpact_finish_CT(ofpacts, &ct); > > > > > > > >> + > >> + > > Nit: too many empty lines. > > >> } > >> > >> static void > >> @@ -5306,6 +5364,8 @@ parse_action(struct action_context *ctx) > >> parse_CT_NEXT(ctx); > >> } else if (lexer_match_id(ctx->lexer, "ct_commit")) { > >> parse_CT_COMMIT(ctx); > >> + } else if (lexer_match_id(ctx->lexer, "ct_commit_to_zone")) { > >> + parse_CT_COMMIT_TO_ZONE(ctx); > >> } else if (lexer_match_id(ctx->lexer, "ct_dnat")) { > >> parse_CT_DNAT(ctx); > >> } else if (lexer_match_id(ctx->lexer, "ct_snat")) { > >> diff --git a/northd/en-global-config.c b/northd/en-global-config.c > >> index 34e393b33..842ac5b64 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,16 @@ 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 4c26c6714..0d1f640dd 100644 > >> --- a/ovn-sb.xml > >> +++ b/ovn-sb.xml > >> @@ -1432,6 +1432,30 @@ > >> </p> > >> </dd> > >> > >> + <dt><code>ct_commit_to_zone(dnat);</code></dt> > >> + <dt><code>ct_commit_to_zone(snat);</code></dt> > >> + <dd> > >> + <p> > >> + Commit the flow to the specific zone in the connection > >> tracker. > >> + Similar to the <code>ct_commit</code> action, this action > >> requires > >> + previous call to <code>ct_next</code> to initialize > connection > >> + tracking state. > >> + </p> > >> > > > > ct_next implies only the DNAT zone, which is not correct for SNAT. > > > > > >> + > >> + <p> > >> + If you want processing to continue in the next table, you > must > >> + execute the <code>next</code> action after > >> + <code>ct_commit_to_zone</code>. > >> + </p> > >> > > > > As mentioned above, I don't think we will want to commit without > proceeding. > > > > > >> + > >> + <p> > >> + Note that this action is meaningful only in the Logical > Router > >> + Datapath as the Logical Switch Datapath does not use > separate > >> + connection tracking zones. Using this action in Logical > Switch > >> + Datapath falls back to committing the flow into the > switch's > >> + default zone. > > This is not accurate. It's actually the logical port's conntrack zone. > > >> + </p> > >> + </dd> > >> <dt><code>ct_dnat;</code></dt> > >> <dt><code>ct_dnat(<var>IP</var>);</code></dt> > >> <dd> > >> diff --git a/tests/ovn.at b/tests/ovn.at > >> index c8cc1d37f..cbb303459 100644 > >> --- a/tests/ovn.at > >> +++ b/tests/ovn.at > >> @@ -1316,6 +1316,22 @@ ct_commit { > >> ct_label=0x181716151413121110090807060504030201; }; > >> ct_commit { ip4.dst = 192.168.0.1; }; > >> Field ip4.dst is not modifiable. > >> > >> +# ct_commit_to_zone > >> +ct_commit_to_zone(dnat); > >> + encodes as ct(commit,zone=NXM_NX_REG13[[0..15]]) > >> + has prereqs ip > >> +ct_commit_to_zone(dnat); > >> > > > > s/dnat/snat > > > > > >> + encodes as ct(commit,zone=NXM_NX_REG13[[0..15]]) > >> + has prereqs ip > >> +ct_commit_to_zone; > >> + Syntax error at `;' expecting `('. > >> +ct_commit_to_zone(); > >> + Syntax error at `)' expecting parameter 'dnat' or 'snat'. > >> +ct_commit_to_zone(foo); > >> + Syntax error at `foo' expecting parameter 'dnat' or 'snat'. > >> +ct_commit_to_zone(dnat; > >> + Syntax error at `;' expecting `)'. > >> + > >> # Legact ct_commit_v1 action. > >> ct_commit(); > >> Syntax error at `(' expecting `;'. > >> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c > >> index 5e55fbbcc..554682cd0 100644 > >> --- a/utilities/ovn-trace.c > >> +++ b/utilities/ovn-trace.c > >> @@ -3107,6 +3107,7 @@ trace_actions(const struct ovnact *ovnacts, size_t > >> ovnacts_len, > >> execute_ct_next(ovnact_get_CT_NEXT(a), dp, uflow, pipeline, > >> super); > >> break; > >> > >> + case OVNACT_CT_COMMIT_TO_ZONE: > >> case OVNACT_CT_COMMIT_V2: > >> /* Nothing to do. */ > >> break; > >> -- > >> 2.40.1 > >> > >> > > Thanks, > > Ales > > > > Regards, > Dumitru > > -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
