Re: [ovs-dev] [Patch ovn 1/2] actions.c/h: Enable specifying zone for ct_commit.

[Numan Siddique]

On Thu, Mar 7, 2024, 5:50 AM Martin Kalcok 
wrote:

> On Thu, Mar 7, 2024 at 1:26 AM Numan Siddique  wrote:
>
> >
> >
> > 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  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 
> 
> >>>
> >>> 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")) {

Re: [ovs-dev] [Patch ovn 1/2] actions.c/h: Enable specifying zone for ct_commit.

[Martin Kalcok]

On Thu, Mar 7, 2024 at 1:26 AM Numan Siddique  wrote:

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

>>>
>>> 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);

Re: [ovs-dev] [Patch ovn 1/2] actions.c/h: Enable specifying zone for ct_commit.

[Numan Siddique]

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

Re: [ovs-dev] [Patch ovn 1/2] actions.c/h: Enable specifying zone for ct_commit.

[Martin Kalcok]

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  wrote:

>
>
> On Mon, Mar 4, 2024 at 11:56 AM Martin Kalcok 
> 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 
>>
>
> 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
>> --- 

Re: [ovs-dev] [Patch ovn 1/2] actions.c/h: Enable specifying zone for ct_commit.

[Ales Musil]

On Mon, Mar 4, 2024 at 11:56 AM Martin Kalcok 
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 
>

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 @@
>  ct_commit { ct_mark=value[/mask];
> };
>  ct_commit { ct_label=value[/mask];
> };
>  ct_commit { ct_mark=value[/mask];
> ct_label=value[/mask]; };
> +ct_commit(snat);
> +ct_commit(dnat);
>  
>
>  Commit the flow to the connection tracking entry associated
> with it
> @@ -1421,6 +1423,13 @@
>  in order to have specific bits set.
>
>
> +  
> +Parameters ct_commit(snat) or
> ct_commit(dnat)
> + 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.
> +  
> +
>
>  Note that if you want processing to continue in the next
> table,
>  you must execute the next action after
> --
> 2.40.1
>
> 

[ovs-dev] [Patch ovn 1/2] actions.c/h: Enable specifying zone for ct_commit.

[Martin Kalcok]

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 
---
 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,
+};
+
 /* 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 @@
 ct_commit { ct_mark=value[/mask]; };
 ct_commit { ct_label=value[/mask]; };
 ct_commit { ct_mark=value[/mask]; 
ct_label=value[/mask]; };
+ct_commit(snat);
+ct_commit(dnat);
 
   
 Commit the flow to the connection tracking entry associated with it
@@ -1421,6 +1423,13 @@
 in order to have specific bits set.
   
 
+  
+Parameters ct_commit(snat) or ct_commit(dnat)
+ 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.
+  
+
   
 Note that if you want processing to continue in the next table,
 you must execute the next action after
-- 
2.40.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev