ospfd: add instance id for one command #16075
Conversation
anlancs
force-pushed
the
ospfd/fix-cmd-instance
branch
from
1ab824a
to
66cfc37
Compare
ospfd/ospf_vty.c
Outdated
| @@ -10755,10 +10755,11 @@ DEFUN (ospf_route_aggregation_timer, | |||
|
|
|||
| DEFPY (show_ip_ospf_gr_helper, | |||
| show_ip_ospf_gr_helper_cmd, | |||
| "show ip ospf [vrf <NAME|all>] graceful-restart helper [detail] [json]", | |||
| "show ip ospf [(1-65535)]$instance [vrf <NAME|all>] graceful-restart helper [detail] [json]", | |||
There was a problem hiding this comment.
I'm pretty sure that instance cannot be mixed with vrf. They are orthogonal. So shouldn't that imply then that it could be instance or vrf?
There was a problem hiding this comment.
@donaldsharp @odd22
Yes, the ospf instance can't support vrf, i have pushed new one.
But, i have double-checked the current ospf code, i found it is disorder about instance-id in many ospf commands relating to instance/vrf.
There was a problem hiding this comment.
But , this command can be executed successfully:
show ip ospf 1 vrf default database detail json
There was a problem hiding this comment.
I think it is working only because you specify the default vrf for which you could have many instances. But, if you try to specify a different instance than the default one and specify a given vrf, it will not work.
The CLI should be show ip ospf vrf foo ... or show ip ospf 10 ... but not a combination of two.
Thus, the correct CLI syntax should be show ip ospf [{(1-65535)$instance|vrf <NAME|all>}] graceful-restart helper [detail] [json] Note the '{}' to be sure that both options are exclusive.
Just have a look to the creation of OSPF router at the beginning of ospf_vty.c as an example.
I also note, for the various show command, that there is a version with vrf and another DEFUN for the instance version. I agree that there is no graceful-restart show command with instance option while there is one for vrf. Perhaps, the good way to proceed, like for other show CLI commands, is to create a dedicated version of show graceful-restart command for instance option.
anlancs
force-pushed
the
ospfd/fix-cmd-instance
branch
from
66cfc37
to
ed9474c
Compare
There was a problem hiding this comment.
The CLI should be show ip ospf vrf foo ... or show ip ospf 10 ... but not a combination of two.
Thus, the correct CLI syntax should be show ip ospf [{(1-65535)$instance|vrf <NAME|all>}] graceful-restart helper [detail] [json] Note the '{}' to be sure that both options are exclusive.
Just have a look to the creation of OSPF router at the beginning of ospf_vty.c as an example.
I also note, for the various show command, that there is a version with vrf and another DEFUN for the instance version. I agree that there is no graceful-restart show command with instance option while there is one for vrf. Perhaps, the good way to proceed, like for other show CLI commands, is to create a dedicated version of show graceful-restart command for instance option.
Add the specific instance id for the command:
```
show ip ospf [{(1-65535)$instance|vrf <NAME|all>}] graceful-restart helper [detail] [json]
```
Signed-off-by: anlan_cs <[email protected]>
Signed-off-by: anlan_cs <[email protected]>
anlancs
force-pushed
the
ospfd/fix-cmd-instance
branch
from
ed9474c
to
b212c4d
Compare
|
Done, thanks for your help. @odd22 |
|
CI:rerun |
|
I agree that there are a couple of other commands that allow the combination of the two when they should not. We did not catch those when they went in and should be fixed. They have nothing to do with this commit though |
Add the specific instance id for one ospf command, more in commit log.