-
Notifications
You must be signed in to change notification settings - Fork 95
Optimizing volumepropverification_test #1310
Optimizing volumepropverification_test #1310
Conversation
tests/constants/dockercli/cmd.go
Outdated
@@ -76,4 +76,13 @@ const ( | |||
|
|||
// QueryContainer checks whether container exists or not | |||
QueryContainer = docker + "ps -aq --filter name=" | |||
|
|||
// StopAllContainers refers to stopping all the containers | |||
StopAllContainers = docker + "kill $(docker ps -q)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be "docker ps -aq"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
tests/constants/dockercli/cmd.go
Outdated
RemoveAllContainers = docker + "rm $(docker ps -a -q)" | ||
|
||
// DeleteAllDockerVolumes refers to removing all the docker volumes | ||
DeleteAllDockerVolumes = ListVolumes + "-qf dangling=true | xargs -r docker volume rm" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this.
func (s *VolumePropVerificationSuite) SetUpTest(c *C) { | ||
s.dockerHostIP = inputparams.GetDockerHostIP("VM2") | ||
s.dockerHostName = govc.RetrieveVMNameFromIP(s.dockerHostIP) | ||
s.esxIP = os.Getenv("ESX") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pls. use inputparams.GetEsxIP()
s.dockerHostName = govc.RetrieveVMNameFromIP(s.dockerHostIP) | ||
s.esxIP = os.Getenv("ESX") | ||
s.volumeName = inputparams.GetVolumeNameWithTimeStamp("volumepropverification_test") | ||
s.containerName = inputparams.GetContainerNameWithTimeStamp("volumepropverificationSuite_test") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two volume name substrings can be as constants at the top of this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can check if any of the needed params is not valid and call c.Skip() so that the test is skipped vs. running and spewing errors or Assert()s.
log.Printf("Actual values : %s. Does not contains %s ", op, volmProps[i]) | ||
func hasElement(volumeProps []string, op string) bool { | ||
for i := 0; i < len(volumeProps); i++ { | ||
if !strings.Contains(op, volumeProps[i]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use range to iterate over the array?
func (s *VolumePropVerificationSuite) TestVolumeProperties(c *C) { | ||
log.Printf("START: TestVolumeProperties") | ||
formatTypes := []string{"thin", "zeroedthick", "eagerzeroedthick"} | ||
for i := 0; i < len(formatTypes); i++ { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pls. use "range" to iterate over the array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
|
||
if formatTypes[i] == "thin" { | ||
out, err = dockercli.AttachVolume(s.dockerHostIP, s.volumeName, s.containerName) | ||
c.Assert(err, IsNil, Commentf("Failed to attach the volume [%s]", s.volumeName)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please print value of "out" so test output shows the cause of failure.
c.Assert(err, IsNil, Commentf(out)) | ||
|
||
volumePropsAdminCli := verification.GetVolumePropertiesAdminCli(s.volumeName, s.esxIP) | ||
expectedPropsAdminCli := []string{"100MB", formatTypes[i], "detached"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The verification functions themselves seem to be doing comparison and verification. Suggest defining new functions or update existing functions to do the verification needed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or if the expected and actual values are of a specific format (like strings) then create a function in this file and call it for each of the verifications here vs. adding code for each type of verification below.
c.Assert(err, IsNil, Commentf(out)) | ||
|
||
volumePropsAdminCli := verification.GetVolumePropertiesAdminCli(s.volumeName, s.esxIP) | ||
expectedPropsAdminCli := []string{"100MB", formatTypes[i], "detached"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make the expected values as constants (top of the file) with appropriate names so its easy to read and understand the code.
c.Assert(hasElement(expectedPropsAdminCli, volumePropsAdminCli), Equals, true, Commentf("Volume %s properties "+ | ||
"on ESX fetched using admin cli does not matches with the expected values", s.volumeName)) | ||
|
||
volumePropsDockerCli := verification.GetVolumePropertiesDockerCli(s.volumeName, s.dockerHostIP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is buggy already - please fix it in this PR - its logging a fatal log if inspect fails. We've already decided that fatal logs are disallowed. Instead return a string and error type from this function.
"on ESX fetched using admin cli does not matches with the expected values", s.volumeName)) | ||
|
||
volumePropsDockerCli := verification.GetVolumePropertiesDockerCli(s.volumeName, s.dockerHostIP) | ||
expectedPropsDockerCli := []string{"100MB", formatTypes[i], "<no value>"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pls. use a constant with a meaningful name instead.
vmNameFromAdminCli := verification.GetVMAttachedToVolUsingAdminCli(s.volumeName, s.esxIP) | ||
vmNameFromDockerCli := verification.GetVMAttachedToVolUsingDockerCli(s.volumeName, s.dockerHostIP) | ||
|
||
// Get vm name based on ip and compare ith with the docker cli and admin cli |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment - compare ith with
c.Assert(vmNameFromAdminCli, Equals, vmNameFromDockerCli, Commentf("Information mis-match - "+ | ||
"Attached-to-VM field for volume from docker cli is [%s] and attched-to-vm field from admin cli is [%s]", vmNameFromDockerCli, vmNameFromAdminCli)) | ||
|
||
// Again verifyying volume properties after starting a container |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment - verifyying
|
||
// Again verifyying volume properties after starting a container | ||
volumePropsDockerCli = verification.GetVolumePropertiesDockerCli(s.volumeName, s.dockerHostIP) | ||
expectedProprtyDockerCli := []string{"100MB", formatTypes[i]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use appropriately named constant instead.
"for volume %s is not same as that fetched from its IP.", s.volumeName)) | ||
|
||
c.Assert(vmNameFromAdminCli, Equals, vmNameFromDockerCli, Commentf("Information mis-match - "+ | ||
"Attached-to-VM field for volume from docker cli is [%s] and attched-to-vm field from admin cli is [%s]", vmNameFromDockerCli, vmNameFromAdminCli)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why shouldn't these two checks on the VM name be done for all disk format types - why is it done only for "thin" disks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test was taking long time to finish and intermittently test was failing on CI because test ran too long. So it was decided to perform certain steps only for "thin" disk format to reduce the overall test duration. Basically wanted to finish CI runs faster.
Please refer to Issue #1096
c.Assert(hasElement(expectedPropsDockerCli, volumePropsDockerCli), Equals, true, Commentf("Volume %s properties "+ | ||
"on docker vm fetched using docker cli does not matches with the expected values", s.volumeName)) | ||
|
||
if formatTypes[i] == "thin" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Move this handling for thin disks into a separate function.
-
Add a new function that does "mandatory checks on volume properties"
-
Get the admin CLI and docker CLI volume properties and pass to function in (2) to make the checks.
-
If volume is thin then call another function to verify volume properties specific for thin disks.
Make this function smaller by breaking it up into separate functions
- One function to create the volumes,
- Then loop over the volumes and get the admin and docker CLI properties for each volume.
- Call the property validation function for each volume.
"on docker vm fetched using docker cli does not matches with the expected values", s.volumeName)) | ||
|
||
volumePropsAdminCli = verification.GetVolumePropertiesAdminCli(s.volumeName, s.esxIP) | ||
expectedPropsAdminCli = []string{"100MB", formatTypes[i]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we keep getting docker CLI and admin CLI volume properties each time. Get it once for each volume and do the property validations with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set of changes to layout, simpler functions, more properties to validate.
|
||
con "github.com/vmware/docker-volume-vsphere/tests/constants/dockercli" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: con?? ... how about dockerconst
ssh.InvokeCommand(vmIP, dockerVolmRmvCmd) | ||
log.Println("-----Clean-up finished - current time: ", time.Now()) | ||
func (s *VolumePropVerificationSuite) TearDownTest(c *C) { | ||
out, err := dockercli.StopAllContainers(s.dockerHostIP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is better to avoid this step and invoke forceful removal of containers .. unless it is required to have this extra step.
tests/constants/dockercli/cmd.go
Outdated
StopAllContainers = docker + "kill $(docker ps -q)" | ||
|
||
// RemoveAllContainers refers to removing all the containers | ||
RemoveAllContainers = docker + "rm $(docker ps -a -q)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docker ps -a -q
=> docker ps -aq
??
} | ||
|
||
// DeleteAllDockerVolumes - removes all the containers on a particular vm | ||
func DeleteAllDockerVolumes(ip string) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks cool but dangerous .. it may delete unwanted volumes as well. one serious problem I can think of running test in parallel.
Ideal approach should be deleting volumes those are created as part of current running test instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. This is useful only if we are sure we are doing the right thing (i.e. no side effects).
tests/constants/dockercli/cmd.go
Outdated
@@ -76,4 +76,13 @@ const ( | |||
|
|||
// QueryContainer checks whether container exists or not | |||
QueryContainer = docker + "ps -aq --filter name=" | |||
|
|||
// StopAllContainers refers to stopping all the containers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Just for consistency, let's remove "refers to" to keep the comments simple.
return ssh.InvokeCommand(ip, dockercli.RemoveAllContainers) | ||
} | ||
|
||
// DeleteAllDockerVolumes - removes all the containers on a particular vm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comments need to be updated.
} | ||
|
||
// DeleteAllDockerVolumes - removes all the containers on a particular vm | ||
func DeleteAllDockerVolumes(ip string) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. This is useful only if we are sure we are doing the right thing (i.e. no side effects).
s.dockerHostIP = inputparams.GetDockerHostIP("VM2") | ||
s.dockerHostName = govc.RetrieveVMNameFromIP(s.dockerHostIP) | ||
s.esxIP = os.Getenv("ESX") | ||
s.volumeName = inputparams.GetVolumeNameWithTimeStamp("volumepropverification_test") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name is just too long - can we rename it (and the file name) to volumeproperty_test?
func (s *VolumePropVerificationSuite) TestVolumeProperties(c *C) { | ||
log.Printf("START: TestVolumeProperties") | ||
formatTypes := []string{"thin", "zeroedthick", "eagerzeroedthick"} | ||
for i := 0; i < len(formatTypes); i++ { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
} | ||
|
||
func (s *VolumePropVerificationSuite) TestVolumeProperties(c *C) { | ||
log.Printf("START: TestVolumeProperties") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Just for consistency, let's include the test group name in the log as well.
tests/e2e/volumeproperty_test.go
Outdated
) | ||
|
||
const ( | ||
TestName = "volumeproperty" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all this should start with lower case unless it is meant for exporting.
"volume name, attached-to-vm status, size and disk-format. Actual output: %s", op) | ||
out, err := ssh.InvokeCommand(hostname, cmd) | ||
if err != nil { | ||
log.Printf("Null value is returned by admin cli when looking for, size, disk-format and attached-to-vm for volume [%s].", volName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not needed as InvokeCommand
takes care of this one.
cmd := dockercli.InspectVolume + " --format ' {{index .Name}} {{index .Status.capacity.size}} {{index .Status.diskformat}} {{index .Status \"attached to VM\"}}' " + volName | ||
out, err := ssh.InvokeCommand(hostname, cmd) | ||
if err != nil { | ||
log.Printf("Null value is returned by docker cli when looking for, size, disk-format and attached-to-vm for volume [%s].", volName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
tests/e2e/volumeproperty_test.go
Outdated
for i, formatType := range s.formatTypes { | ||
s.containerName = inputparams.GetContainerNameWithTimeStamp(TestName) | ||
s.volumeNames[i] = inputparams.GetVolumeNameWithTimeStamp(TestName) | ||
out, err := ssh.InvokeCommand(s.dockerHostIP, dockerconst.CreateVolume+"--name="+s.volumeNames[i]+" -o diskformat="+formatType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I knew that it was in review but wasn't sure if it got checked-in. Will remove it.
tests/e2e/volumeproperty_test.go
Outdated
// createVolumes - creates volumes of each format type | ||
func (s *VolumePropVerificationSuite) createVolumes(c *C) { | ||
for i, formatType := range s.formatTypes { | ||
s.containerName = inputparams.GetContainerNameWithTimeStamp(TestName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't it keep setting and overriding value and set with the last loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed it. Will remove it.
tests/e2e/volumeproperty_test.go
Outdated
|
||
// getPropertiesAndVerify - gets properties of a volume from esx and docker host | ||
func (s *VolumePropVerificationSuite) getPropertiesAndVerify(c *C, volumeName string) { | ||
valuesAdminCli, err := verification.GetVolumePropertiesAdminCli(volumeName, s.esxIP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to invoke admin_cli volume ls once > save the response and perform validation this reduces unnecessary ssh calls for each volumes following.
tests/e2e/volumeproperty_test.go
Outdated
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is so special about i==2
? you may want to put some inline comment to make reviewer's life easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated comments on the approach to verification. Please see comments on the GetVolumeProperties*() functions.
tests/e2e/volumeproperty_test.go
Outdated
for _, volumeName := range s.volumeNames { | ||
// getting and verifying the admin cli and docker cli values | ||
s.getPropertiesAndVerify(c, volumeName) | ||
s.dockerVolumeRemoveCmd = s.dockerVolumeRemoveCmd + " " + volumeName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a global and why is it getting initialized here? if getPropertiesAndVerify() wants to use this global let it use it. Seems odd why this global is set here and not used.
tests/e2e/volumeproperty_test.go
Outdated
func (s *VolumePropVerificationSuite) createVolumes(c *C) { | ||
for i, formatType := range s.formatTypes { | ||
s.containerName = inputparams.GetContainerNameWithTimeStamp(TestName) | ||
s.volumeNames[i] = inputparams.GetVolumeNameWithTimeStamp(TestName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Watchout, the GetVolumeNameWithTimeStamp() is using time.New().Unix() which returns the number of seconds since some time. Successive calls can return the same timestamp. I'd seen this and use names with a random generated number attached to it. For clarity sake you could have different names - thick, zeroedthick, thin and attach the timestamp that will make it unique anyway with different names - thick-timestamp, zeroedthick-timestamp, thin-timestamp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've filed issue #1344 for this.
tests/e2e/volumeproperty_test.go
Outdated
for i, formatType := range s.formatTypes { | ||
s.containerName = inputparams.GetContainerNameWithTimeStamp(TestName) | ||
s.volumeNames[i] = inputparams.GetVolumeNameWithTimeStamp(TestName) | ||
out, err := ssh.InvokeCommand(s.dockerHostIP, dockerconst.CreateVolume+"--name="+s.volumeNames[i]+" -o diskformat="+formatType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please use dockercli.CreateVolume() API.
tests/e2e/volumeproperty_test.go
Outdated
valuesAdminCli, err := verification.GetVolumePropertiesAdminCli(volumeName, s.esxIP) | ||
c.Assert(err, IsNil, Commentf(valuesAdminCli)) | ||
|
||
valuesDockerCli, err := verification.GetVolumePropertiesDockerCli(volumeName, s.dockerHostIP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only a suggestion and you can decide how difficult or easy it may be to do this. The verifyProperties() function is may be a bit complex. Is it possible instead for the GetVolumeProperties*() functions to return a map of values so that the verification is easy to do. That would be ideal as it makes verifyProperties() pretty easy.
tests/e2e/volumeproperty_test.go
Outdated
c.Assert(sliceDockerCliValues[i], Equals, sliceAdminCliValues[i], Commentf("Value from admincli is [%s] and that from dockercli is [%s]", sliceAdminCliValues[i], sliceDockerCliValues[i])) | ||
} | ||
} | ||
// Get vm name based on ip and compare ith with the docker cli/admin cli only when vm is attached |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit comment - "ith with"
tests/e2e/volumeproperty_test.go
Outdated
// 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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code can be very delicate and can easily fail with these assumed locations of volume properties. Its also the wrong way to compare attributes of a volume based on position of values at specific indices. Lets make the GetVolumeProperties*() functions to return maps vs strings. May be more work but its worth the effort to make the test code here accurate without complexity.
tests/e2e/volumeproperty_test.go
Outdated
sliceDockerCliValues := arrDockerCliValues[1:len(arrDockerCliValues)] | ||
if !s.vmAttached { | ||
sliceDockerCliValues[len(sliceDockerCliValues)-2] = sliceDockerCliValues[len(sliceDockerCliValues)-2] + sliceDockerCliValues[len(sliceDockerCliValues)-1] | ||
sliceDockerCliValues = sliceDockerCliValues[0 : len(sliceDockerCliValues)-1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:-))))), but no Sir, this isn't right at all. Please make those util function return maps and we all can use those to benefit of more tests.
@@ -29,7 +29,7 @@ const ( | |||
// CreateVolume create a volume with vsphere driver | |||
CreateVolume = dockerVol + " create --driver=vsphere " | |||
|
|||
// RemoveVolume constant refers delete volume command | |||
// RemoveVolume delete volume command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: just for consistency, let's remove " command" from the comments. Same for below constants.
tests/e2e/volumeproperty_test.go
Outdated
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's update this comment to briefly explain the main purpose of this test group, instead of explaining detailed test steps here.
tests/e2e/volumeproperty_test.go
Outdated
DiskStatusEmpty = "<novalue>" | ||
) | ||
|
||
type VolumePropVerificationSuite struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename this to VolumePropertyTestSuite.
tests/e2e/volumeproperty_test.go
Outdated
containerName string | ||
esxIP string | ||
formatTypes []string | ||
dockerVolumeRemoveCmd string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This field doesn't make much sense. Why don't use dockerconst.RemoveVolume directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am creating a single command to remove all the volumes so that we don't need to ssh for each volume.
This is how final command looks like: docker volume rm vol1 vol2 vol3
tests/e2e/volumeproperty_test.go
Outdated
*/ | ||
|
||
func (s *VolumePropVerificationSuite) TestVolumeProperties(c *C) { | ||
log.Printf("START: TestVolumeProperties") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: can we add test group name as well?
log.Printf("START: volumeproperty_test.TestVolumeProperties")
tests/e2e/volumeproperty_test.go
Outdated
s.vmAttached = true | ||
s.getPropertiesAndVerify(c, s.volumeNames[0]) | ||
|
||
log.Printf("END: TestVolumeProperties") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
tests/e2e/volumeproperty_test.go
Outdated
func (s *VolumePropVerificationSuite) createVolumes(c *C) { | ||
for i, formatType := range s.formatTypes { | ||
s.containerName = inputparams.GetContainerNameWithTimeStamp(TestName) | ||
s.volumeNames[i] = inputparams.GetVolumeNameWithTimeStamp(TestName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've filed issue #1344 for this.
3c1a33c
to
c3932a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments on current code changes.
tests/e2e/volumeproperty_test.go
Outdated
s.dockerHostIP = inputparams.GetDockerHostIP("VM2") | ||
s.dockerHostName = govc.RetrieveVMNameFromIP(s.dockerHostIP) | ||
s.esxIP = inputparams.GetEsxIP() | ||
s.containerName = inputparams.GetContainerNameWithTimeStamp(testName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR #1286 is adding up a inputparams/testparams.go:GetTestConfig() which gets all these in a single call inside a struct. You could use that once its merged (ready for merge now). The test code doesn't need to use multiple API (inputparams, govc) just to set up params.
tests/e2e/volumeproperty_test.go
Outdated
func (s *VolumePropertyTestSuite) verifyParams(c *C) { | ||
if s.dockerHostIP == "" || s.dockerHostName == "" || s.esxIP == "" { | ||
c.Skip("Empty values - hence skipping the test") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is simplified with GetTestConfig() from above comment (see #1286)
tests/e2e/volumeproperty_test.go
Outdated
c.Assert(err, IsNil, Commentf(out)) | ||
out, err = ssh.InvokeCommand(s.dockerHostIP, s.dockerVolumeRemoveCmd) | ||
c.Assert(err, IsNil, Commentf(out)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can add a log with timestamp at the end of Setup and Teardown - just so we know when each test starts and when it ends, what takes time etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I will add log statements at end of setup and teardown and that automatically prints current time.
e.g.
2017/06/05 17:01:15 SETUP Finished: volumeproperty_test.TestVolumeProperties
2017/06/05 17:01:40 CLEAN-UP Finished: volumeproperty_test.TestVolumeProperties
tests/e2e/volumeproperty_test.go
Outdated
|
||
// Verifying docker cli and admin cli properties of a volume are same | ||
status := reflect.DeepEqual(dockerCliMap, adminCliMap) | ||
c.Assert(status, Equals, true, Commentf("Admin cli and docker cli properties of volumes are not same")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If status is false, its good to print the maps, when an error happens we can immediately figure what the issue is, from the logs.
tests/e2e/volumeproperty_test.go
Outdated
s.getVolumePropertiesDockerCli(c, volumeName) | ||
} | ||
|
||
// Verifying docker cli and admin cli properties of a volume are same |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verify docker and ESX properties of "all three volumes" are the same. Thats what we are doing, using the CLIs on both platforms and figuring they have the same values.
tests/e2e/volumeproperty_test.go
Outdated
expectedValues[volumeName] = [3]string{size, s.formatTypes[i], diskStatusDetached} | ||
} | ||
} | ||
status := reflect.DeepEqual(expectedValues, adminCliMap) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't we comparing the docker-side volume props for three volumes with ESX-side volume props for all volumes. DeepEqual will fail here, unless its ensured that there are only three volumes in ESX. IN whcih case set up suite() should delete all volumes in the vmgroup to ensure only the three volumes will exist in the vmgroup.
tests/e2e/volumeproperty_test.go
Outdated
func (s *VolumePropertyTestSuite) verifyProperties(c *C) { | ||
for i, volumeName := range s.volumeNames { | ||
if s.volumeAttached { | ||
tmp := expectedValues[s.volumeNames[0]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the first call; to verifyProperties then expectedValues[] won't have the key (volume names) and will cause a panic of the GO run time.need to do like,
tmp, ok := expectedValues[volumeName]; ok == true {
_ assign the volume name_
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s.volumeAttached is 'true' only if volume is attached. So this piece of code is only executed if volume is attached else map of expected values is populated. I had tested it locally and it ran fine. So things work fine even for first call.
tests/e2e/volumeproperty_test.go
Outdated
// 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 { | ||
if s.volumeAttached { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is common for all volumes and hence when thin disk is attached, we will always go thru this path and mark all with the docker host name like as though they are attached, which isn't the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is not what's happening. Please see the below output (map of expected values) when thin volume is attached.
2017/06/05 20:53:26 Expected values: map[thin_volume_1496695990:[100MB thin ubuntu-VM2.3] zeroedthick_volume_1496695993:[100MB zeroedthick detached] eagerzeroedthick_volume_1496695995:[100MB eagerzeroedthick detached]]
admincliValues := strings.Fields(out) | ||
adminCliMap := make(map[string][3]string) | ||
for i := 0; i < len(admincliValues); { | ||
adminCliMap[admincliValues[i]] = [3]string{admincliValues[i+1], admincliValues[i+2], admincliValues[i+3]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[3] isn't needed []string{......} is ok
} | ||
return op | ||
return adminCliMap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a global, does it need to be returned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its global in a test file tests/e2e/volumeproperty_test.go and not under tests/utils/verification/volumeproperties.go
680ba57
to
42d6fdb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Please address merge conflicts. A few comments below.
"volume name, attached-to-vm status, size and disk-format. Actual output: %s", op) | ||
// GetPropertiesForAllVolumesAdminCli returns a map of all the volumes from ESX and their corresponding | ||
// properties returned - capacity, attached-to-vm and disk-format | ||
func GetPropertiesForAllVolumesAdminCli(hostName string) map[string][]string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I strongly suggest to add a common util function to return a map of all volume properties in which the key is the property name and the value is the property value. That will be very useful to avoid such complicated function which is only useful for a specific test.
Can we file a issue to track this improvement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed issue #1365
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to have Map(volumeName,VolPropStruct) where volume name is the key and Struct object is for various properties.
struct VolPropStruct {
capacity
attached-to-vm
disk-format
}
tests/e2e/volumeproperty_test.go
Outdated
s.containerName = inputparams.GetContainerNameWithTimeStamp(testName) | ||
s.formatTypes = []string{"thin", "zeroedthick", "eagerzeroedthick"} | ||
s.dockerVolumeRemoveCmd = dockerconst.RemoveVolume | ||
log.Println(" SETUP Finished: volumeproperty_test.TestVolumeProperties") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: remove the space before "SETUP"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I will remove it.
tests/e2e/volumeproperty_test.go
Outdated
} | ||
|
||
var _ = Suite(&VolumePropertyTestSuite{}) | ||
var dockerCliMap = make(map[string][]string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some comments for these 3 maps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I will add it.
"volume name, attached-to-vm status, size and disk-format. Actual output: %s", op) | ||
// GetPropertiesForAllVolumesAdminCli returns a map of all the volumes from ESX and their corresponding | ||
// properties returned - capacity, attached-to-vm and disk-format | ||
func GetPropertiesForAllVolumesAdminCli(hostName string) map[string][]string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method name is too big .. it should be simplified .. how about his GetAllVolProperties
and the method doc mentions how are we retrieving.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I will change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly also remove the AdminCLI and DockerCLI from the function names
tests/e2e/volumeproperty_test.go
Outdated
|
||
const ( | ||
testName = "volumeproperty" | ||
diskStatusDetached = "detached" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests/e2e/volumeproperty_test.go
Outdated
volumeNames []string | ||
containerName string | ||
formatTypes []string | ||
dockerVolumeRemoveCmd string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be removed and used the const directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? and how?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed it.
tests/e2e/volumeproperty_test.go
Outdated
s.containerName = inputparams.GetContainerNameWithTimeStamp(testName) | ||
s.formatTypes = []string{"thin", "zeroedthick", "eagerzeroedthick"} | ||
s.dockerVolumeRemoveCmd = dockerconst.RemoveVolume | ||
log.Println(" SETUP Finished: volumeproperty_test.TestVolumeProperties") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be replaced with something similar to https://github.com/vmware/docker-volume-vsphere/blob/master/tests/utils/misc/misc.go#L53 .. you may introduce new log util.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on a second thought .. can we remove this line? IMO, it will not be helpful by looking at SetUpTest
.. if agree please remove otherwise share more details.
tests/e2e/volumeproperty_test.go
Outdated
*/ | ||
|
||
func (s *VolumePropertyTestSuite) TestVolumeProperties(c *C) { | ||
log.Printf("START: volumeproperty_test.TestVolumeProperties") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be replaced with https://github.com/vmware/docker-volume-vsphere/blob/master/tests/utils/misc/misc.go#L53
tests/e2e/volumeproperty_test.go
Outdated
// Verify if docker and ESX properties of volumes are same and as expected. | ||
s.verifyProperties(c) | ||
|
||
log.Printf("END: volumeproperty_test.TestVolumeProperties") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use appropriate util from https://github.com/vmware/docker-volume-vsphere/blob/master/tests/utils/misc/misc.go#L53
tests/e2e/volumeproperty_test.go
Outdated
} | ||
|
||
// getVolumeProperties - get properties of three volumes from ESX and docker host | ||
func (s *VolumePropertyTestSuite) getVolumeProperties(c *C) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry need to improve ... as this method is not returning anything .. we should avoid using getXXX
; it spoils objective of get
(leads to confusion)
tests/e2e/volumeproperty_test.go
Outdated
} | ||
|
||
// getVolumePropertiesAdminCli - get properties of a volume from ESX | ||
func (s *VolumePropertyTestSuite) getVolumePropertiesAdminCli(c *C, volumeName string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is better to return the map from this and let caller to handle and process instead of setting global variable .. it spoils the code continuation and hard to track who sets it.
log.Printf("Getting size, disk-format and attached-to-vm for volume [%s] from ESX [%s] using admin cli. \n", volumeName, hostName) | ||
cmd := admincli.ListVolumes + "-c volume,capacity,disk-format,attached-to 2>/dev/null | grep " + volumeName + " | awk -v OFS='\t' '{print $2, $3, $4}' " | ||
out, _ := ssh.InvokeCommand(hostName, cmd) | ||
admincliValues := strings.Fields(out) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is better to return from here itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me! Now proposed code looks much better and cleaner.
tests/e2e/swarm_test.go
Outdated
@@ -112,7 +112,7 @@ func (s *SwarmTestSuite) TestDockerSwarm(c *C) { | |||
status, host := verification.IsDockerContainerRunning(s.swarmNodes, s.serviceName, 2) | |||
c.Assert(status, Equals, true, Commentf("Container %s is not running on any hosts", s.serviceName)) | |||
|
|||
containerName, err = dockercli.GetContainerName(host, s.serviceName+".1") | |||
containerName, err := dockercli.GetContainerName(host, s.serviceName+".1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
taken care .. need to remove
tests/e2e/volumeproperty_test.go
Outdated
s.containerName = inputparams.GetContainerNameWithTimeStamp(testName) | ||
s.formatTypes = []string{"thin", "zeroedthick", "eagerzeroedthick"} | ||
s.dockerVolumeRemoveCmd = dockerconst.RemoveVolume | ||
log.Println(" SETUP Finished: volumeproperty_test.TestVolumeProperties") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on a second thought .. can we remove this line? IMO, it will not be helpful by looking at SetUpTest
.. if agree please remove otherwise share more details.
tests/e2e/volumeproperty_test.go
Outdated
s.dockerVolumeRemoveCmd = s.dockerVolumeRemoveCmd + strings.Join(s.volumeNames, " ") | ||
out, err = ssh.InvokeCommand(s.config.DockerHosts[1], s.dockerVolumeRemoveCmd) | ||
c.Assert(err, IsNil, Commentf(out)) | ||
log.Println(" CLEAN-UP Finished: volumeproperty_test.TestVolumeProperties") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above .. not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it needed to be done for every test if not then lets do this in TearDownSuite() else it happens for every test.
out, _ := ssh.InvokeCommand(hostName, cmd) | ||
admincliValues := strings.Fields(out) | ||
adminCliMap := make(map[string][]string) | ||
for i := 0; i < len(admincliValues); { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason for not using struct object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments.
tests/e2e/volumeproperty_test.go
Outdated
s.config = inputparams.GetTestConfig() | ||
if s.config == nil { | ||
c.Skip("Unable to retrieve test config, skipping volumeproperty_test.TestVolumeProperties.") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do this only in SetUpSuite(), SetUpTest() runs for every single test.
tests/e2e/volumeproperty_test.go
Outdated
s.dockerVolumeRemoveCmd = s.dockerVolumeRemoveCmd + strings.Join(s.volumeNames, " ") | ||
out, err = ssh.InvokeCommand(s.config.DockerHosts[1], s.dockerVolumeRemoveCmd) | ||
c.Assert(err, IsNil, Commentf(out)) | ||
log.Println(" CLEAN-UP Finished: volumeproperty_test.TestVolumeProperties") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it needed to be done for every test if not then lets do this in TearDownSuite() else it happens for every test.
"volume name, attached-to-vm status, size and disk-format. Actual output: %s", op) | ||
// GetPropertiesForAllVolumesAdminCli returns a map of all the volumes from ESX and their corresponding | ||
// properties returned - capacity, attached-to-vm and disk-format | ||
func GetPropertiesForAllVolumesAdminCli(hostName string) map[string][]string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly also remove the AdminCLI and DockerCLI from the function names
42d6fdb
to
711e966
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
contingent on CI pass.
Addresses Issue #1096
Also converts volumepropverification_test.go to use gocheck testing framework
Ran the test locally. Test output is as follows: