Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #1173 #1368

Merged
merged 1 commit into from
Feb 7, 2019
Merged

Fix #1173 #1368

merged 1 commit into from
Feb 7, 2019

Conversation

amandahla
Copy link

I changed to get all VMs but it's a little bit slow because I'm retrieving the config.template property for each VM otherwise will count Templates as VM too.

Copy link
Member

@dougm dougm left a comment

Choose a reason for hiding this comment

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

Thanks @amandahla , using ContainerView would speed things up, do you want to try that? For an example, see:

// Create view of VirtualMachine objects
m := view.NewManager(c.Client)
v, err := m.CreateContainerView(ctx, c.ServiceContent.RootFolder, []string{"VirtualMachine"}, true)
if err != nil {
log.Fatal(err)
}
defer v.Destroy(ctx)
// Retrieve summary property for all machines
// Reference: http://pubs.vmware.com/vsphere-60/topic/com.vmware.wssdk.apiref.doc/vim.VirtualMachine.html
var vms []mo.VirtualMachine
err = v.Retrieve(ctx, []string{"VirtualMachine"}, []string{"summary"}, &vms)

@amandahla
Copy link
Author

@dougm Much faster, thanks! Do you think is necessary to change others too to not use finder?

@dougm
Copy link
Member

dougm commented Feb 6, 2019

@amandahla looking good, can you also remove "WIP" from the commit message?

There might be other places where ContainerView would be better. In general, ContainerView is best when you want all instances, Finder is better for specific/subset of instances. I think Finder could also be optimized to use ContainerView underneath in the * pattern case at least.

@amandahla
Copy link
Author

@dougm sure, done!

Thanks!

@amandahla amandahla changed the title [WIP] Fix #1173 Fix #1173 Feb 6, 2019
Copy link
Member

@dougm dougm left a comment

Choose a reason for hiding this comment

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

@amandahla sorry I just realized I had started a review with these 2 comments, but hadn't actually submitted the review with my commit message update request.

govc/datacenter/info.go Show resolved Hide resolved
govc/datacenter/info.go Outdated Show resolved Hide resolved
@amandahla
Copy link
Author

@dougm no problem, thanks for the comments.

Copy link
Member

@dougm dougm left a comment

Choose a reason for hiding this comment

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

Great, thanks again @amandahla

@dougm dougm merged commit a5f3a2f into vmware:master Feb 7, 2019
@dougm dougm added this to the 2019.03 milestone Feb 7, 2019
@amandahla
Copy link
Author

Glad to help! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants