Test to delete vm from its vmgroup and few dependent utils #1483

ashahi1 · 2017-06-26T19:11:38Z

This PR covers following things:

Added a test to delete vm from its vmgroup - PR Tenant ls command does not works if vm that was added to tenant is deleted. #929
Added govc utils to create and destroy vm.
Made GetRandomNumber() public so it can be directly used. Eg. Get a unique name for vm

Test steps:

Create a vm VM1 using govc
Create a vmgroup and associate VM1 to it.
Execute vmgroup ls command.
Delete the vm that was added to the vmgroup.
Again execute vmgroup ls command to verify command works fine.

Tested it locally. All test test-esx passed

=> Running target test-e2e-runalways
OK: 6 passed
--- PASS: Test (115.64s)
PASS

=> Running target test-e2e-runonce
--- PASS: Test (493.81s)
PASS
OK: 22 passed, 3 skipped

lipingxue · 2017-06-26T22:10:08Z

tests/e2e/vmgroupmisc_test.go

+// 4. Again execute vmgroup ls command to verify command works fine.
+func (s *vgBasicSuite) TestDeleteVMFromVmgroup(c *C) {
+	misc.LogTestStart(c.TestName())
+	vmName := "VM_" + inputparams.GetRandomNumber()


Why do you want to create a new vmgroup here? Why not using existing vmgroup "vmGroupName"?

@lipingxue Because of issue #1484, I am creating a separate tenant else verifications in other tests will start failing. For remaining tests in the same file, I am creating vmgroup once in the SetUpSuite()

lipingxue · 2017-06-26T22:11:43Z

tests/e2e/vmgroupmisc_test.go

+	vgName := "VG_" + inputparams.GetRandomNumber()
+
+	// Create a vm - we need this to add to vmgroup and later on delete this vm
+	esx.CreateVM(vmName, s.config.Datastores[0])


Should check the out and err returned by function "esx.CreateVM".

@lipingxue We are already doing it in InvokeCommandLocally()
https://github.com/vmware/docker-volume-vsphere/blob/master/tests/utils/ssh/ssh.go#L50

lipingxue · 2017-06-26T22:12:47Z

tests/e2e/vmgroupmisc_test.go

+	// Create a vm - we need this to add to vmgroup and later on delete this vm
+	esx.CreateVM(vmName, s.config.Datastores[0])
+
+	admincli.CreateVMgroup(s.config.EsxHost, vgName, vmName, admincliconst.VMHomeDatastore)


Need to add assert to make sure no error is returned from "CreateVMgroup".

lipingxue · 2017-06-26T22:15:11Z

tests/e2e/vmgroupmisc_test.go

+	admincli.CreateVMgroup(s.config.EsxHost, vgName, vmName, admincliconst.VMHomeDatastore)
+	// Verify if vmgroup exists
+	isVmgroupAvailable := admincli.IsVmgroupPresent(s.config.EsxHost, vgName)
+	c.Assert(isVmgroupAvailable, Equals, true, Commentf("vmgroup ls command does not lists the vmgroup %s .", vgName))


Change "vmgroup ls command does not lists the vmgroup %s ." to "Vmgroup %s dose not exist."

lipingxue · 2017-06-26T22:16:09Z

tests/e2e/vmgroupmisc_test.go

+	c.Assert(isVMPartofVg, Equals, true, Commentf("VM %s does not belong to vmgroup %s .", vmName, vgName))
+
+	// Destroy the vm
+	esx.DestroyVM(vmName)


Check the out and error returned from DestroyVM.

+1 .. atleast validate VM is no more present.

lipingxue · 2017-06-26T22:16:38Z

tests/e2e/vmgroupmisc_test.go

+
+	// Destroy the vm
+	esx.DestroyVM(vmName)
+	misc.SleepForSec(3)


Why the sleep is needed? Please add comments for that.

lipingxue · 2017-06-26T22:17:21Z

tests/e2e/vmgroupmisc_test.go

+
+	// Check vmgroup ls
+	isVmgroupAvailable = admincli.IsVmgroupPresent(s.config.EsxHost, vgName)
+	c.Assert(isVmgroupAvailable, Equals, true, Commentf("vmgroup ls command does not lists the vmgroup %s .", vgName))


Same comments as above.

lipingxue · 2017-06-26T22:18:50Z

tests/utils/esx/govc.go

+// CreateVM creates a vm on the specified ds and esx
+func CreateVM(vmName, datastore string) string {
+	log.Printf("Creating a vm [%s] \n", vmName)
+	cmd := esx.VMCreate + datastore + " -on=false -link=false -net.adapter=vmxnet3 " + vmName


Do we always use "-net.adapter=vmxnet3"?

Changed into a parameter to be passed to function.

shuklanirdesh82

LGTM!

shuklanirdesh82 · 2017-06-27T01:25:32Z

tests/e2e/vmgroupmisc_test.go

+	vgName := "VG_" + inputparams.GetRandomNumber()
+
+	// Create a vm - we need this to add to vmgroup and later on delete this vm
+	esx.CreateVM(vmName, s.config.Datastores[0])


minor: can we atleast validate vm is created or not? (helps in debugging failure)

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

tests/e2e/vmgroupmisc_test.go

+	c.Assert(isVMPartofVg, Equals, true, Commentf("VM %s does not belong to vmgroup %s .", vmName, vgName))
+
+	// Destroy the vm
+	esx.DestroyVM(vmName)


+1 .. atleast validate VM is no more present.

shuklanirdesh82 · 2017-06-27T01:28:53Z

tests/utils/esx/govc.go

+func DestroyVM(vmName string) string {
+	log.Printf("Deleting a vm - %s \n", vmName)
+	cmd := esx.VMDestroy + vmName
+	return ssh.InvokeCommandLocally(cmd)


minor: command can be simply moved here.

shuklanirdesh82 · 2017-06-27T01:30:50Z

tests/e2e/vmgroupmisc_test.go

+	// Check vmgroup ls
+	isVmgroupAvailable = admincli.IsVmgroupPresent(s.config.EsxHost, vgName)
+	c.Assert(isVmgroupAvailable, Equals, true, Commentf("vmgroup ls command does not lists the vmgroup %s .", vgName))
+


It is better to add TODO here saying .. due to product behavior vmgroup deletion is not possible and add the reference of the issue here .. if there is any issue otherwise please raise one and add its reference here.

shuklanirdesh82

LGTM! Please address minor comments before merging.

shuklanirdesh82 · 2017-06-28T01:14:51Z

tests/utils/esx/govc.go

+	log.Printf("Verifying if vm - %s exists \n", vmName)
+	listVMs := " /ha-datacenter/vm"
+	cmd := esx.LsOption + listVMs
+	vmList := ssh.InvokeCommandLocally(cmd)


minor: need to combine 94-96.

shuklanirdesh82 · 2017-06-28T01:16:05Z

tests/e2e/vmgroupmisc_test.go

+	esx.DestroyVM(vmName)
+
+	// Added the sleep before next step as deletion may take time to finish completely.
+	misc.SleepForSec(3)


it is better to use polling mechanism here instead 3 seconds ..

shuklanirdesh82 · 2017-06-28T01:17:33Z

tests/constants/esx/govc.go

+	VMDestroy = govcCmd + "vm.destroy "
+
+	// LsOption refers to govc ls
+	LsOption = govcCmd + "ls "


could have created ListVMs as an exported variable.

lipingxue

One general comment, after you addressed comments, please don't squash before you push. I would like o see only the delta between the current code and my last review.

lipingxue · 2017-06-29T21:35:22Z

tests/e2e/vmgroupmisc_test.go

+	c.Assert(isVMPartofVg, Equals, true, Commentf("VM %s does not belong to vmgroup %s .", vmName, vgName))
+
+	// Destroy the vm
+	esx.DestroyVM(vmName)


This still not address. You don't check the out and error returned from DestroyVM

govc does not returns error code. I wrote a util to check if vm exists. That I am using after creating a vm as well as after destroying vm.

ashahi1 self-assigned this Jun 26, 2017

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

ashahi1 requested review from shuklanirdesh82, shaominchen and pshahzeb June 26, 2017 19:42

lipingxue suggested changes Jun 26, 2017

View reviewed changes

shuklanirdesh82 reviewed Jun 27, 2017

View reviewed changes

shuklanirdesh82 approved these changes Jun 28, 2017

View reviewed changes

shuklanirdesh82 reviewed Jun 28, 2017

View reviewed changes

ashahi1 force-pushed the vmgrouplsAfterVMdelete.ashahi1 branch from f3bb9c9 to 03791aa Compare June 28, 2017 19:08

lipingxue reviewed Jun 29, 2017

View reviewed changes

lipingxue approved these changes Jun 29, 2017

View reviewed changes

Rebased code with master

b64b05c

ashahi1 force-pushed the vmgrouplsAfterVMdelete.ashahi1 branch from 03791aa to b64b05c Compare June 29, 2017 23:52

shuklanirdesh82 merged commit 4a95c2c into vmware-archive:master Jun 30, 2017

ashahi1 deleted the vmgrouplsAfterVMdelete.ashahi1 branch August 16, 2017 01:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test to delete vm from its vmgroup and few dependent utils #1483

Test to delete vm from its vmgroup and few dependent utils #1483

ashahi1 commented Jun 26, 2017

lipingxue Jun 26, 2017

ashahi1 Jun 27, 2017

lipingxue Jun 26, 2017

ashahi1 Jun 27, 2017

lipingxue Jun 26, 2017

ashahi1 Jun 27, 2017

lipingxue Jun 26, 2017

ashahi1 Jun 27, 2017

lipingxue Jun 26, 2017

shuklanirdesh82 Jun 27, 2017

lipingxue Jun 26, 2017

ashahi1 Jun 27, 2017

lipingxue Jun 26, 2017

ashahi1 Jun 27, 2017

lipingxue Jun 26, 2017

ashahi1 Jun 27, 2017

shuklanirdesh82 left a comment

shuklanirdesh82 Jun 27, 2017

shuklanirdesh82 Jun 27, 2017

shuklanirdesh82 Jun 27, 2017

shuklanirdesh82 Jun 27, 2017

shuklanirdesh82 left a comment

shuklanirdesh82 Jun 28, 2017

shuklanirdesh82 Jun 28, 2017

ashahi1 Jun 28, 2017

shuklanirdesh82 Jun 28, 2017

lipingxue left a comment

lipingxue Jun 29, 2017

ashahi1 Jun 29, 2017

Test to delete vm from its vmgroup and few dependent utils #1483

Test to delete vm from its vmgroup and few dependent utils #1483

Conversation

ashahi1 commented Jun 26, 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

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

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 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

lipingxue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment