isisd: configure SRGB and SRLB together#8085
Conversation
polychaeta
left a comment
polychaeta
left a comment
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/f634a512a4f2545edaedba645832fc70/raw/4689a4508f4c6b759118983357eb5cc8ae47b76a/cr_8085_1613402230.diff | git apply
diff --git a/isisd/isis_cli.c b/isisd/isis_cli.c
index 74d8e30f2..77b0e5abf 100644
--- a/isisd/isis_cli.c
+++ b/isisd/isis_cli.c
@@ -1598,52 +1598,57 @@ void cli_show_isis_sr_enabled(struct vty *vty, struct lyd_node *dnode,
/*
* XPath: /frr-isisd:isis/instance/segment-routing/label-block
*/
-DEFPY_YANG (isis_sr_label_blocks_range,
- isis_sr_label_blocks_range_cmd,
- "segment-routing label-blocks global (16-1048575)$srgb_lbound (16-1048575)$srgb_ubound local (16-1048575)$srlb_lbound (16-1048575)$srlb_ubound",
- SR_STR
- "Set both Local and Global label blocks\n"
- "Segment Routing Global Block label range\n"
- "The lower bound of the global block\n"
- "The upper bound of the global block (block size may not exceed 65535)\n"
- "Segment Routing Local Block label range\n"
- "The lower bound of the local block\n"
- "The upper bound of the local block (block size may not exceed 65535)\n"
- )
+DEFPY_YANG(
+ isis_sr_label_blocks_range, isis_sr_label_blocks_range_cmd,
+ "segment-routing label-blocks global (16-1048575)$srgb_lbound (16-1048575)$srgb_ubound local (16-1048575)$srlb_lbound (16-1048575)$srlb_ubound",
+ SR_STR
+ "Set both Local and Global label blocks\n"
+ "Segment Routing Global Block label range\n"
+ "The lower bound of the global block\n"
+ "The upper bound of the global block (block size may not exceed 65535)\n"
+ "Segment Routing Local Block label range\n"
+ "The lower bound of the local block\n"
+ "The upper bound of the local block (block size may not exceed 65535)\n")
{
- nb_cli_enqueue_change(vty, "./segment-routing/label-blocks/srgb/lower-bound",
+ nb_cli_enqueue_change(vty,
+ "./segment-routing/label-blocks/srgb/lower-bound",
NB_OP_MODIFY, srgb_lbound_str);
- nb_cli_enqueue_change(vty, "./segment-routing/label-blocks/srgb/upper-bound",
+ nb_cli_enqueue_change(vty,
+ "./segment-routing/label-blocks/srgb/upper-bound",
NB_OP_MODIFY, srgb_ubound_str);
- nb_cli_enqueue_change(vty, "./segment-routing/label-blocks/srlb/lower-bound",
+ nb_cli_enqueue_change(vty,
+ "./segment-routing/label-blocks/srlb/lower-bound",
NB_OP_MODIFY, srlb_lbound_str);
- nb_cli_enqueue_change(vty, "./segment-routing/label-blocks/srlb/upper-bound",
+ nb_cli_enqueue_change(vty,
+ "./segment-routing/label-blocks/srlb/upper-bound",
NB_OP_MODIFY, srlb_ubound_str);
return nb_cli_apply_changes(vty, NULL);
}
-DEFPY_YANG (no_isis_sr_label_blocks_range,
- no_isis_sr_label_blocks_range_cmd,
- "no segment-routing label-blocks [global (16-1048575) (16-1048575) local (16-1048575) (16-1048575)]",
- NO_STR
- SR_STR
- "Set both Local and Global label blocks\n"
- "Segment Routing Global Block label range\n"
- "The lower bound of the global block\n"
- "The upper bound of the global block (block size may not exceed 65535)\n"
- "Segment Routing Local Block label range\n"
- "The lower bound of the local block\n"
- "The upper bound of the local block (block size may not exceed 65535)\n"
- )
+DEFPY_YANG(
+ no_isis_sr_label_blocks_range, no_isis_sr_label_blocks_range_cmd,
+ "no segment-routing label-blocks [global (16-1048575) (16-1048575) local (16-1048575) (16-1048575)]",
+ NO_STR SR_STR
+ "Set both Local and Global label blocks\n"
+ "Segment Routing Global Block label range\n"
+ "The lower bound of the global block\n"
+ "The upper bound of the global block (block size may not exceed 65535)\n"
+ "Segment Routing Local Block label range\n"
+ "The lower bound of the local block\n"
+ "The upper bound of the local block (block size may not exceed 65535)\n")
{
- nb_cli_enqueue_change(vty, "./segment-routing/label-blocks/srgb/lower-bound",
+ nb_cli_enqueue_change(vty,
+ "./segment-routing/label-blocks/srgb/lower-bound",
NB_OP_MODIFY, NULL);
- nb_cli_enqueue_change(vty, "./segment-routing/label-blocks/srgb/upper-bound",
+ nb_cli_enqueue_change(vty,
+ "./segment-routing/label-blocks/srgb/upper-bound",
NB_OP_MODIFY, NULL);
- nb_cli_enqueue_change(vty, "./segment-routing/label-blocks/srlb/lower-bound",
+ nb_cli_enqueue_change(vty,
+ "./segment-routing/label-blocks/srlb/lower-bound",
NB_OP_MODIFY, NULL);
- nb_cli_enqueue_change(vty, "./segment-routing/label-blocks/srlb/upper-bound",
+ nb_cli_enqueue_change(vty,
+ "./segment-routing/label-blocks/srlb/upper-bound",
NB_OP_MODIFY, NULL);
return nb_cli_apply_changes(vty, NULL);
@@ -1651,7 +1656,7 @@ DEFPY_YANG (no_isis_sr_label_blocks_range,
void cli_show_isis_label_blocks(struct vty *vty, struct lyd_node *dnode,
- bool show_defaults)
+ bool show_defaults)
{
vty_out(vty, " segment-routing label-blocks global %s %s local %s %s\n",
yang_dnode_get_string(dnode, "./srgb/lower-bound"),
@@ -1671,9 +1676,11 @@ DEFPY_YANG (isis_sr_global_block_label_range,
"The lower bound of the block\n"
"The upper bound of the block (block size may not exceed 65535)\n")
{
- nb_cli_enqueue_change(vty, "./segment-routing/label-blocks/srgb/lower-bound",
+ nb_cli_enqueue_change(vty,
+ "./segment-routing/label-blocks/srgb/lower-bound",
NB_OP_MODIFY, lower_bound_str);
- nb_cli_enqueue_change(vty, "./segment-routing/label-blocks/srgb/upper-bound",
+ nb_cli_enqueue_change(vty,
+ "./segment-routing/label-blocks/srgb/upper-bound",
NB_OP_MODIFY, upper_bound_str);
return nb_cli_apply_changes(vty, NULL);
@@ -1688,9 +1695,11 @@ DEFPY_YANG (no_isis_sr_global_block_label_range,
"The lower bound of the block\n"
"The upper bound of the block (block size may not exceed 65535)\n")
{
- nb_cli_enqueue_change(vty, "./segment-routing/label-blocks/srgb/lower-bound",
+ nb_cli_enqueue_change(vty,
+ "./segment-routing/label-blocks/srgb/lower-bound",
NB_OP_MODIFY, NULL);
- nb_cli_enqueue_change(vty, "./segment-routing/label-blocks/srgb/upper-bound",
+ nb_cli_enqueue_change(vty,
+ "./segment-routing/label-blocks/srgb/upper-bound",
NB_OP_MODIFY, NULL);
return nb_cli_apply_changes(vty, NULL);
@@ -1707,9 +1716,11 @@ DEFPY_YANG (isis_sr_local_block_label_range,
"The lower bound of the block\n"
"The upper bound of the block (block size may not exceed 65535)\n")
{
- nb_cli_enqueue_change(vty, "./segment-routing/label-blocks/srlb/lower-bound",
+ nb_cli_enqueue_change(vty,
+ "./segment-routing/label-blocks/srlb/lower-bound",
NB_OP_MODIFY, lower_bound_str);
- nb_cli_enqueue_change(vty, "./segment-routing/label-blocks/srlb/upper-bound",
+ nb_cli_enqueue_change(vty,
+ "./segment-routing/label-blocks/srlb/upper-bound",
NB_OP_MODIFY, upper_bound_str);
return nb_cli_apply_changes(vty, NULL);
@@ -1724,9 +1735,11 @@ DEFPY_YANG (no_isis_sr_local_block_label_range,
"The lower bound of the block\n"
"The upper bound of the block (block size may not exceed 65535)\n")
{
- nb_cli_enqueue_change(vty, "./segment-routing/label-blocks/srlb/lower-bound",
+ nb_cli_enqueue_change(vty,
+ "./segment-routing/label-blocks/srlb/lower-bound",
NB_OP_MODIFY, NULL);
- nb_cli_enqueue_change(vty, "./segment-routing/label-blocks/srlb/upper-bound",
+ nb_cli_enqueue_change(vty,
+ "./segment-routing/label-blocks/srlb/upper-bound",
NB_OP_MODIFY, NULL);
return nb_cli_apply_changes(vty, NULL);
diff --git a/isisd/isis_nb.h b/isisd/isis_nb.h
index 6d2906c78..591324307 100644
--- a/isisd/isis_nb.h
+++ b/isisd/isis_nb.h
@@ -482,7 +482,7 @@ void cli_show_isis_mt_ipv6_dstsrc(struct vty *vty, struct lyd_node *dnode,
void cli_show_isis_sr_enabled(struct vty *vty, struct lyd_node *dnode,
bool show_defaults);
void cli_show_isis_label_blocks(struct vty *vty, struct lyd_node *dnode,
- bool show_defaults);
+ bool show_defaults);
void cli_show_isis_node_msd(struct vty *vty, struct lyd_node *dnode,
bool show_defaults);
void cli_show_isis_prefix_sid(struct vty *vty, struct lyd_node *dnode,
diff --git a/isisd/isis_nb_config.c b/isisd/isis_nb_config.c
index ee0dbd032..d9be4fd82 100644
--- a/isisd/isis_nb_config.c
+++ b/isisd/isis_nb_config.c
@@ -2294,10 +2294,10 @@ int isis_instance_segment_routing_prefix_sid_map_prefix_sid_pre_validate(
struct isis_prefix_sid psid = {};
yang_dnode_get_prefix(&prefix, args->dnode, "./prefix");
- srgb_lbound = yang_dnode_get_uint32(args->dnode,
- "../../label-blocks/srgb/lower-bound");
- srgb_ubound = yang_dnode_get_uint32(args->dnode,
- "../../label-blocks/srgb/upper-bound");
+ srgb_lbound = yang_dnode_get_uint32(
+ args->dnode, "../../label-blocks/srgb/lower-bound");
+ srgb_ubound = yang_dnode_get_uint32(
+ args->dnode, "../../label-blocks/srgb/upper-bound");
sid = yang_dnode_get_uint32(args->dnode, "./sid-value");
sid_type = yang_dnode_get_enum(args->dnode, "./sid-value-type");
diff --git a/isisd/isis_sr.c b/isisd/isis_sr.c
index 9fe851795..21f534641 100644
--- a/isisd/isis_sr.c
+++ b/isisd/isis_sr.c
@@ -1213,14 +1213,14 @@ void isis_sr_area_init(struct isis_area *area)
/* Pull defaults from the YANG module. */
#ifndef FABRICD
srdb->config.enabled = yang_get_default_bool("%s/enabled", ISIS_SR);
- srdb->config.srgb_lower_bound =
- yang_get_default_uint32("%s/label-blocks/srgb/lower-bound", ISIS_SR);
- srdb->config.srgb_upper_bound =
- yang_get_default_uint32("%s/label-blocks/srgb/upper-bound", ISIS_SR);
- srdb->config.srlb_lower_bound =
- yang_get_default_uint32("%s/label-blocks/srlb/lower-bound", ISIS_SR);
- srdb->config.srlb_upper_bound =
- yang_get_default_uint32("%s/label-blocks/srlb/upper-bound", ISIS_SR);
+ srdb->config.srgb_lower_bound = yang_get_default_uint32(
+ "%s/label-blocks/srgb/lower-bound", ISIS_SR);
+ srdb->config.srgb_upper_bound = yang_get_default_uint32(
+ "%s/label-blocks/srgb/upper-bound", ISIS_SR);
+ srdb->config.srlb_lower_bound = yang_get_default_uint32(
+ "%s/label-blocks/srlb/lower-bound", ISIS_SR);
+ srdb->config.srlb_upper_bound = yang_get_default_uint32(
+ "%s/label-blocks/srlb/upper-bound", ISIS_SR);
#else
srdb->config.enabled = false;
srdb->config.srgb_lower_bound = SRGB_LOWER_BOUND;
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: 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-17154/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warningsWarnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base5 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-17155/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warningsWarnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base5 Static Analyzer issues remaining.See details at |
|
please update the documentation |
|
do we need a deprecation cycle for the old style commands? Will they auto-generate the new commands? |
riw777
left a comment
riw777
left a comment
There was a problem hiding this comment.
These changes look good ... I do wonder about the existing commands, however. I would think it would be better to auto-generate the new way of entering the command, leaving the old way in place only for changes to existing ranges, rather than for configuring new ranges. Maybe the existing commands should be left in as hidden for backward compatibility, with a plan to deprecate them in a year for configuring ranges, converting them over so they can only be used if ranges already exist, or something like this?
|
OK, I am updating the docs and adding a deprecation notice to the old commands, as requested. As for hiding them, what is the equivalent of |
polychaeta
left a comment
polychaeta
left a comment
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/a25a927cafc3c439111e7375952eb1fd/raw/30c4a99562131a35734a123d5dd49209df799fc6/cr_8085_1613744659.diff | git apply
diff --git a/isisd/isis_cli.c b/isisd/isis_cli.c
index f404772bd..dfda31ce7 100644
--- a/isisd/isis_cli.c
+++ b/isisd/isis_cli.c
@@ -1677,7 +1677,7 @@ DEFPY_HIDDEN(
"The upper bound of the block (block size may not exceed 65535)\n")
{
#if CONFDATE > 20220217
-CPP_NOTICE("Use of global-block is deprecated, please use label-blocks")
+ CPP_NOTICE("Use of global-block is deprecated, please use label-blocks")
#endif
nb_cli_enqueue_change(vty,
"./segment-routing/label-blocks/srgb/lower-bound",
@@ -1719,7 +1719,7 @@ DEFPY_HIDDEN(
"The upper bound of the block (block size may not exceed 65535)\n")
{
#if CONFDATE > 20220217
-CPP_NOTICE("Use of local-block is deprecated, please use label-blocks")
+ CPP_NOTICE("Use of local-block is deprecated, please use label-blocks")
#endif
nb_cli_enqueue_change(vty,
"./segment-routing/label-blocks/srlb/lower-bound",
diff --git a/tests/topotests/isis-sr-topo1/test_isis_sr_topo1.py b/tests/topotests/isis-sr-topo1/test_isis_sr_topo1.py
index 22d1acf8d..cf476713d 100644
--- a/tests/topotests/isis-sr-topo1/test_isis_sr_topo1.py
+++ b/tests/topotests/isis-sr-topo1/test_isis_sr_topo1.py
@@ -134,6 +134,7 @@ class TemplateTopo(Topo):
switch.add_link(tgen.gears["rt5"], nodeif="eth-rt6")
switch.add_link(tgen.gears["rt6"], nodeif="eth-rt5")
+
@pytest.mark.isis
def setup_module(mod):
"Sets up the pytest environment"
diff --git a/tests/topotests/isis-tilfa-topo1/test_isis_tilfa_topo1.py b/tests/topotests/isis-tilfa-topo1/test_isis_tilfa_topo1.py
index 982e33d30..06b477c5d 100755
--- a/tests/topotests/isis-tilfa-topo1/test_isis_tilfa_topo1.py
+++ b/tests/topotests/isis-tilfa-topo1/test_isis_tilfa_topo1.py
@@ -177,6 +177,7 @@ class TemplateTopo(Topo):
f_in.close()
f_out.close()
+
@pytest.mark.isis
def setup_module(mod):
"Sets up the pytest environment"
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: 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-17219/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warningsWarnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base1 Static Analyzer issues remaining.See details at |
I think that's correct ... you might want to pm @qlyoung on slack to verify |
odd22
left a comment
odd22
left a comment
There was a problem hiding this comment.
Thanks for the patch and the new combine command. But, I would prefer that the local part of the combined command remains optional. So that, you could configure only the global block without the need to set the local part to default value.
In addition, keeping the same syntax could avoid to mark the command deprecated for the global block. Only the local block command should be mark as deprecated.
Sure, that can be done
I'm not sure I follow, it should be the other way around, right? Making the local block part optional in the combined command means that one can configure just the SRGB or both, so I think it would make more sense to deprecate the global-block command and leave the local-block one if you just wanted to configure that. In any case the request to deprecate them came from @riw777 - I'm fine with either solution, just want to make sure everyone's on board |
Sorry, I was not clear enough. I would suggest to modify the command like this:
So, the old I hope I have been clearer |
This works ... and it's a good solution. |
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-17545/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings |
odd22
left a comment
odd22
left a comment
There was a problem hiding this comment.
New command is fine for me.
I have just 2 questions about the management of the srlb with this new command.
when changing both ranges at the same time the order of the commands matters, as we need to make sure that the intermediate state is valid. This represents a problem when pushing configuration via frr-reload. To fix this, the global-block command was extended to optionally allow setting the local-block range as well. The local-block command is deprecated with a 1-year notice. Signed-off-by: Emanuele Di Pascale <emanuele@voltanet.io>
💚 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-17556/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings |
7 participants
when changing both ranges at the same time the order of the commands matters, as we need to make sure that the intermediate state is valid. This represents a problem when pushing configuration via frr-reload.
Add a new command 'segment-routing label-blocks global XX XX local XX XX' and use this to show the configuration, so that we can process both changes in a single sweep. The existing individual commands are left there both for backwards compatibility and as utility to set/unset a single range.
Signed-off-by: Emanuele Di Pascale emanuele@voltanet.io