E2E tests related to default vmgroup and non-default vmgroup #1381

ashahi1 · 2017-06-09T03:03:14Z

This PR fixes following issues:

1. TestDeleteDefaultVmgroup - [Test Automation] Verify volume creation after deleting _DEFAULT tenant #1486
1. Fixing test cleanup for unit test vmdk_ops_test.py - Issue Unit test - test_vmdkops_on_default_tenant_vm test fails #1477

TestDeleteDefaultVmgroup has already been reviewed.

Fixing test cleanup is the new change in the current commit. Please review it.

=============================

Test: TestDeleteDefaultVmgroup

Steps:
Objective: Verify volume creation fails after deleting _DEFAULT tenant
 1. Create a volume
 2. Verify volume from docker host and esx
 2. Delete default tenant
 3. Verify not able to create volume from docker host - Error: VM does not belong to any vmgroup
 4. Verify docker volume ls does not show any volumes
 5. Verify admin cli shows volume with VMGroup set as N/A
 6. Create _DEFAULT vmgroup with —default-datastore=_VM_DS
 7. Again create new volume - operation should succeed
 8. Verify volume from esx and docker host

2017/06/13 22:20:49 Removing policy [validPolicy] on esx [10.162.4.8]
--- PASS: Test (463.98s)
PASS
OK: 25 passed
ok  	github.com/vmware/docker-volume-vsphere/tests/e2e	463.982s
make: Leaving directory `/go/src/github.com/vmware/docker-volume-vsphere/vmdk_plugin'

shuklanirdesh82 · 2017-06-09T05:08:04Z

tests/utils/inputparams/testparams.go

@@ -142,3 +144,8 @@ func GetTestConfig() *TestConfig {

 	return config
 }
+
+// GetTenantNameWithTimeStamp prepares unique tenant name by appending current time-stamp value
+func GetTenantNameWithTimeStamp(vmgroupName string) string {


you know there is an open issue of avoid using strconv.FormatInt(time.Now().Unix(), 10) approach and use random string instead.

govint · 2017-06-09T06:32:41Z

tests/e2e/vmgroupbasic_test.go

+	if s.config == nil {
+		c.Skip("Unable to retrieve test config, skipping vmgroupbasic.TestVmgroupLs.")
+	}
+	out, err := admincli.ConfigInit(s.config.EsxHost)


Its incorrect to init and cleanup the DB for each test. The DB is a part of the system being tested. If a bunch of ops are performed on the DB we need to verify that the DB itself is sane at the end of the test. Customers aren't going to be cleaning up their DB every now and then. ConfigInit must be done either on the ESX host before the test or done once at the start of all tests.

govint · 2017-06-09T06:32:59Z

tests/e2e/vmgroupbasic_test.go

+
+// Removes config init
+func (s *vgBasicSuite) TearDownSuite(c *C) {
+	out, err := admincli.ConfigRemove(s.config.EsxHost)


Incorrect to remove config DB.

govint · 2017-06-09T06:47:57Z

tests/e2e/vmgroupbasic_test.go

+}
+
+func (s *vgBasicSuite) TearDownTest(c *C) {
+	out, err := admincli.DeleteVMgroup(s.config.EsxHost, s.tenantName)


If something is done in setup-suite then undo it in teardown-suite. Unless its something to be done after each test. Is that the case here.

govint · 2017-06-09T06:57:35Z

tests/e2e/vmgroupbasic_test.go

+func (s *vgBasicSuite) TestVmgroupLs(c *C) {
+	misc.LogTestStart("", c.TestName())
+
+	out, err := admincli.CreateVMgroup(s.config.EsxHost, s.tenantName, s.config.DockerHostNames[0])


Admin cli doesn't return errors :-( so while these checks are there for asserting nil error, its doing nothing. The CLI command could have failed and we know nothing about it. The CLI shell command itself doesn't exit with a non-zero exit status to check this.

govint · 2017-06-09T07:13:00Z

tests/e2e/vmgroupbasic_test.go

+	log.Printf("VM[%s]'s current power state is [%s]", s.config.DockerHostNames[0], properties.PoweredOnState)
+	c.Assert(powerState, Equals, properties.PoweredOnState, Commentf("VM [%s] should be powered on state", s.config.DockerHostNames[0]))
+
+	vmProcessID := esxcli.GetVMProcessID(s.config.EsxHost, s.config.DockerHostNames[0])


esxcli and govc routines are all doing the same thing - getting/setting data on ESX. May be then create an esx.go module and add functions there to GetVMPowerState(), KillVMByName(name) (will get the process ID and kill the VM), WaitVMPowerState(state), etc. I can add that or you can do this. Let me know. The misc.WaitForExpectedState() should move that esx module.

I will add it.

govint · 2017-06-09T07:50:31Z

tests/utils/admincli/vmgroupmgmt.go

+// GetVmgroupVMs - lists all the VMs of a particular vmgroup
+// [] Empty array is returned if no VMs exist in vmgroup
+func GetVmgroupVMs(esxIP, vmGroupName string) []string {
+	log.Printf("Getting all the VMs belonging to a particular tenant [%s] \n", vmGroupName)


"particular vmgroup"

govint · 2017-06-09T08:02:27Z

tests/utils/admincli/vmgroupmgmt.go

+	cmd := admincli.ListVmgroupVMs + vmGroupName + " 2>/dev/null | cut -d ' ' -f 3 "
+	out, _ := ssh.InvokeCommand(esxIP, cmd)
+	vmNames := strings.Fields(out)
+	vmList := vmNames[1:len(vmNames)]


can specify vmNames[1:] meaning from index 1 till last index.

govint · 2017-06-09T08:06:22Z

tests/utils/admincli/vmgroupmgmt.go

+	if out == "" {
+		return false
+	}
+	log.Printf("out : \n %s", out)


Can remove if its only debug.

govint · 2017-06-09T08:09:24Z

tests/utils/admincli/vmgroupmgmt.go

+		return false
+	}
+	log.Printf("out : \n %s", out)
+	return strings.Contains(out, vmgroupName)


strings.Contains can handle "" strings. Can just return strings.Contains(out, vmgroupName) at line 185

govint · 2017-06-09T08:10:03Z

tests/utils/inputparams/testparams.go

@@ -142,3 +144,8 @@ func GetTestConfig() *TestConfig {

 	return config
 }
+
+// GetTenantNameWithTimeStamp prepares unique tenant name by appending current time-stamp value
+func GetTenantNameWithTimeStamp(vmgroupName string) string {


GetVmgroupNameWithTimeStamp

+1 ... GetVmgroupUniqueName to keep consistent with others.

msterin · 2017-06-13T23:31:21Z

@ashahi1 - can this be closed ? You can reopen it when it's ready to be reviewed and merged

ashahi1 · 2017-06-14T00:28:15Z

@msterin I am done with all the changes, will be sending out the code for review later today.

pshahzeb · 2017-06-14T08:43:59Z

tests/constants/admincli/cmd.go


 	// DefaultVMgroup referring name of default vmgroup
-	DefaultVMgroup = "_DEFAULT "
+	DefaultVMgroup = "_DEFAULT"


AFAIK, such spaces after a const are introduced to properly construct the remote command.
If you remove such spaces, please make sure all existing test pass correctly (i.e . CI passes)

pshahzeb · 2017-06-14T08:46:04Z

tests/e2e/defaultvmgroup_test.go

 	. "gopkg.in/check.v1"
 )

 const (
-	ErrorMsg = "This feature is not supported for vmgroup _DEFAULT."
+	ErrorMsg     = "This feature is not supported for vmgroup _DEFAULT."


Which error msg? Please rename this const to be more specific. defintelu not ErrorMsg .

changed to vgErrorMsg.

pshahzeb · 2017-06-14T08:51:33Z

tests/e2e/defaultvmgroup_test.go

-	ErrorMsg = "This feature is not supported for vmgroup _DEFAULT."
+	ErrorMsg     = "This feature is not supported for vmgroup _DEFAULT."
+	NotAvailable = "N/A"
+	defaultVG    = "defaultVG"


please rename this to defualtVGTest. it doesn't imply if this is a test var something we need to define for our test to pass.

pshahzeb · 2017-06-14T08:53:05Z

tests/e2e/defaultvmgroup_test.go

-	esxIP    string
-	hostName string
+	config      *inputparams.TestConfig
+	volumeName1 string


I can certainly imagine more volumes coming in as a part of this part. do you mind having this a slice instead of volumeName1, volumeName2 etc. it would only create inconvenience later.

pshahzeb · 2017-06-14T08:55:45Z

tests/e2e/defaultvmgroup_test.go


-	out, err := admincli.ReplaceVMFromVMgroup(s.esxIP, con.DefaultVMgroup, s.hostName)
+	out, err := admincli.ReplaceVMFromVMgroup(s.config.EsxHost, admincliconst.DefaultVMgroup, s.config.DockerHostNames[1])


why not s.config.DockerHostNames[0] ?
Is it due to something due to env dependency? please call out in PR description so that all of us are aware about it.

I didn't get you. You are asking why I am using s.config.DockerHostNames[1]? Nothing specific, trying to replace DockerHostNames[1] and not DockerHostNames[0]

pshahzeb · 2017-06-14T08:59:16Z

tests/e2e/defaultvmgroup_test.go

+func (s *DefaultVMGroupTestSuite) TestDeleteDefaultVmgroupAndVerify(c *C) {
+	misc.LogTestStart(c.TestName())
+	s.volumeName1 = inputparams.GetUniqueVolumeName(defaultVG)
+	s.volumeName2 = inputparams.GetUniqueVolumeName(defaultVG)


can we use c.TestName() here instead of defaultVG

volume name becomes extremely long in that case. e.g. TestDeleteDefaultVmgroupAndVerify_volume_<Some 6-7 digit random number>

pshahzeb · 2017-06-14T09:02:12Z

tests/e2e/defaultvmgroup_test.go

+
+	// Create a volume - operation should fail as default vmgroup no longer exist
+	out, err = dockercli.CreateVolume(s.config.DockerHosts[0], s.volumeName2)
+	c.Assert(err, Not(IsNil), Commentf(out))


This check is good.
Do we also have another string that we can double check as part of out to indicate the scenario that vmgroup no longer exist.

Right before this step, I am already checking if default vmgroup exists or not. I am basically listing all the vmgroup using vmgroup ls cmd and then verifying if default vmgroup exist in there

Added additional check to verify the error message as well from vm.

pshahzeb · 2017-06-14T09:05:25Z

tests/e2e/defaultvmgroup_test.go

+	c.Assert(isVmgroupAvailable, Equals, false, Commentf("Falied to delete vmgroup %s .", admincliconst.DefaultVMgroup))
+
+	// Create a volume - operation should fail as default vmgroup no longer exist
+	out, err = dockercli.CreateVolume(s.config.DockerHosts[0], s.volumeName2)


Same steps are done for volume1 and then for volume 2.

Do you mind wrapping them in a func.

If there are more incoming test in this suite, it would be good to do this steps to do these steps as part of test setup/or not.

Feel free to make the call.

pshahzeb · 2017-06-14T09:08:26Z

tests/e2e/swarm_test.go

-	c.Assert(status, Equals, true, Commentf("Volume %s is still attached", s.volumeName))
-
-	log.Printf("END: swarm_test.TestDockerSwarm")
-}


I am assuming deletion of this file was by mistake. If not; why? //CC @shaominchen

Yes, added the file back.

shaominchen · 2017-06-19T18:40:00Z

tests/constants/properties/vmproperties.go

+
+const (
+	//PoweredOffState - vm powered-off
+	PoweredOffState = "poweredOff"


We have moved these 2 constants to a new file, but why the below moving file change shows "File renamed without changes."? Shouldn't we remove these VM related constants from volumeproperties.go?

BTW, the file name "volumeroperties.go" has a typo, please fix it.

shaominchen · 2017-06-19T18:41:05Z

tests/e2e/defaultvmgroup_test.go

+// 6. Create _DEFAULT vmgroup with —default-datastore=_VM_DS
+// 7. Again create new volume - operation should succedd
+// 8. Verify volume from esx and docker host
+func (s *DefaultVMGroupTestSuite) TestDeleteDefaultVmgroupAndVerify(c *C) {


The suffix "AndVerify" seems redundant.

shaominchen · 2017-06-19T18:45:33Z

tests/e2e/defaultvmgroup_test.go

+}
+
+// Create volumes
+func (s *DefaultVMGroupTestSuite) createVolume(c *C, hostIp, volumeName string) {


I'd suggest avoid passing *C to private functions. A private function should just take care of one specific task (for code reuse or whatever purpose), and return a result (e.g. string, boolean, error, etc.) that can be verified by the main test workflow using c.Assert().

shuklanirdesh82 · 2017-06-20T05:39:21Z

tests/constants/admincli/cmd.go

+	VMlist = " --vm-list="
+
+	// AddDatastoreToVMgroup adds datastore to vmgroup
+	AddDatastoreToVMgroup = vmdkopsAdmin + "vmgroup access add --name="


AddDatastoreToVMgroup sounds confusing .. I guess idea is to add datastore access to VMgroup. .. it can be simplified with name AddDatastoreAccess or AssignDatastoreAccess

shuklanirdesh82 · 2017-06-20T05:40:36Z

tests/e2e/defaultvmgroup_test.go

 	. "gopkg.in/check.v1"
 )

 const (
-	ErrorMsg = "This feature is not supported for vmgroup _DEFAULT."
+	vgErrorMsg   = "This feature is not supported for vmgroup _DEFAULT."
+	NotAvailable = "N/A"


is this required to be exported constant?

shuklanirdesh82 · 2017-06-20T05:41:59Z

tests/utils/inputparams/testparams.go

@@ -143,3 +144,12 @@ func GetTestConfig() *TestConfig {

 	return config
 }
+
+// GetRandomNumber returns random number
+func GetRandomNumber() string {


no need to be exported member .. getRandomNumber is sufficient.

shuklanirdesh82 · 2017-06-20T05:48:38Z

tests/utils/dockercli/volumelifecycle.go

+// VerifyVDVSIsRunning -  finds out if vDVS is running. This util can be useful
+// in scenarios where vm is powered-on and user wants to find out if
+// vm is fully up to be able to run docker volume commands.
+func VerifyVDVSIsRunning(ip string) bool {


volumelifecycle.go is not a better home to host this util .. Please move to appropriate file.

Created a separate file named vDVSUtil.go under tests/utils/verification/

shuklanirdesh82 · 2017-06-20T05:49:22Z

tests/utils/dockercli/volumelifecycle.go

+func VerifyVDVSIsRunning(ip string) bool {
+	log.Printf("Verifying if vDVS is running on vm : %s", ip)
+	maxAttempt := 20
+	waitTime := 5


minor: let's reduce this to 2 or 3

shuklanirdesh82 · 2017-06-20T06:00:01Z

tests/utils/admincli/vmgroupmgmt.go

 }

 // DeleteVMgroup method deletes a vmgroup and removes its volumes as well
 func DeleteVMgroup(ip, name string) (string, error) {
 	log.Printf("Deleting a vmgroup [%s] on esx [%s]\n", name, ip)
-	return ssh.InvokeCommand(ip, admincli.RemoveVMgroup+name+admincli.RemoveVolumes)
+	return ssh.InvokeCommand(ip, admincli.RemoveVMgroup+name)


what is the reason behind this change?

shuklanirdesh82 · 2017-06-20T20:05:24Z

tests/utils/admincli/vmgroupmgmt.go

 }

 // AddCreateAccessForVMgroup - set allow-create access on the vmgroup
 func AddCreateAccessForVMgroup(ip, name, datastore string) (string, error) {
 	log.Printf("Enabling create access for vmgroup %s, datastore %s on esx [%s] \n", name, datastore, ip)
-	return ssh.InvokeCommand(ip, admincli.SetAccessForVMgroup + name + " --allow-create True --datastore " + datastore)
+
+	// Following two lines are only for debugging purposes - will remove before final merge


minor: I guess CI is passed and this debug lines should be removed.

shuklanirdesh82 · 2017-06-20T20:06:49Z

tests/utils/admincli/vmgroupmgmt.go

@@ -84,3 +91,56 @@ func ConfigRemove(ip string) (string, error) {
 	log.Printf("Removing the SingleNode Config DB on esx [%s] \n", ip)
 	return ssh.InvokeCommand(ip, admincli.RemoveLocalConfigDb)
 }
+
+// CheckVmgroupAvailability - checks for a vmgroup in list of vmgroups
+func CheckVmgroupAvailability(esxIP, vmgroupName string) bool {


CheckVmgroupAvailability should be renamed to IsVMgroupPresent/Exist

shuklanirdesh82 · 2017-06-20T20:08:20Z

tests/utils/admincli/vmgroupmgmt.go

+}
+
+// CheckVolumeExistInVmgroup - verify volume's vmgroup is same as expected
+func CheckVolumeExistInVmgroup(esxIP, volName, vmgroupName string) bool {


minor: CheckVolumeExistInVmgroup => IsVolumeExistInVmgroup

shuklanirdesh82 · 2017-06-20T20:10:52Z

tests/utils/admincli/vmgroupmgmt.go

+	log.Printf("Getting vmgroup %s access rights to datastore %s ,  on esx [%s] \n", name, datastore, ip)
+	cmd := admincli.GetAccessForVMgroup + name + " 2>/dev/null | awk -v OFS='\t' '{print $1, $2}' | grep " + datastore
+	out, _ := ssh.InvokeCommand(ip, cmd)
+	accessRights := strings.Fields(out)


don't we have to check out is not empty? What happens when datastore access rule is not set for the vmgroup?

shuklanirdesh82 · 2017-06-20T20:12:26Z

tests/utils/admincli/vmgroupmgmt.go

+	cmd := admincli.CreateVMgroup + admincli.DefaultVMgroup + " --default-datastore " + admincli.VMHomeDatastore
+	_, err := ssh.InvokeCommand(ip, cmd)
+	if err != nil {
+		return false


need to add logging statement here like Error occurred while creating _DEFAULT vm group

shuklanirdesh82 · 2017-06-20T20:13:39Z

tests/utils/admincli/vmgroupmgmt.go

+	if err != nil {
+		return false
+	}
+	_, err = ssh.InvokeCommand(ip, admincli.AddDatastoreToVMgroup+admincli.DefaultVMgroup+" --datastore=_ALL_DS --allow-create ")


same as above ... is err field reliable?

shuklanirdesh82 · 2017-06-20T20:13:55Z

tests/utils/admincli/vmgroupmgmt.go

+	}
+	_, err = ssh.InvokeCommand(ip, admincli.AddDatastoreToVMgroup+admincli.DefaultVMgroup+" --datastore=_ALL_DS --allow-create ")
+	if err != nil {
+		return false


minor: need logging statement for better triaging.

shuklanirdesh82 · 2017-06-20T20:23:29Z

tests/e2e/defaultvmgroup_test.go

+// 4. Verify docker volume ls does not show any volumes
+// 5. Verify admin cli shows volume with VMGroup set as N/A
+// 6. Create _DEFAULT vmgroup with —default-datastore=_VM_DS
+// 7. Again create new volume - operation should succedd


nit: need to fix succedd

shuklanirdesh82 · 2017-06-20T20:24:44Z

tests/e2e/defaultvmgroup_test.go

+	NoVgErrorMsg := "VolumeDriver.Create: VM " + s.config.DockerHostNames[0] + " does not belong to any vmgroup"
+
+	// Create a volume
+	out, err := dockercli.CreateVolume(s.config.DockerHosts[0], s.volumeNames[0])


how about adding a check that only _default vmgroup is present?

shuklanirdesh82 · 2017-06-20T20:27:07Z

tests/e2e/defaultvmgroup_test.go

+	out, err = dockercli.CreateVolume(s.config.DockerHosts[0], s.volumeNames[1])
+	c.Assert(err, IsNil, Commentf(out))
+
+	// Check if volume was successfully created


need to add one more check here .. after recreation of default vmgroup .. previously created volume (L#136) should be available.

Note: comment is for L#146 in fact immediately after creating _default vmgroup.

shuklanirdesh82

This PR is having too many changes and making reviewer's life harder.

By taking this PR as an example, I would suggest to break this down in the future.

shuklanirdesh82 · 2017-06-20T20:41:24Z

tests/e2e/defaultvmgroup_test.go

+	c.Assert(isAvailable, Equals, true, Commentf("Volume %s is not available after creation", s.volumeNames[1]))
+
+	// Verify if volume exists in default vmgroup
+	isVolInVmgroup = admincli.CheckVolumeExistInVmgroup(s.config.EsxHost, s.volumeNames[1], admincliconst.DefaultVMgroup)


It is better to check about both volumes are exist and having _DEFAULT under vmgroup column and not N/A

shuklanirdesh82 · 2017-06-20T20:51:27Z

tests/e2e/vmgroupbasic_test.go

+
+const (
+	vmGroupName   = "vg_basictest"
+	NotApplicable = "N/A"


seen this again .. it is better to move to admincli const file.

shuklanirdesh82 · 2017-06-20T20:58:03Z

tests/e2e/vmgroupbasic_test.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// This test suite tries to verify if vmgroup ls command works fine even after tenant's vm is killed.


this information is not sufficient ... current file is holding 4 tests .. it is better to have some general comments.

shuklanirdesh82 · 2017-06-20T21:16:50Z

tests/e2e/vmgroupbasic_test.go

+	misc.LogTestStart(c.TestName())
+
+	// Create a vmgroup and add vm to it
+	log.Printf("Created a vm with default ds: %s", s.config.Datastores[0])


Created a vm with default ds? need to fix here

shuklanirdesh82 · 2017-06-20T21:37:05Z

tests/e2e/vmgroupbasic_test.go

+
+	// Remove the create privilege on the non-default vmgroup for specified datastore
+	admincli.RemoveCreateAccessForVMgroup(s.config.EsxHost, vmGroupName, s.config.Datastores[1])
+


need to make sure .. access is revoked.

pshahzeb

Looks good. Minor comments. Good to merge then

pshahzeb · 2017-06-21T23:01:48Z

tests/e2e/vmgroupbasic_test.go

+
+func (s *vgBasicSuite) SetUpTest(c *C) {
+	// Creating non-default vmgroup only if it does not exists
+	if !admincli.IsVmgroupPresent(s.config.EsxHost, vmGroupName) {


too nested. just return if Vmgroup is present.
If not then execute steps to create it. No need for nesting.

changed it.

pshahzeb · 2017-06-21T23:03:37Z

tests/e2e/vmgroupbasic_test.go

+	c.Assert(isVmgroupAvailable, Equals, false, Commentf("Failed to delete the vmgroup [%s] .", vmGroupName))
+
+	// Removing the DB at the end of suite
+	admincli.ConfigRemove(s.config.EsxHost)


check if this is successful.

Added a new util that gets the current config mode from esx

pshahzeb · 2017-06-21T23:07:59Z

tests/utils/admincli/vmgroupmgmt.go

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


nit: no need of empty line.

shaominchen

Overall looks good.

shaominchen · 2017-06-21T23:46:27Z

tests/e2e/defaultvmgroup_test.go

+	misc.LogTestEnd(c.TestName())
+}
+
+func (s *DefaultVMGroupTestSuite) isVolumeInVmgroup(esxIP string, volumeNames []string) bool {


We don't really need a receiver here. I'd suggest to remove the receiver so that we can move this private function to common util whenever needed.

Removed it.

shaominchen · 2017-06-21T23:53:52Z

tests/e2e/vmgroupbasic_test.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// This test suite contains tests to verify behaviors of non-default vmgroup


Can we please clarify the difference between vmgroupbasic_test and vmgroup_test? If necessary, we should update the comments in existing vmgroup_test as well to make sure the intentions of these two test groups are clear.

As discussed offline, added a statement to existing vmgroup_test and renamed my test file to vmgroupmisc_test

shaominchen

LGTM.

shuklanirdesh82 · 2017-06-22T05:29:15Z

tests/constants/admincli/cmd.go

@@ -56,14 +56,17 @@ const (
 	GetAccessForVMgroup = vmdkopsAdmin + "vmgroup access ls --name "

 	// ListVMgroups list vm groups
-	ListVMgroups = vmdkopsAdmin + "vmgroup ls"
+	ListVMgroups = vmdkopsAdmin + "vmgroup ls "


minor: do we really need "vmgroup ls " .. extra spacing.

shuklanirdesh82

Please make sure you have addressed all open comments.

shuklanirdesh82 · 2017-06-22T05:55:44Z

tests/utils/verification/vDVSUtil.go

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


nit: need to fix timeout status

shuklanirdesh82 · 2017-06-22T05:56:03Z

tests/utils/verification/vDVSUtil.go

+// VerifyVDVSIsRunning -  finds out if vDVS is running. This util can be useful
+// in scenarios where vm is powered-on and user wants to find out if
+// vm is fully up to be able to run docker volume commands.
+func VerifyVDVSIsRunning(ip string) bool {


VerifyVDVSIsRunning =>IsVDVSRunning

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

tests/utils/dockercli/volumelifecycle.go

@@ -242,3 +242,22 @@ func GetVolumeProperties(volumeName, hostName string) (string, error) {
 	cmd := dockercli.InspectVolume + volumeName + " --format ' {{index .Status.capacity.size}} {{index .Status.diskformat}} {{index .Status \"attached to VM\"}}' | sed -e 's/<no value>/detached/' "
 	return ssh.InvokeCommand(hostName, cmd)
 }
+
+// VerifyVDVSIsRunning -  finds out if vDVS is running. This util can be useful


this method should be removed ... you may want to fix the diff.

shuklanirdesh82 · 2017-06-22T06:01:18Z

@ashahi1 Can you please add issue# here? Which issue current PR is addressing?

govint

Need to review the remaining functions for the vmgroup tests.

We are now having three vmgroup test files, vmgroup, default and misc - please look at combining at least into two (default and misc)

govint · 2017-06-22T12:43:26Z

tests/constants/admincli/cmd.go

+	AddDatastoreAccess = vmdkopsAdmin + "vmgroup access add --name="
+
+	// NotApplicable or NotAvailable
+	NotApplicable = "N/A"


Wow! :-)) this is really not needed, constants should be for whats really usable across tests in different files. And this is something that simply can be used as "N/A" there's no need for a global for this sort of strings or ",", ".", etc.

Yes, that's how I had done initially - made it local to test. And then I was asked to put it into separate constant file. Now again bring it back to local test. Gosh!!

govint · 2017-06-22T12:43:58Z

tests/constants/admincli/vmdkops.go

@@ -31,4 +31,10 @@ const (

 	// StartService starts vmdkops service
 	StartService = vmdkopsd + "start"
+
+	// DBNotConfigured - DB mode not configured
+	DBNotConfigured = "NotConfigured"


Same as above just use a constant in the relevant test file.

This will be used by cross-esx tests when we start automating them. I will keep it here.

govint · 2017-06-22T13:18:45Z

tests/e2e/defaultvmgroup_test.go

+
+	// Verify if volume exists in default vmgroup
+	isVolInVmgroup := admincli.IsVolumeExistInVmgroup(s.config.EsxHost, s.volumeNames[0], admincliconst.DefaultVMgroup)
+	c.Assert(isVolInVmgroup, Equals, true, Commentf("Volume [%s] does not belong to vmgroup [%s]", s.volumeNames[0], admincliconst.DefaultVMgroup))


If this check and the above one can't be combined then please make a issue to have verification.CheckVolume() which will check against a bunch of volume properties. No need to have multiple checks on the same volume. I remember you wrote code to get a map of volume properties.

Can't that be used to make one check for group membership. Being able to get the volume properties itself shows the volume exits.

I will create a separate issue for this.

Issue #1470

govint · 2017-06-22T13:21:23Z

tests/e2e/defaultvmgroup_test.go

+
+	// Check volumes now again belong to default vmgroup
+	c.Assert(isVolumeInVmgroup(s.config.EsxHost, s.volumeNames), Equals, true, Commentf("Volume [%s] does not belong to vmgroup [%s]", s.volumeNames, admincliconst.DefaultVMgroup))
+


Same as above, the test function is more or less lengthy with repeat checks which is needed but can be a single function call to do these - verification.CheckVolume() which checks all specified properties vs. a call per check. Assertions can be done in CheckVolume() itself.

govint · 2017-06-22T13:24:33Z

tests/e2e/defaultvmgroup_test.go

+		if !isVolInVmgroup {
+			return false
+		}
+	}


Looks like we can move this into verification pkg into verification.CheckVolume() as mentioned above - seems like a common verification other tests can use.

+1 .. yeah I was thinking on the same note too.

govint · 2017-06-22T13:36:54Z

tests/e2e/vmgroupmisc_test.go

+
+	// Verify vm belongs to vmgroup
+	isVMPartofVg := admincli.IsVMInVmgroup(s.config.EsxHost, s.config.DockerHostNames[0], vmGroupName)
+	c.Assert(isVMPartofVg, Equals, true, Commentf("VM %s does not belong to vmgroup %s .", s.config.DockerHostNames[0], vmGroupName))


Or just add the VM always and ignore any error?

@govint I think verification is needed here cause if test fails because of vm not being there then we will have hard finding the actual issue. So it's better to have different verifications to save our time during debugging.

Pls. see comment above, we may not need this function if none of the tests are going to remove the VM from the vmgroup. If they did then this logic to check for vmgroup present and exit won;t work. If no test is removing the VM from the vmgroup then why this function to check and exit for every test, its unnecessary.

govint · 2017-06-22T13:38:23Z

tests/e2e/vmgroupmisc_test.go

+
+	// Verifying DB successfully removed
+	c.Assert(admincli.GetDBmode(s.config.EsxHost), Equals, admincliconst.DBNotConfigured, Commentf("Failed to remove the DB mode on ESX -  .", s.config.EsxHost))
+


Thats my point about setup and teardown test functions, its almost yet another test done for each and every test.

Can we skip these verifications in the setup/teardown functions. Because what you are doing here should be in a vmgroup create, add VM to vmgroup, create/delete config test.

Lets make these tests simpleer code and remove unnecessary verifications. Keep verifications only in the actual test functions. Asserting error here is sufficient.

Again, I was asked to verify if DB mode is as expected. Hence I added this check. We just go back and forth and delay the check-in.

And how do we decide which is necessary and which ones are unnecessary verifications? Do we really know where tests can fail? What if there was some issue with step 1 but test failed at step 4's verification. We will spend more time debugging because we think Step 4 failed whereas actually the issue was with Step 1.

I'd suggest the setup and tear down code (especially the test setup/teardown functions) really don;'t need verifications. Those are items to be done as tests and the setup and tear down can limit to asserting function return values alone. Verification should be just in the test functions.

/cc @shuklanirdesh82

We have already ran into an issue because clean-up was not verified. #1448 (comment)

govint · 2017-06-22T13:58:04Z

tests/e2e/vmgroupmisc_test.go

+
+	// Verify if volume exists in vmgroup
+	isVolInVmgroup := admincli.IsVolumeExistInVmgroup(s.config.EsxHost, s.volumeNames[0], vmGroupName)
+	c.Assert(isVolInVmgroup, Equals, true, Commentf("Volume [%s] does not belong to vmgroup [%s]", s.volumeNames[0], vmGroupName))


So, one verification.CheckVolume() should reduce code every where.

govint · 2017-06-22T14:04:14Z

tests/e2e/vmgroupmisc_test.go

+	c.Assert(isVmgroupAvailable, Equals, false, Commentf("Failed to delete the vmgroup [%s] .", vmGroupName))
+
+	// Verify vmgroup for volume is N/A
+	isVolInVmgroup = admincli.IsVolumeExistInVmgroup(s.config.EsxHost, s.volumeNames[0], admincliconst.NotApplicable)


using "N/A" is fine here or make it a local global string like
NullGroup = "N/A" using NotApplicable doesn't seem correct.

General comment - I had made it local only and then I was asked to put in a constant file in separate review. Now I have to again put it back to local. No wonder reviews take longer. :)

Fully agree, that keeps happening. But as code author I'd suggest you going over review comments and figuring out exactly whats really needed vs. good to have or specific code style. And only then making changes based on your judgement.

In this case a function like IsVolumeExistingInVmgroup() should get a vmgroup name always and hence a special case like NotApplicable doesn't seem correct. When we define an API, the params have a meaning and the values passed should reflect that. So defining a constant for a NullVmgroup as N/A and using that keeps the code understandable. And if no one else needs a NullVmgroup in their tests lets keep it locally. Thats the reasoning behind doing it this way.

govint · 2017-06-22T14:07:22Z

tests/e2e/vmgroupmisc_test.go

+	c.Assert(isVolInVmgroup, Equals, true, Commentf("Unexpected Behavior: Vmgroup [%s] for volume [%s] is not N/A. ", s.volumeNames[0], vmGroupName))
+
+	// Delete the orphaned volume which does not belong to any vmgroup
+	admincli.DeleteOrphanedVolume(s.config.EsxHost, s.volumeNames[0], vmGroupName, s.config.Datastores[0])


Why is this function needed? Why pass vmgroup, volume isn't in the group anymore and vmgroup is already deleted. Lets just use DeleteVolume instead?

It should be possible to delete a volume irrespective of where it is. Make a bug issue to have that supported or make --remove-volumes the default behavior whether user specifies --remove-volumes or not. There is no point allowing volumes to get orphaned and then not be able to delete them (if thats the case really, can you confirm).

Yes, we are not able to delete volumes for which there are no vmgroups - N/A. I will file an issue for it. Wrote this util to not pollute our test setup else we will have left-over volumes.

Thanks and this is a good find.

The tests should never create or use any method except whats advertised, documented as the way to handle the scenario. If a method doesn't exist then the test must not create one and instead that should become an issue.

shuklanirdesh82 · 2017-06-22T14:17:08Z

@ashahi1 Let's sync up offline today to break down your current review and we can merge irrelevant to new test + test util (keep E2E tests related to default vmgroup and non-default vmgroup #1381 related code). It helps to avoid conflict and getting some of the files stale.

govint · 2017-06-23T18:40:29Z

tests/e2e/vmgroupmisc_test.go

+	admincli.RemoveVMFromVMgroup(s.config.EsxHost, vmGroupName, s.config.DockerHostNames[0])
+
+	// Verify vm does not belongs to vmgroup
+	isVMPartofVg := admincli.IsVMInVmgroup(s.config.EsxHost, s.config.DockerHostNames[0], vmGroupName)


Nit - this verification isn't needed because setupTest() has placed the VM in the group and the tests aren't removing the VM from the group is that right?

I am removing the vm from vmgroup so I am just making sure removal was successful.

govint · 2017-06-23T18:42:01Z

tests/e2e/vmgroupmisc_test.go

+	// Creating non-default vmgroup only if it does not exists
+	if admincli.IsVmgroupPresent(s.config.EsxHost, vmGroupName) {
+		return
+	}


In which case is this code needed in the setupTest() at all. Say for the first test we add the VM to the VMGroup thereafter do we need to keep checking for every test? If none of the tests are moving the VM out of this VMGroup then there is no need for a setupTest() to keep checking and exiting for every test.

govint · 2017-06-23T18:43:58Z

tests/e2e/vmgroupmisc_test.go

+
+	// Verify vm belongs to vmgroup
+	isVMPartofVg := admincli.IsVMInVmgroup(s.config.EsxHost, s.config.DockerHostNames[0], vmGroupName)
+	c.Assert(isVMPartofVg, Equals, true, Commentf("VM %s does not belong to vmgroup %s .", s.config.DockerHostNames[0], vmGroupName))


Pls. see comment above, we may not need this function if none of the tests are going to remove the VM from the vmgroup. If they did then this logic to check for vmgroup present and exit won;t work. If no test is removing the VM from the vmgroup then why this function to check and exit for every test, its unnecessary.

govint · 2017-06-23T18:53:41Z

tests/e2e/vmgroupmisc_test.go

+	isAvailable = verification.CheckVolumeAvailability(s.config.DockerHosts[0], s.volumeNames[0])
+	c.Assert(isAvailable, Equals, false, Commentf("Unexpected Behavior: Orphaned volume %s "+
+		"is visible from host [%s] ", s.volumeNames[0], s.config.DockerHosts[0]))
+


I'd suggest skipping this test till these kind of volumes can be handled correctly.

OK. Will do it.

govint · 2017-06-23T19:22:24Z

tests/e2e/vmgroupmisc_test.go

+	c.Assert(isStatusChanged, Equals, true, Commentf("Failed to power-on the VM [%s]", s.config.DockerHostNames[0]))
+
+	// Verify vDVS successfully started
+	isVDVSRunning := verification.IsVDVSIsRunning(s.config.DockerHosts[0])


it should be like vdvs is available meaning he plugin is able to accept requests vs. running. But not for this PR.

govint

Pls.check tje coments provided and handle those.

pshahzeb

LGTM. Let the CI pass.
As you have created separate PR #1478 for some of the tests, please update the description of this PR.

shuklanirdesh82

Thanks for keeping patience and addressing all review comments.

ashahi1 self-assigned this Jun 9, 2017

ashahi1 requested review from shuklanirdesh82 and pshahzeb June 9, 2017 03:03

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

shuklanirdesh82 suggested changes Jun 9, 2017

View reviewed changes

govint suggested changes Jun 9, 2017

View reviewed changes

ashahi1 changed the title ~~Tenant ls command does not works if vm that was added to tenant is deleted.~~ [Test Automation] Verify vmgroup ls command works even after vmgroup's vm is killed Jun 9, 2017

ashahi1 mentioned this pull request Jun 9, 2017

Some e2e util functions are not generating unique numbers properly #1344

Closed

ashahi1 changed the title ~~[Test Automation] Verify vmgroup ls command works even after vmgroup's vm is killed~~ [Test Automation] [Do NOT Review]Verify vmgroup ls command works even after vmgroup's vm is killed Jun 9, 2017

ashahi1 changed the title ~~[Test Automation] [Do NOT Review]Verify vmgroup ls command works even after vmgroup's vm is killed~~ [Test Automation] [Do NOT Review] Verify vmgroup ls command works even after vmgroup's vm is killed Jun 9, 2017

ashahi1 changed the title ~~[Test Automation] [Do NOT Review] Verify vmgroup ls command works even after vmgroup's vm is killed~~ [Test Automation] [Do Not Review] Verify vmgroup ls command works even after vmgroup's vm is killed Jun 9, 2017

ashahi1 added the kind/test label Jun 9, 2017

shuklanirdesh82 changed the title ~~[Test Automation] [Do Not Review] Verify vmgroup ls command works even after vmgroup's vm is killed~~ [Do Not Review] Verify vmgroup ls command works even after vmgroup's vm is killed Jun 9, 2017

ashahi1 force-pushed the vmgroupls.ashahi1 branch from 0de596e to 4f28659 Compare June 13, 2017 22:28

ashahi1 force-pushed the vmgroupls.ashahi1 branch from 4f28659 to a54bcc6 Compare June 13, 2017 23:43

ashahi1 force-pushed the vmgroupls.ashahi1 branch 2 times, most recently from 1ceec05 to 3d882a0 Compare June 14, 2017 04:29

ashahi1 changed the title ~~[Do Not Review] Verify vmgroup ls command works even after vmgroup's vm is killed~~ E2E tests related to default vmgroup and non-default vmgroup Jun 14, 2017

pshahzeb suggested changes Jun 14, 2017

View reviewed changes

ashahi1 force-pushed the vmgroupls.ashahi1 branch from 3d882a0 to 9f3cf27 Compare June 14, 2017 17:38

This was referenced Jun 15, 2017

Admin CLI should return proper exit code when requested operation fails #1411

Closed

Code refactoring: Move esxcli and govc test utils & consts to single package #1426

Merged

ashahi1 force-pushed the vmgroupls.ashahi1 branch 2 times, most recently from 0602bc6 to aa16d47 Compare June 18, 2017 00:56

shaominchen reviewed Jun 19, 2017

View reviewed changes

ashahi1 force-pushed the vmgroupls.ashahi1 branch 2 times, most recently from 0b142ef to f2a84f0 Compare June 20, 2017 03:53

shuklanirdesh82 reviewed Jun 20, 2017

View reviewed changes

ashahi1 force-pushed the vmgroupls.ashahi1 branch from f2a84f0 to 78c31bf Compare June 21, 2017 08:29

pshahzeb reviewed Jun 21, 2017

View reviewed changes

shaominchen reviewed Jun 21, 2017

View reviewed changes

shaominchen approved these changes Jun 22, 2017

View reviewed changes

shuklanirdesh82 reviewed Jun 22, 2017

View reviewed changes

govint suggested changes Jun 22, 2017

View reviewed changes

This was referenced Jun 23, 2017

[5 tests] Automate P0 Tenancy testcases #1261

Closed

Code refactoring for e2e tests #1470

Closed

shuklanirdesh82 mentioned this pull request Jun 23, 2017

Various cosmetic fixes (random string generation fix) #1468

Merged

ashahi1 force-pushed the vmgroupls.ashahi1 branch from dec6cc3 to 23d3000 Compare June 23, 2017 15:18

govint reviewed Jun 23, 2017

View reviewed changes

govint approved these changes Jun 23, 2017

View reviewed changes

govint mentioned this pull request Jun 23, 2017

Automation test for removing user created vmgroup. #1474

Merged

ashahi1 force-pushed the vmgroupls.ashahi1 branch from be0210f to fc30072 Compare June 24, 2017 03:55

ashahi1 mentioned this pull request Jun 24, 2017

Adding vmgroup tests #1478

Merged

pshahzeb approved these changes Jun 25, 2017

View reviewed changes

Fix cleanup in vmdk_ops_test.py

d68050d

ashahi1 force-pushed the vmgroupls.ashahi1 branch from fc30072 to d68050d Compare June 26, 2017 23:42

shuklanirdesh82 approved these changes Jun 27, 2017

View reviewed changes

shuklanirdesh82 merged commit 76ebd29 into vmware-archive:master Jun 27, 2017

shuklanirdesh82 mentioned this pull request Jun 27, 2017

Unit test - test_vmdkops_on_default_tenant_vm test fails #1477

Closed

ashahi1 mentioned this pull request Jun 30, 2017

Create a util to get a bunch of volume properties #1527

Closed

1 task

shuklanirdesh82 mentioned this pull request Jul 9, 2017

Add a new util function to check if VM is fully back online #1563

Merged

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


		out, err := admincli.ReplaceVMFromVMgroup(s.esxIP, con.DefaultVMgroup, s.hostName)
		out, err := admincli.ReplaceVMFromVMgroup(s.config.EsxHost, admincliconst.DefaultVMgroup, s.config.DockerHostNames[1])


		// Remove the create privilege on the non-default vmgroup for specified datastore
		admincli.RemoveCreateAccessForVMgroup(s.config.EsxHost, vmGroupName, s.config.Datastores[1])


		// Check volumes now again belong to default vmgroup
		c.Assert(isVolumeInVmgroup(s.config.EsxHost, s.volumeNames), Equals, true, Commentf("Volume [%s] does not belong to vmgroup [%s]", s.volumeNames, admincliconst.DefaultVMgroup))


		// Verifying DB successfully removed
		c.Assert(admincli.GetDBmode(s.config.EsxHost), Equals, admincliconst.DBNotConfigured, Commentf("Failed to remove the DB mode on ESX - .", s.config.EsxHost))

E2E tests related to default vmgroup and non-default vmgroup #1381

E2E tests related to default vmgroup and non-default vmgroup #1381

Conversation

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

Choose a reason for hiding this comment

shuklanirdesh82 Jun 9, 2017 • edited Loading

Choose a reason for hiding this comment

msterin commented Jun 13, 2017

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

ashahi1 Jun 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashahi1 Jun 17, 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

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

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 Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

ashahi1 commented Jun 9, 2017 •

edited

Loading

shuklanirdesh82 Jun 9, 2017 •

edited

Loading

ashahi1 Jun 17, 2017 •

edited

Loading

ashahi1 Jun 17, 2017 •

edited

Loading

shuklanirdesh82 Jun 20, 2017 •

edited

Loading