[DPB][YANG] Add POLL_INTERVAL in flex_counter yang model #9276

mykolaxgerasymenko · 2021-11-16T17:49:03Z

Why I did it

DPB falls due to missing POLL_INTERVAL in sonic-flex_counter yang model.

How I did it

Added POLL_INTERVAL leaf to ACL container in sonic-flex_counter yang model.

How to verify it

Run the command config interface breakout <breakout_mode>

NOTE:
To verify this fix, a PR (add set_owner to feature yang) that fix another bug in SONiC should be merged to master.

Which release branch to backport (provide reason below if selected)

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

mykolaxgerasymenko · 2021-11-16T17:54:48Z

@stepanblyschak I found that you added this ACL container to sonic-flex_counter yang model.
Please review my changes.

stepanblyschak · 2021-11-17T00:04:50Z

src/sonic-yang-models/yang-models/sonic-flex_counter.yang

@@ -159,6 +159,11 @@ module sonic-flex_counter {
                leaf FLEX_COUNTER_DELAY_STATUS {
                    type flex_delay_status;
                }
+                leaf POLL_INTERVAL {


How about adding POLL_INTERVAL leaf to every container on the same level here? Why is this fix specific to ACL?

I added the POLL_INTERVAL leaf to ACL container, because only the ACL requires this for DPB feature.
But I will try to add this leaf to other containers and test it.

Do you have other counters enabled in your configuration? I believe if you do, then it will also affect DPB. Don't see anything special about ACL. I think it is better to add it to this PR.

I added POLL_INTERVAL for other containers. But I didn't find poll intervals for some container here: https://github.com/Azure/sonic-utilities/blob/master/counterpoll/main.py
For example, there is no polling interval range for RIF: https://github.com/Azure/sonic-utilities/blob/master/counterpoll/main.py#L254
Therefore, I did not add the poll interval for containers which I did not find in this file.
Maybe you know where I can find these polling intervals?

@noaOrMlnx Please, comment on the RIF poll interval question

you can check here
https://github.com/Azure/sonic-utilities/blob/master/counterpoll/main.py#L336

@noaOrMlnx thank you.
But here only the default value of poll_interval for RIF.
Do we need a range of polling interval here for the RIF as well as for other containers?
Or we can use only default value without range?

Added RIF poll interval

src/sonic-yang-models/yang-models/sonic-flex_counter.yang

mykolaxgerasymenko · 2021-11-18T15:06:37Z

@praveen-li @lguohan @neethajohn
Please review.

praveen-li

Looks good overall to me, Kindly address the comments from other reviewers. Thx

praveen-li · 2021-11-18T17:39:23Z

src/sonic-yang-models/yang-models/sonic-flex_counter.yang

@@ -119,6 +149,12 @@ module sonic-flex_counter {
                leaf FLEX_COUNTER_DELAY_STATUS {
                    type flex_delay_status;
                }
+                leaf POLL_INTERVAL {


If it stayed in same range and with same default value, then we can have typedef similar as Example:flex_delay_status.

But we have three different ranges and three different default values for POLL_INTERVAL for different containers.
Or do you mean that we can have typedef just for range 1000..30000 and default 10000?

@praveen-li
I added a typedef for poll_interval range 1000..30000 and default 10000.
Comments from other reviewers have been resolved.
Please review.

@praveen-li I added a typedef for all poll_interval ranges. Please review.

mykolaxgerasymenko · 2021-11-22T09:33:30Z

/Azp run

azure-pipelines · 2021-11-22T09:33:38Z

Commenter does not have sufficient privileges for PR 9276 in repo Azure/sonic-buildimage

Junchao-Mellanox · 2021-11-29T01:46:35Z

/azpw run Azure.sonic-buildimage

src/sonic-yang-models/yang-models/sonic-flex_counter.yang

qiluo-msft · 2021-12-01T00:30:25Z

src/sonic-yang-models/yang-models/sonic-flex_counter.yang

@@ -39,6 +57,9 @@ module sonic-flex_counter {
                leaf FLEX_COUNTER_DELAY_STATUS {
                    type flex_delay_status;
                }
+                leaf POLL_INTERVAL {
+                    type poll_interval_range_1s_30s;


poll_interval_range_1s_30s

type poll_interval_range_1s_30s;

Is there a technical reason that the maximal is 30s? How about just using non-zero uint32? #Closed

@qiluo-msft
It was suggested in the comments to use ranges from this file: https://github.com/Azure/sonic-utilities/blob/master/counterpoll/main.py#L26
When the PR was opened, this range was from 1 to 30 seconds. But two weeks ago, the maximum was increased to the 60 seconds.
About using non-zero uint32: Do you mean using the whole range for uint32: range 1..4294967295? And have only one maximum range for all containers?
Is this correct?

Yes that's my question. How do you think?

As far as I understand, the POLL_INTERVAL range is limited by the CLI. For example - https://github.com/Azure/sonic-utilities/blob/master/counterpoll/main.py#L94.
I think for consistency, we can have the same maximum range for all containers here. But we should limit the lower bound.
As for me we can use not limiting the upper bound of the polling interval, however the lower bound is useful. Setting the counters polling interval to < 100ms will produce a huge load on the CPU.
The same was written in the comments: #9276 (comment)
So, we can use the same following range for all containers: 100..4294967295.
What do you think?

@qiluo-msft
Extended poll interval range to max uint32 with limiting lower bound: range 100..4294967295.
Please review.

Signed-off-by: Mykola Gerasymenko <[email protected]>

mykolaxgerasymenko · 2021-12-06T16:24:46Z

@qiluo-msft @praveen-li
Addresses comments were resolved. Could you please review?

qiluo-msft

LGTM. Please also check with other reviewers.

qiluo-msft · 2021-12-06T23:17:34Z

Please resolve conflict.

qiluo-msft · 2021-12-07T18:58:35Z

@akokhan, @stepanblyschak, @dgsudharsan Your review conclusions are dismissed due to new commits. Would you like to check again?

mykolaxgerasymenko · 2021-12-07T21:00:04Z

Please resolve conflict.

Conflict has been resolved.

qiluo-msft · 2021-12-07T22:18:39Z

You mentioned

To verify this fix, a PR (add set_owner to feature yang) that fix another bug in SONiC should be merged to master.

Can we merge this PR before that one?

mykolaxgerasymenko · 2021-12-08T07:27:12Z

You mentioned

To verify this fix, a PR (add set_owner to feature yang) that fix another bug in SONiC should be merged to master.

Can we merge this PR before that one?

@qiluo-msft
I think we can merge this PR before #9075. These are two independent issues.
But merging can lead to conflicts in 9075 that need to be resolved.
Also we can merge #9418.

…#9418) Fixes #9326 #### Why I did it When we try execute DPB from CLI we have error: `libyang[0]: Invalid value "False" in "has_global_scope" element. (path: /sonic-feature:sonic-feature/FEATURE/FEATURE_LIST[name='bgp']/has_global_scope)` The reason for this issue is that has_global_scope and other have been stored in redis database with value False or True form capital letter: ``` "FEATURE":{ "bgp":{ "auto_restart":"enabled", "has_global_scope":"False", "has_per_asic_scope":"True", "has_timer":"False", "high_mem_alert":"disabled", "state":"enabled" } ``` But yang model support boolean just in lowercase letters (https://datatracker.ietf.org/doc/html/rfc6020#section-9.5.1). #### How I did it Added boolean to sonic-types as typedef with different literal cases. #### How to verify it Run the command config interface breakout <breakout_mode> **NOTE:** To verify this fix, the following PRs that fix other problems in SONiC must be merged into master: 1) /pull/9075 2) /pull/9276

#### Why I did it DPB falls due to missing POLL_INTERVAL in sonic-flex_counter yang model. #### How I did it Added POLL_INTERVAL leaf to ACL container in sonic-flex_counter yang model. #### How to verify it Run the command config interface breakout <interface> <breakout_mode> **NOTE:** To verify this fix, a PR ([add set_owner to feature yang](#9075)) that fix another bug in SONiC should be merged to master.

arlakshm · 2022-03-01T03:56:48Z

cherry-pick failing in 202106. Please create a new PR

mykolaxgerasymenko requested a review from lguohan as a code owner November 16, 2021 17:49

akokhan previously approved these changes Nov 16, 2021

View reviewed changes

stepanblyschak reviewed Nov 17, 2021

View reviewed changes

stepanblyschak mentioned this pull request Nov 17, 2021

[DPB] Unable to breakout a port on Mellanox SN2700 #8993

Closed

mykolaxgerasymenko dismissed akokhan’s stale review via 213ae64 November 17, 2021 18:10

stepanblyschak previously approved these changes Nov 17, 2021

View reviewed changes

praveen-li reviewed Nov 18, 2021

View reviewed changes

mykolaxgerasymenko mentioned this pull request Nov 19, 2021

[DPB] DPB falls cause of invalid value in "has_global_scope" element #9326

Closed

mykolaxgerasymenko dismissed stepanblyschak’s stale review via 785b1e0 November 19, 2021 16:23

mykolaxgerasymenko requested a review from qiluo-msft as a code owner November 19, 2021 16:23

mykolaxgerasymenko requested review from praveen-li, stepanblyschak and noaOrMlnx November 19, 2021 16:43

mykolaxgerasymenko force-pushed the mykolag/poll_interval_missing_in_yang branch from cf41ab8 to 052332b Compare November 22, 2021 10:21

qiluo-msft added the YANG YANG model related changes label Nov 22, 2021

mykolaxgerasymenko force-pushed the mykolag/poll_interval_missing_in_yang branch 2 times, most recently from 97ade43 to 7ef8fbf Compare November 24, 2021 18:06

stepanblyschak mentioned this pull request Nov 26, 2021

[sonic-utilities] update submodule #9379

Closed

5 tasks

mykolaxgerasymenko force-pushed the mykolag/poll_interval_missing_in_yang branch from 7ef8fbf to 84da083 Compare November 26, 2021 09:12

mykolaxgerasymenko force-pushed the mykolag/poll_interval_missing_in_yang branch from 84da083 to ac4a143 Compare November 30, 2021 11:11

dgsudharsan suggested changes Nov 30, 2021

View reviewed changes

src/sonic-yang-models/yang-models/sonic-flex_counter.yang Outdated Show resolved Hide resolved

mykolaxgerasymenko requested a review from dgsudharsan November 30, 2021 18:30

mykolaxgerasymenko force-pushed the mykolag/poll_interval_missing_in_yang branch from f9778ca to 0514390 Compare November 30, 2021 18:46

dgsudharsan previously approved these changes Nov 30, 2021

View reviewed changes

qiluo-msft reviewed Dec 1, 2021

View reviewed changes

mykolaxgerasymenko added 5 commits December 1, 2021 07:02

Add POLL_INTERVAL for other containers and add tests

2496b3a

Signed-off-by: Mykola Gerasymenko <[email protected]>

Add typedef for poll_interval range 1s..30s

9d6b480

Add default value to typedef

24cb279

Remove default values and add typedef for all ranges

9d5bc98

Fix typo with semicolon and increase range to 60s

8ad2f2a

mykolaxgerasymenko mentioned this pull request Dec 1, 2021

[DPB][YANG] Fix cases when boolean is used in different literal cases #9418

Merged

5 tasks

mykolaxgerasymenko added 2 commits December 3, 2021 07:13

Extended poll interval range to max uint32 and added test cases

3c7326f

Fixup typo

097db8e

Signed-off-by: Mykola Gerasymenko <[email protected]>

mykolaxgerasymenko requested review from qiluo-msft and praveen-li December 6, 2021 16:26

qiluo-msft previously approved these changes Dec 6, 2021

View reviewed changes

Merge branch 'master' into mykolag/poll_interval_missing_in_yang

cc10e5e

mykolaxgerasymenko dismissed qiluo-msft’s stale review via cc10e5e December 7, 2021 11:03

qiluo-msft approved these changes Dec 7, 2021

View reviewed changes

dgsudharsan approved these changes Dec 7, 2021

View reviewed changes

stepanblyschak approved these changes Dec 7, 2021

View reviewed changes

qiluo-msft merged commit b0f06ba into sonic-net:master Dec 8, 2021

dgsudharsan added the Request for 202106 Branch label Dec 14, 2021

judyjoseph added the Included in 202111 Branch label Dec 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DPB][YANG] Add POLL_INTERVAL in flex_counter yang model #9276

[DPB][YANG] Add POLL_INTERVAL in flex_counter yang model #9276

mykolaxgerasymenko commented Nov 16, 2021

mykolaxgerasymenko commented Nov 16, 2021

stepanblyschak Nov 17, 2021

mykolaxgerasymenko Nov 17, 2021

stepanblyschak Nov 17, 2021

mykolaxgerasymenko Nov 17, 2021

stepanblyschak Nov 17, 2021

noaOrMlnx Nov 18, 2021

mykolaxgerasymenko Nov 18, 2021

mykolaxgerasymenko Dec 3, 2021

mykolaxgerasymenko commented Nov 18, 2021

praveen-li left a comment

praveen-li Nov 18, 2021

mykolaxgerasymenko Nov 18, 2021 •

edited

Loading

mykolaxgerasymenko Nov 19, 2021

mykolaxgerasymenko Nov 30, 2021

mykolaxgerasymenko commented Nov 22, 2021

azure-pipelines bot commented Nov 22, 2021

Junchao-Mellanox commented Nov 29, 2021

qiluo-msft Dec 1, 2021 •

edited

Loading

mykolaxgerasymenko Dec 1, 2021 •

edited

Loading

qiluo-msft Dec 1, 2021

mykolaxgerasymenko Dec 2, 2021 •

edited

Loading

mykolaxgerasymenko Dec 6, 2021

mykolaxgerasymenko commented Dec 6, 2021

qiluo-msft left a comment

qiluo-msft commented Dec 6, 2021

qiluo-msft commented Dec 7, 2021

mykolaxgerasymenko commented Dec 7, 2021

qiluo-msft commented Dec 7, 2021

mykolaxgerasymenko commented Dec 8, 2021

arlakshm commented Mar 1, 2022

[DPB][YANG] Add POLL_INTERVAL in flex_counter yang model #9276

[DPB][YANG] Add POLL_INTERVAL in flex_counter yang model #9276

Conversation

mykolaxgerasymenko commented Nov 16, 2021

Why I did it

How I did it

How to verify it

Which release branch to backport (provide reason below if selected)

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

mykolaxgerasymenko commented Nov 16, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykolaxgerasymenko commented Nov 18, 2021

praveen-li left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykolaxgerasymenko Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykolaxgerasymenko commented Nov 22, 2021

azure-pipelines bot commented Nov 22, 2021

Junchao-Mellanox commented Nov 29, 2021

qiluo-msft Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

type poll_interval_range_1s_30s;

mykolaxgerasymenko Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykolaxgerasymenko Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykolaxgerasymenko commented Dec 6, 2021

qiluo-msft left a comment

Choose a reason for hiding this comment

qiluo-msft commented Dec 6, 2021

qiluo-msft commented Dec 7, 2021

mykolaxgerasymenko commented Dec 7, 2021

qiluo-msft commented Dec 7, 2021

mykolaxgerasymenko commented Dec 8, 2021

arlakshm commented Mar 1, 2022

mykolaxgerasymenko Nov 18, 2021 •

edited

Loading

qiluo-msft Dec 1, 2021 •

edited

Loading

mykolaxgerasymenko Dec 1, 2021 •

edited

Loading

mykolaxgerasymenko Dec 2, 2021 •

edited

Loading