COUNTERS_DB bare-table enumeration and BUFFER_POOL_WATERMARKS virtual path support - #733
Verma-Anukul wants to merge 12 commits into
Conversation
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Hi, there are workflow run(s) waiting for approval, you may be first-time contributor. I will notify maintainers to help approve once PR is approved. Thanks! ---Powered by SONiC BuildBot
|
9cf05d6 to
3f0bd87
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR improves gNMI behavior for COUNTERS_DB tables that use keyed Redis entries (notably BUFFER_POOL_WATERMARKS) by translating bare/wildcard paths into concrete COUNTERS:<oid> rows, fixes COUNTERS_DB keyspace subscription matching/extraction for ONCE/Subscribe workflows, and cleans up JSON output by stripping SONiC’s "NULL":"NULL" placeholder fields.
Changes:
- Add virtual-path translation for
/COUNTERS_DB/BUFFER_POOL_WATERMARKS(bare and wildcard) viaCOUNTERS_BUFFER_POOL_NAME_MAP. - Make bare-table reads enumerate both keyed (
<table><delim>*) and flat-hash (<table>) Redis key layouts; adjust subscribe key extraction when delimiter is omitted in PSUBSCRIBE patterns. - Strip
"NULL"placeholder fields from Redis-hash JSON output and add a unit test for the behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| sonic_data_client/virtual_db.go | Adds BUFFER_POOL_WATERMARKS virtual-path translation and initializes pool-name→OID mappings per namespace; adjusts COUNTERS_DB subscribe pattern/key handling. |
| sonic_data_client/mixed_db_client.go | Uses shared bare-table key enumeration and fixes subscription key extraction when delimiter is omitted for COUNTERS_DB non-COUNTERS tables. |
| sonic_data_client/db_client.go | Introduces shared listBareTableKeys helper, updates bare-table reads to use it, and strips "NULL" fields in JSON rendering; aligns subscribe key extraction with delimiter-omission logic. |
| sonic_data_client/client_test.go | Adds unit test coverage for stripping "NULL":"NULL" placeholder fields from JSON output. |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Add virtual path translation for BUFFER_POOL_WATERMARKS, listBareTableKeys for bare table Get/Subscribe, NULL placeholder stripping in makeJSON_redis, and delimSkipped PSUBSCRIBE pattern fix for keyed COUNTERS_DB tables. Signed-off-by: Verma-Anukul <anukulverma2013@gmail.com>
Avoid concurrent map writes during lazy init by assembling nested COUNTERS_PG_NAME_MAP and COUNTERS_BUFFER_POOL_NAME_MAP lookups in local maps and assigning once when complete. Signed-off-by: Verma-Anukul <anukulverma2013@gmail.com>
- Pass context.Background() to redis Keys/Exists/Ping/HGetAll calls - Serialize initCountersBufferPoolNameMap with clearMappingsMu - Close TestMakeJSONRedisSkipsNull with missing brace Signed-off-by: Verma-Anukul <anukulverma2013@gmail.com>
c514b09 to
ceaaf6c
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Use countersDbHasTableKeys instead of tableName != COUNTERS when deciding whether to omit the delimiter in COUNTERS_DB keyspace subscriptions. PORT_PHY_ATTR has per-object keys and must keep the delimiter; bare tables like BUFFER_POOL_WATERMARKS still omit it. Signed-off-by: Verma-Anukul <anukulverma2013@gmail.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (4)
sonic_data_client/db_client.go:952
- The code comment/test describe skipping the SONiC placeholder pair {"NULL":"NULL"}, but this loop currently skips any field named "NULL" regardless of its value. Tighten the check to
f == "NULL" && v == "NULL"to avoid dropping a legitimate "NULL" field if it ever appears with a real value.
for f, v := range mfv {
if f == "NULL" {
continue
}
sonic_data_client/db_client.go:1424
- Delete/hdel events do not update
msi, so a laterhsetthat recreates the same content can be incorrectly suppressed byreflect.DeepEqual(newMsi, msi)(becausemsistill holds the pre-delete state). Persist the delete state intomsiso re-adds after deletes are always forwarded.
key := keyFromChannel(subscr.Channel)
newMsi[key] = fp
sonic_data_client/mixed_db_client.go:2143
- Delete/hdel events do not update
msi, so a laterhsetthat recreates the same content can be incorrectly suppressed byreflect.DeepEqual(newMsi, msi)(becausemsistill holds the pre-delete state). Persist the delete state intomsiso re-adds after deletes are always forwarded.
key := keyFromChannel(subscr.Channel)
newMsi[key] = fp
newMsi["delete"] = "null_value"
sonic_data_client/db_client.go:942
- The code comment/test describe skipping the SONiC placeholder pair {"NULL":"NULL"}, but this branch currently skips any field named "NULL" regardless of its value. Tighten the check to
f == "NULL" && v == "NULL"to avoid dropping a legitimate "NULL" field if it ever appears with a real value.
This issue also appears on line 949 of the same file.
for f, v := range mfv {
if f == "NULL" {
continue
}
Cover listBareTableKeys, delimSkipped keyspace handling for PORT_PHY_ATTR vs bare COUNTERS_DB tables, and v2rBufferPoolWatermarks translation to raise PR sonic-net#733 diff coverage. Signed-off-by: Verma-Anukul <anukulverma2013@gmail.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
miniredis v2.35.0 has no SetNotifyKeyspaceEvents helper, which caused sonic_data_client integration tests to fail at compile time. Publish __keyspace@<db>__ notifications manually in subscribe unit tests instead. Signed-off-by: Verma-Anukul <anukulverma2013@gmail.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
hdwhdw
left a comment
There was a problem hiding this comment.
Two inline findings on API coverage and mapping lifecycle.
hdwhdw
left a comment
There was a problem hiding this comment.
Inline finding on buffer-pool mapping lifecycle.
Apply lookupV2R in MixedDbClient.getDbtablePath for COUNTERS_DB virtual paths so native sonic-db Get returns pool-name-keyed BUFFER_POOL_WATERMARKS output. Honor jsonTableKey in MixedDbClient.tableData2Msi and reload COUNTERS_BUFFER_POOL_NAME_MAP on each resolution instead of caching for the process lifetime. Signed-off-by: Verma-Anukul <anukulverma2013@gmail.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
miniredis Del() accepts only a key name; use HDel() to remove a hash field in TestInitCountersBufferPoolNameMap_RefreshesOnChange so sonic_data_client integration tests build in CI. Signed-off-by: Verma-Anukul <anukulverma2013@gmail.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Use COUNTERS_DB/localhost gNMI prefix so getDbtablePath resolves BUFFER_POOL_WATERMARKS through the V2R mapping instead of rejecting single-element paths. Signed-off-by: Verma-Anukul <anukulverma2013@gmail.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Cover listBareTableKeys error paths, bare-table tableData2Msi failures, jsonField/jsonTableKey handling, subscribe delete events, and buffer pool map init error paths targeted by the diff coverage report. Signed-off-by: Verma-Anukul <anukulverma2013@gmail.com>
Signed-off-by: Verma-Anukul <anukulverma2013@gmail.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Summary
Fix bare-path Get/Subscribe for keyed COUNTERS_DB tables (notably
BUFFER_POOL_WATERMARKS) and strip SONiC NULL placeholders from JSON.Before
gNMI Get on bare path
/COUNTERS_DB/BUFFER_POOL_WATERMARKSreturned empty or incomplete results (pools required an explicit wildcard).gNMI JSON output for hashes carrying only a SONiC placeholder:
{"NULL": "NULL"}Subscribe / ONCE mode (
gnmic --mode once) on keyed COUNTERS_DB tables could hang or miss updates because PSUBSCRIBE patterns did not match keyed rows likeBUFFER_POOL_WATERMARKS:<oid>.gnmi.log (when key enumeration failed):
After
gNMI Get on
/COUNTERS_DB/BUFFER_POOL_WATERMARKSreturns all buffer pools by name (viaCOUNTERS_BUFFER_POOL_NAME_MAP→COUNTERS:<oid>).gNMI JSON output — NULL placeholder stripped:
{}or with real fields:
{"ingress_lossless_pool": {"SAI_BUFFER_POOL_STAT_WATERMARK_BYTES": "1234"}}Subscribe uses corrected keyspace patterns with
delimSkippedfor COUNTERS_DB non-COUNTERStables; ONCE-mode subscriptions exit cleanly.Test plan
go test ./sonic_data_client/... -run TestMakeJSONRedisSkipsNull/COUNTERS_DB/BUFFER_POOL_WATERMARKS→ all poolsgnmic --mode oncesubscribe on buffer pool watermarks → exits cleanly