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

Add optional primary_ip on virtual machines initializer #301

Merged
merged 2 commits into from
Oct 18, 2020

Conversation

Tassatux
Copy link
Contributor

@Tassatux Tassatux commented May 22, 2020

New Behavior

Allow to set virtual machines primary ip via initializer.

Contrast to Current Behavior

primary_ip4 and primary_ip6 value on initializers/virtual_machines.yml are ignored.

Discussion: Benefits and Drawbacks

It add an easy way to set primary IP for VM as we could for devices.

Proposed Release Note Entry

Allow primary_ip in initializers #301

Add optional primary_ip on initializer

Double Check

  • I have read the comments and followed the PR template.
  • I have explained my PR according to the information in the comments.
  • My PR targets the develop branch.

Copy link
Collaborator

@cimnine cimnine left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. This looks very promising already and I will test it shortly.
In the meantime, could you add an example with primary_ipv4 and primary_ipv6 to one of the devices in devices.yml, please?

Please make sure that the information you add is consistent, i.e. that all the initializer scripts work afterwards. You can locally run ./test.sh to do that. It will uncomment all initializers and try to import them. (You might want to temporarily comment-out test_netbox_unit_tests to speed up the execution of test.sh.)

@cimnine cimnine added the enhancement The issue describes an enhancement that we would like to implement in the future. label Jun 2, 2020
@cimnine cimnine added this to the 0.25.0 milestone Jun 2, 2020
@Tassatux
Copy link
Contributor Author

Tassatux commented Jun 2, 2020

Adding primary_ipv4 and primary_ipv6 will require another update on startup script to create the IP on IPAM, as the IP startup script if currently loaded after because if may require VM or device to exist.
For now, in my use case I've added a startup script before devices/virtual_machines ones and which create an active IP address on IPAM for each primary_ip found in devices and virtual_machines initializers.

Do you want I add such startup script, for example 125_primary_ip.py or should I update 130_devices.py and 230_virtual_machines.py to get_or_create the primary IP if needed?

@cimnine
Copy link
Collaborator

cimnine commented Jun 4, 2020

You're saying that when the devices/virtual machines are created, the IP's are not yet created. Did I get this right? This makes sense.

Let me summarize my thoughts:

I believe a separate script would be in order that runs when both – the ip addresses and the devices/virtual machines – have been created.
It would read the primary_ipvX properties defined in devices.yml and virtual_machines.yml and would try to find the corresponding IPs in Netbox. If found, the primary_ip would be set, otherwise an error should be printed to the console.

Such a script would have to run after the 260_ip_addresses.py script. 125_primary_ip.py would be too early, as the IP addresses are not yet created at that stage. So it's filename would have to be 270_primary_ips.py or alike.

Did I miss something?

@Tassatux
Copy link
Contributor Author

Tassatux commented Jun 4, 2020

Yes, that's right, but it seem counter-intuitive to me at first, as I saw we could already use primary_ipX on devices initializer, but when I tried, it failed with ipam.models.DoesNotExist: IPAddress matching query does not exist.

I wondered if I could move the 260_ip_addresses.py script earlier, but as it could optionally get device/virtual machine to link them to IP address, there a kind of circular dependency.

I've pushed a script to link primary ips after the 260_ip_addresses.py script as you suggested.

startup_scripts/230_virtual_machines.py Outdated Show resolved Hide resolved
startup_scripts/130_devices.py Outdated Show resolved Hide resolved
@cimnine
Copy link
Collaborator

cimnine commented Jun 5, 2020

Thank you for this PR and your changes. I think this is good, but I hadn't had the time to run it locally. That's the only reason why it's not yet approved :)

@cimnine cimnine merged commit 2b361c4 into netbox-community:develop Oct 18, 2020
@cimnine cimnine modified the milestones: 0.25.0, 0.26.0 Oct 26, 2020
@cimnine cimnine mentioned this pull request Oct 26, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement The issue describes an enhancement that we would like to implement in the future.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants