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

[yang]: Add admin_status to BGP_NEIGHBOR_TEMPLATE_LIST. #9341

Merged
merged 3 commits into from
Nov 25, 2021
Merged

[yang]: Add admin_status to BGP_NEIGHBOR_TEMPLATE_LIST. #9341

merged 3 commits into from
Nov 25, 2021

Conversation

ganglyu
Copy link
Contributor

@ganglyu ganglyu commented Nov 22, 2021

Signed-off-by: Gang Lv [email protected]

Why I did it

#9119
BGP_NEIGHBOR_TEMPLATE_LIST does not have admin_status field.

How I did it

Add admin_status to BGP_NEIGHBOR_TEMPLATE_LIST.

How to verify it

Follow the steps in #9119.
Build sonic-yang-model.

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106

Description for the changelog

fix #9119

A picture of a cute animal (not mandatory but encouraged)

@ganglyu ganglyu requested a review from qiluo-msft as a code owner November 22, 2021 06:15
@qiluo-msft qiluo-msft added the YANG YANG model related changes label Nov 22, 2021
qiluo-msft
qiluo-msft reviewed Nov 22, 2021
View reviewed changes
src/sonic-yang-models/yang-models/sonic-bgp-neighbor.yang Outdated

leaf admin_status {
type boolean;
description "To enable BGP peer";
Copy link
Collaborator

@qiluo-msft qiluo-msft Nov 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

To enable BGP peer

We can "enable" a BGP peer but keep it as admin down. How about "Admin status of BGP peer"? #Closed

dgsudharsan
dgsudharsan suggested changes Nov 22, 2021
View reviewed changes
Copy link
Collaborator

@dgsudharsan dgsudharsan left a comment

Choose a reason for hiding this comment

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

Please add UT to cover this case

src/sonic-yang-models/yang-models/sonic-bgp-neighbor.yang Outdated
@@ -97,6 +97,11 @@ module sonic-bgp-neighbor {
}
description "Route reflector client";
}

leaf admin_status {
type boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe you should use the sonic type admin status as the values for the field will be 'up'/'down'
https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-yang-models/yang-models/sonic-types.yang#L54

Copy link
Collaborator

Choose a reason for hiding this comment

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

			leaf admin_status {
				type stypes:admin_status;
			}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe you should use the sonic type admin status as the values for the field will be 'up'/'down' https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-yang-models/yang-models/sonic-types.yang#L54

Good suggestion!
I used the same type as sonic-bgp-common.yang, and please refer to BGP_NEIGHBOR_LIST.
https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-yang-models/yang-models/sonic-bgp-common.yang#L316-L319
Would you please confirm which type should be used?

Copy link
Collaborator

Choose a reason for hiding this comment

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

https://github.com/Azure/sonic-buildimage/blob/master/dockers/docker-fpm-frr/frr/bgpd/bgpd.spine_chassis_frontend_router.conf.j2

Per the above bgpcfgd module (refer above code), we should use 'up'/'down', please make the changes.

@ganglyu
Copy link
Contributor Author

ganglyu commented Nov 23, 2021

Please add UT to cover this case

I can't find any UT for BGP_NEIGHBOR_TEMPLATE_LIST, current UT only covers BGP_NEIGHBOR_LIST.
Do you know why?

@dgsudharsan
Copy link
Collaborator

I can't find any UT for BGP_NEIGHBOR_TEMPLATE_LIST, current UT only covers BGP_NEIGHBOR_LIST.

I am not sure. @venkatmahalingam Do you know?

@dgsudharsan
Copy link
Collaborator

@venkatmahalingam Can you please review?

venkatmahalingam
venkatmahalingam suggested changes Nov 23, 2021
View reviewed changes
Copy link
Collaborator

@venkatmahalingam venkatmahalingam left a comment

Choose a reason for hiding this comment

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

Let's make the admin_status type based on the backend implementation which is 'up'/'down'.

@ganglyu
1. Use the sonic type admin status.
8c9be7a 
2. Add UT for NEIGHBOR_TEMPLATE_LIST.

Signed-off-by: Gang Lv [email protected]
venkatmahalingam
venkatmahalingam reviewed Nov 23, 2021
View reviewed changes
src/sonic-yang-models/yang-models/sonic-bgp-neighbor.yang Outdated
@@ -7,6 +7,11 @@ module sonic-bgp-neighbor {
prefix inet;
}

import sonic-types {
prefix stypes;
revision-date 2019-07-01;
Copy link
Collaborator

@venkatmahalingam venkatmahalingam Nov 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Please remove the revision-date.

venkatmahalingam
venkatmahalingam suggested changes Nov 23, 2021
View reviewed changes
Copy link
Collaborator

@venkatmahalingam venkatmahalingam 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 comment.

qiluo-msft
qiluo-msft previously approved these changes Nov 23, 2021
View reviewed changes
Copy link
Collaborator

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

LGTM. Please also wait other active reviewers' approval.

@ganglyu
Copy link
Contributor Author

ganglyu commented Nov 24, 2021

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ganglyu
Copy link
Contributor Author

ganglyu commented Nov 24, 2021

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 9341 in repo Azure/sonic-buildimage

@ganglyu
Copy link
Contributor Author

ganglyu commented Nov 24, 2021

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lguohan lguohan merged commit adf1990 into sonic-net:master Nov 25, 2021
@ganglyu ganglyu added the Request for 202111 Branch For PRs being requested for 202111 branch label Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@venkatmahalingam venkatmahalingam venkatmahalingam approved these changes

@dgsudharsan dgsudharsan dgsudharsan approved these changes

@qiluo-msft qiluo-msft qiluo-msft approved these changes

@praveen-li praveen-li Awaiting requested review from praveen-li

@rathnasabapathyv rathnasabapathyv Awaiting requested review from rathnasabapathyv

Assignees
No one assigned
Labels
Included in 202111 Branch Request for 202111 Branch For PRs being requested for 202111 branch YANG YANG model related changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[yang-models] BGP_NEIGHBOR yang model is not up-to-date
6 participants
@ganglyu @dgsudharsan @qiluo-msft @venkatmahalingam @lguohan @judyjoseph