[ovs-dev] [PATCH] raft: Report jsonrpc backlog in kilobytes.

Dumitru Ceara dceara at redhat.com
Wed Oct 21 07:57:00 UTC 2020


On 10/20/20 6:20 PM, Ilya Maximets wrote:
> On 10/20/20 1:51 PM, Dumitru Ceara wrote:
>> On 10/20/20 1:16 PM, Ilya Maximets wrote:
>>> While sending snapshots backlog on raft connections could quickly
>>> grow over 4GB and this will overflow raft-backlog counter.
>>>
>>> Let's report it in kB instead. (Using kB and not KB to match with
>>> ru_maxrss counter reported by kernel)
>>>
>>> Fixes: 3423cd97f88f ("ovsdb: Add raft memory usage to memory report.")
>>> Signed-off-by: Ilya Maximets <i.maximets at ovn.org>
>>> ---
>>>  ovsdb/raft.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/ovsdb/raft.c b/ovsdb/raft.c
>>> index 708b0624c..3411323aa 100644
>>> --- a/ovsdb/raft.c
>>> +++ b/ovsdb/raft.c
>>> @@ -1020,13 +1020,14 @@ void
>>>  raft_get_memory_usage(const struct raft *raft, struct simap *usage)
>>>  {
>>>      struct raft_conn *conn;
>>> +    uint64_t backlog = 0;
>>>      int cnt = 0;
>>>  
>>>      LIST_FOR_EACH (conn, list_node, &raft->conns) {
>>> -        simap_increase(usage, "raft-backlog",
>>> -                       jsonrpc_session_get_backlog(conn->js));
>>> +        backlog += jsonrpc_session_get_backlog(conn->js);
>>>          cnt++;
>>>      }
>>> +    simap_increase(usage, "raft-backlog-kB", backlog / 1000);
>>>      simap_increase(usage, "raft-connections", cnt);
>>>  }
>>>  
>>>
>>
>> Hi Ilya,
>>
>> This change looks OK to me.  However, since "raft-backlog" was already
>> displayed in v2.14.0, should we consider not breaking backwards compatibility
>> and reporting both "raft-backlog" (potentially overflowed) and
>> "raft-backlog-kB" at "ovs-appctl .. memory/show"?
> 
> I don't think that reporting overflowed value makes any sense.  I thought
> about reporting just 'raft-backlog' if there is no overflow.  It could look
> like this (not so pretty):
> 
> diff --git a/ovsdb/raft.c b/ovsdb/raft.c
> index 3411323aa..b967fff3d 100644
> --- a/ovsdb/raft.c
> +++ b/ovsdb/raft.c
> @@ -1021,13 +1021,23 @@ raft_get_memory_usage(const struct raft *raft, struct simap *usage)
>  {
>      struct raft_conn *conn;
>      uint64_t backlog = 0;
> +    uint64_t old_backlog;
>      int cnt = 0;
>  
>      LIST_FOR_EACH (conn, list_node, &raft->conns) {
>          backlog += jsonrpc_session_get_backlog(conn->js);
>          cnt++;
>      }
> -    simap_increase(usage, "raft-backlog-kB", backlog / 1000);
> +
> +    old_backlog = simap_get(usage, "raft-backlog");
> +    if (simap_contains(usage, "raft-backlog-kB")) {
> +        simap_increase(usage, "raft-backlog-kB", backlog / 1000);
> +    } else if (backlog + old_backlog > UINT_MAX) {
> +        simap_put(usage, "raft-backlog-kB", (backlog + old_backlog) / 1000);
> +        simap_find_and_delete(usage, "raft-backlog");
> +    } else {
> +        simap_increase(usage, "raft-backlog", backlog);
> +    }
>      simap_increase(usage, "raft-connections", cnt);
>  }
>  
> ---
> 
> Do you think this is better?
> 

I agree with you, it doesn't look so pretty :)

>>
>> On the other hand, ovn-k8s parses the output of "memory/show" but ignores
>> "raft-backlog" for now [1] so at least from that perspective it should be fine.
>>
>> Is this considered a user visible change?
> 
> I'd consider this as a debug information.  The output format is not documented,
> so I'd like to not care much about this interface.
> 

Ok, in this case I think your original patch is fine. For that:

Acked-by: Dumitru Ceara <dceara at redhat.com>

Thanks,
Dumitru



More information about the dev mailing list