Add ospf nsr - #1547
Add ospf nsr#1547ky-teramae wants to merge 3 commits into
Conversation
OpenConfig's ISIS model has a dedicated nsr container (distinct from graceful-restart) to enable Non-Stop Routing, but neither OSPF (v3) nor OSPFv2 has an equivalent, despite NSR being a distinct failover mechanism from GR (no protocol restart/signalling with neighbours). Add an nsr container with a single enabled leaf to both, matching the ISIS admin-config pattern. LDP NSR support is out of scope for this change.
The nsr-specific grouping only ever contained a single "enabled" leaf, but its description was phrased as if it were a re-usable grouping (copied from the ISIS admin-config pattern) despite being used by nothing else. Replace it with an actual re-usable admin-config-style grouping (ospf(v2)-global-admin-config), mirroring ISIS's admin-config exactly, and have the nsr container use that; the nsr-specific context now lives only in the nsr container's own description, not in the grouping. Also move the new grouping to after inter-areapp-config, and the new nsr container to after inter-area-propagation-policies, so the diff appends rather than inserting into the middle of the existing order. No schema change. Also replace the placeholder "Align revisions across modules." text on unmodified sibling submodules with the same revision description used on the files that actually changed, per reviewer request.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds Non-Stop Routing (NSR) configuration and state containers to the global structures of both OSPF and OSPFv2 models, aligning them with the ISIS global configuration. The feedback recommends removing the default false; statement from the enabled leaves in the newly introduced admin-config groupings. This change aligns with OpenConfig design patterns to prevent platform-dependent compatibility issues.
| leaf enabled { | ||
| type boolean; | ||
| default false; | ||
| description | ||
| "When set to true, the functionality within which this leaf | ||
| is defined is enabled; when set to false it is explicitly | ||
| disabled."; | ||
| } |
There was a problem hiding this comment.
In OpenConfig models, it is a standard design pattern to omit default values for boolean enabled leaves. Specifying default false; can cause issues on platforms where the feature is enabled by default or where the default behavior is platform-dependent. Removing the default statement ensures better platform compatibility and aligns with other enabled leaves in the OSPF models (such as graceful-restart and BFD).
leaf enabled {
type boolean;
description
"When set to true, the functionality within which this leaf
is defined is enabled; when set to false it is explicitly
disabled.";
}
There was a problem hiding this comment.
Good catch — applying this. doc/openconfig_style_guide.md says defaults should be avoided unless required by a feature specification, and NSR has no such spec. The existing enabled leaves for graceful-restart in both OSPF modules also carry no default.
The platform default varies — IOS XR documents NSR as enabled by default for OSPF (nsr disable to turn it off), while classic IOS/IOS-XE has it disabled by default. Baking default false into the model would contradict at least one of them.
Note this intentionally diverges from ISIS admin-config, which does specify default false.
| leaf enabled { | ||
| type boolean; | ||
| default false; | ||
| description | ||
| "When set to true, the functionality within which this leaf | ||
| is defined is enabled; when set to false it is explicitly | ||
| disabled."; | ||
| } |
There was a problem hiding this comment.
In OpenConfig models, it is a standard design pattern to omit default values for boolean enabled leaves. Specifying default false; can cause issues on platforms where the feature is enabled by default or where the default behavior is platform-dependent. Removing the default statement ensures better platform compatibility and aligns with other enabled leaves in the OSPF models (such as graceful-restart and BFD).
leaf enabled {
type boolean;
description
"When set to true, the functionality within which this leaf
is defined is enabled; when set to false it is explicitly
disabled.";
}
There was a problem hiding this comment.
Good catch — applying this. doc/openconfig_style_guide.md says defaults should be avoided unless required by a feature specification, and NSR has no such spec. The existing enabled leaves for graceful-restart in both OSPF modules also carry no default.
The platform default varies — IOS XR documents NSR as enabled by default for OSPF (nsr disable to turn it off), while classic IOS/IOS-XE has it disabled by default. Baking default false into the model would contradict at least one of them.
Note this intentionally diverges from ISIS admin-config, which does specify default false.
Boolean enabled leafs in OpenConfig conventionally have no default, to avoid constraining platform behavior.
1 participant
Change Scope
ISIS global config.
Platform Implementations
nsr(OSPFv3: same command underrouter ospfv3)nsrTree View
module: openconfig-ospfv2 +--rw network-instances +--rw network-instance* [name] +--rw protocols +--rw protocol* [identifier name] +--rw ospfv2 +--rw global +--rw inter-area-propagation-policies ... + +--rw nsr + +--rw config + +--rw enabled? booleanmodule: openconfig-ospf (OSPFv3, shares the ospf-global submodule) +--rw network-instances +--rw network-instance* [name] +--rw protocols +--rw protocol* [identifier name] +--rw ospfv3 +--rw global +--rw inter-area-propagation-policies ... + +--rw nsr + +--rw config + +--rw enabled? boolean