Added a trace property to the traceable interface classes #280

raddessi · 2020-08-01T01:00:21Z

I have this in a working state here, but the code is not solid enough I would feel happy about merging it. More I would like your feedback on how you would like to approach this. The main issue is the response_loader function needing to be able to handle multiple return_obj types since the trace endpoint returns multiple types of items from the api - cables, interfaces, front ports, console ports, console server ports, etc.

I saw some places where you already inspected the uri to determine object type so that is what I've done here.. I'm not married to it though. Thanks for all the work on this project! I love the module, it's been a huge help :)

raddessi · 2020-08-01T01:01:27Z

I would also like to add this for the other items that have trace methods, but most of those will need to be created in the dcim submodule first so I have not done that yet, pending your thoughts on how to proceed.

raddessi · 2020-08-01T01:05:03Z

pynetbox/core/endpoint.py

+                            )
+                        )
+                    else:
+                        # the last trace can consist of [cable_a, None, None] if there is no


copy pasta, I will remove

zachmoody · 2020-08-05T21:23:15Z

Interesting, thanks for the PR. I haven't really dug into the trace API yet. I feel like it's unique enough that I'd probably like to deal with it in the .trace property instead of messing with the more universal response_loader. We could call RODetailEndpoint and then return what we needed to from its return.

raddessi · 2020-08-06T16:58:30Z

Ah ok that makes good sense. I'll play around with it and send something over. Thanks!

raddessi

added some comments/questions

pynetbox/models/dcim.py

raddessi · 2020-08-20T18:11:33Z

pynetbox/models/dcim.py

+                elif url_path.startswith("/api/dcim/rear-ports"):
+                    return_obj_class = RearPorts
+                else:
+                    raise NotImplementedError(


is this how you would want to handle this situation?

I'd probably make a dict here that mapped the endpoint name it might encounter with the custom object instead of an if/else tree (really wish switches were a thing in python). e.g.

{ "cables": Cables, "front-ports": FrontPorts, ... }

You'll probably want to add some other endpoint/objects you might come across in the traces as well like ConsolePort/ConsoleServerPort and PowerPort/Outlets. I'd probably default to just a simple Record object at the end of it instead of raising.

Perfect. I'll update to a map and return a Record instead of raising. It's been a while.. I think I verified all possible objects returned are listed here but I'll check once more, good call.

raddessi · 2020-08-20T18:23:40Z

re: the failing tests.. yeah I have never seen python complain that self actually needs to be passed back in to a function when calling it but I got this error when testing locally and that seemed to fix it in my case:

~/dev/pynetbox/pynetbox/core/response.py in __repr__(self)
    215 
    216     def __repr__(self):
--> 217         return str(self)
    218 
    219     def __getstate__(self):

~/dev/pynetbox/pynetbox/models/dcim.py in __str__(self)
    227             termination_a_name = self.termination_a.name
    228         except AttributeError:
--> 229             self.termination_a.full_details()
    230             termination_a_name = self.termination_a.name
    231 

TypeError: full_details() missing 1 required positional argument: 'self'

I'll dig in to why it's wanting that locally and fix that issue. Comments would still be appreciated, thanks!

I'm not sure if I fixed it in a proper way though

raddessi

I will add tests for the trace property if this looks good so far

raddessi · 2020-08-21T00:13:08Z

pynetbox/models/dcim.py

+        try:
+            termination_a_name = self.termination_a.name
+        except AttributeError:
+            try:
+                self.termination_a.full_details()
+            except TypeError:
+                self.termination_a.full_details(self)
+            termination_a_name = getattr(self.termination_a, "name", self.termination_a)


I don't really like this logic but I'm not sure of a better method. Ideas welcomed

What's the goal here? Both of those should be Termination objects which return .name (assuming they're not a circuit) anyways. I suspect this might be the source of the excessive calls to NetBox in your tests.

This was a change to force each of the returned items along the trace to get fully populated so that when you printed their name in the trace it wasn't just displayed as a Record item with no data. Once each item was accessed they would then print their name properly, but having a trace full of unlabeled hops was not very useful. It's not very clear, and I don't like it at all. I think it may be better to do something like this in the trace function itself (I think maybe you mentioned that before?) so that only on a trace will it try to prefetch the data of each item in the hops so they print out nicely.

EDIT: When I said It's not very clear, and I don't like it at all. I was referring to my first attempt to patch this :)

That's weird. Do you have an example you can show of the old behavior? Those should be Termination objects, but either way even Record's default __str__ behavior is to return .name, iirc.

Sure, with this section of code reverted this is what I'm getting now:

In [1]: iface.trace() Out[1]: [[em1, <class 'pynetbox.models.dcim.Termination'> <> <class 'pynetbox.models.dcim.Termination'>, pair-11 (ports 21-22)], [port-2, <class 'pynetbox.models.dcim.Termination'> <> <class 'pynetbox.models.dcim.Termination'>, port-2], [pair-11 (ports 21-22), <class 'pynetbox.models.dcim.Termination'> <> <class 'pynetbox.models.dcim.Termination'>, Ethernet11]]

So it looks like maybe I was mistaken or confused about only the middle (not the first or last) having the issue, it may be all of the items returned from the trace until they are accessed in some way. You can see here how the info is filled out once the item is accessed directly:

In [2]: trace = iface.trace() In [3]: trace Out[3]: [[em1, <class 'pynetbox.models.dcim.Termination'> <> <class 'pynetbox.models.dcim.Termination'>, pair-11 (ports 21-22)], [port-2, <class 'pynetbox.models.dcim.Termination'> <> <class 'pynetbox.models.dcim.Termination'>, port-2], [pair-11 (ports 21-22), <class 'pynetbox.models.dcim.Termination'> <> <class 'pynetbox.models.dcim.Termination'>, Ethernet11]] In [4]: trace[0][1] Out[4]: em1 <> pair-11 (ports 21-22) In [5]: trace Out[5]: [[em1, em1 <> pair-11 (ports 21-22), pair-11 (ports 21-22)], [port-2, <class 'pynetbox.models.dcim.Termination'> <> <class 'pynetbox.models.dcim.Termination'>, port-2], [pair-11 (ports 21-22), <class 'pynetbox.models.dcim.Termination'> <> <class 'pynetbox.models.dcim.Termination'>, Ethernet11]]

You are correct, they are Termination objects. I'm sorry it's been a while since I worked on this so I've forgotten a little of the details. It seems like none of the cable connection object information at all is set until the item is accessed directly for some reason though. I have not dug in to that much yet.

EDIT: Actually to be more clear, it seems like the Termination objects referenced by the Cable connection object are not being loaded until the Cable object is itself referenced directly. The Cable object is always printing in the correct format, just the Termination objects inside of it have no data in them yet.

By moving this to inside the trace function it is muuuuch clearer now.

raddessi · 2020-08-21T16:48:28Z

tests/test_dcim.py

+            mock.assert_has_calls(
+                [
+                    call(
+                        "http://localhost:8000/api/{}/{}/1/".format(
+                            self.app, self.name.replace("_", "-")
+                        ),
+                        headers=HEADERS,
+                        params={},
+                        json=None,
+                    ),
+                    call(
+                        "http://localhost:8000/api/{}/{}/1/".format(
+                            "circuits", "circuit-terminations"
+                        ),
+                        headers=HEADERS,
+                        params={},
+                        json=None,
+                    ),
+                    call(
+                        "http://localhost:8000/api/{}/{}/1/".format(
+                            "circuits", "circuit-terminations"
+                        ),
+                        headers=HEADERS,
+                        params={},
+                        json=None,
+                    ),
+                ]


I also don't love the idea that each call for a cable will now result in 3 calls.. I didn't realize when fixing this test that that is essentially what changed in this test. It doesn't feel right that this needs to happen for each cable returned.. I want to dig in to this because I think this should only be needed for the cables between either the first or last cable returned in a trace since those are the only ones I have ever seen that exhibited the problem of their names not being initialized until you actually interact with the cable object (then it calling it's full_details method) so it seems wrong that this would be called 3 times for this simple query. I'll dig later when I have time.

Alternatively.. we could move this logic to the trace property and have it done there only for cables returned from traces but that feels less consistent somehow. I'm on the fence.

Yeah, have a look at the previous comment, the __str__() method might have something to do with it. Either way, we'd definitely want some tests that are specifically testing .trace.

Yeah. I'll also add testing with the next round of changes.

raddessi · 2020-09-16T17:09:19Z

@zachmoody do you have any thoughts?

zachmoody · 2020-09-16T17:12:44Z

@zachmoody do you have any thoughts?

Yeah, been meaning to review it, sorry to leave you hanging. Will do my best to get to it this week.

raddessi · 2020-09-16T17:17:28Z

No worries, this is working as needed for my current work just fine so as long as it's still on your radar I'm happy. Thanks :)

zachmoody

Great progress, made some comments below. Was thinking about maybe changing this from a property to a method, it's a bit of a nit but you only get one shot at getting that part right without cutting a new release 😄.

zachmoody · 2020-09-23T23:36:27Z

pynetbox/models/dcim.py

+        try:
+            termination_a_name = self.termination_a.name
+        except AttributeError:
+            try:
+                self.termination_a.full_details()
+            except TypeError:
+                self.termination_a.full_details(self)
+            termination_a_name = getattr(self.termination_a, "name", self.termination_a)


What's the goal here? Both of those should be Termination objects which return .name (assuming they're not a circuit) anyways. I suspect this might be the source of the excessive calls to NetBox in your tests.

pynetbox/models/dcim.py

zachmoody · 2020-09-24T00:05:52Z

pynetbox/models/dcim.py

+                elif url_path.startswith("/api/dcim/rear-ports"):
+                    return_obj_class = RearPorts
+                else:
+                    raise NotImplementedError(


I'd probably make a dict here that mapped the endpoint name it might encounter with the custom object instead of an if/else tree (really wish switches were a thing in python). e.g.

{ "cables": Cables, "front-ports": FrontPorts, ... }

You'll probably want to add some other endpoint/objects you might come across in the traces as well like ConsolePort/ConsoleServerPort and PowerPort/Outlets. I'd probably default to just a simple Record object at the end of it instead of raising.

pynetbox/models/dcim.py

zachmoody · 2020-09-24T00:09:12Z

tests/test_dcim.py

+            mock.assert_has_calls(
+                [
+                    call(
+                        "http://localhost:8000/api/{}/{}/1/".format(
+                            self.app, self.name.replace("_", "-")
+                        ),
+                        headers=HEADERS,
+                        params={},
+                        json=None,
+                    ),
+                    call(
+                        "http://localhost:8000/api/{}/{}/1/".format(
+                            "circuits", "circuit-terminations"
+                        ),
+                        headers=HEADERS,
+                        params={},
+                        json=None,
+                    ),
+                    call(
+                        "http://localhost:8000/api/{}/{}/1/".format(
+                            "circuits", "circuit-terminations"
+                        ),
+                        headers=HEADERS,
+                        params={},
+                        json=None,
+                    ),
+                ]


Yeah, have a look at the previous comment, the __str__() method might have something to do with it. Either way, we'd definitely want some tests that are specifically testing .trace.

raddessi

Thanks for all the great feedback. I'm sorry I dropped off the map, life got in the way.

Was thinking about maybe changing this from a property to a method, it's a bit of a nit but you only get one shot at getting that part right without cutting a new release

I'll definitely change this from a property to a method in the next update as well unless I hear back from you, I think that's probably better anyway but I was trying to match styles.

raddessi · 2020-12-01T21:59:14Z

pynetbox/models/dcim.py

+                elif url_path.startswith("/api/dcim/rear-ports"):
+                    return_obj_class = RearPorts
+                else:
+                    raise NotImplementedError(


Perfect. I'll update to a map and return a Record instead of raising. It's been a while.. I think I verified all possible objects returned are listed here but I'll check once more, good call.

pynetbox/models/dcim.py

raddessi · 2020-12-01T22:04:42Z

pynetbox/models/dcim.py

+        try:
+            termination_a_name = self.termination_a.name
+        except AttributeError:
+            try:
+                self.termination_a.full_details()
+            except TypeError:
+                self.termination_a.full_details(self)
+            termination_a_name = getattr(self.termination_a, "name", self.termination_a)


This was a change to force each of the returned items along the trace to get fully populated so that when you printed their name in the trace it wasn't just displayed as a Record item with no data. Once each item was accessed they would then print their name properly, but having a trace full of unlabeled hops was not very useful. It's not very clear, and I don't like it at all. I think it may be better to do something like this in the trace function itself (I think maybe you mentioned that before?) so that only on a trace will it try to prefetch the data of each item in the hops so they print out nicely.

EDIT: When I said It's not very clear, and I don't like it at all. I was referring to my first attempt to patch this :)

raddessi · 2020-12-01T22:06:02Z

tests/test_dcim.py

+            mock.assert_has_calls(
+                [
+                    call(
+                        "http://localhost:8000/api/{}/{}/1/".format(
+                            self.app, self.name.replace("_", "-")
+                        ),
+                        headers=HEADERS,
+                        params={},
+                        json=None,
+                    ),
+                    call(
+                        "http://localhost:8000/api/{}/{}/1/".format(
+                            "circuits", "circuit-terminations"
+                        ),
+                        headers=HEADERS,
+                        params={},
+                        json=None,
+                    ),
+                    call(
+                        "http://localhost:8000/api/{}/{}/1/".format(
+                            "circuits", "circuit-terminations"
+                        ),
+                        headers=HEADERS,
+                        params={},
+                        json=None,
+                    ),
+                ]


Yeah. I'll also add testing with the next round of changes.

zachmoody · 2020-12-02T14:27:56Z

Thanks for all the great feedback. I'm sorry I dropped off the map, life got in the way.

Ha, no worries, been in the same boat myself.

raddessi

OK I think this is much better.

tests/fixtures/dcim/interface_trace.json

tests/test_dcim.py

raddessi · 2020-12-07T17:54:37Z

Right on! Is there anything left you would like done here then? If not then I feel like it's ready to go in.

raddessi · 2020-12-09T02:01:46Z

Hold on this actually, I broke functionality in the last update. Will fix asap.

raddessi

another change

pynetbox/models/dcim.py

raddessi · 2020-12-09T02:18:54Z

OK that should be resolved now. I'm not sure why I had the else statement in there to begin with.

zachmoody · 2020-12-09T14:20:05Z

Nice work! I'd like to put it through its paces as well before merging. This month's pretty crazy for me, but I should be able to get to it before month's end.

raddessi · 2020-12-09T17:42:22Z

Yeah ok no problem. Do you have any plans for an integration testing suite?

zachmoody · 2020-12-26T15:40:57Z

Yeah ok no problem. Do you have any plans for an integration testing suite?

No serious plans, but that'd be an improvement over the fixtures we're using now. 🤔

raddessi · 2020-12-27T22:56:53Z

Right on.. how would you feel about me starting a WIP PR to try to set up a basic system using pytest-docker then? If its beyond what you want to deal with right now that's fine it can wait until some point in the future, no big deal.

markkuleinio · 2020-12-28T05:58:27Z

A side note: does the proposed code work if the NetBox server address is like https://appserver.example.com/netbox? The URL paths will then be /netbox/api/dcim/something/something.

Checking because we fixed that kind of issue somewhere else in the code recently.

raddessi · 2020-12-28T06:40:42Z

Oh now that is a great point. You're right that would break this. I'll amend asap.

markkuleinio · 2020-12-28T10:30:04Z

@zachmoody An idea (that you maybe already had in the earlier discussion): maybe we should parse and store the "NetBox path" in an Api instance attribute or something like that? To prevent the need to parse the same thing in several places.

Edit: actually normalizing the base URL (= drop http:80 or https:443) should be quite enough, to make it possible to just remove the start of the URL and continue there, if I'm correct.

zachmoody · 2020-12-28T16:05:19Z

Right on.. how would you feel about me starting a WIP PR to try to set up a basic system using pytest-docker then? If its beyond what you want to deal with right now that's fine it can wait until some point in the future, no big deal.

Yeah, go for it. Was thinking about tackling it myself once I cleared out some of the PR backlog. Will try and help out once you get a PR going, assuming I don't run into any surprises this week.

raddessi · 2020-12-28T19:06:15Z

This is actually much better as I didn't before realize that the uri I needed is already being passed in as self.endpoint.url, so this issue should be fixed now and is less confusing as well.

zachmoody · 2020-12-29T01:06:29Z

This is actually much better as I didn't before realize that the uri I needed is already being passed in as self.endpoint.url, so this issue should be fixed now and is less confusing as well.

Nice, been playing around with your PR today. I noticed this change caused the Cable objects in the trace to start coming across as generic Records. Might be worth testing that in some unit tests.

Also been thinking about ways we can avoid requiring additional calls to the cables endpoint since we only get a summarized cable object in the trace response. I'm leaning towards adjusting the Cable object's str method to return something that doesn't require terminations be present. E.g.

def __str__(self):
    if all(["name" in dict(i) for i in (self.termination_a, self.termination_b)]):
        return "{} <> {}".format(self.termination_a, self.termination_b)
    return "Cable #{}".format(self.id)

That's a little more inline with the UI experience, and would be more useful in any other situations (if they exist) where a cable object is sent without terminations. Though if anyone has a better approach, I'm all for it. Just don't like making any more calls to NetBox than are necessary.

raddessi · 2020-12-29T06:03:32Z

Nice, been playing around with your PR today. I noticed this change caused the Cable objects in the trace to start coming across as generic Records. Might be worth testing that in some unit tests.

Oh wow, ok I did not see that yet. Yeah I'll try to play around with it to reproduce.

Also been thinking about ways we can avoid requiring additional calls to the cables endpoint since we only get a summarized cable object in the trace response.

That is really nice since as you said it does match with the ui experience. I like it, if we need the full info for a report we can just query the terminations instead of printing the cable. Muuuch cleaner. Also, many fewer calls like you said - it's an all around win. Feel free to push up unless you want me to.

FYI I sent in a PR for integration testing at #315 - this change may be something that be integration tested especially with the upcoming cable tracing changes..?

When we're processing an object from the traces detail route it doesn't include a/b terminations. This commit changes the str representation of cable objects when that's the case.

If termination's are unintialized we won't be able to call them in dict(). So instead check to see if they're Termination types.

zachmoody · 2020-12-29T17:36:38Z

Oh wow, ok I did not see that yet. Yeah I'll try to play around with it to reproduce.

Ok, this is fixed up in 4a0d8ca.

That is really nice since as you said it does match with the ui experience. I like it, if we need the full info for a report we can just query the terminations instead of printing the cable. Muuuch cleaner. Also, many fewer calls like you said - it's an all around win. Feel free to push up unless you want me to.

.trace() output looks like this now.

In [28]: x.trace()
Out[28]: [[eth2, Cable #2, fp0], [rp0, Cable #5, rp0], [fp0, Cable #4, eth2]]

Calling x.trace()[1] or x.trace()[1].full_details() will populate the terminations, but that's kind of a round about way of getting data that's already in x.trace()[0] and x.trace()[2].

FYI I sent in a PR for integration testing at #315 - this change may be something that be integration tested especially with the upcoming cable tracing changes..?

Nice! Had a cursory look, seemed off to a good start. Hope to dig into it more later.

Added a trace method to the Interfaces class

af4a35e

raddessi commented Aug 1, 2020

View reviewed changes

Redesigned cable trace functionality

0ff839f

raddessi commented Aug 20, 2020

View reviewed changes

raddessi changed the title ~~WIP: Added a trace method to the Interfaces class~~ Added a trace method to the Interfaces class Aug 20, 2020

raddessi added 3 commits August 20, 2020 12:26

Re-blacked the ocde and fixed an instanciation issue

c8cc839

I'm not sure if I fixed it in a proper way though

Fixed a test failure

74b6e53

Updated pytest tests

d020bcc

raddessi commented Aug 21, 2020

View reviewed changes

Re-blacked after updating my local black module

c670197

raddessi commented Aug 21, 2020

View reviewed changes

raddessi changed the title ~~Added a trace method to the Interfaces class~~ Added a trace property to the traceable interface classes Aug 22, 2020

zachmoody self-requested a review September 2, 2020 02:26

zachmoody suggested changes Sep 24, 2020

View reviewed changes

raddessi commented Dec 1, 2020

View reviewed changes

raddessi added 4 commits December 2, 2020 18:54

Updated with changes from last review and added dedicated test

55cacaf

Reverted some now-unused additions

bb5ef86

Removed more dead code

754633c

Added missing newline

cc5210d

raddessi commented Dec 3, 2020

View reviewed changes

tests/fixtures/dcim/interface_trace.json Show resolved Hide resolved

tests/test_dcim.py Outdated Show resolved Hide resolved

raddessi requested a review from zachmoody December 3, 2020 02:03

raddessi commented Dec 9, 2020

View reviewed changes

pynetbox/models/dcim.py Outdated Show resolved Hide resolved

Fixed a logic issue and removed a debug statement

0a6e0a8

Fixed logic bug regarding the api url path

8d69e4a

zmoody added 5 commits December 29, 2020 10:13

Update str method on Cables

d8acca0

When we're processing an object from the traces detail route it doesn't include a/b terminations. This commit changes the str representation of cable objects when that's the case.

Remove population of cable str repr

c8b91e3

Fix str repr on Cables method

997854f

If termination's are unintialized we won't be able to call them in dict(). So instead check to see if they're Termination types.

Fix object url-to-class parsing

4a0d8ca

Don't look for termination data for cables in trace test

e6d2b78

zachmoody approved these changes Dec 29, 2020

View reviewed changes

zachmoody merged commit acb7670 into netbox-community:master Dec 29, 2020

raddessi deleted the master.cable-traces branch December 29, 2020 21:07

Added a trace property to the traceable interface classes #280

Added a trace property to the traceable interface classes #280

Conversation

raddessi commented Aug 1, 2020

raddessi commented Aug 1, 2020

Choose a reason for hiding this comment

zachmoody commented Aug 5, 2020

raddessi commented Aug 6, 2020

raddessi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raddessi commented Aug 20, 2020

raddessi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raddessi Dec 1, 2020 • edited

Choose a reason for hiding this comment

zachmoody Dec 2, 2020 • edited

Choose a reason for hiding this comment

raddessi Dec 3, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raddessi commented Sep 16, 2020

zachmoody commented Sep 16, 2020

raddessi commented Sep 16, 2020 • edited

zachmoody left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raddessi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raddessi Dec 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachmoody commented Dec 2, 2020

raddessi left a comment

Choose a reason for hiding this comment

raddessi commented Dec 7, 2020

raddessi commented Dec 9, 2020

raddessi left a comment

Choose a reason for hiding this comment

raddessi commented Dec 9, 2020

zachmoody commented Dec 9, 2020

raddessi commented Dec 9, 2020

zachmoody commented Dec 26, 2020

raddessi commented Dec 27, 2020

markkuleinio commented Dec 28, 2020

raddessi commented Dec 28, 2020

markkuleinio commented Dec 28, 2020 • edited

zachmoody commented Dec 28, 2020

raddessi commented Dec 28, 2020

zachmoody commented Dec 29, 2020

raddessi commented Dec 29, 2020

zachmoody commented Dec 29, 2020

raddessi Dec 1, 2020 •

edited

zachmoody Dec 2, 2020 •

edited

raddessi Dec 3, 2020 •

edited

raddessi commented Sep 16, 2020 •

edited

raddessi Dec 1, 2020 •

edited

markkuleinio commented Dec 28, 2020 •

edited