Automate vsan policy test case #1361

lipingxue · 2017-06-06T22:58:16Z

Fixed #1264.
I was not able to run the whole e2e-test locally due to issue #1284, so I run only my newly added test locally. Please refer the following result:

GO15VENDOREXPERIMENT=1 go test github.com/vmware/docker-volume-vsphere/tests/e2e -v -timeout 30m -check.f VsanPolicyTestSuite
=== RUN   Test
2017/06/06 22:07:43 Initializing the SingleNode Config DB on esx [10.160.52.65] 
2017/06/06 22:07:44 START: VsanPolicyTest.TestDeleteVsanPolicyAlreadyInUse
2017/06/06 22:07:44 Creating a vsanPolicy [some-policy] on esx [10.160.52.65]
2017/06/06 22:07:46 Verify vsanPolicy [some-policy] on esx [10.160.52.65] has active as Unused
2017/06/06 22:07:47 policy ls output for vsanPolicy [some-policy] is some-policy  (("proportionalCapacity" i50)("hostFailuresToTolerate" i0))  Unused:
2017/06/06 22:07:47 Creating volume [vsanVol@vsanDatastore] on VM [10.160.51.216]
2017/06/06 22:07:51 Verify vsanPolicy [some-policy] on esx [10.160.52.65] has active as In use by 1 volumes
2017/06/06 22:07:52 policy ls output for vsanPolicy [some-policy] is some-policy  (("proportionalCapacity" i50)("hostFailuresT..  In use by 1 volumes:
2017/06/06 22:07:52 Removing a vsanPolicy [some-policy] on esx [10.160.52.65]
2017/06/06 22:07:54 Remove vsanPolicy "some-policy" returns with Error: Cannot remove. Policy is in use by {'volume': 'vsanVol.vmdk', 'path': '/vmfs/volumes/vsanDatastore/dockvols/11111111-1111-1111-1111-111111111111', 'policy': 'some-policy'}
2017/06/06 22:07:54 END: VsanPolicyTest.TestDeleteVsanPolicyAlreadyInUse
2017/06/06 22:07:54 Removing the SingleNode Config DB on esx [10.160.52.65] 
OK: 1 passed
--- PASS: Test (11.26s)

pshahzeb · 2017-06-06T23:14:44Z

tests/constants/admincli/cmd.go

+
+	// CreateVsanPolicy referring to create vsanPolicy
+	// where --name will be name of the vsanPolicy
+	CreateVsanPolicy = vmdkopsAdmin + "policy create --name="


I think we already have a constant to create policy in the same file. Not sure if this is needed.

I don't use that. I think in the current code, no use for "CreatePolicy". I will remove it.

It is being used. we had a separate PR to introduce a common utils. The change that I merged yesterday had a test to create volume with vsan policy. That uses CreatePolicy

pshahzeb · 2017-06-06T23:15:29Z

tests/constants/admincli/vsanpolicy.go

+	PolicyContent = "'((\"proportionalCapacity\" i50)''(\"hostFailuresToTolerate\" i0))'"
+
+	// vsanDatastore is the name of vsanDatastore which will be used in test
+	vsanDatastore = "vsanDatastore"


use util govc.GetDatastoreByType to get the name of vsan datastore. This name might be different in different setups.

pshahzeb · 2017-06-06T23:15:59Z

tests/constants/admincli/vsanpolicy.go

+	vsanDatastore = "vsanDatastore"
+
+	// VsanVol1 is the name of volume to be created on vsanDatastore
+	VsanVol1 = "vsanVol1@" + vsanDatastore


use util to get the unique volume name and then append vsan datastore name

pshahzeb

Test is good.
I have a few comments about using the latest utils introduced in recent PRs

pshahzeb · 2017-06-06T23:41:40Z

tests/e2e/vsan_policy_test.go

+}
+
+func (s *VsanPolicyTestSuite) SetUpSuite(c *C) {
+	s.hostIP = os.Getenv("VM1")


henceforth, we plan to use the common test config introduced in #1286 . You can refer vmgroup tests to see how it that config is consumed.

pshahzeb · 2017-06-06T23:42:56Z

tests/e2e/vsan_policy_test.go

+	res := admincli.VerifyActiveFromVsanPolicyListOutput(s.esxIP, con.PolicyName, "Unused")
+	c.Assert(res, Equals, true, Commentf("vsanPolicy should be \"Unused\""))
+
+	out, err = dockercli.CreateVolumeWithVsanPolicy(s.hostIP, con.VsanVol1, con.PolicyName)


do we need this extra util since using a vsan policy for policy is just a matter of specifying option? we have a util to create volume by options.

pshahzeb · 2017-06-06T23:43:23Z

tests/e2e/vsan_policy_test.go

+// 6. run "vmdkops_admin policy rm" to remove the policy, which should fail since the volume is still
+// use the vsan policy
+func (s *VsanPolicyTestSuite) TestDeleteVsanPolicyAlreadyInUse(c *C) {
+	log.Printf("START: VsanPolicyTest.TestDeleteVsanPolicyAlreadyInUse")


minor: use the misc util to log start and end of test.

pshahzeb · 2017-06-06T23:46:14Z

tests/utils/verification/volumeproperties.go

@@ -172,3 +172,13 @@ func VerifyDetachedStatus(name, hostName, esxName string) bool {
 	log.Printf("Timed out to poll status\n")
 	return false
 }
+
+// GetVsanPolicyNameVOlumeUsed returns the vsan policy name used by the volume using docker cli
+func GetVsanPolicyNameVolumeUsedDockerCli(volName string, hostname string) string {


quite a big name. I would suggest GetPolicyNameforVol

pshahzeb · 2017-06-06T23:46:37Z

tests/utils/admincli/vsan_policy.go

+		return false
+	}
+	log.Printf("policy ls output for vsanPolicy [%s] is %s:", policyName, out)
+	res := strings.Contains(out, active)


return strings.Contains directly

pshahzeb · 2017-06-06T23:48:02Z

tests/e2e/vsan_policy_test.go

+
+	out, err = admincli.RemoveVsanPolicy(s.esxIP, con.PolicyName)
+	log.Printf("Remove vsanPolicy \"%s\" returns with %s", con.PolicyName, out)
+	c.Assert(out, Matches, "Error: Cannot remove.*", Commentf("vsanPolicy is still used by volumes and cannot be removed"))


this check is good. I think err is also not nil here. Can add one more assert.

err is Nil here.

ashahi1 · 2017-06-07T02:35:14Z

tests/e2e/vsan_policy_test.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// This test suite tries to add, remove and replace vm to the _DEFAULT vmgroup


This doesn't look right. Can you please check?

ashahi1 · 2017-06-07T02:36:12Z

tests/e2e/vsan_policy_test.go

+	"log"
+	"os"
+
+	con "github.com/vmware/docker-volume-vsphere/tests/constants/admincli"


How about admincliconst?

ashahi1 · 2017-06-07T02:42:50Z

tests/utils/admincli/vsan_policy.go

+// limitations under the License.
+
+// This util holds various helper methods related to vmgroup to be consumed by testcases.
+// vmgroup creation, deletion or adding, removing and replacing vm from vmgroup is supported currently.


Please fix this. This util does not contains vmgroup helper methods

ashahi1 · 2017-06-07T02:56:30Z

tests/utils/verification/volumeproperties.go

+	cmd := dockercli.InspectVolume + " --format '{{index .Status \"vsan-policy-name\"}}' " + volName
+	op, _ := ssh.InvokeCommand(hostname, cmd)
+	if op == "" {
+		log.Fatal("Null value is returned by docker cli when looking for attached to vm field for volume. Output: ", op)


We should avoid using fatal logs.

lipingxue · 2017-06-07T23:32:14Z

@pshahzeb @ashahi1 I have addressed your comments, please review it.

shaominchen

Looks good. Some nits below.

shaominchen · 2017-06-07T23:39:12Z

tests/constants/admincli/cmd.go

@@ -85,4 +85,10 @@ const (

 	// ReadWriteAccess read-write rights for the volume
 	ReadWriteAccess = "read-write"
+
+	// ListPolicy referring to list all existing policys


Nit: policys => policies

shaominchen · 2017-06-07T23:42:57Z

tests/constants/admincli/vsanpolicy.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// A home to hold test constants related with vmdkops_admin cli.


Please update the comments.

shaominchen · 2017-06-07T23:45:30Z

tests/constants/admincli/vsanpolicy.go

+
+package admincli
+
+const (


Since we introduced a separate file to hold VSAN policy specific constants, I think we should move the "CreatePolicy" constant from cmd.go to this file.

shaominchen · 2017-06-07T23:50:30Z

tests/constants/admincli/vsanpolicy.go

+	PolicyName = "some-policy"
+
+	// PolicyContent is the content of vsan policy which will be used in test
+	PolicyContent = "'((\"proportionalCapacity\" i50)''(\"hostFailuresToTolerate\" i0))'"


Since we introduced this constant, let's use this in the existing code: vsan_test.go: line 82.

shaominchen · 2017-06-07T23:51:46Z

tests/e2e/vsan_test.go

@@ -101,3 +103,41 @@ func (s *VsanTestSuite) TestVSANPolicy(c *C) {

 	misc.LogTestStart(volCreateTest, "TestVSANPolicy")
 }
+


Let's add a brief description about the purpose of this test (even though the test name is pretty clear)

shaominchen · 2017-06-07T23:58:23Z

tests/utils/admincli/cmd.go

@@ -29,6 +30,26 @@ func CreatePolicy(ip, name, content string) (string, error) {
 	return ssh.InvokeCommand(ip, admincli.CreatePolicy+" --name "+name+" --content "+content)
 }

+// RemovePolicy method is going to remove a policy.


Let's keep it simple: "RemovePolicy removes a policy."

Please note that in Go language a method is a special function with a receiver argument, i.e. it belongs to a specific type.

shaominchen · 2017-06-07T23:59:16Z

tests/utils/admincli/cmd.go

+
+// VerifyActiveFromVsanPolicyListOutput is going to check, for the given vsan policy, the active
+// column returned by "vmdkops policy ls" command is the same as the value specified by
+// param @active


Nit: remove this if there's no meaningful comments for this parameter.

shaominchen · 2017-06-08T00:00:44Z

tests/utils/verification/volumeproperties.go

@@ -188,3 +188,13 @@ func VerifyDetachedStatus(name, hostName, esxName string) bool {
 	log.Printf("Timed out to poll status\n")
 	return false
 }
+
+// GetPolicyNameForVol returns the vsan policy name used by the volume using docker cli
+func GetPolicyNameForVol(volName string, hostname string) string {


In the existing code we are following a convention to put the hostname as the 1st parameter, so let's just follow this.

shaominchen · 2017-06-08T00:01:04Z

vmdk_plugin/Makefile

@@ -366,7 +366,7 @@ e2e-test-runalways:

 e2e-test-runonce:
 	$(log_target)
-	$(GO) test -v -timeout 30m -tags runonce $(E2E_Tests)
+	$(GO) test $(E2E_Tests) -v -timeout 30m -check.f VsanPolicyTestSuite


Please revert this before merge.

shaominchen · 2017-06-08T00:04:20Z

tests/e2e/vsan_test.go

+// 6. run "vmdkops_admin policy rm" to remove the policy, which should fail since the volume is still
+// use the vsan policy
+func (s *VsanTestSuite) TestDeleteVsanPolicyAlreadyInUse(c *C) {
+	misc.LogTestStart("VsanTestSuite", "TestDeleteVsanPolicyAlreadyInUse")


Nit: In the existing tests we are following a convention to use the file name, i.e. "vsan_test" instead of "VsanTestSuite". Both are equally fine, but let's just keep all tests consistent.

It looks like the constant at line 41 is not being used. Let's change it to:
testName = "vsan_test"
And use it in these tests.

shaominchen · 2017-06-08T00:41:53Z

tests/e2e/vsan_test.go

+// is shown as "In use by 1 volumes"
+// 6. run "vmdkops_admin policy rm" to remove the policy, which should fail since the volume is still
+// use the vsan policy
+func (s *VsanTestSuite) TestDeleteVsanPolicyAlreadyInUse(c *C) {


Since we are adding more VSAN related test cases, let's also rename the existing test case "TestVSANPolicy" - the name is too generic. In fact we should split it into two.

I know this request is out of the scope of this PR, but I think the change is straightforward, so I'd suggest to fix it.

pshahzeb · 2017-06-08T17:13:11Z

Looks good. Please revert vmdk_plugin/Makefile
Will give LGTM once CI passes. U might want to disable volume_access_test by changing it's tag since we are intermittently hitting the issue tracked through #1371

lipingxue · 2017-06-08T23:10:58Z

@shaominchen @pshahzeb I have addressed your comments and CI is passed too. Please take a look.

shaominchen

Looks good. Few more nits below.

shaominchen · 2017-06-08T23:20:01Z

tests/e2e/vsan_test.go

-
+// 2. Volume creation with valid policy should pass
+// 3. Valid volume should be accessible
+func (s *VsanTestSuite) TestVolumeWithValidPolicy(c *C) {


Nit: I think we can rename this to TestValidPolicy (or TestCreateVolumeWithValidPolicy, which seems too long), and add a comment above.

shaominchen · 2017-06-08T23:20:23Z

tests/e2e/vsan_test.go

+// 1. Create an invalid vsan policy (wrong content)
+// 2. Volume creation with non existing policy should fail
+// 3. Volume creation with invalid policy should fail
+func (s *VsanTestSuite) TestVolumeWithInValidPolicy(c *C) {


Same as above, we can rename this to TestInvalidPolicy and add a comment above.

shaominchen · 2017-06-08T23:23:11Z

tests/utils/admincli/cmd.go

+
+// VerifyActiveFromVsanPolicyListOutput is going to check, for the given vsan policy, whether the active
+// column returned by "vmdkops policy ls" command matches the value specified by input param "active"
+func VerifyActiveFromVsanPolicyListOutput(ip, policyName, active string) bool {


How about renaming this to IsVsanPolicyInUse?

+1. VerifyActiveFromVsanPolicyListOutput is too long

This function is not test whether the vsan policy is in use. It need to test whether the active column from vsan policy output is the same as expected value.

I would suggest to be renamed as IsVsanPolicyInUse or IsVsanPolicyActive.

Since we are using this function to verify "unused" status as well, I think we can rename it to "verifyVsanPolicyStatus".

shuklanirdesh82 · 2017-06-09T05:27:30Z

tests/e2e/vsan_test.go

@@ -36,6 +38,7 @@ import (
 )

 const (
+	vsanTest       = "vsan_test"
 	vsanVolumeTest = "vsan_vol"
 	vsanPolicyFlag = "vsan-policy-name"


this should be part of tests/constants/admincli/vsanpolicy.go

Those const are only used in this file locally.

I meant for vsan-policy-name, shouldn't it be moved to tests/constants/admincli/vsanpolicy.go?

I think we only need to put const which need to me shared across the package to /tests/constants/...

correct!

By taking vsanOpts := " -o " + vsanPolicyFlag + "=" + policyName as an example; it is going to be consumed by multiple files in the future.

Anyway this is a trivial comment, my only intention is to avoid future refactoring.

shuklanirdesh82 · 2017-06-09T05:28:59Z

tests/e2e/vsan_test.go

+// 2. Volume creation with valid policy should pass
+// 3. Valid volume should be accessible
+func (s *VsanTestSuite) TestVolumeWithValidPolicy(c *C) {
+	misc.LogTestStart(vsanTest, "TestVolumeWithValidPolicy")


as discussed during standup today .. let's use c.TestName and avoid using local constant.

Reference: #1368

shuklanirdesh82 · 2017-06-09T05:31:54Z

tests/utils/verification/volumeproperties.go

@@ -150,3 +150,13 @@ func VerifyDetachedStatus(name, hostName, esxName string) bool {
 	log.Printf("Timed out to poll status\n")
 	return false
 }
+
+// GetPolicyNameForVol returns the vsan policy name used by the volume using docker cli
+func GetPolicyNameForVol(hostname string, volName string) string {


better home should be tests/utils/dockercli so the reference will be dockercli. GetPolicyNameForVol

how about renaming to GetAssociatedPolicyName()?

I don't think we should move this function to test/utils/dockercli. This function is used to get some volume attribute, which is the same as existing function in this file.

shuklanirdesh82 · 2017-06-09T05:35:03Z

tests/utils/verification/volumeproperties.go

+// GetPolicyNameForVol returns the vsan policy name used by the volume using docker cli
+func GetPolicyNameForVol(hostname string, volName string) string {
+	cmd := dockercli.InspectVolume + " --format '{{index .Status \"vsan-policy-name\"}}' " + volName
+	op, _ := ssh.InvokeCommand(hostname, cmd)


is there a reason for ignoring error field? Util may return err field to testcase and let test case to fail with proper message.

shuklanirdesh82 · 2017-06-09T05:37:02Z

tests/constants/admincli/vsanpolicy.go

+	PolicyContent = "'((\"proportionalCapacity\" i50)''(\"hostFailuresToTolerate\" i0))'"
+
+	// CreatePolicy Create a policy
+	CreatePolicy = vmdkopsAdmin + " policy create "


It is better to append --name= e.g. https://github.com/vmware/docker-volume-vsphere/blob/master/tests/constants/admincli/cmd.go#L37

Move the test code to vsan_test.go and delete unused files. Minor fix.

Address comments from Sam. Minor fix.

shaominchen

LGTM.

shaominchen · 2017-06-09T18:30:38Z

tests/e2e/vsan_test.go

 }

 // Steps:
 // 1. Create an invalid vsan policy (wrong content)
 // 2. Volume creation with non existing policy should fail
 // 3. Volume creation with invalid policy should fail
-func (s *VsanTestSuite) TestVolumeWithInValidPolicy(c *C) {
-	misc.LogTestStart(vsanTest, "TestVolumeWithInValidPolicy")
+func (s *VsanTestSuite) TestInValidPolicy(c *C) {


Nit: InValid => Invalid

shuklanirdesh82

LGTM!

Please address open comments before merging in.

shuklanirdesh82 · 2017-06-09T21:44:46Z

tests/e2e/vsan_test.go

+	out, err = admincli.RemovePolicy(s.config.EsxHost, adminclicon.PolicyName)
+	log.Printf("Remove vsanPolicy \"%s\" returns with %s", adminclicon.PolicyName, out)
+	c.Assert(out, Matches, "Error: Cannot remove.*", Commentf("vsanPolicy is still used by volumes and cannot be removed"))
+


Need to add following verification after removing policy.

invoke ListPolicy (admincli policy ls) and make sure the policy adminclicon.PolicyName is not part of the output ... that validate the policy removal part.

vmwclabot added the cla-not-required label Jun 6, 2017

pshahzeb reviewed Jun 6, 2017

View reviewed changes

ashahi1 reviewed Jun 7, 2017

View reviewed changes

lipingxue force-pushed the vsan_policy_test.liping branch 2 times, most recently from 4f52423 to a4fe92c Compare June 7, 2017 23:30

shaominchen reviewed Jun 8, 2017

View reviewed changes

lipingxue force-pushed the vsan_policy_test.liping branch from a4fe92c to a164d12 Compare June 8, 2017 18:03

shaominchen reviewed Jun 8, 2017

View reviewed changes

pshahzeb approved these changes Jun 9, 2017

View reviewed changes

shuklanirdesh82 suggested changes Jun 9, 2017

View reviewed changes

shuklanirdesh82 assigned lipingxue Jun 9, 2017

lipingxue added 6 commits June 9, 2017 10:06

Automate vsanpolicy test.

23f7ebc

Cleanup the code.

50e2a6f

Address comments from Shahzeb and Anup.

0090066

Move the test code to vsan_test.go and delete unused files.

aaa5c6b

Move the test code to vsan_test.go and delete unused files. Minor fix.

Address comments from Sam.

aaf0f60

Address comments from Sam. Minor fix.

Address comments from Nirdesh and Sam.

c295eca

lipingxue force-pushed the vsan_policy_test.liping branch from a164d12 to c295eca Compare June 9, 2017 17:06

shaominchen approved these changes Jun 9, 2017

View reviewed changes

Small fix to change the test name.

9dd8b67

Address more comments from Nirdesh.

11f77d0

shuklanirdesh82 approved these changes Jun 9, 2017

View reviewed changes

Add code to removing policys in test tearDown.

5c0544c

lipingxue merged commit 8ed1887 into master Jun 10, 2017

shuklanirdesh82 deleted the vsan_policy_test.liping branch November 10, 2017 05:30

		@@ -101,3 +103,41 @@ func (s VsanTestSuite) TestVSANPolicy(c C) {

		misc.LogTestStart(volCreateTest, "TestVSANPolicy")
		}

Automate vsan policy test case #1361

Automate vsan policy test case #1361

Conversation

lipingxue commented Jun 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

lipingxue commented Jun 7, 2017

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

pshahzeb commented Jun 8, 2017 • edited Loading

lipingxue commented Jun 8, 2017

shaominchen left a comment

Choose a reason for hiding this comment

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

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 commented Jun 8, 2017 •

edited

Loading

shaominchen Jun 8, 2017 •

edited

Loading