[shadow-pr] #15#17
Closed
william8545 wants to merge 10 commits into
Closed
Conversation
Replace Unicode dash with ASCII hyphen-minus in INVALID_CONTAINER_NAME constant and related comment, so the invalid container name matches what docker stats actually returns.
Why I did this
We found some error in syslog:
ERR memory_threshold_check: Failed to parse memory usage for "{'CPU%': '--', 'MEM%': '--', 'MEM_BYTES': '0', 'MEM_LIMIT_BYTES': '0', 'NAME': '--', 'PIDS': '--'}": could not convert string to float: '--'
2026 Feb 6 22:43:04.172360 air-2700-1 ERR memory_threshold_check: Failed to parse memory usage for "{'CPU%': '--', 'MEM%': '--', 'MEM_BYTES': '0', 'MEM_LIMIT_BYTES': '0', 'NAME': '--', 'PIDS': '--'}": could not convert string to float: '--'
2026 Feb 6 22:43:04.172422 air-2700-1 ERR memory_threshold_check: Failure occurred could not convert string to float: '--'
The error is statistical. The flow is as below:
procdockerstatsd calls "docker stats" command periodically, parse the output and store to STATE DB
memory_threshold_check which is called by monit service, handles the data in STATE DB and check the memory usage
For detailed docker code, please check:
https://github.com/docker/cli/blob/93fa57bbcd08f2f5be7f6cf22f4273a2b5a49e71/cli/command/container/formatter_stats.go#L25
https://github.com/docker/cli/blob/93fa57bbcd08f2f5be7f6cf22f4273a2b5a49e71/cli/command/container/formatter_stats.go#L169
How I did this
procdockerstatsd should check the command output and ignore invalid value:
If the name is empty of "--", ignore the output, log a warning message to syslog.
How I test this
Manual test.
…s (#352) * [gnoi-shutdown-daemon] Skip gNOI shutdown for already powered-off DPUs Check DPU operational status before attempting gNOI Reboot HALT. If the DPU is not Online (e.g. Offline, PoweredDown), skip the gNOI shutdown sequence and clear the halt flag directly. This avoids error logs when config reload or reboot is issued while DPUs are already in the down state. Fixes: sonic-net/sonic-buildimage#25889 Signed-off-by: Vasundhara Volam <vvolam@microsoft.com> * Address PR review comments - Use ModuleBase constants (MODULE_STATUS_OFFLINE, MODULE_STATUS_POWERED_DOWN) instead of hardcoded strings for oper_status checks - Only skip gNOI shutdown for Offline/PoweredDown states; other non-Online states like Fault still proceed with the shutdown attempt - Propagate _clear_halt_flag() return value in early-return path - Add unit tests for new skip behavior and clear-halt failure propagation Signed-off-by: Vasundhara Volam <vvolam@microsoft.com> * Address PR review: extract _should_skip_gnoi_shutdown method Refactor per review from @hdwhdw: extract the oper-status check logic from _handle_transition into a dedicated _should_skip_gnoi_shutdown method with tri-state return (True/False/None) to reduce nesting and improve readability. Add unit tests for the new method covering all return paths: - Offline -> True - PoweredDown -> True - Online -> False - Fault -> False - Bad module index -> None - Module is None -> None Signed-off-by: Vasundhara Volam <vvolam@microsoft.com> --------- Signed-off-by: Vasundhara Volam <vvolam@microsoft.com>
…sor (#365) On T2 chassis first boot, featured daemon races with systemd to disable BGP services. If systemctl stop arrives while the service is still in 'activating' state (ExecStartPre), systemd kills the startup script via SIGTERM without running ExecStop, leaving Docker containers orphaned. Add wait_for_service_stable() to disable_feature() that polls systemctl is-active until the service leaves 'activating' state before issuing the stop command. This ensures ExecStop runs and containers are properly cleaned up. Fixes: sonic-net/sonic-buildimage#24875 Signed-off-by: Deepak Singhal <deepsinghal@microsoft.com>
…nic_platform (#359) This test, determine-reboot-cause_test.py and the script it tests, scripts/determine-reboot-cause expect the module sonic_platform to not exist on the system during unit tests on build hosts. The script implementation handles this situation by trying to import sonic_platform in the funciton calls, and having them return fake values in an ImportError exception handler. Otherwise, if sonic_platform is present in the build system when running unit tests, it will throw exceptions because the platform code is designed to run an a real SONiC device host. However, on the build system, there are races in the build, and it is possible for sonic_platform to be installed and loaded. This will break this test. This PR adds a fixture to remove the module and block it using temporary monkeypatching. None is assigned to system.modules['sonic_platform'] to evict any cached loaded modules and block future loading, until the test is complete.
Adding back PROC_CMDLINE definition to prevent the crash seen on UT2 Signed-off-by: Tejaswini Chadaga <tchadaga@microsoft.com>
Signed-off-by: Pattela JAYARAGINI <pattelaj@google.com> Co-authored-by: Neha Das <nehadas@google.com>
This PR propagates a fix for timing issue caused by caclmgrd processing of ACL tables during Config DB record removal operation.
Evidence from code
What caclmgrd does
self._tables_db_info = config_db_connector.get_table(self.ACL_TABLE)
...
if table_data["type"] != self.ACL_TABLE_TYPE_CTRLPLANE:
ConfigDBConnector.get_table() is implemented as
const auto& keys = client.keys(pattern);
for (auto& key: keys)
{
auto const& entry = client.hgetall<map<string, string>>(key);
This is a two-step read:
list keys (KEYS ACL_TABLE|*)
per key HGETALL
If a key is deleted/changed between step 1 and step 2, hgetall can return {}.
Then table_data exists as an empty map in _tables_db_info, and table_data["type"] throws KeyError.
So missing type does NOT require field-only deletion; plain key deletion racing with get_table() is enough.
Also field-only deletion is possible in the stack:
client.hmset(_hash, data.begin(), data.end());
...
if (!found)
{
client.hdel(_hash, k);
}
set_entry() can remove extra fields with HDEL while keeping the record key alive.
Evidence from pytest
The log shows rapid table deletes:
sudo config acl remove table DATA_INGRESS_L3TEST
...
sudo config acl remove table DATA_EGRESS_L3V6TEST
That high churn exactly matches a race window for the non-atomic get_table() read in caclmgrd.
So missing type can happen via:
field-level HDEL updates
key-level delete race during KEYS + HGETALL snapshot collection
Signed-off-by: Nazarii Hnydyn <nazariig@nvidia.com>
* [featured]: Skip redundant CONFIG_DB writes in sync_feature_scope * Add read-before-write check in sync_feature_scope to only write scope fields when values actually differ from CONFIG_DB * Add unit tests covering unchanged, changed, and missing entry cases Signed-off-by: Bojun Feng <bojundf@gmail.com> * [featured]: Fix sync_feature_scope test placement and coverage * Move tests from TestFeatureDaemon to TestFeatureHandler to avoid class-level mock.patch argument injection issues * Improve assertions and add partial-change test case Signed-off-by: Bojun Feng <bojundf@gmail.com> * [featured]: Consolidate sync_feature_scope tests * Merge skip/write/missing-entry tests into one multi-scenario method following the test_feature_resync pattern * Remove redundant partial-change and empty-dict sub-cases Signed-off-by: Bojun Feng <bojundf@gmail.com> * [featured]: Address comments Signed-off-by: Bojun-Feng <bojundf@gmail.com> * [featured]: Extract duplicated scope update logic into helper method Signed-off-by: Bojun-Feng <bojundf@gmail.com> --------- Signed-off-by: Bojun Feng <bojundf@gmail.com> Signed-off-by: Bojun-Feng <bojundf@gmail.com>
…nitDone On multi-ASIC, PortInitDone is published in each ASIC namespace's APPL_DB, not the host APPL_DB. featured only watched the host APPL_DB, so it never observed PortInitDone and released delayed features (lldp, gnmi, mgmt-framework, snmp, sflow, pmon) only after the fallback timeout. * Subscribe every port-bearing namespace's PORT_TABLE and release delayed features once all of them report PortInitDone (quorum) * Detect port-bearing namespaces via PORT table presence; reconcile the quorum against the connectors actually established so a failed connector degrades to the timeout backstop instead of blocking release forever * Key subscriptions and callbacks by (namespace, table) to support per-namespace PORT_TABLE listeners; single-ASIC behavior is unchanged * Use get_keys instead of get_table for the port existence check * Add unit tests for the quorum, re-release after swss restart, out-of-quorum namespaces, non-SET/other keys, and the reconcile path Signed-off-by: William Tsai <willtsai@nvidia.com>
Cover the multi-ASIC vs single-ASIC branch of register_callbacks, which was previously exercised only through hand-built handlers: * Multi-ASIC: each port-bearing namespace's PORT_TABLE is subscribed on its own connector and the host APPL_DB is NOT subscribed for ports * Single-ASIC: the host APPL_DB PORT_TABLE is subscribed The two tests share a _make_bare_daemon helper, mirroring the existing _make_multi_asic_handler, so each test shows only what differs. Signed-off-by: William Tsai <willtsai@nvidia.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: nvidia-sonic/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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
9 participants
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.
Automated shadow of #15 onto
multi-asicfor internal CI (poke-commands §6.2).Source PR target branch:
master_RC. Do not merge — this branch exists only to run internal CI onmulti-asic.