Automation test for removing user created vmgroup. #1474

lipingxue · 2017-06-23T18:49:42Z

This PR includes

the automation test for removing user created vmgroup with "--remove-volumes" option
code refactoring for function VerifyDetachedStatus to meet the need for automating this test

Test Done:
Only swarm related tests failed since I do not have a swarm setup

=> Running target test-e2e-runalways Tue Jun 27 22:57:46 UTC 2017
GO15VENDOREXPERIMENT=1 go test -v -timeout 30m -tags runalways github.com/vmware/docker-volume-vsphere/tests/e2e

PASS
OK: 6 passed
ok  	github.com/vmware/docker-volume-vsphere/tests/e2e	124.073s


=> Running target test-e2e-runonce Tue Jun 27 22:59:53 UTC 2017

GO15VENDOREXPERIMENT=1 go test -v -timeout 30m -tags runonce github.com/vmware/docker-volume-vsphere/tests/e2e

-- FAIL: Test (481.53s)
FAIL
OOPS: 24 passed, 2 FAILED, 1 MISSED
exit status 1
FAIL	github.com/vmware/docker-volume-vsphere/tests/e2e	481.539s

govint · 2017-06-23T19:34:44Z

tests/constants/admincli/cmd.go

@@ -49,6 +49,9 @@ const (
 	// where --name will be name of the vmgroup
 	ReplaceVMFromVMgroup = vmdkopsAdmin + "vmgroup vm replace --name="

+	// AddAccessForVMgroup create access perms on vmgroup
+	AddAccessForVMgroup = vmdkopsAdmin + "vmgroup access add --name "


#1381 is also modifying this code pls. check if there is any overlap.

this needs to be removed.

L#99 is doing the same thing .. Please remove this one.

govint · 2017-06-23T19:38:54Z

tests/e2e/vmgroups_test.go

+	misc.SleepForSec(2)
+
+	// Status should be detached
+	status = verification.VerifyDetachedStatusOnNonDefaultDS(vg.volName2, vg.config.Datastores[1],


Can't the same VerifyDetachedStatus() be used here? Why have a separate function for the non-default DS, the DS name is being passed anyway.

I introduce this because most of the existing tests only need to verify volumes created on default datastore and already call function VerifyDetachedStatus(. By introducing this new function, we can keep those part unchanged. Those two util functions can be used by caller in different cases.

govint · 2017-06-23T19:40:58Z

tests/e2e/vmgroups_test.go

+
+	out, err = adminutils.AddVMToVMgroup(vg.config.EsxHost, vgTestVMgroup1, vg.config.DockerHostNames[1])
+	c.Assert(err, IsNil, Commentf(out))
+	misc.LogTestEnd(c.TestName())


Why do we need to recreate and add the VMs back? is it for maintaining the test config?

Yes. Just for maintaining the test config.

govint · 2017-06-23T19:42:07Z

tests/e2e/vmgroups_test.go

+// 2. create two volumes for this vmgroup, one is on default datastore, and the other is on non-default datastore
+// 3. verify mount/unmount for this volume
+// 4. after unmount, verify the volume is detached for both volumes
+// 5. remove VMs from this vmgroup, and remove the vmgroup with "--remove-volume" option


with or without the option.

ashahi1 · 2017-06-23T21:07:32Z

tests/e2e/vmgroups_test.go

+
+	out, err = adminutils.RemoveVMFromVMgroup(vg.config.EsxHost, vgTestVMgroup1, vg.config.DockerHostNames[1])
+	c.Assert(err, IsNil, Commentf(out))
+


If possible, please combine them as to me it seems that we are removing VMs from the same vmgroup.

Here we do remove VMs from same vmgroup, and it is what we need to do here.

ashahi1 · 2017-06-23T21:09:52Z

tests/e2e/vmgroups_test.go

+	out, err = adminutils.RemoveVMFromVMgroup(vg.config.EsxHost, vgTestVMgroup1, vg.config.DockerHostNames[1])
+	c.Assert(err, IsNil, Commentf(out))
+
+	log.Printf("Removing test vmgroup %s", vgTestVMgroup1)


Minor: We do not really need this statement as DeleteVMgroup method itself has log message.

pshahzeb

Test looks good.
I have some refactoring comments tagged as minor. It doesn't affect the test case as such but just to avoid code repetition.
Another comment is about the sleep added in test. I think it is not needed. If yes, please add a comment.

pshahzeb · 2017-06-23T21:01:19Z

tests/e2e/vmgroups_test.go

@@ -46,6 +47,7 @@ type VmGroupTest struct {
 	volName1      string
 	volName2      string
 	volName3      string
+	volName4      string


This isn't because of your change but this should have been a slice instead. You might wanna handle it if it is trivial.

pshahzeb · 2017-06-23T21:08:37Z

tests/e2e/vmgroups_test.go

+	out, err := adminutils.AddCreateAccessForVMgroup(vg.config.EsxHost, vgTestVMgroup1, vg.config.Datastores[1])
+	c.Assert(err, IsNil, Commentf(out))
+
+	cmd := adminconst.GetAccessForVMgroup + vgTestVMgroup1


I think this is a verification step after access has been added. Might wanna wrap it into a util. If we always do this after adding the access, you might wanna add these steps to AddCreateAccessForVMgroup util so that the test steps itself are clear.

This is not a verification step, remove this.

pshahzeb · 2017-06-23T21:09:07Z

tests/e2e/vmgroups_test.go

+		log.Printf(out)
+	}
+
+	misc.SleepForSec(2)


Why this sleep? Does it take time for access to be added? if so please comment.

I copied from existing code, and verified that this is not needed. So I remove the sleep.

pshahzeb · 2017-06-23T21:11:05Z

tests/e2e/vmgroups_test.go

+	c.Assert(err, IsNil, Commentf(out))
+
+	// Docker may not have completed the detach yet with the host.
+	misc.SleepForSec(2)


ExecContainer fires a container with --rm flag. So this detach should happen instantly. I am not sure if a sleep is needed here. Are we hitting a race without this sleep?

pshahzeb · 2017-06-23T21:11:42Z

tests/e2e/vmgroups_test.go

+	misc.SleepForSec(2)
+
+	// Status should be detached
+	status = verification.VerifyDetachedStatusOnNonDefaultDS(vg.volName2, vg.config.Datastores[1],


pshahzeb · 2017-06-23T21:12:24Z

tests/e2e/vmgroups_test.go

+	out, err = adminutils.RemoveVMFromVMgroup(vg.config.EsxHost, vgTestVMgroup1, vg.config.DockerHostNames[0])
+	c.Assert(err, IsNil, Commentf(out))
+
+	out, err = adminutils.RemoveVMFromVMgroup(vg.config.EsxHost, vgTestVMgroup1, vg.config.DockerHostNames[1])


Minor: wrap this removal and assert in a loop.

pshahzeb · 2017-06-23T21:14:21Z

tests/e2e/vmgroups_test.go

+
+	out, err = verification.GetVMGroupForVolume(vg.config.EsxHost, vg.volName2)
+	log.Println("GetVMGroupForVolume return out[%s] err[%s] for volume %s", out, err, vg.volName2)
+	c.Assert(err.Error(), Equals, "exit status 1", Commentf("volume %s should be removed", vg.volName2))


minor: these 3 repeating lines can be wrapped after you the volume name as slice.

pshahzeb · 2017-06-23T21:20:16Z

tests/e2e/vmgroups_test.go

+	out, err = adminutils.AddVMToVMgroup(vg.config.EsxHost, vgTestVMgroup1, vg.config.DockerHostNames[0])
+	c.Assert(err, IsNil, Commentf(out))
+
+	out, err = adminutils.AddVMToVMgroup(vg.config.EsxHost, vgTestVMgroup1, vg.config.DockerHostNames[1])


Should be wrapped in a loop

pshahzeb · 2017-06-23T21:22:37Z

tests/utils/verification/volumeproperties.go

@@ -152,6 +155,29 @@ func VerifyDetachedStatus(name, hostName, esxName string) bool {
 	return false
 }

+// VerifyDetachedStatusOnNonDefaultDS - For volume on non-default DS,
+// check if the status gets detached within the timeout
+func VerifyDetachedStatusOnNonDefaultDS(name, datastore, hostName, esxName string) bool {


Do we need this since we already have a util to verify detached status?

Agree, the existing function should be used.

lipingxue · 2017-06-24T23:07:26Z

@govint @ashahi1 @pshahzeb I have addressed your comments, please take a look.

pshahzeb · 2017-06-26T18:13:17Z

tests/utils/verification/volumeproperties.go

+	const maxAttempt = 60
+	for attempt := 0; attempt < maxAttempt; attempt++ {
+		misc.SleepForSec(2)
+		status := getVolumeStatusHost(name+"@"+datastore, hostName)


The only difference between this util and VerifyDetachedStatus is the datastore arg which is used to construct a fully qualified name to get volume status from docker host.
If it is worth, please put the common part of these utils into a func which can be called from these utils.

I still think we can use VerifyDetachedStatus, unless we are worried about same volume names on different datastores.

Agree with @pshahzeb. I think we can do the same thing as what we are doing in VerifyAttachedStatus - please refer to issue #1506 and the fix: 54ea0f1

govint · 2017-06-26T17:54:50Z

tests/utils/verification/volumeproperties.go

@@ -152,6 +155,29 @@ func VerifyDetachedStatus(name, hostName, esxName string) bool {
 	return false
 }

+// VerifyDetachedStatusOnNonDefaultDS - For volume on non-default DS,
+// check if the status gets detached within the timeout
+func VerifyDetachedStatusOnNonDefaultDS(name, datastore, hostName, esxName string) bool {


Agree, the existing function should be used.

govint · 2017-06-26T17:55:30Z

tests/e2e/vmgroups_test.go

+		log.Println("GetVMGroupForVolume return out[%s] err[%s] for volume %s", out, err, volume)
+		c.Assert(out, Equals, "N/A", Commentf("volume %s should not belong to any vmgroup", volume))
+	}
+


Should clean up the volumes created for the test.

Those volumes are no orphan volumes, and user have no way to access/cleanup those orphan volumes. So we cannot cleanup them through adminCLI or dockerCLI.
As we dicussed in #1469, we should NOT allow user to remove a vmgroup if volumes still in the vmgroup. So testing the scenario that "remove vmgroup without removing volumes" is not needed if we decided to not support remove vmgroup without removing volumes.

I plan to remove this test from the PR.

govint · 2017-06-26T18:07:09Z

tests/e2e/vmgroups_test.go

+	c.Assert(err, IsNil, Commentf(out))
+
+	// Status should be detached
+	status = verification.VerifyDetachedStatusOnNonDefaultDS(vg.volName2, vg.config.Datastores[1],


Same function as above can be used for the verification. Verification is best done by exec'ing a container on another docker host VM, which will be successful only when the volume is really detached from the VM.

lipingxue · 2017-06-26T21:47:22Z

@govint @pshahzeb I have addressed your comments and disable the test that removing vmgroup without "--remove-volumes" option.

shuklanirdesh82

@lipingxue

PR description is missing out following information.

What fixes this PR is targeting. (title says about the automating test infact there are some refactoring work and improvement to existing code)
Testing Done section is missing.

shuklanirdesh82 · 2017-06-27T01:40:02Z

tests/utils/verification/volumeproperties.go


 	//TODO: Need to implement generic polling logic for better reuse
 	const maxAttempt = 60
 	for attempt := 0; attempt < maxAttempt; attempt++ {
 		misc.SleepForSec(2)
-		status := getVolumeStatusHost(name, hostName)
+		status := getVolumeStatusHost(name+"@"+datastore, hostName)


can you please shade some information why do we need this change?

Function getVolumeStatusHost calls dockercli to get the volume status. It actually run docker volume inspect command, so for volume which is not created on the default_datastore, we need to pass in full name of the volume.

shuklanirdesh82 · 2017-06-27T01:40:29Z

tests/utils/verification/volumeproperties.go

-// VerifyDetachedStatus - check if the status gets detached within the timeout
-func VerifyDetachedStatus(name, hostName, esxName string) bool {
-	log.Printf("Confirming detached status for volume [%s]\n", name)
+// // VerifyDetachedStatus - check if the status gets detached within the timeout


I guess this is dead code and can be cleaned up.

shuklanirdesh82 · 2017-06-27T02:16:11Z

tests/e2e/vmgroups_test.go

+	// need to use full name for volume name
+	vg.createVolumes(c, vg.volName2+"@"+vg.config.Datastores[1])
+
+	// Verify volume can be mounted and unmounted for the first volume


As per the inline comment, please add verification step when volume was attached otherwise inline comment needs to be updated.

Update the comments.

shuklanirdesh82 · 2017-06-27T02:17:25Z

tests/e2e/vmgroups_test.go

+	status := verification.VerifyDetachedStatus(vg.volName1, vg.config.Datastores[0], vg.config.DockerHosts[0], vg.config.EsxHost)
+	c.Assert(status, Equals, true, Commentf("Volume %s is not detached", vg.volName1))
+
+	// Verify volume can be mounted and unmounted for the second volume


same as above

shuklanirdesh82 · 2017-06-27T02:20:01Z

tests/e2e/vmgroups_test.go

+	// remove VM1 and VM2 from vgTestVMgroup1 and then remove the vmgroup
+	vmList := []string{vg.config.DockerHostNames[0], vg.config.DockerHostNames[1]}
+	for _, vm := range vmList {
+		out, err = adminutils.RemoveVMFromVMgroup(vg.config.EsxHost, vgTestVMgroup1, vm)


only one call is sufficient to remove both vms .. admin_cli vmgroup vm remove --name --vm-list .. am I missing anything here?

shuklanirdesh82 · 2017-06-27T02:22:39Z

tests/e2e/vmgroups_test.go

+	// vmgroup has been removed and all volumes belong to it have been removed
+	// "err.Error()" will be filled with "exit status 1"
+	volumeList := []string{vg.volName1, vg.volName2}
+	for _, volume := range volumeList {


could be replaced with one remote call invocation ..admin_cli volume ls > verifies volumes created as part of this test method should not be present.

We should use util function instead of call "admin_cli volume ls" directly.

As we discussed, #1496 is filed to track this.

shuklanirdesh82 · 2017-06-27T02:25:04Z

tests/e2e/vmgroups_test.go

+		c.Assert(err.Error(), Equals, "exit status 1", Commentf("volume %s should be removed", volume))
+	}
+
+	// Recreate the test VM group1 and add VM1, VM2 back to the vmgroup


sorry we should avoid this approach .. this leads to confusion any of the step fails (L494-502) instead test should create its own vmgroup and deletes it

Why? I checked the existing tests in "vmgroups_test.go", and I followed the approach as other tests. Why it will lead to confusion? Comments already say what is the purpose of the code from L494-502.

As we discussed, #1497 is filed to track this.

shuklanirdesh82 · 2017-06-28T06:23:33Z

tests/utils/verification/volumeproperties.go

-// VerifyDetachedStatus - check if the status gets detached within the timeout
-func VerifyDetachedStatus(name, hostName, esxName string) bool {
-	log.Printf("Confirming detached status for volume [%s]\n", name)
+// VerifyDetachedStatus- check if the status gets detached within the timeout


minor: golint is complaining about this method document ..

Reference: https://ci.vmware.run/vmware/docker-volume-vsphere/763

../tests/utils/verification/volumeproperties.go:136:1: comment on exported function VerifyDetachedStatus should be of the form "VerifyDetachedStatus ..."

govint

Approving the change as we already have issues raised for some comments here.

govint · 2017-06-27T03:53:20Z

tests/e2e/vmgroups_test.go

+
+// 	Recreate the test VM group1 and add VM1, VM2 back to the vmgroup
+// 	cmd := adminconst.CreateVMgroup + vgTestVMgroup1 + " --default-datastore " + vg.config.Datastores[0]
+// 	log.Printf("Creating test vmgroup %s", vgTestVMgroup1)


The vmgroup can be created and the two VMs can be added in this command itself via a "--vm-list"

Done. Move the restore logic to function RestoreVmgroup

govint · 2017-06-27T03:53:48Z

tests/utils/verification/volumeproperties.go

+// 	}
+// 	log.Printf("Timed out to poll status\n")
+// 	return false
+// }


Delete this function instead?

govint · 2017-06-28T10:47:12Z

tests/e2e/vmgroups_test.go

+
+	// remove VM1 and VM2 from vgTestVMgroup1 and then remove the vmgroup
+	vmList := vg.config.DockerHostNames[0] + "," + vg.config.DockerHostNames[1]
+	out, err = adminutils.RemoveVMFromVMgroup(vg.config.EsxHost, vgTestVMgroup1, vmList)


Suggestion, can instead pass the vg.config.DockerHostNames to RemoveVMFromVMgroup() and let it do the creation of the vmlist to pass to the admin cli command. Basically, the test functions are simple and shouldn't have to create special args for the util functions.

govint · 2017-06-28T10:51:30Z

tests/e2e/vmgroups_test.go

+	c.Assert(err, IsNil, Commentf(out))
+
+	out, err = adminutils.AddVMToVMgroup(vg.config.EsxHost, vgTestVMgroup1, vmList)
+	c.Assert(err, IsNil, Commentf(out))


This is ok, just place it in a separate function RestoreVmgroups() so this test function doesn't look like its cluttered with this restoration code.

govint · 2017-06-28T10:52:26Z

tests/e2e/vmgroups_test.go

+// 	for _, vm := range vmList {
+// 		out, err = adminutils.AddVMToVMgroup(vg.config.EsxHost, vgTestVMgroup1, vm)
+// 		c.Assert(err, IsNil, Commentf(out))
+// 	}


Makes sense to have a separate function to keep this code to restore the configuration.

Done. Same as above.

govint · 2017-06-28T11:00:56Z

tests/utils/verification/volumeproperties.go

+	}
+	if len(volProps) != 2 {
+		log.Printf("Admin cli output is expected to consist of two elements only - "+
+			"volume name and vmgrop. Actual output %s ", op)


what happens if the same volume name is on two separate datastores, then we will get two lines (I had this scenario and verification/volumeproperties.go:GetVMAttachedToVolUsingAdminCli() failed).

I'll raise an issue for docker volume inspect to return the vmgroup for a volume so we can use it directly.

I've added a function to return a map of volume properties in PR #1500 and that can be modified to return the vmgroup for a volume.

The above approach is prone to errors in different scenarios and have a lot of this code, as much as possible should be using docker to get properties filtered out for our use.

The volume name used in test is unique since it use random generator. So it is not very possible that we hit the scenario that we have same volume name with two different datastores.

For a orphan volume, docker volume inspect may not able to get the vmgroup associated with the volume.

lipingxue · 2017-06-29T19:06:14Z

I think more and decide that we should add function "VerifyDetachedStatusNonDefaultDS" back.
This function is used to check whether volume is detached if the volume is created on the non default datastore. Original function "VerifyDetachedStatus" should be used to check whether volume created on the default datastore is detached or not.

We should not try to combine function "VerifyDetachedStatusNonDefaultDS" and "VerifyDetachedStatus" into one.

Here are the reasons:

From user point of view, for volume created on the default datastore, user should use short name instead of full vol name. Full vol should be used if the volume is not in the default datastore. So it is make sense to have separate functions to handle these two different scenarios. If the language support function overloading, we should have overload function for these two scenarios. Since go does not support function overloading, we should have two functions with different names here.
For automation tests which need to check whether a volume is detached or not, most of the existing tests create the volume on the default datastore and the default_datastore is vm_datastore for most cases. It does not make sense to find out the vm_datastore and then pass in to function "VerifyDetachedStatus" . In order to do that, we must write another util function to get the vm_store for a VM, which is not needed. And then most existing tests need to be refactored. I don't see much value to do this.

@shuklanirdesh82 @govint
I decide to add function "VerifyDetachedStatusNonDefaultDS" back, and please review it.

shuklanirdesh82

overall LGTM!

Please address supplied comments with this review before merging.

shuklanirdesh82 · 2017-06-30T05:32:55Z

tests/utils/admincli/vmgroupmgmt.go

+func DeleteVMgroup(ip, name string, delete_vol bool) (string, error) {
+	log.Printf("Deleting a vmgroup [%s] on esx [%s] with delete_vol[%d]\n", name, ip, delete_vol)
+	if delete_vol {
+		return ssh.InvokeCommand(ip, admincli.RemoveVMgroup+name+admincli.RemoveVolumes)


minor: actually command should have been prepared in if ...else .. block that improve readability and ssh.InvokeCommand( would be placed at once out of if block.

shuklanirdesh82 · 2017-06-30T05:47:49Z

tests/constants/admincli/cmd.go

@@ -49,6 +49,9 @@ const (
 	// where --name will be name of the vmgroup
 	ReplaceVMFromVMgroup = vmdkopsAdmin + "vmgroup vm replace --name="

+	// AddAccessForVMgroup create access perms on vmgroup
+	AddAccessForVMgroup = vmdkopsAdmin + "vmgroup access add --name "


L#99 is doing the same thing .. Please remove this one.

shuklanirdesh82 · 2017-06-30T05:50:57Z

tests/utils/verification/volumeproperties.go

+	//TODO: Need to implement generic polling logic for better reuse
+	const maxAttempt = 60
+	for attempt := 0; attempt < maxAttempt; attempt++ {
+		misc.SleepForSec(2)


this needs to be removed; it may hide if there is any race condition.

govint · 2017-06-30T12:40:27Z

@lipingxue, no the VerifyDetachedState() function is sufficient for any scenario - whether default or non-default datastore. The user has the choice to specify the datastore whether its a default or not. The function is only figuring out whether a volume is in detached or attached state. Why should that depend on the datastore?

In fact, the restart tests explicitly test for use case where user can attach a volume via short and long name on the default and non-default datastores.

lipingxue · 2017-06-30T14:57:48Z

@govint I don't get your point what do you mean "user has the choice to specify the datastore whether its a default or not". Which parameter that the function need to take so that "user has the choice to specify the datastore whether its a default or not"? The function calls both dockerCLI and adminCLI to figure out the volume is detached or not. For dockerCLI, short name can only be used if the volume is created in the default datastore. Otherwise, user need to specify the full vol name.

shuklanirdesh82 · 2017-06-30T16:36:50Z

@lipingxue Let's continue the investigation in the next sprint. So not merging this PR.

CI is failing: https://ci.vmware.run/vmware/docker-volume-vsphere/842

2017/06/30 15:40:00 START: vgBasicSuite.TestDSAccessPrivilegeForUserVG
2017/06/30 15:40:00 Creating create access for vmgroup vg_basictest, datastore vsanDatastore on esx [192.168.31.62] 
2017/06/30 15:40:03 Getting vmgroup's [vg_basictest] access rights to datastore vsanDatastore ,  on esx [192.168.31.62] 
2017/06/30 15:40:05 Access rights to datastore vsanDatastore for vmgroup - vg_basictest are: [vsanDatastore True] . 

----------------------------------------------------------------------
FAIL: vmgroupmisc_test.go:157: vgBasicSuite.TestDSAccessPrivilegeForUserVG

vmgroupmisc_test.go:165:
    c.Assert(isDatastoreAccessible, Equals, false, Commentf("Unexpected Behavior: Datastore %s is accessible for vmgroup %s .", s.config.Datastores[1], vmGroupName))
... obtained bool = true
... expected bool = false
... Unexpected Behavior: Datastore vsanDatastore is accessible for vmgroup vg_basictest .

shaominchen

Sorry for the late comments. Change looks good overall.

shaominchen · 2017-06-30T16:57:57Z

tests/e2e/swarm_test.go

@@ -35,6 +35,7 @@ import (

 type SwarmTestSuite struct {
 	esxName     string
+	dataStores  []string


Leftover during refactoring? Let's remove this unused field.

shaominchen · 2017-06-30T17:00:19Z

tests/e2e/vmgroups_test.go

@@ -431,3 +432,138 @@ func (vg *VmGroupTest) TestVmGroupVolumeClone(c *C) {

 	misc.LogTestEnd(c.TestName())
 }
+
+// Restore VMgroup
+func (vg *VmGroupTest) RestoreVmgroup(c *C) {


Nit: This function should start with small letter since we don't really want to export it.

shaominchen · 2017-06-30T17:10:59Z

tests/utils/verification/volumeproperties.go

+	const maxAttempt = 60
+	for attempt := 0; attempt < maxAttempt; attempt++ {
+		misc.SleepForSec(2)
+		status := getVolumeStatusHost(name+"@"+datastore, hostName)


Agree with @pshahzeb. I think we can do the same thing as what we are doing in VerifyAttachedStatus - please refer to issue #1506 and the fix: 54ea0f1

lipingxue · 2017-07-03T22:37:33Z

@shaominchen I have addressed your comments, please take a look. Thanks!

shaominchen

LGTM.

First commit. First checkpoint. Second checkpoint. Third checkpoint. Checkpoint 4. Revert change in Makefile

vmwclabot added the cla-not-required label Jun 23, 2017

govint suggested changes Jun 23, 2017

View reviewed changes

ashahi1 reviewed Jun 23, 2017

View reviewed changes

pshahzeb reviewed Jun 23, 2017

View reviewed changes

pshahzeb reviewed Jun 26, 2017

View reviewed changes

govint suggested changes Jun 26, 2017

View reviewed changes

lipingxue force-pushed the vmgroup_rm_test.liping branch from 31c1ae9 to 239e8c0 Compare June 26, 2017 21:35

shuklanirdesh82 suggested changes Jun 27, 2017

View reviewed changes

shuklanirdesh82 reviewed Jun 27, 2017

View reviewed changes

lipingxue force-pushed the vmgroup_rm_test.liping branch 2 times, most recently from 9839eb5 to 6feda38 Compare June 28, 2017 05:24

shuklanirdesh82 reviewed Jun 28, 2017

View reviewed changes

govint approved these changes Jun 28, 2017

View reviewed changes

lipingxue force-pushed the vmgroup_rm_test.liping branch 2 times, most recently from 3854de9 to 0e7d4db Compare June 29, 2017 17:54

lipingxue force-pushed the vmgroup_rm_test.liping branch 2 times, most recently from 0b3d0a9 to 022e8c4 Compare June 30, 2017 04:16

shuklanirdesh82 approved these changes Jun 30, 2017

View reviewed changes

lipingxue force-pushed the vmgroup_rm_test.liping branch from 022e8c4 to bd35daf Compare June 30, 2017 06:36

shaominchen reviewed Jun 30, 2017

View reviewed changes

lipingxue force-pushed the vmgroup_rm_test.liping branch from bd35daf to 545b76c Compare July 3, 2017 22:31

shaominchen approved these changes Jul 5, 2017

View reviewed changes

lipingxue added 14 commits July 5, 2017 15:21

Automation test for removing user created vmgroup.

35626cf

First commit. First checkpoint. Second checkpoint. Third checkpoint. Checkpoint 4. Revert change in Makefile

Address comments from Govindan, Anup and Shahzeb.

6814626

Address more comments from Govindan and Shahzeb.

283f2d2

Disable the test TestVmgroupRemove.

a15baa8

Address comments from Nirdesh.

379f556

Minor fix.

d951e2b

Address additional comments from Nirdesh and Govindan.

f280ee7

Add function VerifyDetachedStatusNonDefaultDS back.

2846be3

Solve merge conflict.

e6f28e0

Trigger CI.

a35901d

Remove duplicate function to add datastore access for vmgroup.

614f29c

Address comments from Nirdesh and Sam.

f89e86e

Trigger CI run.

c52b8d3

Trigger CI.

40eaf58

lipingxue force-pushed the vmgroup_rm_test.liping branch from 760c2bc to 40eaf58 Compare July 5, 2017 22:22

Trigger CI again.

735a8e4

shuklanirdesh82 assigned lipingxue Jul 6, 2017

lipingxue merged commit 92868b1 into master Jul 6, 2017

shuklanirdesh82 deleted the vmgroup_rm_test.liping branch November 10, 2017 05:31


		out, err = adminutils.RemoveVMFromVMgroup(vg.config.EsxHost, vgTestVMgroup1, vg.config.DockerHostNames[1])
		c.Assert(err, IsNil, Commentf(out))

Automation test for removing user created vmgroup. #1474

Automation test for removing user created vmgroup. #1474

Conversation

lipingxue commented Jun 23, 2017 • edited Loading

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

pshahzeb 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

lipingxue commented Jun 24, 2017

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

lipingxue commented Jun 26, 2017

shuklanirdesh82 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

shuklanirdesh82 Jun 27, 2017 • edited Loading

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

govint 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

lipingxue commented Jun 29, 2017

shuklanirdesh82 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lipingxue commented Jun 23, 2017 •

edited

Loading

shuklanirdesh82 Jun 27, 2017 •

edited

Loading