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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
Thanks,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
