Path monitoring daemon #6424
Path monitoring daemon #6424
Conversation
this path monitoring daemon is instantiated, and is available like other daemons. This daemon permits to create per session context. Sessions indexation is with peer ip address. optionally it is possible to configure the source ip address and the interface. The sessions can also be configured with the frequency, timeout. That single commit permits to create pm sessions in configuration, but not operational contexts. Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
pm session can trigger periodic emission of icmp echo packets. a pm echo session is then created, and on each periodic emission, a socket is allocated, and socket operations are performed at that time. Note that among the socket operations, the option to forge the whole IPv4 header packet has been chosen. This permits having more flexibility about how we want to send packet ( especially where, because we can have some use cases without routing tables available, and however send packets with an unknown destination ip - chosen by admin -, to a known gateway. Also, about forging the packet, either the source IP is given by the configuration, or the source IP will be selected from the local VRF hosting the pm session. Note that this piece of code has not the IPv6 support yet. Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
This piece of commit implements support for ipv6 emission of icmp echo packets. Like for IPv4, the IPv6 socket options used permit to forge the packet, thus enabling flexibility on where packet should be sent to, and how packet should be forged. Note however, that the IPv6 code differs a bit with IPv4; main reason is that there is an issue that forbids using a SOCK_RAW ICMPV6 socket: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b9aa52c4cb457e7416cc0c95f475e72ef4a61336 This forces to use two sockets to handle separately the emission and the reception. Also some other differences in terms of behaviour did not permit to offer same level: - not possible to check for src IP received with recvfrom() API - not possible to use connect() operations with IPV6 raw socket too Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
the rtt service calculation permits to obtain the rtt according to 2 timestamp value. The return value is either expressed in timeval, or obtained in milliseconds Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
pmd will start only if nht is resolved. for now, nht will continue to work if nht fails to resolve nexthop. and a new state is defined to inform the user that pm session fails because nht fails. Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
it is possible to create entries based on vrfs. Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
a retry parameter permits to set session to down, only when a successive number of retries failed. Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
the path monitoring framework offers common api for daemons to access to services offered by path monitoring daemon. for that, it uses zebra server as proxy to relay messages. the path monitoring framework is very similar to the bfd framework done. Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
this api will store the last_alarm to either timeout or network unreachable, then will inform the remote daemon to down. Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
the changes should include the following: - add new vty command with <pm> keyword. ip route A.B.C.D/mm GW pm - upon nexthop reachability of GW, a new path monitoring session is requested to PM daemon. The exchange is between staticd and bfdd, and zebra daemon is the intermediate daemon to do the proxy server. Messages are mapped on the bfd messaging style. - the notification of reachability or unreachability can cancel the installation of a static route entry. consequence is that it will modify routing. Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
pmd daemon should be started before static daemon is alive. the reason is that some messages may be rerouted to pmd daemon, before pmd daemon is available. this is a problem, since the pm lib framework is not reliable. Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
nexthop tracking can be disabled in path monitoring daemon; this can be used in that way, because daemons that drive pmd already know about the nexthop reachability. Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
this setup uses staticd with path monitoring used for both ipv4 and ipv6 routes, in a vrf. Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
documentation explains how to use path monitoring daemon. Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
In addition to peer, add the nexthop ip to send packet to. this information can be used to pm daemon. For instance, if a nexthop to send traffic to is unreachable, having the nexthop information can be useful to forge layer 2 information. sendto() api will use that nexthop information. Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
There was a problem hiding this comment.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/e8061dbdfb3c307316f92e4772908d08/raw/3621643a17e91d77f14add4ba27d28f6ecd75715/cr_6424_1589815210.diff | git apply
diff --git a/lib/command.h b/lib/command.h
index d23d847a7..f499ce73b 100644
--- a/lib/command.h
+++ b/lib/command.h
@@ -81,86 +81,86 @@ struct host {
/* List of CLI nodes. Please remember to update the name array in command.c. */
enum node_type {
- AUTH_NODE, /* Authentication mode of vty interface. */
- VIEW_NODE, /* View node. Default mode of vty interface. */
- AUTH_ENABLE_NODE, /* Authentication mode for change enable. */
- ENABLE_NODE, /* Enable node. */
- CONFIG_NODE, /* Config node. Default mode of config file. */
- DEBUG_NODE, /* Debug node. */
- VRF_DEBUG_NODE, /* Vrf Debug node. */
- NORTHBOUND_DEBUG_NODE, /* Northbound Debug node. */
- DEBUG_VNC_NODE, /* Debug VNC node. */
- RMAP_DEBUG_NODE, /* Route-map debug node */
- RESOLVER_DEBUG_NODE, /* Resolver debug node */
- AAA_NODE, /* AAA node. */
- KEYCHAIN_NODE, /* Key-chain node. */
- KEYCHAIN_KEY_NODE, /* Key-chain key node. */
- IP_NODE, /* Static ip route node. */
- VRF_NODE, /* VRF mode node. */
- INTERFACE_NODE, /* Interface mode node. */
- NH_GROUP_NODE, /* Nexthop-Group mode node. */
- ZEBRA_NODE, /* zebra connection node. */
- TABLE_NODE, /* rtm_table selection node. */
- RIP_NODE, /* RIP protocol mode node. */
- RIPNG_NODE, /* RIPng protocol mode node. */
- BABEL_NODE, /* BABEL protocol mode node. */
- EIGRP_NODE, /* EIGRP protocol mode node. */
- BGP_NODE, /* BGP protocol mode which includes BGP4+ */
- BGP_VPNV4_NODE, /* BGP MPLS-VPN PE exchange. */
- BGP_VPNV6_NODE, /* BGP MPLS-VPN PE exchange. */
- BGP_IPV4_NODE, /* BGP IPv4 unicast address family. */
- BGP_IPV4M_NODE, /* BGP IPv4 multicast address family. */
- BGP_IPV4L_NODE, /* BGP IPv4 labeled unicast address family. */
- BGP_IPV6_NODE, /* BGP IPv6 address family */
- BGP_IPV6M_NODE, /* BGP IPv6 multicast address family. */
- BGP_IPV6L_NODE, /* BGP IPv6 labeled unicast address family. */
- BGP_VRF_POLICY_NODE, /* BGP VRF policy */
- BGP_VNC_DEFAULTS_NODE, /* BGP VNC nve defaults */
- BGP_VNC_NVE_GROUP_NODE, /* BGP VNC nve group */
- BGP_VNC_L2_GROUP_NODE, /* BGP VNC L2 group */
- RFP_DEFAULTS_NODE, /* RFP defaults node */
- BGP_EVPN_NODE, /* BGP EVPN node. */
- OSPF_NODE, /* OSPF protocol mode */
- OSPF6_NODE, /* OSPF protocol for IPv6 mode */
- LDP_NODE, /* LDP protocol mode */
- LDP_IPV4_NODE, /* LDP IPv4 address family */
- LDP_IPV6_NODE, /* LDP IPv6 address family */
- LDP_IPV4_IFACE_NODE, /* LDP IPv4 Interface */
- LDP_IPV6_IFACE_NODE, /* LDP IPv6 Interface */
- LDP_L2VPN_NODE, /* LDP L2VPN node */
- LDP_PSEUDOWIRE_NODE, /* LDP Pseudowire node */
- ISIS_NODE, /* ISIS protocol mode */
- ACCESS_NODE, /* Access list node. */
- PREFIX_NODE, /* Prefix list node. */
- ACCESS_IPV6_NODE, /* Access list node. */
- ACCESS_MAC_NODE, /* MAC access list node*/
- PREFIX_IPV6_NODE, /* Prefix list node. */
- AS_LIST_NODE, /* AS list node. */
- COMMUNITY_LIST_NODE, /* Community list node. */
- RMAP_NODE, /* Route map node. */
- PBRMAP_NODE, /* PBR map node. */
- SMUX_NODE, /* SNMP configuration node. */
- DUMP_NODE, /* Packet dump node. */
- FORWARDING_NODE, /* IP forwarding node. */
- PROTOCOL_NODE, /* protocol filtering node */
- MPLS_NODE, /* MPLS config node */
- PW_NODE, /* Pseudowire config node */
- VTY_NODE, /* Vty node. */
- FPM_NODE, /* Dataplane FPM node. */
- LINK_PARAMS_NODE, /* Link-parameters node */
- BGP_EVPN_VNI_NODE, /* BGP EVPN VNI */
- RPKI_NODE, /* RPKI node for configuration of RPKI cache server
- connections.*/
- BGP_FLOWSPECV4_NODE, /* BGP IPv4 FLOWSPEC Address-Family */
- BGP_FLOWSPECV6_NODE, /* BGP IPv6 FLOWSPEC Address-Family */
- BFD_NODE, /* BFD protocol mode. */
- BFD_PEER_NODE, /* BFD peer configuration mode. */
- OPENFABRIC_NODE, /* OpenFabric router configuration node */
- VRRP_NODE, /* VRRP node */
- BMP_NODE, /* BMP config under router bgp */
- PM_NODE, /* PM protocol mode. */
- PM_SESSION_NODE, /* PM peer configuration mode. */
- NODE_TYPE_MAX, /* maximum */
+ AUTH_NODE, /* Authentication mode of vty interface. */
+ VIEW_NODE, /* View node. Default mode of vty interface. */
+ AUTH_ENABLE_NODE, /* Authentication mode for change enable. */
+ ENABLE_NODE, /* Enable node. */
+ CONFIG_NODE, /* Config node. Default mode of config file. */
+ DEBUG_NODE, /* Debug node. */
+ VRF_DEBUG_NODE, /* Vrf Debug node. */
+ NORTHBOUND_DEBUG_NODE, /* Northbound Debug node. */
+ DEBUG_VNC_NODE, /* Debug VNC node. */
+ RMAP_DEBUG_NODE, /* Route-map debug node */
+ RESOLVER_DEBUG_NODE, /* Resolver debug node */
+ AAA_NODE, /* AAA node. */
+ KEYCHAIN_NODE, /* Key-chain node. */
+ KEYCHAIN_KEY_NODE, /* Key-chain key node. */
+ IP_NODE, /* Static ip route node. */
+ VRF_NODE, /* VRF mode node. */
+ INTERFACE_NODE, /* Interface mode node. */
+ NH_GROUP_NODE, /* Nexthop-Group mode node. */
+ ZEBRA_NODE, /* zebra connection node. */
+ TABLE_NODE, /* rtm_table selection node. */
+ RIP_NODE, /* RIP protocol mode node. */
+ RIPNG_NODE, /* RIPng protocol mode node. */
+ BABEL_NODE, /* BABEL protocol mode node. */
+ EIGRP_NODE, /* EIGRP protocol mode node. */
+ BGP_NODE, /* BGP protocol mode which includes BGP4+ */
+ BGP_VPNV4_NODE, /* BGP MPLS-VPN PE exchange. */
+ BGP_VPNV6_NODE, /* BGP MPLS-VPN PE exchange. */
+ BGP_IPV4_NODE, /* BGP IPv4 unicast address family. */
+ BGP_IPV4M_NODE, /* BGP IPv4 multicast address family. */
+ BGP_IPV4L_NODE, /* BGP IPv4 labeled unicast address family. */
+ BGP_IPV6_NODE, /* BGP IPv6 address family */
+ BGP_IPV6M_NODE, /* BGP IPv6 multicast address family. */
+ BGP_IPV6L_NODE, /* BGP IPv6 labeled unicast address family. */
+ BGP_VRF_POLICY_NODE, /* BGP VRF policy */
+ BGP_VNC_DEFAULTS_NODE, /* BGP VNC nve defaults */
+ BGP_VNC_NVE_GROUP_NODE, /* BGP VNC nve group */
+ BGP_VNC_L2_GROUP_NODE, /* BGP VNC L2 group */
+ RFP_DEFAULTS_NODE, /* RFP defaults node */
+ BGP_EVPN_NODE, /* BGP EVPN node. */
+ OSPF_NODE, /* OSPF protocol mode */
+ OSPF6_NODE, /* OSPF protocol for IPv6 mode */
+ LDP_NODE, /* LDP protocol mode */
+ LDP_IPV4_NODE, /* LDP IPv4 address family */
+ LDP_IPV6_NODE, /* LDP IPv6 address family */
+ LDP_IPV4_IFACE_NODE, /* LDP IPv4 Interface */
+ LDP_IPV6_IFACE_NODE, /* LDP IPv6 Interface */
+ LDP_L2VPN_NODE, /* LDP L2VPN node */
+ LDP_PSEUDOWIRE_NODE, /* LDP Pseudowire node */
+ ISIS_NODE, /* ISIS protocol mode */
+ ACCESS_NODE, /* Access list node. */
+ PREFIX_NODE, /* Prefix list node. */
+ ACCESS_IPV6_NODE, /* Access list node. */
+ ACCESS_MAC_NODE, /* MAC access list node*/
+ PREFIX_IPV6_NODE, /* Prefix list node. */
+ AS_LIST_NODE, /* AS list node. */
+ COMMUNITY_LIST_NODE, /* Community list node. */
+ RMAP_NODE, /* Route map node. */
+ PBRMAP_NODE, /* PBR map node. */
+ SMUX_NODE, /* SNMP configuration node. */
+ DUMP_NODE, /* Packet dump node. */
+ FORWARDING_NODE, /* IP forwarding node. */
+ PROTOCOL_NODE, /* protocol filtering node */
+ MPLS_NODE, /* MPLS config node */
+ PW_NODE, /* Pseudowire config node */
+ VTY_NODE, /* Vty node. */
+ FPM_NODE, /* Dataplane FPM node. */
+ LINK_PARAMS_NODE, /* Link-parameters node */
+ BGP_EVPN_VNI_NODE, /* BGP EVPN VNI */
+ RPKI_NODE, /* RPKI node for configuration of RPKI cache server
+ connections.*/
+ BGP_FLOWSPECV4_NODE, /* BGP IPv4 FLOWSPEC Address-Family */
+ BGP_FLOWSPECV6_NODE, /* BGP IPv6 FLOWSPEC Address-Family */
+ BFD_NODE, /* BFD protocol mode. */
+ BFD_PEER_NODE, /* BFD peer configuration mode. */
+ OPENFABRIC_NODE, /* OpenFabric router configuration node */
+ VRRP_NODE, /* VRRP node */
+ BMP_NODE, /* BMP config under router bgp */
+ PM_NODE, /* PM protocol mode. */
+ PM_SESSION_NODE, /* PM peer configuration mode. */
+ NODE_TYPE_MAX, /* maximum */
};
extern vector cmdvec;
diff --git a/lib/zclient.c b/lib/zclient.c
index 1d2684fe6..a2b92a417 100644
--- a/lib/zclient.c
+++ b/lib/zclient.c
@@ -3350,7 +3350,7 @@ static int zclient_read(struct thread *thread)
case ZEBRA_INTERFACE_PM_DEST_UPDATE:
if (zclient->interface_pm_dest_update)
(*zclient->interface_pm_dest_update)(command, zclient,
- length, vrf_id);
+ length, vrf_id);
break;
default:
break;
diff --git a/staticd/static_nht.c b/staticd/static_nht.c
index 2c84e90d6..3a5af9b9d 100644
--- a/staticd/static_nht.c
+++ b/staticd/static_nht.c
@@ -59,7 +59,8 @@ static void static_nht_update_rn(struct route_node *rn,
si->nh_valid = !!nh_num;
if (si->state == STATIC_START) {
- ret = static_zebra_route_add(rn, si, vrf->vrf_id, safi, true);
+ ret = static_zebra_route_add(rn, si, vrf->vrf_id, safi,
+ true);
if (ret)
static_pm_update_si(si, true);
else
diff --git a/staticd/static_routes.c b/staticd/static_routes.c
index 8e6d4161a..a46cdf8ac 100644
--- a/staticd/static_routes.c
+++ b/staticd/static_routes.c
@@ -61,9 +61,11 @@ static void static_uninstall_route(vrf_id_t vrf_id, safi_t safi,
int ret;
if (rn->info)
- ret = static_zebra_route_add(rn, si_changed, vrf_id, safi, true);
+ ret = static_zebra_route_add(rn, si_changed, vrf_id, safi,
+ true);
else
- ret = static_zebra_route_add(rn, si_changed, vrf_id, safi, false);
+ ret = static_zebra_route_add(rn, si_changed, vrf_id, safi,
+ false);
static_pm_update_si(si_changed, ret);
}
diff --git a/staticd/static_vty.c b/staticd/static_vty.c
index fcd1fe74d..325c65e2a 100644
--- a/staticd/static_vty.c
+++ b/staticd/static_vty.c
@@ -263,13 +263,14 @@ static int zebra_static_route_holdem(
return CMD_SUCCESS;
}
-static int static_route_leak(
- struct vty *vty, struct static_vrf *svrf, struct static_vrf *nh_svrf,
- afi_t afi, safi_t safi, const char *negate, const char *dest_str,
- const char *mask_str, const char *src_str, const char *gate_str,
- const char *ifname, const char *flag_str, const char *tag_str,
- const char *distance_str, const char *label_str, const char *table_str,
- bool onlink, bool pm)
+static int static_route_leak(struct vty *vty, struct static_vrf *svrf,
+ struct static_vrf *nh_svrf, afi_t afi, safi_t safi,
+ const char *negate, const char *dest_str,
+ const char *mask_str, const char *src_str,
+ const char *gate_str, const char *ifname,
+ const char *flag_str, const char *tag_str,
+ const char *distance_str, const char *label_str,
+ const char *table_str, bool onlink, bool pm)
{
int ret;
uint8_t distance;
@@ -665,7 +666,7 @@ int static_config(struct vty *vty, struct static_vrf *svrf, afi_t afi,
vty_out(vty, "nexthop-vrf %s ", shr->nhvrf_name);
if (shr->onlink)
vty_out(vty, "onlink");
- vty_out(vty, "onlink ");
+ vty_out(vty, "onlink ");
if (shr->pm)
vty_out(vty, "pm ");
vty_out(vty, "\n");
@@ -849,12 +850,11 @@ DEFPY(ip_route_blackhole_vrf,
assert(prefix);
return static_route_leak(vty, svrf, svrf, AFI_IP, SAFI_UNICAST, no,
prefix, mask_str, NULL, NULL, NULL, flag,
- tag_str, distance_str, label, table_str,
- false, false);
+ tag_str, distance_str, label, table_str, false,
+ false);
}
-DEFPY(ip_route_address_interface,
- ip_route_address_interface_cmd,
+DEFPY(ip_route_address_interface, ip_route_address_interface_cmd,
"[no] ip route\
<A.B.C.D/M$prefix|A.B.C.D$prefix A.B.C.D$mask> \
A.B.C.D$gate \
@@ -879,12 +879,9 @@ DEFPY(ip_route_address_interface,
"Null interface\n"
"Set tag for this route\n"
"Tag value\n"
- "Distance value for this route\n"
- VRF_CMD_HELP_STR
- MPLS_LABEL_HELPSTR
+ "Distance value for this route\n" VRF_CMD_HELP_STR MPLS_LABEL_HELPSTR
"Table to configure\n"
- "The table number to configure\n"
- VRF_CMD_HELP_STR
+ "The table number to configure\n" VRF_CMD_HELP_STR
"Treat the nexthop as directly attached to the interface\n"
"Enables Path Monitoring support\n")
{
@@ -925,8 +922,7 @@ DEFPY(ip_route_address_interface,
!!onlink, !!pm);
}
-DEFPY(ip_route_address_interface_vrf,
- ip_route_address_interface_vrf_cmd,
+DEFPY(ip_route_address_interface_vrf, ip_route_address_interface_vrf_cmd,
"[no] ip route\
<A.B.C.D/M$prefix|A.B.C.D$prefix A.B.C.D$mask> \
A.B.C.D$gate \
@@ -950,14 +946,12 @@ DEFPY(ip_route_address_interface_vrf,
"Null interface\n"
"Set tag for this route\n"
"Tag value\n"
- "Distance value for this route\n"
- MPLS_LABEL_HELPSTR
+ "Distance value for this route\n" MPLS_LABEL_HELPSTR
"Table to configure\n"
- "The table number to configure\n"
- VRF_CMD_HELP_STR
+ "The table number to configure\n" VRF_CMD_HELP_STR
"Treat the nexthop as directly attached to the interface\n"
"Enables Path Monitoring support\n")
- {
+{
VTY_DECLVAR_CONTEXT(vrf, vrf);
const char *flag = NULL;
struct static_vrf *svrf = vrf->info;
@@ -990,8 +984,7 @@ DEFPY(ip_route_address_interface_vrf,
!!onlink, !!pm);
}
-DEFPY(ip_route,
- ip_route_cmd,
+DEFPY(ip_route, ip_route_cmd,
"[no] ip route\
<A.B.C.D/M$prefix|A.B.C.D$prefix A.B.C.D$mask> \
<A.B.C.D$gate|<INTERFACE|Null0>$ifname> \
@@ -1014,12 +1007,9 @@ DEFPY(ip_route,
"Null interface\n"
"Set tag for this route\n"
"Tag value\n"
- "Distance value for this route\n"
- VRF_CMD_HELP_STR
- MPLS_LABEL_HELPSTR
+ "Distance value for this route\n" VRF_CMD_HELP_STR MPLS_LABEL_HELPSTR
"Table to configure\n"
- "The table number to configure\n"
- VRF_CMD_HELP_STR
+ "The table number to configure\n" VRF_CMD_HELP_STR
"Enables Path Monitoring support\n")
{
struct static_vrf *svrf;
@@ -1053,14 +1043,13 @@ DEFPY(ip_route,
return CMD_WARNING_CONFIG_FAILED;
}
- return static_route_leak(
- vty, svrf, nh_svrf, AFI_IP, SAFI_UNICAST, no, prefix, mask_str,
- NULL, gate_str, ifname, flag, tag_str, distance_str, label,
- table_str, false, !!pm);
+ return static_route_leak(vty, svrf, nh_svrf, AFI_IP, SAFI_UNICAST, no,
+ prefix, mask_str, NULL, gate_str, ifname, flag,
+ tag_str, distance_str, label, table_str, false,
+ !!pm);
}
-DEFPY(ip_route_vrf,
- ip_route_vrf_cmd,
+DEFPY(ip_route_vrf, ip_route_vrf_cmd,
"[no] ip route\
<A.B.C.D/M$prefix|A.B.C.D$prefix A.B.C.D$mask> \
<A.B.C.D$gate|<INTERFACE|Null0>$ifname> \
@@ -1082,11 +1071,9 @@ DEFPY(ip_route_vrf,
"Null interface\n"
"Set tag for this route\n"
"Tag value\n"
- "Distance value for this route\n"
- MPLS_LABEL_HELPSTR
+ "Distance value for this route\n" MPLS_LABEL_HELPSTR
"Table to configure\n"
- "The table number to configure\n"
- VRF_CMD_HELP_STR
+ "The table number to configure\n" VRF_CMD_HELP_STR
"Enables Path Monitoring support\n")
{
VTY_DECLVAR_CONTEXT(vrf, vrf);
@@ -1115,10 +1102,10 @@ DEFPY(ip_route_vrf,
return CMD_WARNING_CONFIG_FAILED;
}
- return static_route_leak(
- vty, svrf, nh_svrf, AFI_IP, SAFI_UNICAST, no, prefix, mask_str,
- NULL, gate_str, ifname, flag, tag_str, distance_str, label,
- table_str, false, !!pm);
+ return static_route_leak(vty, svrf, nh_svrf, AFI_IP, SAFI_UNICAST, no,
+ prefix, mask_str, NULL, gate_str, ifname, flag,
+ tag_str, distance_str, label, table_str, false,
+ !!pm);
}
DEFPY(ipv6_route_blackhole,
@@ -1199,14 +1186,13 @@ DEFPY(ipv6_route_blackhole_vrf,
* valid. Add an assert to make it happy
*/
assert(prefix);
- return static_route_leak(
- vty, svrf, svrf, AFI_IP6, SAFI_UNICAST, no, prefix_str, NULL,
- from_str, NULL, NULL, flag, tag_str, distance_str, label,
- table_str, false, false);
+ return static_route_leak(vty, svrf, svrf, AFI_IP6, SAFI_UNICAST, no,
+ prefix_str, NULL, from_str, NULL, NULL, flag,
+ tag_str, distance_str, label, table_str, false,
+ false);
}
-DEFPY(ipv6_route_address_interface,
- ipv6_route_address_interface_cmd,
+DEFPY(ipv6_route_address_interface, ipv6_route_address_interface_cmd,
"[no] ipv6 route X:X::X:X/M$prefix [from X:X::X:X/M] \
X:X::X:X$gate \
<INTERFACE|Null0>$ifname \
@@ -1220,8 +1206,7 @@ DEFPY(ipv6_route_address_interface,
|onlink$onlink \
|pm$pm \
}]",
- NO_STR
- IPV6_STR
+ NO_STR IPV6_STR
"Establish static routes\n"
"IPv6 destination prefix (e.g. 3ffe:506::/32)\n"
"IPv6 source-dest route\n"
@@ -1231,12 +1216,9 @@ DEFPY(ipv6_route_address_interface,
"Null interface\n"
"Set tag for this route\n"
"Tag value\n"
- "Distance value for this prefix\n"
- VRF_CMD_HELP_STR
- MPLS_LABEL_HELPSTR
+ "Distance value for this prefix\n" VRF_CMD_HELP_STR MPLS_LABEL_HELPSTR
"Table to configure\n"
- "The table number to configure\n"
- VRF_CMD_HELP_STR
+ "The table number to configure\n" VRF_CMD_HELP_STR
"Treat the nexthop as directly attached to the interface\n"
"Enables Path Monitoring support\n")
{
@@ -1271,14 +1253,13 @@ DEFPY(ipv6_route_address_interface,
ifname = NULL;
}
- return static_route_leak(
- vty, svrf, nh_svrf, AFI_IP6, SAFI_UNICAST, no, prefix_str, NULL,
- from_str, gate_str, ifname, flag, tag_str, distance_str, label,
- table_str, !!onlink, !!pm);
+ return static_route_leak(vty, svrf, nh_svrf, AFI_IP6, SAFI_UNICAST, no,
+ prefix_str, NULL, from_str, gate_str, ifname,
+ flag, tag_str, distance_str, label, table_str,
+ !!onlink, !!pm);
}
-DEFPY(ipv6_route_address_interface_vrf,
- ipv6_route_address_interface_vrf_cmd,
+DEFPY(ipv6_route_address_interface_vrf, ipv6_route_address_interface_vrf_cmd,
"[no] ipv6 route X:X::X:X/M$prefix [from X:X::X:X/M] \
X:X::X:X$gate \
<INTERFACE|Null0>$ifname \
@@ -1291,8 +1272,7 @@ DEFPY(ipv6_route_address_interface_vrf,
|onlink$onlink \
|pm$pm \
}]",
- NO_STR
- IPV6_STR
+ NO_STR IPV6_STR
"Establish static routes\n"
"IPv6 destination prefix (e.g. 3ffe:506::/32)\n"
"IPv6 source-dest route\n"
@@ -1302,11 +1282,9 @@ DEFPY(ipv6_route_address_interface_vrf,
"Null interface\n"
"Set tag for this route\n"
"Tag value\n"
- "Distance value for this prefix\n"
- MPLS_LABEL_HELPSTR
+ "Distance value for this prefix\n" MPLS_LABEL_HELPSTR
"Table to configure\n"
- "The table number to configure\n"
- VRF_CMD_HELP_STR
+ "The table number to configure\n" VRF_CMD_HELP_STR
"Treat the nexthop as directly attached to the interface\n"
"Enables Path Monitoring support\n")
{
@@ -1336,14 +1314,13 @@ DEFPY(ipv6_route_address_interface_vrf,
ifname = NULL;
}
- return static_route_leak(
- vty, svrf, nh_svrf, AFI_IP6, SAFI_UNICAST, no, prefix_str, NULL,
- from_str, gate_str, ifname, flag, tag_str, distance_str, label,
- table_str, !!onlink, !!pm);
+ return static_route_leak(vty, svrf, nh_svrf, AFI_IP6, SAFI_UNICAST, no,
+ prefix_str, NULL, from_str, gate_str, ifname,
+ flag, tag_str, distance_str, label, table_str,
+ !!onlink, !!pm);
}
-DEFPY(ipv6_route,
- ipv6_route_cmd,
+DEFPY(ipv6_route, ipv6_route_cmd,
"[no] ipv6 route X:X::X:X/M$prefix [from X:X::X:X/M] \
<X:X::X:X$gate|<INTERFACE|Null0>$ifname> \
[{ \
@@ -1355,8 +1332,7 @@ DEFPY(ipv6_route,
|nexthop-vrf NAME \
|pm$pm \
}]",
- NO_STR
- IPV6_STR
+ NO_STR IPV6_STR
"Establish static routes\n"
"IPv6 destination prefix (e.g. 3ffe:506::/32)\n"
"IPv6 source-dest route\n"
@@ -1366,12 +1342,9 @@ DEFPY(ipv6_route,
"Null interface\n"
"Set tag for this route\n"
"Tag value\n"
- "Distance value for this prefix\n"
- VRF_CMD_HELP_STR
- MPLS_LABEL_HELPSTR
+ "Distance value for this prefix\n" VRF_CMD_HELP_STR MPLS_LABEL_HELPSTR
"Table to configure\n"
- "The table number to configure\n"
- VRF_CMD_HELP_STR
+ "The table number to configure\n" VRF_CMD_HELP_STR
"Enables Path Monitoring support\n")
{
struct static_vrf *svrf;
@@ -1405,14 +1378,13 @@ DEFPY(ipv6_route,
ifname = NULL;
}
- return static_route_leak(
- vty, svrf, nh_svrf, AFI_IP6, SAFI_UNICAST, no, prefix_str, NULL,
- from_str, gate_str, ifname, flag, tag_str, distance_str, label,
- table_str, false, !!pm);
+ return static_route_leak(vty, svrf, nh_svrf, AFI_IP6, SAFI_UNICAST, no,
+ prefix_str, NULL, from_str, gate_str, ifname,
+ flag, tag_str, distance_str, label, table_str,
+ false, !!pm);
}
-DEFPY(ipv6_route_vrf,
- ipv6_route_vrf_cmd,
+DEFPY(ipv6_route_vrf, ipv6_route_vrf_cmd,
"[no] ipv6 route X:X::X:X/M$prefix [from X:X::X:X/M] \
<X:X::X:X$gate|<INTERFACE|Null0>$ifname> \
[{ \
@@ -1423,8 +1395,7 @@ DEFPY(ipv6_route_vrf,
|nexthop-vrf NAME \
|pm$pm \
}]",
- NO_STR
- IPV6_STR
+ NO_STR IPV6_STR
"Establish static routes\n"
"IPv6 destination prefix (e.g. 3ffe:506::/32)\n"
"IPv6 source-dest route\n"
@@ -1434,11 +1405,9 @@ DEFPY(ipv6_route_vrf,
"Null interface\n"
"Set tag for this route\n"
"Tag value\n"
- "Distance value for this prefix\n"
- MPLS_LABEL_HELPSTR
+ "Distance value for this prefix\n" MPLS_LABEL_HELPSTR
"Table to configure\n"
- "The table number to configure\n"
- VRF_CMD_HELP_STR
+ "The table number to configure\n" VRF_CMD_HELP_STR
"Enables Path Monitoring support\n")
{
VTY_DECLVAR_CONTEXT(vrf, vrf);
@@ -1467,10 +1436,10 @@ DEFPY(ipv6_route_vrf,
ifname = NULL;
}
- return static_route_leak(
- vty, svrf, nh_svrf, AFI_IP6, SAFI_UNICAST, no, prefix_str, NULL,
- from_str, gate_str, ifname, flag, tag_str, distance_str, label,
- table_str, false, !!pm);
+ return static_route_leak(vty, svrf, nh_svrf, AFI_IP6, SAFI_UNICAST, no,
+ prefix_str, NULL, from_str, gate_str, ifname,
+ flag, tag_str, distance_str, label, table_str,
+ false, !!pm);
}
DEFPY(debug_staticd,
debug_staticd_cmd,
diff --git a/staticd/static_zebra.c b/staticd/static_zebra.c
index 4bea7cbf2..2b1f60790 100644
--- a/staticd/static_zebra.c
+++ b/staticd/static_zebra.c
@@ -212,8 +212,7 @@ static int static_zebra_nexthop_update(ZAPI_CALLBACK_ARGS)
nhtd->nh_num = nhr.nexthop_num;
static_nht_reset_start(&nhr.prefix, afi, nhtd->nh_vrf_id);
- static_pm_update_connected(&nhr, &nhr.prefix,
- nhtd->nh_vrf_id);
+ static_pm_update_connected(&nhr, &nhr.prefix, nhtd->nh_vrf_id);
static_nht_update(NULL, &nhr.prefix, nhr.nexthop_num, afi,
nhtd->nh_vrf_id);
} else
diff --git a/vtysh/vtysh.c b/vtysh/vtysh.c
index 909d329d1..5ffd40622 100644
--- a/vtysh/vtysh.c
+++ b/vtysh/vtysh.c
@@ -1964,7 +1964,8 @@ DEFUNSH(VTYSH_BFDD, bfd_peer_enter, bfd_peer_enter_cmd,
}
#endif /* HAVE_BFDD */
-DEFUNSH(VTYSH_PMD, pm_enter, pm_enter_cmd, "pm", "Configure Path Monitoring sessions\n")
+DEFUNSH(VTYSH_PMD, pm_enter, pm_enter_cmd, "pm",
+ "Configure Path Monitoring sessions\n")
{
vty->node = PM_NODE;
return CMD_SUCCESS;
@@ -1983,11 +1984,9 @@ DEFUNSH(VTYSH_PMD, pm_enter, pm_enter_cmd, "pm", "Configure Path Monitoring sess
DEFUNSH(VTYSH_PMD, pm_session_enter, pm_session_enter_cmd,
"session <A.B.C.D|X:X::X:X> [{local-address <A.B.C.D|X:X::X:X>|interface IFNAME|vrf NAME}]",
- SESSION_STR SESSION_IPV4_STR SESSION_IPV6_STR
- LOCAL_STR LOCAL_IPV4_STR LOCAL_IPV6_STR
- INTERFACE_STR
- LOCAL_INTF_STR
- VRF_STR VRF_NAME_STR)
+ SESSION_STR SESSION_IPV4_STR SESSION_IPV6_STR LOCAL_STR LOCAL_IPV4_STR
+ LOCAL_IPV6_STR INTERFACE_STR LOCAL_INTF_STR VRF_STR
+ VRF_NAME_STR)
{
vty->node = PM_SESSION_NODE;
return CMD_SUCCESS;
diff --git a/vtysh/vtysh.h b/vtysh/vtysh.h
index 96805c853..214139b77 100644
--- a/vtysh/vtysh.h
+++ b/vtysh/vtysh.h
@@ -43,7 +43,7 @@ DECLARE_MGROUP(MVTYSH)
#define VTYSH_BFDD 0x10000
#define VTYSH_FABRICD 0x20000
#define VTYSH_VRRPD 0x40000
-#define VTYSH_PMD 0x80000
+#define VTYSH_PMD 0x80000
#define VTYSH_WAS_ACTIVE (-2)
@@ -52,7 +52,12 @@ DECLARE_MGROUP(MVTYSH)
/* watchfrr is not in ALL since library CLI functions should not be
* run on it (logging & co. should stay in a fixed/frozen config, and
* things like prefix lists are not even initialised) */
-#define VTYSH_ALL VTYSH_ZEBRA|VTYSH_RIPD|VTYSH_RIPNGD|VTYSH_OSPFD|VTYSH_OSPF6D|VTYSH_LDPD|VTYSH_BGPD|VTYSH_ISISD|VTYSH_PIMD|VTYSH_NHRPD|VTYSH_EIGRPD|VTYSH_BABELD|VTYSH_SHARPD|VTYSH_PBRD|VTYSH_STATICD|VTYSH_BFDD|VTYSH_FABRICD|VTYSH_VRRPD|VTYSH_PMD
+#define VTYSH_ALL \
+ VTYSH_ZEBRA | VTYSH_RIPD | VTYSH_RIPNGD | VTYSH_OSPFD | VTYSH_OSPF6D \
+ | VTYSH_LDPD | VTYSH_BGPD | VTYSH_ISISD | VTYSH_PIMD \
+ | VTYSH_NHRPD | VTYSH_EIGRPD | VTYSH_BABELD | VTYSH_SHARPD \
+ | VTYSH_PBRD | VTYSH_STATICD | VTYSH_BFDD | VTYSH_FABRICD \
+ | VTYSH_VRRPD | VTYSH_PMD
#define VTYSH_RMAP VTYSH_ZEBRA|VTYSH_RIPD|VTYSH_RIPNGD|VTYSH_OSPFD|VTYSH_OSPF6D|VTYSH_BGPD|VTYSH_ISISD|VTYSH_PIMD|VTYSH_EIGRPD|VTYSH_SHARPD|VTYSH_FABRICD
#define VTYSH_INTERFACE VTYSH_ZEBRA|VTYSH_RIPD|VTYSH_RIPNGD|VTYSH_OSPFD|VTYSH_OSPF6D|VTYSH_ISISD|VTYSH_PIMD|VTYSH_NHRPD|VTYSH_EIGRPD|VTYSH_BABELD|VTYSH_PBRD|VTYSH_FABRICD|VTYSH_VRRPD
#define VTYSH_VRF VTYSH_ZEBRA|VTYSH_PIMD|VTYSH_STATICD
If you are a new contributor to FRR, please see our contributing guidelines.
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedUbuntu 16.04 amd64 build: Failed (click for details)Make failed for Ubuntu 16.04 amd64 build: Ubuntu 16.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/CI014BUILD/config.status/config.status Ubuntu 18.04 amd64 build: Failed (click for details)Make 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-12325/artifact/U1804AMD64/config.status/config.status FreeBSD 12 amd64 build: Failed (click for details)Make failed for FreeBSD 12 amd64 build: FreeBSD 12 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/FBSD12AMD64/config.status/config.status Ubuntu 16.04 i386 build: Failed (click for details)Make failed for Ubuntu 16.04 i386 build: Ubuntu 16.04 i386 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/U1604I386/config.status/config.status Debian 8 amd64 build: Failed (click for details)Make failed for Debian 8 amd64 build: Debian 8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/CI008BLD/config.status/config.status FreeBSD 11 amd64 build: Failed (click for details)Make failed for FreeBSD 11 amd64 build: FreeBSD 11 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/CI009BUILD/config.status/config.status Ubuntu 18.04 ppc64le build: Failed (click for details)Make failed for Ubuntu 18.04 ppc64le build: Ubuntu 18.04 ppc64le build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/U1804PPC64LEBUILD/config.status/config.status CentOS 7 amd64 build: Failed (click for details)Package building failed for CentOS 7 amd64 build: CentOS 7 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/CI005BUILD/config.status/config.status Ubuntu 20.04 amd64 build: Failed (click for details)Make failed for Ubuntu 20.04 amd64 build: Ubuntu 20.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/U2004AMD64BUILD/config.status/config.status OpenBSD 6 amd64 build: Failed (click for details)Make failed for OpenBSD 6 amd64 build: OpenBSD 6 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/CI011BUILD/config.status/config.status NetBSD 8 amd64 build: Failed (click for details)Make failed for NetBSD 8 amd64 build: NetBSD 8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/CI012BUILD/config.status/config.status Debian 9 amd64 build: Failed (click for details)Make failed for Debian 9 amd64 build: Debian 9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/CI021BUILD/config.status/config.status Fedora 29 amd64 build: Failed (click for details)Make failed for Fedora 29 amd64 build: Fedora 29 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/F29BUILD/config.status/config.status Debian 10 amd64 build: Failed (click for details)Make failed for Debian 10 amd64 build: Debian 10 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/DEB10BUILD/config.status/config.status Warnings Generated during build:Checkout code: Successful with additional warningsUbuntu 16.04 amd64 build: Failed (click for details)Make failed for Ubuntu 16.04 amd64 build: Ubuntu 16.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/CI014BUILD/config.status/config.status Ubuntu 18.04 amd64 build: Failed (click for details)Make 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-12325/artifact/U1804AMD64/config.status/config.status FreeBSD 12 amd64 build: Failed (click for details)Make failed for FreeBSD 12 amd64 build: FreeBSD 12 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/FBSD12AMD64/config.status/config.status Ubuntu 16.04 i386 build: Failed (click for details)Make failed for Ubuntu 16.04 i386 build: Ubuntu 16.04 i386 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/U1604I386/config.status/config.status Debian 8 amd64 build: Failed (click for details)Make failed for Debian 8 amd64 build: Debian 8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/CI008BLD/config.status/config.status FreeBSD 11 amd64 build: Failed (click for details)Make failed for FreeBSD 11 amd64 build: FreeBSD 11 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/CI009BUILD/config.status/config.status Ubuntu 18.04 ppc64le build: Failed (click for details)Make failed for Ubuntu 18.04 ppc64le build: Ubuntu 18.04 ppc64le build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/U1804PPC64LEBUILD/config.status/config.status CentOS 7 amd64 build: Failed (click for details)Package building failed for CentOS 7 amd64 build: CentOS 7 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/CI005BUILD/config.status/config.status Ubuntu 20.04 amd64 build: Failed (click for details)Make failed for Ubuntu 20.04 amd64 build: Ubuntu 20.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/U2004AMD64BUILD/config.status/config.status OpenBSD 6 amd64 build: Failed (click for details)Make failed for OpenBSD 6 amd64 build: OpenBSD 6 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/CI011BUILD/config.status/config.status NetBSD 8 amd64 build: Failed (click for details)Make failed for NetBSD 8 amd64 build: NetBSD 8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/CI012BUILD/config.status/config.status Debian 9 amd64 build: Failed (click for details)Make failed for Debian 9 amd64 build: Debian 9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/CI021BUILD/config.status/config.status Fedora 29 amd64 build: Failed (click for details)Make failed for Fedora 29 amd64 build: Fedora 29 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/F29BUILD/config.status/config.status Debian 10 amd64 build: Failed (click for details)Make failed for Debian 10 amd64 build: Debian 10 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12325/artifact/DEB10BUILD/config.status/config.status |
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
| @@ -785,6 +785,7 @@ def __init__(self, name, **params): | |||
|
|
|||
| self.daemondir = None | |||
| self.hasmpls = False | |||
| <<<<<<< HEAD | |||
There was a problem hiding this comment.
conflict resolution left overs.
|
so ... would it make sense to evaluate modifying the bfd daemon to support an 'icmp' mode (and modified state machine), instead of a copy-paste approach like this? |
|
@mjstapp at least, from my point of view this code should be apart from BFD. I think BFD RFCs isn’t talking about ICMP mode, I think it’s very intrusive and aggresive to introduce this on BFD. |
|
so ... the issue isn't about the packets that are sent - it's about the frr system. is it worth 6K+ lines to introduce a copy of the bfd daemon, and a parallel set of zapi messages? or is it possible to imagine taking advantage of the daemon and zapi messaging we already have to do reachability detection using an additional kind of packet, using icmp packets? I don't think that's an "aggressive" question. some routing daemons may only care about reachability, in a practical sense, and may not care what kind of packets are used to make the determination.
|
|
Then, maybe creating a daemon it's being overkill. I think PMD it's introduced as suggestion on issue #6297. I consider overkill to fix those issue, but a nice feature to rely in a common way to implement new future functions. Maybe another approach it's create a common function for all routing daemons without be a daemon indeed. @pguibert6WIND can you explain some use cases for PMD? |
There was a problem hiding this comment.
Overall this new daemon seems like a very different thing from bfdd and I wouldn't recommend to merge with it. I can see this as useful for people who want to keep statistics about connection jitter / availability, however it belongs to a slightly different domain from BFD.
Code-wise I think we could improve it by sharing more code where possible with BFD or by changing strategy. Examples:
- The processes tracking code in
zebrais exactly the same forbfddandpmd! I think this could easily be generalized by just using different list head pointers. - The
lib/part could be rewritten to not use the same mistakes BFD integration did (see review comment). - The PID tracking code in
bfdd/ptm_adapter.ccould also be generalized (e.g. moved tolib) to reduce the line count.
Like @paunadeu said: the echo code could also be used by ospfd to fix the ping issue.
Perhaps if you could talk more about your daemon in next meeting it could help to convince more people, example:
- Show the data commands output with protocol details: whats the path mtu (is there such a thing?), connection jitter, etc...
- Log generated messages (maybe ICMP has some useful diagnostics that BFD doesn't have)
| /* | ||
| * pm_gbl_init - Initialize the PM global structure | ||
| */ | ||
| void pm_gbl_init(void) |
There was a problem hiding this comment.
Please don't use this code.
I don't mean to insult anyone but I think this could be better written, here are some points against it:
-
It assumes your daemon will keep feeding its integration protocols information (e.g. settings change, counters etc...). This is actually one problem we have with BFD because we expect it to work with
ptm-bfd(which has noshowcommands) andbfdd(see issue non default bfd timers not reflected in BGP #4485 ). -
The logic of settings management is a bit convoluted: it expect to own all your session settings in the protocol integration code (not your daemon). You'll have synchronization issues with your daemon later.
-
The message building code is awful because if you don't have a information it will omit from the packet, then you have to do all kinds of logic in your daemon to support readings messages from it.
Here is the BFD message format for reference:
/* * Register/Deregister/Update Message format: * - header: Command, VRF * - l: pid * - w: family * - AF_INET: * - l: destination ipv4 * - AF_INET6: * - 16 bytes: destination IPv6 * - command != ZEBRA_BFD_DEST_DEREGISTER * - l: min_rx * - l: min_tx * - c: detect multiplier * - c: is_multihop? * - multihop: * - w: family * - AF_INET: * - l: destination ipv4 * - AF_INET6: * - 16 bytes: destination IPv6 * - c: ttl * - no multihop * - AF_INET6: * - w: family * - 16 bytes: ipv6 address * - c: ifname length * - X bytes: interface name * - c: bfd_cbit * * q(64), l(32), w(16), c(8) */Example:
- If command is not
DEREGISTERthen the message has timers. - If you have a
single-hoppeer withIPv6then it expects asourceaddress. It could be made simplier by ALWAYS including the source address (even if blank).
- If command is not
| inet_ntop(AF_INET, &daddr, buf, sizeof(buf)); | ||
| inet_ntop(AF_INET, &pm->key.local.sin.sin_addr, | ||
| buf2, sizeof(buf2)); | ||
| zlog_err("PMD: wrong dst address %s, expected %s. retrying", |
There was a problem hiding this comment.
Better use FRR formatting here:
%pFX for struct prefix and %pI4/%pI6 for IPv4/IPv6 addresses.
More info:
http://docs.frrouting.org/projects/dev-guide/en/latest/logging.html#extensions
| #ifndef __PM_VTY_H__ | ||
| #define __PM_VTY_H__ | ||
|
|
||
| extern void pm_vty_init(void); |
There was a problem hiding this comment.
Could you move this function declaration to another header? I think it is a bit wasteful to have an entire header for a single function.
| /* | ||
| * Process PID registration. | ||
| */ | ||
| static struct pm_process *pp_new(pid_t pid, struct zserv *zs) |
There was a problem hiding this comment.
I think we could move this code somewhere else and use it for bfdd AND pm. This way you can get your line count down and make a lot of maintainers happy :) .
Maybe RTT, jitter, PMTU will be nice! It's very util have route tracked by PMTU. This is why I think this daemon could be very useful in a near future and shouldn't be merged with BFD! 😄 |
|
you seem to think that the only way to get a new capability or feature is to add an entirely new daemon (even if that means cut-and-pasting an existing daemon). I don't think that's necessarily true - I think it's worth figuring out whether a daemon that already does a lot of useful things could do a few more. that's exactly the point of my question: I'm wondering whether we could offer routing daemons a "path reachability" service and associated notifications, a service that could be somewhat more general. I suspect that many daemons only desire to know about loss of reachability in a way that is more responsive than their protocols' generic timers allow.
|
|
I 200% agree with @mjstapp. |
All those metrics can be gathered from the OS easily, I don't think this is worth. That's only my opinion. |
|
I was expecting this daemon to handle target/SLA registerations in a more generic fashion as compared to just tracking the exact next-hop of the static route. It could be that a service is required to be running on a target before installing route towards that. I posted similar question on slack for supporting SLA track and some of the suggestions were to use LUA, which may not be a right choice (imo). But that's a separate discussion. Few references: And the config could look like this: or, The SLA tracking object will be future-proof as it can be extended to support tracking of other objects/services as required. |
|
why is this required? Existing tools in OS not enough? OSPF need of external ping is only for required when OSPF running on P2P links. What are other protocols which are in need of this? |
|
|
||
| Interval between each emission. This value is the interval | ||
| in milliseconds between each emission of packet. Default | ||
| interval value is set to 5000 ms. |
There was a problem hiding this comment.
this is as aggressive as BFD is today. If everything remains same and encap is the only difference here then we better merge it with BFD to avoid code duplication. ICMP and BFD are services this process can offer to routing protocols.
| after emission, where one considers that the packet is not | ||
| received in time. That value can not be greater than | ||
| interval value. Default value is set to 5000 ms. | ||
|
|
There was a problem hiding this comment.
Internal default is 500ms and timeout is 500ms as well?
| However, it is possible to make more flexible the flip- | ||
| flap algorithm by increasing the retry timers. Those timers | ||
| permit to consider the session to go up or down, only after | ||
| a defined amount of received or non received retries. |
There was a problem hiding this comment.
default value for this? default value * default interval should be our default timeout.
| interface r1-eth0 | ||
| ip address 192.168.0.1/24 | ||
| ! | ||
| ip route 192.168.2.0/24 192.168.0.2 pm vrf r1-cust1 |
There was a problem hiding this comment.
if we are using PM to monitor nexthop of a static route then why not use BFD? What are the advantages of ICMP here? If packet size is the only deal here then https://tools.ietf.org/html/draft-ietf-bfd-large-packets-02 defines the same.
There was a problem hiding this comment.
Many services may not support BFD, e.g. load-balancers.
There was a problem hiding this comment.
My general comment is going to follow along the same lines as @mjstapp -- we shouldn't copy an entire set of code to create a new daemon, as this is going to cause a maintenance nightmare in the future. We need to sort how to get this capability using existing daemons.
|
Hi, I did not expect so many comments. This is a subject that is interesting. " I expect there is a consensus of having such service on frrouting. Now, what does contain the service. The idea behind that is
Today, that daemon has several concerns, and I don't intend to continue the pull request without a severe rework.
About path reachability service, I wonder if that service should not be supervised first at zebra level; this would avoid the proxy mechanism to put in place. To ease maintenance, bfd daemon could be reused, but I think zapi messaging should be improved; for instance, the message would contain the kind of probe to use ( bfd or icmp, or ...). To many that think this service can be externalised, I think having it centralised could ease the configuration. For instance, what if 2 daemons register to the same probe. |
|
I think this PR needs major rework and it would be more interesting to not having it polluting the PR list. I'll mark it for autoclose, but feel free to comment to avoid it. @polychaeta autoclose in 4 days (lets close it until Friday). |
|
@polychaeta autoclose in 4 days |
Code that introduces the ability to test the reachability of of remote peers with an ICMP ping.
The architecture is mapped over what has been done for BFD ( copy/paste)
so if someone has some new BFD architecture in its pocket, now, it is time to show it.