Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OSPFv3 model #1257

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

OSPFv3 model #1257

wants to merge 3 commits into from

Conversation

cpeng90
Copy link

@cpeng90 cpeng90 commented Feb 13, 2025

Change Scope

This change defines a OSPFv3 Yang provisioning and operational model. The LS database definition and MPLS add-ons are out of the scope of this change.
The change is backward compatible.

Platform Implementations

[Note: Please provide at least two references to implementations which are relevant to the model changes proposed. Each implementation should be from separate organizations.].

[Note: If the feature being proposed is new - and something that is being
proposed as an enhancement to device functionality, it is sufficient to have
reviewers from the producers of two different implementations].

@cpeng90 cpeng90 requested a review from a team as a code owner February 13, 2025 19:07
@dplore
Copy link
Member

dplore commented Feb 19, 2025

/gcbrun

@OpenConfigBot
Copy link

OpenConfigBot commented Feb 19, 2025

No major YANG version changes in commit c254592

@dplore
Copy link
Member

dplore commented Feb 19, 2025

@cpeng90 please check the ygot and linter errors. For example, all config leafs must also be present as state leaves

To avoid duplicating leaves, can the openconfig-ospf-area-interface.yang submodule replace the existing ospfv2 area interface submodule? (openconfig-ospfv2-area-interface.yang). Then this new ospf-area-interface sub module can serve as a common submodule for both v2 and v3? (And then the additional v3 leaves would continue to appear in the v3 module you are adding here).

I haven't looked super closely yet, but if this would still result in the same ospfv2 tree to be rendered, without any breaking changes, then this is a good idea I think. If there would be breaking changes to the ospv2 tree, please point out where those would be. We certainly do not want to make any breaking changes to ospfv2 (or really any changes at all).

@dplore
Copy link
Member

dplore commented Feb 19, 2025

Adding tree view:

+        |     +--rw ospfv3
+        |     |  +--rw global
+        |     |  |  +--rw config
+        |     |  |  |  +--rw router-id?                    yang:dotted-quad
+        |     |  |  |  +--rw log-adjacency-changes?        boolean
+        |     |  |  |  +--rw hide-transit-only-networks?   boolean
+        |     |  |  |  +--rw abr-capability?               identityref
+        |     |  |  +--ro state
+        |     |  |  |  +--ro router-id?                    yang:dotted-quad
+        |     |  |  |  +--ro log-adjacency-changes?        boolean
+        |     |  |  |  +--ro hide-transit-only-networks?   boolean
+        |     |  |  |  +--ro abr-capability?               identityref
+        |     |  |  +--rw timers
+        |     |  |  |  +--rw spf
+        |     |  |  |  |  +--rw config
+        |     |  |  |  |  |  +--rw initial-delay?   uint32
+        |     |  |  |  |  |  +--rw maximum-delay?   uint32
+        |     |  |  |  |  +--ro state
+        |     |  |  |  |     +--ro initial-delay?   uint32
+        |     |  |  |  |     +--ro maximum-delay?   uint32
+        |     |  |  |  |     +--ro timer-type?      enumeration
+        |     |  |  |  +--rw max-metric
+        |     |  |  |  |  +--rw config
+        |     |  |  |  |  |  +--rw set?       boolean
+        |     |  |  |  |  |  +--rw timeout?   uint64
+        |     |  |  |  |  |  +--rw include*   identityref
+        |     |  |  |  |  |  +--rw trigger*   identityref
+        |     |  |  |  |  +--ro state
+        |     |  |  |  |     +--ro set?       boolean
+        |     |  |  |  |     +--ro timeout?   uint64
+        |     |  |  |  |     +--ro include*   identityref
+        |     |  |  |  |     +--ro trigger*   identityref
+        |     |  |  |  +--rw lsa-generation
+        |     |  |  |     +--rw config
+        |     |  |  |     |  +--rw initial-delay?   uint32
+        |     |  |  |     |  +--rw maximum-delay?   uint32
+        |     |  |  |     +--ro state
+        |     |  |  |        +--ro initial-delay?   uint32
+        |     |  |  |        +--ro maximum-delay?   uint32
+        |     |  |  |        +--ro timer-type?      enumeration
+        |     |  |  +--rw graceful-restart
+        |     |  |  |  +--rw config
+        |     |  |  |  |  +--rw enabled?       boolean
+        |     |  |  |  |  +--rw helper-only?   boolean
+        |     |  |  |  +--ro state
+        |     |  |  |     +--ro enabled?       boolean
+        |     |  |  |     +--ro helper-only?   boolean
+        |     |  |  +--rw inter-area-propagation-policies
+        |     |  |     +--rw inter-area-propagation-policy* [src-area dst-area]
+        |     |  |        +--rw src-area    -> ../config/src-area
+        |     |  |        +--rw dst-area    -> ../config/dst-area
+        |     |  |        +--rw config
+        |     |  |        |  +--rw src-area?                -> ../../../../../areas/area/identifier
+        |     |  |        |  +--rw dst-area?                -> ../../../../../areas/area/identifier
+        |     |  |        |  +--rw import-policy*           -> /oc-rpol:routing-policy/policy-definitions/policy-definition/name
+        |     |  |        |  +--rw default-import-policy?   default-policy-type
+        |     |  |        +--ro state
+        |     |  |           +--ro src-area?                -> ../../../../../areas/area/identifier
+        |     |  |           +--ro dst-area?                -> ../../../../../areas/area/identifier
+        |     |  |           +--ro import-policy*           -> /oc-rpol:routing-policy/policy-definitions/policy-definition/name
+        |     |  |           +--ro default-import-policy?   default-policy-type
+        |     |  +--rw areas
+        |     |     +--rw area* [identifier]
+        |     |        +--rw identifier       -> ../config/identifier
+        |     |        +--rw config
+        |     |        |  +--rw identifier?          oc-ospf-types:ospf-area-identifier
+        |     |        |  +--rw type?                identityref
+        |     |        |  +--rw stub-default-cost?   uint32
+        |     |        |  +--rw import-summaries?    boolean
+        |     |        |  +--rw address-ranges* [prefix]
+        |     |        |     +--rw prefix    -> ../config/prefix
+        |     |        |     +--rw config
+        |     |        |     |  +--rw prefix?   inet:ip-prefix
+        |     |        |     |  +--rw status?   enumeration
+        |     |        |     +--ro state
+        |     |        |        +--ro prefix?   inet:ip-prefix
+        |     |        |        +--ro status?   enumeration
+        |     |        +--ro state
+        |     |        |  +--ro identifier?          oc-ospf-types:ospf-area-identifier
+        |     |        |  +--ro type?                identityref
+        |     |        |  +--ro stub-default-cost?   uint32
+        |     |        |  +--ro import-summaries?    boolean
+        |     |        |  +--ro address-ranges* [prefix]
+        |     |        |     +--ro prefix    -> ../config/prefix
+        |     |        |     +--ro config
+        |     |        |     |  +--ro prefix?   inet:ip-prefix
+        |     |        |     |  +--ro status?   enumeration
+        |     |        |     +--ro state
+        |     |        |        +--ro prefix?   inet:ip-prefix
+        |     |        |        +--ro status?   enumeration
+        |     |        +--rw interfaces
+        |     |        |  +--rw interface* [id]
+        |     |        |     +--rw id                  -> ../config/id
+        |     |        |     +--rw config
+        |     |        |     |  +--rw id?                             string
+        |     |        |     |  +--rw network-type?                   identityref
+        |     |        |     |  +--rw priority?                       uint8
+        |     |        |     |  +--rw multi-area-adjacency-primary?   boolean
+        |     |        |     |  +--rw authentication-type?            string
+        |     |        |     |  +--rw metric?                         oc-ospf-types:ospf-metric
+        |     |        |     |  +--rw passive?                        boolean
+        |     |        |     |  +--rw hide-network?                   boolean
+        |     |        |     |  +--rw oc-ospfv3:instance-id?          uint8
+        |     |        |     |  +--rw oc-ospfv3:interface-id?         uint32
+        |     |        |     +--ro state
+        |     |        |     |  +--ro id?                             string
+        |     |        |     |  +--ro network-type?                   identityref
+        |     |        |     |  +--ro priority?                       uint8
+        |     |        |     |  +--ro multi-area-adjacency-primary?   boolean
+        |     |        |     |  +--ro authentication-type?            string
+        |     |        |     |  +--ro metric?                         oc-ospf-types:ospf-metric
+        |     |        |     |  +--ro passive?                        boolean
+        |     |        |     |  +--ro hide-network?                   boolean
+        |     |        |     |  +--ro dr-router-id?                   yang:dotted-quad
+        |     |        |     |  +--ro dr-ip-address?                  inet:ip-address
+        |     |        |     |  +--ro bdr-router-id?                  yang:dotted-quad
+        |     |        |     |  +--ro bdr-ip-address?                 inet:ip-address
+        |     |        |     |  +--ro oc-ospfv3:instance-id?          uint8
+        |     |        |     |  +--ro oc-ospfv3:interface-id?         uint32
+        |     |        |     +--rw interface-ref
+        |     |        |     |  +--rw config
+        |     |        |     |  |  +--rw interface?      -> /oc-if:interfaces/interface/name
+        |     |        |     |  |  +--rw subinterface?   -> /oc-if:interfaces/interface[oc-if:name=current()/../interface]/subinterfaces/subinterface/index
+        |     |        |     |  +--ro state
+        |     |        |     |     +--ro interface?      -> /oc-if:interfaces/interface/name
+        |     |        |     |     +--ro subinterface?   -> /oc-if:interfaces/interface[oc-if:name=current()/../interface]/subinterfaces/subinterface/index
+        |     |        |     +--rw timers
+        |     |        |     |  +--rw config
+        |     |        |     |  |  +--rw dead-interval?                  uint32
+        |     |        |     |  |  +--rw hello-interval?                 uint32
+        |     |        |     |  |  +--rw retransmission-interval?        uint32
+        |     |        |     |  |  +--rw interface-transmission-delay?   uint32
+        |     |        |     |  +--ro state
+        |     |        |     |     +--ro dead-interval?                  uint32
+        |     |        |     |     +--ro hello-interval?                 uint32
+        |     |        |     |     +--ro retransmission-interval?        uint32
+        |     |        |     |     +--ro interface-transmission-delay?   uint32
+        |     |        |     +--rw lsa-filter
+        |     |        |     |  +--rw config
+        |     |        |     |  |  +--rw all?   boolean
+        |     |        |     |  +--ro state
+        |     |        |     |     +--ro all?   boolean
+        |     |        |     +--rw static-neighbors
+        |     |        |     |  +--rw static-neighbor* [neighbor-ip-address]
+        |     |        |     |     +--rw neighbor-ip-address    -> ../config/neighbor-ip-address
+        |     |        |     |     +--rw config
+        |     |        |     |        +--rw neighbor-ip-address?   inet:ip-address
+        |     |        |     |        +--rw metric?                oc-ospf-types:ospf-metric
+        |     |        |     |        +--rw poll-interval?         uint16
+        |     |        |     |        +--rw priority?              uint8
+        |     |        |     +--ro neighbors
+        |     |        |     |  +--ro neighbor* [router-id]
+        |     |        |     |     +--ro router-id    -> ../state/router-id
+        |     |        |     |     +--ro state
+        |     |        |     |        +--ro router-id?                     yang:dotted-quad
+        |     |        |     |        +--ro neighbor-ip-address?           inet:ip-address
+        |     |        |     |        +--ro priority?                      uint8
+        |     |        |     |        +--ro dead-time?                     oc-types:timeticks64
+        |     |        |     |        +--ro dr-router-id?                  yang:dotted-quad
+        |     |        |     |        +--ro dr-ip-address?                 inet:ip-address
+        |     |        |     |        +--ro bdr-router-id?                 yang:dotted-quad
+        |     |        |     |        +--ro bdr-ip-address?                inet:ip-address
+        |     |        |     |        +--ro optional-capabilities?         yang:hex-string
+        |     |        |     |        +--ro last-established-time?         oc-types:timeticks64
+        |     |        |     |        +--ro adjacency-state?               identityref
+        |     |        |     |        +--ro state-changes?                 uint32
+        |     |        |     |        +--ro retransmission-queue-length?   uint32
+        |     |        |     +--rw enable-bfd
+        |     |        |        +--rw config
+        |     |        |        |  +--rw enabled?                       boolean
+        |     |        |        |  +--rw desired-minimum-tx-interval?   uint32
+        |     |        |        |  +--rw required-minimum-receive?      uint32
+        |     |        |        |  +--rw detection-multiplier?          uint8
+        |     |        |        +--ro state
+        |     |        |           +--ro enabled?                       boolean
+        |     |        |           +--ro desired-minimum-tx-interval?   uint32
+        |     |        |           +--ro required-minimum-receive?      uint32
+        |     |        |           +--ro detection-multiplier?          uint8
+        |     |        +--rw virtual-links
+        |     |           +--rw virtual-link* [remote-router-id]
+        |     |              +--rw remote-router-id    -> ../config/remote-router-id
+        |     |              +--rw config
+        |     |              |  +--rw remote-router-id?   inet:ipv4-address-no-zone
+        |     |              +--ro state
+        |     |                 +--ro remote-router-id?              inet:ipv4-address-no-zone
+        |     |                 +--ro router-id?                     yang:dotted-quad
+        |     |                 +--ro neighbor-ip-address?           inet:ip-address
+        |     |                 +--ro priority?                      uint8
+        |     |                 +--ro dead-time?                     oc-types:timeticks64
+        |     |                 +--ro dr-router-id?                  yang:dotted-quad
+        |     |                 +--ro dr-ip-address?                 inet:ip-address
+        |     |                 +--ro bdr-router-id?                 yang:dotted-quad
+        |     |                 +--ro bdr-ip-address?                inet:ip-address
+         |     |                 +--ro optional-capabilities?         yang:hex-string
+         |     |                 +--ro last-established-time?         oc-types:timeticks64
+         |     |                 +--ro adjacency-state?               identityref
+         |     |                 +--ro state-changes?                 uint32
+         |     |                 +--ro retransmission-queue-length?   uint32

@dplore
Copy link
Member

dplore commented Feb 19, 2025

/gcbrun

@dplore
Copy link
Member

dplore commented Feb 19, 2025

Note, the ygot check failing appears to be a problem with the github check, not your model. We'll fix that soon.

@dplore
Copy link
Member

dplore commented Feb 19, 2025

/gcbrun

@cpeng90
Copy link
Author

cpeng90 commented Feb 20, 2025

@cpeng90 please check the ygot and linter errors. For example, all config leafs must also be present as state leaves

To avoid duplicating leaves, can the openconfig-ospf-area-interface.yang submodule replace the existing ospfv2 area interface submodule? (openconfig-ospfv2-area-interface.yang). Then this new ospf-area-interface sub module can serve as a common submodule for both v2 and v3? (And then the additional v3 leaves would continue to appear in the v3 module you are adding here).

I haven't looked super closely yet, but if this would still result in the same ospfv2 tree to be rendered, without any breaking changes, then this is a good idea I think. If there would be breaking changes to the ospv2 tree, please point out where those would be. We certainly do not want to make any breaking changes to ospfv2 (or really any changes at all).

@dplore I am thinking we could break this into two steps. The first step is to get OSPFv3 model in. The second step is to migrate OSPFv2 model to the common one. Right now, the LSDB portion of the OSPFv2 model does not look right and needs quite a bit restructure. Some of the definitions in that model needs to be split as they only apply to OSPFv2. My initial plan is that after we merge this change, I will submit another to address OSPFv2 modeling. Please let me know your thoughts.

@cpeng90 cpeng90 force-pushed the ospfv3 branch 2 times, most recently from 2d80b49 to 5677018 Compare February 21, 2025 19:47
@cpeng90
Copy link
Author

cpeng90 commented Feb 21, 2025

       |     +--rw ospfv3
      |     |  +--rw global
      |     |  |  +--rw config
      |     |  |  |  +--rw router-id?                    yang:dotted-quad
      |     |  |  |  +--rw log-adjacency-changes?        boolean
      |     |  |  |  +--rw hide-transit-only-networks?   boolean
      |     |  |  |  +--rw abr-capability?               identityref
      |     |  |  +--ro state
      |     |  |  |  +--ro router-id?                    yang:dotted-quad
      |     |  |  |  +--ro log-adjacency-changes?        boolean
      |     |  |  |  +--ro hide-transit-only-networks?   boolean
      |     |  |  |  +--ro abr-capability?               identityref
      |     |  |  +--rw timers
      |     |  |  |  +--rw spf
      |     |  |  |  |  +--rw config
      |     |  |  |  |  |  +--rw initial-delay?   uint32
      |     |  |  |  |  |  +--rw maximum-delay?   uint32
      |     |  |  |  |  +--ro state
      |     |  |  |  |     +--ro initial-delay?   uint32
      |     |  |  |  |     +--ro maximum-delay?   uint32
      |     |  |  |  |     +--ro timer-type?      enumeration
      |     |  |  |  +--rw max-metric
      |     |  |  |  |  +--rw config
      |     |  |  |  |  |  +--rw set?       boolean
      |     |  |  |  |  |  +--rw timeout?   uint64
      |     |  |  |  |  |  +--rw include*   identityref
      |     |  |  |  |  |  +--rw trigger*   identityref
      |     |  |  |  |  +--ro state
      |     |  |  |  |     +--ro set?       boolean
      |     |  |  |  |     +--ro timeout?   uint64
      |     |  |  |  |     +--ro include*   identityref
      |     |  |  |  |     +--ro trigger*   identityref
      |     |  |  |  +--rw lsa-generation
      |     |  |  |     +--rw config
      |     |  |  |     |  +--rw initial-delay?   uint32
      |     |  |  |     |  +--rw maximum-delay?   uint32
      |     |  |  |     +--ro state
      |     |  |  |        +--ro initial-delay?   uint32
      |     |  |  |        +--ro maximum-delay?   uint32
      |     |  |  |        +--ro timer-type?      enumeration
      |     |  |  +--rw graceful-restart
      |     |  |  |  +--rw config
      |     |  |  |  |  +--rw enabled?       boolean
      |     |  |  |  |  +--rw helper-only?   boolean
      |     |  |  |  +--ro state
      |     |  |  |     +--ro enabled?       boolean
      |     |  |  |     +--ro helper-only?   boolean
      |     |  |  +--rw inter-area-propagation-policies
      |     |  |     +--rw inter-area-propagation-policy* [src-area dst-area]
      |     |  |        +--rw src-area    -> ../config/src-area
      |     |  |        +--rw dst-area    -> ../config/dst-area
      |     |  |        +--rw config
      |     |  |        |  +--rw src-area?                -> ../../../../../areas/area/identifier
      |     |  |        |  +--rw dst-area?                -> ../../../../../areas/area/identifier
      |     |  |        |  +--rw import-policy*           -> /oc-rpol:routing-policy/policy-definitions/policy-definition/name
      |     |  |        |  +--rw default-import-policy?   default-policy-type
      |     |  |        +--ro state
      |     |  |           +--ro src-area?                -> ../../../../../areas/area/identifier
      |     |  |           +--ro dst-area?                -> ../../../../../areas/area/identifier
      |     |  |           +--ro import-policy*           -> /oc-rpol:routing-policy/policy-definitions/policy-definition/name
      |     |  |           +--ro default-import-policy?   default-policy-type
      |     |  +--rw areas
      |     |     +--rw area* [identifier]
      |     |        +--rw identifier        -> ../config/identifier
      |     |        +--rw config
      |     |        |  +--rw identifier?          oc-ospf-types:ospf-area-identifier
      |     |        |  +--rw type?                identityref
      |     |        |  +--rw stub-default-cost?   uint32
      |     |        |  +--rw import-summaries?    boolean
      |     |        +--ro state
      |     |        |  +--ro identifier?          oc-ospf-types:ospf-area-identifier
      |     |        |  +--ro type?                identityref
      |     |        |  +--ro stub-default-cost?   uint32
      |     |        |  +--ro import-summaries?    boolean
      |     |        +--rw interfaces
      |     |        |  +--rw interface* [id]
      |     |        |     +--rw id                  -> ../config/id
      |     |        |     +--rw config
      |     |        |     |  +--rw id?                             string
      |     |        |     |  +--rw network-type?                   identityref
      |     |        |     |  +--rw priority?                       uint8
      |     |        |     |  +--rw multi-area-adjacency-primary?   boolean
      |     |        |     |  +--rw authentication-type?            string
      |     |        |     |  +--rw metric?                         oc-ospf-types:ospf-metric
      |     |        |     |  +--rw passive?                        boolean
      |     |        |     |  +--rw hide-network?                   boolean
      |     |        |     |  +--rw oc-ospfv3:instance-id?          uint8
      |     |        |     |  +--rw oc-ospfv3:interface-id?         uint32
      |     |        |     +--ro state
      |     |        |     |  +--ro id?                             string
      |     |        |     |  +--ro network-type?                   identityref
      |     |        |     |  +--ro priority?                       uint8
      |     |        |     |  +--ro multi-area-adjacency-primary?   boolean
      |     |        |     |  +--ro authentication-type?            string
      |     |        |     |  +--ro metric?                         oc-ospf-types:ospf-metric
      |     |        |     |  +--ro passive?                        boolean
      |     |        |     |  +--ro hide-network?                   boolean
      |     |        |     |  +--ro dr-router-id?                   yang:dotted-quad
      |     |        |     |  +--ro dr-ip-address?                  inet:ip-address
      |     |        |     |  +--ro bdr-router-id?                  yang:dotted-quad
      |     |        |     |  +--ro bdr-ip-address?                 inet:ip-address
      |     |        |     |  +--ro oc-ospfv3:instance-id?          uint8
      |     |        |     |  +--ro oc-ospfv3:interface-id?         uint32
      |     |        |     +--rw interface-ref
      |     |        |     |  +--rw config
      |     |        |     |  |  +--rw interface?      -> /oc-if:interfaces/interface/name
      |     |        |     |  |  +--rw subinterface?   -> /oc-if:interfaces/interface[oc-if:name=current()/../interface]/subinterfaces/subinterface/index
      |     |        |     |  +--ro state
      |     |        |     |     +--ro interface?      -> /oc-if:interfaces/interface/name
      |     |        |     |     +--ro subinterface?   -> /oc-if:interfaces/interface[oc-if:name=current()/../interface]/subinterfaces/subinterface/index
      |     |        |     +--rw timers
      |     |        |     |  +--rw config
      |     |        |     |  |  +--rw dead-interval?                  uint32
      |     |        |     |  |  +--rw hello-interval?                 uint32
      |     |        |     |  |  +--rw retransmission-interval?        uint32
      |     |        |     |  |  +--rw interface-transmission-delay?   uint32
      |     |        |     |  +--ro state
      |     |        |     |     +--ro dead-interval?                  uint32
      |     |        |     |     +--ro hello-interval?                 uint32
      |     |        |     |     +--ro retransmission-interval?        uint32
      |     |        |     |     +--ro interface-transmission-delay?   uint32
      |     |        |     +--rw lsa-filter
      |     |        |     |  +--rw config
      |     |        |     |  |  +--rw all?   boolean
      |     |        |     |  +--ro state
      |     |        |     |     +--ro all?   boolean
      |     |        |     +--rw static-neighbors
      |     |        |     |  +--rw static-neighbor* [neighbor-ip-address]
      |     |        |     |     +--rw neighbor-ip-address    -> ../config/neighbor-ip-address
      |     |        |     |     +--rw config
      |     |        |     |     |  +--rw neighbor-ip-address?   inet:ip-address
      |     |        |     |     |  +--rw metric?                oc-ospf-types:ospf-metric
      |     |        |     |     |  +--rw poll-interval?         uint16
      |     |        |     |     |  +--rw priority?              uint8
      |     |        |     |     +--ro state
      |     |        |     |        +--ro neighbor-ip-address?   inet:ip-address
      |     |        |     |        +--ro metric?                oc-ospf-types:ospf-metric
      |     |        |     |        +--ro poll-interval?         uint16
      |     |        |     |        +--ro priority?              uint8
      |     |        |     +--ro neighbors
      |     |        |     |  +--ro neighbor* [router-id]
      |     |        |     |     +--ro router-id    -> ../state/router-id
      |     |        |     |     +--ro state
      |     |        |     |        +--ro router-id?                     yang:dotted-quad
      |     |        |     |        +--ro neighbor-ip-address?           inet:ip-address
      |     |        |     |        +--ro priority?                      uint8
      |     |        |     |        +--ro dead-time?                     oc-types:timeticks64
      |     |        |     |        +--ro dr-router-id?                  yang:dotted-quad
      |     |        |     |        +--ro dr-ip-address?                 inet:ip-address
      |     |        |     |        +--ro bdr-router-id?                 yang:dotted-quad
      |     |        |     |        +--ro bdr-ip-address?                inet:ip-address
      |     |        |     |        +--ro optional-capabilities?         yang:hex-string
      |     |        |     |        +--ro last-established-time?         oc-types:timeticks64
      |     |        |     |        +--ro adjacency-state?               identityref
      |     |        |     |        +--ro state-changes?                 uint32
      |     |        |     |        +--ro retransmission-queue-length?   uint32
      |     |        |     +--rw enable-bfd
      |     |        |        +--rw config
      |     |        |        |  +--rw enabled?                       boolean
      |     |        |        |  +--rw desired-minimum-tx-interval?   uint32
      |     |        |        |  +--rw required-minimum-receive?      uint32
      |     |        |        |  +--rw detection-multiplier?          uint8
      |     |        |        +--ro state
      |     |        |           +--ro enabled?                       boolean
      |     |        |           +--ro desired-minimum-tx-interval?   uint32
      |     |        |           +--ro required-minimum-receive?      uint32
      |     |        |           +--ro detection-multiplier?          uint8
      |     |        +--rw virtual-links
      |     |        |  +--rw virtual-link* [remote-router-id]
      |     |        |     +--rw remote-router-id    -> ../config/remote-router-id
      |     |        |     +--rw config
      |     |        |     |  +--rw remote-router-id?   inet:ipv4-address-no-zone
      |     |        |     +--ro state
      |     |        |        +--ro remote-router-id?              inet:ipv4-address-no-zone
      |     |        |        +--ro router-id?                     yang:dotted-quad
      |     |        |        +--ro neighbor-ip-address?           inet:ip-address
      |     |        |        +--ro priority?                      uint8
      |     |        |        +--ro dead-time?                     oc-types:timeticks64
      |     |        |        +--ro dr-router-id?                  yang:dotted-quad
      |     |        |        +--ro dr-ip-address?                 inet:ip-address
      |     |        |        +--ro bdr-router-id?                 yang:dotted-quad
      |     |        |        +--ro bdr-ip-address?                inet:ip-address
      |     |        |        +--ro optional-capabilities?         yang:hex-string
      |     |        |        +--ro last-established-time?         oc-types:timeticks64
      |     |        |        +--ro adjacency-state?               identityref
      |     |        |        +--ro state-changes?                 uint32
      |     |        |        +--ro retransmission-queue-length?   uint32
      |     |        +--rw address-ranges
      |     |           +--rw address-range* [prefix]
      |     |              +--rw prefix    -> ../config/prefix
      |     |              +--rw config
      |     |              |  +--rw prefix?   inet:ip-prefix
      |     |              |  +--rw status?   enumeration
      |     |              +--ro state
      |     |                 +--ro prefix?   inet:ip-prefix
      |     |                 +--ro status?   enumeration

@dplore
Copy link
Member

dplore commented Feb 21, 2025

/gcbrun

@dplore
Copy link
Member

dplore commented Mar 5, 2025

There are some problems with the schema style to be OpenConfig compliant. See the github checks. I think the key issues are found in the OpenConfig linter check which reports:

ospf/openconfig-ospf-area.yang (39): error:
list key "prefix" should be type leafref with a reference to the corresponding leaf in config or state container

ospf/openconfig-ospf-area.yang (150): error:
Parent node (config) has multiple children where one is a list, but a list is not allowed to have siblings: (leaf: identifier, leaf: type, leaf: stub-default-cost, leaf: import-summaries, list: address-ranges)

ospf/openconfig-ospf-area.yang (157): error:
Parent node (state) has multiple children where one is a list, but a list is not allowed to have siblings: (leaf: identifier, leaf: type, leaf: stub-default-cost, leaf: import-summaries, list: address-ranges)

@dplore
Copy link
Member

dplore commented Mar 5, 2025

For example, the inter-area-propagation-policies container has a proper list and key schema:

        list inter-area-propagation-policy {
          key "src-area dst-area";
          description
            "A list of connections between pairs of areas - routes are
            propagated from the source (src) area to the destination (dst)
            area according to the policy specified";

          leaf src-area {
            type leafref {
              path "../config/src-area";
            }
            description
              "Reference to the source area";
          }

          leaf dst-area {
            type leafref {
              path "../config/dst-area";
            }
            description
              "Reference to the destination area";
          }

@dplore
Copy link
Member

dplore commented Mar 5, 2025

@cpeng90 please check the ygot and linter errors. For example, all config leafs must also be present as state leaves
To avoid duplicating leaves, can the openconfig-ospf-area-interface.yang submodule replace the existing ospfv2 area interface submodule? (openconfig-ospfv2-area-interface.yang). Then this new ospf-area-interface sub module can serve as a common submodule for both v2 and v3? (And then the additional v3 leaves would continue to appear in the v3 module you are adding here).
I haven't looked super closely yet, but if this would still result in the same ospfv2 tree to be rendered, without any breaking changes, then this is a good idea I think. If there would be breaking changes to the ospv2 tree, please point out where those would be. We certainly do not want to make any breaking changes to ospfv2 (or really any changes at all).

@dplore I am thinking we could break this into two steps. The first step is to get OSPFv3 model in. The second step is to migrate OSPFv2 model to the common one. Right now, the LSDB portion of the OSPFv2 model does not look right and needs quite a bit restructure. Some of the definitions in that model needs to be split as they only apply to OSPFv2. My initial plan is that after we merge this change, I will submit another to address OSPFv2 modeling. Please let me know your thoughts.

I don't object to this two step approach. It seems like a way to quickly add OSPF v3 and push the refactoring of OSPF v2 to a future PR.

Copy link
Member

@dplore dplore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address the OC linter errors. Then we can complete the review

@dplore dplore self-assigned this Mar 5, 2025
@cpeng90 cpeng90 force-pushed the ospfv3 branch 2 times, most recently from 4012515 to a2da241 Compare March 5, 2025 18:07
@cpeng90
Copy link
Author

cpeng90 commented Mar 5, 2025

There are some problems with the schema style to be OpenConfig compliant. See the github checks. I think the key issues are found in the OpenConfig linter check which reports:

ospf/openconfig-ospf-area.yang (39): error:
list key "prefix" should be type leafref with a reference to the corresponding leaf in config or state container

ospf/openconfig-ospf-area.yang (150): error:
Parent node (config) has multiple children where one is a list, but a list is not allowed to have siblings: (leaf: identifier, leaf: type, leaf: stub-default-cost, leaf: import-summaries, list: address-ranges)

ospf/openconfig-ospf-area.yang (157): error:
Parent node (state) has multiple children where one is a list, but a list is not allowed to have siblings: (leaf: identifier, leaf: type, leaf: stub-default-cost, leaf: import-summaries, list: address-ranges)

Hi Darren, I made the change to address these issues. Please let me know if there is anything that needs to be updated. By the way, is there a command that I can run locally to check this kind of errors?

@dplore
Copy link
Member

dplore commented Mar 5, 2025

/gcbrun

1 similar comment
@dplore
Copy link
Member

dplore commented Mar 5, 2025

/gcbrun

@dplore
Copy link
Member

dplore commented Mar 5, 2025

Hi, couple more checks are failing:

  • You'll need to update the spec.yml build (for documentation generation) to include your new model file.

spec.yml build reachability check
openconfig-ospfv3-area-interface.yang: file not used by any .spec.yml build.

  • Ensure all submodules have version numbers which match the module:

⛔ submodule versions must match the belonging module's version
module set openconfig-network-instance is at 4.5.1 (openconfig-network-instance-l2.yang), non-matching files: openconfig-network-instance.yang (4.5.0)

@Shashank-arista
Copy link
Contributor

Shashank-arista commented Mar 6, 2025

authentication-type parameter doesn't correctly/fully capture all the auth related configuration. This is true even for ospfv2 model. We have an open PR to augment the same(#1127 )which is not merged yet. It might be interesting to update auth related structure for ospfv3 as well.

@cpeng90
Copy link
Author

cpeng90 commented Mar 6, 2025

authentication-type parameter doesn't correctly/fully capture all the auth related configuration. This is true even for ospfv2 model. We have an open PR to augment the same(#1127 )which is not merged yet. It might be interesting to update auth related structure for ospfv3 as well.

Unlike OSPFv2, OSPFv3 uses IPsec as the only mechanism for authenticating protocol packets as described in RFC7166. The intention is to complete OSPFv3 definition defined in RFC5340 which removes authentication. RFC7166 could be a separate activity for SMEs in that domain.

@cpeng90
Copy link
Author

cpeng90 commented Mar 6, 2025

/gcbrun

@cpeng90
Copy link
Author

cpeng90 commented Mar 6, 2025

Hi, couple more checks are failing:

  • You'll need to update the spec.yml build (for documentation generation) to include your new model file.

spec.yml build reachability check openconfig-ospfv3-area-interface.yang: file not used by any .spec.yml build.

  • Ensure all submodules have version numbers which match the module:

⛔ submodule versions must match the belonging module's version module set openconfig-network-instance is at 4.5.1 (openconfig-network-instance-l2.yang), non-matching files: openconfig-network-instance.yang (4.5.0)

Done

@dplore
Copy link
Member

dplore commented Mar 6, 2025

/gcbrun

@dplore
Copy link
Member

dplore commented Mar 7, 2025

/gcbrun

@@ -2,6 +2,8 @@
docs:
- yang/ospf/openconfig-ospf-types.yang
- yang/ospf/openconfig-ospfv2.yang
- yang/ospf/openconfig-ospf.yang
- yang/ospf/openconfig-ospfv3-area-interface.yang
build:
- yang/network-instance/openconfig-network-instance.yang
- yang/policy/openconfig-routing-policy.yang
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also add openconfig-ospfv3-area-interface.yang here

(note error in the github checks:
⛔ .spec.yml build reachability check
openconfig-ospfv3-area-interface.yang: file not used by any .spec.yml build.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Waiting for author
Development

Successfully merging this pull request may close these issues.

4 participants