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.

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

[config reload] Fixing config reload when timer based services are disabled #2200

Merged
merged 2 commits into from
Jun 19, 2022

Conversation

dgsudharsan
Copy link
Collaborator

@dgsudharsan dgsudharsan commented Jun 8, 2022

Signed-off-by: Sudharsan Dhamal Gopalarathnam sudharsand@nvidia.com

DO NOT MERGE UNTIL sonic-net/sonic-mgmt#5783 IS MERGED

What I did

Fixed config reload when timer based delayed services are disabled. When they are disabled, the property property=LastTriggerUSecMonotonic returns "0". This will cause config reload to fail even though all enabled services are up.

How I did it

Fixed the delayed services logic to check if the services are enabled before getting the property LastTriggerUSecMonotonic . Additionally fixed the return codes when config reload fails due to system checks

How to verify it

Added UT to verify it. Modified sonic-mgmt tests to verify it additionally.

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
stepanblyschak previously approved these changes Jun 8, 2022
…sabled

Signed-off-by: Sudharsan Dhamal Gopalarathnam <sudharsand@nvidia.com>
config/main.py Outdated

if not _delay_timers_elapsed():
click.echo("Relevant services are not up. Retry later or use -f to avoid system checks")
return
sys.exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

1

Better use different return codes for different reasons.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All these three error conditions are identical which are meant to signal config reload is not ready to be executed. The appropriate error message would give user the hint why it is not ready. I believe since they are for the same behavior the same exit code should be used.
Please let me know your thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you define a constant for "config reload is not ready to be executed" return code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@dgsudharsan
Copy link
Collaborator Author

@qiluo-msft Can we merge this?

liat-grozovik pushed a commit that referenced this pull request Jun 19, 2022
…s are disabled (#2201)

Porting of #2200 into 202111

- What I did
Fixed config reload when timer based delayed services are disabled. When they are disabled, the property property=LastTriggerUSecMonotonic returns "0". This will cause config reload to fail even though all enabled services are up.

- How I did it
Fixed the delayed services logic to check if the services are enabled before getting the property LastTriggerUSecMonotonic . Additionally fixed the return codes when config reload fails due to system checks

- How to verify it
Added UT to verify it. Modified sonic-mgmt tests to verify it additionally.

Signed-off-by: Sudharsan Dhamal Gopalarathnam <sudharsand@nvidia.com>
@liat-grozovik liat-grozovik merged commit 9f2607d into sonic-net:master Jun 19, 2022
@yxieca
Copy link
Contributor

yxieca commented Jun 20, 2022

@dgsudharsan this change cannot be cherry-picked cleanly to 202205 branch, as result, I also cannot cherry-pick sonic-net/sonic-swss#2143 into 202205 at this time.

@dgsudharsan
Copy link
Collaborator Author

@dgsudharsan this change cannot be cherry-picked cleanly to 202205 branch, as result, I also cannot cherry-pick Azure/sonic-swss#2143 into 202205 at this time.

@yxieca I will raise a PR for 202205. But can you explain me how this PR is affecting the sonic-swss PR you mentioned?

dgsudharsan added a commit to dgsudharsan/sonic-utilities that referenced this pull request Jun 21, 2022
…sabled (sonic-net#2200)

- What I did
Fixed config reload when timer based delayed services are disabled. When they are disabled, the property property=LastTriggerUSecMonotonic returns "0". This will cause config reload to fail even though all enabled services are up.

- How I did it
Fixed the delayed services logic to check if the services are enabled before getting the property LastTriggerUSecMonotonic . Additionally fixed the return codes when config reload fails due to system checks

- How to verify it
Added UT to verify it. Modified sonic-mgmt tests to verify it additionally.

Signed-off-by: Sudharsan Dhamal Gopalarathnam <sudharsand@nvidia.com>
@dgsudharsan
Copy link
Collaborator Author

@dgsudharsan this change cannot be cherry-picked cleanly to 202205 branch, as result, I also cannot cherry-pick Azure/sonic-swss#2143 into 202205 at this time.

@yxieca I will raise a PR for 202205. But can you explain me how this PR is affecting the sonic-swss PR you mentioned?

Here is the PR #2226

yxieca pushed a commit that referenced this pull request Jun 22, 2022
…sabled (#2200) (#2226)

- What I did
Fixed config reload when timer based delayed services are disabled. When they are disabled, the property property=LastTriggerUSecMonotonic returns "0". This will cause config reload to fail even though all enabled services are up.

- How I did it
Fixed the delayed services logic to check if the services are enabled before getting the property LastTriggerUSecMonotonic . Additionally fixed the return codes when config reload fails due to system checks

- How to verify it
Added UT to verify it. Modified sonic-mgmt tests to verify it additionally.

Signed-off-by: Sudharsan Dhamal Gopalarathnam <sudharsand@nvidia.com>
yxieca pushed a commit that referenced this pull request Jun 22, 2022
…sabled (#2200) (#2226)

- What I did
Fixed config reload when timer based delayed services are disabled. When they are disabled, the property property=LastTriggerUSecMonotonic returns "0". This will cause config reload to fail even though all enabled services are up.

- How I did it
Fixed the delayed services logic to check if the services are enabled before getting the property LastTriggerUSecMonotonic . Additionally fixed the return codes when config reload fails due to system checks

- How to verify it
Added UT to verify it. Modified sonic-mgmt tests to verify it additionally.

Signed-off-by: Sudharsan Dhamal Gopalarathnam <sudharsand@nvidia.com>
@dgsudharsan dgsudharsan deleted the config_rel_fix branch March 9, 2023 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants