Re: [ovs-dev] [PATCH v2 1/2] ofproto-dpif: trigger revalidation when ipfix config set

[lic121]

>On 4 Feb 2022, at 15:25, lic121 wrote:
>
>>> On 28 Jan 2022, at 7:40, lic121 wrote:
>>>
 Currently, ipfix conf creation/deletion don't trigger dpif backer
 revalidation. This is not expected, as we need the revalidation
 to commit ipfix into xlate. So that xlate can generate ipfix
 actions.

 This patch covers only new creation/deletion of ipfix config.
 Will upload one more patch to cover ipfix option changes.
>>>
>>> I think you could drop this patch from the series until you have the full 
>>> fix, but I let the maintainers decide how they would like to handle this.
>>
>> Without this patch, two ipfix test cases fails with my second patch.
>
>Which test cases are they, as I see no newly introduced test cases for ipfix?

They are not new test cases: "Bridge IPFIX sanity check" and "Bridge IPFIX 
statistics check".
I copied the first test case failure log:
## test log #
--- -   2022-02-05 02:07:11.901471095 +
+++ /root/ovs/tests/testsuite.dir/at-groups/1190/stdout 2022-02-05 
02:07:11.899001638 +
@@ -1,3 +1,3 @@
 flow-dump from the main thread:
-packets:2, bytes:68, used:0.001s, 
actions:userspace(pid=0,ipfix(output_port=4294967295))
+packets:2, bytes:68, used:0.001s, actions:drop
## log end #

It fails because ipfix creating doesn't trigger revalidator. So no ipfix action 
is generated.
It didn't fail becase lldp always trigger revalidator in bridge_reconfigure() 
even if lldp config is empty 
In my second patch,  lldp issue is fixed, which avoid unneccesary revalidator 
triggered by lldp.


>
>> With my second patch, revalidator is not triggerd when no lldp changes.
>> As the result, ipfix config changes is not commited into xlate layer, 
>> because ipfix doesn't trigger revalidator by itself.
>>
>>>
>>> Ilya/Ian/William?
>>>
 Signed-off-by: lic121 
 ---
  ofproto/ofproto-dpif.c | 6 ++
  1 file changed, 6 insertions(+)

 diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
 index bc3df8e..5737615 100644
 --- a/ofproto/ofproto-dpif.c
 +++ b/ofproto/ofproto-dpif.c
 @@ -2333,6 +2333,12 @@ set_ipfix(
  dpif_ipfix_unref(di);
  ofproto->ipfix = NULL;
  }
 +
 +/* TODO: need to test ipfix option changes more than
 + * enable/disable */
 +if (new_di || !ofproto->ipfix) {
 +ofproto->backer->need_revalidate = REV_RECONFIGURE;
 +}
  }

  return 0;
 --
 1.8.3.1

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

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

Re: [ovs-dev] [PATCH v2 1/2] ofproto-dpif: trigger revalidation when ipfix config set

[Eelco Chaudron]

On 4 Feb 2022, at 15:25, lic121 wrote:

>> On 28 Jan 2022, at 7:40, lic121 wrote:
>>
>>> Currently, ipfix conf creation/deletion don't trigger dpif backer
>>> revalidation. This is not expected, as we need the revalidation
>>> to commit ipfix into xlate. So that xlate can generate ipfix
>>> actions.
>>>
>>> This patch covers only new creation/deletion of ipfix config.
>>> Will upload one more patch to cover ipfix option changes.
>>
>> I think you could drop this patch from the series until you have the full 
>> fix, but I let the maintainers decide how they would like to handle this.
>
> Without this patch, two ipfix test cases fails with my second patch.

Which test cases are they, as I see no newly introduced test cases for ipfix?

> With my second patch, revalidator is not triggerd when no lldp changes.
> As the result, ipfix config changes is not commited into xlate layer, because 
> ipfix doesn't trigger revalidator by itself.
>
>>
>> Ilya/Ian/William?
>>
>>> Signed-off-by: lic121 
>>> ---
>>>  ofproto/ofproto-dpif.c | 6 ++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>>> index bc3df8e..5737615 100644
>>> --- a/ofproto/ofproto-dpif.c
>>> +++ b/ofproto/ofproto-dpif.c
>>> @@ -2333,6 +2333,12 @@ set_ipfix(
>>>  dpif_ipfix_unref(di);
>>>  ofproto->ipfix = NULL;
>>>  }
>>> +
>>> +/* TODO: need to test ipfix option changes more than
>>> + * enable/disable */
>>> +if (new_di || !ofproto->ipfix) {
>>> +ofproto->backer->need_revalidate = REV_RECONFIGURE;
>>> +}
>>>  }
>>>
>>>  return 0;
>>> --
>>> 1.8.3.1
>>>
>>> ___
>>> dev mailing list
>>> d...@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>

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

Re: [ovs-dev] [PATCH v2 1/2] ofproto-dpif: trigger revalidation when ipfix config set

[lic121]

>On 28 Jan 2022, at 7:40, lic121 wrote:
>
>> Currently, ipfix conf creation/deletion don't trigger dpif backer
>> revalidation. This is not expected, as we need the revalidation
>> to commit ipfix into xlate. So that xlate can generate ipfix
>> actions.
>>
>> This patch covers only new creation/deletion of ipfix config.
>> Will upload one more patch to cover ipfix option changes.
>
>I think you could drop this patch from the series until you have the full fix, 
>but I let the maintainers decide how they would like to handle this.

Without this patch, two ipfix test cases fails with my second patch.

With my second patch, revalidator is not triggerd when no lldp changes.
As the result, ipfix config changes is not commited into xlate layer, because 
ipfix doesn't trigger revalidator by itself.

>
>Ilya/Ian/William?
>
>> Signed-off-by: lic121 
>> ---
>>  ofproto/ofproto-dpif.c | 6 ++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index bc3df8e..5737615 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -2333,6 +2333,12 @@ set_ipfix(
>>  dpif_ipfix_unref(di);
>>  ofproto->ipfix = NULL;
>>  }
>> +
>> +/* TODO: need to test ipfix option changes more than
>> + * enable/disable */
>> +if (new_di || !ofproto->ipfix) {
>> +ofproto->backer->need_revalidate = REV_RECONFIGURE;
>> +}
>>  }
>>
>>  return 0;
>> --
>> 1.8.3.1
>>
>> ___
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>

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

Re: [ovs-dev] [PATCH v2 1/2] ofproto-dpif: trigger revalidation when ipfix config set

[Eelco Chaudron]

On 28 Jan 2022, at 7:40, lic121 wrote:

> Currently, ipfix conf creation/deletion don't trigger dpif backer
> revalidation. This is not expected, as we need the revalidation
> to commit ipfix into xlate. So that xlate can generate ipfix
> actions.
>
> This patch covers only new creation/deletion of ipfix config.
> Will upload one more patch to cover ipfix option changes.

I think you could drop this patch from the series until you have the full fix, 
but I let the maintainers decide how they would like to handle this.

Ilya/Ian/William?

> Signed-off-by: lic121 
> ---
>  ofproto/ofproto-dpif.c | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index bc3df8e..5737615 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -2333,6 +2333,12 @@ set_ipfix(
>  dpif_ipfix_unref(di);
>  ofproto->ipfix = NULL;
>  }
> +
> +/* TODO: need to test ipfix option changes more than
> + * enable/disable */
> +if (new_di || !ofproto->ipfix) {
> +ofproto->backer->need_revalidate = REV_RECONFIGURE;
> +}
>  }
>
>  return 0;
> --
> 1.8.3.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[ovs-dev] [PATCH v2 1/2] ofproto-dpif: trigger revalidation when ipfix config set

[lic121]

Currently, ipfix conf creation/deletion don't trigger dpif backer
revalidation. This is not expected, as we need the revalidation
to commit ipfix into xlate. So that xlate can generate ipfix
actions.

This patch covers only new creation/deletion of ipfix config.
Will upload one more patch to cover ipfix option changes.

Signed-off-by: lic121 
---
 ofproto/ofproto-dpif.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index bc3df8e..5737615 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2333,6 +2333,12 @@ set_ipfix(
 dpif_ipfix_unref(di);
 ofproto->ipfix = NULL;
 }
+
+/* TODO: need to test ipfix option changes more than
+ * enable/disable */
+if (new_di || !ofproto->ipfix) {
+ofproto->backer->need_revalidate = REV_RECONFIGURE;
+}
 }

 return 0;
--
1.8.3.1

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