[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