lib, zebra: Notice DAD Failed but don't use #6638
Conversation
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/16029428d9fbcdf8637c6fca4600edd9/raw/2e8be21419860bfdf33050f7e190536591b591ba/cr_6638_1593030696.diff | git apply
diff --git a/lib/if.h b/lib/if.h
index 5dc16abc2..ba7975389 100644
--- a/lib/if.h
+++ b/lib/if.h
@@ -411,7 +411,7 @@ struct connected {
#define ZEBRA_IFA_SECONDARY (1 << 0)
#define ZEBRA_IFA_PEER (1 << 1)
#define ZEBRA_IFA_UNNUMBERED (1 << 2)
-#define ZEBRA_IFA_DADFAILED (1 << 3)
+#define ZEBRA_IFA_DADFAILED (1 << 3)
/* N.B. the ZEBRA_IFA_PEER flag should be set if and only if
a peer address has been configured. If this flag is set,
the destination field must contain the peer address.
diff --git a/lib/zclient.h b/lib/zclient.h
index 9bfac453b..7149cbaaf 100644
--- a/lib/zclient.h
+++ b/lib/zclient.h
@@ -456,14 +456,14 @@ struct zapi_route {
* route entry. This mainly is used for backup static routes.
*/
#define ZEBRA_FLAG_RR_USE_DISTANCE 0x40
-/*
- * With IPv6 routes, there is a mechanism for DAD
- * when FRR detects this, we need to note to the rib
- * that it *exists* but there is a problem.
- * this is how this is done.
-#define ZEBRA_FLAG_NEVER_SELECT 0x80
+ /*
+ * With IPv6 routes, there is a mechanism for DAD
+ * when FRR detects this, we need to note to the rib
+ * that it *exists* but there is a problem.
+ * this is how this is done.
+ #define ZEBRA_FLAG_NEVER_SELECT 0x80
- /* The older XXX_MESSAGE flags live here */
+ /* The older XXX_MESSAGE flags live here */
uint8_t message;
/*
If you are a new contributor to FRR, please see our contributing guidelines.
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedUbuntu 16.04 arm8 build: Failed (click for details)Make failed for Ubuntu 16.04 arm8 build: 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-12855/artifact/F29BUILD/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-12855/artifact/CI009BUILD/config.status/config.status Ubuntu 16.04 arm7 build: Failed (click for details)Make failed for Ubuntu 16.04 arm7 build: 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-12855/artifact/FBSD12AMD64/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-12855/artifact/DEB10BUILD/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-12855/artifact/CI012BUILD/config.status/config.status Ubuntu 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-12855/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-12855/artifact/U1804AMD64/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-12855/artifact/U1604I386/config.status/config.status CentOS 7 amd64 build: Failed (click for details)Make 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-12855/artifact/CI005BUILD/config.status/config.status Ubuntu 18.04 arm7 build: Failed (click for details)Make failed for Ubuntu 18.04 arm7 build: 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-12855/artifact/CI008BLD/config.status/config.status Ubuntu 18.04 arm8 build: Failed (click for details)Make failed for Ubuntu 18.04 arm8 build: 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-12855/artifact/U1804PPC64LEBUILD/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-12855/artifact/CI021BUILD/config.status/config.status Ubuntu 20.04 amd64 build: Failed (click for details)Ubuntu 20.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12855/artifact/U2004AMD64BUILD/config.status/config.statusMake failed for Ubuntu 20.04 amd64 build: 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-12855/artifact/CI011BUILD/config.status/config.status |
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/49943a56615ee6ef7b159e90ddf624c0/raw/e466d8ab4fb43136600121fd7381f72f793194b5/cr_6638_1593032481.diff | git apply
diff --git a/lib/if.h b/lib/if.h
index 5dc16abc2..ba7975389 100644
--- a/lib/if.h
+++ b/lib/if.h
@@ -411,7 +411,7 @@ struct connected {
#define ZEBRA_IFA_SECONDARY (1 << 0)
#define ZEBRA_IFA_PEER (1 << 1)
#define ZEBRA_IFA_UNNUMBERED (1 << 2)
-#define ZEBRA_IFA_DADFAILED (1 << 3)
+#define ZEBRA_IFA_DADFAILED (1 << 3)
/* N.B. the ZEBRA_IFA_PEER flag should be set if and only if
a peer address has been configured. If this flag is set,
the destination field must contain the peer address.
diff --git a/lib/zclient.h b/lib/zclient.h
index 2f3d64051..129aa036d 100644
--- a/lib/zclient.h
+++ b/lib/zclient.h
@@ -462,7 +462,7 @@ struct zapi_route {
* that it *exists* but there is a problem.
* this is how this is done.
*/
-#define ZEBRA_FLAG_NEVER_SELECT 0x80
+#define ZEBRA_FLAG_NEVER_SELECT 0x80
/* The older XXX_MESSAGE flags live here */
uint8_t message;
If you are a new contributor to FRR, please see our contributing guidelines.
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopo tests part 0 un Ubuntu 16.04 arm8: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO0U16ARM8-12857/test Topology Tests failed for Topo tests part 0 un Ubuntu 16.04 arm8: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12857/artifact/TOPO0U16ARM8/ErrorLog/log_topotests.txt Successful on other platforms/tests
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
|
ci:rerun |
Continuous Integration Result: SUCCESSFULContinuous 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-12864/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
| @@ -1103,7 +1103,8 @@ static void rib_process(struct route_node *rn) | |||
| ROUTE_ENTRY_CHANGED); | |||
| new_fib = best; | |||
| } else { | |||
| best = rib_choose_best(new_selected, re); | |||
| if (!CHECK_FLAG(re->flags, ZEBRA_FLAG_NEVER_SELECT)) | |||
There was a problem hiding this comment.
would it be better to stick this check into rib_choose_best() itself? just thinking that that might encapsulate the decision
There was a problem hiding this comment.
I thought about that and decided I wanted it right where we use the ZEBRA_FLAG_FIB_OVERRIDE since this is the opposite of that one.
| @@ -411,6 +411,7 @@ struct connected { | |||
| #define ZEBRA_IFA_SECONDARY (1 << 0) | |||
| #define ZEBRA_IFA_PEER (1 << 1) | |||
| #define ZEBRA_IFA_UNNUMBERED (1 << 2) | |||
| #define ZEBRA_IFA_DADFAILED (1 << 3) | |||
There was a problem hiding this comment.
This might be functionally identical to / duplicate with !ZEBRA_IFC_REAL/ZEBRA_IFC_QUEUED. I think it might be better if we don't create a new flag for this and instead put a "unusable reason" in there separately.
There was a problem hiding this comment.
Btw: despite the name, ZEBRA_IFC_QUEUED is very much intended for this use; I remember that @cfra actually worked on this a few years back. It's named QUEUED since the primary use was the DAD delay after adding an address until it becomes usable, but that's functionally pretty much equivalent to DAD failure.
There was a problem hiding this comment.
QUEUED should match the TENTATIVE flag right? This is different right?
ipv6 addresses on interfaces can be notified to zebra that the kernel has noticed that a interface has a duplicate address on an interface. When this happens the kernel tells zebra about this situation. Prior to this change we were just silently dropping this and never creating a connected interface. With this change we now notice that a interface is in a DAD state, keep the connected interface as well as marking the created route as rejected. Hopefully we add enough bread crumbs to allow the end user to figure out what has gone wrong. From talking with kernel people you can modify kernel behavior for DAD routes via the accept_dad sysctl. Please see the kernel documenation for how it works. Additionally the `official` recovery methodology is to fix the address and then down/up the link. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.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/d771bde9a2e30ff3acfa8d7aff496cf2/raw/278f75a1144faa8e51a99071adba97d68578dd00/cr_6638_1595867568.diff | git apply
diff --git a/lib/if.h b/lib/if.h
index 3bf745f75..07f0fd213 100644
--- a/lib/if.h
+++ b/lib/if.h
@@ -411,7 +411,7 @@ struct connected {
#define ZEBRA_IFA_SECONDARY (1 << 0)
#define ZEBRA_IFA_PEER (1 << 1)
#define ZEBRA_IFA_UNNUMBERED (1 << 2)
-#define ZEBRA_IFA_DADFAILED (1 << 3)
+#define ZEBRA_IFA_DADFAILED (1 << 3)
/* N.B. the ZEBRA_IFA_PEER flag should be set if and only if
a peer address has been configured. If this flag is set,
the destination field must contain the peer address.
diff --git a/lib/zclient.h b/lib/zclient.h
index ef4ef459a..510428a16 100644
--- a/lib/zclient.h
+++ b/lib/zclient.h
@@ -463,7 +463,7 @@ struct zapi_route {
* that it *exists* but there is a problem.
* this is how this is done.
*/
-#define ZEBRA_FLAG_NEVER_SELECT 0x80
+#define ZEBRA_FLAG_NEVER_SELECT 0x80
/* The older XXX_MESSAGE flags live here */
uint8_t message;
diff --git a/zebra/kernel_socket.c b/zebra/kernel_socket.c
index 6553a7a81..a4221627a 100644
--- a/zebra/kernel_socket.c
+++ b/zebra/kernel_socket.c
@@ -923,11 +923,10 @@ int ifam_read(struct ifa_msghdr *ifam)
}
if (ifam->ifam_type == RTM_NEWADDR)
- connected_add_ipv6(ifp, flags, &addr.sin6.sin6_addr,
- NULL,
- ip6_masklen(mask.sin6.sin6_addr),
- (isalias ? ifname : NULL),
- METRIC_MAX, false);
+ connected_add_ipv6(
+ ifp, flags, &addr.sin6.sin6_addr, NULL,
+ ip6_masklen(mask.sin6.sin6_addr),
+ (isalias ? ifname : NULL), METRIC_MAX, false);
else
connected_delete_ipv6(ifp, &addr.sin6.sin6_addr, NULL,
ip6_masklen(mask.sin6.sin6_addr));
If you are a new contributor to FRR, please see our contributing guidelines.
💚 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-13325/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: |
| 0, 0, &p, NULL, &nh, 0, zvrf->table_id, metric, 0, 0, 0); | ||
| if (CHECK_FLAG(ifc->conf, ZEBRA_IFC_QUEUED) | ||
| && !CHECK_FLAG(ifc->conf, ZEBRA_IFC_REAL)) | ||
| flags |= ZEBRA_FLAG_NEVER_SELECT; |
There was a problem hiding this comment.
The route is independent from the address; DAD failure doesn't make the route unusable. If we do weird things with the route, we can get desynced between what FRR expects to happen & what actually happens in forwarding...
ipv6 addresses on interfaces can be notified to zebra
that the kernel has noticed that a interface has
a duplicate address on an interface.
When this happens the kernel tells zebra about this
situation. Prior to this change we were just silently
dropping this and never creating a connected interface.
With this change we now notice that a interface is in
a DAD state, keep the connected interface as well as
marking the created route as rejected. Hopefully we
add enough bread crumbs to allow the end user to figure
out what has gone wrong.
From talking with kernel people you can modify kernel
behavior for DAD routes via the accept_dad sysctl.
Please see the kernel documenation for how it works.
Additionally the
officialrecovery methodologyis to fix the address and then down/up the link.
Signed-off-by: Donald Sharp sharpd@cumulusnetworks.com