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

Isis configuration support #1234

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

Conversation

cchoate54
Copy link

@cchoate54 cchoate54 commented Jan 26, 2023

Here is the documentation to describe the initial changes proposed to introduce FRR-ISIS configuration to SONiC.

Enable isisd and add ISIS Config Support.

Repo PR title State
sonic-buildimage Enable isisd and add ISIS config Support ![GitHub issue/pull request detail](https://img.shields.io/github/pulls/detail/state/Azure/sonic-buildimage/13527 [img.shields.io])
sonic-utilities ISIS show command ![GitHub issue/pull request detail](https://img.shields.io/github/pulls/detail/state/Azure/sonic-utilities/2685 [img.shields.io])

@zhangyanzhao zhangyanzhao requested a review from rlhui January 26, 2023 23:45
Update frr-isis-sonic-yang-model-hld.md to include isis interface auth

Following section describes the changes to DB.

Added new configuration tables specific to FRR_ISIS features:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you be adding more details for each of these tables, and also define the schema?

Copy link
Author

Choose a reason for hiding this comment

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

doc/isis/frr-isis-sonic-yang-model-hld.md defines these tables


#### CLI/YANG Model Enhancements

There are no CLI changes made in this feature thus far.
Copy link
Contributor

Choose a reason for hiding this comment

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

Some basic CLI's should be planned.

Copy link
Author

Choose a reason for hiding this comment

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

Added some new cli support

@zhangyanzhao
Copy link
Collaborator

@cchoate54 can you please add the code PRs by referring to #806 ? Thanks.


sonic:~$ show run isis --config_db
{
"ISIS_GLOBAL": {
Copy link
Collaborator

@venkatmahalingam venkatmahalingam Apr 25, 2023

Choose a reason for hiding this comment

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

Can we include VRF as the key in the initial schema itself? we can support just default VRF for now. This will help to extend the support easily for user VRF in the future.

Copy link
Author

Choose a reason for hiding this comment

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

Included support for VRF similarly to how it was supported in the sonic-bgp-global yang model

ISIS Level Yang container is sonic-isis.yang.

```
container ISIS_LEVEL {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest to use the name ISIS_LEVELS

Copy link
Author

Choose a reason for hiding this comment

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

Updated the container name to ISIS_LEVELS

}

leaf ifname {
type string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should leafref to INTERFACE table.

Copy link
Author

Choose a reason for hiding this comment

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

Updated the type to leafref interface tables FRR would consider in-scope: PORT, PORTCHANNEL, and LOOPBACK_INTERFACE

```

##### SONiC ISIS Defined Types
Types defined in sonic-types.yang.j2.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

All defined ISIS types are now from openconfig isis types

key "instance level_number";

leaf instance {
type string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use leafref to instance present in global table.

Copy link
Author

Choose a reason for hiding this comment

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

ISIS_LEVELS and ISIS_GLOBALS can be configured independently of each other based on FRR allowed behavior. I added a must statement to make sure they are always the same instance value.

key "instance ifname";

leaf instance {
type string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use leafref to instance present in global table.

Copy link
Author

Choose a reason for hiding this comment

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

Added a leafref to instance in either ISIS_LEVELS or ISIS_GLOBAL

- FRR-ISIS config template files and isisd enabled by default in the FRR container
- /dockers/docker-fpm-frr
- Enable ISIS trap messages
- /files/image_config/copp
Copy link
Contributor

Choose a reason for hiding this comment

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

Please capture the copp section. Please ensure trap is installed only when feature is enabled

Copy link
Author

Choose a reason for hiding this comment

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

Based on https://github.com/sonic-net/SONiC/blob/master/doc/copp/CoPP%20Config%20and%20Management.md , it seems we should be able to add isis to the COPP_TRAP table only when needed. Would this be a sufficient solution for now ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Please add it on run time by updating COPP_TRAP entry. Please check is as example

https://github.com/sonic-net/sonic-ztp/pull/37/files

Copy link
Author

Choose a reason for hiding this comment

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

Added a step to add/remove 'isis' in the COPP_TRAP entry on bgp container startup when frr_mgmt_framework_config is updated

@zhangyanzhao
Copy link
Collaborator

@andriy-kokhan
Copy link

I believe that in an ideal case it would be great to have systemd service per routing protocol and also systemd target that groups these services. This could provide the best flexibility. E.g.,

routing.target
   bgp.service
   isis.service
   ospf.service

That's the way how we group SONiC services with sonic.target at the moment.
With this approach we can even implement a case with the static routes only (no routing protocols) which can be applicable for the mgmt switches.

@cchoate54
Copy link
Author

@venkatmahalingam and @dgsudharsan can you please review the changes made based on your suggestions

type string {
pattern "default";
}
type leafref {
Copy link
Collaborator

@venkatmahalingam venkatmahalingam May 20, 2023

Choose a reason for hiding this comment

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

Do we have ISIS support for user VRF as well? if yes, why do we have max-elements 1 at line 225?

Copy link
Author

Choose a reason for hiding this comment

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

This was added based on the FRR documentation mentioning "isisd does not yet support multiple ISIS processes but you must specify the name of ISIS process" in https://docs.frrouting.org/en/latest/isisd.html

Copy link
Author

Choose a reason for hiding this comment

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

@venkatmahalingam what are your thoughts here ?

@zhangyanzhao
Copy link
Collaborator

Moved from 202305 release to 202311.

@zhangyanzhao
Copy link
Collaborator

@cchoate54 can you please help to double check if all comments have been addressed? We need reviewers to approve this PR if no additional comments. Thanks.

@zhangyanzhao
Copy link
Collaborator

no update, move to backlog

@QupinghaoNN
Copy link

@cchoate54 Can configuration be done through the CLI of sonic-utilities to write to config_db, or can it only be done through REST or gNMI in the management framework?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: MovedToBacklog
Development

Successfully merging this pull request may close these issues.

7 participants