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

Get attached VM name for attached volume in admin CLI #745

Merged
merged 1 commit into from
Nov 17, 2016
Merged

Conversation

govint
Copy link
Contributor

@govint govint commented Nov 17, 2016

Modified to get the VM name for attached disks from VM UUID.

...
snap-disk1 bigone Ubuntu-16-04 Wed Nov 9 12:43:47 2016 detached N/A 100.00MB 17.00MB ext4 read-write persistent
clonedisk bigone photon-custom-hw10-1.0-13c08b6 Fri Oct 28 07:12:04 2016 detached N/A 100.00MB 100.00MB ext4 read-write independent_persistent
try-attach bigone 1 Thu Nov 17 05:10:33 2016 detached N/A 100.00MB 15.00MB ext4 read-write independent_persistent

docker run --rm -it -w /data -v try-attach:/data busybox
/data #

...
snap-disk1 bigone Ubuntu-16-04 Wed Nov 9 12:43:47 2016 detached N/A 100.00MB 17.00MB ext4 read-write persistent
clonedisk bigone photon-custom-hw10-1.0-13c08b6 Fri Oct 28 07:12:04 2016 detached N/A 100.00MB 100.00MB ext4 read-write independent_persistent
try-attach bigone 1 Thu Nov 17 05:10:33 2016 1 N/A 100.00MB 15.00MB ext4 read-write independent_persistent

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

Copy link
Contributor

@msterin msterin left a comment

Choose a reason for hiding this comment

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

LGTM with 2 format nits (fix them or drop them)

def vm_uuid2name(vm_uuid):
vm = findVmByUuid(vm_uuid)
if vm:
return vm.config.name
Copy link
Contributor

Choose a reason for hiding this comment

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

it does not return anything if not vm. It's OK in python (will just return None) but it looks unpleasant.
Something like this reads easier IMO . at any rate, your call as the result is correct

if not vm or not vm.config:
  return None
return vm.config.name

vm_name = vmdk_ops.vm_uuid2name(metadata[kv.ATTACHED_VM_UUID])
if not vm_name:
return kv.DETACHED
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: 'else' is not needed IMO

@govint
Copy link
Contributor Author

govint commented Nov 17, 2016

Updated changes to address review comments. Tried running with Python 3.5 and there are exception and print statements that are incompatible. Fixed those also.

@msterin
Copy link
Contributor

msterin commented Nov 17, 2016

@govint - the actual fix looks good and ready to merge. The changes related to whitespaces and fixing except to comply with Py3 , while correct, do not belong in the same commit. And generally do not belong in this PR at all. They have a potential to introduce conflicts for PRs still in flight (for tenants), and they do not make product run on the Py3 (CI will still fail), nor do they clean up trailing whitespaces in the whole code base (there are still many)

For now, let's merge when/if CI passes , since it's a fairly small set of changes and the commit is already squashed, but please - do not mix different work in the same commit. Also, I am planning to do a pass on Py3/whitespaces work, so please do not do anymore fixes in this area to avoid conflicts without pinging me.

@govint
Copy link
Contributor Author

govint commented Nov 17, 2016

Sure, I couldn't run on 6.5 which I have here as a test bed and hence had
to fix these.

On Thu, Nov 17, 2016 at 11:57 PM, Mark Sterin notifications@github.com
wrote:

@govint https://github.com/govint - the actual fix looks good and ready
to merge. The changes related to whitespaces and fixing except to comply
with 6.5 , while correct, do not belong in the same commit. And generally
do not belong in this PR at all. They have a potential to introduce
conflicts for PRs still in flight (for tenants), and they do not make
product run on the latest ESX (CI will fail there), not do they clean up
trailing whitespaces in the whole code base (there are still many)

For now, let's merge (since it's a fairly small set of changes and the
commit is squashed) , but please - do not mix different work in the same
commit. Also, I am planning to do a pass on Py3/whitespaces work, so please
do not do anymore fixes in this area to avoid conflicts.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#745 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/APHseNBw_cYmgJBR-xunovp4Vg9vdr0Hks5q_Jx0gaJpZM4K049s
.

@kerneltime
Copy link
Contributor

Fixes #738

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants