Add counters enabling redesign document #918

noaOrMlnx · 2022-01-04T08:01:17Z

enable_counters.py script sleeps for 3 minutes before enabling the counters.
This design purpose is to change the sleep mechanism to be event driven.

After the change, the script logic will be in flexCounterOrch and we will wait for the system to be stable and just then enable the counters.

lguohan · 2022-01-04T17:57:53Z

doc/counters_enabling_redesign.md

+
+### Ports Daemon
+
+enable_counters.py script will be refactored to be running as a daemon.


this should be merged into the orchagent

please also add config db schame and yang schema for this feature.

@lguohan
Updated the design according to your request.
regarding the config db schema & yang model - why do we need to add config db schema? we don't add anything new to DB - it's all internal.
If you mean the delay_status attributes, we already have schema & yang for them.
https://github.com/Azure/sonic-swss-common/blob/master/common/schema.h#L225
https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-yang-models/yang-models/sonic-flex_counter.yang#L49

i think in the config db, we need to add something to set the delay time, for example 300 seconds which allow user to specify how long they want to delay?

@lguohan
This PR is related to the fact we are moving from sleep mechanism to be event driven.
I agree that adding an option of how much delay the user wants is good, but I don't think it's related to this design.

If you want, our team will take this feature, but in another design and plan.
what do you think?

bingwang-ms · 2022-01-05T01:50:06Z

Will this change increase the time cost of reconciliation after fast/warm reboot since the counter is expected to be enabled earlier after this change?

…ounters_redesign

yxieca · 2022-01-10T17:24:02Z

@vaibhavhd please review from the fast/warm reboot perspective.

liat-grozovik · 2022-01-15T14:22:25Z

@vaibhavhd any feedback on warm/fast boot? we wish to close the coding part and would like to get your feedback soon please.
@lguohan HLD was updated following your comments. Could you please take further look?

vaibhavhd · 2022-01-18T15:54:55Z

For fast/warm reboot -
I don't see concerns for warmboot.
For fastboot - do we have any prototype to be evaluated for the design? It will be interesting to measure CPU consumption at the time when counters are enabled much earlier than 3 minutes after reboot.
AFAIK, high CPU consumption due to enabled counters was the impacting factor in fastboot reconciliation. This caused the dataplane downtime to be > 30s (which is not accepted).

vaibhavhd · 2022-01-18T16:51:59Z

More for fast-reboot:

@shlomibitton with this new enable_counters re-design, do we still need FLEX_COUNTER_DELAY_STATUS and its related changes?
FLEX_COUNTER_DELAY_STATUS was added to prevent FlexCountersOrch from enabling counters until enable_counters script is called.

Since enable_counters.py script is now not needed, and the delay mechanism is local to FlexCountersOrch, we probably not need the changes that were added to support new flag - FLEX_COUNTER_DELAY_STATUS.:

sonic-net/sonic-buildimage#8500
sonic-net/sonic-swss-common#523
sonic-net/sonic-swss#1877
sonic-net/sonic-utilities#1768

shlomibitton · 2022-01-18T18:36:37Z

@vaibhavhd Yes, we don't need it anymore.
But the new change need to handle this internally.
Means, ignore all events from Config DB and enable after all ports are up or the timeout reached.

doc/counters_enabling_redesign.md

vaibhavhd · 2022-01-18T15:21:54Z

doc/counters_enabling_redesign.md

+## Requirements
+
+Remove enable_counters.py script and merge the logic into FlexCountersOrch.
+FlexCountersOrch will wait for system to be up using events from APP DB, then enable the counters using the logic written in enable_counters.py.


FlexCountersOrch will wait for system to be up

Please define/specify what is meant by system to be up.

doc/counters_enabling_redesign.md

vaibhavhd · 2022-01-18T15:34:42Z

doc/counters_enabling_redesign.md

+
+- The daemon will also wait for a timer in order to be able to enable counters even if one of the ports is not stable.
+
+    If after 3 minutes (180 seconds), the counters were not enabled yet, enable counters.


What is the offset for After 3 minutes? Is it reboot? Please specify that.

Also, in the present design we wait for 3m if the uptime was less than 5mins. I think you are covering this.

However we also have a wait for 1m in cases other than post-reboot. Do you not want to cover this scenario, and why not?

https://github.com/Azure/sonic-buildimage/blob/a0376a6e5972a7dde8810c6108bd9bf7a4c29e08/dockers/docker-orchagent/enable_counters.py#L57

vaibhavhd · 2022-01-18T15:45:03Z

doc/counters_enabling_redesign.md

+    The daemon will create a SelectableTimer and wait for the it to poke.
+
+    When doTask(SelectableTimer*) function will be triggered, it will check if the counters are already enabled.
+    If not, enable them.


Chances of race-condition here? What happens if counters enabling is in progress and the SelectableTimer is also triggered?

Also, after the enabling of counters, what happens to the new data structure defined in FlexCountersOrch?
There can be two cases to be covered here - event-driven mechanism (new design) and timeout (old design).

Chances of race-condition here? What happens if counters enabling is in progress and the SelectableTimer is also triggered?

If counters enabling is in progress, it means the daemon first changed a boolean to True. so when the timer will expire, it will check the boolean and will not enable the counters as well.

Also, after the enabling of counters, what happens to the new data structure defined in FlexCountersOrch? There can be two cases to be covered here - event-driven mechanism (new design) and timeout (old design).

The data structure will be in use only for the event-driven mechanism.
If we got to a point we need to enable the counters - we will enable the counters for all ports as it was in previous design.
Note: We are using the same behavior as in enable_counters.py script, but here we will trigger the counters enabling when all ports and LAGs are in their expected state.
If the system is not stable, we will do a fallback to the previous design - wait for the timer to expire.

…ounters_redesign

noaOrMlnx · 2022-01-25T13:03:05Z

@vaibhavhd Yes, we don't need it anymore. But the new change need to handle this internally. Means, ignore all events from Config DB and enable after all ports are up or the timeout reached.

@vaibhavhd @shlomibitton
In current design, FLEX_COUNTER_DELAY_STATUS is an optional flag that is being set only when we do fast-boot.
If I will implement your suggestion, we will delay counters enabling after all boot types.
do you think this is the right behavior?

…ounters_redesign

kperumalbfn · 2022-02-01T16:29:55Z

doc/counters_enabling_redesign.md

+    When doTask(SelectableTimer*) function will be triggered, it will check if the counters are already enabled.
+    If not, enable them.
+
+


Please include a section for fast/cold/warm reboot and dynamic port breakout scenarios.

liat-grozovik · 2022-02-01T16:41:55Z

@noaOrMlnx
Please align the HLD according to the HLD template. In additional

Please add a clear table for each type of reboot what will be the max time. please include: reboot, fastboot, warmboot, config reload, swss service restart, etc. It will be helpful to understand the system behaviour
Please add a section for testing. Lets cover the entire flow including the time expired as well as counters start before and timer is stopped.
3.Following the community review feedback need to find a simpler way to declare 'fastboot' completed. based on that we can start counters/stop the max timer.

liat-grozovik · 2022-04-18T07:39:42Z

Following offline discussion we will improve fastboot flow and wont move forward with this HLD. thus it is closed.
Please refer to #980 for more information

Add counters enabling redesign document

e700cfb

lguohan reviewed Jan 4, 2022

View reviewed changes

noaOrMlnx and others added 4 commits January 6, 2022 06:56

Fix design review comments

08d47b6

Fix design review comments

aee2041

Merge branch 'counters_redesign' of github.com:noaOrMlnx/SONiC into c…

5f09f5a

…ounters_redesign

Update counters_enabling_redesign.md

76c85de

yxieca requested a review from vaibhavhd January 10, 2022 17:23

vaibhavhd reviewed Jan 19, 2022

View reviewed changes

Merge branch 'counters_redesign' of github.com:noaOrMlnx/SONiC into c…

56a3483

…ounters_redesign

noaOrMlnx added 9 commits January 25, 2022 13:56

Merge branch 'counters_redesign' of github.com:noaOrMlnx/SONiC into c…

e140018

…ounters_redesign

Merge branch 'counters_redesign' of github.com:noaOrMlnx/SONiC into c…

17ea4a7

…ounters_redesign

Fix review comments

c6e945e

Fix review comments

987cb06

Merge branch 'counters_redesign' of github.com:noaOrMlnx/SONiC into c…

5ac47c4

…ounters_redesign

Merge branch 'counters_redesign' of github.com:noaOrMlnx/SONiC into c…

21d9d14

…ounters_redesign

Merge branch 'counters_redesign' of github.com:noaOrMlnx/SONiC into c…

826d16d

…ounters_redesign

Merge branch 'counters_redesign' of github.com:noaOrMlnx/SONiC into c…

61857c1

…ounters_redesign

Merge branch 'counters_redesign' of github.com:noaOrMlnx/SONiC into c…

1e5a5cd

…ounters_redesign

kperumalbfn reviewed Feb 1, 2022

View reviewed changes

yxieca force-pushed the master branch 2 times, most recently from 8498931 to 8837dc2 Compare April 15, 2022 16:51

liat-grozovik closed this Apr 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add counters enabling redesign document #918

Add counters enabling redesign document #918

noaOrMlnx commented Jan 4, 2022 •

edited

Loading

lguohan Jan 4, 2022

lguohan Jan 4, 2022

noaOrMlnx Jan 9, 2022

lguohan Jan 10, 2022

noaOrMlnx Jan 11, 2022

bingwang-ms commented Jan 5, 2022

yxieca commented Jan 10, 2022

liat-grozovik commented Jan 15, 2022

vaibhavhd commented Jan 18, 2022

vaibhavhd commented Jan 18, 2022

shlomibitton commented Jan 18, 2022

vaibhavhd Jan 18, 2022

noaOrMlnx Jan 24, 2022

vaibhavhd Jan 18, 2022

noaOrMlnx Jan 24, 2022

vaibhavhd Jan 18, 2022

vaibhavhd Jan 18, 2022

noaOrMlnx Jan 24, 2022 •

edited

Loading

noaOrMlnx commented Jan 25, 2022

kperumalbfn Feb 1, 2022

liat-grozovik commented Feb 1, 2022

liat-grozovik commented Apr 18, 2022


		### Ports Daemon

		enable_counters.py script will be refactored to be running as a daemon.


		- The daemon will also wait for a timer in order to be able to enable counters even if one of the ports is not stable.

		If after 3 minutes (180 seconds), the counters were not enabled yet, enable counters.

		When doTask(SelectableTimer*) function will be triggered, it will check if the counters are already enabled.
		If not, enable them.

Add counters enabling redesign document #918

Add counters enabling redesign document #918

Conversation

noaOrMlnx commented Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bingwang-ms commented Jan 5, 2022

yxieca commented Jan 10, 2022

liat-grozovik commented Jan 15, 2022

vaibhavhd commented Jan 18, 2022

vaibhavhd commented Jan 18, 2022

shlomibitton commented Jan 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noaOrMlnx Jan 24, 2022 • edited Loading

Choose a reason for hiding this comment

noaOrMlnx commented Jan 25, 2022

Choose a reason for hiding this comment

liat-grozovik commented Feb 1, 2022

liat-grozovik commented Apr 18, 2022

noaOrMlnx commented Jan 4, 2022 •

edited

Loading

noaOrMlnx Jan 24, 2022 •

edited

Loading