fix: normalize identityref values in JSON importer and leafref validation - #444
fix: normalize identityref values in JSON importer and leafref validation#444steiler wants to merge 7 commits into
Conversation
…tion GetKeyValue in the JSON importer now routes through GetTVValue/ToString so identityref keys are emitted as module-qualified strings (e.g. "sdcio-model-identity:ETHERNET") rather than a raw proto-struct dump. resolveLeafrefKeyPath switches from tv.GetStringVal() to tv.ToString() for the same reason — GetStringVal returns an empty string for TypedValue_IdentityRef, causing leafref lookups to silently fail. Navigate in yangParserEntryAdapter now calls StripPathElemPrefixPath before NavigateSdcpbPath so xpath evaluation against identityref-keyed list entries succeeds when path elements carry a module prefix. Adds regression tests: TestLeafref_IdentityrefKey, TestMust_IdentityrefKey (validation), and JSON-importer round-trip test with identityref list key. Extends sdcio_model_identity.yang with matching fixtures. Co-authored-by: Cursor <cursoragent@cursor.com>
Periodic GET sync revert raced with concurrent intent delete transactions: LoadAllButRunningIntents read stale cache (deleted intent still present), then applyIntent pushed deleted config back to device, undoing the deletion. Acquiring dmutex in performRevert forces it to wait for any active transaction to fully complete (device write + IntentDelete) before snapshotting the intent store. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
ErrTransactionOngoing was falling through translateInternalToGrpcError and being wrapped by gRPC as codes.Unknown. config-server treats codes.Unknown as non-recoverable, causing a cascade of permanent failures instead of a backoff-and-retry when a transaction collision occurs. Map it to codes.Aborted alongside ErrDatastoreLocked so config-server correctly retries. Additionally, split the single dmutex acquisition in performRevert into two narrow critical sections: one covering LoadAllButRunningIntents (the cache snapshot that must be atomic with IntentDelete) and one covering applyIntent (the gNMI device write). FinishInsertionPhase, GetDeletes, and ToProtoUpdates operate only on the local tree copy and no longer hold the mutex, reducing contention on concurrent transactions. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
are we sure here about 2x locking? I don't get the local copy t statement - we don't have a guaranteed local copy as t is a pointer passed in
There was a problem hiding this comment.
Good catch — you're right on both counts.
Fixed by merging the two critical sections into one: dmutex is now held for the whole snapshot → diff → apply sequence in performRevert (8f6082d), instead of being released between the cache read and the device write. Releasing it in between left a window where a concurrent transaction could commit new intent/device state after we snapshotted the cache but before we pushed our diff, so the revert could apply a now-stale diff and clobber that transaction's change — same class of race as the one this PR originally fixed, just on the other side of the split.
Also dropped the "t is a caller-owned deep copy, so no lock needed here" comment since it's no longer relevant (the whole function is now under one lock), and tightened the dmutex field doc-comment since it's not just for Set operations.
Added a test (TestPerformRevert_HoldsDmutexAcrossSnapshotAndApply) that blocks a fake device write mid-flight and asserts a concurrent TryLock fails for the entire duration and only succeeds once performRevert returns.
| return y, nil | ||
| } | ||
|
|
||
| p.StripPathElemPrefixPath() |
There was a problem hiding this comment.
could you add a comment as to why we do this?
There was a problem hiding this comment.
Done — added an explanatory comment above the p.StripPathElemPrefixPath() call in 7d83ef5, explaining that xpath-produced path elements can carry a module prefix that NavigateSdcpbPath's exact-name matching can't resolve, consistent with how BreadthSearch handles this in validation_entry_leafref.go.
sync.go: clarify that the "no-lock" section relies on t being a caller-owned deep copy (d.syncTree.DeepCopy in ApplyToRunning), not a guarantee the function itself can make from the pointer parameter. yang-parser-adapter.go: add comment explaining why Navigate strips module prefixes before NavigateSdcpbPath — xpath may produce elements like "sdcio-model:list-name" that exact-name matching cannot resolve, consistent with how BreadthSearch handles paths in validation_entry_leafref.go. Co-authored-by: Cursor <cursoragent@cursor.com>
alexandernorth flagged that splitting performRevert's dmutex hold into two critical sections (snapshot read, then device apply) left a window where a concurrent transaction could commit in between, causing the revert to push a diff computed against a now-stale snapshot and clobber that transaction's changes. Merge the two sections back into one lock spanning snapshot, diff computation, and device apply; the diff computation is in-memory only, so the added hold time is small. Also tighten the dmutex field doc-comment, which described it as only guarding client-initiated Set operations even though performRevert (deviation-revert / self-healing) has always used it too. Co-authored-by: Cursor <cursoragent@cursor.com>
golangci-lint (errcheck) flagged the unchecked Unmarshal in TestPerformRevert_HoldsDmutexAcrossSnapshotAndApply. Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
2 participants
Summary
Together with the companion schema-server PR these four fixes fully resolve the Arista `routing-policy` silent validation failures first reported in #349.
Companion PR
schema-server (compile-time must-statement literal normalisation):
→ sdcio/schema-server#241