-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FC] remove FC delay status field #20495
[FC] remove FC delay status field #20495
Conversation
Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>
@@ -39,7 +36,7 @@ def enable_rates(): | |||
def enable_counters(): | |||
db = swsscommon.ConfigDBConnector() | |||
db.connect() | |||
default_enabled_counters = ['PORT', 'RIF', 'QUEUE', 'PFCWD', 'PG_WATERMARK', 'PG_DROP', | |||
default_enabled_counters = ['PORT', 'RIF', 'QUEUE', 'PFCWD', 'PG_WATERMARK', 'PG_DROP', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we put those entry to init_cfg.json?
If I understand it right, based on line44-45, the FLEX table would still change from
"FLEX_COUNTER_TABLE": {
"ACL": {
"FLEX_COUNTER_STATUS": "enable",
"POLL_INTERVAL": "10000"
}
},
to
"FLEX_COUNTER_TABLE": {
"ACL": {
"FLEX_COUNTER_STATUS": "enable",
"POLL_INTERVAL": "10000"
},
"PORT": {
"FLEX_COUNTER_STATUS": "enable",
"POLL_INTERVAL": "10000"
},
"RIF": {
"FLEX_COUNTER_STATUS": "enable",
"POLL_INTERVAL": "10000"
},
...
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wen587 this is not related to delay status field removal, will be in seperate pr
lgtm |
@@ -50,9 +50,6 @@ module sonic-flex_counter { | |||
leaf FLEX_COUNTER_STATUS { | |||
type flex_status; | |||
} | |||
leaf FLEX_COUNTER_DELAY_STATUS { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put off the YANG model change? I am wondering if there is gap between the config change and yang model change, which will cause GCU failure in PROD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wen587 Who is setting FLEX_COUNTER_DELAY_STATUS field? I removed it from the code base, so removing it from the yang model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean during config migration. If a device was on an image which has FLEX_DELAY field in old config, if we boot up with new image it will carry over the old config which has FLEX_DELAY field.
However, the YANG model will fail to recognize this field because of the removal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wen587 Please check utilities PR sonic-net/sonic-utilities#3577.
It handles this flow in db_migrator.py. It should remove this field during DB migration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wen587 Discussed offline, there's a bypass of db migrator, thus reverted yang model change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @stepanblyschak , just double confirm:
If we have a old config carried by old version, this is also valid. right?
"FLEX_COUNTER_TABLE": {
"ACL": {
"FLEX_COUNTER_STATUS": "enable",
"FLEX_COUNTER_DELAY_STATUS": "false",
"POLL_INTERVAL": "10000"
}
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wen587 Yes
Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>
@qiluo-msft Can you please help merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Hi @stepanblyschak , could you pull recent change to this PR? Want to make sure it cover the change you made in init_cfg.json":#20506 |
Hi @qiluo-msft please help merge it. |
Related to sonic-net/sonic-swss#3326. Why I did it Simplify approach to delaying counters on warm boot and fast boot. Removed FLEX_COUNTER_DELAY_STATUS_FIELD and instead postpone all FC processing to happen after apply view to not delay data plane configuration. The CONFIG_DB should not be updated in runtime anymore for counters to be delayed. To address sonic-net#20302. Work item tracking Microsoft ADO (number only): How I did it Removed FLEX_COUNTER_DELAY_STATUS_FIELD set in enable_counters.py. How to verify it Run warm-boot - make sure FC orch runs only after APPLY_VIEW.
Related to sonic-net/sonic-swss#3326.
Why I did it
Simplify approach to delaying counters on warm boot and fast boot. Removed FLEX_COUNTER_DELAY_STATUS_FIELD and instead postpone all FC processing to happen after apply view to not delay data plane configuration.
The CONFIG_DB should not be updated in runtime anymore for counters to be delayed.
To address #20302.
Work item tracking
How I did it
Removed FLEX_COUNTER_DELAY_STATUS_FIELD set in enable_counters.py.
How to verify it
Run warm-boot - make sure FC orch runs only after APPLY_VIEW.
Which release branch to backport (provide reason below if selected)
Tested branch (Please provide the tested image version)
Description for the changelog
Link to config_db schema for YANG module changes
A picture of a cute animal (not mandatory but encouraged)