[Automation] Restart test 1 on plugin side #1265

pshahzeb · 2017-05-16T18:13:11Z

This PR automates test 1 from #1252

Its the part of e2e tests

Usage for gocheck : http://labix.org/gocheck

=== RUN   TestRestart
Creating volume [abc_1494954324] on VM [10.192.99.52]
Attaching volume [abc_1494954324] on VM[10.192.99.52]
Killing docker on VM [10.192.99.52]
Confirming detached status for volume [abc_1494954324]
Destroying volume [abc_1494954324]
OK: 1 passed
--- PASS: TestRestart (37.61s)

shaominchen

Great job done - overall looks neat and clean! A few comments below.

shaominchen · 2017-05-17T03:41:05Z

tests/e2e/restart_test.go

+package e2e
+
+import (
+	"os"


Let's use 4-space indention consistently.

gofmt does that part ... no need to perform such step.

shaominchen · 2017-05-17T03:41:11Z

tests/e2e/restart_test.go

+	"os"
+	"testing"
+
+	. "gopkg.in/check.v1"


Please add the reference link (http://labix.org/gocheck) in the PR description to make the reviewer's life easier :)

shaominchen · 2017-05-17T03:41:43Z

tests/e2e/restart_test.go

+
+func (s *PluginSuite) SetUpTest(c *C) {
+	s.volumeName = testparams.GetVolumeNameWithTimeStamp("abc")
+	s.hostName = os.Getenv("VM2")


Any specific reason to use VM2 instead of VM1?

Was debugging failure on CI, if it is photon(vm1) or ubuntu(vm2) issue.
It wasn't the reason. This can be either VM

shaominchen · 2017-05-17T03:43:29Z

tests/utils/dockercli/VolumeCreationTestUtil.go

+// AttachVolume - attach volume to container on given host
+func AttachVolume(host, volName, containerName string) ([]byte, error) {
+	fmt.Printf("Attaching volume [%s] on VM[%s]\n", volName, host)
+	return InvokeCommand(host, "docker run -d -v "+volName+


Nit: let's add space before and after + operator.

BTW, are you using VS Code as the editor? VS Code plugins should be able to correct code style issues automatically.

same as above .. gofmt runs as part of the build target which auto corrects the formatting.

gofmt removes the spaces

shaominchen · 2017-05-17T03:53:24Z

tests/utils/dockercli/VolumeCreationTestUtil.go

+		" busybox tail -f /dev/null")
+}
+
+// DeleteVolume - deletes the created volume as per passed volume name.
 func DeleteVolume(name string, ip string) ([]byte, error) {


I noticed that the APIs defined in these utils are not consistent: some APIs have hostName (or ip) as the 1st parameter, some others have it as the 2nd or last parameter. This inconsistency makes the APIs difficult to use. Can we fix all of them together?

shaominchen · 2017-05-17T03:55:49Z

tests/utils/verification/VolumePropertiesVerificationUtil.go

+}
+
+// PollStatusDetached - check if the status gets detached within the timeout
+func PollStatusDetached(name, ip string, timeout int) bool {


Can we improve this API to make it generic to support polling both attached and detached status?

Moreover, I'd prefer not to expose the timeout parameter, because it will not only simplify the API client, but also avoid changing multiple places if the default timeout value turns out inappropriate (too long or too short).

Done. not exposing timeout and making it constant as of now.

shuklanirdesh82 · 2017-05-17T20:03:10Z

tests/e2e/restart_test.go

+
+	out, err = dockercli.AttachVolume(s.hostName, s.volumeName, s.containerName)
+	c.Assert(err, IsNil, Commentf(vf.FormatOutput(out)))
+


better to add verification step using docker cli as well as admincli to check whether volume is attached or not.

shuklanirdesh82 · 2017-05-17T20:13:34Z

tests/utils/verification/volumeproperties.go

@@ -107,6 +141,11 @@ func ExecCmd(hostname string, cmd string) string {
 	return string(out[:])
 }

+// FormatOutput - convert output of a command to a string
+func FormatOutput(out []byte) string {


It is better to be part of separate util, definitely not as verification util.

shuklanirdesh82 · 2017-05-17T20:14:07Z

tests/utils/verification/volumeproperties.go

 	sshutil "github.com/vmware/docker-volume-vsphere/tests/utils/dockercli"
 )

 var adminCliLs = "/usr/lib/vmware/vmdkops/bin/vmdkops_admin.py volume ls "
 var dockerCliInspect = "docker volume inspect "

+const (
+	//DetachedStatus - volume detached
+	DetachedStatus = "detached"


minor: you may want to consider it moving to separate constant file.

moving it to constants package

shuklanirdesh82 · 2017-05-17T20:15:09Z

tests/utils/dockercli/volumelifecycle.go

+	return InvokeCommand(ip, "docker rm -f "+containerName)
+}
+
+// InvokeCommand - can be consumed by test directly to invoke


as you are here: can you please move this util out of this file?

shuklanirdesh82 · 2017-05-17T20:16:32Z

tests/utils/dockercli/volumelifecycle.go

+}
+
+// CleanContainer - clean the container (stops and removes it)
+func CleanContainer(ip, containerName string) ([]byte, error) {


CleanContainer sounds confusing to me .. would it be better to rename as RemoveContainer or "DeleteContainer" .... would prefer RemoveContainer instead

shuklanirdesh82 · 2017-05-17T20:24:16Z

tests/e2e/restart_test.go

+
+	status := vf.PollStatus(s.volumeName, vf.DetachedStatus, s.hostName)
+	c.Assert(status, Equals, true, Commentf("Volume %s is still attached", s.volumeName))
+


need to add verification step using admin_cli volume ls

Yes. Thanks for pointing this.
Added verification for attach and detach. Keeping these checks separate as of now because,
in attached mode, we also verify the the VM name to which volume is attached is same on docker host and esx side.
Also we dont really need to keep trying again to verify attach status, it only happens in detach.

shaominchen · 2017-05-17T21:54:30Z

tests/e2e/restart_test.go

+)
+
+// Hook up gocheck into the "go test" runner.
+func TestRestart(t *testing.T) { TestingT(t) }


Nirdesh just did a test - this hook up should be done only once in the whole test group. Otherwise, if we add another test file which also set this hook-up, then all the test cases in two files will be run twice.

So we should have a separate file to hook up this only once.

moving this to init_test.go

shuklanirdesh82

over all looks good to me .. please have a look at some minor comments

shuklanirdesh82 · 2017-05-17T23:36:28Z

tests/constants/admincli/cmd.go

@@ -25,4 +25,9 @@ const (

 	// ListVolumes referring to vmdkops_admin volume ls
 	ListVolumes = vmdkopsAdminVolume + "ls "
+
+	//DetachedStatus - volume detached


admincli is not better home .. please move this const out .. it leads to confusion.

shuklanirdesh82 · 2017-05-17T23:37:20Z

tests/e2e/init_test.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package e2e


minor: Please add the package level description here to shed some idea on objective of this file.

shuklanirdesh82 · 2017-05-17T23:37:54Z

tests/e2e/restart_test.go

+	s.volumeName = inputparams.GetVolumeNameWithTimeStamp("abc")
+	s.hostName = os.Getenv("VM2")
+	s.esxName = os.Getenv("ESX")
+	s.containerName = "busybox_test"


better to randomize this name too.

shuklanirdesh82 · 2017-05-17T23:40:58Z

tests/utils/dockercli/volumelifecycle.go

-//	remoteHostIP value
-func InvokeCommand(remoteHostIP string, cmd string) ([]byte, error) {
-	return exec.Command("/usr/bin/ssh", append(sshIdentity, "root@"+remoteHostIP, cmd)...).CombinedOutput()
+// RemoveContainer - clean the container (stops and removes it)


nit: clean the container sounds confusing .. need to improve the method inline doc

Can you suggest what needs to be improved?

clean the container (stops and removes it) => delete/remove the container forcefully (stops and removes/deletes it)

shuklanirdesh82 · 2017-05-17T23:42:15Z

vmdk_plugin/Makefile

@@ -101,7 +101,9 @@ SRC = main.go log_formatter.go utils/refcount/refcnt.go \

 TEST_SRC = ../tests/utils/inputparams/testparams.go \
 	../tests/utils/dockercli/volumelifecycle.go \
-	../tests/utils/verification/volumeproperties.go
+	../tests/utils/verification/volumeproperties.go \


no need to add all this here .. can you explain why is the need here?

Did it in alignment with other utils (line 102,103). required to build test files. If not required, should we remove all of them?

as far as I remember .. IIRC /tests/utils/inputparams/testparams.go is the only need .. can you verify by adjusting .. keep only the required one and knock of irrelevant.

shuklanirdesh82 · 2017-05-17T23:46:42Z

tests/utils/dockercli/volumelifecycle.go

-	return exec.Command("/usr/bin/ssh", append(sshIdentity, "root@"+remoteHostIP, cmd)...).CombinedOutput()
+// RemoveContainer - clean the container (stops and removes it)
+func RemoveContainer(ip, containerName string) ([]byte, error) {
+	return ssh.InvokeCommand(ip, "docker rm -f "+containerName)


couple things here

"docker rm -f" move as constant

need to check spacing before/after +

gofmt is removing space for me for + operators used inside parenthesis

moved all docker related constants to constants package

shuklanirdesh82 · 2017-05-17T23:49:30Z

tests/utils/verification/volumeproperties.go

+	vmAttachedHost := GetVMAttachedToVolUsingDockerCli(name, hostName)
+	vmAttachedESX := GetVMAttachedToVolUsingAdminCli(name, esxName)
+
+	return (vmAttachedHost == vmAttachedESX)


this is kind of alarming ... need to add one more check against our expectation. expectedAttachStatus == vmAttachedHost & vmAttachedESX

as discussed offline .. let's add inline comment that no way to know about expected vmname and need to rely on vmAttachedHost == vmAttachedESX until we come up with govmomi integration to know about the vm name.

We have a todo to verify the name of VM with the name from IP. Can be done incrementally.

can you point me to the line?

shuklanirdesh82 · 2017-05-17T23:52:39Z

tests/utils/verification/volumeproperties.go

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


would suggest to use log.Warn or log.Error instead of log.Printf that helps during debugging test failure

done. Using log.Fatalf.

shuklanirdesh82

Great work!! LGTM

shaominchen

All look great, except that we should have a better name for "restart_test". We can rename it later if there are any ambiguity with other new test suites.

vmwclabot added the cla-not-required label May 16, 2017

pshahzeb requested review from shuklanirdesh82 and shaominchen May 16, 2017 18:22

pshahzeb changed the title ~~Automating restart test 1 on plugin side~~ [Automation] Restart test 1 on plugin side May 16, 2017

shaominchen reviewed May 17, 2017

View reviewed changes

pshahzeb force-pushed the issue_1252.pshahzeb branch 2 times, most recently from 9781674 to 332d31b Compare May 17, 2017 18:41

shuklanirdesh82 suggested changes May 17, 2017

View reviewed changes

shuklanirdesh82 reviewed May 17, 2017

View reviewed changes

shaominchen reviewed May 17, 2017

View reviewed changes

pshahzeb force-pushed the issue_1252.pshahzeb branch from 332d31b to 75ac4f0 Compare May 17, 2017 22:48

shuklanirdesh82 reviewed May 17, 2017

View reviewed changes

test stale mounts

4a4f512

pshahzeb force-pushed the issue_1252.pshahzeb branch from e167e6f to 4a4f512 Compare May 18, 2017 01:50

shuklanirdesh82 approved these changes May 18, 2017

View reviewed changes

shaominchen approved these changes May 18, 2017

View reviewed changes

pshahzeb merged commit 2de15ce into master May 18, 2017

shuklanirdesh82 deleted the issue_1252.pshahzeb branch May 18, 2017 04:41

shuklanirdesh82 added the kind/test label May 18, 2017

pshahzeb mentioned this pull request May 24, 2017

[3 tests] Automate refcount P0 testcases #1252

Closed

3 tasks

pshahzeb mentioned this pull request Jun 19, 2017

Added tests for restarting docker service. #1428

Merged


		out, err = dockercli.AttachVolume(s.hostName, s.volumeName, s.containerName)
		c.Assert(err, IsNil, Commentf(vf.FormatOutput(out)))


		status := vf.PollStatus(s.volumeName, vf.DetachedStatus, s.hostName)
		c.Assert(status, Equals, true, Commentf("Volume %s is still attached", s.volumeName))

[Automation] Restart test 1 on plugin side #1265

[Automation] Restart test 1 on plugin side #1265

Conversation

pshahzeb commented May 16, 2017 • edited Loading

shaominchen 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

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

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

shaominchen left a comment • edited Loading

Choose a reason for hiding this comment

pshahzeb commented May 16, 2017 •

edited

Loading

shaominchen left a comment •

edited

Loading