Skip to content

[featured] Fix namespace CONFIG_DBs rendering and cleanup#373

Open
william8545 wants to merge 3 commits into
sonic-net:masterfrom
william8545:multi_asic_feature_sync_public
Open

[featured] Fix namespace CONFIG_DBs rendering and cleanup#373
william8545 wants to merge 3 commits into
sonic-net:masterfrom
william8545:multi_asic_feature_sync_public

Conversation

@william8545

@william8545 william8545 commented Apr 10, 2026

Copy link
Copy Markdown

What I did

Fixed two issues in featured daemon for multi-ASIC platforms:

  1. Feature deregistration now cleans up FEATURE entries from all per-ASIC namespace CONFIG_DBs and STATE_DBs.
  2. resync_feature_state() now independently checks each namespace CONFIG_DB for unrendered Jinja2 templates, instead of assuming all namespaces match the host.

How I did it

Deregistration cleanup: In FeatureHandler.handler(), when a feature is deregistered (feature_cfg is empty), added cleanup of the FEATURE entry from all namespace CONFIG_DBs via set_entry(None) and from all namespace STATE_DBs via _del().

Independent namespace rendering: In resync_feature_state(), removed the early return when host state matches the rendered state. The host CONFIG_DB update still only happens for immutable or template states. Added a new loop that independently checks each namespace CONFIG_DB — if a namespace entry still contains a Jinja2 template, it gets rendered to the correct state value regardless of host CONFIG_DB state.

How to verify it

On a multi-ASIC system:

# Test deregistration cleanup
sudo sonic-package-manager install xxx -y
sudo sonic-package-manager uninstall xxx -y --force
# Verify per-ASIC entries are removed
sonic-db-cli -n asic0 CONFIG_DB HGETALL "FEATURE|xxx"
# Should return {}

# Test template rendering
# After boot, check that per-ASIC FEATURE entries have rendered state values
sonic-db-cli -n asic0 CONFIG_DB HGET "FEATURE|xxx" state

Signed-off-by: William Tsai <willtsai@nvidia.com>
Signed-off-by: William Tsai <willtsai@nvidia.com>
@mssonicbld

Copy link
Copy Markdown

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@william8545 william8545 marked this pull request as ready for review April 10, 2026 19:57
@Sourabh-Kumar7 Sourabh-Kumar7 requested review from abdosi and stepanblyschak and removed request for stepanblyschak May 14, 2026 02:01
@Sourabh-Kumar7

Copy link
Copy Markdown
Member

@abdosi could you pls help with review?

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes multi-ASIC behavior in the featured host daemon by ensuring feature deregistration fully cleans up per-namespace databases, and by making template rendering/resync logic operate independently per namespace rather than assuming host and namespace CONFIG_DB entries are aligned.

Changes:

  • On feature deregistration, remove FEATURE|<name> from all namespace CONFIG_DBs and STATE_DBs.
  • Update resync_feature_state() to always check each namespace CONFIG_DB for template values and render them even when the host CONFIG_DB already matches the rendered state.
  • Add unit tests covering namespace cleanup on deregistration and namespace-only template rendering behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
tests/featured/featured_test.py Adds unit tests for namespace cleanup during deregistration and namespace template rendering in resync_feature_state().
scripts/featured Implements namespace cleanup on deregistration and independent namespace template rendering/resync logic.

Comment thread scripts/featured Outdated
Comment on lines 533 to 535
current_entry = self._config_db.get_entry('FEATURE', feature.name)
current_feature_state = current_entry.get('state') if current_entry else None

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

fix

Comment thread scripts/featured Outdated
Comment on lines 551 to 555
for ns, db in self.ns_cfg_db.items():
ns_entry = db.get_entry('FEATURE', feature.name)
ns_state = ns_entry.get('state') if ns_entry else None
if ns_state is not None and self._feature_state_is_template(ns_state):
db.mod_entry('FEATURE', feature.name, {'state': feature.state})
Comment thread tests/featured/featured_test.py Outdated
feature_handler.handler('sflow', 'DEL', {})

mock_feature_state_table._del.assert_called_once_with('sflow')
mock_ns_cfg_db.set_entry.assert_called_once_with('FEATURE', 'sflow', None)
Comment thread tests/featured/featured_test.py Outdated
@mock.patch('featured.FeatureHandler.update_feature_state', mock.MagicMock())
@mock.patch('featured.FeatureHandler.sync_feature_scope', mock.MagicMock())
@mock.patch('featured.FeatureHandler.sync_feature_delay_state', mock.MagicMock())
def test_namespace_template_rendered_when_host_matches(self):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

None of the four patched methods are reachable, so the decorators are dead weight that makes the test noisier than it needs to be. Drop them.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

fix

Signed-off-by: William Tsai <willtsai@nvidia.com>
@mssonicbld

Copy link
Copy Markdown

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@Sourabh-Kumar7

Copy link
Copy Markdown
Member

@abdosi @qiluo-msft could you please help re-review the change?

@abdosi

abdosi commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

what is reason for making this change ?

@william8545

Copy link
Copy Markdown
Author

what is reason for making this change ?

This is the featured-daemon half of a two-part fix for multi-ASIC platforms, where per-ASIC namespace FEATURE state diverged from the host for dynamically installed packages — config feature state enabled failed, and uninstall left stale entries in the ASIC namespaces. The package-manager/CLI half is here: sonic-net/sonic-utilities#4445 featured subscribes only to the host CONFIG_DB and is what propagates state into the namespaces, so three things can only be done here:

  1. STATE_DB cleanup on deregister
  2. Independent per-namespace rendering in resync_feature_state()
  3. Resync on (re-)registration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants