Addressed latest review comments from Govindan, Nirdesh and Sam

tests/constants/dockercli/cmd.go

@@ -82,7 +82,6 @@ const (
 	// QueryContainer checks whether container exists or not
 	QueryContainer = docker + "ps -aq --filter name="
 
-<<<<<<< HEAD
 	// ListContainers list all running docker containers
 	ListContainers = docker + "ps "
 
@@ -101,9 +100,9 @@ const (
 	// RemoveService remove docker services
 	RemoveService = dockerService + "rm "
 
-	// StopAllContainers stopping all the containers command
+	// StopAllContainers stopping all the containers
 	StopAllContainers = docker + "kill $(docker ps -aq)"
 
-	// RemoveAllContainers removing all the containers forcefully command
+	// RemoveAllContainers removing all the containers forcefully
 	RemoveAllContainers = docker + "rm $(docker ps -aq) -f"
 )

tests/e2e/volumeproperty_test.go

@@ -12,8 +12,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-// This is an end-to-end test. Test will ssh to a vm and create a volume using docker cli.
-// After creating a volume, test will ssh to the ESX and verify the properties of the volume.
+// This is an end-to-end test. Test creates volumes of different format-types
+// and verifies their properties from ESX as well as docker host.
 // Properties being verified - capacity, disk-format and vm-attached field.
 
 // Test assumes that SSH cert has been setup to enable password-less login to VM and ESX.
@@ -37,37 +37,22 @@ import (
 const (
 	testName           = "volumeproperty"
 	diskStatusDetached = "detached"
-	//	diskStatusEmpty    = "<novalue>"
-	diskFormatOption = " -o diskformat="
-	size             = "100MB"
+	size               = "100MB"
+	diskFormatOption   = " -o diskformat="
 )
 
-/*
-expectedValues := map[string]string{
-  "Size":  "100MB",
-  "DiskFormat": "",
-  "DiskAttachStatus": "",
-}
-*/
-
-/*
-type ExpectedValues struct {
-	size, diskFormat, diskAttachStatus string
-}
-*/
-
-type VolumePropVerificationSuite struct {
+type VolumePropertyTestSuite struct {
 	volumeNames           [3]string
 	dockerHostIP          string
 	dockerHostName        string
 	containerName         string
 	esxIP                 string
 	formatTypes           []string
 	dockerVolumeRemoveCmd string
-	vmAttached            bool
+	volumeAttached        bool
 }
 
-func (s *VolumePropVerificationSuite) SetUpTest(c *C) {
+func (s *VolumePropertyTestSuite) SetUpTest(c *C) {
 	s.dockerHostIP = inputparams.GetDockerHostIP("VM2")
 	s.dockerHostName = govc.RetrieveVMNameFromIP(s.dockerHostIP)
 	s.esxIP = inputparams.GetEsxIP()
@@ -78,23 +63,24 @@ func (s *VolumePropVerificationSuite) SetUpTest(c *C) {
 }
 
 // verifyParams - skips the test if dockerHostIP/dockerHostName/esxIP are empty
-func (s *VolumePropVerificationSuite) verifyParams(c *C) {
+func (s *VolumePropertyTestSuite) verifyParams(c *C) {
 	if s.dockerHostIP == "" || s.dockerHostName == "" || s.esxIP == "" {
 		c.Skip("Empty values - hence skipping the test")
 	}
 }
 
 // TearDownTest - Removes all the containers and deletes the volumes
-func (s *VolumePropVerificationSuite) TearDownTest(c *C) {
+func (s *VolumePropertyTestSuite) TearDownTest(c *C) {
 	out, err := dockercli.RemoveAllContainers(s.dockerHostIP)
 	c.Assert(err, IsNil, Commentf(out))
 	out, err = ssh.InvokeCommand(s.dockerHostIP, s.dockerVolumeRemoveCmd)
 	c.Assert(err, IsNil, Commentf(out))
 }
 
-var _ = Suite(&VolumePropVerificationSuite{})
+var _ = Suite(&VolumePropertyTestSuite{})
 var dockerCliMap = make(map[string][3]string)
 var adminCliMap = make(map[string][3]string)
+var expectedValues = make(map[string][3]string)
 
 /*
 Test steps:
@@ -111,8 +97,8 @@ Test steps:
 NOTE: Do steps 5 ,6 and 7 only for volume of 'thin' disk format
 */
 
-func (s *VolumePropVerificationSuite) TestVolumeProperties(c *C) {
-	log.Printf("START: TestVolumeProperties")
+func (s *VolumePropertyTestSuite) TestVolumeProperties(c *C) {
+	log.Printf("START: volumeproperty_test.TestVolumeProperties")
 
 	// creating volumes of all disk formats
 	s.createVolumes(c)
@@ -135,6 +121,7 @@ func (s *VolumePropVerificationSuite) TestVolumeProperties(c *C) {
 	// attaching only thin volume
 	out, err := dockercli.AttachVolume(s.dockerHostIP, s.volumeNames[0], s.containerName)
 	c.Assert(err, IsNil, Commentf("Failed to attach the volume [%s]", out))
+	s.volumeAttached = true
 
 	// As "thin" volume is now attached, again getting properties using admin cli and docker cli
 	s.getVolumePropertiesAdminCli(c)
@@ -144,22 +131,16 @@ func (s *VolumePropVerificationSuite) TestVolumeProperties(c *C) {
 	status = reflect.DeepEqual(dockerCliMap, adminCliMap)
 	c.Assert(status, Equals, true, Commentf("Admin cli and docker cli properties of volumes are not same"))
 
-	// verifying properties of only thin-volume
-	// s.verifyProperties(c, s.volumeNames[0])
+	// Again verifying if admin cli properties of a volume are same as the expected values
+	s.verifyProperties(c)
 
-	log.Printf("END: TestVolumeProperties")
+	log.Printf("END: volumeproperty_test.TestVolumeProperties")
 }
 
 // createVolumes - creates volumes of each format type
-func (s *VolumePropVerificationSuite) createVolumes(c *C) {
+func (s *VolumePropertyTestSuite) createVolumes(c *C) {
 	for i, formatType := range s.formatTypes {
 		s.volumeNames[i] = inputparams.GetVolumeNameWithTimeStamp(formatType)
-
-		/*
-			out, err := ssh.InvokeCommand(s.dockerHostIP, dockerconst.CreateVolume+"--name="+s.volumeNames[i]+" -o diskformat="+formatType)
-			c.Assert(err, IsNil, Commentf(out))
-		*/
-
 		out, err := dockercli.CreateVolumeWithOptions(s.dockerHostIP, s.volumeNames[i], diskFormatOption+formatType)
 		c.Assert(err, IsNil, Commentf(out))
 
@@ -170,58 +151,32 @@ func (s *VolumePropVerificationSuite) createVolumes(c *C) {
 }
 
 // getPropertiesAndVerify - gets properties of a volume from esx
-func (s *VolumePropVerificationSuite) getVolumePropertiesAdminCli(c *C) {
+func (s *VolumePropertyTestSuite) getVolumePropertiesAdminCli(c *C) {
 	adminCliMap = verification.GetVolumePropertiesAdminCli(s.esxIP)
 	c.Assert(len(adminCliMap), Not(Equals), 0, Commentf("Map of volume names and their corresponding properties is EMPTY."))
 }
 
 // getVolumePropertiesDockerCli - gets properties of a volume from the docker host
-func (s *VolumePropVerificationSuite) getVolumePropertiesDockerCli(c *C, volumeName string) {
+func (s *VolumePropertyTestSuite) getVolumePropertiesDockerCli(c *C, volumeName string) {
 	out, err := verification.GetVolumePropertiesDockerCli(volumeName, s.dockerHostIP)
 	c.Assert(err, IsNil, Commentf(out))
 
 	dockerCliValues := strings.Fields(out)
 	dockerCliMap[volumeName] = [3]string{dockerCliValues[0], dockerCliValues[1], dockerCliValues[2]}
 }
 
-// verifyProperties - verifies admin cli values for a volume are same as expected values
-func (s *VolumePropVerificationSuite) verifyProperties(c *C) {
-	var expectedValues = make(map[string][3]string)
+// verifyProperties - verifies admin cli values for a volume are same as the expected values
+// Only for 'thin', after volume is attached, modifying attached-to-vm field with vm name.
+func (s *VolumePropertyTestSuite) verifyProperties(c *C) {
 	for i, volumeName := range s.volumeNames {
-		expectedValues[volumeName] = [3]string{size, s.formatTypes[i], diskStatusDetached}
-	}
-	log.Printf("expectedValues: \n [%s]", expectedValues)
-
-	status := reflect.DeepEqual(expectedValues, adminCliMap)
-	c.Assert(status, Equals, true, Commentf("Admin cli properties of volumes are not same as the expected values."))
-
-}
-
-/*
-// verifyProperties - verifies admin cli and docker cli values for size, disk format and attached-to-vm field.
-func (s *VolumePropVerificationSuite) verifyProperties(c *C, volumeName, valuesAdmincli, valuesDockerCli string) {
-	log.Printf("Verifying volume properties like size, disk-format and attached-to-vm fields "+
-		"at vm [%s] and esx [%s] for volume - [%s] ", s.dockerHostIP, s.esxIP, volumeName)
-	arrAdminCliValues := strings.Fields(valuesAdmincli)
-	sliceAdminCliValues := arrAdminCliValues[1:len(arrAdminCliValues)]
-	// status := verification.VerifyAttachedStatus(volumeName, s.dockerHostIP, s.esxIP)
-
-	sliceDockerCliValues := s.arrangeDockerCliValues(valuesDockerCli)
-	for i := range sliceDockerCliValues {
-		c.Assert(len(sliceDockerCliValues), Equals, len(sliceAdminCliValues), Commentf("Values mis-match between admin cli and docker cli for volume %s", volumeName))
-		if i == 2 && !s.vmAttached {
-			c.Assert(sliceDockerCliValues[i], Equals, diskStatusEmpty, Commentf("Value from dockercli is [%s]", sliceDockerCliValues[i]))
-			c.Assert(sliceAdminCliValues[i], Equals, diskStatusDetached, Commentf("Value from dockercli is [%s]", i, sliceAdminCliValues[i]))
+		if s.volumeAttached {
+			tmp := expectedValues[s.volumeNames[0]]
+			tmp[2] = s.dockerHostName
+			expectedValues[s.volumeNames[0]] = tmp
 		} else {
-			c.Assert(sliceDockerCliValues[i], Equals, sliceAdminCliValues[i], Commentf("Value from admincli is [%s] and that from dockercli is [%s]", sliceAdminCliValues[i], sliceDockerCliValues[i]))
+			expectedValues[volumeName] = [3]string{size, s.formatTypes[i], diskStatusDetached}
 		}
 	}
-	// Get vm name based on ip and compare ith with the docker cli/admin cli only when vm is attached
-	if s.vmAttached {
-		c.Assert(sliceDockerCliValues[len(sliceDockerCliValues)-1], Equals, s.dockerHostName, Commentf("VM name fetched from attached-to-VM field "+
-			"for volume %s is not same as that fetched from its IP.", volumeName))
-	}
-
+	status := reflect.DeepEqual(expectedValues, adminCliMap)
+	c.Assert(status, Equals, true, Commentf("Admin cli properties of volumes are not same as the expected values."))
 }
-
-*/

tests/utils/verification/volumeproperties.go

@@ -55,10 +55,10 @@ func GetVMAttachedToVolUsingAdminCli(volName string, hostname string) string {
 	return volProps[1]
 }
 
-// GetVolumePropertiesAdminCli returns capacity, attached-to-vm and disk-format field
-// for volume using Admin cli
+// GetVolumePropertiesAdminCli returns a map of volumes and their corresponding
+// properties - capacity, attached-to-vm and disk-format field using Admin cli
 func GetVolumePropertiesAdminCli(hostName string) map[string][3]string {
-	log.Printf("Getting size, disk-format and attached-to-vm for all volumes from ESX [%s]\n", hostName)
+	log.Printf("Getting size, disk-format and attached-to-vm for all volumes from ESX [%s] using admin cli. \n", hostName)
 	cmd := admincli.ListVolumes + "-c volume,capacity,disk-format,attached-to 2>/dev/null | awk -v OFS='\t' '{print $1, $2, $3, $4}' | sed '1,2d'  "
 	out, _ := ssh.InvokeCommand(hostName, cmd)
 	admincliValues := strings.Fields(out)
@@ -73,7 +73,7 @@ func GetVolumePropertiesAdminCli(hostName string) map[string][3]string {
 // GetVolumePropertiesDockerCli returns capacity,  attached-to-vm and disk-format field
 // for volume using Docker cli
 func GetVolumePropertiesDockerCli(volumeName string, hostName string) (string, error) {
-	log.Printf("Getting size, disk-format and attached-to-vm for volume [%s] from vm [%s]\n", volumeName, hostName)
+	log.Printf("Getting size, disk-format and attached-to-vm for volume [%s] from vm [%s] using docker cli \n", volumeName, hostName)
 	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)
 }