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

[hostcfgd] check cached state instead of the next state #6067

Merged
merged 2 commits into from
Dec 2, 2020

Conversation

yxieca
Copy link
Contributor

@yxieca yxieca commented Nov 30, 2020

- Why I did it
'always_enabled' feature can still be disabled/enabled.

- How I did it
When checking if a feature is 'always_enabled', check the cached state to prevent new change to be accepted.
Fix an issue where cache value is updated before all the check is done.
Restore 'always_enabled' value in config db if someone wants to change.

Signed-off-by: Ying Xie ying.xie@microsoft.com

- How to verify it
Without the change:
root@str2-dx010-acs-6:/home/admin# show feature status
Feature State AutoRestart


...
teamd always_enabled enabled
...

root@str2-dx010-acs-6:/home/admin# config feature state teamd disabled
root@str2-dx010-acs-6:/home/admin# show feature status
Feature State AutoRestart


...
teamd disabled enabled
...

With the fix:
root@str2-dx010-acs-6:/home/admin# config feature state teamd disabled
root@str2-dx010-acs-6:/home/admin# show feature status
Feature State AutoRestart


...
teamd always_enabled enabled
...

  • 201811
  • 201911
  • 202006

When checking if a feature is 'always_enabled', check the
cached state to prevent new change to be accepted.

Signed-off-by: Ying Xie <ying.xie@microsoft.com>
jleveque
jleveque previously approved these changes Nov 30, 2020
@abdosi
Copy link
Contributor

abdosi commented Dec 1, 2020

@yxieca I think we do not need this change this once we update sonic-util submodule which will bring change from this PR: sonic-net/sonic-utilities#1271

PR for submodule update: #6070

@yxieca
Copy link
Contributor Author

yxieca commented Dec 1, 2020

@yxieca I think we do not need this change this once we update sonic-util submodule which will bring change from this PR: Azure/sonic-utilities#1271

PR for submodule update: #6070

@abdosi I think we still need this change. I fixed 2 logic issues in this change and added a protection (reverting wrong change). I hope we never need to exercise the protection. But the 2 logic error fixes are necessary.

@yxieca
Sure, but I think there is no way to trigger those logical errors as of now

a) We update cache state first in update_all_feature_states. This make sure we get “always_enabled”
b) After that state it can not be updated (via CLI)

Idea to use next ‘state’ (user provided value) to protect was based on assumption sonic-util has correct check.

Copy link
Collaborator

@lguohan lguohan left a comment

Choose a reason for hiding this comment

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

extra protection, looks ok.

Copy link
Contributor

@abdosi abdosi left a comment

Choose a reason for hiding this comment

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

LGTM. The change helps "always_enabled" to be maintain and not to be over-written in any case
as extra check over CLI command.

@yxieca yxieca merged commit 443f81f into sonic-net:master Dec 2, 2020
@yxieca yxieca deleted the hostcfgd branch December 2, 2020 01:56
santhosh-kt pushed a commit to santhosh-kt/sonic-buildimage that referenced this pull request Feb 25, 2021
- Why I did it
'always_enabled' feature can still be disabled/enabled.

- How I did it
When checking if a feature is 'always_enabled', check the cached state to prevent new change to be accepted.
Fix an issue where cache value is updated before all the check is done.
Restore 'always_enabled' value in config db if someone wants to change.

Signed-off-by: Ying Xie ying.xie@microsoft.com

- How to verify it
Without the fix, 'always_enabled' feature can be enabled or disabled without cli protection. With the protection, the change will be rejected properly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants