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.

Sign up for GitHub

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

Jump to bottom

Fix ospf lsdb router-lsa link list #1080

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

Fix ospf lsdb router-lsa link list #1080

wants to merge 15 commits into from

Conversation

dplore
Copy link
Member

@dplore dplore commented Mar 21, 2024 •
edited
Loading

Fixes #1074

Operational use case

OSPF router-lsa describes a list of link-ids, but the OC model only allows for a single link id. This PR moves the link-id and it's attributes to a new list inside of the router-lsa.

Change Scope

Also note this proposes using the yang status obsolete for the leaf. OpenConfig has not traditionally used this status.
Consider this a solicitation for comments if we should start using obsolete for 'deleted' leafs, versus removing them from the model entirely.

Tree view

--- /Users/dloher/old-tree.txt  2024-03-21 10:52:00
+++ /Users/dloher/ospf-tree.txt 2024-03-21 13:40:35
@@ -5878,18 +5878,23 @@
         |     |        |              |  +--ro age?                  uint16
         |     |        |              +--ro router-lsa
         |     |        |              |  +--ro state
-        |     |        |              |  |  +--ro type?                 identityref
-        |     |        |              |  |  +--ro link-id?              yang:dotted-quad
-        |     |        |              |  |  +--ro link-data?            union
-        |     |        |              |  |  +--ro metric?               oc-ospf-types:ospf-metric
-        |     |        |              |  |  +--ro number-links?         uint16
-        |     |        |              |  |  +--ro number-tos-metrics?   uint16
-        |     |        |              |  +--ro types-of-service
-        |     |        |              |     +--ro type-of-service* [tos]
-        |     |        |              |        +--ro tos      -> ../state/tos
+        |     |        |              |  |  +--ro metric?         oc-ospf-types:ospf-metric
+        |     |        |              |  |  +--ro number-links?   uint16
+        |     |        |              |  +--ro links
+        |     |        |              |     +--ro link* [link-id]
+        |     |        |              |        +--ro link-id             -> ../state/link-id
         |     |        |              |        +--ro state
-        |     |        |              |           +--ro tos?      uint8
-        |     |        |              |           +--ro metric?   oc-ospf-types:ospf-metric
+        |     |        |              |        |  +--ro link-id?              yang:dotted-quad
+        |     |        |              |        |  +--ro type?                 identityref
+        |     |        |              |        |  +--ro link-data?            union
+        |     |        |              |        |  +--ro number-tos-metrics?   uint16
+        |     |        |              |        +--ro types-of-service
+        |     |        |              |           +--ro type-of-service* [tos]
+        |     |        |              |              +--ro tos      -> ../state/tos
+        |     |        |              |              +--ro state
+        |     |        |              |                 +--ro tos?      uint8
+        |     |        |              |                 +--ro metric?   oc-ospf-types:ospf-metric
         |     |        |              +--ro network-lsa
         |     |        |              |  +--ro state
         |     |        |              |     +--ro network-mask?      uint8

@dplore dplore requested a review from a team as a code owner March 21, 2024 18:07
@OpenConfigBot
Copy link

OpenConfigBot commented Mar 21, 2024 •
edited
Loading

No major YANG version changes in commit ae1fb73

@dplore dplore mentioned this pull request Mar 21, 2024
Open
@robshakir
Copy link
Contributor

What's the motivation for obsolete vs deprecated?

@dplore
Copy link
Member Author

dplore commented Mar 21, 2024

What's the motivation for obsolete vs deprecated?

In this case it's because the link-id leaf is not valid. Per OSPF implementation, router-lsa must contain a list of all the directly connected links of a given router. This is nearly always going to be > 1. So the current OC model is 'broken' and I suspect has not been implemented? I guess someone might have implemented this and chose to place just one of the router-lsa links?

@robshakir
Copy link
Contributor

What's the motivation for obsolete vs deprecated?

In this case it's because the link-id leaf is not valid. Per OSPF implementation, router-lsa must contain a list of all the directly connected links of a given router. This is nearly always going to be > 1. So the current OC model is 'broken' and I suspect has not been implemented? I guess someone might have implemented this and chose to place just one of the router-lsa links?

Whilst I see the argument here -- it seems that the two things are the same -- i.e., this leaf is no longer in the model. If I'm a developer now implementing the model whether it's obsolete or deprecated is the same thing -- "do not implement this leaf" :-)

From a tooling perspective though, we now need to ensure that all toolchains support both variants -- which are the same "don't generate for this leaf" where is code, documentation etc.

I prefer to limit our use of YANG "twiddles" to the absolute minimum possible -- supporting them in toolchains has been painful.

@dplore
Copy link
Member Author

dplore commented Mar 22, 2024

Whilst I see the argument here -- it seems that the two things are the same -- i.e., this leaf is no longer in the model. If I'm a developer now implementing the model whether it's obsolete or deprecated is the same thing -- "do not implement this leaf" :-)

From a tooling perspective though, we now need to ensure that all toolchains support both variants -- which are the same "don't generate for this leaf" where is code, documentation etc.

I prefer to limit our use of YANG "twiddles" to the absolute minimum possible -- supporting them in toolchains has been painful.

No worries, I'll deprecate the node instead. The node seems not usable as is, but the new node which replaces it doesn't conflict, so it seems there is little harm to leaving this node present in a deprecated form until a future major OC release. I appreciate any thoughts on this approach as well.

FWIW, deprecated means supported, but scheduled for deletion in an future release. Obsolete is the same as deleting the node from the model, but leaves the information what what the node used to be intact. I proposed obsolete here as I was curious about whether we should use 'obsolete' as we do the keyword 'reserved' in proto. Of course, the same issue with field numbering doesn't exist in yang (except perhaps with enums).

@dplore
Copy link
Member Author

dplore commented Mar 26, 2024

Upon reviewing with OC operators, we realized that there are many breaking changes here due to moving the leafs for links into the link list and moving the types of service list underneath the new link list. Since router-lsa is currently a broken node, it seems to make sense to just delete the link-id as well and make this a breaking change.

@dplore
Copy link
Member Author

dplore commented Mar 26, 2024

@DonaMaria2024 for your review

@dplore
Copy link
Member Author

dplore commented May 14, 2024

@DonaMaria2024 please provide any comments. I am waiting for an OC reviewer to give the final LGTM and then this can move to last call.

@dplore
Copy link
Member Author

dplore commented May 16, 2024

Adding to last-call. This will be merged on May 30, 2024

@DonaMaria2024
Copy link

DonaMaria2024 commented May 16, 2024 •
edited
Loading

Metric is the cost of a router-link which is a per link attribute.
Metric should be moved inside links/link [link-id] list.

@marcushines marcushines self-requested a review May 20, 2024 19:43
marcushines
marcushines reviewed May 20, 2024
View reviewed changes
release/models/ospf/openconfig-ospfv2-lsdb.yang

uses ospfv2-lsdb-router-lsa-link-state-specification;
}
uses ospfv2-lsdb-generic-lsa-tos-metric-structure;
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm is this an extra uses that is no longer necessary - i didn't notice this before that there are now 2 metrics?

@DonaMaria2024
Copy link

Hi,

Please find comments from [email protected] below.

  1. /router-lsa/state should have 2 leafs
    • V,E B flags
    • Number-of-links
  2. There should be a metric under
    /router-lsa/links/link

Thanks,
Dona

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcushines marcushines marcushines approved these changes

At least 1 approving review is required to merge this pull request.

Assignees

@dplore dplore

@marcushines marcushines

Labels
non-breaking
Projects
OC Operator Review
Status: Waiting for author
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support to fetch multiple link information from a single OSPF Router-LSA is missing.
5 participants
@dplore @OpenConfigBot @robshakir @DonaMaria2024 @marcushines