Verify properties (disk-format, capacity, attached-to-vm) of a volume at vm and esx. #831

ashahi1 · 2016-12-20T23:10:54Z

Replaces and extends #825 (Add a test to verify the capacity of the volume created using vmkfstools)

====
Fixes #847 (Test to verify the 'Attached to VM' field for volume)
Fixes #848 (Test to verify 'Disk Format' field for volume )
Fixes #824 (End_to_End test to verify the size of volume using admin cli and docker cli )

I have tested this latest change by running tests locally on setup consisting of one Esx and two VMs.

Test output:
=== RUN TestVolumeProperties
2017/02/15 07:40:35 running end_to_end tests - current time: 2017-02-15 07:40:35.882135039 +0000 UTC
2017/02/15 07:40:35 Running test on VM - 10.160.196.62
2017/02/15 07:40:36 Docker version: Docker version 1.12.6, build 78d1802

2017/02/15 07:40:36 Creating a volume of Format Type - thin
2017/02/15 07:40:41
successfully created a volume -- dockerVol_1487144436
2017/02/15 07:41:08 Creating a volume of Format Type - zeroedthick
2017/02/15 07:41:13
successfully created a volume -- dockerVol_1487144468
2017/02/15 07:41:40 Creating a volume of Format Type - eagerzeroedthick
2017/02/15 07:41:45
successfully created a volume -- dockerVol_1487144500
--- PASS: TestVolumeProperties (96.77s)
PASS

msterin · 2016-12-20T23:42:58Z

The test LGTM. Do we still need #825 ??

shuklanirdesh82 · 2016-12-20T23:45:49Z

esx_service/vmdk_ops_test.py

+            self.assertEqual(os.path.isfile(self.name), True,
+                             "VMDK {0} is missing after create.".format(self.name))
+            response = vmdkops_admin.get_vmdk_size_info(path=self.name)
+            head = s.rstrip('bgtmk')


why do we have 'k' here?

'k' was only added for KB

Please remove that as input is sizes = ['21gb', '20tb', '200mb']

OK. I will remove 'k'.

shuklanirdesh82 · 2016-12-20T23:48:46Z

esx_service/vmdk_ops_test.py

+            response = vmdkops_admin.get_vmdk_size_info(path=self.name)
+            head = s.rstrip('bgtmk')
+            tail = s[len(head):]
+            self.assertEqual(response["capacity"], "{0:.2f}".format(float(head))+ tail.upper(),\


we can simply compare the outcome of response["capacity"] with the expected in this case 's' .. there is no need for extra steps of head and tail ..

If size of vmdk is 21gb, response we are getting is 21.00GB.
Objective behind passing 21gb to create_vmdk is to verify create_vmdk works if we specify unit is small.

ashahi1 · 2016-12-21T00:02:00Z

@msterin No, we do not need #825 after this PR #831 . I wasn't sure if I can close it hence I left it open for the time being.

shuklanirdesh82 · 2016-12-21T21:50:41Z

need to extend verification highlighted at #835

shuklanirdesh82 · 2016-12-21T21:26:00Z

esx_service/vmdk_ops_test.py

@@ -38,6 +38,7 @@
 import auth_api
 import error_code
 import vmdk_utils
+import vmdkops_admin


Come across #472 ... I would suggest to limit the dependency on the product code/module and breakdown the utility method for testing purpose.

On a side note this test case was for vmdk_ops and we are creating unnecessary dependency on vmdkops_admin for consuming utility from _admin.

msterin · 2017-01-10T02:59:23Z

this is quite old .... any updates ?

shuklanirdesh82 · 2017-01-11T00:50:33Z

spoke to @ashahi1 , he is currently working on the change and will update the PR soon.

shuklanirdesh82 · 2017-01-23T07:12:44Z

@ashahi1 seeing some unrelated commits to your PR. You may want to refresh the commit by squashing correctly.

msterin · 2017-01-23T21:01:12Z

@ashahi1 - if the PR is still being worked on, please close it for now and reopen when it's ready.
If it is ready, then please squash commits properly , as @shuklanirdesh82 requested (no unrelated stuff please) and ping us to review.

ashahi1 · 2017-01-23T21:06:45Z

@msterin Yes, I will squash the commits and ping for review.

ashahi1 · 2017-01-25T18:21:40Z

@msterin @shuklanirdesh82 @kerneltime - Please review the changes

Test tries to ssh to photon vm and esx and verifies the capacity, disk format, attached-to-vm fields are same for volume at the vm as well as at the esx.

We do the same steps across both the vms.

shuklanirdesh82 · 2017-01-26T04:48:04Z

vmdk_plugin/E2E_Tests/e2e_verifyVolmProperties_test.go

+		op := TestUtil.ExecuteCmd(cmd, vms[i])
+		if strings.Contains(op, "testVolume_0") {
+			cmd := "docker volume rm testVolume_0"
+			TestUtil.ExecuteCmd(cmd, esx)


esx ?? This line should be failed ... what is the value set for esx?

Using InvokeCommand method.

shuklanirdesh82 · 2017-01-26T04:52:34Z

vmdk_plugin/E2E_Tests/e2e_verifyVolmProperties_test.go

@@ -0,0 +1,130 @@
+package e2e_test


need copyright information

Added the copyright information.

shuklanirdesh82 · 2017-01-26T04:53:15Z

vmdk_plugin/E2E_Tests/e2e_verifyVolmProperties_test.go

+	}
+}
+
+func TestVolumeProperties(t *testing.T) {


It should be better to have test steps information to get better review; steps would be helpful for future reference too.

Added the steps.

shuklanirdesh82 · 2017-01-26T05:36:34Z

vmdk_plugin/E2E_Tests/e2e_verifyVolmProperties_test.go

+}
+
+func TestVolumeProperties(t *testing.T) {
+	defer teardownFunction()


isn't this redundant?

shuklanirdesh82 · 2017-01-26T05:37:51Z

vmdk_plugin/E2E_Tests/e2e_verifyVolmProperties_test.go

+		dkrVrsn := TestUtil.GetDockerVersion(vms[j])
+		fmt.Println("---- ", dkrVrsn)
+		if strings.Contains(dkrVrsn, "Docker version 1.11.") {
+			fmt.Println("Since docker version is less than 1.12, skipping test on this host")


we have decided to skip the verification not the test, need to address that comment.

shuklanirdesh82 · 2017-01-26T05:44:27Z

vmdk_plugin/utils/test_util/VolumeCreationTestUtil.go

+func GetDiskFormat_AdminCli(volName string, hostname string) string {
+	cmd := "/usr/lib/vmware/vmdkops/bin/vmdkops_admin.py ls -c volume,disk-format | grep " + volName
+	io := ExecuteCmd(cmd, hostname)
+	if strings.Contains(io, "stty: standard input: Inappropriate ioctl for device") {


#58 - #60 can be replaced by appending 2>/dev/null to the command as it is being ignored here

shuklanirdesh82 · 2017-01-26T05:47:18Z

vmdk_plugin/utils/test_util/VolumeCreationTestUtil.go

+	io := ExecuteCmd(cmd, hostname)
+	volProps := strings.Fields(io)
+	i := 0
+	for range volProps {


can you help me to understand this code block? or you may want to add some inline comment for better review

Added the comments to the function

shuklanirdesh82 · 2017-01-26T05:48:43Z

vmdk_plugin/utils/test_util/VolumeCreationTestUtil.go

+// returns attached to vm field of volume using docker cli
+func GetVmAttachedToVol_DockerCli(volName string, hostname string) string {
+	cmd := "docker volume inspect " + volName
+	io := ExecuteCmd(cmd, hostname)


this code has been copied atleast 3 times .. it should be properly organized by moving into separate util.

shuklanirdesh82 · 2017-01-26T05:51:41Z

vmdk_plugin/utils/test_util/VolumeCreationTestUtil.go

+}
+
+// ssh to the vm and executes the command
+func ExecuteCmd(cmd string, hostname string) string {


we do have InvokeCommand .. please try to reuse or improve further by keeping only one copy

shuklanirdesh82 · 2017-02-14T06:29:04Z

vmdk_plugin/E2E_Tests/e2e_verifyVolmProperties_test.go

+// method checks if the output from the ssh commands contains the expected values
+func contains(volmProps []string, op string) bool {
+	for i := 0; i < len(volmProps); i++ {
+		if !strings.Contains(op, volmProps[i]) {


oh wait! some confusion here .. what if ssh command's output contains additional values compare with the expected one? We are going to miss some serious bug if additional output returns back.

shuklanirdesh82 · 2017-02-14T06:30:35Z

vmdk_plugin/E2E_Tests/e2e_verifyVolmProperties_test.go

+}
+
+// method checks if the output from the ssh commands contains the expected values
+func contains(volmProps []string, op string) bool {


need to improve method name .. it is misleading and lead to confusion with strings.contain() method

shuklanirdesh82 · 2017-02-14T06:32:30Z

vmdk_plugin/utils/volumeProperties/VolumePropertiesUtil.go

@@ -0,0 +1,91 @@
+// Copyright 2017 VMware, Inc. All Rights Reserved.


need to rename this file name something like VolumePropertiesVerificationUtil.go certainly not VolumePropertiesUtil.go

shuklanirdesh82 · 2017-02-14T06:34:53Z

vmdk_plugin/utils/volumeProperties/VolumePropertiesUtil.go

+func GetVmAttachedToVolUsingDockerCli(volName string, hostname string) string {
+	cmd := DOCKER_CLI_INSPC + " --format '{{index .Status \"created by VM\"}}' " + volName
+	op := ExecCmd(hostname, cmd)
+	return strings.TrimSpace(string(op))


some confusion here .. ExecCmd returns string .. I don't think we need to convert it again at line 35

shuklanirdesh82 · 2017-02-14T06:43:16Z

vmdk_plugin/utils/volumeProperties/VolumePropertiesUtil.go

+	cmd := ADMIN_CLI_LS + "-c volume,attached-to 2>/dev/null | grep " + volName
+	op := ExecCmd(hostname, cmd)
+	volProps := strings.Fields(op)
+	return string(volProps[1])


This raises a serious concern by taking account of the common code...

some thoughts

what happens op is null or empty i.e. the asked volume not found .. this util will not be useful

let's say there is a bug in the system and return duplicates .. this approach is dangerous as it is returning 1st index.

shuklanirdesh82 · 2017-02-14T06:43:44Z

vmdk_plugin/utils/volumeProperties/VolumePropertiesUtil.go

+func GetVolumePropertiesAdminCli(volName string, hostname string) string {
+	cmd := ADMIN_CLI_LS + "-c volume,attached-to,capacity,disk-format 2>/dev/null | grep " + volName
+	op := ExecCmd(hostname, cmd)
+	return string(op)


same as above

shuklanirdesh82 · 2017-02-14T06:44:12Z

vmdk_plugin/utils/volumeProperties/VolumePropertiesUtil.go

+func GetVolumePropertiesDockerCli(volName string, hostname string) string {
+	cmd := DOCKER_CLI_INSPC + " --format '{{index .Status.capacity.size}} {{index .Status.diskformat}} {{index .Status \"attached to VM\"}}' " + volName
+	op := ExecCmd(hostname, cmd)
+	return string(op)


same as above

shuklanirdesh82 · 2017-02-14T06:45:08Z

vmdk_plugin/utils/volumeProperties/VolumePropertiesUtil.go

+//method takes command and host and calls InvokeCommand
+//and then returns the output after converting to string
+func ExecCmd(hostname string, cmd string) string {
+	io, err := TestUtil.InvokeCommand(hostname, cmd)


io .. what does it mean?

shuklanirdesh82 · 2017-02-14T06:48:26Z

vmdk_plugin/utils/volumeProperties/VolumePropertiesUtil.go

+// So this method can be useful if we
+// do not want to run certain verifications
+// on docker 1.11
+func IsDockerCliCheckNeeded(hostname string) bool {


does it hold IPAddr or Name?

IMO, variable name should reflect its meaning.

shuklanirdesh82

Overall looks fine to me ... supplying some comments for your review.

msterin · 2017-02-15T02:23:01Z

vmdk_plugin/E2E_Tests/e2e_verifyVolmProperties_test.go

+
+var (
+	esx              string = os.Getenv("ESX")
+	volSizes                = []string{"100MB"}


nit: the code is either misaligned or has some tabs - can you check it ?

msterin · 2017-02-15T02:26:19Z

vmdk_plugin/utils/test_util/VolumePropertiesVerificationUtil.go

+	}
+	if len(strings.Fields(op)) != expctedLen {
+		log.Println("Actual docker cli inspect output when looking for size, disk-format and attached to vm - ", string(op))
+		log.Fatal("Docker cli inpect output is expected to consist of three elements only - size, disk-format and attached-to-vm status")


the overall message looks weird Why to Prinln and then Fatal instead of single Fatal , multi-lined ? That applies to multiple places where PrintLn();Fatal() is used

shuklanirdesh82

It seems some of the comments were not addressed so providing required changes and providing contingent approval on the CI pass after addressing few comments.

shuklanirdesh82 · 2017-02-16T08:37:23Z

vmdk_plugin/E2E_Tests/e2e_verifyVolmProperties_test.go

+// Properties being verified - capacity, disk-format and vm-attached field.
+
+// Test assumes that SSH cert has been setup to enable password-less login
+package e2e_test.


nit: need to remove period . from package name.

shuklanirdesh82 · 2017-02-16T08:44:23Z

vmdk_plugin/E2E_Tests/e2e_verifyVolmProperties_test.go

+        formatTypes = []string{"thin", "zeroedthick", "eagerzeroedthick"}
+        vmIp string = ""
+        dockerVolmRmvCmd string = "docker volume rm "
+        dockerCliCheck bool = true


no need to initialize value here ... let the method decide what to set for dockerCliCheck

shuklanirdesh82 · 2017-02-16T08:48:49Z

vmdk_plugin/E2E_Tests/e2e_verifyVolmProperties_test.go

+					volmPropertiesDkrCli := TestUtil.GetVolumePropertiesDockerCli(volName, vms[vmIndx])
+					expctdPropsDkr := []string{"100MB", formatTypes[k], "<no value>"}
+					if !hasElement(expctdPropsDkr, volmPropertiesDkrCli) {
+						log.Fatal("Volume properties on admin cli do not matches the expected values")


logging statement needs to be corrected ... while checking against docker cli outcome; logging should not be about "admin cli"

shuklanirdesh82 · 2017-02-16T08:50:59Z

vmdk_plugin/E2E_Tests/e2e_verifyVolmProperties_test.go

+					volmPropertiesDkrCli := TestUtil.GetVolumePropertiesDockerCli(volName, vms[vmIndx])
+					expctdPropsDkr := []string{"100MB", formatTypes[k]}
+					if !hasElement(expctdPropsDkr, volmPropertiesDkrCli) {
+						log.Fatal("Volume properties on ESX fetcchced using admin cli do not matches the expected values")


same as above

logging is misleading

fetcchced ??

shuklanirdesh82 · 2017-02-16T08:52:30Z

vmdk_plugin/E2E_Tests/e2e_verifyVolmProperties_test.go

+						log.Fatal("Volume properties on admin cli do not matches the expected values")
+					}
+				}
+				TestUtil.InvokeCommand(vms[vmIndx], "docker run -d -v "+volName+":/vol --name e2e busybox tail -f /dev/null")


as discussed offline: better to randomize container name to avoid cascading failure if test stops abruptly

Run test across both the VMs. Cleaned up the code Unique name for each volume Used TestUtil.InvokeCommand Addressed review comments from Nirdesh Review comments from Nirdesh addressed Trivial change to restart CI run Modified the log output Minor change in log stmt to restart CI run CI didn't pick the last changes so pushing again Change to restart CI run Restarting CI run Run test on only one VM Trivial change so that CI run can restart Addressed recent comments from Nirdesh - still incomplete Addressed review comments Change to restart CI run Addressed Nirdesh's comments Minor change to restart CI run Modified test clean-up Previous changes were not picked up by drone, hence re-pushing to restart the CI run Restarting CI More logging to figure out time-out failure on one particular host Modified deploy-and-test-wrapper to only run end-to-end test Restarting CI run Trivial change to restart CI Restarting CI as previous run failed due to - Issue 887 Refactoring cleanup code Re-pushing to start new CI run. Last CI run failed because of - Issue 878 Repushing to restart CI run - Issue 887 Re-pushing to restart CI run as last run failed because of Issue - 887 Pushing to restart CI Push to restart CI run Contains changes from PR - 894 Running test-esx and test-vm as well with E2E tests Making changes so that test picks VM2 Repushing to restart CI Repushing to CI Repushing to start CI Push to restart CI Push to restart CI Addressed Mark's review comments Push to restart CI Reverted deploy-and-test-wrapper.sh Restart CI Addressed review comments from Nirdesh Reverted changes to misc/drone-scripts/setup.sh Restart CI Rebased feature branch with master Pushing to restart CI Pushing to restart CI run Created a seperate util class consisting of methods that return properties of volume like cpacity, disk-format and attached-to-vm fields using docker cli and admin cli Push to restart CI run Restarting CI Addressed latest review comments from Nirdesh Added the new file Removing unnecessary log statements Addressed review comments from Mark Re-pushing with trivial change to restart CI run as previous run failed due to - no route to host Improved logging Aligned a piece of code Push to restart CI Addressed latest review comments from Nirdesh. Trivial change to restart CI run as previous run failed due to - no route to host Saving the current changes Repushing after taking latest changes from vmware master Previous changes were not picked by CI, hence repushing Previous run failed due to 'no route to host' hence repushing to restart ci Pushing to restart CI.

msterin · 2017-02-17T00:22:43Z

vmdk_plugin/E2E_Tests/e2e_verifyVolmProperties_test.go

+				containerName = "busybox_" + strconv.FormatInt(time.Now().Unix(), 20)
+				log.Println("Creating a volume of Format Type - ", formatTypes[k])
+				volName := TestInputParamsUtil.GetVolumeNameWithTimeStamp("dockerVol")
+				_, err := TestUtil.InvokeCommand(vms[vmIndx], "docker volume create --driver=vmdk --name="+volName+" -o size="+volSizes[i]+" -o diskformat="+formatTypes[k])


nit - this line is WAY to looong.

ashahi1 self-assigned this Dec 20, 2016

vmwclabot added the cla-not-required label Dec 20, 2016

shuklanirdesh82 reviewed Dec 20, 2016

View reviewed changes

shuklanirdesh82 suggested changes Dec 20, 2016

View reviewed changes

msterin mentioned this pull request Dec 21, 2016

Add a test to verify the capacity of the volume created using vmkfstools #825

Closed

shuklanirdesh82 approved these changes Dec 21, 2016

View reviewed changes

shuklanirdesh82 mentioned this pull request Dec 21, 2016

Test to check the capacity of volume using AdminCLI #835

Closed

shuklanirdesh82 suggested changes Dec 21, 2016

View reviewed changes

shuklanirdesh82 changed the title ~~Verify the size of vmdk with the size reported by vmkfstools~~ Verify the size of vmdk with the size reported by vmkfstools and admin_cli command Dec 21, 2016

ashahi1 force-pushed the issueVerifyVmdkSize-new branch 2 times, most recently from 94e249e to a7ddef4 Compare January 12, 2017 23:44

ashahi1 force-pushed the issueVerifyVmdkSize-new branch 2 times, most recently from b7b49af to f2a3ff9 Compare January 24, 2017 17:21

shuklanirdesh82 suggested changes Jan 26, 2017

View reviewed changes

ashahi1 force-pushed the issueVerifyVmdkSize-new branch 7 times, most recently from 40140a7 to 7c8f7f3 Compare February 4, 2017 09:04

shuklanirdesh82 reviewed Feb 14, 2017

View reviewed changes

shuklanirdesh82 suggested changes Feb 14, 2017

View reviewed changes

ashahi1 force-pushed the issueVerifyVmdkSize-new branch from 4adade1 to 22c17e0 Compare February 15, 2017 00:40

msterin added the kind/test label Feb 15, 2017

msterin changed the title ~~[End-to-End test] Verify properties (disk-format, capacity, attached-to-vm) of a volume at vm and esx. Issue #847, #848 and #824.~~ Verify properties (disk-format, capacity, attached-to-vm) of a volume at vm and esx. Feb 15, 2017

msterin reviewed Feb 15, 2017

View reviewed changes

ashahi1 force-pushed the issueVerifyVmdkSize-new branch 3 times, most recently from 31b0a90 to a93c601 Compare February 15, 2017 17:58

shuklanirdesh82 approved these changes Feb 16, 2017

View reviewed changes

ashahi1 force-pushed the issueVerifyVmdkSize-new branch 5 times, most recently from 746abaa to ea6b455 Compare February 16, 2017 22:09

ashahi1 force-pushed the issueVerifyVmdkSize-new branch from ea6b455 to ec9c6e9 Compare February 16, 2017 22:16

msterin reviewed Feb 17, 2017

View reviewed changes

Addressed Mark's comment to reduce the size of line as it was too long

baeaa5b

msterin approved these changes Feb 17, 2017

View reviewed changes

shuklanirdesh82 merged commit 52376c8 into vmware-archive:master Feb 17, 2017

ashahi1 deleted the issueVerifyVmdkSize-new branch June 8, 2017 00:12

		@@ -0,0 +1,91 @@
		// Copyright 2017 VMware, Inc. All Rights Reserved.

Verify properties (disk-format, capacity, attached-to-vm) of a volume at vm and esx. #831

Verify properties (disk-format, capacity, attached-to-vm) of a volume at vm and esx. #831

Conversation

ashahi1 commented Dec 20, 2016 • edited

msterin commented Dec 20, 2016

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 commented Dec 21, 2016

shuklanirdesh82 commented Dec 21, 2016

Choose a reason for hiding this comment

msterin commented Jan 10, 2017

shuklanirdesh82 commented Jan 11, 2017 • edited

shuklanirdesh82 commented Jan 23, 2017

msterin commented Jan 23, 2017 • edited

ashahi1 commented Jan 23, 2017

ashahi1 commented Jan 25, 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashahi1 commented Dec 20, 2016 •

edited

shuklanirdesh82 commented Jan 11, 2017 •

edited

msterin commented Jan 23, 2017 •

edited