ospfv3 is showing enabled on loopback interface although it is not #9292

mobash-rasool · 2021-08-04T12:08:11Z

ospfv3 is showing enabled on loopback interface although
it is not configured in "show ipv6 ospf6 interface json"

Fix: Make the json output similar to the CLI output.

Signed-off-by: Mobashshera Rasool mrasool@vmware.com

LabN-CI · 2021-08-04T13:29:07Z

Outdated results 💚

Basic BGPD CI results: SUCCESS, 0 tests failed

_	_
Result	SUCCESS git merge/9292 `7ce2243`
Date	08/04/2021
Start	09:03:35
Finish	09:29:02
Run-Time	25:27
Total	1813
Pass	1813
Fail	0
Valgrind-Errors
Valgrind-Loss
Details	vncregress-2021-08-04-09:03:35.txt
Log	autoscript-2021-08-04-09:04:47.log.bz2
Memory	487 479 430

For details, please contact louberger

idryzhov

This change makes json and vty code inconsistent.

Currently, both code flows consider the existence of ifp->info as an indication that OSPF6 is enabled on an interface. And there's a separate check for oi->area that sets the attachedToArea parameter.

I agree that it's a little bit misleading, but at least now ospf6Enabled and attachedToArea parameters represent different things, and with this change, they become the same thing.

In my opinion, this ospf6Enabled really means nothing to the user, because ifp->info is set when there's at least one IPv6 address on an interface and it has nothing to do with OSPF6. So I think there are two possible options here:

Completely remove ospf6Enabled parameter as it doesn't really represent anything useful.
Leave it as it is for people who really want to know whether ifp->info is not NULL.

NetDEF-CI · 2021-08-04T17:34:36Z

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-20792/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

donaldsharp · 2021-08-04T23:36:47Z

as much as I hate to say it we cannot remove ospf6Enabled unless we go through a deprecation cycle( at the very least )

idryzhov · 2021-08-05T07:56:02Z

Then I think we should just keep things as they are. I don't see any point in making ospf6Enabled and attachedToArea the same thing.

mobash-rasool · 2021-08-05T08:33:29Z

Here the problem with json output is that is shows ospf6Enabled as true even though it is not enabled. To make the json and CLI output exactly the same, we can remove the below case so it shows only as false similar to CLI output but I remember someone asked to always keep the json parameter as true/false should not remove it, therefore I did it this way.
json_object_boolean_true_add(json_obj, "ospf6Enabled");

mobash-rasool · 2021-08-05T08:34:44Z

I have not enabled OSPFv3 on loopback interface but it shows like below:

"lo":{
"status":"up",
"type":"LOOPBACK",
"interfaceId":1,
"ospf6Enabled":true, =======>>> ISSUE
"internetAddress":[
{

idryzhov · 2021-08-05T08:57:18Z

Mobash, I completely understand what you are doing here. I explained my thoughts in my review comment, but let's do it again:

JSON and VTY code flows are already the same.
When ifp->info is NULL:

json outputs "ospf6Enabled":false and stops processing
vty outputs OSPF not enabled on this interface and stops processing

When ifp->info is not NULL they both assume that OSPF6 is enabled and output all other information.

There is already a separate parameter that checks oi->area named attachedToArea.
Your change just makes ospf6Enabled and attachedToArea the same thing and makes json and vty code inconsistent.

mobash-rasool · 2021-08-05T09:54:27Z

Mobash, I completely understand what you are doing here. I explained my thoughts in my review comment, but let's do it again:

JSON and VTY code flows are already the same.
When ifp->info is NULL:

json outputs "ospf6Enabled":false and stops processing

vty outputs OSPF not enabled on this interface and stops processing

When ifp->info is not NULL they both assume that OSPF6 is enabled and output all other information.

There is already a separate parameter that checks oi->area named attachedToArea.
Your change just makes ospf6Enabled and attachedToArea the same thing and makes json and vty code inconsistent.

Yes Igor, I do understand your thoughts here as well.

I want to point the only difference between CLI and Json which is as follows:
When OSPFv3 is not enabled, the output in the CLI shows "Not Attached to Area" and never says in this case that OSPF is enabled on the interface but the JSON output says so which is confusing.

frr(config-if)# do show ipv6 ospf6 interface
lo is up, type LOOPBACK
Interface ID: 1
Internet Address:
inet : 1.0.1.17/32
inet6: 2001:db8:f::1:17/128
**Not Attached to Area**
State Down, Transmit Delay 1 sec, Priority 1
Timer intervals configured:
Hello 10, Dead 40, Retransmit 5
DR: 0.0.0.0 BDR: 0.0.0.0
Number of I/F scoped LSAs is 0
0 Pending LSAs for LSUpdate in Time 00:00:00 [thread off]
0 Pending LSAs for LSAck in Time 00:00:00 [thread off]
'

JSON Output:

"lo":{
"status":"up",
"type":"LOOPBACK",
"interfaceId":1,
"ospf6Enabled":true, =======>>> ISSUE
"internetAddress":[
{

When ifp->info is not NULL CLI does not says that OSPF is enabled, it just outputs all the information along with it it says "Not attached to area" but the json indicates that OSPFv3 is enabled on the interface by saying true, that is why I want to fix it.

idryzhov · 2021-08-05T17:27:01Z

Mobash, I completely understand what you are doing here. I explained my thoughts in my review comment, but let's do it again:

JSON and VTY code flows are already the same.

When ifp->info is NULL:

json outputs "ospf6Enabled":false and stops processing

vty outputs OSPF not enabled on this interface and stops processing

When ifp->info is not NULL they both assume that OSPF6 is enabled and output all other information.

There is already a separate parameter that checks oi->area named attachedToArea.

Your change just makes ospf6Enabled and attachedToArea the same thing and makes json and vty code inconsistent.

Yes Igor, I do understand your thoughts here as well.

I want to point the only difference between CLI and Json which is as follows:

When OSPFv3 is not enabled, the output in the CLI shows "Not Attached to Area" and never says in this case that OSPF is enabled on the interface but the JSON output says so which is confusing.

Yes and JSON sets attachedToArea to false to indicate that interface is not attached to area. So there is no difference here.

frr(config-if)# do show ipv6 ospf6 interface

lo is up, type LOOPBACK

Interface ID: 1

Internet Address:

inet : 1.0.1.17/32

inet6: 2001:db8:f::1:17/128

**Not Attached to Area**

State Down, Transmit Delay 1 sec, Priority 1

Timer intervals configured:

Hello 10, Dead 40, Retransmit 5

DR: 0.0.0.0 BDR: 0.0.0.0

Number of I/F scoped LSAs is 0

0 Pending LSAs for LSUpdate in Time 00:00:00 [thread off]

0 Pending LSAs for LSAck in Time 00:00:00 [thread off]

'
JSON Output:
"lo":{

"status":"up",

"type":"LOOPBACK",

"interfaceId":1,

"ospf6Enabled":true, =======>>> ISSUE

"internetAddress":[

{
When ifp->info is not NULL CLI does not says that OSPF is enabled, it just outputs all the information along with it it says "Not attached to area" but the json indicates that OSPFv3 is enabled on the interface by saying true, that is why I want to fix it.

Yes, and the correct fix would be to remove this ospf6Enabled field to make the JSON code the same as CLI code. What you propose is to make JSON and CLI code different, that's why I don't like this approach.

I'm not sure what exactly we have to do as a deprecation cycle. @donaldsharp could you clarify?

qlyoung

In this case the correct thing to do is to mark the parameter deprecated, set the appropriate CPP_NOTICE date, make a note of it in the release notes, and leave the parameter as is until the deprecation period finishes and we can remove it.

@idryzhov I take it you're fine with this plan. I agree with you that making ospf6Enabled reflect something already expressed elsewhere is not the correct approach. As is, ospf6Enabled communicates effectively no information; changing it to another value is not helping anyone. It should just be removed.

qlyoung · 2021-11-30T05:54:06Z

And I'm sorry, but as a matter of practicality, I'm going to have this one closed in a couple months if it doesn't go anywhere. We can't have these PRs laying around dead in the water.

@frrbot autoclose in 2 months

mobash-rasool · 2021-11-30T05:58:32Z

In this case the correct thing to do is to mark the parameter deprecated, set the appropriate CPP_NOTICE date, make a note of it in the release notes, and leave the parameter as is until the deprecation period finishes and we can remove it.

@idryzhov I take it you're fine with this plan. I agree with you that making ospf6Enabled reflect something already expressed elsewhere is not the correct approach. As is, ospf6Enabled communicates effectively no information; changing it to another value is not helping anyone. It should just be removed.

Thanks @qlyoung for your opinion. I agree with this. We can remove it after marking it as deprecate. Let's wait for @idryzhov confirmation and then we can proceed with the removal.

frrbot · 2021-11-30T05:58:34Z

This issue will no longer be automatically closed.

idryzhov · 2021-12-04T14:16:16Z

Sorry for the long response, yes, I agree that ospf6Enabled should be removed after a deprecation cycle.

mobash-rasool · 2021-12-04T16:58:24Z

Sorry for the long response, yes, I agree that ospf6Enabled should be removed after a deprecation cycle.

Thanks @idryzhov . How to mark this should be removed after a deprecation cycle? What is the process?

mobash-rasool · 2022-01-08T08:34:38Z

@idryzhov @qlyoung : Let me know how to mark that this should be removed after a deprecation cycle, what is the process?

idryzhov · 2022-01-09T14:00:57Z

Just add something like

#if CONFDATE > 20220706
CPP_NOTICE("Time to remove ospf6Enabled from JSON output")
#endif

And we probably also need to mention the future change in the release notes of the next release.

Since ospf6Enabled and attachedToArea are denoting the same thing. It is decided to remove ospf6Enabled from json output to make CLI and json output similar. Fixes: FRRouting#9286 Signed-off-by: Mobashshera Rasool <mrasool@vmware.com>

mobash-rasool · 2022-01-10T12:01:03Z

Just add something like
#if CONFDATE > 20220706
CPP_NOTICE("Time to remove ospf6Enabled from JSON output")
#endif
And we probably also need to mention the future change in the release notes of the next release.

I have added a CPP_NOTICE.

NetDEF-CI · 2022-01-10T14:05:27Z

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-2585/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Warnings Generated during build:

Checkout code: Successful with additional warnings

Report for ospf6_interface.c | 2 issues
===============================================
< ERROR: need consistent spacing around '*' (ctx:WxV)
< #980: FILE: /tmp/f1-30743/ospf6_interface.c:980:

polychaeta added bugfix ospfv3 labels Aug 4, 2021

idryzhov requested changes Aug 4, 2021

View reviewed changes

mobash-rasool requested a review from idryzhov August 5, 2021 08:35

qlyoung self-requested a review August 31, 2021 15:46

qlyoung requested changes Nov 30, 2021

View reviewed changes

frrbot bot added the autoclose label Nov 30, 2021

frrbot bot removed the autoclose label Nov 30, 2021

mobash-rasool force-pushed the ospfv3-bug2 branch from 7ce2243 to 40a0880 Compare January 10, 2022 11:56

mobash-rasool requested a review from qlyoung January 10, 2022 12:01

idryzhov approved these changes Jan 12, 2022

View reviewed changes

idryzhov merged commit 9a80925 into FRRouting:master Jan 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ospfv3 is showing enabled on loopback interface although it is not #9292

ospfv3 is showing enabled on loopback interface although it is not #9292

mobash-rasool commented Aug 4, 2021

LabN-CI commented Aug 4, 2021 •

edited

Loading

Basic BGPD CI results: SUCCESS, 0 tests failed

idryzhov left a comment •

edited

Loading

NetDEF-CI commented Aug 4, 2021 •

edited

Loading

Continuous Integration Result: SUCCESSFUL

donaldsharp commented Aug 4, 2021

idryzhov commented Aug 5, 2021

mobash-rasool commented Aug 5, 2021

mobash-rasool commented Aug 5, 2021

idryzhov commented Aug 5, 2021 •

edited

Loading

mobash-rasool commented Aug 5, 2021

idryzhov commented Aug 5, 2021

qlyoung left a comment

qlyoung commented Nov 30, 2021

mobash-rasool commented Nov 30, 2021

frrbot bot commented Nov 30, 2021

idryzhov commented Dec 4, 2021

mobash-rasool commented Dec 4, 2021

mobash-rasool commented Jan 8, 2022

idryzhov commented Jan 9, 2022

mobash-rasool commented Jan 10, 2022

NetDEF-CI commented Jan 10, 2022

ospfv3 is showing enabled on loopback interface although it is not #9292

ospfv3 is showing enabled on loopback interface although it is not #9292

Conversation

mobash-rasool commented Aug 4, 2021

LabN-CI commented Aug 4, 2021 • edited Loading

Basic BGPD CI results: SUCCESS, 0 tests failed

idryzhov left a comment • edited Loading

Choose a reason for hiding this comment

NetDEF-CI commented Aug 4, 2021 • edited Loading

Continuous Integration Result: SUCCESSFUL

donaldsharp commented Aug 4, 2021

idryzhov commented Aug 5, 2021

mobash-rasool commented Aug 5, 2021

mobash-rasool commented Aug 5, 2021

idryzhov commented Aug 5, 2021 • edited Loading

mobash-rasool commented Aug 5, 2021

idryzhov commented Aug 5, 2021

qlyoung left a comment

Choose a reason for hiding this comment

qlyoung commented Nov 30, 2021

mobash-rasool commented Nov 30, 2021

frrbot bot commented Nov 30, 2021

idryzhov commented Dec 4, 2021

mobash-rasool commented Dec 4, 2021

mobash-rasool commented Jan 8, 2022

idryzhov commented Jan 9, 2022

mobash-rasool commented Jan 10, 2022

NetDEF-CI commented Jan 10, 2022

Continuous Integration Result: SUCCESSFUL

Warnings Generated during build:

LabN-CI commented Aug 4, 2021 •

edited

Loading

idryzhov left a comment •

edited

Loading

NetDEF-CI commented Aug 4, 2021 •

edited

Loading

idryzhov commented Aug 5, 2021 •

edited

Loading