On Wed, Mar 6, 2024 at 7:26 AM Martin Kalcok <martin.kal...@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 <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.
>
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
>>> 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
