diff mbox series

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

Message ID 20220128144019858375129@chinatelecom.cn
State Superseded
Headers show
Series fix dpif backer revalidation | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Cheng Li Jan. 28, 2022, 6:40 a.m. UTC 
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 <lic121@chinatelecom.cn>
---
 ofproto/ofproto-dpif.c | 6 ++++++
 1 file changed, 6 insertions(+)

--
1.8.3.1

Comments

Eelco Chaudron Feb. 4, 2022, 12:27 p.m. UTC | #1 
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 <lic121@chinatelecom.cn>
> ---
>  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
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Cheng Li Feb. 4, 2022, 2:25 p.m. UTC | #2 
>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 <lic121@chinatelecom.cn>
>> ---
>>  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
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Eelco Chaudron Feb. 4, 2022, 2:36 p.m. UTC | #3 
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 <lic121@chinatelecom.cn>
>>> ---
>>>  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
>>> dev@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
Cheng Li Feb. 5, 2022, 2:31 a.m. UTC | #4 
>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 +0000
+++ /root/ovs/tests/testsuite.dir/at-groups/1190/stdout 2022-02-05 02:07:11.899001638 +0000
@@ -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 <lic121@chinatelecom.cn>
>>>> ---
>>>>  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
>>>> dev@openvswitch.org
>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>>>
>
>
diff mbox series

Patch

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;