Thanks for another round of review. On Wed, Apr 17, 2024 at 1:45 PM Ales Musil <[email protected]> wrote:
> > > 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. > Just to make sure I understand this correctly Ales, you'd like me to rename the `ovnact_ct_commit_nat` structure and add a field like `bool do_nat` to it. Then adjust current `{parse,encode,format}_CT_COMMIT_NAT` functions to differentiate based on the `do_nat` variable (and presumably rename these functions too, to match the new structure name)? > > >> >> 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. >> > ack > >> >> } >> >> >> >> 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. >> > Oh right, my bad, `ct_snat` or `ct_dnat` are the correct thing to do here (based on which zone you want to use), right? > > >> > >> >> + >> >> + <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. >> > thx, ack. > >> >> + </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 >> > thx, ack. > > >> > >> >> + 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> > -- Best Regards, Martin Kalcok. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
