| Message ID | 20240304105542.349690-1-martin.kalcok@canonical.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,1/2] actions.c/h: Enable specifying zone for ct_commit. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Mon, Mar 4, 2024 at 11:56 AM 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. > > Original behavior of `ct_commit` without the arguments remains unchanged. > > Signed-off-by: Martin Kalcok <martin.kalcok@canonical.com> > Hi Martin, thank you for the followup, I have a few comments down below. One small nit for the commit subject, we usually specify only the module name without .c/.h e.g. action, ovn-controller, northd etc. AFAIR ct_commit_v1 is deprecated, but I guess it's fine to repurpose it for this. Maybe we could remove the old behavior and leave just this? Before posting v2 we should discuss this with others. Adding Dumitru and Numan to this discussion. > --- > include/ovn/actions.h | 9 +++++++++ > lib/actions.c | 20 +++++++++++++++++++- > ovn-sb.xml | 9 +++++++++ > 3 files changed, 37 insertions(+), 1 deletion(-) > > 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, > +}; > + > There is no need for this enum, see details below. > /* 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/lib/actions.c b/lib/actions.c > index a45874dfb..319e65b6f 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); > } > @@ -814,7 +819,20 @@ 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); > + switch (cc->zone) { > + case OVNACT_CT_ZONE_SNAT: > + ct->zone_src.field = mf_from_id(MFF_LOG_SNAT_ZONE); > + break; > + > + case OVNACT_CT_ZONE_DNAT: > + ct->zone_src.field = mf_from_id(MFF_LOG_DNAT_ZONE); > + break; > + > + case OVNACT_CT_ZONE_DEFAULT: > + default: > + ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE); > + break; > + } > This would actually break potential use in the switch pipeline when someone would specify ct_commit(snat)/ct_commit(dnat). The switch pipeline uses MFF_LOG_CT_ZONE only. See ct_commit_nat for example [0]. There is only indication if it's snat or dnat and check for switch/router pipeline. ct->zone_src.ofs = 0; > ct->zone_src.n_bits = 16; > > diff --git a/ovn-sb.xml b/ovn-sb.xml > index ac4e585f2..66cb9747d 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,13 @@ > in order to have specific bits set. > </p> > > + <p> > + 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. > + </p> > + > <p> > Note that if you want processing to continue in the next > table, > you must execute the <code>next</code> action after > -- > 2.40.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > There is a test cae missing for this in "action parsing" test [1]. Thanks, Ales [0] https://github.com/ovn-org/ovn/blob/b92ad9e0b408a202273d69ba573f2538e53c6e48/lib/actions.c#L1219-L1225 [1] https://github.com/ovn-org/ovn/blob/b92ad9e0b408a202273d69ba573f2538e53c6e48/tests/ovn.at#L1308
Hi Ales, Thank you for review and helpful comments. I'll update commit subjects for this series in v2. On Wed, Mar 6, 2024 at 7:45 AM Ales Musil <amusil@redhat.com> wrote: > > > On Mon, Mar 4, 2024 at 11:56 AM 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. >> >> Original behavior of `ct_commit` without the arguments remains unchanged. >> >> Signed-off-by: Martin Kalcok <martin.kalcok@canonical.com> >> > > Hi Martin, > > thank you for the followup, I have a few comments down below. > One small nit for the commit subject, we usually specify only the module > name without .c/.h e.g. action, ovn-controller, northd etc. > AFAIR ct_commit_v1 is deprecated, but I guess it's fine to repurpose it > for this. Maybe we could remove the old behavior and leave just this? > Before posting v2 we should discuss this with others. > Adding Dumitru and Numan to this discussion. > Ack, I'll defer to your recommendation as to where this should be implemented. If I read it correctly, `parse_ct_commit_v1_arg` still has code to parse mark/label options but it's never used because if `ct_commit` action is followed by "{", the `parse_CT_COMMIT` will send it to `parse_nested_action` instead. If that's the case and the `parse_CT_COMMIT_V1` is unused, I'd be happy to remove it/repurpose it to something like `parse_CT_COMMIT_ZONE` that handles only `ct_commit({snat,dnat})` actions. > >> --- >> include/ovn/actions.h | 9 +++++++++ >> lib/actions.c | 20 +++++++++++++++++++- >> ovn-sb.xml | 9 +++++++++ >> 3 files changed, 37 insertions(+), 1 deletion(-) >> >> 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, >> +}; >> + >> > > There is no need for this enum, see details below. > > >> /* 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/lib/actions.c b/lib/actions.c >> index a45874dfb..319e65b6f 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); >> } >> @@ -814,7 +819,20 @@ 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); >> + switch (cc->zone) { >> + case OVNACT_CT_ZONE_SNAT: >> + ct->zone_src.field = mf_from_id(MFF_LOG_SNAT_ZONE); >> + break; >> + >> + case OVNACT_CT_ZONE_DNAT: >> + ct->zone_src.field = mf_from_id(MFF_LOG_DNAT_ZONE); >> + break; >> + >> + case OVNACT_CT_ZONE_DEFAULT: >> + default: >> + ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE); >> + break; >> + } >> > > This would actually break potential use in the switch pipeline when > someone would specify ct_commit(snat)/ct_commit(dnat). > The switch pipeline uses MFF_LOG_CT_ZONE only. See ct_commit_nat for > example [0]. There is only indication if it's snat or dnat and check for > switch/router pipeline. > I see. I did not quite understood that the MFF_LOG_CT_ZONE is there for the switch, thanks, I'll copy the behavior from `encode_CT_COMMIT_NAT` and I'll use `bool dnat_zone` instead of the enum. > ct->zone_src.ofs = 0; >> ct->zone_src.n_bits = 16; >> >> diff --git a/ovn-sb.xml b/ovn-sb.xml >> index ac4e585f2..66cb9747d 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,13 @@ >> in order to have specific bits set. >> </p> >> >> + <p> >> + 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. >> + </p> >> + >> <p> >> Note that if you want processing to continue in the next >> table, >> you must execute the <code>next</code> action after >> -- >> 2.40.1 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > There is a test cae missing for this in "action parsing" test [1]. > ack. > > Thanks, > Ales > > [0] > https://github.com/ovn-org/ovn/blob/b92ad9e0b408a202273d69ba573f2538e53c6e48/lib/actions.c#L1219-L1225 > [1] > https://github.com/ovn-org/ovn/blob/b92ad9e0b408a202273d69ba573f2538e53c6e48/tests/ovn.at#L1308 > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA <https://www.redhat.com> > > amusil@redhat.com > <https://red.ht/sig> >
On Wed, Mar 6, 2024 at 7:26 AM Martin Kalcok <martin.kalcok@canonical.com> wrote: > Hi Ales, > Thank you for review and helpful comments. I'll update commit subjects for > this series in v2. > > On Wed, Mar 6, 2024 at 7:45 AM Ales Musil <amusil@redhat.com> wrote: > >> >> >> On Mon, Mar 4, 2024 at 11:56 AM 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. >>> >>> Original behavior of `ct_commit` without the arguments remains unchanged. >>> >>> Signed-off-by: Martin Kalcok <martin.kalcok@canonical.com> >>> >> >> Hi Martin, >> >> thank you for the followup, I have a few comments down below. >> One small nit for the commit subject, we usually specify only the module >> name without .c/.h e.g. action, ovn-controller, northd etc. >> AFAIR ct_commit_v1 is deprecated, but I guess it's fine to repurpose it >> for this. Maybe we could remove the old behavior and leave just this? >> Before posting v2 we should discuss this with others. >> Adding Dumitru and Numan to this discussion. >> > > Ack, I'll defer to your recommendation as to where this should be > implemented. If I read it correctly, `parse_ct_commit_v1_arg` still has > code to parse mark/label options but it's never used because if `ct_commit` > action is followed by "{", the `parse_CT_COMMIT` will send it to > `parse_nested_action` instead. If that's the case and the > `parse_CT_COMMIT_V1` is unused, I'd be happy to remove it/repurpose it to > something like `parse_CT_COMMIT_ZONE` that handles only > `ct_commit({snat,dnat})` actions. > Instead of modifying ct_commit, I'd suggest adding 2 new actions - ct_commit_snat and ct_commit_dnat. This would not have any ambiguity on the backward compatibility. Thanks Numan > >> >>> --- >>> include/ovn/actions.h | 9 +++++++++ >>> lib/actions.c | 20 +++++++++++++++++++- >>> ovn-sb.xml | 9 +++++++++ >>> 3 files changed, 37 insertions(+), 1 deletion(-) >>> >>> 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, >>> +}; >>> + >>> >> >> There is no need for this enum, see details below. >> >> >>> /* 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/lib/actions.c b/lib/actions.c >>> index a45874dfb..319e65b6f 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); >>> } >>> @@ -814,7 +819,20 @@ 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); >>> + switch (cc->zone) { >>> + case OVNACT_CT_ZONE_SNAT: >>> + ct->zone_src.field = mf_from_id(MFF_LOG_SNAT_ZONE); >>> + break; >>> + >>> + case OVNACT_CT_ZONE_DNAT: >>> + ct->zone_src.field = mf_from_id(MFF_LOG_DNAT_ZONE); >>> + break; >>> + >>> + case OVNACT_CT_ZONE_DEFAULT: >>> + default: >>> + ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE); >>> + break; >>> + } >>> >> >> This would actually break potential use in the switch pipeline when >> someone would specify ct_commit(snat)/ct_commit(dnat). >> The switch pipeline uses MFF_LOG_CT_ZONE only. See ct_commit_nat for >> example [0]. There is only indication if it's snat or dnat and check for >> switch/router pipeline. >> > > I see. I did not quite understood that the MFF_LOG_CT_ZONE is there for > the switch, thanks, I'll copy the behavior from `encode_CT_COMMIT_NAT` and > I'll use `bool dnat_zone` instead of the enum. > > >> ct->zone_src.ofs = 0; >>> ct->zone_src.n_bits = 16; >>> >>> diff --git a/ovn-sb.xml b/ovn-sb.xml >>> index ac4e585f2..66cb9747d 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,13 @@ >>> in order to have specific bits set. >>> </p> >>> >>> + <p> >>> + 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. >>> + </p> >>> + >>> <p> >>> Note that if you want processing to continue in the next >>> table, >>> you must execute the <code>next</code> action after >>> -- >>> 2.40.1 >>> >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >>> >> There is a test cae missing for this in "action parsing" test [1]. >> > > ack. > > >> >> Thanks, >> Ales >> >> [0] >> https://github.com/ovn-org/ovn/blob/b92ad9e0b408a202273d69ba573f2538e53c6e48/lib/actions.c#L1219-L1225 >> [1] >> https://github.com/ovn-org/ovn/blob/b92ad9e0b408a202273d69ba573f2538e53c6e48/tests/ovn.at#L1308 >> -- >> >> Ales Musil >> >> Senior Software Engineer - OVN Core >> >> Red Hat EMEA <https://www.redhat.com> >> >> amusil@redhat.com >> <https://red.ht/sig> >> > > > -- > Best Regards, > Martin Kalcok. >
On Thu, Mar 7, 2024 at 1:26 AM Numan Siddique <numans@ovn.org> wrote: > > > On Wed, Mar 6, 2024 at 7:26 AM Martin Kalcok <martin.kalcok@canonical.com> > wrote: > >> Hi Ales, >> Thank you for review and helpful comments. I'll update commit subjects >> for this series in v2. >> >> On Wed, Mar 6, 2024 at 7:45 AM Ales Musil <amusil@redhat.com> wrote: >> >>> >>> >>> On Mon, Mar 4, 2024 at 11:56 AM 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. >>>> >>>> Original behavior of `ct_commit` without the arguments remains >>>> unchanged. >>>> >>>> Signed-off-by: Martin Kalcok <martin.kalcok@canonical.com> >>>> >>> >>> Hi Martin, >>> >>> thank you for the followup, I have a few comments down below. >>> One small nit for the commit subject, we usually specify only the module >>> name without .c/.h e.g. action, ovn-controller, northd etc. >>> AFAIR ct_commit_v1 is deprecated, but I guess it's fine to repurpose it >>> for this. Maybe we could remove the old behavior and leave just this? >>> Before posting v2 we should discuss this with others. >>> Adding Dumitru and Numan to this discussion. >>> >> >> Ack, I'll defer to your recommendation as to where this should be >> implemented. If I read it correctly, `parse_ct_commit_v1_arg` still has >> code to parse mark/label options but it's never used because if `ct_commit` >> action is followed by "{", the `parse_CT_COMMIT` will send it to >> `parse_nested_action` instead. If that's the case and the >> `parse_CT_COMMIT_V1` is unused, I'd be happy to remove it/repurpose it to >> something like `parse_CT_COMMIT_ZONE` that handles only >> `ct_commit({snat,dnat})` actions. >> > > > Instead of modifying ct_commit, I'd suggest adding 2 new actions - > ct_commit_snat and ct_commit_dnat. > > This would not have any ambiguity on the backward compatibility. > > Thanks > Numan > > Hi Numan, Thanks for the feedback, my only concern is that this approach could cause some confusion wrt the already existing action ct_commit_nat. Aside from having ct_commit_nat, ct_commit_snat and ct_commit_dnat, there would also be slight functional differences. According to the ovn-sb docs "[ct_commit_nat] Applies NAT and commits the connection to the CT", but ct_commit_snat and ct_commit_dnat would only commit the connection to CT, without applying any NAT. Martin. > >> >>> >>>> --- >>>> include/ovn/actions.h | 9 +++++++++ >>>> lib/actions.c | 20 +++++++++++++++++++- >>>> ovn-sb.xml | 9 +++++++++ >>>> 3 files changed, 37 insertions(+), 1 deletion(-) >>>> >>>> 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, >>>> +}; >>>> + >>>> >>> >>> There is no need for this enum, see details below. >>> >>> >>>> /* 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/lib/actions.c b/lib/actions.c >>>> index a45874dfb..319e65b6f 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); >>>> } >>>> @@ -814,7 +819,20 @@ 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); >>>> + switch (cc->zone) { >>>> + case OVNACT_CT_ZONE_SNAT: >>>> + ct->zone_src.field = mf_from_id(MFF_LOG_SNAT_ZONE); >>>> + break; >>>> + >>>> + case OVNACT_CT_ZONE_DNAT: >>>> + ct->zone_src.field = mf_from_id(MFF_LOG_DNAT_ZONE); >>>> + break; >>>> + >>>> + case OVNACT_CT_ZONE_DEFAULT: >>>> + default: >>>> + ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE); >>>> + break; >>>> + } >>>> >>> >>> This would actually break potential use in the switch pipeline when >>> someone would specify ct_commit(snat)/ct_commit(dnat). >>> The switch pipeline uses MFF_LOG_CT_ZONE only. See ct_commit_nat for >>> example [0]. There is only indication if it's snat or dnat and check for >>> switch/router pipeline. >>> >> >> I see. I did not quite understood that the MFF_LOG_CT_ZONE is there for >> the switch, thanks, I'll copy the behavior from `encode_CT_COMMIT_NAT` and >> I'll use `bool dnat_zone` instead of the enum. >> >> >>> ct->zone_src.ofs = 0; >>>> ct->zone_src.n_bits = 16; >>>> >>>> diff --git a/ovn-sb.xml b/ovn-sb.xml >>>> index ac4e585f2..66cb9747d 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,13 @@ >>>> in order to have specific bits set. >>>> </p> >>>> >>>> + <p> >>>> + 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. >>>> + </p> >>>> + >>>> <p> >>>> Note that if you want processing to continue in the next >>>> table, >>>> you must execute the <code>next</code> action after >>>> -- >>>> 2.40.1 >>>> >>>> _______________________________________________ >>>> dev mailing list >>>> dev@openvswitch.org >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>> >>>> >>> There is a test cae missing for this in "action parsing" test [1]. >>> >> >> ack. >> >> >>> >>> Thanks, >>> Ales >>> >>> [0] >>> https://github.com/ovn-org/ovn/blob/b92ad9e0b408a202273d69ba573f2538e53c6e48/lib/actions.c#L1219-L1225 >>> [1] >>> https://github.com/ovn-org/ovn/blob/b92ad9e0b408a202273d69ba573f2538e53c6e48/tests/ovn.at#L1308 >>> -- >>> >>> Ales Musil >>> >>> Senior Software Engineer - OVN Core >>> >>> Red Hat EMEA <https://www.redhat.com> >>> >>> amusil@redhat.com >>> <https://red.ht/sig> >>> >> >> >> -- >> Best Regards, >> Martin Kalcok. >> >
On Thu, Mar 7, 2024, 5:50 AM Martin Kalcok <martin.kalcok@canonical.com> wrote: > On Thu, Mar 7, 2024 at 1:26 AM Numan Siddique <numans@ovn.org> wrote: > > > > > > > On Wed, Mar 6, 2024 at 7:26 AM Martin Kalcok < > martin.kalcok@canonical.com> > > wrote: > > > >> Hi Ales, > >> Thank you for review and helpful comments. I'll update commit subjects > >> for this series in v2. > >> > >> On Wed, Mar 6, 2024 at 7:45 AM Ales Musil <amusil@redhat.com> wrote: > >> > >>> > >>> > >>> On Mon, Mar 4, 2024 at 11:56 AM 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. > >>>> > >>>> Original behavior of `ct_commit` without the arguments remains > >>>> unchanged. > >>>> > >>>> Signed-off-by: Martin Kalcok <martin.kalcok@canonical.com> > >>>> > >>> > >>> Hi Martin, > >>> > >>> thank you for the followup, I have a few comments down below. > >>> One small nit for the commit subject, we usually specify only the > module > >>> name without .c/.h e.g. action, ovn-controller, northd etc. > >>> AFAIR ct_commit_v1 is deprecated, but I guess it's fine to repurpose it > >>> for this. Maybe we could remove the old behavior and leave just this? > >>> Before posting v2 we should discuss this with others. > >>> Adding Dumitru and Numan to this discussion. > >>> > >> > >> Ack, I'll defer to your recommendation as to where this should be > >> implemented. If I read it correctly, `parse_ct_commit_v1_arg` still has > >> code to parse mark/label options but it's never used because if > `ct_commit` > >> action is followed by "{", the `parse_CT_COMMIT` will send it to > >> `parse_nested_action` instead. If that's the case and the > >> `parse_CT_COMMIT_V1` is unused, I'd be happy to remove it/repurpose it > to > >> something like `parse_CT_COMMIT_ZONE` that handles only > >> `ct_commit({snat,dnat})` actions. > >> > > > > > > Instead of modifying ct_commit, I'd suggest adding 2 new actions - > > ct_commit_snat and ct_commit_dnat. > > > > This would not have any ambiguity on the backward compatibility. > > > > Thanks > > Numan > > > > > Hi Numan, > Thanks for the feedback, my only concern is that this approach could cause > some confusion wrt the already existing action ct_commit_nat. Aside from > having ct_commit_nat, ct_commit_snat and ct_commit_dnat, there would also > be slight functional differences. According to the ovn-sb docs > "[ct_commit_nat] Applies NAT and commits the connection to the CT", but > ct_commit_snat and ct_commit_dnat would only commit the connection to CT, > without applying any NAT. > Ok. How about ct_commit_szone and ct_commit_dzone ? I'm ok with the names you are using in this patch as long as we don't have any backward compatibility issues. Numan > Martin. > > > > > >> > >>> > >>>> --- > >>>> include/ovn/actions.h | 9 +++++++++ > >>>> lib/actions.c | 20 +++++++++++++++++++- > >>>> ovn-sb.xml | 9 +++++++++ > >>>> 3 files changed, 37 insertions(+), 1 deletion(-) > >>>> > >>>> 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, > >>>> +}; > >>>> + > >>>> > >>> > >>> There is no need for this enum, see details below. > >>> > >>> > >>>> /* 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/lib/actions.c b/lib/actions.c > >>>> index a45874dfb..319e65b6f 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); > >>>> } > >>>> @@ -814,7 +819,20 @@ 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); > >>>> + switch (cc->zone) { > >>>> + case OVNACT_CT_ZONE_SNAT: > >>>> + ct->zone_src.field = mf_from_id(MFF_LOG_SNAT_ZONE); > >>>> + break; > >>>> + > >>>> + case OVNACT_CT_ZONE_DNAT: > >>>> + ct->zone_src.field = mf_from_id(MFF_LOG_DNAT_ZONE); > >>>> + break; > >>>> + > >>>> + case OVNACT_CT_ZONE_DEFAULT: > >>>> + default: > >>>> + ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE); > >>>> + break; > >>>> + } > >>>> > >>> > >>> This would actually break potential use in the switch pipeline when > >>> someone would specify ct_commit(snat)/ct_commit(dnat). > >>> The switch pipeline uses MFF_LOG_CT_ZONE only. See ct_commit_nat for > >>> example [0]. There is only indication if it's snat or dnat and check > for > >>> switch/router pipeline. > >>> > >> > >> I see. I did not quite understood that the MFF_LOG_CT_ZONE is there for > >> the switch, thanks, I'll copy the behavior from `encode_CT_COMMIT_NAT` > and > >> I'll use `bool dnat_zone` instead of the enum. > >> > >> > >>> ct->zone_src.ofs = 0; > >>>> ct->zone_src.n_bits = 16; > >>>> > >>>> diff --git a/ovn-sb.xml b/ovn-sb.xml > >>>> index ac4e585f2..66cb9747d 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,13 @@ > >>>> in order to have specific bits set. > >>>> </p> > >>>> > >>>> + <p> > >>>> + 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. > >>>> + </p> > >>>> + > >>>> <p> > >>>> Note that if you want processing to continue in the next > >>>> table, > >>>> you must execute the <code>next</code> action after > >>>> -- > >>>> 2.40.1 > >>>> > >>>> _______________________________________________ > >>>> dev mailing list > >>>> dev@openvswitch.org > >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >>>> > >>>> > >>> There is a test cae missing for this in "action parsing" test [1]. > >>> > >> > >> ack. > >> > >> > >>> > >>> Thanks, > >>> Ales > >>> > >>> [0] > >>> > https://github.com/ovn-org/ovn/blob/b92ad9e0b408a202273d69ba573f2538e53c6e48/lib/actions.c#L1219-L1225 > >>> [1] > >>> > https://github.com/ovn-org/ovn/blob/b92ad9e0b408a202273d69ba573f2538e53c6e48/tests/ovn.at#L1308 > >>> -- > >>> > >>> Ales Musil > >>> > >>> Senior Software Engineer - OVN Core > >>> > >>> Red Hat EMEA <https://www.redhat.com> > >>> > >>> amusil@redhat.com > >>> <https://red.ht/sig> > >>> > >> > >> > >> -- > >> Best Regards, > >> Martin Kalcok. > >> > > > > -- > Best Regards, > Martin Kalcok. > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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/lib/actions.c b/lib/actions.c index a45874dfb..319e65b6f 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); } @@ -814,7 +819,20 @@ 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); + switch (cc->zone) { + case OVNACT_CT_ZONE_SNAT: + ct->zone_src.field = mf_from_id(MFF_LOG_SNAT_ZONE); + break; + + case OVNACT_CT_ZONE_DNAT: + ct->zone_src.field = mf_from_id(MFF_LOG_DNAT_ZONE); + break; + + case OVNACT_CT_ZONE_DEFAULT: + default: + ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE); + break; + } ct->zone_src.ofs = 0; ct->zone_src.n_bits = 16; diff --git a/ovn-sb.xml b/ovn-sb.xml index ac4e585f2..66cb9747d 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,13 @@ in order to have specific bits set. </p> + <p> + 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. + </p> + <p> Note that if you want processing to continue in the next table, you must execute the <code>next</code> action after
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. Original behavior of `ct_commit` without the arguments remains unchanged. Signed-off-by: Martin Kalcok <martin.kalcok@canonical.com> --- include/ovn/actions.h | 9 +++++++++ lib/actions.c | 20 +++++++++++++++++++- ovn-sb.xml | 9 +++++++++ 3 files changed, 37 insertions(+), 1 deletion(-)