add mpls auto mode to update mpls control upon routing events #13965
Conversation
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 amd64 part 1: Failed (click for details)Topotests Ubuntu 18.04 amd64 part 1: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-12820/artifact/TP1U1804AMD64/TopotestDetails/Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TP1U1804AMD64-12820/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 1 Topotests Ubuntu 18.04 arm8 part 8: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 8: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-12820/artifact/TOPO8U18AMD64/TopotestDetails/ Topotests Ubuntu 18.04 arm8 part 8: No useful log foundSuccessful on other platforms/tests
|
zebra/interface.c
Outdated
| NB_OP_CREATE, "true"); | ||
| else | ||
| nb_cli_enqueue_change(vty, "./frr-zebra:zebra/mpls", | ||
| NB_OP_DESTROY, "false"); |
There was a problem hiding this comment.
I think "false" is not needed, since you call DESTROY, right?
zebra/zebra_nb_config.c
Outdated
| int lib_interface_zebra_mpls_destroy(struct nb_cb_destroy_args *args) | ||
| { | ||
| struct interface *ifp; | ||
| struct zebra_if *if_data; |
There was a problem hiding this comment.
the convention is to call these pointers "zif"
zebra/zebra_nb_config.c
Outdated
|
|
||
| if_data->mpls_config = IF_ZEBRA_DATA_UNSPEC; | ||
|
|
||
| dplane_intf_mpls_modify_state(ifp, false); |
There was a problem hiding this comment.
so ... this is sort of a problem we've had in this area. a 'destroy' should mean 'no configuration', but this looks like "configured to 'off'".
There was a problem hiding this comment.
you are right. I added a commit to fix this issue.
|
I'm a little uncertain what this nb work means. Is this actually plumbed through zebra - many of the apis look to be stubs? There's no mgmtd connection, right? so ... who's talking to zebra to effect this mpls config change via the nb? |
The NB API is supposed to completely replace the 'struct interface' support, when it deals with configuration. So I guess this is where we have to go to, even if the path is not completely clear to me. |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDTest incomplete. See below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: IncompleteAddresssanitizer topotests part 8: Incomplete(check logs for details)Successful on other platforms/tests
|
|
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 Ubuntu 18.04 amd64 part 9: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO9U18AMD64-12920/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 9 Topotests Ubuntu 18.04 i386 part 9: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO9U18I386-12920/test Topology Tests failed for Topotests Ubuntu 18.04 i386 part 9 Topotests Ubuntu 18.04 amd64 part 1: Failed (click for details)Topotests Ubuntu 18.04 amd64 part 1: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-12920/artifact/TP1U1804AMD64/TopotestDetails/Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TP1U1804AMD64-12920/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 1 Successful on other platforms/tests
|
|
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-PULLREQ2-12923/ This is a comment from an automated CI system. |
zebra/interface.c
Outdated
| NO_STR | ||
| MPLS_STR | ||
| "Set mpls to be on for the interface\n") | ||
| DEFPY(mpls, mpls_cmd, "[no] mpls <enable|disable|auto>$state", |
There was a problem hiding this comment.
Please indent as others DEFPYs/DEFUNs.
| @@ -26,6 +26,7 @@ extern "C" { | |||
| #define IF_ZEBRA_DATA_UNSPEC 0 | |||
| #define IF_ZEBRA_DATA_ON 1 | |||
| #define IF_ZEBRA_DATA_OFF 2 | |||
| #define IF_ZEBRA_DATA_AUTO 3 | |||
There was a problem hiding this comment.
Can we use the same spacing as above?
done |
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-PULLREQ2-13207/ This is a comment from an automated CI system. |
|
whoa, whoa - the description here just talks about making a northbound/yang-model change, but there's a number of new pieces of dynamic logic that appears to be switching mpls forwarding on and off during rib and lsp processing. that needs to be, at a bare minimum, described clearly, and preferably be in a separate PR, separate from the mechanical yang-model changes. |
pguibert6WIND
changed the title
You are right to mention two changes in a single PR. however, the auto mode does not take too much code. and it is a separate commit. |
pguibert6WIND
force-pushed
the
mpls_yang
branch
from
49de474 to
0ab52f3
Compare
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 arm8 part 9: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 9: No useful log foundAddresssanitizer topotests part 5: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-ASAN5-13312/test Topology Tests failed for Addresssanitizer topotests part 5 Successful on other platforms/tests
|
|
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 Ubuntu 18.04 i386 part 9: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO9U18I386-13319/test Topology Tests failed for Topotests Ubuntu 18.04 i386 part 9 Successful on other platforms/tests
|
pguibert6WIND
force-pushed
the
mpls_yang
branch
2 times, most recently
from
779947b to
c3dc82c
Compare
|
ci:rerun |
pguibert6WIND
force-pushed
the
mpls_yang
branch
2 times, most recently
from
508f649 to
0d6c3cd
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
pguibert6WIND
force-pushed
the
mpls_yang
branch
from
0d6c3cd to
deba9c9
Compare
lib/ldp_sync.h
Outdated
| @@ -56,6 +56,11 @@ struct ldp_igp_sync_if_state { | |||
| bool sync_start; | |||
| }; | |||
|
|
|||
| struct ldp_igp_configure_if_mpls { | |||
| ifindex_t ifindex; | |||
| bool status; | |||
There was a problem hiding this comment.
if the status is enabled/disabled, then it's more clear it would be named bool enabled (IMO)... Because status could be more than enabled/disabled, right?
There was a problem hiding this comment.
changed to enabled.
lib/zclient.c
Outdated
| ifindex_t ifindex, bool set) | ||
| { | ||
| struct stream *s; | ||
| int w; |
There was a problem hiding this comment.
Where do you use this except for +?
There was a problem hiding this comment.
fixed. removed the w
| pytest.skip(tgen.errors) | ||
|
|
||
| router = tgen.gears["r2"] | ||
| output = router.vtysh_cmd("config terminal\nmpls lsp 33 192.168.2.3 implicit-null") |
There was a problem hiding this comment.
you don't check/print the output of output, we can just remove this assignment...
tests/topotests/static_routing_mpls/test_static_routing_mpls.py
Outdated
Show resolved
Hide resolved
| "r2, configuring a route with labeled nexthop interface, checking that MPLS state is on on r2-eth0" | ||
| ) | ||
| router = tgen.gears["r2"] | ||
| output = router.vtysh_cmd( |
zebra/interface.c
Outdated
| @@ -49,6 +49,10 @@ DEFINE_HOOK(zebra_if_config_wr, (struct vty * vty, struct interface *ifp), | |||
|
|
|||
| DEFINE_MTYPE(ZEBRA, ZIF_DESC, "Intf desc"); | |||
|
|
|||
| FRR_CFG_DEFAULT_UINT8_T(ZEBRA_MPLS, | |||
| { .val_uint8_t = IF_ZEBRA_DATA_AUTO}, | |||
There was a problem hiding this comment.
styling: missing whitespace after AUTO:
{ .val_uint8_t = IF_ZEBRA_DATA_AUTO },|
|
||
| if (!ifp) | ||
| return; | ||
| zif = ifp->info; |
There was a problem hiding this comment.
Shouldn't we check if zif is not NULL?
| @@ -116,6 +118,9 @@ struct zebra_if { | |||
| /* MPLS configuration */ | |||
| uint8_t mpls_config; | |||
|
|
|||
| /* MPLS dynamic configuration - when mpls_config is auto */ | |||
| uint8_t mpls_dynamic; | |||
There was a problem hiding this comment.
What values can it have actually?
There was a problem hiding this comment.
MPLS dynamic configuration - when mpls_config is auto
by default, set to IF_ZEBRA_DATA_UNSPEC
when used, acceptable values: IF_ZEBRA_DATA_ON or IF_ZEBRA_DATA_OFF
| } | ||
|
|
||
| if (nlabel != MPLS_LABEL_NONE) { | ||
| mpls_label_t out_label = MPLS_LABEL_IMPLICIT_NULL; | ||
| mpls_lsp_install(def_zvrf, ltype, nlabel, 1, &out_label, | ||
| NEXTHOP_TYPE_IFINDEX, NULL, ifp->ifindex); | ||
| mpls_auto_interface_data_on(ifp); |
There was a problem hiding this comment.
Shouldn't this be turned on before installing MPLS LSP?
There was a problem hiding this comment.
no, this should be turned on, only when used.
actually, setting the /proc/sys/net/mpls/input flag may have negative impact on kernel, when interface flapping happens, and when platform_auto value has huge value.
this method avoids generalising the usage of the input flag on every interface.
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
pguibert6WIND
force-pushed
the
mpls_yang
branch
2 times, most recently
from
cd4ea1f to
6bb690a
Compare
pguibert6WIND
force-pushed
the
mpls_yang
branch
2 times, most recently
from
e4dacef to
138acea
Compare
pguibert6WIND
force-pushed
the
mpls_yang
branch
3 times, most recently
from
ee5e2a1 to
fc5a397
Compare
To support the various values of mpls configuration, as the value is encoded in a 8 bits value, define a new uint8_t type to record it. Signed-off-by: Philippe Guibert <[email protected]>
Add 'mplsConfig' json option, that defines which MPLS
configuration is applied to a given interface.
> # show interface loop1 json
> {
> "loop1":{
> [..]
> "mplsConfig":"mplsEnabled",
> "mplsEnabled":false,
> [..]
Signed-off-by: Philippe Guibert <[email protected]>
By default, MPLS configuration per interface is unspecified, but can not be changed by compilation. Add the framework to change the MPLS settings. Signed-off-by: Philippe Guibert <[email protected]>
There is not auto mode for mpls settings, so that when any labeled route or mpls entry is configured, then the used interface should automatically turn on its mpls settings. Add the following per interface command: > mpls auto Change the yang model to replace the mpls boolean value with an enumerate, which is mapped over the data values declared under zebra/interface.h. Signed-off-by: Philippe Guibert <[email protected]>
When a labeled route, or an MPLS entry is configured, the interface may not have properly enabled the interface with mpls. When the interface is in mpls auto mode, change the mpls interface value when a route or an mpls entry requests it to be turned on. Signed-off-by: Philippe Guibert <[email protected]>
There is no test that checks for the mpls auto mode configuration. New checks are added to the static mpls routing test: - check that a labeled route turns on mpls on the interface where the auto mode is set - check that a configured interface recovers the mpls setting Signed-off-by: Philippe Guibert <[email protected]>
When LDP is enabled per interface, MPLS is not turned on on each of those interfaces. Fix this by letting LDP configure the interface if the 'mpls auto' mode is configured. Create an IMSG to send interface config request from ldpe thread to main thread of ldp. Create a ZAPI message to set MPLS from ldp to zebra. Update the MPLS setting. Add a test that ensures that LDP properly configures the interface. Signed-off-by: Philippe Guibert <[email protected]>
Change the default MPLS setting per interface to auto mode. Signed-off-by: Philippe Guibert <[email protected]>
The yang NB API does not handle the mpls configuration on its leaf.
Add an mpls leaf to stick to the mpls configuration.