[watremark] Add watermark HLD by mykolaf · Pull Request #245 · sonic-net/SONiC

mykolaf · 2018-09-07T13:54:40Z

Watermark High Level Design Document for SONiC

lguohan · 2018-09-10T17:52:22Z

doc/watermarks_HLD.md

the regular user should also be able to reset the watermark.

What is the use case for resetting the watermark?

clear the watermark, for example, if user wants to know how buffer is utilized under current traffic, he will want to clear the existing watermark and check the new one.

if we have clear the watermark, to be able to check the new one, we should also have a get watermark?

User will have CLI to show current watermark, f.e. "show queue watermark unicast" for current watermark, and "show queue persistent-watermark unicast" for persistent one.

lguohan · 2018-09-10T17:53:20Z

doc/watermarks_HLD.md

streaming telemetry is periodically resetting the watermark.

I agree. This sentence is copy-pasted from requirements :)

lguohan · 2018-09-10T17:55:20Z

I cannot see those figures, have you uploaded them?

lguohan

add more command line

lguohan · 2018-09-11T19:24:06Z

doc/watermarks_HLD.md

do we have the total pool watermark here? I think we need that as well.

Pool watermark was not in the requirements. Also as far as I know pool objects are not created in SONiC, so more complexity.

lguohan · 2018-09-11T19:27:00Z

doc/watermarks_HLD.md

show priority-group watermark headroom

lguohan · 2018-09-11T19:31:53Z

doc/watermarks_HLD.md

show priority-group watermark shared

lguohan · 2018-09-11T19:32:07Z

doc/watermarks_HLD.md

show queue watermark unicast

lguohan · 2018-09-11T19:32:23Z

doc/watermarks_HLD.md

show queue watermark multicast

lguohan · 2018-09-11T19:33:50Z

doc/watermarks_HLD.md

clear priority-group watermark headroom
clear priority-group watermark shared
clear queue watermark unicast
clear qeueu watermark mutlicast

lguohan · 2018-09-11T19:34:52Z

doc/watermarks_HLD.md

show priority-group persistent-watermark headroom
show priority-group persistent-watermark shared
show queue persistent-watermark unicast
show queue persistent-watermark multicast

lguohan · 2018-09-11T19:35:34Z

doc/watermarks_HLD.md

also add following

show pool watermark ingress
show pool watermark egress
show pool watermark headroom

lguohan · 2018-09-11T19:37:25Z

I can see the figure now, but the there is no marker on the figure and there is no explanation on the figure. couldn't understand, can you explain the figure that has the timeline and events.

lguohan · 2018-09-15T16:22:35Z

doc/watermarks_HLD.md

why not use flex counter to do the query? It is better to consolidate all counter query into one place. We already have a flex counter thread to do the query, in my opinion, all counter query should be there.

lguohan · 2018-09-15T16:31:33Z

doc/watermarks_HLD.md

rename to WATERMARKS - containing periodically cleared watermarks.

add CACHED_WATERMARK to temporarily store the water mark before last user clear.

need HIGHEST_WATERMARK table to store last highest watermark, this is not persistent watermark table. when user clear the water mark, it needs also clear the highest_watermark.

lguohan · 2018-09-15T16:39:35Z

doc/watermarks_HLD.md

Periocially, the flex counter put the current watermark into WATERMARK table, and then clear it on hardware.

lguohan · 2018-09-15T17:03:20Z

doc/watermarks_HLD.md

the watermarks need to be integrated in telemetry virtual path, but this can be phase II.

lguohan · 2018-09-15T17:14:00Z

doc/watermarks_HLD.md

there should be a CACHED_WATERMARK table that is used to save the watermark before user clear. whenever user clears the watermark, it needs to put the last read watermark into the CACHED_WATERMARK. telemetry should read the CACHED_WATERMARK and WATERMARK and return the maximum value if there is CACHED_WATERMARK. After that, CAHCED_WATERMARK should be deleted.

btw, I would suggest to use the lua plugin to the comparision and merge.

lguohan · 2018-09-15T17:20:26Z

doc/watermarks_HLD.md

at point 2, the CACHED_WATERMARK is generated, in next periodically clear point, the CACHED_WATERMARK is compared with WATERMARK and send to telemetry. after that, the CACHED_WATERMARK is deleted.

lguohan · 2018-09-15T17:33:38Z

doc/watermarks_HLD.md

we need to walk through the timeline and connect these events to the detail design to see how everything works. currently, the description of events is not associated with your design.

lguohan · 2018-09-19T16:30:18Z

doc/watermarks_HLD.md

no need to have --period in the cli.

no --period, issue query immediately

lguohan · 2018-09-19T16:34:36Z

doc/watermarks_HLD.md

can you add the schema you want to introduce for read-on-clear for a group of counters?

lguohan · 2018-09-19T16:40:15Z

doc/watermarks_HLD.md

rename -> USER_WATERMARKS

remove the --period

lguohan · 2018-09-19T16:43:39Z

doc/watermarks_HLD.md

sonic-clear priority-group [watermark|persistent-watermark] headroom

lguohan · 2018-09-19T17:08:48Z

doc/watermarks_HLD.md

depending the performance, we may need to increase the polling interval for the watermark and introduce on-demand asic counter clear on the watermarks.

Chornei · 2018-09-19T18:09:39Z

Then I dont understand the req When the user issue this cli it specified a period. What that period means. Backwards or forward? If backwards then it means lots of storage which has no mean. Best regards, Liat Grozovik I sr. Director I SW systems Sorry for all typo. I am sure there are few... Sent from my Samsung Galaxy smartphone.

-------- Original message -------- From: Wenda Ni <notifications@github.com> Date: 9/19/18 20:21 (GMT+02:00) To: Azure/SONiC <SONiC@noreply.github.com> Cc: Subscribed <subscribed@noreply.github.com> Subject: Re: [Azure/SONiC] [watremark] Add watermark HLD (#245) @wendani commented on this pull request.

________________________________ In doc/watermarks_HLD.md<#245 (comment)>:

+An unified API which represent the switch state as a set of objects. In SONiC represented in two implementations - SAI DB frontend and ASIC SDK wrapper.

+# 2 Requirements +The following watermarks should be supported: +## 2.1 Watermark counters requirements +| | SAI attribute mapping | +|------------------------------------------|------------------------| +| Ingress headroom per PG | SAI_INGRESS_PRIORITY_GROUP_STAT_XOFF_ROOM_WATERMARK_BYTES | +| Ingress shared pool occupancy per PG | SAI_INGRESS_PRIORITY_GROUP_STAT_SHARED_WATERMARK_BYTES | +| Egress shared pool occupancy per queue (including both unicast queues and multicast queues) | SAI_QUEUE_STAT_SHARED_WATERMARK_BYTES | + +System behavior: +We consider a maximum of one regular user and a maximum of one special user that comes from streaming telemetry (grpc) + +Streaming telemetry is only interested in periodic watermark, i.e., it queries the watermark at regular intervals. The interval is configurable. Streaming telemetry does not care about persistent watermark. + +Regular user is able to query the watermark over a period specified in the cli. When the cli is issued, the cli blocks over the specified period, which counts the issuing of the cli as its starting point, and then returns the watermark over the specified period. no --period, issue query immediately — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<#245 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ARmmi8ykMlTAvZ71HjturT_4AtdT9g7Kks5ucn0LgaJpZM4We3su>.

Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>

* [watremark] Add watermark HLD Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>

lguohan reviewed Sep 10, 2018

View reviewed changes

lguohan reviewed Sep 11, 2018

View reviewed changes

lguohan reviewed Sep 15, 2018

View reviewed changes

mykolaf force-pushed the w branch from 24b805d to 17abdff Compare September 17, 2018 16:33

lguohan reviewed Sep 19, 2018

View reviewed changes

doc/watermarks_HLD.md Outdated

Copy link
Copy Markdown

Contributor

lguohan Sep 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sonic-clear priority-group [watermark|persistent-watermark] headroom

lguohan reviewed Sep 19, 2018

View reviewed changes

[watremark] Add watermark HLD

4a678aa

Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>

mykolaf force-pushed the w branch from 694754a to 4a678aa Compare October 3, 2018 11:59

[watermark] clear watermark CLI requires sudo

d479572

Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>

andriymoroz-mlnx merged commit 90ff8a9 into sonic-net:gh-pages Nov 23, 2018

lguohan pushed a commit that referenced this pull request Nov 28, 2018

[watremark] Add watermark HLD (#245)

05df96c

* [watremark] Add watermark HLD Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>

mykolaf deleted the w branch March 6, 2019 15:02

Conversation

mykolaf commented Sep 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wendani Sep 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lguohan commented Sep 10, 2018

Uh oh!

lguohan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lguohan commented Sep 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lguohan Sep 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mykolaf commented Sep 7, 2018 •

edited

Loading

wendani Sep 10, 2018 •

edited

Loading

lguohan Sep 15, 2018 •

edited

Loading