Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[FC] remove FC delay field #3577

Merged
merged 5 commits into from
Feb 11, 2025
Merged

[FC] remove FC delay field #3577

merged 5 commits into from
Feb 11, 2025

Conversation

stepanblyschak
Copy link
Contributor

What I did

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.

How I did it

Removed FLEX_COUNTER_DELAY_STATUS_FIELD and corresponding counterpoll commands (that weren't supposed to be used by user directly anyway). Updated db_migrator.py to remove that field.

How to verify it

Ran fast-reboot from 202405. Made sure no delay field present, counters are enabled and delayed.

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

@stepanblyschak
[FC] remove FC delay field
9163729 
Signed-off-by: Stepan Blyschak <[email protected]>
@stepanblyschak
remmove delay tests
c3bbe05 
Signed-off-by: Stepan Blyschak <[email protected]>
@wen587 wen587 requested review from vaibhavhd and ganglyu October 22, 2024 06:16
@stepanblyschak stepanblyschak mentioned this pull request Oct 22, 2024
Merged
11 tasks
@stepanblyschak
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bingwang-ms
Copy link
Contributor

@qiluo-msft @wen587 Please help review

@qiluo-msft qiluo-msft requested a review from wen587 November 11, 2024 17:49
qiluo-msft
qiluo-msft previously approved these changes Dec 2, 2024
View reviewed changes
@qiluo-msft
Copy link
Contributor

@vaibhavhd Could you review fastreboot, and db_migrator parts?

@stepanblyschak
Merge branch 'master' of https://github.com/sonic-net/sonic-utilities …
0a3be59 
…into remove-fc-delay

Signed-off-by: Stepan Blyschak <[email protected]>
@stepanblyschak
Copy link
Contributor Author 

==============================================================================
##[error]Not found workingDirectory: /__w/1/target/debs/bullseye/
Finishing: Install Debian dependencies

Will restart

@stepanblyschak
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bingwang-ms
Copy link
Contributor

/azp run Azure.sonic-utilities

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bingwang-ms
Copy link
Contributor

@stepanblyschak Can you please sync code with master branch? The PR test has been fixed in master branch.

vaibhavhd
vaibhavhd reviewed Dec 23, 2024
View reviewed changes
scripts/db_migrator.py Outdated
@@ -1170,8 +1168,6 @@ def version_4_0_2(self):
Version 4_0_2.
"""
log.log_info('Handling version_4_0_2')
if self.stateDB.keys(self.stateDB.STATE_DB, "FAST_REBOOT|system"):
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing the functionality of old and already deployed versions is not generally recommended - the db version handling should be same on all feature branches. Otherwise it creates conflicting views and also causes confusion.

I think you can leave version_4_0_2 and migrate_config_db_flex_counter_delay_status untouched, and handle your new intention in a new function (as you are already doing in common ops).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vaibhavhd Done

@vaibhavhd
Copy link
Contributor

@shlomibitton can you review this PR?

@stepanblyschak
Merge branch 'master' of https://github.com/sonic-net/sonic-utilities …
cbd441f 
…into remove-fc-delay

Signed-off-by: Stepan Blyschak <[email protected]>
@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liat-grozovik
Copy link
Collaborator

@vaibhavhd kindly reminder to have this reviewed and merged

@bingwang-ms
Copy link
Contributor

@qiluo-msft , @wen587 Can you help review again?

vaibhavhd
vaibhavhd reviewed Feb 6, 2025
View reviewed changes
scripts/db_migrator.py
@@ -1249,6 +1261,7 @@ def version_202505_01(self):
master branch until 202505 branch is created.
"""
log.log_info('Handling version_202505_01')
self.migrate_flex_counter_delay_status_removal()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you are missing set version call to version_202505_02 and a placeholder function for version_202505_02.

Copy link
Contributor

Choose a reason for hiding this comment

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

Before your change the version is already set to version_202505_01, so with your change (a new migration), the version should be moved to version_202505_02, and that's where migrate_flex_counter_delay_status_removal will get called. Please check previous pr examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vaibhavhd I am confused by the comment on this function:

Version 202505_01, this version should be the final version for
        master branch until 202505 branch is created.

So, I didn't create a new version.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I think by "final version" the comment is referring to the major final version, which is "version_202505_*".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vaibhavhd unless someone is doing master -> master upgrades no need to create intermediate versions.
This approach makes too many conflicts. Before branch is out it is easier to put all in one version handler and then split versions.

@yxieca Could you please advise how to handle versions in db_migrator when the branch is not out yet?

@bingwang-ms bingwang-ms merged commit 3abd19e into sonic-net:master Feb 11, 2025
7 checks passed
@stepanblyschak stepanblyschak mentioned this pull request Feb 17, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vaibhavhd vaibhavhd vaibhavhd left review comments

@wen587 wen587 wen587 approved these changes

@qiluo-msft qiluo-msft qiluo-msft left review comments

@ganglyu ganglyu Awaiting requested review from ganglyu

Assignees
No one assigned
Labels
Cherry Pick Conflict_202411 Included in 202411 Branch Request for 202411 Branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@stepanblyschak @mssonicbld @bingwang-ms @qiluo-msft @vaibhavhd @liat-grozovik @wen587 @kperumalbfn @dprital