[config/syslog]: Resolve per-ASIC-only features to their container on single-ASIC platforms#4479
[config/syslog]: Resolve per-ASIC-only features to their container on single-ASIC platforms#4479nsoma-cisco wants to merge 1 commit into
Conversation
|
|
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
78d9772 to
c3a3450
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@mssonicbld There is a check pending for a long time. Could you please check? |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@mssonicbld There is a required check pending for a long time. Could you trigger it? |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Hi @Junchao-Mellanox @saiarcot895 — gentle ping on this small bug fix to the syslog rate-limit-feature CLI on single-ASIC platforms (PR you reviewed/authored adjacent code in). Would either of you have a moment to take a look? Thank you. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
@mssonicbld The PR has been opened for 6+ weeks and this test never ran. And the reviewers haven't been added. Is there anything I need to do? |
… 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>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@saiarcot895 Thank you for the review and approval. Hoping the PR will be merged soon. |
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
```