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

Handling dockvols symlink on vsanDatastore to update volume status on VM poweroff event #1235

Merged
merged 2 commits into from
May 10, 2017
Merged

Handling dockvols symlink on vsanDatastore to update volume status on VM poweroff event #1235

merged 2 commits into from
May 10, 2017

Conversation

pshahzeb
Copy link
Contributor

Resolving the symlinks for dockvols directory for a datastore and using it to decide if it is a vDVS volume.

Fixes #1233

Testing:

  1. Attach a vsan and shared-Vmfs volume to a container.
root@sc-rdops-vm02-dhcp-52-237:~# docker volume ls
DRIVER              VOLUME NAME
vsphere:latest      vol1@vsanDatastore
vsphere:latest      vol2@sharedVmfs-0
root@sc-rdops-vm02-dhcp-52-237:~# docker run -it -v vol1@vsanDatastore:/vol1 -v vol2@sharedVmfs-0:/vol2 busybox
/ #

[root@sc2-rdops-vm03-dhcp-126-220:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py volume ls
Volume  Datastore      VMGroup   Capacity  Used  Filesystem  Policy          Disk Format  Attached-to   Access      Attach-as               Created By    Created Date
------  -------------  --------  --------  ----  ----------  --------------  -----------  ------------  ----------  ----------------------  ------------  ------------------------
vol2    sharedVmfs-0   _DEFAULT  100MB     15MB  ext4        N/A             thin         ubuntu-VM0.0  read-write  independent_persistent  ubuntu-VM0.0  Wed May 10 18:33:22 2017
vol1    vsanDatastore  _DEFAULT  100MB     80MB  ext4        [VSAN default]  thin         ubuntu-VM0.0  read-write  independent_persistent  ubuntu-VM0.0  Wed May 10 18:32:53 2017
  1. Kill the VM
  2. check the status of volumes
[root@sc2-rdops-vm03-dhcp-126-220:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py volume ls
Volume  Datastore      VMGroup   Capacity  Used  Filesystem  Policy          Disk Format  Attached-to  Access      Attach-as               Created By    Created Date
------  -------------  --------  --------  ----  ----------  --------------  -----------  -----------  ----------  ----------------------  ------------  ------------------------
vol2    sharedVmfs-0   _DEFAULT  100MB     15MB  ext4        N/A             thin         detached     read-write  independent_persistent  ubuntu-VM0.0  Wed May 10 18:33:22 2017
vol1    vsanDatastore  _DEFAULT  100MB     80MB  ext4        [VSAN default]  thin         detached     read-write  independent_persistent  ubuntu-VM0.0  Wed May 10 18:32:53 2017

msterin
msterin suggested changes May 10, 2017
View reviewed changes
Copy link
Contributor

@msterin msterin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The code is clean but it seems to be redundant with check_docker_volume(). Which also seems to have the same issue.
I suggest changing 'bool check_docker_volume' to ' 'disk_path docker_volume_path(dev)' (return None if not a path, returns path if it is) , fix the issue with realpath and use this in both new code and check_volumes_mounted
Oops, ignore. I was looking at a stale branch. LGTM with a couple of nits

esx_service/utils/vmdk_utils.py
@@ -64,6 +64,9 @@
VMDK_RETRY_COUNT = 5
VMDK_RETRY_SLEEP = 1

# root for all the volumes
VOLUME_ROOT = "/vmfs/volumes/"
Copy link
Contributor

Choose a reason for hiding this comment

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

If you introduce this (which is good), you'd have to update the code to use it, at east in vmdk_utils.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this string has been used as a part regex in some. Will do this as a part refactoring all constants in our vmdkops and vmdk_utils separately.

esx_service/utils/vmdk_utils.py
if disk_path.startswith(vmdk_ops.DOCK_VOLS_DIR):
# returning the vmdk path for dvs volume
return os.path.join("/vmfs/volumes/", datastore, disk_path)
# find the dockvols dir on current datastore and resolve symlinks if any
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add comment to L339 that 'dev' is of type vim.Device (I think)

esx_service/utils/vmdk_utils.py Outdated
return os.path.join("/vmfs/volumes/", datastore, disk_path)
# find the dockvols dir on current datastore and resolve symlinks if any
dvol_dir_path = os.path.realpath(os.path.join(VOLUME_ROOT,
datastore, vmdk_ops.DOCK_VOLS_DIR))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: better to name it datastore_name

shaominchen
shaominchen approved these changes May 10, 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
@@ -64,6 +64,9 @@
VMDK_RETRY_COUNT = 5
VMDK_RETRY_SLEEP = 1

# root for all the volumes
VOLUME_ROOT = "/vmfs/volumes/"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to your change: the string "/vmfs/volumes" has been hardcoded everywhere in our code base. We should refactor those code to use this constant. Let's file an issue for tracking purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will use this constant in this file for now.

@pshahzeb
Copy link
Contributor Author

#1236 for usages of constants in our esx side code.

pdhamdhere
pdhamdhere approved these changes May 10, 2017
View reviewed changes
Copy link
Contributor

@pdhamdhere pdhamdhere left a comment

Choose a reason for hiding this comment

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

LGTM.

@pshahzeb pshahzeb merged commit 0577889 into master May 10, 2017
@shuklanirdesh82 shuklanirdesh82 deleted the issue_1233.pshahzeb branch May 13, 2017 13:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@msterin msterin msterin approved these changes

@pdhamdhere pdhamdhere pdhamdhere 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.
5 participants
@pshahzeb @msterin @pdhamdhere @shaominchen @vmwclabot