allow to add ips to vrouter nics #565

Thiryn · 2024-07-29T14:30:43Z

Community Note

Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for PR followers and do not help prioritize the request

Description

References

closes #564

New or Affected Resource(s)

opennebula_virtual_router_nic

Checklist

I have created an issue and I have mentioned it in References
My code follows the style guidelines of this project (use go fmt)
My changes generate no new warnings or errors
I have updated the unit tests and they pass succesfuly
I have commented my code, particularly in hard-to-understand areas
I have updated the documentation (if needed)
I have updated the changelog file

Thiryn · 2024-07-29T14:31:08Z

Will update docs and changelog if this gets accepted

Thiryn · 2024-08-20T15:46:26Z

Found out the reason why the CI is failing, I didn't see the provider tests are creating 2 VRouter instance, and obviously we can't attach 2 non-floating IP to the instances.
What's the best way to tackle this in your opinion ? Should I isolate the NIC IP attachement test in a separate test file? I'm not familiar enough with the provider test to know if there is a way to delete a specific resource from a previous test step.

Scratch that, I didn't see that the test contained previous resources too, I think I should be able to update the test to cover the use cases correctly.

adapt the tests to ensure there is only one vrouter instance when creating nic with non-floating IPs

Thiryn · 2024-08-21T13:13:12Z

just putting this back as draft until I fix the tests entirely

…C IP assignation on vrouters

Thiryn · 2024-08-26T12:24:46Z

A couple things have changed:

The NIC delete function now waits on the leases to be freed. There is a sync issue in ON where even when the NIC gets freed, the IP lease isn't quite free for another few seconds. I made this specific error a retry-able error.
I added some tests:
- Testing with and without floating IP
- Removing the 2nd vrouter instance before attaching the first NIC, as we obviously can't assign a static IP to 2 different VRouter VMs
- Did some housekeeping and arranged the test terraform for that file in a uniform manner (4-spaces seemed to be the standard, I used that)

github-actions · 2024-09-26T00:11:36Z

This pull request is stale because it has been open for 30 days with no activity and it is not in a milestone. Remove 'status: stale' label or comment, or this will be closed in 5 days.

treywelsh

I took a quick look at the PR. Thanks for this contribution.
It's nice to have some tests in it.

Reworking the commit history would be nice:
Please take a look at master commit names, they're often prepended with an issue number and F for feature, B for bug (I tried to keep more or less then OpenNebula way to name the commits)
As a side note: formatting the acceptance test adds a bit of noise to the whole PR changes, maybe that should be in a separate commit but it's not a big deal

treywelsh · 2024-09-30T15:14:50Z

opennebula/resource_opennebula_virtual_router_test.go

+					resource.TestCheckResourceAttrSet("opennebula_virtual_router.test", "gid"),
+					resource.TestCheckResourceAttrSet("opennebula_virtual_router.test", "uname"),
+					resource.TestCheckResourceAttrSet("opennebula_virtual_router.test", "gname"),
+					resource.TestCheckResourceAttr("opennebula_virtual_router_nic.nic_floating_IP_specified", "floating_ip", "true"),


when snake case is used, keep it lowercase to be consistent

treywelsh · 2024-09-30T15:15:32Z

opennebula/resource_opennebula_virtual_router_nic.go

@@ -209,6 +219,13 @@ func resourceOpennebulaVirtualRouterNICRead(ctx context.Context, d *schema.Resou

 	floatingIP, _ := nic.GetStr("FLOATING_IP")
 	floatingOnly, _ := nic.GetStr("FLOATING_ONLY")
+	ip, _ := nic.Get("IP")
+
+	// For VRouter NICs, floating IPs are set using the "IP" field, but it is represented in the ON API


tiny typo: ONE

Hi! I believe this implementation suggestion is incorrect. If you look at implementation of a virtual machine https://registry.terraform.io/providers/OpenNebula/opennebula/latest/docs/resources/virtual_machine#nic there is the computed_ip "attribute" at the same time the ip is a "parameter". The VR NIC is somewhat similar and I believe it should follow the same convention. 🤔

I understand the need for consistency, but do we really want two fields, an ip to set and a computed_ip to get? It seems counterintuitive, and a bit of an anti-pattern.

treywelsh · 2024-09-30T15:20:56Z

opennebula/helpers_vr.go

@@ -101,6 +101,13 @@ func vrNICAttach(ctx context.Context, timeout time.Duration, controller *goca.Co
 		updatedNICsLoop:
 			for i, nic := range updatedNICs {

+				// For VRouter NICs, floating IPs are set using the "IP" field, but it is represented in the ON API


unless I miss something it seems that this whole code part in helpers_vr.go is useless ?

These few lines enable the IP to be set on the NIC resource for vrouter. On VRouter NICs that use floating IP, ON represents the IP in the VROUTER_IP field.
This loop is ran when updating NICs. Without this, when running terraform apply to update the NIC would yield an empty IP (and therefore trigger a recreation when not necessary)

treywelsh · 2024-09-30T15:24:58Z

opennebula/resource_opennebula_virtual_network.go

-		})
-		return diags
-	}
+	err = resource.RetryContext(ctx, d.Timeout(schema.TimeoutCreate), func() *resource.RetryError {


Did you ensured that prepending the Delete call with a waitForVNetworkState call wouldn't fix this ?
As a side note, you're using TimeoutCreate in the delete method here

It does in the tests. This was explicitly a problem with floating IPs if I remember. It happens that ON takes a little more time to release floating IPs and it was creating test failures (see one of such tests here https://github.com/Thiryn/terraform-provider-opennebula/actions/runs/10548061643/job/29221515518#step:10:575).

In reality, this fix should also be applied to the AR resources, but this isn't the point of this PR. The only reason the fix is limited to the network, is because we use the now-deprecated feature of AR entries in the vnet resources in our VR tests https://github.com/OpenNebula/terraform-provider-opennebula/blob/master/opennebula/resource_opennebula_virtual_router_test.go#L285-L289

sk4zuzu

https://github.com/OpenNebula/terraform-provider-opennebula/pull/565/files#r1802885865

Thiryn · 2024-10-29T16:50:44Z

@treywelsh sorry for the formatting, you're right it should be separate, do you want me to revert those changes?
With regard to the commit history, don't you usually do a squash merge? Meaning that the history of individual commits in the PR is 'lost' anyways as its all under a single commit on master?

I don't mind reworking the history, but please confirm that it is necessary before I do so.

Cheers,
Dorian

…tual networks

allow to add ips to vrouter nics

efefbd6

Thiryn mentioned this pull request Aug 20, 2024

allow to add ips to vrouter nics true/terraform-provider-opennebula#3

Merged

Update the test to have the IP range match the network AR

16684e0

Thiryn force-pushed the vrouter-nic-add-ip branch from 8611212 to 16684e0 Compare August 20, 2024 13:07

set the ip upon vrouter read and adapt tests

71630fa

adapt the tests to ensure there is only one vrouter instance when creating nic with non-floating IPs

Thiryn marked this pull request as draft August 21, 2024 13:12

Thiryn added 4 commits August 21, 2024 16:36

remove the test step that was deleting the second vrouter

e8ed893

add back mistakenly removed test step

482fea9

Add dependency to prevent the VRouter VM to be deleted after the NIC

0f79e87

Make network wait for leases to be free upon delete. Add tests for NI…

fbcf514

…C IP assignation on vrouters

Thiryn marked this pull request as ready for review August 26, 2024 12:24

github-actions bot added the status: stale label Sep 26, 2024

treywelsh removed the status: stale label Sep 26, 2024

treywelsh reviewed Sep 30, 2024

View reviewed changes

sk4zuzu self-assigned this Oct 16, 2024

sk4zuzu requested changes Oct 18, 2024

View reviewed changes

rsmontero added this to the 1.4.2 milestone Oct 21, 2024

Thiryn requested a review from sk4zuzu October 29, 2024 16:47

Thiryn requested a review from treywelsh October 29, 2024 16:50

update resource naming for consistency; fix wrong timeout used in vir…

eca6a1d

…tual networks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow to add ips to vrouter nics #565

allow to add ips to vrouter nics #565

Thiryn commented Jul 29, 2024 •

edited

Loading

Thiryn commented Jul 29, 2024

Thiryn commented Aug 20, 2024 •

edited

Loading

Thiryn commented Aug 21, 2024

Thiryn commented Aug 26, 2024

github-actions bot commented Sep 26, 2024

treywelsh left a comment

treywelsh Sep 30, 2024

treywelsh Sep 30, 2024

sk4zuzu Oct 16, 2024 •

edited

Loading

Thiryn Oct 29, 2024 •

edited

Loading

treywelsh Sep 30, 2024

Thiryn Oct 29, 2024

treywelsh Sep 30, 2024

Thiryn Oct 29, 2024

sk4zuzu left a comment

Thiryn commented Oct 29, 2024

allow to add ips to vrouter nics #565

Are you sure you want to change the base?

allow to add ips to vrouter nics #565

Conversation

Thiryn commented Jul 29, 2024 • edited Loading

Community Note

Description

References

New or Affected Resource(s)

Checklist

Thiryn commented Jul 29, 2024

Thiryn commented Aug 20, 2024 • edited Loading

Thiryn commented Aug 21, 2024

Thiryn commented Aug 26, 2024

github-actions bot commented Sep 26, 2024

treywelsh left a comment

Choose a reason for hiding this comment

treywelsh Sep 30, 2024

Choose a reason for hiding this comment

treywelsh Sep 30, 2024

Choose a reason for hiding this comment

sk4zuzu Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Thiryn Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

treywelsh Sep 30, 2024

Choose a reason for hiding this comment

Thiryn Oct 29, 2024

Choose a reason for hiding this comment

treywelsh Sep 30, 2024

Choose a reason for hiding this comment

Thiryn Oct 29, 2024

Choose a reason for hiding this comment

sk4zuzu left a comment

Choose a reason for hiding this comment

Thiryn commented Oct 29, 2024

Thiryn commented Jul 29, 2024 •

edited

Loading

Thiryn commented Aug 20, 2024 •

edited

Loading

sk4zuzu Oct 16, 2024 •

edited

Loading

Thiryn Oct 29, 2024 •

edited

Loading