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] Fix issue: FeatureHandler might override user configuration #58

Merged
merged 2 commits into from
Apr 27, 2023

Conversation

Junchao-Mellanox
Copy link
Contributor

Why I did this?

hostcfgd is using a subscribe/listen way to handle configuration data. It handles feature configuration like this:

  1. subscribe to CONFIG DB FEATURE table
  2. read initial data from CONFIG DB FEATURE table and call FeatureHandler.sync_state_field to process the initial data
  3. As FEATURE table "state" field might be a template, FeatureHandler is responsible for rendering it
  4. FeatureHandler will "resync" the rendered "state" back to CONFIG DB

However, if there is a user configuration occurs between step 2 and step 3, user configuration will be silently ignored and override. Here is an example:

  1. subscribe to CONFIG DB FEATURE table
  2. read initial FEATURE data, say sflow.state = disabled. FeatureHandler.sync_state_field get an input that "sflow.state = disabled"
  3. user issued command "config feature state sflow enabled". Now sflow.state is enabled in FEATURE table.
  4. FeatureHandler.sync_state_field continues running and resync "sflow.state = disabled" back to FEATURE table

In this case, user configuration in step#3 will be override. The PR is a fix for this issue.

How I verified?

Added new unit test

----------- coverage: platform linux, python 3.9.2-final-0 -----------
Name                             Stmts   Miss  Cover
----------------------------------------------------
host_modules/config_engine.py       35      1    97%
host_modules/gcu.py                 54      0   100%
host_modules/host_service.py        19      3    84%
host_modules/showtech.py            27      7    74%
scripts/caclmgrd                   608    211    65%
scripts/determine-reboot-cause     144     13    91%
scripts/hostcfgd                  1236    261    79%
scripts/procdockerstatsd           157     90    43%
----------------------------------------------------
TOTAL                             2280    586    74%
Coverage HTML written to dir htmlcov
Coverage XML written to file coverage.xml

======================= 104 passed, 4 warnings in 6.44s ========================

@Junchao-Mellanox Junchao-Mellanox added the bug Something isn't working label Apr 25, 2023
@Junchao-Mellanox
Copy link
Contributor Author

Hi @ganglyu , would you please kindly review?

Copy link
Collaborator

@ganglyu ganglyu left a comment

Choose a reason for hiding this comment

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

LGTM

@Junchao-Mellanox
Copy link
Contributor Author

Hi @ganglyu , could you please help merge this?

@Junchao-Mellanox
Copy link
Contributor Author

Junchao-Mellanox commented Apr 27, 2023

It causes UT failure after cherry-picking to 202211, so I created a backport PR for 202211: #59

liat-grozovik pushed a commit that referenced this pull request May 4, 2023
…figuration (#58) (#59)

- Why I did this?
hostcfgd is using a subscribe/listen way to handle configuration data. It handles feature configuration like this:

subscribe to CONFIG DB FEATURE table
read initial data from CONFIG DB FEATURE table and call FeatureHandler.sync_state_field to process the initial data
As FEATURE table "state" field might be a template, FeatureHandler is responsible for rendering it
FeatureHandler will "resync" the rendered "state" back to CONFIG DB
However, if there is a user configuration occurs between step 2 and step 3, user configuration will be silently ignored and override. Here is an example:

subscribe to CONFIG DB FEATURE table
read initial FEATURE data, say sflow.state = disabled. FeatureHandler.sync_state_field get an input that "sflow.state = disabled"
user issued command "config feature state sflow enabled". Now sflow.state is enabled in FEATURE table.
FeatureHandler.sync_state_field continues running and resync "sflow.state = disabled" back to FEATURE table
In this case, user configuration in step#3 will be override. The PR is a fix for this issue.

- How I verified?
Added new unit test
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.

6 participants