Fix multi-ASIC package feature management#4445
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: William Tsai <willtsai@nvidia.com>
c0d677a to
6b7d048
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
securely1g
left a comment
There was a problem hiding this comment.
Review on behalf of @lguohan:
The fix for multi-ASIC package feature management — please provide:
- What specific failure does this address? (The PR description is truncated)
- How does this interact with
config featurecommands on multi-ASIC platforms? - Are there sonic-mgmt tests that validate multi-ASIC feature enable/disable?
|
|
@lguohan @securely1g could you please re-review and approve the changes? Thanks |
There was a problem hiding this comment.
Pull request overview
This PR fixes multi-ASIC handling for dynamically installed package features by ensuring FEATURE table updates are applied across all per-ASIC namespace CONFIG_DB instances, and by hardening config feature commands against incomplete per-namespace FEATURE entries.
Changes:
- Extend
SonicDB.get_connectors()to include per-ASIC namespace CONFIG_DB connectors on multi-ASIC systems. - Make
config feature stateandconfig feature autorestarttolerant of missingstate/auto_restartfields across namespaces (avoidsKeyError). - Add targeted unit tests for the new SonicDB namespace connector behavior and the CLI/feature validation behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/sonic_package_manager/test_sonic_db.py | Adds unit tests for per-ASIC namespace connector discovery and connector enumeration behavior. |
| tests/feature_test.py | Adds tests to cover missing state / auto_restart fields and expected CLI behavior. |
| sonic_package_manager/service_creator/sonic_db.py | Adds per-ASIC namespace CONFIG_DB connector support and extends connector iteration to cover namespaces. |
| config/feature.py | Avoids crashing on incomplete FEATURE entries by using .get() and adds guards when fields are missing everywhere. |
| if cls._namespace_db_conns is None: | ||
| cls._namespace_db_conns = [] | ||
| if device_info.is_multi_npu(): | ||
| from swsscommon.swsscommon import SonicDBConfig | ||
| SonicDBConfig.initializeGlobalConfig() | ||
| for ns in device_info.get_namespaces(): | ||
| try: | ||
| conn = swsscommon.ConfigDBConnector(namespace=ns) | ||
| conn.connect() | ||
| cls._namespace_db_conns.append(conn) | ||
| except RuntimeError: | ||
| pass | ||
|
|
||
| return cls._namespace_db_conns |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Address the Copilot review on get_namespace_db_connectors(): - Fail fast on a namespace connect error instead of swallowing RuntimeError and returning a partial connector set; a missing namespace CONFIG_DB is a real fault, not something to silently skip. - Cache only after every namespace connects (local accumulator assigned to _namespace_db_conns at the end), so a failed run leaves the cache unset and the next call retries rather than caching a partial list. - Guard the global DB init with isGlobalInit() and call it through the module-level swsscommon import, matching the pattern used in utilities_common/db.py; drop the in-function lazy import. Update tests to cover fail-fast propagation, no-cache-on-failure retry, both isGlobalInit branches, and single/multi-ASIC caching. Signed-off-by: William Tsai <willtsai@nvidia.com>
d14ff99 to
9edd74b
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
What I did
Fixed two issues with dynamically installed packages on multi-ASIC platforms:
sonic-package-manager installnow writes complete FEATURE entries to all per-ASIC namespace CONFIG_DBs, not just the host CONFIG_DB.config feature stateandconfig feature autorestartno longer crash withKeyErrorwhen per-ASIC FEATURE entries are incomplete.How I did it
sonic_package_manager/service_creator/sonic_db.py: Addedget_namespace_db_connectors()toSonicDBthat returns per-ASIC namespace CONFIG_DB connectors on multi-ASIC platforms.get_connectors()now yields these after the host connector, soFeatureRegistry.register()andderegister()write to all CONFIG_DBs.config/feature.py: Changedentry_data['state']toentry_data.get('state')inset_feature_state(), andentry_data['auto_restart']toentry_data.get('auto_restart')infeature_autorestart(). Namespace entries missing the field are skipped during validation. Added a guard for the case where no namespace has the field at all. Fixed thealways_enabledcheck to use the collected set instead of relying on the last loop variable.How to verify it
On a multi-ASIC system:
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)