On 10/21/20 9:57 AM, Dumitru Ceara wrote:
> 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!
Applied to master and branch-2.14.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
