Use VirtualDiskManager API in vmdk_ops. #792

brunotm · 2016-12-03T18:32:26Z

This PR remove the usage of vmkfstools in vmdk_ops, and provide other small fixes.

With this change i was unable to reproduce the timing/locking (open descriptors after vmkfstools completed execution) issues observed in the parallel tests.

Probably fixes #768 #695 and part of #39

//CC @msterin @kerneltime @govint

msterin · 2016-12-05T04:27:34Z

@kerneltime - any hints as to why this one shows "failed" ? the log seems to indicate a pass...

msterin

Looks good, a few comments/questions inline

msterin · 2016-12-05T04:40:35Z

esx_service/vmdk_ops.py

+    volume_datastore_path = vmdk_utils.get_datastore_path(vmdk_path)
+
+    si = get_si()
+    task = si.content.virtualDiskManager.CreateVirtualDisk(


[FYI] in just-released-6.5, the recommended way seems to be HostCreateDisk_Task(createDisk), see
https://pubs.vmware.com/vsphere-65/index.jsp#com.vmware.wssdk.apiref.doc/vim.VirtualDiskManager.html?path=4_2_0_2_5_3_98#createVirtualDisk
I think we should stick with the CreateVirtualDisk() as soon as we carefully test on VMFS and VSAN

Thanks, i wasn't aware of that. Also pyvmomi doesn't seem to expose it yet.

Can we please file a tracking issue to investigate s/vim.VirtualDiskManager/ vim.vslm.host.VStorageObjectManager/ APIs.

msterin> created #799

msterin · 2016-12-05T04:41:53Z

esx_service/vmdk_ops.py

+    # Handle vsan policy
+    if kv.VSAN_POLICY_NAME in opts:
+        if not vsan_policy.set_policy_by_name(vmdk_path, opts[kv.VSAN_POLICY_NAME]):
+            logging.error("Could not set policy: %s to volume %s",


this seems to be treated as a warning and the policy is ignored. We should at least say ("policy ignored") then

I wasn't sure of what to do in that case.
Rolling back creation for something that can be updated in admin cli seemed overkill. So i just logged and ignored it.

You're right, it should be clear in the metadata that the volume has no policy.

I have misread your comment. The mean was for logging and not for metadata, right?

Maybe we could just drop the logging here and delete the key from opts, since we'll get the warning from vsan_info.set_policy:

rc, out = vmdk_ops.RunCommand(OBJTOOL_SET_POLICY.format(uuid, policy_string)) if rc != 0: logging.warning("Failed to set policy for %s : %s", vmdk_path, out)

msterin · 2016-12-05T04:44:39Z

esx_service/vmdk_ops_test.py

-        os.makedirs(path)
+    # Calculate the path, use the first datastore in datastores
+    datastores = vmdk_utils.get_datastores()
+    path = datastores[0][2]


when the dir is created on a fresh test bed then ?

vmdk_utils.get_datastores -> vmdk_utils.init_datastoreCache -> vmdk_ops.get_vol_path

brunotm · 2016-12-05T06:49:40Z

@msterin

@kerneltime - any hints as to why this one shows "failed" ? the log seems to indicate a pass...

There is a failure:

FAIL: testPolicyUpdate (__main__.VmdkCreateRemoveTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/vmdk_ops_unittest17937/vmdk_ops_test.py", line 207, in testPolicyUpdate
    self.assertEqual(err, None, err)
AssertionError: {u'Error': 'File /vmfs/volumes/vsanDatastore/dockvols/vol_UnitTest_Create.vmdk already exists'}

This is not really clear to me, because all the tests in VmdkCreateRemoveTestCase that create the vol_UnitTest_Create volume has a assertion for the removal. I will need to add additional logging in the tests to investigate this.

brunotm · 2016-12-05T15:38:08Z

@msterin @kerneltime
On the first run the removeVMDK in tearDown is failing with (but it doesn't fail the test):

11/15/16 14:29:21 6048576 [MainThread] [INFO   ] *** removeVMDK: /vmfs/volumes/datastore1/dockvols/vol_UnitTest_Create.vmdk
11/15/16 14:29:21 6048576 [MainThread] [ERROR  ] Failed to access /vmfs/volumes/datastore1/dockvols/vol_UnitTest_Create-1d616b145f50626a.vmfd
Traceback (most recent call last):
 File "/usr/lib/vmware/vmdkops/Python/kvESX.py", line 265, in load
   with open(meta_file, "r") as fh:

I still have to identify the cause. As i don't have a vsan environment right now to test, i have submitted another run to fetch logs, sorry about that.

To help debugging wouldn't be better to have the removeVMDK asserted inside testPolicyUpdate?

vmdk_ops.create/clone/removeVMDK: Use VirtualDiskManager API vsan_policy: Add get_policy_content vsan_policy: Add set_policy_by_name volume_kv: Make VALID_ALLOCATION_FORMATS a dict of "user option": "api value"

… can be correctly initialised vsan_info.get_vsan_dockvols_path: use vmdk_ops.get_vol_path

@msterin

Is already logged in vsan_info.set_policy @msterin

Assert removeVMDK within test scope

brunotm · 2016-12-06T16:36:00Z

@msterin @kerneltime
The issue with the testPolicyUpdate is fixed, the cause was the tearDown looking for the volume in the global path.

I can also confirm that this change eliminates the timing/locking issue.
From my part is ready for review/merge.

Thanks.

pdhamdhere

LGTM.

pdhamdhere · 2016-12-06T21:55:10Z

esx_service/vmdk_ops.py

+    volume_datastore_path = vmdk_utils.get_datastore_path(vmdk_path)
+
+    si = get_si()
+    task = si.content.virtualDiskManager.CreateVirtualDisk(


Can we please file a tracking issue to investigate s/vim.VirtualDiskManager/ vim.vslm.host.VStorageObjectManager/ APIs.

msterin> created #799

brunotm force-pushed the brunotm.vmdk-api branch 5 times, most recently from b7134aa to 7c344c0 Compare December 4, 2016 16:50

msterin reviewed Dec 5, 2016

View reviewed changes

brunotm force-pushed the brunotm.vmdk-api branch 2 times, most recently from bffcd72 to ff3ea63 Compare December 5, 2016 15:26

brunotm force-pushed the brunotm.vmdk-api branch from ff3ea63 to 4565922 Compare December 5, 2016 16:44

brunotm added 7 commits December 6, 2016 13:53

convert: Add convert_to_KB for VirtualDiskManager API

85e2672

Use VirtualDiskManager API for CCR ops.

1eabfed

vmdk_ops.create/clone/removeVMDK: Use VirtualDiskManager API vsan_policy: Add get_policy_content vsan_policy: Add set_policy_by_name volume_kv: Make VALID_ALLOCATION_FORMATS a dict of "user option": "api value"

vmdk_utils.init_datastoreCache: Make sure we only use datastores that…

1a1058d

… can be correctly initialised vsan_info.get_vsan_dockvols_path: use vmdk_ops.get_vol_path

vmdk_ops_test: Use vmdk_utils.get_datastores() for path calculation

5400271

auth_data._remove_volumes_for_tenant: Fix error message concatenation

8af779a

Remove logging error in failure setting policy

4238cfa

Is already logged in vsan_info.set_policy @msterin

Fix testPolicyUpdate

45dcba8

Assert removeVMDK within test scope

brunotm force-pushed the brunotm.vmdk-api branch from 4565922 to 45dcba8 Compare December 6, 2016 13:54

pdhamdhere reviewed Dec 6, 2016

View reviewed changes

msterin mentioned this pull request Dec 7, 2016

Investigate slvim APIs for vSPhere 6.5 #799

Open

msterin approved these changes Dec 7, 2016

View reviewed changes

msterin merged commit 88704cb into vmware-archive:master Dec 7, 2016

msterin mentioned this pull request Feb 2, 2017

remove command lines usage where API is available #39

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use VirtualDiskManager API in vmdk_ops. #792

Use VirtualDiskManager API in vmdk_ops. #792

brunotm commented Dec 3, 2016 •

edited

Loading

msterin commented Dec 5, 2016

msterin left a comment

msterin Dec 5, 2016

brunotm Dec 5, 2016

pdhamdhere Dec 6, 2016 •

edited by msterin

Loading

msterin Dec 5, 2016

brunotm Dec 5, 2016

brunotm Dec 5, 2016

brunotm Dec 5, 2016

msterin Dec 5, 2016

brunotm Dec 5, 2016

brunotm commented Dec 5, 2016

brunotm commented Dec 5, 2016

brunotm commented Dec 6, 2016

pdhamdhere left a comment

pdhamdhere Dec 6, 2016 •

edited by msterin

Loading

Use VirtualDiskManager API in vmdk_ops. #792

Use VirtualDiskManager API in vmdk_ops. #792

Conversation

brunotm commented Dec 3, 2016 • edited Loading

msterin commented Dec 5, 2016

msterin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdhamdhere Dec 6, 2016 • edited by msterin 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

brunotm commented Dec 5, 2016

brunotm commented Dec 5, 2016

brunotm commented Dec 6, 2016

pdhamdhere left a comment

Choose a reason for hiding this comment

pdhamdhere Dec 6, 2016 • edited by msterin Loading

Choose a reason for hiding this comment

brunotm commented Dec 3, 2016 •

edited

Loading

pdhamdhere Dec 6, 2016 •

edited by msterin

Loading

pdhamdhere Dec 6, 2016 •

edited by msterin

Loading