search

netbox-community / netbox

The premier source of truth powering network automation. Open source under Apache 2. Public demo: https://demo.netbox.dev
http://netboxlabs.com/oss/netbox/
Apache License 2.0
15.67k stars 2.53k forks source link

Clean up code for showing CircuitTermination on interfaces #4812

Closed steffann closed 3 years ago

steffann commented 4 years ago

This template code relies on trace() not traversing circuits: https://github.com/netbox-community/netbox/blob/43d610405f625f2044ebdab0c21df01b75dc856d/netbox/templates/dcim/inc/interface.html#L94-L123

Since https://github.com/netbox-community/netbox/commit/e143158f121c7d147bec820af227388deca6dfb5 this is no longer the case, which makes the template code partially obsolete. This needs to be cleaned up.

Example 1

Consider the following topology:

+--------+     +------+  +-----------+  +------+     +--------+
| Intf X +-----+ CT A +--+ Circuit C +--+ CT Z +-----+ Intf Y |
+--------+  1  +------+  +-----------+  +------+  2  +--------+

Old implementation

Before https://github.com/netbox-community/netbox/commit/e143158f121c7d147bec820af227388deca6dfb5 the signal handler to update connected_endpoints would call trace(follow_circuits=False). This would cause the connected_endpoint for Interface X to be CircuitTermination A (and vice versa), and the connected_endpoint for Interface Y to be CircuitTermination Z (and vice versa).

When the template is showing the connected endpoint it will try to call get_peer_termination on it. Because the connected endpoint is a CircuitTermination this may succeed (depending on whether both CircuitTerminations exist for the Circuit, in this example it exists) and if it does the CircuitTermination on the other end of the Circuit will be used. If that CircuitTermination has a connected endpoint then that connected endpoint will be shown as the connected endpoint of the interface, and the circuit information will be displayed as "via …".

So in the example above, for Interface X, the old code would have CircuitTermination A stored as the connected endpoint in the database, but it would display Interface Y via Circuit C in the user interface.

Current implementation

Since https://github.com/netbox-community/netbox/commit/e143158f121c7d147bec820af227388deca6dfb5 the follow_circuits parameter to trace() no longer exists, and the implementation always follows circuits. So now the connected_endpoint of Interface X will be Interface Y (and vice versa). The code that displays the connected endpoint of the peer of a circuit termination if the original connected endpoint is a circuit termination is therefore obsolete (and considering the length of that sentence probably rightfully so).

Example 2

Consider the following topology:

+--------+     +------+  +-----------+  +------+
| Intf X +-----+ CT A +--+ Circuit C +--+ CT Z |
+--------+  1  +------+  +-----------+  +------+

Old implementation

The old implementation would stop at CircuitTermination A and store that as the connected endpoint. The user interface would then find the other CircuitTermination through get_peer_termination and display that.

Current implementation

Because circuits are now always traversed trace will now return these steps in the path:

CableTermination Cable CableTermination
Interface X 1 CircuitTermination A
CircuitTermination Z None None

Because the last CableTermination is empty the connected_endpoint will be empty and the user interface will show "Not connected"

Example 3

Consider the following topology:

+--------+     +------+  +-----------+
| Intf X +-----+ CT A +--+ Circuit C |
+--------+  1  +------+  +-----------+

Both the old and the new implementation handle this in the same way. What makes this a special case is that this is the only remaining situation where the connected_endpoint of CircuitTermination A is updated. When CircuitTermination Z is later added and connected to an interface the connected_endpoint of Interface X will be updated to reflect that but the connected_endpoint of CircuitTermination A will still be Interface X.

This behaviour is inconsistent because it only occurs when there is only one CircuitTermination connected to the Circuit. When both CircuitTerminations are created before connecting the cable the connected_endpoint of CableTermination A will always remain empty.

steffann commented 4 years ago

I propose the following clean-ups:

Example 1

There is no good way to consistently determine the connected_endpoint based on the connected_endpoint of a CircuitTermination. Going back to the old implementation where we store the CircuitTermination instead of the Interface will break whenever there is a one-to-many RearPort connected to the CircuitTermination. I therefore propose to keep the current implementation and remove the template code for displaying "via Circuit C".

Example 2

In this scenario I think it makes the most sense to store CircuitTermination Z as the connected_endpoint for Interface X (and vice versa). The template can then display the remote site again with the "via Circuit C", as it used to be in the old implementation but with cleaner template code.

Example 3

Here a choice has to be made about what to put in the connected_endpoint of CircuitTerminations. I propose to make it consistent that a CircuitTermination consistently has an Interface as its connected endpoint, but not vice-versa. So when taking the topology from example 1 we would end up with:

CableTermination Connected endpoint
Interface X Interface Y
Interface Y Interface X
CircuitTermination A Interface X
CircuitTermination Z Interface Y

I think this would make the most sense to the user.

tyler-8 commented 4 years ago

I don't think there should be inconsistency between the way the 'via Circuit ID' is displayed as presented in these 3 examples. I understand the challenges with Example 1, however that behavior is going to cause confusion with users. In a way, the more complete your circuit connection model, the less information the GUI gives. That seems counter-intuitive.

Personally, I find the information that an interface is connected to a circuit more valuable than the distant-end interface (that may or may not exist) on the device detail page. If I need the distant-end information, then I could click the cable trace button to get that.

This would also cause conflict with https://github.com/netbox-community/netbox/issues/4619

[Edit] I realize you also mentioned in the above issue that you were working on it. Wasn't sure if this issue was a separate workstream altogether or not.

steffann commented 4 years ago

Personally, I find the information that an interface is connected to a circuit more valuable than the distant-end interface (that may or may not exist) on the device detail page. If I need the distant-end information, then I could click the cable trace button to get that.

This would be a major change to NetBox and goes beyond this cleanup issue. It would require a different implementation of tracing connections and determining the endpoint.

tyler-8 commented 4 years ago

This would be a major change to NetBox and goes beyond this cleanup issue. It would require a different implementation of tracing connections and determining the endpoint.

I'm not suggesting anything different than the way NetBox has displayed interfaces connected to circuits since ~the beginning of the project being open-sourced~ v1.8.0. Up until recently, NetBox has always shown 'Via CircuitXYZ' for an interface connected to a circuit, regardless of where the distant-end connection's modeling (circuit term, interface, etc) ended.

I was stating my preference of which information I found more helpful (from the historical behavior of NetBox) when looking at a device detail page vs the cable trace page.

Adding...

I understand this is a complex data model and also appreciate that you're working to make it better. I wanted to voice my concern about the change to displaying via inconsistently so we can hopefully continue the conversation in further refinements. Thanks for what you do!

steffann commented 4 years ago

What has changed is that cable traces ended at the circuit termination, and the assumption was made that every circuit would lead to one interface. However that assumption is not always true, for example when using multiplexed connections or multiple circuits after each other. The code was fixed to deal with that bad assumption, but the consequence (for now) is that the circuit isn't shown anymore.

I have ideas for improving this, but they require a major overhaul of the way we trace paths and store the useful data about them. It would definitely be nice, but I don't think it's feasible for now. There are too many other issues that need to be handled for the next releases.

wols commented 4 years ago

Please check a dependency with #4851

jeremystretch commented 4 years ago

Related to #4900

XioNoX commented 4 years ago

Thanks for working on that and clearly documenting the different behaviors. I think we're hitting 2 issues related to "Example 1". Please let me know if that's not the right place, or if I should provide more details.

Issue 1: In our infrastructure we have paths in the following form: Intf X +-----+ CT A +--+ Circuit C +--+ CT Z +-----+ Intf Y But some show up the (old?) way (as "via XXX" in the UI, and "connected_endpoint_type": "circuits.circuittermination" through the API. And others as they were directly connected (no "via XXX" and "connected_endpoint_type": "dcim.interface" through the API). So there are inconsistencies between paths that should be similar. Maybe it only happen for newly created paths?

Issue 2: Our automation relies on having "connected_endpoint_type": "circuits.circuittermination" when an interface is connected to a circuit but still continues all the way to a remote interface. If I understand correctly the "regression" happened to accommodate for more complex use-cases, although I was wondering if it would be possible to keep the same behavior for the current/simple use-cases.

jeremystretch commented 3 years ago

This has been addressed under #4900.