Thanks for the feedback Ales, Dumitru. I'm back from vacation so I'm again focusing on this change.
On Tue, Apr 2, 2024 at 5:33 PM Dumitru Ceara <dce...@redhat.com> wrote: > On 3/20/24 12:20, Ales Musil wrote: > > On Tue, Mar 12, 2024 at 9:18 PM Martin Kalcok < > martin.kal...@canonical.com> > > wrote: > > > > Hi Martin, > > > > Hi Ales, Martin, Numan, > > > sorry for the late reply. > > > > Sorry for jumping in late in the discussion. > > > 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. > > > > Right, in theory this shouldn't be backported so unless there's a strong > case for it to be backported we don't really need the feature flags. > My main motivation for using the feature flag was to avoid problems in a scenario where northd gets upgraded to a new version before controller does. Without the feature flag, in that scenario, northd would start using new action that controller would not understand. With the feature flag, northd would use this new action only if the controller supports it, and fall back to the old behavior if it does not. Is there a way to achieve this without feature flag? Or is it a non-issue? > > > > >> > >> @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. > > > > I replied on the other thread but, to summarize here too, I agree with > Numan. I think it's more clear if we add new actions and just remove > the old one. > > Regards, > Dumitru > > -- Best Regards, Martin Kalcok. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
