feat: SONiC device-profile — encoder, materialize dispatch, and gRPC layer - #480
Open
steiler wants to merge 31 commits into
Open
feat: SONiC device-profile — encoder, materialize dispatch, and gRPC layer#480steiler wants to merge 31 commits into
steiler wants to merge 31 commits into
Conversation
3 tasks
Collaborator
Author
|
Config-server-side (KRM layer) counterpart: sdcio/config-server#484 — adds |
Collaborator
Author
|
Retrigger CI: workflows never ran because the PR was opened against main (merge conflict) and the later base change to ciscoiosxrd2 does not retrigger pull_request workflows. |
Adds DeviceProfileSonic and IsSonic(), mirroring the existing Cisco IOS-XR device-profile plumbing, and rejects device-profile=sonic SBIs at config-load time unless GnmiOptions.Encoding is JSON_IETF. Ticket: .scratch/sonic-device-profile/issues/01-device-profile-config-field-and-validation.md
sdc-protos deviceprofile branch now carries DEVICE_PROFILE_SONIC = 2.
Link sdc-protos commit 40ed0bc / PR#120 to the go.mod bump ticket.
40ed0bc is now on origin/deviceprofile.
Introduce a schema-driven gNMI Set encoder that groups changes by immediate parent (plain container batching vs keyed-list full-row wraps), forces sonic_yang origin, and strips RFC 7951 prefixes recursively so translib accepts the payload.
serializeListRowUpdate walked up only one tree level (GetParent()) to find the list container's name, which is correct for single-key lists but wrong for multi-key lists: a compound key inserts one schema-less key-level entry per key, so GetParent() landed on an intermediate key-level entry instead of the schema-bearing list container, producing a wrap object keyed by a key value instead of the list name. Walk up to the nearest schema-bearing ancestor instead, which is correct for any key count. Also drop an unreachable branch in groupingTarget: a leaf's immediate parent can never itself be a schema-bearing keyed-list container, since the tree model always inserts schema-less key-level entries first. Adds a regression test against the doublekey (2-key) fixture list.
Unlike permodule (which derives Path.Origin dynamically per-module from the schema, making it genuinely reusable across NOS targets), this package unconditionally forces Path.Origin to the fixed literal "sonic_yang" on every Update. That hardcoded, non-schema-derived value ties the package to the sonic device-profile specifically, so naming it for its encoding shape overstated its reusability. Name it for the device profile instead, matching the DeviceProfileSonic/IsSonic() naming already used at the dispatch layer. No behavior change; also fixes a pre-existing gofmt import-ordering issue.
Commit 0825534 (git mv parentbound/ -> sonic/) never updated the `package parentbound` / `parentbound_test` declarations or the `parentbound: ...` error-wrap prefixes, so HEAD failed to compile under `go vet`/`go test` (only `go build ./...` stayed silent, since it skips _test.go). This finishes the rename the commit message already claimed.
When type=gnmi and the SBI carries device-profile=sonic, BuildPlan now routes to sonic.Encode instead of falling through to the generic single-root gNMI plan. The branch mirrors the existing Cisco IOS-XR pattern and contains no profile-specific logic of its own; all SONiC encoding decisions remain inside the sonic package. Adds routing tests covering the sonic path and verifying non-sonic profiles are unaffected. Closes ticket 04.
…r-wrap Add SBI.IsGnmi() predicate alongside IsSonic()/IsCiscoIOSXR() so the dispatch guards in BuildPlan don't repeat the "gnmi" string literal. Extract a gnmiPlan helper that converts (*GnmiSetPlan, error) into (SouthboundSetPlan, error), eliminating the identical three-line error-wrap block that appeared in both profile branches.
Export sbiGNMI/sbiNETCONF/sbiNOOP as SBITypeGnmi/SBITypeNetconf/SBITypeNoop so callers can compare against them directly in guards and switch cases. Remove the IsGnmi() method, which was a Middle Man wrapping a single constant comparison. materialize.BuildPlan now uses the exported constants in both the profile dispatch guards and the type switch.
DeviceProfile is already an exported typed constant — callers can compare directly. The predicate methods were thin wrappers with no added value. materialize.BuildPlan now compares sbi.DeviceProfile against the exported constants directly. The predicate-only tests are removed; the behaviour they nominally covered is exercised by the validation and materialize routing tests.
…ate removal refactor
Bumps sdc-protos to v0.0.55-0.20260828064538-40ed0bc26a71 (deviceprofile branch, DEVICE_PROFILE_SONIC = 2). Adds the sonic case to both sdcpbDeviceProfileToConfig and configDeviceProfileToSdcpb so the enum round-trips correctly through the gRPC layer, matching the existing Cisco IOS-XR mapping. Covered by new table-driven tests. Closes ticket 05 (sonic-device-profile).
Mirrors the equivalent note added on the config-server side, after a session almost implemented config-server ticket 07 on the wrong branch (config-server-cache-backend) because nothing recorded which worktree this effort actually uses.
connectSBI() sets Datastore.sbi asynchronously after New() returns, while ConnectionState(), Stop(), and applyIntent() read it concurrently from RPC handlers, causing a data race detected by -race.
gnmiTarget.Get inlined an `if cfg.DeviceProfile == DeviceProfileSonic` check to force DataType_ALL, coupling the generic gNMI driver to one profile's quirks. Move the fact into gnmi/sonic.ShapeGetRequest and select it once in NewTarget via getRequestShaperFor, mirroring how target.New and materialize.BuildPlan dispatch by type/profile. Get() no longer knows DeviceProfile exists, and the behaviour is now unit-testable without a live/mocked gNMI connection. Add an ADR-0002 addendum recording NewTarget as the sanctioned per-profile dispatch point for the Get path.
GetContainer().GetKeys() returns the schema's own slice, which is shared and read concurrently by other import tasks. Sorting it in place raced with those readers/sorters running in other worker-pool goroutines. Sort a cloned copy instead.
must-statements declared directly on a YANG list were evaluated both at the list's key-level node (whose children are keyed instances, not the instance's own siblings) and at each resolved instance. At the key-level node, xpath expressions referencing sibling leaves degrade to an empty node-set and spuriously fail, producing errors with no key predicate in the path. Mirror validateMandatory's key-level descent (now extracted into a shared descendKeyLevels helper) so must-statements on a list are only ever evaluated once, against a resolved instance.
Bumps github.com/sdcio/yang-parser to ee8fd4a, which fixes two related bugs behind count(leaflist[predicate]) must-statements (e.g. SONiC's "count(adv_speeds[text()='all']) = 0 or count(adv_speeds) = 1"): DatumSlice comparisons collapsing to Bool instead of composing with count(), and a path-stack leak between a predicated and a bare reference to the same leaf-list in one expression. That fix only takes effect when a leaf-list Entry's GetValue() reports as a DatumSlice. yangParserEntryAdapter.GetValue() returned an empty NodesetDatum instead for a leaf-list with no value at all, which is the common case (e.g. adv_speeds/adv_interface_types are normally unset on every SONiC port) -- causing the same crash to persist for that specific shape. Report an empty DatumSliceDatum for that case instead, matching the populated-leaf-list branch. Adds a count-leaflist leaf-list + must-statement to the test schema (ygot bindings regenerated) and a regression test covering unset, single-value, and multi-value leaf-list states.
…self-reference, improve must-statement error context
- targettypes.NewGnmiPlan: normalize a nil *GnmiSetPlan into an empty
(non-nil) one when wrapping into SouthboundSetPlan. Encoders (e.g. the
sonic encoder) may return nil to signal a no-op; without normalization
that nil flattens into a SouthboundSetPlan with neither variant
populated, indistinguishable from a plan built for the wrong driver
and rejected as such instead of treated as a no-op. materialize.go's
gnmiPlan() now goes through this helper.
- utils.Converter.ExpandContainerValue: unwrap RFC 7951 top-level
self-reference wrapping (e.g. SONiC translib GET returning
{"sonic-srv6:sonic-srv6": {...}} instead of bare content) so it's
treated as the container's own value rather than an unknown child.
- validation_entry_must.go: wrap raw xpath execution errors with the
entry's path and must-statement text, matching the context already
attached to the plain !result case. Surfaced the count()/leaf-list
bug fixed by the yang-parser bump in an earlier commit.
…ebug logging in transaction_rpc - NewOnceSync/NewStreamSync now take a targetName parameter and set gapi.Target(targetName) on their SubscribeRequest, matching gnmi.go's existing t.cfg.GnmiOptions.TargetName usage for Get/Set requests (already committed). gnmi.go's call sites already passed this argument; without this commit, HEAD fails to build: 'too many arguments in call to NewOnceSync/NewStreamSync'. - transaction_rpc.go: add two V(VDebug)-gated log lines (expanded intent updates; intent updates inserted into tree) added while investigating the sonic-spine1 SRv6 locator/must-statement bugs (see .scratch/sonic-spine1-srv6-locator-bug/handoff.md, now fully resolved end-to-end). Kept as low-risk diagnostic instrumentation.
…scoiosxrd2 rebase TestSBI_validateSetDefaults_DeviceProfile_UnaffectedProfilesAndEncodingsUnchanged asserted that cisco-ios-xr + gnmi + json was accepted, written before ciscoiosxrd2's JSON_IETF-only restriction for cisco-ios-xr landed. That case is now correctly rejected and already covered by TestSBI_validateSetDefaults_DeviceProfile_CiscoIOSXRGNMIPlainJSONIsRejected; this only removes the now-contradictory assertion.
steiler
force-pushed
the
sonic-device-profile
branch
from
September 8, 2026 07:28
005d82e to
facd005
Compare
…ual Datastore literal
commit 8984fef added Datastore.sbiMutex (*sync.RWMutex) to guard sbi,
but this test builds a Datastore{} literal directly and set sbi without
ever setting sbiMutex, leaving it nil. applyIntent()'s d.getSBI() call
dereferences the nil *sync.RWMutex, panicking. No other test constructs
Datastore with sbi set, so this went unnoticed until now.
… as real schema members The self-wrap unwrap added in 95b8408 ran unconditionally for any top-level container whose single JSON key matched its own qualified name, with no device-profile check (converter.go is generic infrastructure shared by every vendor's GET/Subscribe response parsing, not just SONiC's). That's a plausible false-positive for any non-SONiC target whose single populated key legitimately happens to share the container's own qualified name as a real schema-defined child, silently misinterpreting a legitimate field as a self-wrap and dropping a path segment instead of processing it normally. Guard the unwrap with getItem: a container never lists itself as one of its own schema children, so a true self-reference is guaranteed to fail that lookup, while any real, resolvable child (the common case) is left untouched and processed via the normal field/child path below. This keeps the fix scoped to the exact translib quirk it targets without adding a device-profile/vendor check to this generic function. Added TestExpandUpdateContainerDoesNotUnwrapLegitimateChildSharingTheContainersName covering the false-positive case. This is a defensive scoping fix for a suspected (not yet CI-confirmed) cause of the 02-Crud.12-Srl-Create-Delete integration test regression seen after rebasing onto ciscoiosxrd2 (SRL's ConfigSet never reaching Ready, while 11-Sros-Create-Delete passes unaffected).
…igateSdcpbPath commit 4275945 moved must-statement evaluation from the list's key-level node to the resolved instance (key-value) node so that must-statements referencing the instance's own sibling fields evaluate correctly. However, must-statements that navigate `../field` (to the YANG parent) broke: from a key-value node (schema==nil), `..` landed on the key-level (the list's schema node), and `type` was then looked up among the key-level's children — which are other key-value instances, not the parent container's fields. The resulting empty node-set made XPath `emptyNodeset != 'module:identity'` evaluate to FALSE per RFC 7950 §7.5.3, causing every affected must-statement to spuriously fail. The SDCIO tree contains an extra level of indirection for list instances: parent-container ← YANG parent of the list list KEY-LEVEL ← carries the list schema; children are key-value nodes key-value-1 ← schema==nil; children are the instance's data fields From a key-value node, one `..` step reaches the key-level, not the YANG parent that YANG/XPath authors expect. Fix NavigateSdcpbPath to detect this case (current node has no schema) and skip to the YANG parent directly by calling GetFirstAncestorWithSchema to land on the key-level, then one more GetParent() to reach the actual YANG-level containing node. The consecutive `../.. ` path (used internally) is unchanged: it still calls GetFirstAncestorWithSchema on the starting node, and each `..` leg in the recursive descent then benefits from the same key-value skip.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
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
1 participant
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.
Depends on
Summary
Implements the `sonic` device-profile end-to-end for gNMI targets, from
config validation through southbound encoding to the gRPC API layer.
Builds directly on the IOS-XR encoder infrastructure introduced in #442.
`JSON_IETF`-only validation rule — the SONiC translib only accepts
RFC 7951 encoding.
`Encode(ctx, scb, entry, replace)` function that produces a
`GnmiSetPlan` with parent-bound, `origin: sonic_yang` Updates; strips
RFC 7951 module prefixes recursively; wraps keyed-list rows in their
full current state. Fully unit-tested.
`BuildPlan` routes `sbi.DeviceProfile == "sonic"` to the new encoder,
mirroring the existing Cisco IOS-XR branch. No NOS-specific branching
outside this function.
`v0.0.55-0.20260828064538-40ed0bc26a71` (Add device_profile sdc-protos#120);
adds sonic cases to `sdcpbDeviceProfileToConfig` and
`configDeviceProfileToSdcpb` so the enum round-trips correctly through
the API.
Test plan