Add swarm_test to e2e test suite #1309

shaominchen · 2017-05-30T19:00:39Z

Add swarm_test to e2e test suite: fixes issue #1249.

Passed all e2e tests locally.

shuklanirdesh82 · 2017-06-01T00:55:53Z

@shaominchen can you please increase default timeout for running our e2e test to 30 mins -timeout 30m?

shuklanirdesh82

Overall looks OK to me, supplying some comments for your review.

shuklanirdesh82 · 2017-06-01T01:31:45Z

tests/e2e/swarm_test.go

+	log.Printf("START: swarm_test.TestDockerSwarm")
+
+	fullVolumeName := verification.GetFullVolumeName(s.master, s.volumeName)
+	opts := "--replicas 1 --mount type=volume,source=" + fullVolumeName + ",target=/vol,volume-driver=vsphere busybox tail -f /dev/null"


minor: better to reuse const from https://github.com/vmware/docker-volume-vsphere/blob/master/tests/constants/dockercli/cmd.go e.g. TestContainer, vDVSPluginName (need to export vDVSPluginName)

shuklanirdesh82 · 2017-06-01T01:33:54Z

tests/e2e/swarm_test.go

+
+	containerName, err = verification.GetContainerName(host, s.serviceName+".1")
+	c.Assert(err, IsNil, Commentf("Failed to retrieve container name: %s", containerName))
+	out, err = dockercli.StopService(host, containerName)


nit: new line is required to maintain consistency.

The intention here is to keep the logic of one test step in one single block.

Please see issue #1315, worth adding a test case that verifies vsphere volumes as "external" in the compose file and verify that the volume on a particular datastore is attached to the VM/container.

First of all, if we need any test to cover new scenarios, that should be done in a separate PR. Moreover, I still don't follow why we need a e2e test for #1315. We can discuss that in the issue itself instead of in this PR.

shuklanirdesh82 · 2017-06-01T01:39:04Z

tests/utils/verification/volumeproperties.go

@@ -97,6 +100,75 @@ func GetVolumePropertiesDockerCli(volName string, hostname string) string {
 	return op
 }

+// GetContainerName returns full container name based on given short name
+func GetContainerName(hostName, shortName string) (string, error) {
+	cmd := dockercli.ListContainers + "--filter name='" + shortName + "' --format '{{.Names}}'"


I think we should create filter constants for docker volume properties .. e.g. --format '{{.Names}} .. it will be easier to make change in one shot to sync with docker framework's change.

I didn't define constant for "--format" because we are using quite some different formats: .Name, .Names, .Status, etc. As for "--filter", it needs a input parameter - defining a constant like "--filter name='" looks weird, as we need attach the matching quote ' separately.

shuklanirdesh82 · 2017-06-01T01:40:09Z

tests/utils/verification/volumeproperties.go

+
+// allContainersRunning returns true if all replicated containers are up and running; false otherwise.
+func allContainersRunning(dockerHosts []string, serviceName string, replicas int) (bool, string) {
+	//TODO: It looks like all containers will always be spawned on one node. If this assumption


It is better to avoid TODO in first revision ... do we still need this TODO?

OK, removed it for now.

shuklanirdesh82 · 2017-06-01T01:42:27Z

tests/utils/verification/volumeproperties.go

+	cmd := dockercli.ListVolumes + "--filter name='" + volumeName + "' --format '{{.Name}}'"
+	fullName, err := ssh.InvokeCommand(hostName, cmd)
+	if err != nil {
+		log.Println(err)


this is redundant .. need to be removed.

shuklanirdesh82 · 2017-06-01T03:31:33Z

tests/utils/verification/volumeproperties.go

+}
+
+// allContainersRunning returns true if all replicated containers are up and running; false otherwise.
+func allContainersRunning(dockerHosts []string, serviceName string, replicas int) (bool, string) {


allContainersRunning => areAllContainersRunning something more readable

Sure - renamed it to isContainerRunning.

shuklanirdesh82 · 2017-06-01T03:33:42Z

tests/utils/verification/volumeproperties.go

+	for _, host := range dockerHosts {
+		out, err := GetAllContainers(host, serviceName)
+		if err != nil {
+			log.Println(err)


this is redundant ... invokCommand routine takes care of printing error message.

shuklanirdesh82 · 2017-06-01T03:37:14Z

tests/utils/verification/volumeproperties.go

+
+// VerifyDockerContainer returns true and the host IP if the containers are running on one of
+// the given docker hosts with specified replicas; otherwise returns false and empty string.
+func VerifyDockerContainer(dockerHosts []string, serviceName string, replicas int) (bool, string) {


can you please help me to understand the advantage of returning host value here? IMO, there is no need but will look forward to your response.

Please refer to swarm_test line 114 - the returned host is needed for the next steps: query the container name and stop the service on this specific host. My first version of this function does not return host actually. But the test failed in CI setup because the container might be spawned in any hosts. So when verifying the running containers, I need to return the host and then run subsequent steps on this specific host.

got it. Thanks @shaominchen for the explanation.

shuklanirdesh82 · 2017-06-01T03:39:34Z

tests/utils/verification/volumeproperties.go

+	log.Printf("Checking running containers for docker service [%s] on docker hosts: %v\n", serviceName, dockerHosts)
+
+	//TODO: Need to implement generic polling logic for better reuse
+	for attempt := 0; attempt < maxAttempt; attempt++ {


I would suggest to move out this code block into another method and put in place at misc.

We should refactor this (and existing VerifyDetachedStatus) in a separate change.

shuklanirdesh82 · 2017-06-01T03:40:40Z

tests/utils/verification/volumeproperties.go

+
+// VerifyDockerService returns true if the given service is running on
+// the specified VM with specified replicas; false otherwise.
+func VerifyDockerService(hostName, serviceName string, replicas int) bool {


as this method is returning bool how about renaming to IsDockerServiceVerified?

Renamed it to IsDockerServiceRunning.

shuklanirdesh82 · 2017-06-01T05:00:54Z

tests/utils/verification/volumeproperties.go

+}
+
+// GetAllContainers returns all running containers on the give host based on the filtering criteria
+func GetAllContainers(hostName, filter string) (string, error) {


I think /volumeproperties.go is not the better home to hold such utils. I would suggest to create another file or place into existing suitable place.

Make sense. Will add a separate dockercontainer.go file.

pshahzeb

Overall great.
I have a few minor comments.

pshahzeb · 2017-06-01T06:45:03Z

tests/utils/dockercli/swarm.go

+
+import (
+	"log"
+


nit: no newline needed

Not sure why gofmt does not take care of this. Fixed.

pshahzeb · 2017-06-01T06:47:46Z

tests/utils/dockercli/swarm.go

+
+// ListService lists the tasks of a docker service
+func ListService(ip, name string) (string, error) {
+	log.Printf("Listing docker services running on VM [%s]\n", ip)


minor: can log the service name as well.

pshahzeb · 2017-06-01T06:59:26Z

tests/utils/verification/volumeproperties.go

+			log.Println(err)
+			continue
+		}
+		if out != "" {


as per go suggestions, we can avoid nesting here.

if out == "" { continue } .... for loop...

pshahzeb · 2017-06-01T07:01:12Z

tests/e2e/swarm_test.go

+	s.master = inputparams.GetSwarmManager1()
+	s.worker1 = inputparams.GetSwarmWorker1()
+	s.worker2 = inputparams.GetSwarmWorker2()
+	s.dockerHosts = []string{s.master, s.worker1, s.worker2}


if you think these vars need to be read just once for the entire test suite, you can have them in SetUpSuite instead of SetupTest.

govint

Comments around common code mostly and possibly consider a test to verify vsphere volumes defined as external in the compose file for a service.
[Sam] This should be taken care of in separate tests.

govint · 2017-06-01T10:57:10Z

tests/e2e/swarm_test.go

+	s.master = inputparams.GetSwarmManager1()
+	s.worker1 = inputparams.GetSwarmWorker1()
+	s.worker2 = inputparams.GetSwarmWorker2()
+	s.dockerHosts = []string{s.master, s.worker1, s.worker2}


These should be a function in inputparams that return a map of these values vs. each test duplicating this their setup functions.

I will add a GetSwarmNodes function in inputparams. I don't follow why we need a map here.

govint · 2017-06-01T15:21:26Z

tests/utils/dockercli/swarm.go

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


Pls. make the changes for test infa/framework as a separate PR.

As we discussed offline, we should keep each PR small enough so that it’s easier to review and also keep the dependency to the minimum. The current PR is probably a special case as there's only one test case, but it's still pretty big and complicated. Since the review is almost done, I'd prefer to keep the current PR as is instead of wasting time to split it into two.

govint · 2017-06-01T18:55:10Z

tests/e2e/swarm_test.go

+
+	containerName, err = verification.GetContainerName(host, s.serviceName+".1")
+	c.Assert(err, IsNil, Commentf("Failed to retrieve container name: %s", containerName))
+	out, err = dockercli.StopService(host, containerName)


Please see issue #1315, worth adding a test case that verifies vsphere volumes as "external" in the compose file and verify that the volume on a particular datastore is attached to the VM/container.

shuklanirdesh82

LGTM! Requesting two minor comments to address.

shuklanirdesh82 · 2017-06-01T21:10:16Z

tests/utils/verification/volumeproperties.go

@@ -29,10 +29,6 @@ import (
 	"github.com/vmware/docker-volume-vsphere/tests/utils/ssh"
 )

-const (


It is better to have constant to modify the attempt count. I would suggest to add this back; it will be easier to adjust in the future instead of skimming line of code.

I removed this because there's a conflict on this constant name after splitting this file into two. Also we need to refactor this part anyway (I've already added TODO on both functions). We should have a generic poller function, which accepts a maximum time limit instead of the maximum attempts as the input.

shuklanirdesh82 · 2017-06-01T21:11:53Z

tests/utils/verification/dockercontainer.go

+)
+
+// GetContainerName returns full container name based on given short name
+func GetContainerName(hostName, shortName string) (string, error) {


GetContainerName & GetAllContainers should move out from verifier package ... both of them spoils the context of verifier .. /util/dockercli would be a better home.

Good idea! Fixed.

shuklanirdesh82

LGTM! You may want to rebase with master to resolve merge conflict and squash unrelated commits.

shuklanirdesh82 · 2017-06-01T22:20:29Z

@shaominchen If this PR is targeted for #1249 ... it is better to add fixes #1249 to close the issue automatically with merge.

lipingxue · 2017-06-01T22:03:51Z

tests/e2e/swarm_test.go

+// 1. Create a docker service with replicas setting to 1
+// 2. Verify the service is up and running with one node
+// 3. Verify one container is spawned
+// 5. Verify the volume is in attached status


The --replica option in swarm is not used to scale a stateful application. Instead, it should used to scale a stateless application. So why do we try to add the test with --replica 2 here? Any specific reason? @ashahi1

I think we should just verify the fail-over case here.

@lipingxue Can we add this comment in the issue #601 itself? This PR is almost ready to merge. We can discuss your comment in the issue and once we have an agreement we can come back to update this test in a separate PR.

OK. I reopened issue #601 to add my comments.

lipingxue · 2017-06-01T22:30:48Z

tests/utils/verification/dockercontainer.go

+}
+
+// isContainerRunning returns true if all replicated containers are up and running; false otherwise.
+func isContainerRunning(dockerHosts []string, serviceName string, replicas int) (bool, string) {


Nit：function name change to "IsContainerRunning", which is consistent with other func names

This is a convention in Go language: unexported identifiers should start with small letter; exported identifiers should start with capital letter.

pshahzeb

LGTM

shuklanirdesh82 · 2017-06-01T23:47:24Z

@shaominchen Please revisit the commit history and clean that up correctly.

shuklanirdesh82

Please rebase correctly

1. Using full volume name to create docker service 2. Add $WORKER2 to the build script

…spawned on different nodes in Docker Swarm cluster

shuklanirdesh82 · 2017-06-02T00:41:08Z

vmdk_plugin/Makefile

@@ -287,7 +287,7 @@ CLEANESX_SH   := $(DEPLOY_TOOLS) cleanesx
 deploy-esx: clean-esx
 	$(DEPLOY_ESX_SH) "$(ESX)" "$(VIB_BIN)"

-VMS= $(VM1) $(VM2) $(WORKER2)
+VMS= $(VM1) $(VM2)


this is required, isn't it? (I guess it's a victim of bad merge)

Yes, will fix.

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

shaominchen requested review from shuklanirdesh82, ashahi1 and lipingxue May 30, 2017 20:29

shuklanirdesh82 reviewed Jun 1, 2017

View reviewed changes

pshahzeb reviewed Jun 1, 2017

View reviewed changes

govint reviewed Jun 1, 2017

View reviewed changes

shuklanirdesh82 suggested changes Jun 1, 2017

View reviewed changes

shuklanirdesh82 approved these changes Jun 1, 2017

View reviewed changes

lipingxue suggested changes Jun 1, 2017

View reviewed changes

pshahzeb approved these changes Jun 1, 2017

View reviewed changes

lipingxue approved these changes Jun 1, 2017

View reviewed changes

shuklanirdesh82 suggested changes Jun 2, 2017

View reviewed changes

shaominchen force-pushed the swarm_test branch from e9ed722 to 50d4bbb Compare June 2, 2017 00:06

Shaomin Chen added 6 commits June 1, 2017 17:09

Add swarm_test to e2e test suite

acbecb1

Fixed 2 issues:

0419526

1. Using full volume name to create docker service 2. Add $WORKER2 to the build script

Improve some logs for better debugging

1c1b582

Improved test logic to handle the case where the containers might be …

17832c9

…spawned on different nodes in Docker Swarm cluster

Increase e2e timeout to 30 minutes

dc50360

Addressed review comments

3b08de0

Addressed all review comments.

e7484d0

shaominchen force-pushed the swarm_test branch from 50d4bbb to e7484d0 Compare June 2, 2017 00:10

shuklanirdesh82 reviewed Jun 2, 2017

View reviewed changes

Trivial fix for bad merge

581743b

shuklanirdesh82 approved these changes Jun 2, 2017

View reviewed changes

shuklanirdesh82 merged commit 43f26c0 into master Jun 2, 2017

shuklanirdesh82 deleted the swarm_test branch June 2, 2017 03:47

shaominchen mentioned this pull request Jun 2, 2017

Automate docker swarm P0 testcases #1249

Closed

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

Add swarm_test to e2e test suite #1309

Add swarm_test to e2e test suite #1309

Conversation

shaominchen commented May 30, 2017 • edited Loading

shuklanirdesh82 commented Jun 1, 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

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

shuklanirdesh82 Jun 1, 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

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

govint left a comment • edited by shaominchen 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

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 • edited Loading

Choose a reason for hiding this comment

shuklanirdesh82 commented Jun 1, 2017

Choose a reason for hiding this comment

shaominchen Jun 1, 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

pshahzeb left a comment

Choose a reason for hiding this comment

shuklanirdesh82 commented Jun 1, 2017

shuklanirdesh82 left a comment

Choose a reason for hiding this comment

shuklanirdesh82 Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaominchen commented May 30, 2017 •

edited

Loading

shuklanirdesh82 Jun 1, 2017 •

edited

Loading

shuklanirdesh82 Jun 1, 2017 •

edited

Loading

govint left a comment •

edited by shaominchen

Loading

shuklanirdesh82 left a comment •

edited

Loading

shaominchen Jun 1, 2017 •

edited

Loading

shuklanirdesh82 Jun 2, 2017 •

edited

Loading