| Message ID | 20240312194426.88840-1-martin.kalcok@canonical.com |
|---|---|
| State | Superseded |
| Delegated to: | Dumitru Ceara |
| Headers | show |
| Series | [ovs-dev,v2,1/2] actions: Enable specifying zone for ct_commit. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Bleep bloop. Greetings Martin Kalcok, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line is 81 characters long (recommended limit is 79)
#223 FILE: ovn-sb.xml:1431:
Datapath. These parameters have no effect in Logical Switch Datapath.
Lines checked: 250, Warnings: 1, Errors: 0
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
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 <martin.kalcok@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.kalcok@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 > >
On Tue, Mar 12, 2024 at 9:17 PM Martin Kalcok <martin.kalcok@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. > @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.kalcok@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.kalcok@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. >
On Tue, Mar 12, 2024 at 9:18 PM Martin Kalcok <martin.kalcok@canonical.com> wrote: Hi Martin, sorry for the late reply. 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. > > @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.kalcok@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.kalcok@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. Thanks, Ales
On 3/13/24 10:27, Martin Kalcok wrote: > On Tue, Mar 12, 2024 at 9:17 PM Martin Kalcok <martin.kalcok@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-dceara@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.kalcok@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.kalcok@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. >> > >
On 3/20/24 12:20, Ales Musil wrote: > On Tue, Mar 12, 2024 at 9:18 PM Martin Kalcok <martin.kalcok@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. > >> >> @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.kalcok@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.kalcok@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
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 <dceara@redhat.com> wrote: > On 3/20/24 12:20, Ales Musil wrote: > > On Tue, Mar 12, 2024 at 9:18 PM Martin Kalcok < > martin.kalcok@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.kalcok@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.kalcok@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 > >
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
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.kalcok@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(-)