[hostcfgd] Enable/disable the container service only when the feature state was changed. #5689
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
the 'AutoRestart' field of swss/snmp container, snmp container will be stopped and started. Signed-off-by: Yong Zhao <yozhao@microsoft.com>
Signed-off-by: Yong Zhao <yozhao@microsoft.com>
jleveque
suggested changes
Oct 21, 2020
There was a problem hiding this comment.
The PR title is a bit lengthy yet does not accurately describe the change being made. It instead describes one side effect of the change. The title, which will subsequently become the commit message, should summarize the actual change being made. "[hostcfgd] Fix the SNMP container stopped and started if 'AutoRestart' state of snmp/swss was changed" does not summarize the change; SNMP/SwSS should not be mentioned in the title -- it can be mentioned in the description. The actual change here is to invoke systemd commands only if the feature state has changed from its previous state, and not if other data in the FEATURE table has changed. Please try to condense what I wrote above down into a succinct and brief title.
jleveque
suggested changes
Oct 21, 2020
yxieca
previously approved these changes
Oct 21, 2020
yozhao101
changed the title
Signed-off-by: Yong Zhao <yozhao@microsoft.com>
lguohan
approved these changes
Oct 21, 2020
jleveque
approved these changes
Oct 21, 2020
abdosi
approved these changes
Oct 22, 2020
yxieca
approved these changes
Oct 22, 2020
|
retest vsimage please |
abdosi
pushed a commit
that referenced
this pull request
Oct 23, 2020
… state was changed. (#5689) **- Why I did it** If we ran the CLI commands `sudo config feature autorestart snmp disabled/enabled` or `sudo config feature autorestart swss disabled/enabled`, then SNMP container will be stopped and started. This behavior was not expected since we updated the `auto_restart` field not update `state` field in `FEATURE` table. The reason behind this issue is that either `state` field or `auto_restart` field was updated, the function `update_feature_state(...)` will be invoked which then starts snmp.timer service. The snmp.timer service will first stop snmp.service and later start snmp.service. In order to solve this issue, the function `update_feature_state(...)` will be only invoked if `state` field in `FEATURE` table was updated. **- How I did it** When the demon `hostcfgd` was activated, all the values of `state` field in `FEATURE` table of each container will be cached. Each time the function `feature_state_handler(...)` is invoked, it will determine whether the `state` field of a container was changed or not. If it was changed, function `update_feature_state(...)` will be invoked and the cached value will also be updated. Otherwise, nothing will be done. **- How to verify it** We can run the CLI commands `sudo config feature autorestart snmp disabled/enabled` or `sudo config feature autorestart swss disabled/enabled` to check whether SNMP container is stopped and started. We also can run the CLI commands `sudo config feature state snmp disabled/enabled` or `sudo config feature state swss disabled/enabled` to check whether the container is stopped and restarted. Signed-off-by: Yong Zhao <yozhao@microsoft.com>
Merged
3 tasks
santhosh-kt
pushed a commit
to santhosh-kt/sonic-buildimage
that referenced
this pull request
Feb 25, 2021
… state was changed. (sonic-net#5689) **- Why I did it** If we ran the CLI commands `sudo config feature autorestart snmp disabled/enabled` or `sudo config feature autorestart swss disabled/enabled`, then SNMP container will be stopped and started. This behavior was not expected since we updated the `auto_restart` field not update `state` field in `FEATURE` table. The reason behind this issue is that either `state` field or `auto_restart` field was updated, the function `update_feature_state(...)` will be invoked which then starts snmp.timer service. The snmp.timer service will first stop snmp.service and later start snmp.service. In order to solve this issue, the function `update_feature_state(...)` will be only invoked if `state` field in `FEATURE` table was updated. **- How I did it** When the demon `hostcfgd` was activated, all the values of `state` field in `FEATURE` table of each container will be cached. Each time the function `feature_state_handler(...)` is invoked, it will determine whether the `state` field of a container was changed or not. If it was changed, function `update_feature_state(...)` will be invoked and the cached value will also be updated. Otherwise, nothing will be done. **- How to verify it** We can run the CLI commands `sudo config feature autorestart snmp disabled/enabled` or `sudo config feature autorestart swss disabled/enabled` to check whether SNMP container is stopped and started. We also can run the CLI commands `sudo config feature state snmp disabled/enabled` or `sudo config feature state swss disabled/enabled` to check whether the container is stopped and restarted. Signed-off-by: Yong Zhao <yozhao@microsoft.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Yong Zhao yozhao@microsoft.com
- Why I did it
If we ran the CLI commands
sudo config feature autorestart snmp disabled/enabledorsudo config feature autorestart swss disabled/enabled, then SNMP container will be stopped and started. This behavior was not expected since we updated theauto_restartfield not updatestatefield inFEATUREtable. The reason behind this issue is that eitherstatefield orauto_restartfield was updated, the functionupdate_feature_state(...)will be invoked which then starts snmp.timer service.The snmp.timer service will first stop snmp.service and later start snmp.service.
In order to solve this issue, the function
update_feature_state(...)will be only invoked ifstatefield inFEATUREtable wasupdated.
- How I did it
When the demon
hostcfgdwas activated, all the values ofstatefield inFEATUREtable of each container will becached. Each time the function
feature_state_handler(...)is invoked, it will determine whether thestatefield of acontainer was changed or not. If it was changed, function
update_feature_state(...)will be invoked and the cachedvalue will also be updated. Otherwise, nothing will be done.
- How to verify it
We can run the CLI commands
sudo config feature autorestart snmp disabled/enabledorsudo config feature autorestart swss disabled/enabledto check whether SNMP container is stopped and started. We also can run the CLI commandssudo config feature state snmp disabled/enabledorsudo config feature state swss disabled/enabledto check whether the container is stopped and restarted.- Which release branch to backport (provide reason below if selected)
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)