[netdata-publisher] optimize multiple ULA-based route prefixes #8827
Conversation
Size Report of OpenThread
|
abtink
force-pushed
the
nd/optimize-ula-routes
branch
from
f5db24c
to
47fac6a
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #8827 +/- ##
==========================================
+ Coverage 79.89% 82.46% +2.56%
==========================================
Files 518 535 +17
Lines 70747 72830 +2083
==========================================
+ Hits 56523 60056 +3533
+ Misses 14224 12774 -1450
|
abtink
force-pushed
the
nd/optimize-ula-routes
branch
2 times, most recently
from
d1cb79e
to
8b39231
Compare
There was a problem hiding this comment.
See comments inline!
Just to check: based on the size report, I see +400 for CLI-MTD and CLI-FTD builds. So that is because the CLI builds contain a complete netdata-publisher, including any features that would normally be only used by a BR, right? The proposed test also uses this feature on FTDs/Routers. Also makes it useful for simulations to emulate a BR behavior.
| } | ||
| else | ||
| { | ||
| IgnoreError(Get<Local>().RemoveHasRoutePrefix(AsCoreType(&kCompactPrefix))); |
There was a problem hiding this comment.
This is to remove a previously-added kCompactPrefix. But what about these situations
- A kCompactPrefix was never added before (but there is a real route to fc00::/7 to be advertised) -> does this mean the 'real' route gets removed unintendedly?
- A kCompactPrefix was added before, but there's also a real route to fc00::/7 -> could it mean the real route is removed instead of the intended kCompactPrefix entry?
There was a problem hiding this comment.
Thanks @EskoDijk . Yes. The two cases you mention should be covered.
-
On second case (
fc00::/7is itself being published):- We remove prefix first, but then it will be re-added from
AddIfOptimized()call below (the order here is important). - We cover it in the test (i.e. we publish
fc00::/7and then we add/remove other prefixes to go above/below theN=3threshold (last steps intest-019).
- We remove prefix first, but then it will be re-added from
-
On first case:
- It should work with current code (would rely on the fact that publisher would not immediately add prefix).
- But to be safer I changed the code to track if/when we add compact prefix (use a new boolean in new push).
- Also added this situation in the test
test-019.
tests/toranj/cli/test-019-network-data-publisher-compact-routes.py
Outdated
Show resolved
Hide resolved
tests/toranj/cli/test-019-network-data-publisher-compact-routes.py
Outdated
Show resolved
Hide resolved
tests/toranj/cli/test-019-network-data-publisher-compact-routes.py
Outdated
Show resolved
Hide resolved
|
|
||
| def check_netdata_step11(): | ||
| routes = r2.get_netdata_routes() | ||
| verify(len(routes) == 3) |
There was a problem hiding this comment.
Where did the routes fd01:3:0:0::/64 and fd01:4:0:0::/64 go to? Does this mean that more-specific routes are automatically suppressed, in case of ULA routes? Why do this for ULAs but not for non-ULA prefixes then? Seems like we should discuss this in SPEC-1130.
There was a problem hiding this comment.
Maybe it counts N=3 ULA prefixes now, based on the newly added fc00::/7 ? And then all prefixes are optimized into fc00::/7.
There was a problem hiding this comment.
When we get here we have the following prefixes being published on r1:
fc00::/7 s med(this is explicitly published - covering the case from above)fd01:3:0:0::/64 s medfd01:4:0:0::/64 s lowfd01:5:0:0:0:0::/96 sn med(NAT64 flag so not counted).
The first 3 are "optimizable" (have fc00::/7 as sub-prefix) and can be covered by fc00::/7, so we are above N=3 threshold and they are all replaced with compact prefix (which happens to be one the published prefixes).
This commit adds a new mechanism in `NetworkData::Publisher` to optimize published route prefixes that are within ULA range `fc00::/7` (can be covered by this shorter prefix). Whenever there are more than three such route prefixes (excluding NAT64 prefixes) being published, they are marked as optimized and replaced with `fc00::/7` route prefix. The compact `fc00::/7` route is added using highest preference level among all prefixes being optimized. `OPENTHREAD_CONFIG_NETDATA_PUBLISHER_OPTIMIZE_ULA_ROUTES` enables this feature. It is enabled by default. This commit adds a new test cases covering behavior of the new feature in detail, in particular when many route prefixes are published together or separately and we go over or below the threshold.
abtink
force-pushed
the
nd/optimize-ula-routes
branch
from
8b39231
to
56d4164
Compare
Yes. Correct. The code-size check build script seems to include Lines 121 to 156 in 0a79c30 |
|
Closing this PR since discussion the SPEC-1130 moved towards a different approach. |
This commit adds a new mechanism in
NetworkData::Publisherto optimize published route prefixes that are within ULA rangefc00::/7(can be covered by this shorter prefix). Whenever there are more than three such route prefixes (excluding NAT64 prefixes) being published, they are marked as optimized and replaced withfc00::/7route prefix. The compactfc00::/7route is added using highest preference level among all prefixes being optimized.OPENTHREAD_CONFIG_NETDATA_PUBLISHER_OPTIMIZE_ULA_ROUTESenables this feature. It is enabled by default.This commit adds a new test cases covering behavior of the new feature in detail, in particular when many route prefixes are published together or separately and we go over or below the threshold.
This is related to SPEC-1130.