Add support for IP reporting when NIC uses DHCP #283

Didainius · 2020-01-17T11:26:46Z

Part of https://github.com/terraform-providers/terraform-provider-vcd/issues/316

This PR adds the following functions:

EdgeGateway.GetAllNsxvDhcpLeases()
EdgeGateway.GetNsxvActiveDhcpLeaseByMac()
VM.GetParentVApp()
VM.GetParentVdc()
VM.WaitForDhcpIpByNicIndexes()

The main goal was to get VM.WaitForDhcpIpByNicIndexes() function working and satisfy the needs of Terraform provider's capability to report IP addresses for NICs using DHCP mode. It is quite tricky to get the IP because vCD cannot know in advance if VM has guest tools or is using a DHCP addressing.
For this function to work - at least one the following must be true:

VM has guest tools (vCD UI shows IP address). (Takes longer time)
VM DHCP interface is connected to routed Org network and is using Edge Gateway DHCP. (Takes less time, but is more constrained)

Because vCD cannot know if the above requirements are fulfilled, this function requires a user definable timeout as the user must know how much time does it usually take for the VM to report IP addresses in his environment.

Full test suite passes on 9.5 and 10.0 with a caveat of Test_UpdateInternalDisk which is to be fixed in #282

vbauzys

First scroll :)

govcd/vm.go

govcd/vm_dhcp_test.go

govcd/vm.go

govcd/vm_dhcp_test.go

types/v56/nsxv_types.go

dataclouder

Some small changes needed. Running more tests in the meantime

dataclouder · 2020-01-23T13:49:45Z

govcd/vm.go

+func (vm *VM) GetParentVdc() (*Vdc, error) {
+	vapp, err := vm.GetParentVApp()
+	if err != nil {
+		return nil, fmt.Errorf("could not find parent vApp: %s", err)


Suggested change

return nil, fmt.Errorf("could not find parent vApp: %s", err)

return nil, fmt.Errorf("could not find parent vApp for VM %s: %s", vm,VM.Name, err)

The error would be more useful if it provides some context, especially if it is issued during operations in Terraform.

dataclouder · 2020-01-23T13:50:13Z

govcd/vm.go

+
+	vdc, err := vapp.getParentVDC()
+	if err != nil {
+		return nil, fmt.Errorf("could not find parent vDC: %s", err)


Suggested change

return nil, fmt.Errorf("could not find parent vDC: %s", err)

return nil, fmt.Errorf("could not find parent vDC for VM %s: %s", vm,VM.Name, err)

dataclouder · 2020-01-23T13:56:43Z

govcd/vm.go

+
+// getEdgeGatewaysForRoutedNics checks if any NICs are using routed networks and are attached to
+// edge gateway
+func (vm *VM) getEdgeGatewaysForRoutedNics(nicDhcpConfigs []nicDhcpConfig) ([]nicDhcpConfig, error) {


Just wondering if the approach using FindEdgeGatewayNameByNetwork could be practicable here

it is being used deeper inside (in the function getEdgeGatewayNameForNic)

dataclouder · 2020-01-23T13:57:42Z

govcd/vm.go

+}
+
+// WaitForDhcpIpByNicIndexes accepts a slice of NIC indexes in VM, tries to get these IPs up to
+// maxWaitSeconds and the returns:


Suggested change

// maxWaitSeconds and the returns:

// maxWaitSeconds and then returns:

dataclouder · 2020-01-23T13:59:22Z

govcd/vm.go

+// partial result for IP addresses when the timeout is hit.
+//
+// Getting a DHCP address is complicated because vCD (in UI and in types.NetworkConnectionSection)
+// reports IP addresses when guest tools are present on a VM only. This function also attempts to


Suggested change

// reports IP addresses when guest tools are present on a VM only. This function also attempts to

// reports IP addresses only when guest tools are present on a VM. This function also attempts to

dataclouder · 2020-01-23T14:01:51Z

govcd/vm_dhcp_test.go

+// +build nsxv vm functional ALL
+
+/*
+ * Copyright 2019 VMware, Inc.  All rights reserved.  Licensed under the Apache v2 License.


Suggested change

* Copyright 2019 VMware, Inc. All rights reserved. Licensed under the Apache v2 License.

* Copyright 2020 VMware, Inc. All rights reserved. Licensed under the Apache v2 License.

dataclouder · 2020-01-23T14:02:10Z

govcd/nsxv_dhcplease.go

@@ -0,0 +1,69 @@
+/*
+ * Copyright 2019 VMware, Inc.  All rights reserved.  Licensed under the Apache v2 License.


Suggested change

* Copyright 2019 VMware, Inc. All rights reserved. Licensed under the Apache v2 License.

* Copyright 2020 VMware, Inc. All rights reserved. Licensed under the Apache v2 License.

dataclouder · 2020-01-23T14:05:06Z

govcd/vm_dhcp_test.go

+// getOrgVdcNetworkWithDhcp is a helper that creates a routed Org network and a DHCP pool with
+// single IP address to be assigned. Org vDC network and IP address assigned to DHCP pool are
+// returned
+func getOrgVdcNetworkWithDhcp(vcd *TestVCD, check *C, edgeGateway *EdgeGateway) (*types.OrgVDCNetwork, string) {


If this function creates something, it shouldn't be called get....
Perhaps makeOrgVdcNetworkWithDhcp ?

Moreover, the returned IP address is hard-coded. Should we pass it as a parameter instead?

This is actually not needed at all currently (I ignore this output using _ assignment). This is a leftover from development phase and I think it is not worth building a more complicated function (at least at this time).

dataclouder

LGTM

lvirbalas

LGTM!

This reverts commit 4df540f, reversing changes made to 7764172.

Didainius added 11 commits December 18, 2019 17:00

WIP

f09d91f

wip2-not-works

174c4e0

WIP - works

577494e

Use ticker for timed waiting

7992d54

Merge branch 'master' into dhcp_lease_lookup_ip

731a1c0

Before rewrite to struct

21348e2

POC works

0d6bb62

More tests, including timeout

97b1ac7

WIP

c6c43cb

Up

c400abc

Show timeout explicitly

de45660

Didainius self-assigned this Jan 17, 2020

Didainius added the enhancement New feature or request label Jan 17, 2020

vmwclabot added the cla-not-required label Jan 17, 2020

Didainius added 11 commits January 17, 2020 17:45

Remove comments

134105f

Cleanup, docs, public functions

b9f13a6

add docs to functions

58930b3

Docs

8dbad99

more docs

f640947

Extend default timeout

c49497b

Force customization to avoid impact of other tests

a7b43db

Undeploy before power off

bfa6dea

Fix tests, beautify output

5ea28ae

Increase timeouts

2dc62c7

Fix test

ef66399

Didainius requested review from vbauzys, dataclouder and lvirbalas January 21, 2020 05:53

Didainius marked this pull request as ready for review January 21, 2020 05:53

Spawn a VM for this test

f5701ac

Didainius added 4 commits January 21, 2020 07:59

Cleanup test

07a474c

Merge master

2b97df5

merge master

a6e2645

Improve comment

b2a8b7e

vbauzys suggested changes Jan 21, 2020

View reviewed changes

govcd/vm.go Show resolved Hide resolved

govcd/vm.go Outdated Show resolved Hide resolved

govcd/vm.go Outdated Show resolved Hide resolved

govcd/vm_dhcp_test.go Show resolved Hide resolved

govcd/vm_dhcp_test.go Show resolved Hide resolved

govcd/vm_dhcp_test.go Show resolved Hide resolved

Didainius added 2 commits January 22, 2020 08:13

Merge and update changelog

120f924

Add extra check, fix comment

ac1e37b

lvirbalas reviewed Jan 22, 2020

View reviewed changes

Didainius added 3 commits January 22, 2020 18:44

Fix new Travis error

1351ce0

Merge master

1d70eb3

Address comments

3fc7f94

dataclouder suggested changes Jan 23, 2020

View reviewed changes

Didainius added 3 commits January 23, 2020 16:18

Address comments

cbd9483

Remove unused testing function var

3870209

Change comment

67f899f

dataclouder approved these changes Jan 23, 2020

View reviewed changes

Improve comment

7764172

lvirbalas approved these changes Feb 5, 2020

View reviewed changes

vbauzys approved these changes Feb 6, 2020

View reviewed changes

Didainius added 6 commits February 14, 2020 14:13

Merge master

4df540f

Revert "Merge master"

0475942

This reverts commit 4df540f, reversing changes made to 7764172.

merge master

11f7cb3

Cleanup travis

135da1d

Add 'DHCP IP Lookup' keyword to all log messages for easier filtering

dc6109b

Change log format for DHCP lookup

3d6be75

Didainius merged commit 4375d22 into vmware:master Feb 19, 2020

Didainius deleted the dhcp_lease_lookup_ip branch February 19, 2020 06:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for IP reporting when NIC uses DHCP #283

Add support for IP reporting when NIC uses DHCP #283

Didainius commented Jan 17, 2020 •

edited

Loading

vbauzys left a comment

dataclouder left a comment

dataclouder Jan 23, 2020

Didainius Jan 23, 2020

dataclouder Jan 23, 2020

Didainius Jan 23, 2020

dataclouder Jan 23, 2020

Didainius Jan 23, 2020

dataclouder Jan 23, 2020

Didainius Jan 23, 2020

dataclouder Jan 23, 2020

Didainius Jan 23, 2020

dataclouder Jan 23, 2020

Didainius Jan 23, 2020

dataclouder Jan 23, 2020

Didainius Jan 23, 2020

dataclouder Jan 23, 2020

Didainius Jan 23, 2020

Didainius Jan 23, 2020

dataclouder left a comment

lvirbalas left a comment

	return nil, fmt.Errorf("could not find parent vApp: %s", err)
	return nil, fmt.Errorf("could not find parent vApp for VM %s: %s", vm,VM.Name, err)

	// maxWaitSeconds and the returns:
	// maxWaitSeconds and then returns:

	// reports IP addresses when guest tools are present on a VM only. This function also attempts to
	// reports IP addresses only when guest tools are present on a VM. This function also attempts to

	* Copyright 2019 VMware, Inc. All rights reserved. Licensed under the Apache v2 License.
	* Copyright 2020 VMware, Inc. All rights reserved. Licensed under the Apache v2 License.

		@@ -0,0 +1,69 @@
		/*
		* Copyright 2019 VMware, Inc. All rights reserved. Licensed under the Apache v2 License.

Add support for IP reporting when NIC uses DHCP #283

Add support for IP reporting when NIC uses DHCP #283

Conversation

Didainius commented Jan 17, 2020 • edited Loading

vbauzys left a comment

Choose a reason for hiding this comment

dataclouder 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

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

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

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

dataclouder left a comment

Choose a reason for hiding this comment

lvirbalas left a comment

Choose a reason for hiding this comment

Didainius commented Jan 17, 2020 •

edited

Loading