Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IP addresses missing from virtual machine interfaces #334

Merged
merged 3 commits into from Sep 27, 2020

Conversation

haxorof
Copy link
Contributor

@haxorof haxorof commented Sep 9, 2020

Fixes #321

@haxorof
Copy link
Contributor Author

haxorof commented Sep 17, 2020

Fix not correct see referenced issue for comment. Will look into this.

@haxorof
Copy link
Contributor Author

haxorof commented Sep 17, 2020

Did a correction but happy to get some feedback on that. Feel that there might be some better way of doing it. 😄

@ThomasADavis
Copy link
Contributor

If I remember correctly, there is a pynetbox function to return the netbox API version, which should be safer than what you tried to do; ie:

>>> nb = pynetbox.api(
...     'http://localhost:8000',
...     private_key_file='/path/to/private-key.pem',
...     token='d6f4e314a5b5fefd164995169f28ae32d987704f'
... )
>>> nb.version
'2.6'
>>>```

this is really the correct way to version things.

@haxorof
Copy link
Contributor Author

haxorof commented Sep 17, 2020

@ThomasADavis Currently pynetbox is not used in the nb_inventory.py what I can see so your suggestion is to introduce that here? Feels that it can be a bit risky to introduce that dependency now.

@ThomasADavis
Copy link
Contributor

ThomasADavis commented Sep 17, 2020

A function to get the version of the netbox that nb_inventory is talking to should be added, for it's also probably going break on 'tags' also.

The netbox version is returned on every rest call as part of the header information; see netbox-community/netbox#1223

@FragmentedPacket it's up to you... the inventory use direct REST api calls.

@FragmentedPacket
Copy link
Contributor

Keeping it via the API calls is fine. I've been contemplating whether it's worth it to move away from pynetbox with some of the issues that have arisen over use of it in this project.

I'll take a look at this PR soon and provide any feedback once I've had time to actually take a good look at it.

Thanks for your work on this though!

interface["ip_addresses"] = list(
self.ipaddresses_lookup[interface["id"]].values()
)
if before_netbox_v29:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we even need to to determine if it's < 2.9?

Could we just have the two lookups, device and VM ip address lookups? So we handle the difference in versions in the code below by determining which bucket to throw them into?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look in the changes I did in refresh_ipaddresses then you see that for NetBox v2.9 and later I will know if it is an IP address is connected to a VM or device by looking at the assigned_object_type. But in earlier NetBox versions this information is not available like that to determine which bucket to throw them into. Or what kind of change do you suggest?

It would have been great if I just could have the the two lookups and skip the ipaddresses_lookup completely but as said I do not see that I have the information available in earlier version to put IPs in the two buckets.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I do see what you're saying now. And we can't act off anything else with the way the inventory is setup to differentiate.

I will approve and merge this in.

@FragmentedPacket FragmentedPacket merged commit b33e79c into netbox-community:devel Sep 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inventory - IP addresses missing from virtual machine interfaces
3 participants