Fixing list_volumes_and_policies() to correctly list volumes #953

pshahzeb · 2017-02-23T00:36:43Z

Using get_volumes() in list_volumes_and_policies() instead of
list_vmdks() to properly walk in dockvols directory which contain
tenant-uuid based subdirectories
This fixes output by admin policy ls command
Fixing update_vsan_objects_with_policy to make proper use
of list_volumes_and_policies()

Testing:
Porting the update_vsan_policy test to tenant based Vmdk ops test suite

Manual testing of admin policy ls command done

[root@sc-rdops-vm07-dhcp-208-78:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py policy create --name poltest --content '(("proportionalCapacity" i0)("hostFailuresToTolerate" i0))'
Successfully created policy: poltest
[root@sc-rdops-vm07-dhcp-208-78:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py policy ls
Policy Name  Policy Content                                              Active
-----------  ----------------------------------------------------------  ------
poltest      (("proportionalCapacity" i0)("hostFailuresToTolerate" i0))  Unused

Creating two volumes using the above policy

root@photon-HqYoHLUks [ ~ ]# docker volume create --driver=vmdk --name=vol_poltest -o vsan-policy-name=poltest
vol_poltest
root@photon-HqYoHLUks [ ~ ]# docker volume create --driver=vmdk --name=vol_poltest_2 -o vsan-policy-name=poltest
vol_poltest_2

[root@sc-rdops-vm07-dhcp-208-78:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py policy ls
Policy Name  Policy Content                                              Active
-----------  ----------------------------------------------------------  -------------------
poltest      (("proportionalCapacity" i0)("hostFailuresToTolerate" i0))  In use by 2 volumes

Resolves: #853 #948

1. Using get_volumes() in list_volumes_and_policies() instead of list_vmdks() to properly walk in dockvols directory which contain tenant-uuid based subdirectories This fixes output by admin policy ls command 2. Fixing update_vsan_objects_with_policy to make proper use of list_volumes_and_policies() Testing: Porting the update_vsan_policy test to tenant based Vmdk ops test suite Manual testing of admin policy ls command done Resolves: #853

pdhamdhere

LGTM contingent to CI pass.

Can you also confirm in-use policy name can't be deleted?

shuklanirdesh82 · 2017-02-23T00:56:28Z

esx_service/vmdk_ops_test.py

@@ -204,33 +204,6 @@ def testAttachAsOpts(self):

    @unittest.skipIf(not vsan_info.get_vsan_datastore(),
                    "VSAN is not found - skipping vsan_info tests")
-    def testPolicyUpdate(self):


is there a reason for removing this test?

This test creates a vmdk inside dockvols folder, assign a good policy to it and then update the policy with bad content. In this case, it tries to list the vmdks associated with the good policy and set the updated policy to it. The test expects the update to fail because of the bad content. In this process, when the vsan_policy update api is invoked, no vmdks are listed because the vmdks are created inside tenant uuid named sub directories as per multi tenancy design. As no vmdk that would be affected are returned, this test case wasn't throwing an error.
I haven't removed it, but moved it to the test suite where tenant based vmdks are created.
Hence it properly lists the vmdk files, tries to update it and fails appropriately because of the bad content of policy

Okay great!! Makes sense!

shuklanirdesh82 · 2017-02-23T00:58:17Z

esx_service/vmdk_ops_test.py

@@ -1039,6 +1012,55 @@ def test_vmdkops_on_tenant_vm(self):
        error_info = vmdk_ops.executeRequest(vm1_uuid, self.vm1_name, self.vm1_config_path, auth.CMD_REMOVE, self.tenant1_vol1_name, opts)
        self.assertEqual(None, error_info)

+    @unittest.skipIf(not vsan_info.get_vsan_datastore(),
+                    "VSAN is not found - skipping vsan_info tests")
+    def testPolicyUpdateBackup(self):


can you confirm whether the test is added to cover proposed code in this PR?

The proposed code in this PR hits in two ways. One through vsan update policy flow, for which following the test case is added. Second is the policy ls admin command which I have described in manual testing in description. Will add a unit test for this as well.

pshahzeb · 2017-02-23T02:19:34Z

@pdhamdhere
Even after this fix, it is possible to delete the policy which is in use by any volume.
Looking into this through #948

1. Minor change to use list_volumes_and_policies() instead of list_vmdks() to find vmdks using a policy Testing: 1. Manual testing done 2. Adding test cases to cover policy ls and policy rm functionality in vmdk_ops_test Resolves: #948

pshahzeb · 2017-02-23T22:46:36Z

This also fixes #948

pdhamdhere

LGTM.

@shuklanirdesh82 Are you okay with Shahzeb's explanation?

shuklanirdesh82 · 2017-02-24T07:03:51Z

esx_service/vmdk_ops_test.py

@@ -204,33 +204,6 @@ def testAttachAsOpts(self):

    @unittest.skipIf(not vsan_info.get_vsan_datastore(),
                    "VSAN is not found - skipping vsan_info tests")
-    def testPolicyUpdate(self):


Okay great!! Makes sense!

shuklanirdesh82 · 2017-02-24T07:18:38Z

Are you okay with Shahzeb's explanation?
@pdhamdhere Yeah!

@pshahzeb, couple of things to take care

Need to repush your changes to validate CI run.. it seems your PR was the victim of starvation today and did not get executed
Need to update summary of the PR to reflect changes for Prohibit policy delete from admin cli if it is in use by any volume #948 for future reference (stopping policy deletion if attached to any volume)

shuklanirdesh82

few comments for the added testcase.

shuklanirdesh82 · 2017-02-24T07:24:14Z

esx_service/vmdk_ops_test.py

+                if len(datastores) > 1:
+                    datastore1 = datastores[1]
+                    self.datastore1_name = datastore1[0]
+                    self.datastoer1_path = datastore[2]


do you mean datastore1[2]?

shuklanirdesh82 · 2017-02-24T07:26:50Z

esx_service/vmdk_ops_test.py

+                    self.datastore1_name = datastore1[0]
+                    self.datastoer1_path = datastore[2]
+                    logging.debug("Found second datastore: datastore_name=%s datastore_path=%s",
+                                  self.datastore1_name, self.datastore1_path)


are we really using self.datastore1_name?

shuklanirdesh82 · 2017-02-24T07:30:53Z

esx_service/vmdk_ops_test.py

+        self.vm1_config_path = vmdk_utils.get_vm_config_path(self.vm1_name)
+        logging.info("VmdkTenantPolicyUsageTestCase: create vm1 name=%s Done", self.vm1_name)
+
+        error, self.vm2 = create_vm(si=si,


Is there a reason for creating second vm? it seems your test is not consuming second vm unless I have mistaken. Please clarify here.

shuklanirdesh82 · 2017-02-24T07:34:16Z

esx_service/vmdk_ops_test.py

+        error_info = vmdk_ops.executeRequest(vm1_uuid, self.vm1_name, self.vm1_config_path,
+                                auth.CMD_REMOVE, self.default_tenant_vol4_name, opts)
+        self.assertEqual(None, error_info)
+


can you add similar check after removing volumes .. usage count should drop to 0 or vmdkops_admin.py policy ls?

…onal checks

shuklanirdesh82

LGTM! Thanks for addressing comments quickly!

You may want to squash the commit into one.

vmwclabot added the cla-not-required label Feb 23, 2017

pdhamdhere approved these changes Feb 23, 2017

View reviewed changes

shuklanirdesh82 suggested changes Feb 23, 2017

View reviewed changes

pdhamdhere approved these changes Feb 24, 2017

View reviewed changes

shuklanirdesh82 approved these changes Feb 24, 2017

View reviewed changes

shuklanirdesh82 suggested changes Feb 24, 2017

View reviewed changes

pshahzeb added 2 commits February 24, 2017 11:06

Removing unused resources in policy usage test case and adding additi…

94d9f5f

…onal checks

Making use if vsan datastore as default

bdcd9b2

shuklanirdesh82 approved these changes Feb 24, 2017

View reviewed changes

shuklanirdesh82 merged commit 2ff4deb into master Feb 24, 2017

shuklanirdesh82 deleted the issue_853.pshahzeb branch February 24, 2017 22:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing list_volumes_and_policies() to correctly list volumes #953

Fixing list_volumes_and_policies() to correctly list volumes #953

pshahzeb commented Feb 23, 2017 •

edited

pdhamdhere left a comment

shuklanirdesh82 Feb 23, 2017

pshahzeb Feb 23, 2017

shuklanirdesh82 Feb 24, 2017

shuklanirdesh82 Feb 23, 2017

pshahzeb Feb 23, 2017

pshahzeb commented Feb 23, 2017

pshahzeb commented Feb 23, 2017

pdhamdhere left a comment

shuklanirdesh82 Feb 24, 2017

shuklanirdesh82 commented Feb 24, 2017

shuklanirdesh82 left a comment

shuklanirdesh82 Feb 24, 2017

shuklanirdesh82 Feb 24, 2017

shuklanirdesh82 Feb 24, 2017

shuklanirdesh82 Feb 24, 2017

shuklanirdesh82 left a comment

Fixing list_volumes_and_policies() to correctly list volumes #953

Fixing list_volumes_and_policies() to correctly list volumes #953

Conversation

pshahzeb commented Feb 23, 2017 • edited

pdhamdhere 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

pshahzeb commented Feb 23, 2017

pshahzeb commented Feb 23, 2017

pdhamdhere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuklanirdesh82 commented Feb 24, 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

shuklanirdesh82 left a comment

Choose a reason for hiding this comment

pshahzeb commented Feb 23, 2017 •

edited