ospfd: implement passive-interface function of cisco #2359
Conversation
ospfd/ospfd.c
Outdated
| /* | ||
| * if router_id is not configured, dont bring up | ||
| * interfaces. | ||
| * ospf_router_id_update() will call ospf_if_update | ||
| * whenever r-id is configured instead. | ||
| */ | ||
| if ((area->ospf->router_id.s_addr != 0) && if_is_operative(co->ifp)) | ||
| if ((area->ospf->router_id.s_addr != 0) && if_is_operative(co->ifp) && | ||
| oi->passive_interface == OSPF_IF_ACTIVE) |
There was a problem hiding this comment.
Please indent this so it looks like:
if ((area->ospf->router_id.s_addr != 0) && if_is_operative(co->ifp) &&
oi->passive_interface == OSPF_IF_ACTIVE)
|
HI
I have push a second patch for fix the indent "ospfd: format indent for passive interface"
…------------------ Original ------------------
From: "Quentin Young"<notifications@github.com>;
Date: Tue, Jun 5, 2018 01:00 AM
To: "FRRouting/frr"<frr@noreply.github.com>;
Cc: "fengtian6WIND"<fengtian.guo@6wind.com>; "Author"<author@noreply.github.com>;
Subject: Re: [FRRouting/frr] ospfd: implement passive-interface function ofcisco (#2359)
@qlyoung commented on this pull request.
Please rewrap your code to 80 columns.
In ospfd/ospfd.c:
/* * if router_id is not configured, dont bring up * interfaces. * ospf_router_id_update() will call ospf_if_update * whenever r-id is configured instead. */ - if ((area->ospf->router_id.s_addr != 0) && if_is_operative(co->ifp)) + if ((area->ospf->router_id.s_addr != 0) && if_is_operative(co->ifp) && + oi->passive_interface == OSPF_IF_ACTIVE)
Please indent this so it looks like:
if ((area->ospf->router_id.s_addr != 0) && if_is_operative(co->ifp) && oi->passive_interface == OSPF_IF_ACTIVE)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Need to fix remaining Check Style, especially line over 80 characters. See CI report. |
ospfd/ospf_vty.c
Outdated
| ospf->passive_interface_default = OSPF_IF_ACTIVE; | ||
|
|
||
| /* XXX We should call ospf_if_set_multicast on exactly those | ||
| * * interfaces for which the passive property changed. It is too much |
There was a problem hiding this comment.
Please fix these comments, only the first asterisk should be present.
ospfd/ospf_interface.h
Outdated
| @@ -186,6 +186,7 @@ struct ospf_interface { | |||
|
|
|||
| struct prefix *address; /* Interface prefix */ | |||
| struct connected *connected; /* Pointer to connected */ | |||
| uint8_t passive_interface; /* 1 = passive interface, 0 = active interface */ | |||
There was a problem hiding this comment.
Could you clarify why we need this new variable when we already have params->passive_interface and the OSPF_IF_PASSIVE_STATUS macro?
|
continue format the patch |
|
@fengtian6WIND I see you made the following change in your latest commit: For me it makes no sense to have two different variables to control whether an interface is in the passive mode or not. Please remove this new variable or provide a very good explanation of why you're doing this. I see there are still a few places using |
There was a problem hiding this comment.
@fengtian6WIND Thanks for the updates. I've found a few problems with this PR.
Testing ospfd I got this:
rt2-ospfd# show running-config
[snip]
!
router ospf
ospf router-id 2.2.2.2
network 0.0.0.0/0 area 0
!
[snip]
rt2-ospfd#
rt2-ospfd# conf t
rt2-ospfd(config)# router ospf
rt2-ospfd(config-router)# passive-interface rt2-eth0
rt2-ospfd(config-router)# do sh run
[snip]
!
router ospf
ospf router-id 2.2.2.2
network 0.0.0.0/0 area 0
!
[snip]
rt2-ospfd(config-router)# passive-interface default
rt2-ospfd(config-router)#
rt2-ospfd(config-router)#
rt2-ospfd(config-router)# do sh ip ospf nei
Neighbor ID Pri State Dead Time Address Interface RXmtL RqstL DBsmL
1.1.1.1 1 Full/Backup 38.206s 10.0.1.1 rt2-eth0:10.0.1.2 0 0 0
3.3.3.3 1 Full/Backup 38.206s 10.0.2.3 rt2-eth1:10.0.2.2 0 0 0
4.4.4.4 1 Full/DR 38.206s 10.0.2.4 rt2-eth1:10.0.2.2 0 0 0
rt2-ospfd(config-router)#
rt2-ospfd(config-router)#
rt2-ospfd(config-router)# do sh run
[snip]
!
router ospf
ospf router-id 2.2.2.2
network 0.0.0.0/0 area 0
network 2.2.2.2/32 area 0
network 10.0.1.0/24 area 0
network 10.0.2.0/24 area 0
!
[snip]
Now the problems:
1 - The passive-interface command is not working at all. ospfd is forming adjacencies normally.
2 - Both passive-interface default and passive-interface IFNAME are not appearing in the running configuration. And you removed the logic to prepend a no before passive-interface IFNAME depending whether passive-interface default is configured or not.
3 - New network statements were introduced in the running configuration. Thinking from the perspective of the northbound API, my opinion is that a daemon shouldn't change its running configuration internally. sh run should reflect exactly what the operator configured.
To reinforce my last point, imagine someone configuring ospfd like this:
rt2-ospfd(config)# router ospf
rt2-ospfd(config-router)# passive-interface default
rt2-ospfd(config-router)# network 0.0.0.0/0 area 0
rt2-ospfd(config-router)# no network 0.0.0.0/0 area 0
rt2-ospfd(config-router)# network 0.0.0.0/0 area 5
rt2-ospfd(config-router)#
rt2-ospfd(config-router)# do sh run
[snip]
!
router ospf
ospf router-id 2.2.2.2
network 0.0.0.0/0 area 5
network 2.2.2.2/32 area 0
network 10.0.1.0/24 area 0
network 10.0.2.0/24 area 0
!
[snip]
Now the operator has a problem to solve.. and it could be worse depending on the number of interfaces. I think you should find a way to do what you're doing "under the hood" without affecting the running configuration. I'm not very familiar with ospfd code, but maybe you could ping @chiragshah6 to get some pointers on what would be the best way to solve this problem.
ospfd/ospf_vty.c
Outdated
|
|
||
| if (create == OSPF_IF_PASSIVE) { | ||
| /* create passive-interface from connected address */ | ||
| ospf->passive_interface_default = OSPF_IF_PASSIVE; |
There was a problem hiding this comment.
This doesn't make sense. You're setting ospf->passive_interface_default to OSPF_IF_PASSIVE here and setting it to OSPF_IF_ACTIVE a few lines below. The problem is that a) no one is checking the value of this variable and b) this function acts on interfaces but passive_interface_default is tied to OSPF areas and not OSPF interfaces.
|
Renato Westphal
Thanks for acknowledge
This week I have lot work to finish. And I will give a new patch for this
new issue
Thanks
…On Thu, Jun 14, 2018 at 1:41 AM, Renato Westphal ***@***.***> wrote:
***@***.**** requested changes on this pull request.
@fengtian6WIND <https://github.com/fengtian6WIND> Thanks for the updates.
I've found a few problems with this PR.
Testing ospfd I got this:
rt2-ospfd# show running-config
[snip]
!
router ospf
ospf router-id 2.2.2.2
network 0.0.0.0/0 area 0
!
[snip]
rt2-ospfd#
rt2-ospfd# conf t
rt2-ospfd(config)# router ospf
rt2-ospfd(config-router)# passive-interface rt2-eth0
rt2-ospfd(config-router)# do sh run
[snip]
!
router ospf
ospf router-id 2.2.2.2
network 0.0.0.0/0 area 0
!
[snip]
rt2-ospfd(config-router)# passive-interface default
rt2-ospfd(config-router)#
rt2-ospfd(config-router)#
rt2-ospfd(config-router)# do sh ip ospf nei
Neighbor ID Pri State Dead Time Address Interface RXmtL RqstL DBsmL
1.1.1.1 1 Full/Backup 38.206s 10.0.1.1 rt2-eth0:10.0.1.2 0 0 0
3.3.3.3 1 Full/Backup 38.206s 10.0.2.3 rt2-eth1:10.0.2.2 0 0 0
4.4.4.4 1 Full/DR 38.206s 10.0.2.4 rt2-eth1:10.0.2.2 0 0 0
rt2-ospfd(config-router)#
rt2-ospfd(config-router)#
rt2-ospfd(config-router)# do sh run
[snip]
!
router ospf
ospf router-id 2.2.2.2
network 0.0.0.0/0 area 0
network 2.2.2.2/32 area 0
network 10.0.1.0/24 area 0
network 10.0.2.0/24 area 0
!
[snip]
Now the problems:
1 - The passive-interface command is not working at all. *ospfd* is
forming adjacencies normally.
2 - Both passive-interface default and passive-interface IFNAME are not
appearing in the running configuration. And you removed the logic to prefix
a no before passive-interface IFNAME depending whether passive-interface
default is configured or not.
3 - New network statements were introduced in the running configuration.
Thinking from the perspective of the northbound API, my opinion is that a
daemon shouldn't change its running configuration internally. sh run
should reflect exactly what the operator configured.
To reinforce my last point, imagine someone configuring ospfd like this:
rt2-ospfd(config)# router ospf
rt2-ospfd(config-router)# passive-interface default
rt2-ospfd(config-router)# network 0.0.0.0/0 area 0
rt2-ospfd(config-router)# no network 0.0.0.0/0 area 0
rt2-ospfd(config-router)# network 0.0.0.0/0 area 5
rt2-ospfd(config-router)#
rt2-ospfd(config-router)# do sh run
[snip]
!
router ospf
ospf router-id 2.2.2.2
network 0.0.0.0/0 area 5
network 2.2.2.2/32 area 0
network 10.0.1.0/24 area 0
network 10.0.2.0/24 area 0
!
[snip]
Now the operator has a problem to solve.. and it could be worse depending
on the number of interfaces. I think you should find a way to do what
you're doing "under the hood" without affecting the running configuration.
I'm not very familiar with ospfd code, but maybe you could ping
@chiragshah6 <https://github.com/chiragshah6> to get some pointers on
what would be the best way to solve this problem.
------------------------------
In ospfd/ospf_vty.c
<#2359 (comment)>:
> struct ospf_interface *oi;
+ struct in_addr area_id = {0};
+ struct prefix_ipv4 ipv4, *ip_ptr;
+
+ if (create == OSPF_IF_PASSIVE) {
+ /* create passive-interface from connected address */
+ ospf->passive_interface_default = OSPF_IF_PASSIVE;
This doesn't make sense. You're setting ospf->passive_interface_default
to OSPF_IF_PASSIVE here and setting it to OSPF_IF_ACTIVE a few lines
below. The problem is that a) no one is checking the value of this variable
and b) this function acts on interfaces but passive_interface_default is
tied to OSPF areas and not OSPF interfaces.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2359 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AWoqyQGj_pgGZb5Q_t0AT8fA71jH37leks5t8U7OgaJpZM4UYwpj>
.
|
|
Hi, Renato Westphal
1) I have push patches that fix previous issue on FRR
---------------------------------------------------------------------------------------------
commit baa28fb
Author: Fengtian Guo <fengtian.guo@6wind.com>
Date: Thu Jun 28 15:59:27 2018 +0800
ospfd: fix warning in passive-interface
Signed-off-by: Fengtian Guo <fengtian.guo@6wind.com>
commit dfd8128
Author: Fengtian Guo <fengtian.guo@6wind.com>
Date: Thu Jun 28 13:33:27 2018 +0800
ospfd: just update ospf passive-interface when exist
When passive-interface exist, no need to create network
of it, just up/down the interface
Signed-off-by: Fengtian Guo <fengtian.guo@6wind.com>
--------------------------------------------------------------------------------------------- 2) Apply the new patches, following is test result and I think it's the correct behavior
3 - New network statements were introduced in the running configuration. Thinking from the perspective of the northbound API, my opinion is that a daemon shouldn't change its running configuration internally. sh run should reflect exactly what the operator configured.
To reinforce my last point, imagine someone configuring ospfd like this:
router-vm# config t
router-vm(config)# router ospf
router-vm(config-router)# passive-interface default
router-vm(config-router)# exit
router-vm(config)# exit
router-vm# show running-config
Building configuration...
Current configuration:
!
frr version 5.0-dev
frr defaults traditional
hostname router-vm
log syslog
!
router ospf
passive-interface mgmt0 10.0.2.15
passive-interface ntfp1 10.200.0.2
passive-interface ntfp2 10.125.0.2
passive-interface ntfp3 10.175.0.2
network 10.0.2.0/24 area 0
network 10.125.0.0/24 area 0
network 10.175.0.0/24 area 0
network 10.200.0.0/24 area 0
!
line vty
!
end
-------------------------------------------------------------------------
router-vm# config t
router-vm(config)# router ospf
router-vm(config-router)# network 0.0.0.0/0 area 0
router-vm(config-router)# exit
router-vm(config)# router ospf
router-vm(config-router)# no ne
neighbor network
router-vm(config-router)# no ne
neighbor network
router-vm(config-router)# no network 0.0.0.0/0 area 0
router-vm(config-router)# network 0.0.0.0/0 area 5
router-vm(config-router)# exit
router-vm(config)# exit
router-vm# show running-config
Building configuration...
Current configuration:
!
frr version 5.0-dev
frr defaults traditional
hostname router-vm
log syslog
!
router ospf
network 0.0.0.0/0 area 5
network 10.0.2.0/24 area 0
network 10.125.0.0/24 area 0
network 10.175.0.0/24 area 0
network 10.200.0.0/24 area 0
!
line vty
!
end
…------------------ Original ------------------
From: "Fengtian Guo"<fengtian.guo@6wind.com>;
Date: Sat, Jun 16, 2018 10:30 AM
To: "FRRouting/frr"<reply@reply.github.com>;
Cc: "FRRouting/frr"<frr@noreply.github.com>; "Mention"<mention@noreply.github.com>;
Subject: Re: [FRRouting/frr] ospfd: implement passive-interface function ofcisco (#2359)
Renato Westphal
Thanks for acknowledge
This week I have lot work to finish. And I will give a new patch for this new issue
Thanks
On Thu, Jun 14, 2018 at 1:41 AM, Renato Westphal <notifications@github.com> wrote:
@rwestphal requested changes on this pull request.
@fengtian6WIND Thanks for the updates. I've found a few problems with this PR.
Testing ospfd I got this:
rt2-ospfd# show running-config [snip] ! router ospf ospf router-id 2.2.2.2 network 0.0.0.0/0 area 0 ! [snip] rt2-ospfd# rt2-ospfd# conf t rt2-ospfd(config)# router ospf rt2-ospfd(config-router)# passive-interface rt2-eth0 rt2-ospfd(config-router)# do sh run [snip] ! router ospf ospf router-id 2.2.2.2 network 0.0.0.0/0 area 0 ! [snip] rt2-ospfd(config-router)# passive-interface default rt2-ospfd(config-router)# rt2-ospfd(config-router)# rt2-ospfd(config-router)# do sh ip ospf nei Neighbor ID Pri State Dead Time Address Interface RXmtL RqstL DBsmL 1.1.1.1 1 Full/Backup 38.206s 10.0.1.1 rt2-eth0:10.0.1.2 0 0 0 3.3.3.3 1 Full/Backup 38.206s 10.0.2.3 rt2-eth1:10.0.2.2 0 0 0 4.4.4.4 1 Full/DR 38.206s 10.0.2.4 rt2-eth1:10.0.2.2 0 0 0 rt2-ospfd(config-router)# rt2-ospfd(config-router)# rt2-ospfd(config-router)# do sh run [snip] ! router ospf ospf router-id 2.2.2.2 network 0.0.0.0/0 area 0 network 2.2.2.2/32 area 0 network 10.0.1.0/24 area 0 network 10.0.2.0/24 area 0 ! [snip]
Now the problems:
1 - The passive-interface command is not working at all. ospfd is forming adjacencies normally.
2 - Both passive-interface default and passive-interface IFNAME are not appearing in the running configuration. And you removed the logic to prefix a no before passive-interface IFNAME depending whether passive-interface default is configured or not.
3 - New network statements were introduced in the running configuration. Thinking from the perspective of the northbound API, my opinion is that a daemon shouldn't change its running configuration internally. sh run should reflect exactly what the operator configured.
To reinforce my last point, imagine someone configuring ospfd like this:
rt2-ospfd(config)# router ospf rt2-ospfd(config-router)# passive-interface default rt2-ospfd(config-router)# network 0.0.0.0/0 area 0 rt2-ospfd(config-router)# no network 0.0.0.0/0 area 0 rt2-ospfd(config-router)# network 0.0.0.0/0 area 5 rt2-ospfd(config-router)# rt2-ospfd(config-router)# do sh run [snip] ! router ospf ospf router-id 2.2.2.2 network 0.0.0.0/0 area 5 network 2.2.2.2/32 area 0 network 10.0.1.0/24 area 0 network 10.0.2.0/24 area 0 ! [snip]
Now the operator has a problem to solve.. and it could be worse depending on the number of interfaces. I think you should find a way to do what you're doing "under the hood" without affecting the running configuration. I'm not very familiar with ospfd code, but maybe you could ping @chiragshah6 to get some pointers on what would be the best way to solve this problem.
In ospfd/ospf_vty.c:
struct ospf_interface *oi; + struct in_addr area_id = {0}; + struct prefix_ipv4 ipv4, *ip_ptr; + + if (create == OSPF_IF_PASSIVE) { + /* create passive-interface from connected address */ + ospf->passive_interface_default = OSPF_IF_PASSIVE;
This doesn't make sense. You're setting ospf->passive_interface_default to OSPF_IF_PASSIVE here and setting it to OSPF_IF_ACTIVE a few lines below. The problem is that a) no one is checking the value of this variable and b) this function acts on interfaces but passive_interface_default is tied to OSPF areas and not OSPF interfaces.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
There was a problem hiding this comment.
@fengtian6WIND Thanks the updates.
I see that the passive-interface command is now working again. However my other concern about ospfd changing its running configuration internally has not been addressed.
This is what I got in my test topology:
rt2-ospfd(config-router)# do sh run
[snip]
!
router ospf
ospf router-id 2.2.2.2
redistribute connected
!
[snip]
end
rt2-ospfd(config-router)#
rt2-ospfd(config-router)# passive-interface default
rt2-ospfd(config-router)# do sh run
[snip]
!
router ospf
ospf router-id 2.2.2.2
redistribute connected
passive-interface rt2-eth0 10.0.1.2
passive-interface rt2-eth1 10.0.2.2
passive-interface rt2-eth2 10.0.3.2
passive-interface rt2-lo1 2.2.2.2
network 2.2.2.2/32 area 0
!
[snip]
Those passive-interface rt2-xxx statements shouldn't have been added, sh run should show only what was configured explicitly by the operator. A daemon that changes its running configuration on its own would break the transactional configuration model we're trying to implement in FRR. Also, the passive-interface default command is not being displayed, which is wrong.
An additional problem is that new interfaces (e.g. tun interfaces) are not being put into the passive mode when passive-interface default is present. The passive-interface default command only converts the interfaces one time once it's executed. As I said before, I think you need to analyze the problem from a different perspective and restart from scratch. You need to change the internals of ospfd to do what you need to do, changing the ospfd configuration behind the users back is not the correct solution.
|
Hi @rwestphal
1) I give a patch fix the show running for passive-interface
commit 1c23688
Author: Fengtian Guo <fengtian.guo@6wind.com>
Date: Tue Jul 10 14:20:05 2018 +0800
ospf: show configure passive-interface default
2) > An additional problem is that new interfaces (e.g. tun interfaces) are not being put into the passive mode when passive-interface default is present. The passive-interface default command only converts the interfaces one time once it's executed. As I said before, I think you need to analyze the problem from a different perspective and restart from scratch. You need to change the internals of ospfd to do what you need to do, changing the ospfd configuration behind the users back is not the correct solution.
Sorry, I disagree to give solution for this issue. if need, we should create a new request for PR
The new interface can't be processed after ospfd, zebra has started and it read all interface by api and add them into global list
This problem should notify to users.
And if we insist add new function for adding new interface at runtime, we should first provide a new command which provide the
basic function for add new interface into list (&vrf->ifaces_by_name), and after the function, run ospf-interface default
…------------------ Original ------------------
From: "Renato Westphal"<notifications@github.com>;
Date: Tue, Jul 10, 2018 07:28 AM
To: "FRRouting/frr"<frr@noreply.github.com>;
Cc: "fengtian6WIND"<fengtian.guo@6wind.com>; "Mention"<mention@noreply.github.com>;
Subject: Re: [FRRouting/frr] ospfd: implement passive-interface function ofcisco (#2359)
@rwestphal requested changes on this pull request.
@fengtian6WIND Thanks the updates.
I see that the passive-interface command is now working again. However my other concern about ospfd changing its running configuration internally has not been addressed.
This is what I got in my test topology:
rt2-ospfd(config-router)# do sh run [snip] ! router ospf ospf router-id 2.2.2.2 redistribute connected ! [snip] end rt2-ospfd(config-router)# rt2-ospfd(config-router)# passive-interface default rt2-ospfd(config-router)# do sh run [snip] ! router ospf ospf router-id 2.2.2.2 redistribute connected passive-interface rt2-eth0 10.0.1.2 passive-interface rt2-eth1 10.0.2.2 passive-interface rt2-eth2 10.0.3.2 passive-interface rt2-lo1 2.2.2.2 network 2.2.2.2/32 area 0 ! [snip]
Those passive-interface rt2-xxx statements shouldn't have been added, sh run should show only what was configured explicitly by the operator. A daemon that changes its running configuration on its own would break the transactional configuration model we're trying to implement in FRR. Also, the passive-interface default command is not being displayed, which is wrong.
An additional problem is that new interfaces (e.g. tun interfaces) are not being put into the passive mode when passive-interface default is present. The passive-interface default command only converts the interfaces one time once it's executed. As I said before, I think you need to analyze the problem from a different perspective and restart from scratch. You need to change the internals of ospfd to do what you need to do, changing the ospfd configuration behind the users back is not the correct solution.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@fengtian6WIND Adding/deleting interfaces during runtime is a normal operation that is already supported by FRR. When a new interface is added, zebra detects it and notifies all clients about it, which process the message using the |
|
------------------ Original ------------------
From: "Renato Westphal"<notifications@github.com>;
Date: Thu, Jul 12, 2018 10:45 AM
To: "FRRouting/frr"<frr@noreply.github.com>;
Cc: "fengtian6WIND"<fengtian.guo@6wind.com>; "Mention"<mention@noreply.github.com>;
Subject: Re: [FRRouting/frr] ospfd: implement passive-interface function ofcisco (#2359)
@fengtian6WIND Adding/deleting interfaces during runtime is a normal operation that is already supported by FRR. When a new interface is added, zebra detects it and notifies all clients about it, which process the message using the interface_add zclient callback. Your PR is introducing a regression because the ospfd passive-interface default command works for new interfaces on master whereas it doesn't work on your branch. There's also the problem about ospfd changing its running configuration internally, which is a much bigger issue that also needs to be resolved.
1) I think the issue of changing ospf running configuration internally is caused by ospf->passive_interface_default in function add_ospf_interface()
And I push a new patch for fix issue when adding new interface for ospfd
2) there are compiling error which is not caused by my patch
CC ospfd/ospf_vty.o ospfd/ospf_vty.c:2417: error: expected declaration specifiers or ... before string constant ospfd/ospf_vty.c:2419: warning: return type defaults to int ospfd/ospf_vty.c:2417: warning: no previous prototype for CPP_NOTICE ospfd/ospf_vty.c: In function CPP_NOTICE: ospfd/ospf_vty.c:2419: error: storage class specified for parameter ospf_timers_lsa_arrival_cmd ospfd/ospf_vty.c:2419: error: parameter ospf_timers_lsa_arrival_cmd is initialized ospfd/ospf_vty.c:2424: error: expected declaration specifiers before ; token
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Renato WestphalNow I rebase fguo branch on frr/master and fix compile issue
------------------ Original ------------------
From: "Renato Westphal"<notifications@github.com>;
Date: Thu, Jul 12, 2018 10:45 AM
To: "FRRouting/frr"<frr@noreply.github.com>;
Cc: "fengtian6WIND"<fengtian.guo@6wind.com>; "Mention"<mention@noreply.github.com>;
Subject: Re: [FRRouting/frr] ospfd: implement passive-interface function ofcisco (#2359)
@fengtian6WIND Adding/deleting interfaces during runtime is a normal operation that is already supported by FRR. When a new interface is added, zebra detects it and notifies all clients about it, which process the message using the interface_add zclient callback. Your PR is introducing a regression because the ospfd passive-interface default command works for new interfaces on master whereas it doesn't work on your branch. There's also the problem about ospfd changing its running configuration internally, which is a much bigger issue that also needs to be resolved.
1) I think the issue of changing ospf running configuration internally is caused by ospf->passive_interface_default in function add_ospf_interface()
And I push a new patch for fix issue when adding new interface for ospfd
2) there are compiling error which is not caused by my patch
CC ospfd/ospf_vty.o ospfd/ospf_vty.c:2417: error: expected declaration specifiers or ... before string constant ospfd/ospf_vty.c:2419: warning: return type defaults to int ospfd/ospf_vty.c:2417: warning: no previous prototype for CPP_NOTICE ospfd/ospf_vty.c: In function CPP_NOTICE: ospfd/ospf_vty.c:2419: error: storage class specified for parameter ospf_timers_lsa_arrival_cmd ospfd/ospf_vty.c:2419: error: parameter ospf_timers_lsa_arrival_cmd is initialized ospfd/ospf_vty.c:2424: error: expected declaration specifiers before ; token
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Hi @fengtian6WIND, I just tested your latest updates and I noticed a good progress, but there are still a few problems:
I didn't look at the code this time but it seems you're in the right direction. And please squash all your commits into one. |
ospfd/ospf_interface.h
Outdated
| passive: no sending or | ||
| receiving (no need to | ||
| join multicast groups) | ||
| */ | ||
| DECLARE_IF_PARAM(uint8_t, priority); /* OSPF Interface priority */ |
There was a problem hiding this comment.
I disagree with taking one interface configuration option and pulling it out of the ospf_if_params struct while everything else is still in ospf_if_params. If you think the ospf_if_params struct is a bad idea in general, we should talk about that and we can change it for everything.
But either way it's not related to this patch and I don't see why it should be mixed in here.
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an EXPERIMENTAL automated CI system. Get source and apply patch from patchwork: SuccessfulBuilding Stage: FailedOpenBSD60 amd64 build: Successful Ubuntu1604 amd64 build: FailedPackage building failed for Ubuntu1604 amd64 build: Ubuntu1604 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5080/artifact/CI014BUILD/config.status/config.status Ubuntu 16.04 i386: FailedPackage building failed for Ubuntu 16.04 i386: Ubuntu 16.04 i386: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5080/artifact/U1604I386/config.status/config.status Ubuntu 18.04 amd64 build: FailedPackage building failed for Ubuntu 18.04 amd64 build: Ubuntu 18.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5080/artifact/U1804AMD64/config.status/config.status Debian8 amd64 build: FailedPackage building failed for Debian8 amd64 build: Debian8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5080/artifact/CI008BLD/config.status/config.status Debian9 amd64 build: FailedPackage building failed for Debian9 amd64 build: Debian9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5080/artifact/CI021BUILD/config.status/config.status Warnings Generated during build:Checkout code: Successful with additional warnings:Ubuntu1604 amd64 build: FailedPackage building failed for Ubuntu1604 amd64 build: Ubuntu1604 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5080/artifact/CI014BUILD/config.status/config.status Ubuntu 16.04 i386: FailedPackage building failed for Ubuntu 16.04 i386: Ubuntu 16.04 i386: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5080/artifact/U1604I386/config.status/config.status Ubuntu 18.04 amd64 build: FailedPackage building failed for Ubuntu 18.04 amd64 build: Ubuntu 18.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5080/artifact/U1804AMD64/config.status/config.status Debian8 amd64 build: FailedPackage building failed for Debian8 amd64 build: Debian8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5080/artifact/CI008BLD/config.status/config.status Debian9 amd64 build: FailedPackage building failed for Debian9 amd64 build: Debian9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5080/artifact/CI021BUILD/config.status/config.status |
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5081/ This is a comment from an EXPERIMENTAL automated CI system. CLANG Static Analyzer Summary
Fixed warnings:
New warnings:
Static Analysis warning summary compared to base:
4 Static Analyzer issues remaining.See details at |
|
Hi @rwestphal I give a patch for fix the issue and you can check it |
"passive-interface default/interface [IPv4]" will automatic add interface into ospf but not active it for adjacency and futher action "no passive-interface default/interface [IPv4]" will active the passive interface for adjacency and futher Signed-off-by: Fengtian Guo <fengtian.guo@6wind.com>
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5189/ This is a comment from an EXPERIMENTAL automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings:CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base4 Static Analyzer issues remaining.See details at |
Signed-off-by: Fengtian Guo <fengtian.guo@6wind.com>
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5194/ This is a comment from an EXPERIMENTAL automated CI system. |
|
There is no person review this patch ? |
|
Hi @fengtian6WIND - sorry that this is taking very long, but the behaviour and change are very "non-obvious"; it's hard to correctly understand what is going on. I've looked at Cisco docs for passive interface, and as far as I understand, in Cisco a passive interface is not automatically added to any area. So the |
|
Hi, eqvinox
|
|
@fengtian6WIND I've looked at that page and it still has the network statements for passive interfaces: You can see that:
For me it does not make any sense to have |
eqvinox
added
the
submitter action required
note that this pr has been opened to have same behaviour of cisco ( #2065 ), that is to say having passive-interface only - no network command) |
|
ACK, this PR needs to be changed to match Cisco behaviour. |
I just checked the IETF and OpenConfig OSPF models and found this:
leaf passive {
type boolean;
description
"Enable/Disable passive interface - a passive interface's
prefix will be advertised but no neighbor adjacencies
will be formed on the interface.";
}
leaf passive {
type boolean;
description
"When this leaf is set to true, the interface should be
advertised within the OSPF area but OSPF adjacencies should
not be established over the interface";
}In other words, what we're calling the Cisco behavior is also the behavior specified in the IETF/OC YANG models, so I think we should go for it. We should only fix the problem about implicit |
|
So, now the conclusion |
The implicit network statements need to be removed completely, not just in Neither Cisco nor the YANG models have an implicit |
|
closing because will be worked on later |
"passive-interface default/interface [IPv4]" will automatic
add interface into ospf but not active it for adjacency and
futher action
"no passive-interface default/interface [IPv4]" will active
the passive interface for adjacency and futher