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 <amu...@redhat.com> wrote:
>
>
> On Mon, Mar 4, 2024 at 11:56 AM Martin Kalcok <martin.kal...@canonical.com>
> wrote:
>
>> Action `ct_commit` currently does not allow specifying zone into
>> which connection is committed. For example, in LR datapath, the
>> `ct_commit`
>> will always use the DNAT zone.
>>
>> This change adds option to use `ct_commit(snat)` or `ct_commit(dnat)` to
>> explicitly specify the zone into which the connection will be committed.
>>
>> Original behavior of `ct_commit` without the arguments remains unchanged.
>>
>> Signed-off-by: Martin Kalcok <martin.kal...@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
>> d...@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>
>
> amu...@redhat.com
> <https://red.ht/sig>
>
--
Best Regards,
Martin Kalcok.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
