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

Add Bfd diagnostic code for when neighbor signaled session down. #1233

Merged
merged 8 commits into from
Mar 5, 2025

Conversation

AnilKR123
Copy link
Contributor

Change Scope

I have added a Bfd diagnostic code for when neighbor signals session down. This is specified in RFC5880 but is absent from the public model.

From RFC5880:

 A diagnostic code specifying the local system's reason for the
  last change in session state.  Values are:

     0 -- No Diagnostic
     1 -- Control Detection Time Expired
     2 -- Echo Function Failed
     3 -- Neighbor Signaled Session Down
     4 -- Forwarding Plane Reset
     5 -- Path Down
     6 -- Concatenated Path Down
     7 -- Administratively Down
     8 -- Reverse Concatenated Path Down

Verified

This commit was signed with the committer’s verified signature.
yuyichao Yichao Yu
@AnilKR123 AnilKR123 requested a review from a team as a code owner December 16, 2024 23:06
@AnilKR123
Copy link
Contributor Author

I moved NEIGHBOR_DOWN to the end of the enums list.

Thanks.

@AnilKR123
Copy link
Contributor Author

I would like to propose deprecating the bfd-diagnostic-code enum and creating a new enum, perhaps called rfc5880-bfd-diagnostic-code. The bfd-diagnostic-code enum is missing “Neighbor Signaled Session Down” diag, as specified in RFC5880:

A diagnostic code specifying the local system's reason for the
last change in session state. Values are:

 0 -- No Diagnostic
 1 -- Control Detection Time Expired
 2 -- Echo Function Failed
 3 -- Neighbor Signaled Session Down
 4 -- Forwarding Plane Reset
 5 -- Path Down
 6 -- Concatenated Path Down
 7 -- Administratively Down
 8 -- Reverse Concatenated Path Down

The objection to adding this diagnostic code to enum value 3 is that it would be a breaking change. However adding it to the end of the enum would not only make Neighbor Down not compliant, but also all of the enum values after 3 differ in relation to the RFC. This adds operational complexity since clients now need a parser to map between the RFC codes and OC public codes.

By adding a new enum for the diag codes, we could support the RFC compliant values. And each vendor can decide which leaf they want to support for now.

@dplore
Copy link
Member

dplore commented Feb 11, 2025

/gcbrun

@OpenConfigBot
Copy link

OpenConfigBot commented Feb 11, 2025

No major YANG version changes in commit ee8fa28

@dplore
Copy link
Member

dplore commented Feb 11, 2025

OC Operator opinion is that it's more important to have a vendor neutral model where all vendors consistently implement the same OpenConfig model and there is only one way to model the BFD state. It's also viewed as more churn to refactor the model than to add the enum to the end, even though it doesn't match the packets on the wire. So this PR change is approved as is.

Last call for Feb 25, 2025.

@dplore
Copy link
Member

dplore commented Feb 11, 2025

/gcbrun

@dplore
Copy link
Member

dplore commented Feb 11, 2025

@AnilKR123 one misc check is failing, can you fix it please?

openconfig-bfd.yang: file updated but openconfig-version string not updated: "0.3.0"

@AnilKR123
Copy link
Contributor Author

Hi @dplore, I have fixed the issue by bumping the version number. Thanks.

@dplore
Copy link
Member

dplore commented Feb 11, 2025

/gcbrun

@AnilKR123 AnilKR123 requested a review from dplore February 13, 2025 18:50
@dplore
Copy link
Member

dplore commented Feb 19, 2025

/gcbrun

@dplore
Copy link
Member

dplore commented Feb 21, 2025

/gcbrun

@dplore dplore merged commit 11658ea into openconfig:master Mar 5, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants