On Thu, Mar 7, 2024, 5:50 AM Martin Kalcok <[email protected]> wrote:
> 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. > 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 > >>>> [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 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
