isisd: fix redistribution in vrf #8628
Merged
+166
−79
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
polychaeta
reviewed
May 5, 2021
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/e6e58f036a479f012f1a4273ffe60f97/raw/5b670537235d5a511abad1e0288696848bcf267d/cr_8628_1620211728.diff | git apply
diff --git a/isisd/isis_redist.c b/isisd/isis_redist.c
index ae1cf98c2f..5a8c73aad4 100644
--- a/isisd/isis_redist.c
+++ b/isisd/isis_redist.c
@@ -380,9 +380,11 @@ void isis_redist_update_zebra_subscriptions(struct isis *isis, bool disable)
afi_t afi = afi_for_redist_protocol(protocol);
if (do_subscribe[protocol][type] && !disable)
- isis_zebra_redistribute_set(afi, type, isis->vrf_id);
+ isis_zebra_redistribute_set(afi, type,
+ isis->vrf_id);
else
- isis_zebra_redistribute_unset(afi, type, isis->vrf_id);
+ isis_zebra_redistribute_unset(afi, type,
+ isis->vrf_id);
}
}
If you are a new contributor to FRR, please see our contributing guidelines.
After making changes, you do not need to create a new PR. You should perform an amend or interactive rebase followed by a force push.
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
polychaeta
reviewed
May 5, 2021
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/d6fc9f23842a43e07975748e3a02d0a1/raw/450bc654ae5473584661c3da26ccfb1385cf160b/cr_8628_1620219428.diff | git apply
diff --git a/isisd/isisd.c b/isisd/isisd.c
index dcf6175d6b..c90bae48e7 100644
--- a/isisd/isisd.c
+++ b/isisd/isisd.c
@@ -612,9 +612,9 @@ static void isis_set_redist_vrf_bitmaps(struct isis *isis, bool set)
for (protocol = 0; protocol < REDIST_PROTOCOL_COUNT; protocol++)
for (type = 0; type < ZEBRA_ROUTE_MAX + 1; type++)
for (level = 0; level < ISIS_LEVELS; level++)
- if (area->redist_settings[protocol]
- [type]
- [level].redist)
+ if (area->redist_settings
+ [protocol][type][level]
+ .redist)
do_subscribe[protocol][type] =
1;
If you are a new contributor to FRR, please see our contributing guidelines.
After making changes, you do not need to create a new PR. You should perform an amend or interactive rebase followed by a force push.
polychaeta
reviewed
May 5, 2021
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/627ac78bc40d703e549917ff0545a37e/raw/ded3b149baf15a10787fe3b1b0ae416a37f05f7e/cr_8628_1620219480.diff | git apply
diff --git a/isisd/isisd.c b/isisd/isisd.c
index a688ee04bf..c65907b171 100644
--- a/isisd/isisd.c
+++ b/isisd/isisd.c
@@ -612,9 +612,9 @@ static void isis_set_redist_vrf_bitmaps(struct isis *isis, bool set)
for (protocol = 0; protocol < REDIST_PROTOCOL_COUNT; protocol++)
for (type = 0; type < ZEBRA_ROUTE_MAX + 1; type++)
for (level = 0; level < ISIS_LEVELS; level++)
- if (area->redist_settings[protocol]
- [type]
- [level].redist)
+ if (area->redist_settings
+ [protocol][type][level]
+ .redist)
do_subscribe[protocol][type] =
1;
If you are a new contributor to FRR, please see our contributing guidelines.
After making changes, you do not need to create a new PR. You should perform an amend or interactive rebase followed by a force push.
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: FailedTopotests debian 10 amd64 part 1: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO1DEB10AMD64-18828/test Topology Tests failed for Topotests debian 10 amd64 part 1: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-18828/artifact/TOPO1DEB10AMD64/ErrorLog/log_topotests.txt Successful on other platforms/tests
Warnings Generated during build:Checkout code: Successful with additional warningsTopotests debian 10 amd64 part 1: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO1DEB10AMD64-18828/test Topology Tests failed for Topotests debian 10 amd64 part 1: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-18828/artifact/TOPO1DEB10AMD64/ErrorLog/log_topotests.txt CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base2 Static Analyzer issues remaining.See details at |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
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-18831/ This is a comment from an automated CI system. CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base2 Static Analyzer issues remaining.See details at |
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-18836/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warningsCLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base2 Static Analyzer issues remaining.See details at |
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-18837/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warningsCLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base2 Static Analyzer issues remaining.See details at |
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-18838/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warningsCLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base2 Static Analyzer issues remaining.See details at |
polychaeta
reviewed
May 9, 2021
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/04753e06fb8272dc04318a3b2ba4fa86/raw/dd7f8799c82829d40be95c58ba7ee79f74901644/cr_8628_1620572823.diff | git apply
diff --git a/isisd/isis_redist.c b/isisd/isis_redist.c
index dfa5d8251..2f5e490da 100644
--- a/isisd/isis_redist.c
+++ b/isisd/isis_redist.c
@@ -361,9 +361,10 @@ static void isis_redist_update_zebra_subscriptions(struct isis *isis)
for (protocol = 0; protocol < REDIST_PROTOCOL_COUNT; protocol++)
for (type = 0; type < ZEBRA_ROUTE_MAX + 1; type++)
for (level = 0; level < ISIS_LEVELS; level++)
- if (area->redist_settings
- [protocol][type][level]
- .redist == 1)
+ if (area->redist_settings[protocol]
+ [type][level]
+ .redist
+ == 1)
do_subscribe[protocol][type] =
1;
diff --git a/isisd/isisd.c b/isisd/isisd.c
index 4d4ba0a39..9b61c5d28 100644
--- a/isisd/isisd.c
+++ b/isisd/isisd.c
@@ -604,9 +604,10 @@ static void isis_set_redist_vrf_bitmaps(struct isis *isis, bool set)
for (protocol = 0; protocol < REDIST_PROTOCOL_COUNT; protocol++)
for (type = 0; type < ZEBRA_ROUTE_MAX + 1; type++)
for (level = 0; level < ISIS_LEVELS; level++)
- if (area->redist_settings
- [protocol][type][level]
- .redist == 1)
+ if (area->redist_settings[protocol]
+ [type][level]
+ .redist
+ == 1)
do_subscribe[protocol][type] =
1;
@@ -627,16 +628,24 @@ static void isis_set_redist_vrf_bitmaps(struct isis *isis, bool set)
if (type == DEFAULT_ROUTE) {
if (set)
- vrf_bitmap_set(zclient->default_information[afi], isis->vrf_id);
+ vrf_bitmap_set(
+ zclient->default_information
+ [afi],
+ isis->vrf_id);
else
- vrf_bitmap_unset(zclient->default_information[afi], isis->vrf_id);
+ vrf_bitmap_unset(
+ zclient->default_information
+ [afi],
+ isis->vrf_id);
} else {
if (set)
- vrf_bitmap_set(zclient->redist[afi][type],
- isis->vrf_id);
+ vrf_bitmap_set(
+ zclient->redist[afi][type],
+ isis->vrf_id);
else
- vrf_bitmap_unset(zclient->redist[afi][type],
- isis->vrf_id);
+ vrf_bitmap_unset(
+ zclient->redist[afi][type],
+ isis->vrf_id);
}
}
}
If you are a new contributor to FRR, please see our contributing guidelines.
After making changes, you do not need to create a new PR. You should perform an amend or interactive rebase followed by a force push.
|
Rebased and fixed a couple more issues. |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
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-18904/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings |
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: FailedCentOS 7 rpm pkg check: Failed (click for details)CentOS 7 rpm pkg check: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-18905/artifact/CENTOS7RPM/ErrorLog/log_package_install.txt CentOS 7 rpm pkg check: No useful log foundSuccessful on other platforms/tests
Warnings Generated during build:Checkout code: Successful with additional warningsCentOS 7 rpm pkg check: Failed (click for details)CentOS 7 rpm pkg check: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-18905/artifact/CENTOS7RPM/ErrorLog/log_package_install.txt CentOS 7 rpm pkg check: No useful log found |
|
ci:rerun |
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: FailedTopotests debian 10 amd64 part 9: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO9DEB10AMD64-18909/test Topology Tests failed for Topotests debian 10 amd64 part 9: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-18909/artifact/TOPO9DEB10AMD64/ErrorLog/log_topotests.txt Successful on other platforms/tests
Warnings Generated during build:Checkout code: Successful with additional warningsTopotests debian 10 amd64 part 9: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO9DEB10AMD64-18909/test Topology Tests failed for Topotests debian 10 amd64 part 9: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-18909/artifact/TOPO9DEB10AMD64/ErrorLog/log_topotests.txt |
|
Failures are unrelated to this PR. |
riw777
reviewed
May 11, 2021
| [level].redist) | ||
| [type][level] | ||
| .redist | ||
| == 1) |
There was a problem hiding this comment.
Could just be the way I'm seeing this, but this looks misaligned to me?
There was a problem hiding this comment.
This is how polychaeta wants it to be :) I just followed its recommendation.
There was a problem hiding this comment.
nit: it would be great if isis_redist->redist could be turned into an enum, otherwise it's really easy to mistake it as a boolean which is either set or not set (causing bugs like this one you fixed).
Previously ISIS was redistributing routes from the default VRF instead of the VRF it is configured in. I think this is the reason. At least right now metrics are equal to ones from the same test in the default VRF. I believe this is correct. |
Currently the VRF is deregistered only when it is re-enabled again. Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
We don't need to register for default routes from zebra, when the origination type is set to "always". Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
When the redistribution is configured in non-default VRF, isisd should redistribute routes from this VRF instead of default. Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
Release memory for all redistributed route info. Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
We only need an instance when we have at least one area configured in a VRF. Currently we have the following issues: - instance for the default VRF is always created - instance is not removed after the last area config is removed This commit fixes both issues. Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
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: FailedTopotests Ubuntu 18.04 i386 part 3: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO3U18I386-18995/test Topology Tests failed for Topotests Ubuntu 18.04 i386 part 3: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-18995/artifact/TOPO3U18I386/ErrorLog/log_topotests.txt Successful on other platforms/tests
Warnings Generated during build:Checkout code: Successful with additional warningsTopotests Ubuntu 18.04 i386 part 3: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO3U18I386-18995/test Topology Tests failed for Topotests Ubuntu 18.04 i386 part 3: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-18995/artifact/TOPO3U18I386/ErrorLog/log_topotests.txt |
rwestphal
reviewed
May 18, 2021
| @@ -595,6 +589,68 @@ static int isis_vrf_delete(struct vrf *vrf) | |||
| return 0; | |||
| } | |||
|
|
|||
| static void isis_set_redist_vrf_bitmaps(struct isis *isis, bool set) | |||
There was a problem hiding this comment.
This is very similar to isis_redist_update_zebra_subscriptions(). Wouldn't it be possible to merge both functions together?
Overall these changes look good to me (important bug fixes), but it's a bit scary how complex the redistribute handling has gotten. Long term we should consider using VRF names instead of VRF IDs when sending redistribute requests to zebra, since VRF names are immutable. That way we wouldn't need to sync with zebra whenever a VRF is disabled, enabled, or just change its ID (for those adventurous enough to try it :)).
There was a problem hiding this comment.
I was thinking about merging them initially, but there are already a lot of indentation levels in isis_redist_update_zebra_subscriptions, and adding more conditions will make it horrible.
Let's keep them separate, and when we implement your suggestion about name registration, the second function will be simply removed.
|
ci:rerun |
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 i386 build: Failed (click for details)Ubuntu 16.04 i386 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-19113/artifact/U1604I386/ErrorLog/ Ubuntu 16.04 i386 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-19113/artifact/U1604I386/config.status/config.status Ubuntu 16.04 i386 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-19113/artifact/U1604I386/config.log/config.log.gz Ubuntu 16.04 i386 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-19113/artifact/U1604I386/frr.xref.xz/frr.xref.xz Ubuntu 16.04 i386 build: No useful log foundSuccessful on other platforms/tests
Warnings Generated during build:Checkout code: Successful with additional warningsUbuntu 16.04 i386 build: Failed (click for details)Ubuntu 16.04 i386 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-19113/artifact/U1604I386/ErrorLog/ Ubuntu 16.04 i386 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-19113/artifact/U1604I386/config.status/config.status Ubuntu 16.04 i386 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-19113/artifact/U1604I386/config.log/config.log.gz Ubuntu 16.04 i386 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-19113/artifact/U1604I386/frr.xref.xz/frr.xref.xz Ubuntu 16.04 i386 build: No useful log found |
|
ci:rerun |
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-19124/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings |
|
Hi @donaldsharp, I am the author of PR #8215 mentioned by @idryzhov. For the record I want to signal that this PR, for the part relative to redist vrf problem, was taken from my PR without adding or fixing anything in respect of my code. When I've sent the PR I've had the same problem with metrics change because that problem isn't releated with our modifications but is a side effect. I've asked for help to @idryzhov, the reviewer of my PR, to understand the causes of the failure and he has decided to send his PR rewriting mine and patching the test in a way that obviously allow the code to pass the CI verifications. Maybe I've had too much respect for the project, I should have patched the test too. In any case I'm glad that this functionality was integrated, my time was not totally wasted. |
|
My answer is in the mentioned PR - #8215 (comment) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When the redistribution is configured in non-default VRF, isisd should
redistribute routes from this VRF instead of default.
This is an easier implementation of #8215.