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

Ilya Maximets i.maximets at ovn.org
Tue Oct 20 16:20:11 UTC 2020


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?

> 
> 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.

> 
> Regards,
> Dumitru
> 
> [1]
> https://github.com/ovn-org/ovn-kubernetes/blob/dd1289c3ff3543798786baac30c9e9ad51aa27a4/go-controller/pkg/metrics/ovn_db.go#L292
> 



More information about the dev mailing list