diff mbox series

[ovs-dev] ovn-nbctl: Monitor nb_cfg to detect and handle potential overflows

Message ID 20210819004347.665893-1-mheib@redhat.com
State Superseded
Headers show
Series [ovs-dev] ovn-nbctl: Monitor nb_cfg to detect and handle potential overflows | expand

Checks

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

Commit Message

Mohammad Heib Aug. 19, 2021, 12:43 a.m. UTC 
From: Mohammad Heib <mheib@redhat.com>

ovn-nbctl sync command will increase nb_cfg value each time it is executed
with a specific wait_type, this increment will be handled without testing
the current nb_cfg value because it is not monitored and that could lead
to an overflow issue if nb_cfg == LLONG_MAX.

To avoid such potential overflow cases we must monitor the real value
of nb_cfg each time sync is executed and if there is any overflow issue
it will be handled by function nbctl_pre_execute later on the execution.

Fixes: be3a60f8e6a3 ("ovn-nbctl: Deal with nb_cfg overflows.")
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1979774
Signed-off-by: Mohammad Heib <mheib@redhat.com>
---
 utilities/ovn-nbctl.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Numan Siddique Aug. 19, 2021, 3:28 p.m. UTC | #1 
On Wed, Aug 18, 2021 at 8:44 PM <mheib@redhat.com> wrote:
>
> From: Mohammad Heib <mheib@redhat.com>
>
> ovn-nbctl sync command will increase nb_cfg value each time it is executed
> with a specific wait_type, this increment will be handled without testing
> the current nb_cfg value because it is not monitored and that could lead
> to an overflow issue if nb_cfg == LLONG_MAX.
>
> To avoid such potential overflow cases we must monitor the real value
> of nb_cfg each time sync is executed and if there is any overflow issue
> it will be handled by function nbctl_pre_execute later on the execution.
>
> Fixes: be3a60f8e6a3 ("ovn-nbctl: Deal with nb_cfg overflows.")
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1979774
> Signed-off-by: Mohammad Heib <mheib@redhat.com>

Thanks for the patch and fixing this issue.

Would you mind adding a test case in tests/ovn-nbctl.at to make sure
that the nb_cfg value is wrapped when it is about to overflow.

Thanks
Numan

> ---
>  utilities/ovn-nbctl.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index ada53b662d3c..69be775bc567 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -830,6 +830,8 @@ static void
>  nbctl_pre_sync(struct ctl_context *base OVS_UNUSED)
>  {
>      force_wait = true;
> +    /* Monitor nb_cfg to detect and handle potential overflows. */
> +    ovsdb_idl_add_column(base->idl, &nbrec_nb_global_col_nb_cfg);
>  }
>
>  static void
> --
> 2.27.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Mark Gray Aug. 20, 2021, 4:53 p.m. UTC | #2 
On 19/08/2021 16:28, Numan Siddique wrote:
> On Wed, Aug 18, 2021 at 8:44 PM <mheib@redhat.com> wrote:
>>
>> From: Mohammad Heib <mheib@redhat.com>
>>
>> ovn-nbctl sync command will increase nb_cfg value each time it is executed
>> with a specific wait_type, this increment will be handled without testing
>> the current nb_cfg value because it is not monitored and that could lead
>> to an overflow issue if nb_cfg == LLONG_MAX.
>>
>> To avoid such potential overflow cases we must monitor the real value
>> of nb_cfg each time sync is executed and if there is any overflow issue
>> it will be handled by function nbctl_pre_execute later on the execution.
>>
>> Fixes: be3a60f8e6a3 ("ovn-nbctl: Deal with nb_cfg overflows.")
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1979774
>> Signed-off-by: Mohammad Heib <mheib@redhat.com>
> 
> Thanks for the patch and fixing this issue.
> 
> Would you mind adding a test case in tests/ovn-nbctl.at to make sure
> that the nb_cfg value is wrapped when it is about to overflow.
> 

FYI: There seem to be some related tests in ovn-controller.at

ovn-controller - Bump Chassis_Private nb_cfg value
nb_cfg sync to OVS

> Thanks
> Numan
> 
>> ---
>>  utilities/ovn-nbctl.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
>> index ada53b662d3c..69be775bc567 100644
>> --- a/utilities/ovn-nbctl.c
>> +++ b/utilities/ovn-nbctl.c
>> @@ -830,6 +830,8 @@ static void
>>  nbctl_pre_sync(struct ctl_context *base OVS_UNUSED)
>>  {
>>      force_wait = true;
>> +    /* Monitor nb_cfg to detect and handle potential overflows. */
>> +    ovsdb_idl_add_column(base->idl, &nbrec_nb_global_col_nb_cfg);
>>  }
>>
>>  static void
>> --
>> 2.27.0
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Mohammad Heib Aug. 21, 2021, 5:20 p.m. UTC | #3 
Hi Numan,

On 8/19/21 6:28 PM, Numan Siddique wrote:
> On Wed, Aug 18, 2021 at 8:44 PM <mheib@redhat.com> wrote:
>> From: Mohammad Heib <mheib@redhat.com>
>>
>> ovn-nbctl sync command will increase nb_cfg value each time it is executed
>> with a specific wait_type, this increment will be handled without testing
>> the current nb_cfg value because it is not monitored and that could lead
>> to an overflow issue if nb_cfg == LLONG_MAX.
>>
>> To avoid such potential overflow cases we must monitor the real value
>> of nb_cfg each time sync is executed and if there is any overflow issue
>> it will be handled by function nbctl_pre_execute later on the execution.
>>
>> Fixes: be3a60f8e6a3 ("ovn-nbctl: Deal with nb_cfg overflows.")
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1979774
>> Signed-off-by: Mohammad Heib <mheib@redhat.com>
> Thanks for the patch and fixing this issue.
>
> Would you mind adding a test case in tests/ovn-nbctl.at to make sure
> that the nb_cfg value is wrapped when it is about to overflow.
>
> Thanks
> Numan

Thanks Numan, i submitted patch v2 with test case for overflow scenarios.

Thanks,

Mohammad

>> ---
>>   utilities/ovn-nbctl.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
>> index ada53b662d3c..69be775bc567 100644
>> --- a/utilities/ovn-nbctl.c
>> +++ b/utilities/ovn-nbctl.c
>> @@ -830,6 +830,8 @@ static void
>>   nbctl_pre_sync(struct ctl_context *base OVS_UNUSED)
>>   {
>>       force_wait = true;
>> +    /* Monitor nb_cfg to detect and handle potential overflows. */
>> +    ovsdb_idl_add_column(base->idl, &nbrec_nb_global_col_nb_cfg);
>>   }
>>
>>   static void
>> --
>> 2.27.0
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
diff mbox series

Patch

diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
index ada53b662d3c..69be775bc567 100644
--- a/utilities/ovn-nbctl.c
+++ b/utilities/ovn-nbctl.c
@@ -830,6 +830,8 @@  static void
 nbctl_pre_sync(struct ctl_context *base OVS_UNUSED)
 {
     force_wait = true;
+    /* Monitor nb_cfg to detect and handle potential overflows. */
+    ovsdb_idl_add_column(base->idl, &nbrec_nb_global_col_nb_cfg);
 }
 
 static void