Add external network #185

vbauzys · 2019-04-30T11:06:49Z

Based on Megalord work: #159

Related issue: (#129 )

Add methods for creating, getting and deleting an external network.

Signed-off-by: Vaidotas Bauzys <[email protected]>

Improved tests Signed-off-by: Vaidotas Bauzys <[email protected]>

lvirbalas

Very awaited feature! Few inline comments and one over here:

Please add CHANGELOG.md entry.

govcd/sample_govcd_test_config.json

types/v56/constants.go

types/v56/types.go

lvirbalas · 2019-04-30T11:42:46Z

govcd/extension_test.go

+	for i := 0; i < 30; i++ {
+		err = newExternalNetwork.Refresh()
+		check.Assert(err, IsNil)
+		if len(newExternalNetwork.ExternalNetwork.Tasks.Task) == 0 {


What kind of task are we waiting to finish here?

Creation task. I have spent many hours tracking the issue - seems in home lab, create task returns it's finished, but in reality it isn't for second or so, so had to add additional wait for checking if task finished.

Maybe it is because of the data race issue mentioned above.

@vbauzysvmware what do you think with respect to @Didainius comment above? I'm concerned these lines in current test may be masking a potentially bigger issue.

Race condition was due test and http handling go procedures. It is no implicate this case

Signed-off-by: Vaidotas Bauzys <[email protected]>

Didainius

Didn't check everything yet but there are some immediate concerns about race condition problems.

Didainius · 2019-04-30T12:26:22Z

types/v56/types.go

@@ -474,8 +475,8 @@ type Task struct {
 // Since: 0.9
 type CapacityWithUsage struct {
 	Units     string `xml:"Units"`
-	Allocated int64  `xml:"Allocated,omitempty"`
-	Limit     int64  `xml:"Limit,omitempty"`
+	Allocated int64  `xml:"Allocated"`


Any specific reason for these changes?

Megalord changed, I think it is required field. Can be verified..

Updated according API

Didainius · 2019-04-30T12:38:06Z

govcd/sample_govcd_test_config.yaml

@@ -77,6 +77,15 @@ vcd:
    # An external Network name
    externalNetwork: myexternalnet
    #
+    # A port group name for creating an external network


It's an ID, not a name. I guess this is missing updated after refactoring.

I already updated that

govcd/extension_test.go

Didainius · 2019-04-30T12:58:27Z

govcd/extension_test.go

+	for i := 0; i < 30; i++ {
+		err = newExternalNetwork.Refresh()
+		check.Assert(err, IsNil)
+		if len(newExternalNetwork.ExternalNetwork.Tasks.Task) == 0 {


Maybe it is because of the data race issue mentioned above.

Signed-off-by: Vaidotas Bauzys <[email protected]>

Didainius · 2019-05-02T06:20:25Z

govcd/extension_test.go

+	}
+	task, err := CreateExternalNetwork(vcd.client, externalNetwork)
+	check.Assert(err, IsNil)
+	check.Assert(task, Not(Equals), Task{})


To avoid the triggering data race discussed above (because http.Client does some cleanups inside) I'd suggest to change this line to such:

Suggested change

check.Assert(task, Not(Equals), Task{})

check.Assert(task.Task, Not(Equals), types.Task{})

Signed-off-by: Vaidotas Bauzys <[email protected]>

govcd/extension.go

govcd/extension_test.go

govcd/externalnetwork.go

Signed-off-by: Vaidotas Bauzys <[email protected]>

dataclouder

Some more changes

govcd/externalnetwork.go

govcd/system.go

types/v56/types.go

Signed-off-by: Vaidotas Bauzys <[email protected]>

govcd/api_vcd_test.go

Signed-off-by: Vaidotas Bauzys <[email protected]>

govcd/system.go

Signed-off-by: Vaidotas Bauzys <[email protected]>

TESTING.md

govcd/extension_test.go

govcd/extension.go

govcd/externalnetwork_test.go

Signed-off-by: Vaidotas Bauzys <[email protected]>

Didainius

LGTM

Tests green on vCD9.5
ok github.com/vmware/go-vcloud-director/v2/govcd 1568.423s

Didainius · 2019-05-08T10:14:15Z

types/v56/types.go

 	Reserved  int64  `xml:"Reserved,omitempty"`
 	Used      int64  `xml:"Used,omitempty"`
-	Overhead  int64  `xml:"Overhead,omitempty"`
+	Overhead  int64  `xml:"Overhead,omitempty"` // not available anymore from API v30.0


More of a question - we're using v27.0, how did you spot v30.0? Was it only in docs or did you hit it somehow?

Accidentally spotted checking against API:) from time to time I shuffle between versions

megalord and others added 13 commits April 9, 2019 14:51

Create and delete for external networks

a94bd97

Fix errors in marshalled xml

0194be4

document structs; shuffle methods

b728d8c

Move get by name method to external network struct

65eb459

make query virtual centers method public

e753b02

Change import path after rebase with go modules

4d6e3fe

address feedback

94014b4

check more properties in create exnet test

80bdd28

Make new method for getting an external network

399b351

change method name

8091420

change deprecation comment

f4845c5

Add implementation needed for external network creation

2a339ab

Signed-off-by: Vaidotas Bauzys <[email protected]>

Made Ipscopes support multi scopes

5214fd7

Improved tests Signed-off-by: Vaidotas Bauzys <[email protected]>

vbauzys requested review from Didainius, dataclouder and lvirbalas and removed request for dataclouder April 30, 2019 11:06

vmwclabot added the cla-not-required label Apr 30, 2019

vbauzys self-assigned this Apr 30, 2019

vbauzys requested a review from dataclouder April 30, 2019 11:07

vbauzys mentioned this pull request Apr 30, 2019

Create and delete for external networks #159

Closed

lvirbalas requested changes Apr 30, 2019

View reviewed changes

vbauzys added 2 commits April 30, 2019 14:59

Fix comment and add changelog message

6969c2c

Signed-off-by: Vaidotas Bauzys <[email protected]>

Updated configuration samples

bde21a0

Signed-off-by: Vaidotas Bauzys <[email protected]>

Didainius requested changes Apr 30, 2019

View reviewed changes

Fix cleanup name

a697b3d

Signed-off-by: Vaidotas Bauzys <[email protected]>

Didainius reviewed May 2, 2019

View reviewed changes

vbauzys added 2 commits May 2, 2019 09:24

Fix for race check error

29f409b

Signed-off-by: Vaidotas Bauzys <[email protected]>

Add check to require sys admin

460c1d1

Signed-off-by: Vaidotas Bauzys <[email protected]>

vbauzys mentioned this pull request May 2, 2019

Add external network resource vmware/terraform-provider-vcd#230

Merged

dataclouder reviewed May 6, 2019

View reviewed changes

govcd/extension.go Show resolved Hide resolved

dataclouder reviewed May 6, 2019

View reviewed changes

govcd/extension_test.go Outdated Show resolved Hide resolved

dataclouder reviewed May 6, 2019

View reviewed changes

govcd/externalnetwork.go Outdated Show resolved Hide resolved

vbauzys added 3 commits May 6, 2019 11:12

Merge branch 'master' into add-external-network

2bbaf9b

Added tags for tests according latest PR

d24c0bf

Signed-off-by: Vaidotas Bauzys <[email protected]>

Fix error message

59e1029

Signed-off-by: Vaidotas Bauzys <[email protected]>

dataclouder suggested changes May 6, 2019

View reviewed changes

govcd/externalnetwork.go Outdated Show resolved Hide resolved

govcd/system.go Show resolved Hide resolved

govcd/system.go Outdated Show resolved Hide resolved

govcd/system.go Outdated Show resolved Hide resolved

types/v56/types.go Outdated Show resolved Hide resolved

vbauzys added 6 commits May 6, 2019 11:36

Add test tags

9f69a35

Signed-off-by: Vaidotas Bauzys <[email protected]>

Remove extension tag, add missing extnetwork tags

d388f3f

Signed-off-by: Vaidotas Bauzys <[email protected]>

Add elaborated comment

21074b9

Signed-off-by: Vaidotas Bauzys <[email protected]>

Fix groups -> group

c102069

Signed-off-by: Vaidotas Bauzys <[email protected]>

Improve comment

4a78b93

Signed-off-by: Vaidotas Bauzys <[email protected]>

Removed getByName function, added more generic one

e8df197

Signed-off-by: Vaidotas Bauzys <[email protected]>

dataclouder reviewed May 6, 2019

View reviewed changes

govcd/api_vcd_test.go Show resolved Hide resolved

vbauzys added 2 commits May 6, 2019 15:49

Fixed type according vCD API

5778d54

Signed-off-by: Vaidotas Bauzys <[email protected]>

Fixed race condition

17e426f

Signed-off-by: Vaidotas Bauzys <[email protected]>

dataclouder reviewed May 6, 2019

View reviewed changes

govcd/system.go Outdated Show resolved Hide resolved

Fix grammar

2a5141e

Signed-off-by: Vaidotas Bauzys <[email protected]>

Didainius reviewed May 7, 2019

View reviewed changes

vbauzys added 7 commits May 7, 2019 11:09

Fix comments

bea812e

Signed-off-by: Vaidotas Bauzys <[email protected]>

Fix mistype

4bdd2f6

Signed-off-by: Vaidotas Bauzys <[email protected]>

Fix comment

ada5223

Signed-off-by: Vaidotas Bauzys <[email protected]>

Move tests from extension

4a4302a

Signed-off-by: Vaidotas Bauzys <[email protected]>

Moved functions to system.go

a548ed9

Signed-off-by: Vaidotas Bauzys <[email protected]>

Add comment for function and enforce fence mode to creat ext net

009f59c

Signed-off-by: Vaidotas Bauzys <[email protected]>

Improve comment

010e2e8

Signed-off-by: Vaidotas Bauzys <[email protected]>

Didainius approved these changes May 8, 2019

View reviewed changes

lvirbalas approved these changes May 8, 2019

View reviewed changes

dataclouder approved these changes May 8, 2019

View reviewed changes

vbauzys merged commit a49114f into vmware:master May 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add external network #185

Add external network #185

vbauzys commented Apr 30, 2019 •

edited by dataclouder

Loading

lvirbalas left a comment

lvirbalas Apr 30, 2019

vbauzys Apr 30, 2019

Didainius Apr 30, 2019

lvirbalas May 7, 2019

vbauzys May 7, 2019

Didainius left a comment

Didainius Apr 30, 2019

vbauzys Apr 30, 2019

vbauzys May 6, 2019

Didainius Apr 30, 2019

vbauzys Apr 30, 2019

Didainius Apr 30, 2019

Didainius May 2, 2019

vbauzys May 2, 2019

dataclouder left a comment

Didainius left a comment

Didainius May 8, 2019

vbauzys May 8, 2019

	check.Assert(task, Not(Equals), Task{})
	check.Assert(task.Task, Not(Equals), types.Task{})

Add external network #185

Add external network #185

Conversation

vbauzys commented Apr 30, 2019 • edited by dataclouder Loading

lvirbalas 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

Didainius 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

dataclouder left a comment

Choose a reason for hiding this comment

Didainius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbauzys commented Apr 30, 2019 •

edited by dataclouder

Loading