Skip to content
This repository has been archived by the owner on Nov 9, 2020. It is now read-only.

Fix Syntax Warning when running adminCLI command. #1181

Merged
merged 3 commits into from
Apr 20, 2017
Merged

Fix Syntax Warning when running adminCLI command. #1181

merged 3 commits into from
Apr 20, 2017

Conversation

lipingxue
Copy link
Contributor

Fixed #1180

@lipingxue
Copy link
Contributor Author

Test manually.
Run the AdminCLI command, and no "Syntax Warning".

[root@sc2-rdops-vm02-dhcp-88-137:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py vmgroup ls
Uuid                                  Name      Description                Default_datastore  VM_list  
------------------------------------  --------  -------------------------  -----------------  -------  
11111111-1111-1111-1111-111111111111  _DEFAULT  This is a default vmgroup                              
e82dd74f-8985-407c-ae87-009aa5b282b0  vmgroup1                             vsanDatastore               
8cc5401d-f0b0-4f5b-8516-a5407be1f03f  vmgroup2

pshahzeb
pshahzeb reviewed Apr 19, 2017
View reviewed changes
esx_service/utils/vmdk_utils.py Outdated
logging.debug("init_datastoreCache: %s", datastores)

with lockManager.get_lock("init_datastoreCache"):
global datastores

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as @govint has pointed out, we need to have references of datastores under the lock.
Need to move line 76 and 77 under with... statement.

@pshahzeb pshahzeb mentioned this pull request Apr 20, 2017
Merged
@lipingxue
Copy link
Contributor Author

@pshahzeb I have addressed your comments. Please take a look.

pshahzeb
pshahzeb approved these changes Apr 20, 2017
View reviewed changes
Copy link
Contributor

@pshahzeb pshahzeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

shaominchen
shaominchen approved these changes Apr 20, 2017
View reviewed changes
Copy link
Contributor

@shaominchen shaominchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

esx_service/utils/vmdk_utils.py
@@ -73,10 +73,9 @@ def init_datastoreCache(force=False):
from local ESX host. force=True will force it to ignore current cache
and force init
"""
logging.debug("init_datastoreCache: %s", datastores)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd suggest to add logs before and after acquiring locks so that it's easier to debug potential dead lock issues.

@lipingxue lipingxue merged commit dd3eb67 into master Apr 20, 2017
@shuklanirdesh82 shuklanirdesh82 deleted the fix_syntax_warning.liping branch April 20, 2017 18:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pshahzeb pshahzeb pshahzeb approved these changes

@shaominchen shaominchen shaominchen approved these changes

Assignees
No one assigned
Labels
cla-not-required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lipingxue @shaominchen @pshahzeb @vmwclabot