[config/syslog]: Resolve per-ASIC-only features to their container on single-ASIC platforms#4479
Open
nsoma-cisco wants to merge 1 commit intosonic-net:masterfrom
Open
[config/syslog]: Resolve per-ASIC-only features to their container on single-ASIC platforms#4479nsoma-cisco wants to merge 1 commit intosonic-net:masterfrom
nsoma-cisco wants to merge 1 commit intosonic-net:masterfrom
Conversation
… single-ASIC platforms
config syslog rate-limit-feature enable/disable <svc> was a silent
no-op on single-ASIC platforms when <svc> had has_global_scope=False
and has_per_asic_scope=True (e.g. syncd, gbsyncd, and swss on some
platforms). get_feature_names_to_proceed only enumerated per-ASIC
containers inside an `if multi_asic.is_multi_asic():` guard, so the
single-ASIC case ended up with an empty feature list and the outer
for-loop printed nothing and did nothing.
On a single-ASIC platform a feature has exactly one container whose
name equals the service name, regardless of which scope flag is set;
treat both scopes symmetrically there. Harden the flag lookups with
`.get('flag', '')` to avoid a KeyError when a flag is absent. Also
surface an explicit "nothing to enable/disable" message when the
resolved list is empty so future similar misconfigurations cannot
silently no-op.
Tests:
- tests/syslog_test.py::test_enable_rate_limit_feature_single_asic_per_asic_scope
asserts docker cp .../containercfgd.conf and supervisorctl start
containercfgd are invoked for a single-ASIC syncd-like feature
(has_global_scope=False, has_per_asic_scope=True), plus the symmetric
stop + rm -f on disable.
- tests/syslog_test.py::test_enable_rate_limit_feature_empty_feature_list
asserts the new "nothing to enable" message and that no shell
commands run when the resolved list is empty.
- Existing test_enable_syslog_rate_limit_feature,
test_disable_syslog_rate_limit_feature, and
tests/syslog_multi_asic_test.py paths are unaffected.
Signed-off-by: Nageswara Soma <nsoma@cisco.com>
|
|
Collaborator
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This file contains hidden or 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
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.
What I did
Fix
config syslog rate-limit-feature enable/disable <svc>being a silent no-op on single-ASIC platforms for features whoseFEATUREentry hashas_global_scope=Falseandhas_per_asic_scope=True(e.g.syncd,gbsyncd, andswsson some platforms).How I did it
config/syslog.py::get_feature_names_to_proceedonly enumerated per-ASIC container names inside anif multi_asic.is_multi_asic():guard. On a single-ASIC DUT that branch was skipped, the feature list came back empty, and the outerforloop inenable_rate_limit_feature/disable_rate_limit_featureprinted nothing and did nothing..get('flag', '')to avoid aKeyErrorwhen a flag is absent.enable/disablenow emit an explicit "No container resolved … nothing to enable/disable" message when the resolved list is empty, so a future similar misconfiguration cannot silently no-op again.Behavior matrix after the fix
global=True, per_asic=False[svc][svc]global=False, per_asic=True[svc](was[]— the bug)[svc0, …, svc{N-1}]global=True, per_asic=True[svc](was[]when looked up by name)[svc, svc0, …, svc{N-1}]global=False, per_asic=False[]+ "nothing to enable" message[]+ "nothing to enable" messageHow to verify it
Unit tests:
```bash
python3 -m pytest tests/syslog_test.py tests/syslog_multi_asic_test.py -x -q
```
On a single-ASIC DUT:
On a multi-ASIC DUT: existing behavior is unchanged (`syncd0`, `syncd1`, … resolve as before; `bgp` global-only still resolves to `bgp`; `teamd` with both flags still resolves to `teamd` + per-ASIC names).
Previous command output (if the output of a command-line utility has changed)
On a single-ASIC DUT, before the fix:
```
admin@sonic:
$ sudo config syslog rate-limit-feature enable syncd$admin@sonic:
```
(Silent no-op — nothing printed, container untouched.)
New command output (if the output of a command-line utility has changed)
```
admin@sonic:~$ sudo config syslog rate-limit-feature enable syncd
Enabling syslog rate limit feature for syncd
Enabled syslog rate limit feature for syncd
```
And for a feature with neither scope flag set:
```
admin@sonic:~$ sudo config syslog rate-limit-feature enable ghost
No container resolved for service 'ghost' in namespace None, nothing to enable
```