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

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 ?

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

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

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

treywelsh Sep 30, 2024

treywelsh Sep 30, 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

treywelsh Sep 30, 2024

Choose a reason for hiding this comment

treywelsh Sep 30, 2024

Choose a reason for hiding this comment

Thiryn commented Jul 29, 2024 •

edited

Loading

Thiryn commented Aug 20, 2024 •

edited

Loading