Skip to content
This repository has been archived by the owner on Nov 9, 2020. It is now read-only.

Add clone and single vm concurrent tests #802

Merged
merged 5 commits into from
Dec 21, 2016
Merged

Add clone and single vm concurrent tests #802

merged 5 commits into from
Dec 21, 2016

Conversation

brunotm
Copy link
Contributor

@brunotm brunotm commented Dec 7, 2016

Fixes #772

@brunotm brunotm changed the title Add parallel cloning tests Add clone parallel tests Dec 7, 2016
msterin
msterin reviewed Dec 7, 2016
View reviewed changes
Copy link
Contributor

@msterin msterin left a comment

Choose a reason for hiding this comment

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

The new code looks good to me with a few nits. There is a comment about older parallel test - please take a look

vmdk_plugin/sanity_test.go
@@ -240,7 +242,7 @@ func TestSanity(t *testing.T) {
"size": "1gb",
},
}
// Create/delete routine
// Create/delete goroutine
Copy link
Contributor

Choose a reason for hiding this comment

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

So the problem here - which I observed on my testbed- is that if there is a client in clients more than once, the code below (go func) fails because ESX rejects reconfigure request on the same VM when another one is in flight.

and our current Makefiles allow for VM1 and VM2 (test clients) to be the same - in fact this is how single VM testing is done.

I'd suggest checking for this and warn then skip duplicate clients. Yes, it will skip the parallel test but will unblock single VM test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't aware of that possibility, or the fact that esx would error out. But independently of skipping the tests for that specific scenario we should serialize reconfig tasks per vm in vmdk_ops.

vmdk_plugin/sanity_test.go
for idx, elem := range clients {
go func(idx int, c *client.Client) {
for i := 0; i < parallelClones; i++ {
volName := masterVolName + "-clone" + strconv.Itoa(idx) + strconv.Itoa(i)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: formatting with fmt. is easier to read IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

vmdk_plugin/sanity_test.go
}

// Read the results from the channel
for i := 0; i < len(clients)*parallelClones*2; i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: do we need spaces around * ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done by gofmt.

Copy link
Contributor

Choose a reason for hiding this comment

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

ignore my comment about * then :-)

vmdk_plugin/sanity_test.go
@@ -41,6 +41,7 @@ const (
defaultTestLogPath = "/tmp/test-docker-volume-vsphere.log"
// Number of volumes per client for parallel tests
parallelVolumes = 9
parallelClones = 3
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: both these constants seem prime candidates for test arguments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes :)

@brunotm brunotm mentioned this pull request Dec 8, 2016
Closed
@brunotm
Copy link
Contributor Author

brunotm commented Dec 8, 2016

@msterin
I was thinking about also pushing in this PR single vm concurrent tests and address your comments. The corresponding issue was updated to reflect the single vm tests. Unless you think they should go in a different one. :)

@msterin
Copy link
Contributor

msterin commented Dec 8, 2016

@msterin I was thinking about also pushing in this PR single vm concurrent tests and address your comments. The corresponding issue was updated to reflect the single vm tests. Unless you think they should go in a different one. :)

IMO either way is fine as long as there are 2 separate commits

@brunotm brunotm mentioned this pull request Dec 9, 2016
Merged
@brunotm brunotm force-pushed the brunotm.issue772 branch 2 times, most recently from 1cab00c to 6905a24 Compare December 9, 2016 16:08
@brunotm brunotm changed the title Add clone parallel tests Add clone and single vm concurrent tests Dec 9, 2016
@brunotm
Copy link
Contributor Author

brunotm commented Dec 9, 2016

@msterin
I have included the commit from #810 here, otherwise tests would fail.
Hopefully this PR together with #810, will be the last edge for the concurrency case.

@brunotm
Copy link
Contributor Author

brunotm commented Dec 11, 2016

@msterin
Updated the patch to address the last comment and reverted the commit included from #810.
The CI run might fail without #810.

From my part is good to go.

Thanks.

@msterin
Copy link
Contributor

msterin commented Dec 12, 2016

The CI run might fail without #810.

And it does fail:

	sanity_test.go:189: Successfully connected to tcp://192.168.31.206:2375
	sanity_test.go:313: Same docker host concurrent create/delete test failed, err: Error response from daemon: create volTestP-same20: Post http://%2Frun%2Fdocker%2Fplugins%2Fvmdk.sock/VolumeDriver.Create: http: ContentLength=49 with Body length 0
FAIL

Until #810 is in, this one has to wait to avoid breaking the master

@brunotm
Copy link
Contributor Author

brunotm commented Dec 12, 2016

@msterin
Definitely! :)

@brunotm brunotm mentioned this pull request Dec 13, 2016
Closed
@brunotm
Copy link
Contributor Author

brunotm commented Dec 21, 2016

@msterin
The concurrent tests are good to go after #810.

@pdhamdhere pdhamdhere merged commit ed692e0 into vmware-archive:master Dec 21, 2016
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Reviewers

@msterin msterin msterin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@brunotm @msterin @pdhamdhere