On Thu, Mar 7, 2024 at 1:26 AM Numan Siddique <[email protected]> wrote:
> > > On Wed, Mar 6, 2024 at 7:26 AM Martin Kalcok <[email protected]> > 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 <[email protected]> wrote: >> >>> >>> >>> On Mon, Mar 4, 2024 at 11:56 AM Martin Kalcok < >>> [email protected]> 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 <[email protected]> >>>> >>> >>> 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 >>>> [email protected] >>>> 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> >>> >>> [email protected] >>> <https://red.ht/sig> >>> >> >> >> -- >> Best Regards, >> Martin Kalcok. >> > -- Best Regards, Martin Kalcok. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
